From e9d77e54cda825deedb8f61501332439ffb877b9 Mon Sep 17 00:00:00 2001 From: shivam <91240327+shivamhwp@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:30:39 +0000 Subject: [PATCH 1/2] fix(server): PR watch waits once for workflow approval instead of flapping A fork PR whose workflows await maintainer approval woke the watching thread over and over, alternating "Checks failed (action-required)" with "All N checks passed": action-required counted as a failure, and the separate GitHub read that lists runs awaiting approval can answer empty, leaving only the bot checks to pass. action-required is now a wait: one "checks await approval" wake, and nothing counts as passed while any is awaited. A workflow run stays awaited, by its run URL, through reads that miss it until one of its jobs finishes; a head push clears it, and the awaited set survives restarts and re-watches. The provider flags GitHub's "could not tell" stand-in as a placeholder, which never confirms a pass but does not hide failures or pending checks beside it. A read that names no head keeps the head the watch knew. Watches saved before the new field that already reported these as failed are not told again. Fixes #24575 Co-Authored-By: maria-rcks Co-Authored-By: Claude Opus 5.5 --- .../src/orchestration-v2/Orchestrator.ts | 3 + .../PullRequestWatchReactor.ts | 1 + .../orchestration-v2/pullRequestWatch.test.ts | 74 ++++++++++++++++++ .../src/orchestration-v2/pullRequestWatch.ts | 78 +++++++++++++++---- .../src/orchestration-v2/runtimeLayer.test.ts | 39 ++++++---- docs/user/source-control.md | 2 +- packages/contracts/src/pullRequest.ts | 5 ++ packages/contracts/src/threadPullRequest.ts | 6 ++ .../server/GitHubPullRequestProvider.test.ts | 2 + .../src/server/GitHubPullRequestProvider.ts | 1 + 10 files changed, 181 insertions(+), 30 deletions(-) diff --git a/apps/server/src/orchestration-v2/Orchestrator.ts b/apps/server/src/orchestration-v2/Orchestrator.ts index f20ca8bdc92c1..74cd83d6ff0e1 100644 --- a/apps/server/src/orchestration-v2/Orchestrator.ts +++ b/apps/server/src/orchestration-v2/Orchestrator.ts @@ -3908,6 +3908,9 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio ...(existing.endedWatch?.owedChecks !== undefined && { owedChecks: existing.endedWatch.owedChecks, }), + ...(existing.endedWatch?.awaitingApproval !== undefined && { + awaitingApproval: existing.endedWatch.awaitingApproval, + }), ...(existing.endedWatch?.checksAwaitedSince !== undefined && { checksAwaitedSince: existing.endedWatch.checksAwaitedSince, }), diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index 2b588b1d2e23e..ca57d075aef18 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -175,6 +175,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc left.passed === right.passed && left.passedChecks.join("\n") === right.passedChecks.join("\n") && (left.owedChecks ?? []).join("\n") === (right.owedChecks ?? []).join("\n") && + (left.awaitingApproval ?? []).join("\n") === (right.awaitingApproval ?? []).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 6ab0191649490..8e0b94a8c81fb 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -31,6 +31,8 @@ const watch = (overrides: Partial = {}): ThreadPullReque ...overrides, }); +const run = (id: number) => `https://github.com/o/r/actions/runs/${id}`; + const check = (name: string, status: PullRequestCheck["status"]): PullRequestCheck => ({ name, status, @@ -79,6 +81,78 @@ const remark = ( const noRemarks: ReadonlyArray = []; describe("evaluatePullRequestWatch", () => { + // regression: #24575 fork workflows awaiting approval flapped between failed and passed + it("waits for workflow approval once, through reads that miss it, until the run finishes", () => { + const bot = check("bot", "success"); + const approval = { ...check("CI", "action-required"), url: run(1) }; + const gated = detail({ checks: [bot, approval] }); + const first = evaluatePullRequestWatch(watch(), gated, noRemarks); + assert.deepEqual(first.changes, [{ kind: "checks-awaiting-approval", checks: [approval] }]); + const message = pullRequestWatchMessage({ + number: 1, + url: "https://github.com/o/r/pull/1", + baseBranch: "main", + headSha: "aaaaaaaaaa", + report: first, + }); + assert.equal(message.notification.outcome, "updated"); + + // GitHub's approval read can answer empty while the rest of the checks read fine. + const missing = evaluatePullRequestWatch(first.next, detail({ checks: [bot] }), noRemarks); + assert.deepEqual(missing.changes, []); + assert.isFalse(missing.next.passed); + assert.deepEqual(evaluatePullRequestWatch(missing.next, gated, noRemarks).changes, []); + const pushed = detail({ headSha: "bbbbbbbbbb", checks: [bot] }); + assert.deepEqual(evaluatePullRequestWatch(missing.next, pushed, noRemarks).changes, [ + { kind: "checks-passed", count: 1, required: false }, + ]); + + // Approved, the run's jobs carry their own names under the run's URL. + const job = { ...check("lint", "success"), url: `${run(1)}/job/7` }; + const done = evaluatePullRequestWatch(missing.next, detail({ checks: [bot, job] }), noRemarks); + assert.deepEqual(done.changes, [{ kind: "checks-passed", count: 2, required: false }]); + }); + + it("does not pass or fail while the host cannot tell whether workflows await approval", () => { + const bot = check("bot", "success"); + const unknown = { + ...check("Workflow approval status", "action-required"), + url: null, + placeholder: true, + }; + const unread = evaluatePullRequestWatch(watch(), detail({ checks: [bot, unknown] }), noRemarks); + assert.deepEqual(unread.changes, []); + assert.isFalse(unread.next.passed); + const read = evaluatePullRequestWatch(unread.next, detail({ checks: [bot] }), noRemarks); + assert.deepEqual(read.changes, [{ kind: "checks-passed", count: 1, required: false }]); + for (const status of ["failure", "pending"] as const) { + const blocked = evaluatePullRequestWatch( + read.next, + detail({ checks: [{ ...bot, status }, unknown] }), + noRemarks, + ); + assert.isFalse(blocked.next.passed); + assert.deepEqual( + evaluatePullRequestWatch(blocked.next, detail({ checks: [bot] }), noRemarks).changes, + [{ kind: "checks-passed", count: 1, required: false }], + ); + } + }); + + it("still reports failures beside workflows awaiting approval", () => { + const approval = { ...check("CI", "action-required"), url: run(1) }; + const broken = check("lint", "failure"); + const report = evaluatePullRequestWatch( + watch(), + detail({ checks: [approval, broken] }), + noRemarks, + ); + assert.deepEqual(report.changes, [ + { kind: "checks-failed", failed: [broken] }, + { kind: "checks-awaiting-approval", checks: [approval] }, + ]); + }); + it("reports each failure at once, even while another check never finishes", () => { const bot = check("CodeRabbit", "pending"); const first = detail({ checks: [check("lint", "failure"), check("test", "pending"), bot] }); diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index f4c0000847236..7c8e56e5832bb 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -17,6 +17,7 @@ const SNIPPET_LENGTH = 200; export type PullRequestWatchChange = | { readonly kind: "checks-failed"; readonly failed: ReadonlyArray } | { readonly kind: "checks-passed"; readonly count: number; readonly required: boolean } + | { readonly kind: "checks-awaiting-approval"; readonly checks: ReadonlyArray } | { readonly kind: "remarks"; readonly remarks: ReadonlyArray } | { readonly kind: "no-checks"; readonly minutes: number } | { readonly kind: "conflicting" }; @@ -27,9 +28,12 @@ export interface PullRequestWatchReport { readonly exhausted: boolean; } -// action-required is a finished check needing human input. const isFailedCheck = (check: PullRequestCheck) => - check.status === "failure" || check.status === "cancelled" || check.status === "action-required"; + check.status === "failure" || check.status === "cancelled"; + +// A GitHub Actions job's URL extends its workflow run's URL; their names need not match. +const workflowRunOf = (url: string | null) => url?.match(/^.*\/actions\/runs\/\d+/)?.[0] ?? null; +const approvalKey = (check: PullRequestCheck) => workflowRunOf(check.url) ?? check.name; export function evaluatePullRequestWatch( watch: ThreadPullRequestWatch, @@ -50,15 +54,54 @@ export function evaluatePullRequestWatch( let passed = headMoved ? false : watch.passed; let passedChecks = headMoved ? [] : watch.passedChecks; let owedChecks = headMoved ? [] : (watch.owedChecks ?? []); - if (detail.checks.length > 0) { - const failed = detail.checks.filter(isFailedCheck); + let awaitingApproval = headMoved ? [] : (watch.awaitingApproval ?? []); + // "action-required" waits on someone, such as a maintainer approving a fork's workflows, so the + // agent is told once and nothing passes meanwhile. A placeholder is a host that could not tell, + // so it counts as no check and never confirms a pass, though a failed, pending, or awaited check + // beside it ends one. + const unread = detail.checks.some((check) => check.placeholder === true); + const checks = detail.checks.filter((check) => check.placeholder !== true); + const failed = checks.filter(isFailedCheck); + const awaiting = checks.filter((check) => check.status === "action-required"); + if (checks.length > 0) { + // A read beside a placeholder may have missed checks, so it lets go only of those it lists. + const missed = (name: string) => unread && !checks.some((check) => check.name === name); const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name)); if (newlyFailed.length > 0) changes.push({ kind: "checks-failed", failed: newlyFailed }); - failedChecks = failed.map((check) => check.name); + // A watch saved before awaitingApproval reported these as failed checks. + const toldBefore = + awaitingApproval.length > 0 || + (watch.awaitingApproval === undefined && + awaiting.every((check) => failedChecks.includes(check.name))); + if (awaiting.length > 0 && !toldBefore) { + changes.push({ kind: "checks-awaiting-approval", checks: awaiting }); + } + failedChecks = [...failedChecks.filter(missed), ...failed.map((check) => check.name)]; - 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)); + // GitHub lists runs awaiting approval in a separate read that can come back empty, so a run + // stays awaited through reads that miss it until one of its jobs finishes. One known only by + // name stays only while listed, or through a read beside a placeholder that does not list it. + const awaitingNow = [...new Set(awaiting.map(approvalKey))]; + const finished = new Set( + checks + .filter((check) => check.status !== "action-required" && check.status !== "pending") + .map((check) => workflowRunOf(check.url)), + ); + awaitingApproval = [ + ...awaitingNow, + ...awaitingApproval.filter( + (key) => + !awaitingNow.includes(key) && + (workflowRunOf(key) === key ? !finished.has(key) : missed(key)), + ), + ]; + } + const required = checks.filter((check) => check.required === true); + const gate = required.length > 0 ? required : checks; + const passedNow = + awaitingApproval.length === 0 && + gate.every((check) => check.status !== "pending" && !isFailedCheck(check)); + if (checks.length > 0 && !(unread && passedNow)) { const gateNames = gate.map((check) => check.name); const told = passed && passedChecks.length === 0 ? gateNames : passedChecks; // Required checks can finish between sweeps; detect new names. With no required checks, ignore advisory gate growth. @@ -67,23 +110,25 @@ export function evaluatePullRequestWatch( // Only those are owed, so later advisory churn never wakes. One turned required is gate news. const owed = passedNow && passed - ? detail.checks + ? checks .filter((check) => check.required !== true && owedChecks.includes(check.name)) .map((check) => check.name) : []; const settled = - failed.length === 0 && detail.checks.every((check) => check.status !== "pending"); + failed.length === 0 && + awaitingApproval.length === 0 && + checks.every((check) => check.status !== "pending"); owedChecks = owed; if (passedNow && (!passed || gateGrew)) { changes.push({ kind: "checks-passed", count: gate.length, required: required.length > 0 }); owedChecks = required.length > 0 - ? detail.checks + ? checks .filter((check) => check.required !== true && check.status === "pending") .map((check) => check.name) : []; } else if (settled && owed.length > 0) { - changes.push({ kind: "checks-passed", count: detail.checks.length, required: false }); + changes.push({ kind: "checks-passed", count: checks.length, required: false }); owedChecks = []; } passed = passedNow; @@ -99,7 +144,7 @@ export function evaluatePullRequestWatch( const awaitedBefore = headMoved ? DateTime.formatIso(DateTime.makeUnsafe(now)) : (watch.checksAwaitedSince ?? null); - checksAwaitedSince = detail.checks.length > 0 ? null : awaitedBefore; + checksAwaitedSince = checks.length > 0 ? null : awaitedBefore; const awaitedMs = checksAwaitedSince === null ? 0 : now - Date.parse(checksAwaitedSince); if (awaitedMs >= PULL_REQUEST_WATCH_NO_CHECKS_MS) { changes.push({ kind: "no-checks", minutes: Math.round(awaitedMs / 60_000) }); @@ -148,6 +193,7 @@ export function evaluatePullRequestWatch( passed, passedChecks, owedChecks, + awaitingApproval, remarksThrough, remarkIds, conflicting, @@ -190,6 +236,11 @@ function changeLines( ` - ${check.name}${check.status === "failure" ? "" : ` (${check.status})`}${check.url ? ` ${check.url}` : ""}`, ), ]; + case "checks-awaiting-approval": + return [ + `- Checks are waiting for approval or action from a maintainer${context.commit}:`, + ...listed(change.checks, (check) => ` - ${check.name}${check.url ? ` ${check.url}` : ""}`), + ]; case "checks-passed": return [ `- All ${change.count} ${change.required ? "required " : ""}${change.count === 1 ? "check" : "checks"} passed${context.commit}.`, @@ -216,6 +267,7 @@ function changeLines( const SUMMARY: Record = { "checks-failed": "checks failed", "checks-passed": "checks passed", + "checks-awaiting-approval": "checks await approval", remarks: "new comments", "no-checks": "no checks reported", conflicting: "merge conflict", diff --git a/apps/server/src/orchestration-v2/runtimeLayer.test.ts b/apps/server/src/orchestration-v2/runtimeLayer.test.ts index 796a50435ab60..2f37ff176fad3 100644 --- a/apps/server/src/orchestration-v2/runtimeLayer.test.ts +++ b/apps/server/src/orchestration-v2/runtimeLayer.test.ts @@ -3455,12 +3455,12 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { }), ); - it.effect("finishes owed checks once after pending sweeps, a restart, and a re-watch", () => + it.effect.each(["pending", "action-required"] as const)("recovers %s watches", (initialStatus) => Effect.gen(function* () { const orchestrator = yield* Orchestrator.OrchestratorV2; const worker = yield* EffectWorker.OrchestrationEffectWorkerV2; - const threadId = ThreadId.make("runtime-pull-request-watch-owed"); - const projectId = ProjectId.make("pr-watch-owed-project"); + const threadId = ThreadId.make(`runtime-pull-request-watch-owed-${initialStatus}`); + const projectId = ProjectId.make(`pr-watch-owed-project-${initialStatus}`); yield* seedProject({ projectId, title: "Watch owed", @@ -3472,7 +3472,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { type: "thread.create", createdBy: "user", creationSource: "web", - commandId: CommandId.make("pr-watch-owed-create"), + commandId: CommandId.make(`pr-watch-owed-create-${initialStatus}`), threadId, projectId, title: "Watch owed", @@ -3485,7 +3485,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { const watch = (on: boolean, id: string) => orchestrator.dispatch({ type: "thread.pull-request.watch", - commandId: CommandId.make(`pr-watch-owed-${id}`), + commandId: CommandId.make(`pr-watch-owed-${initialStatus}-${id}`), threadId, host: "github.com", repository: "pingdotgg/t3code", @@ -3495,7 +3495,10 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { }); yield* watch(true, "start"); - let remainingStatus: "pending" | "success" = "pending"; + let remainingStatus: "pending" | "action-required" | "success" = initialStatus; + const approval = initialStatus === "action-required"; + const firstSummary = approval ? "#12: checks await approval" : "#12: checks passed"; + const runUrl = "https://github.com/pingdotgg/t3code/actions/runs/123"; const host = Layer.mergeAll( NodeServices.layer, NodeCrypto.layer, @@ -3512,7 +3515,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { name, status: name === "test" ? remainingStatus : ("success" as const), description: null, - url: null, + url: name === "test" ? `${runUrl}/job/7` : null, required: name === "lint", })), })), @@ -3537,38 +3540,42 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { yield* TestClock.adjust("10 minutes"); yield* reactor.sweep; yield* worker.drain(); - assert.deepEqual(yield* summaries, ["#12: checks passed"]); + assert.deepEqual(yield* summaries, [firstSummary]); assert.deepEqual( - (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.owedChecks, - ["test"], + (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.[ + approval ? "awaitingApproval" : "owedChecks" + ], + approval ? [runUrl] : ["test"], ); // The fresh reactor reloads the nonempty obligation from the persisted watch. const restarted = yield* PullRequestWatchReactor.make.pipe(Effect.provide(host)); yield* restarted.sweep; yield* worker.drain(); - assert.deepEqual(yield* summaries, ["#12: checks passed"]); + assert.deepEqual(yield* summaries, [firstSummary]); // A new watch on the same pull request knows what the last one told. yield* watch(false, "stop"); yield* watch(true, "again"); yield* restarted.sweep; yield* worker.drain(); - assert.deepEqual(yield* summaries, ["#12: checks passed"]); + assert.deepEqual(yield* summaries, [firstSummary]); assert.deepEqual( - (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.owedChecks, - ["test"], + (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.[ + approval ? "awaitingApproval" : "owedChecks" + ], + approval ? [runUrl] : ["test"], ); remainingStatus = "success"; yield* TestClock.adjust("10 minutes"); yield* restarted.sweep; yield* worker.drain(); - assert.deepEqual(yield* summaries, ["#12: checks passed", "#12: checks passed"]); + assert.deepEqual(yield* summaries, [firstSummary, "#12: checks passed"]); yield* TestClock.adjust("10 minutes"); yield* restarted.sweep; yield* worker.drain(); - assert.deepEqual(yield* summaries, ["#12: checks passed", "#12: checks passed"]); + assert.deepEqual(yield* summaries, [firstSummary, "#12: checks passed"]); assert.deepEqual( (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.owedChecks, [], diff --git a/docs/user/source-control.md b/docs/user/source-control.md index 9eb2751f9315f..04e3c54463536 100644 --- a/docs/user/source-control.md +++ b/docs/user/source-control.md @@ -296,7 +296,7 @@ review is terminal. An open or unsynced link keeps it active. Ask the agent to watch, monitor, or babysit a pull request and it calls `watch_pull_request`. While the thread is active, the server checks the pull request every two minutes and wakes the agent when a -check fails, the required checks pass, someone else comments or reviews, or the branch starts to +check fails or waits for a maintainer's approval, the required checks pass, someone else comments or reviews, or the branch starts to conflict. Threads in a project that watch the same pull request share one check. On GitHub, a check first asks whether anything changed and reads the pull request only when it did, which keeps watching inside GitHub's rate limit. Comments from your own account do not wake it. Watching ends diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 64111ab4742b1..f2d30e9f1ff1e 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -109,6 +109,11 @@ export const PullRequestCheck = Schema.Struct({ description: Schema.NullOr(Schema.String), url: Schema.NullOr(Schema.String), required: Schema.optional(Schema.Boolean), + /** + * Not a check: it stands for ones the host could not list, such as workflow runs awaiting + * approval when that read fails, so nothing waits on it or counts it as passing. + */ + placeholder: Schema.optional(Schema.Boolean), }); export type PullRequestCheck = typeof PullRequestCheck.Type; diff --git a/packages/contracts/src/threadPullRequest.ts b/packages/contracts/src/threadPullRequest.ts index 3daee9ce197e2..c088a72fd42c2 100644 --- a/packages/contracts/src/threadPullRequest.ts +++ b/packages/contracts/src/threadPullRequest.ts @@ -89,6 +89,12 @@ export const ThreadPullRequestWatch = Schema.Struct({ ), /** Checks still pending when required ones passed early, owed one wake once all settle. */ owedChecks: Schema.optional(Schema.Array(TrimmedNonEmptyString)), + /** + * Checks waiting for someone to approve or act on them, the agent told once: a workflow run's + * URL, kept through reads that miss it until one of its jobs finishes, or else a check's name, + * kept while a read lists it or could not list everything. + */ + awaitingApproval: Schema.optional(Schema.Array(TrimmedNonEmptyString)), remarksThrough: IsoDateTime, /** Reported remarks at remarksThrough, so late arrivals at that timestamp still count. */ remarkIds: Schema.Array(TrimmedNonEmptyString), diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts index 48f3021d1fe38..d732ed214677b 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts @@ -259,6 +259,7 @@ describe("gitHubViewerPermissions", () => { status: "action-required", description: "GitHub could not determine whether workflows are awaiting approval.", url: null, + placeholder: true, }); for (const fingerprint of ["broad", "restricted", "broad"]) { const scoped = yield* provider @@ -519,6 +520,7 @@ it.effect("keeps an unsafe workflow approval scope visible as unknown", () => status: "action-required", description: "GitHub could not determine whether workflows are awaiting approval.", url: null, + placeholder: true, }, ]); }).pipe( diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts index b998d2be735a6..f90c45a6771cf 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts @@ -187,6 +187,7 @@ function withWorkflowApprovals( status: "action-required" as const, description: "GitHub could not determine whether workflows are awaiting approval.", url: null, + placeholder: true, }, ] : []), From 76b8cd38257da5f759adf5fce396d521316bcb82 Mon Sep 17 00:00:00 2001 From: shivam <91240327+shivamhwp@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:11:07 +0000 Subject: [PATCH 2/2] fix(server): retain approval uncertainty across quiet watch reads --- .../PullRequestWatchReactor.ts | 1 + .../orchestration-v2/pullRequestWatch.test.ts | 10 ++++++ .../src/orchestration-v2/pullRequestWatch.ts | 2 +- .../src/orchestration-v2/runtimeLayer.test.ts | 36 ++++++++++++++++++- 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index ca57d075aef18..5bcc8580b8366 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -175,6 +175,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc left.passed === right.passed && left.passedChecks.join("\n") === right.passedChecks.join("\n") && (left.owedChecks ?? []).join("\n") === (right.owedChecks ?? []).join("\n") && + (left.awaitingApproval === undefined) === (right.awaitingApproval === undefined) && (left.awaitingApproval ?? []).join("\n") === (right.awaitingApproval ?? []).join("\n") && left.remarksThrough === right.remarksThrough && left.remarkIds.join("\n") === right.remarkIds.join("\n") && diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index 8e0b94a8c81fb..ad369b7094fcb 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -503,6 +503,16 @@ describe("evaluatePullRequestWatch", () => { assert.isTrue(anHourLater(legacy, detail({ checks: [] })).every((kinds) => kinds.length === 0)); const pushed = anHourLater(legacy, detail({ headSha: "bbbbbbbbbb", checks: [] })); assert.deepEqual(pushed.flat(), ["no-checks"]); + const unknown = detail({ + checks: [{ ...check("Workflow approval status", "action-required"), placeholder: true }], + }); + assert.deepEqual(anHourLater(watch(), unknown).flat(), []); + assert.deepEqual(anHourLater(legacy, { ...unknown, headSha: "bbbbbbbbbb" }).flat(), []); + const unread = evaluate(watch(), unknown, noRemarks, 0); + assert.deepEqual( + evaluate(unread.next, detail({ checks: [] }), noRemarks, 60 * 60_000).changes, + [{ kind: "no-checks", minutes: 60 }], + ); }); it("never reports missing checks on a host where an empty list says nothing", () => { diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index 7c8e56e5832bb..1f095d9a5f571 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -146,7 +146,7 @@ export function evaluatePullRequestWatch( : (watch.checksAwaitedSince ?? null); checksAwaitedSince = checks.length > 0 ? null : awaitedBefore; const awaitedMs = checksAwaitedSince === null ? 0 : now - Date.parse(checksAwaitedSince); - if (awaitedMs >= PULL_REQUEST_WATCH_NO_CHECKS_MS) { + if (!unread && awaitedMs >= PULL_REQUEST_WATCH_NO_CHECKS_MS) { changes.push({ kind: "no-checks", minutes: Math.round(awaitedMs / 60_000) }); checksAwaitedSince = null; } diff --git a/apps/server/src/orchestration-v2/runtimeLayer.test.ts b/apps/server/src/orchestration-v2/runtimeLayer.test.ts index 2f37ff176fad3..8003bbcadae40 100644 --- a/apps/server/src/orchestration-v2/runtimeLayer.test.ts +++ b/apps/server/src/orchestration-v2/runtimeLayer.test.ts @@ -3495,7 +3495,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { }); yield* watch(true, "start"); - let remainingStatus: "pending" | "action-required" | "success" = initialStatus; + let remainingStatus: "pending" | "action-required" | "success" | "failure" = initialStatus; const approval = initialStatus === "action-required"; const firstSummary = approval ? "#12: checks await approval" : "#12: checks passed"; const runUrl = "https://github.com/pingdotgg/t3code/actions/runs/123"; @@ -3535,6 +3535,40 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { ); const reactor = yield* PullRequestWatchReactor.make.pipe(Effect.provide(host)); + if (approval) { + const started = (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch; + assert.isDefined(started); + if (started === undefined) return; + yield* orchestrator.dispatch({ + type: "thread.pull-request-watch.sync", + commandId: CommandId.make("pr-watch-owed-legacy"), + threadId, + host: "github.com", + repository: "pingdotgg/t3code", + number: 12, + startedAt: started.startedAt, + watch: { + ...started, + headSha: "ad70160aaa", + failedChecks: ["test"], + passed: true, + passedChecks: ["lint"], + owedChecks: [], + checksAwaitedSince: null, + }, + }); + remainingStatus = "failure"; + yield* reactor.sweep; + yield* worker.drain(); + assert.deepEqual(yield* summaries, []); + assert.deepEqual( + (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch + ?.awaitingApproval, + [], + ); + remainingStatus = initialStatus; + yield* TestClock.adjust("10 minutes"); + } yield* reactor.sweep; yield* worker.drain(); yield* TestClock.adjust("10 minutes");