authorgravatar for snorteman@gmail.comScott Norteman <snorteman@gmail.com> 2026-10-05 17:37:29-04:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-05 14:37:29-07:00
log404e229f22ef2dc7e42074d5379d5c554dd2c083
tree85dc9a9f7183150622c72d45b350777c27b9bc2c
parent5cebd3b814ae171df83fb81db3036fbbca2a660e
signature Signed by PGP key B5690EEEBB952194

fix(server): PR watch reports a required check that first appears already passed (#15804)

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

7 files changed, 105 insertions(+), 1 deletions(-)

apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts+1
...@@ -252,6 +252,7 @@ describe("pull request toolkit handlers", () => {...@@ -252,6 +252,7 @@ describe("pull request toolkit handlers", () => {
252 headSha: null,252 headSha: null,
253 failedChecks: [],253 failedChecks: [],
254 passed: false,254 passed: false,
255 passedChecks: [],
255 remarksThrough: "2026-08-20T00:00:00.000Z",256 remarksThrough: "2026-08-20T00:00:00.000Z",
256 remarkIds: [],257 remarkIds: [],
257 conflicting: false,258 conflicting: false,
apps/server/src/orchestration-v2/Orchestrator.ts+1
...@@ -3040,6 +3040,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio...@@ -3040,6 +3040,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio
3040 headSha: null,3040 headSha: null,
3041 failedChecks: [],3041 failedChecks: [],
3042 passed: false,3042 passed: false,
3043 passedChecks: [],
3043 remarksThrough: startedAt,3044 remarksThrough: startedAt,
3044 remarkIds: [],3045 remarkIds: [],
3045 conflicting: false,3046 conflicting: false,
apps/server/src/orchestration-v2/PullRequestWatchReactor.ts+1
...@@ -54,6 +54,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc...@@ -54,6 +54,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc
54 left.headSha === right.headSha &&54 left.headSha === right.headSha &&
55 left.failedChecks.join("\n") === right.failedChecks.join("\n") &&55 left.failedChecks.join("\n") === right.failedChecks.join("\n") &&
56 left.passed === right.passed &&56 left.passed === right.passed &&
57 left.passedChecks.join("\n") === right.passedChecks.join("\n") &&
57 left.remarksThrough === right.remarksThrough &&58 left.remarksThrough === right.remarksThrough &&
58 left.remarkIds.join("\n") === right.remarkIds.join("\n") &&59 left.remarkIds.join("\n") === right.remarkIds.join("\n") &&
59 left.conflicting === right.conflicting &&60 left.conflicting === right.conflicting &&
apps/server/src/orchestration-v2/pullRequestWatch.test.ts+56
...@@ -18,6 +18,7 @@ const watch = (overrides: Partial<ThreadPullRequestWatch> = {}): ThreadPullReque...@@ -18,6 +18,7 @@ const watch = (overrides: Partial<ThreadPullRequestWatch> = {}): ThreadPullReque
18 headSha: null,18 headSha: null,
19 failedChecks: [],19 failedChecks: [],
20 passed: false,20 passed: false,
21 passedChecks: [],
21 remarksThrough: STARTED,22 remarksThrough: STARTED,
22 remarkIds: [],23 remarkIds: [],
23 conflicting: false,24 conflicting: false,
...@@ -99,6 +100,61 @@ describe("evaluatePullRequestWatch", () => {...@@ -99,6 +100,61 @@ describe("evaluatePullRequestWatch", () => {
99 assert.deepEqual(evaluatePullRequestWatch(watch(), plain, noRemarks).changes, []);100 assert.deepEqual(evaluatePullRequestWatch(watch(), plain, noRemarks).changes, []);
100 });101 });
101102
103 it("reports passed again when a required check shows up already passed", () => {
104 const required = (name: string, status: PullRequestCheck["status"]) => ({
105 ...check(name, status),
106 required: true,
107 });
108 const tests = detail({
109 checks: [required("Tests", "success"), check("Smoke Tests", "pending")],
110 });
111 const first = evaluatePullRequestWatch(watch(), tests, noRemarks);
112 assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 1, required: true }]);
113
114 // The gate job was created and finished between two passes, so it was never seen pending.
115 const gated = detail({
116 checks: [
117 required("Tests", "success"),
118 check("Smoke Tests", "success"),
119 required("Smoke Tests Gate", "success"),
120 ],
121 });
122 const second = evaluatePullRequestWatch(first.next, gated, noRemarks);
123 assert.deepEqual(second.changes, [{ kind: "checks-passed", count: 2, required: true }]);
124 assert.deepEqual(evaluatePullRequestWatch(second.next, gated, noRemarks).changes, []);
125
126 // Seen pending first, the gate is reported once it passes, as before.
127 const pending = detail({
128 checks: [required("Tests", "success"), required("Smoke Tests Gate", "pending")],
129 });
130 const waiting = evaluatePullRequestWatch(first.next, pending, noRemarks);
131 assert.deepEqual(waiting.changes, []);
132 assert.deepEqual(evaluatePullRequestWatch(waiting.next, gated, noRemarks).changes, [
133 { kind: "checks-passed", count: 2, required: true },
134 ]);
135 });
136
137 it("reports passed again when a check shows up already passed where none is required", () => {
138 const first = evaluatePullRequestWatch(
139 watch(),
140 detail({ checks: [check("test", "success")] }),
141 noRemarks,
142 );
143 assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 1, required: false }]);
144 const both = detail({ checks: [check("test", "success"), check("lint", "success")] });
145 assert.deepEqual(evaluatePullRequestWatch(first.next, both, noRemarks).changes, [
146 { kind: "checks-passed", count: 2, required: false },
147 ]);
148 });
149
150 it("does not wake a watch saved before passed checks were recorded", () => {
151 const green = detail({ checks: [{ ...check("test", "success"), required: true }] });
152 const told = watch({ headSha: "aaaaaaaaaa", passed: true });
153 const saved = evaluatePullRequestWatch(told, green, noRemarks);
154 assert.deepEqual(saved.changes, []);
155 assert.deepEqual(saved.next.passedChecks, ["test"]);
156 });
157
102 it("keeps remarks for a later pass when the conversation was not read whole", () => {158 it("keeps remarks for a later pass when the conversation was not read whole", () => {
103 const comments = [remark("reviewer", "2026-10-02T12:06:00Z")];159 const comments = [remark("reviewer", "2026-10-02T12:06:00Z")];
104 const partial = evaluatePullRequestWatch(watch(), detail(), null);160 const partial = evaluatePullRequestWatch(watch(), detail(), null);
apps/server/src/orchestration-v2/pullRequestWatch.ts+9-1
...@@ -53,6 +53,7 @@ export function evaluatePullRequestWatch(...@@ -53,6 +53,7 @@ export function evaluatePullRequestWatch(
53 // An empty list keeps the last state: a host can answer with one when its check read fails.53 // An empty list keeps the last state: a host can answer with one when its check read fails.
54 let failedChecks = headMoved ? [] : watch.failedChecks;54 let failedChecks = headMoved ? [] : watch.failedChecks;
55 let passed = headMoved ? false : watch.passed;55 let passed = headMoved ? false : watch.passed;
56 let passedChecks = headMoved ? [] : watch.passedChecks;
56 if (detail.checks.length > 0) {57 if (detail.checks.length > 0) {
57 const failed = detail.checks.filter(isFailedCheck);58 const failed = detail.checks.filter(isFailedCheck);
58 const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name));59 const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name));
...@@ -63,10 +64,16 @@ export function evaluatePullRequestWatch(...@@ -63,10 +64,16 @@ export function evaluatePullRequestWatch(
63 const required = detail.checks.filter((check) => check.required === true);64 const required = detail.checks.filter((check) => check.required === true);
64 const gate = required.length > 0 ? required : detail.checks;65 const gate = required.length > 0 ? required : detail.checks;
65 const passedNow = gate.every((check) => check.status !== "pending" && !isFailedCheck(check));66 const passedNow = gate.every((check) => check.status !== "pending" && !isFailedCheck(check));
66 if (passedNow && !passed) {67 const gateNames = gate.map((check) => check.name);
68 // A watch saved before passedChecks existed takes the current names, so it does not wake.
69 const told = passed && passedChecks.length === 0 ? gateNames : passedChecks;
70 // A required job created and finished between two passes is never seen pending.
71 const gateGrew = gateNames.some((name) => !told.includes(name));
72 if (passedNow && (!passed || gateGrew)) {
67 changes.push({ kind: "checks-passed", count: gate.length, required: required.length > 0 });73 changes.push({ kind: "checks-passed", count: gate.length, required: required.length > 0 });
68 }74 }
69 passed = passedNow;75 passed = passedNow;
76 passedChecks = passedNow ? gateNames : [];
70 }77 }
7178
72 const own = (detail.viewer ?? detail.author?.login)?.toLowerCase();79 const own = (detail.viewer ?? detail.author?.login)?.toLowerCase();
...@@ -107,6 +114,7 @@ export function evaluatePullRequestWatch(...@@ -107,6 +114,7 @@ export function evaluatePullRequestWatch(
107 headSha,114 headSha,
108 failedChecks,115 failedChecks,
109 passed,116 passed,
117 passedChecks,
110 remarksThrough,118 remarksThrough,
111 remarkIds,119 remarkIds,
112 conflicting,120 conflicting,
packages/contracts/src/threadPullRequest.test.ts+29
...@@ -54,3 +54,32 @@ it.effect("decodes thread pull request links with snapshot and stack", () =>...@@ -54,3 +54,32 @@ it.effect("decodes thread pull request links with snapshot and stack", () =>
54 assert.strictEqual(links[1]?.snapshot?.state, "open");54 assert.strictEqual(links[1]?.snapshot?.state, "open");
55 }),55 }),
56);56);
57
58it.effect("decodes a watch saved before passed checks were recorded", () =>
59 Effect.gen(function* () {
60 const [link] = yield* decodeLinks([
61 {
62 host: "github.com",
63 repository: "pingdotgg/t3code",
64 number: 42,
65 url: "https://github.com/pingdotgg/t3code/pull/42",
66 source: "agent",
67 linkedAt: "2026-01-01T00:00:00.000Z",
68 snapshot: null,
69 stack: null,
70 watch: {
71 startedAt: "2026-01-01T00:00:00.000Z",
72 headSha: "abc123",
73 failedChecks: [],
74 passed: true,
75 remarksThrough: "2026-01-01T00:00:00.000Z",
76 remarkIds: [],
77 conflicting: false,
78 wakes: 0,
79 },
80 },
81 ]);
82
83 assert.deepStrictEqual(link?.watch?.passedChecks, []);
84 }),
85);
packages/contracts/src/threadPullRequest.ts+8
...@@ -1,3 +1,4 @@...@@ -1,3 +1,4 @@
1import * as Effect from "effect/Effect";
1import * as Schema from "effect/Schema";2import * as Schema from "effect/Schema";
23
3import {4import {
...@@ -106,6 +107,13 @@ export const ThreadPullRequestWatch = Schema.Struct({...@@ -106,6 +107,13 @@ export const ThreadPullRequestWatch = Schema.Struct({
106 failedChecks: Schema.Array(TrimmedNonEmptyString),107 failedChecks: Schema.Array(TrimmedNonEmptyString),
107 /** The agent was told the required checks on that commit passed. */108 /** The agent was told the required checks on that commit passed. */
108 passed: Schema.Boolean,109 passed: Schema.Boolean,
110 /**
111 * Names in the passing gate on that commit, so a required check that first shows up already
112 * passed is news. Empty on watches saved before this field existed.
113 */
114 passedChecks: Schema.Array(TrimmedNonEmptyString).pipe(
115 Schema.withDecodingDefault(Effect.succeed([])),
116 ),
109 /** Remarks from others created up to this host time were reported. */117 /** Remarks from others created up to this host time were reported. */
110 remarksThrough: IsoDateTime,118 remarksThrough: IsoDateTime,
111 /** Remarks created exactly at `remarksThrough` that were reported, so a late one still counts. */119 /** Remarks created exactly at `remarksThrough` that were reported, so a late one still counts. */