From 404e229f22ef2dc7e42074d5379d5c554dd2c083 Mon Sep 17 00:00:00 2001 From: Scott Norteman Date: Mon, 5 Oct 2026 17:37:29 -0400 Subject: [PATCH] fix(server): PR watch reports a required check that first appears already passed (#15804) Co-authored-by: Claude Opus 5.5 (1M context) --- .../toolkits/pullRequests/handlers.test.ts | 1 + .../src/orchestration-v2/Orchestrator.ts | 1 + .../PullRequestWatchReactor.ts | 1 + .../orchestration-v2/pullRequestWatch.test.ts | 56 +++++++++++++++++++ .../src/orchestration-v2/pullRequestWatch.ts | 10 +++- .../contracts/src/threadPullRequest.test.ts | 29 ++++++++++ packages/contracts/src/threadPullRequest.ts | 8 +++ 7 files changed, 105 insertions(+), 1 deletion(-) diff --git a/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts b/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts index 3709abf67f54d73616c291573c5c30ae0ab5cfec..d940916f904c5c1ff2e278bd6a16a88ce306fbbc 100644 --- a/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts +++ b/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts @@ -252,6 +252,7 @@ describe("pull request toolkit handlers", () => { headSha: null, failedChecks: [], passed: false, + passedChecks: [], remarksThrough: "2026-08-20T00:00:00.000Z", remarkIds: [], conflicting: false, diff --git a/apps/server/src/orchestration-v2/Orchestrator.ts b/apps/server/src/orchestration-v2/Orchestrator.ts index 98f547284b575354f18c1c0f41fdf00080f516f5..01c5057d464c9bb4312ea59c33298e327c6098c2 100644 --- a/apps/server/src/orchestration-v2/Orchestrator.ts +++ b/apps/server/src/orchestration-v2/Orchestrator.ts @@ -3040,6 +3040,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio headSha: null, failedChecks: [], passed: false, + passedChecks: [], remarksThrough: startedAt, remarkIds: [], conflicting: false, diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index f668bbd3f10d2b688fc7c5847fed31240a81a49e..2d118fb0a493cc6048b28150de22c59651d8cb8e 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -54,6 +54,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc left.headSha === right.headSha && left.failedChecks.join("\n") === right.failedChecks.join("\n") && left.passed === right.passed && + left.passedChecks.join("\n") === right.passedChecks.join("\n") && left.remarksThrough === right.remarksThrough && left.remarkIds.join("\n") === right.remarkIds.join("\n") && left.conflicting === right.conflicting && diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index 05c1013b1c5feec45108b77fd8e0680a2682388a..f69f2c7b4206a7f78f6f540fc05885a43c0a0687 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -18,6 +18,7 @@ const watch = (overrides: Partial = {}): ThreadPullReque headSha: null, failedChecks: [], passed: false, + passedChecks: [], remarksThrough: STARTED, remarkIds: [], conflicting: false, @@ -99,6 +100,61 @@ describe("evaluatePullRequestWatch", () => { assert.deepEqual(evaluatePullRequestWatch(watch(), plain, noRemarks).changes, []); }); + it("reports passed again when a required check shows up already passed", () => { + const required = (name: string, status: PullRequestCheck["status"]) => ({ + ...check(name, status), + required: true, + }); + const tests = detail({ + checks: [required("Tests", "success"), check("Smoke Tests", "pending")], + }); + const first = evaluatePullRequestWatch(watch(), tests, noRemarks); + assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 1, required: true }]); + + // The gate job was created and finished between two passes, so it was never seen pending. + const gated = detail({ + checks: [ + required("Tests", "success"), + check("Smoke Tests", "success"), + required("Smoke Tests Gate", "success"), + ], + }); + const second = evaluatePullRequestWatch(first.next, gated, noRemarks); + assert.deepEqual(second.changes, [{ kind: "checks-passed", count: 2, required: true }]); + assert.deepEqual(evaluatePullRequestWatch(second.next, gated, noRemarks).changes, []); + + // Seen pending first, the gate is reported once it passes, as before. + const pending = detail({ + checks: [required("Tests", "success"), required("Smoke Tests Gate", "pending")], + }); + const waiting = evaluatePullRequestWatch(first.next, pending, noRemarks); + assert.deepEqual(waiting.changes, []); + assert.deepEqual(evaluatePullRequestWatch(waiting.next, gated, noRemarks).changes, [ + { kind: "checks-passed", count: 2, required: true }, + ]); + }); + + it("reports passed again when a check shows up already passed where none is required", () => { + const first = evaluatePullRequestWatch( + watch(), + detail({ checks: [check("test", "success")] }), + noRemarks, + ); + assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 1, required: false }]); + const both = detail({ checks: [check("test", "success"), check("lint", "success")] }); + assert.deepEqual(evaluatePullRequestWatch(first.next, both, noRemarks).changes, [ + { kind: "checks-passed", count: 2, required: false }, + ]); + }); + + it("does not wake a watch saved before passed checks were recorded", () => { + const green = detail({ checks: [{ ...check("test", "success"), required: true }] }); + const told = watch({ headSha: "aaaaaaaaaa", passed: true }); + const saved = evaluatePullRequestWatch(told, green, noRemarks); + assert.deepEqual(saved.changes, []); + assert.deepEqual(saved.next.passedChecks, ["test"]); + }); + it("keeps remarks for a later pass when the conversation was not read whole", () => { const comments = [remark("reviewer", "2026-10-02T12:06:00Z")]; const partial = evaluatePullRequestWatch(watch(), detail(), null); diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index 4a0c2ec3197a93b32195e61a7e3efd1011cf1ae7..4f2d8c82c6c1303a13078cf89b2737273c76c00b 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -53,6 +53,7 @@ export function evaluatePullRequestWatch( // An empty list keeps the last state: a host can answer with one when its check read fails. let failedChecks = headMoved ? [] : watch.failedChecks; let passed = headMoved ? false : watch.passed; + let passedChecks = headMoved ? [] : watch.passedChecks; if (detail.checks.length > 0) { const failed = detail.checks.filter(isFailedCheck); const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name)); @@ -63,10 +64,16 @@ export function evaluatePullRequestWatch( const required = detail.checks.filter((check) => check.required === true); const gate = required.length > 0 ? required : detail.checks; const passedNow = gate.every((check) => check.status !== "pending" && !isFailedCheck(check)); - if (passedNow && !passed) { + const gateNames = gate.map((check) => check.name); + // A watch saved before passedChecks existed takes the current names, so it does not wake. + const told = passed && passedChecks.length === 0 ? gateNames : passedChecks; + // A required job created and finished between two passes is never seen pending. + const gateGrew = gateNames.some((name) => !told.includes(name)); + if (passedNow && (!passed || gateGrew)) { changes.push({ kind: "checks-passed", count: gate.length, required: required.length > 0 }); } passed = passedNow; + passedChecks = passedNow ? gateNames : []; } const own = (detail.viewer ?? detail.author?.login)?.toLowerCase(); @@ -107,6 +114,7 @@ export function evaluatePullRequestWatch( headSha, failedChecks, passed, + passedChecks, remarksThrough, remarkIds, conflicting, diff --git a/packages/contracts/src/threadPullRequest.test.ts b/packages/contracts/src/threadPullRequest.test.ts index 456761c4ebc841f6cc4520f579732540fd308da7..108902bc59a9ad7b2f781645b92456850adb9afd 100644 --- a/packages/contracts/src/threadPullRequest.test.ts +++ b/packages/contracts/src/threadPullRequest.test.ts @@ -54,3 +54,32 @@ it.effect("decodes thread pull request links with snapshot and stack", () => assert.strictEqual(links[1]?.snapshot?.state, "open"); }), ); + +it.effect("decodes a watch saved before passed checks were recorded", () => + Effect.gen(function* () { + const [link] = yield* decodeLinks([ + { + host: "github.com", + repository: "pingdotgg/t3code", + number: 42, + url: "https://github.com/pingdotgg/t3code/pull/42", + source: "agent", + linkedAt: "2026-01-01T00:00:00.000Z", + snapshot: null, + stack: null, + watch: { + startedAt: "2026-01-01T00:00:00.000Z", + headSha: "abc123", + failedChecks: [], + passed: true, + remarksThrough: "2026-01-01T00:00:00.000Z", + remarkIds: [], + conflicting: false, + wakes: 0, + }, + }, + ]); + + assert.deepStrictEqual(link?.watch?.passedChecks, []); + }), +); diff --git a/packages/contracts/src/threadPullRequest.ts b/packages/contracts/src/threadPullRequest.ts index 9df5cbe9670a919f583288e38c1af5c25c7cbb0e..2dfb532c7f2ffaeb033aa547755d5b017d8ca461 100644 --- a/packages/contracts/src/threadPullRequest.ts +++ b/packages/contracts/src/threadPullRequest.ts @@ -1,3 +1,4 @@ +import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; import { @@ -106,6 +107,13 @@ export const ThreadPullRequestWatch = Schema.Struct({ failedChecks: Schema.Array(TrimmedNonEmptyString), /** The agent was told the required checks on that commit passed. */ passed: Schema.Boolean, + /** + * Names in the passing gate on that commit, so a required check that first shows up already + * passed is news. Empty on watches saved before this field existed. + */ + passedChecks: Schema.Array(TrimmedNonEmptyString).pipe( + Schema.withDecodingDefault(Effect.succeed([])), + ), /** Remarks from others created up to this host time were reported. */ remarksThrough: IsoDateTime, /** Remarks created exactly at `remarksThrough` that were reported, so a late one still counts. */ -- 2.54.0