diff --git a/apps/server/src/orchestration-v2/Orchestrator.ts b/apps/server/src/orchestration-v2/Orchestrator.ts index 61929131e9d7..87b718c6901b 100644 --- a/apps/server/src/orchestration-v2/Orchestrator.ts +++ b/apps/server/src/orchestration-v2/Orchestrator.ts @@ -5070,6 +5070,9 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio ...(existing.endedWatch && { headSha: existing.endedWatch.headSha, failedChecks: existing.endedWatch.failedChecks, + ...(existing.endedWatch.reportedFailures !== undefined && { + reportedFailures: existing.endedWatch.reportedFailures, + }), passed: existing.endedWatch.passed, passedChecks: existing.endedWatch.passedChecks, conflicting: existing.endedWatch.conflicting, diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index 5b69a7efa337..ffa62c3c0c4e 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -205,6 +205,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc left.startedAt === right.startedAt && left.headSha === right.headSha && left.failedChecks.join("\n") === right.failedChecks.join("\n") && + (left.reportedFailures ?? []).join("\n") === (right.reportedFailures ?? []).join("\n") && left.passed === right.passed && left.passedChecks.join("\n") === right.passedChecks.join("\n") && (left.owedChecks ?? []).join("\n") === (right.owedChecks ?? []).join("\n") && diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index 7275fd845b37..01da7ce15f80 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -478,6 +478,47 @@ describe("evaluatePullRequestWatch", () => { { kind: "checks-failed", failed: [check("lint", "failure")] }, { kind: "checks-awaiting-approval", checks: [approval] }, ]); + + // A job URL identifies its run and immutable job attempt. A transient missing + // or pending row must not make the same consumed failure news again. + const job = (runId: number, jobId: number) => ({ + ...check("lint", "failure"), + url: `${run(runId)}/job/${jobId}`, + }); + const seen = evaluatePullRequestWatch( + watch(), + detail({ checks: [job(10, 20), bot] }), + noRemarks, + ); + const pending = evaluatePullRequestWatch( + seen.next, + detail({ checks: [check("lint", "pending"), bot] }), + noRemarks, + ); + assert.deepEqual( + evaluatePullRequestWatch(pending.next, detail({ checks: [job(10, 20), bot] }), noRemarks) + .changes, + [], + ); + assert.deepEqual( + evaluatePullRequestWatch( + seen.next, + detail({ + checks: [{ ...job(10, 20), url: `${run(10)}/job/20?check_suite_focus=true#step:1` }, bot], + }), + noRemarks, + ).changes, + [], + ); + for (const fresh of [job(10, 21), job(11, 22)]) { + const next = evaluatePullRequestWatch(seen.next, detail({ checks: [fresh, bot] }), noRemarks); + assert.deepEqual(next.changes, [{ kind: "checks-failed", failed: [fresh] }]); + assert.deepEqual( + evaluatePullRequestWatch(next.next, detail({ checks: [job(10, 20), bot] }), noRemarks) + .changes, + [], + ); + } }); it("reports passed once the required checks pass, whatever the others do", () => { diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index e4a00f062424..306d32517143 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -37,6 +37,16 @@ const isFailedCheck = (check: PullRequestCheck) => const workflowRunOf = (url: string | null) => url?.match(/^.*\/actions\/runs\/\d+/)?.[0] ?? null; const approvalKey = (check: PullRequestCheck) => workflowRunOf(check.url) ?? check.name; +function immutableFailureKey(head: string | null, check: PullRequestCheck): string | undefined { + // GitHub allocates a new job id for every run attempt. URLs without that + // immutable identity retain the existing transition-based reporting policy. + const job = check.url?.match( + /^(https:\/\/[^/]+\/[^/]+\/[^/]+\/actions\/runs\/\d+(?:\/attempts\/\d+)?\/job\/\d+)(?:$|[?#])/, + )?.[1]; + if (head === null || job === undefined) return; + return JSON.stringify([head, job.toLowerCase(), check.status]); +} + export function evaluatePullRequestWatch( watch: ThreadPullRequestWatch, detail: Pick< @@ -61,6 +71,7 @@ export function evaluatePullRequestWatch( // Hosts can return no checks on read failure; preserve prior state. let failedChecks = headMoved ? [] : watch.failedChecks; + const reportedFailures = new Set(watch.reportedFailures ?? []); let passed = headMoved ? false : watch.passed; let passedChecks = headMoved ? [] : watch.passedChecks; let owedChecks = headMoved ? [] : (watch.owedChecks ?? []); @@ -77,7 +88,16 @@ export function evaluatePullRequestWatch( if (checks.length > 0) { // A run awaiting approval waits on a person, like a review, so only running checks count. checksPending = checks.some((check) => check.status === "pending"); - const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name)); + const newlyFailed = failed.filter((check) => { + const key = immutableFailureKey(headSha, check); + return key !== undefined && watch.reportedFailures !== undefined + ? !reportedFailures.has(key) + : !failedChecks.includes(check.name); + }); + for (const check of failed) { + const key = immutableFailureKey(headSha, check); + if (key !== undefined) reportedFailures.add(key); + } if (!checksBaseline && newlyFailed.length > 0) { changes.push({ kind: "checks-failed", failed: newlyFailed }); } @@ -238,6 +258,9 @@ export function evaluatePullRequestWatch( startedAt: watch.startedAt, headSha, failedChecks, + ...(reportedFailures.size > 0 || watch.reportedFailures !== undefined + ? { reportedFailures: [...reportedFailures] } + : {}), passed, passedChecks, owedChecks, diff --git a/apps/server/src/orchestration-v2/runtimeLayer.test.ts b/apps/server/src/orchestration-v2/runtimeLayer.test.ts index d488c76f313b..fba33bb4251f 100644 --- a/apps/server/src/orchestration-v2/runtimeLayer.test.ts +++ b/apps/server/src/orchestration-v2/runtimeLayer.test.ts @@ -3764,7 +3764,13 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { }); assert.deepEqual(yield* watchOf, started); - const recorded = { ...started, headSha: "abc123", failedChecks: ["lint"], wakes: 1 }; + const recorded = { + ...started, + headSha: "abc123", + failedChecks: ["lint"], + reportedFailures: ['["abc123","https://github.com/o/r/actions/runs/1/job/2","failure"]'], + wakes: 1, + }; yield* orchestrator.dispatch({ type: "thread.pull-request-watch.sync", commandId: CommandId.make("pr-watch-record"), @@ -3803,6 +3809,21 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { assert.isUndefined(yield* watchOf); const { messages } = yield* orchestrator.getThreadRecords(threadId, ["messages"]); assert.deepEqual(messages, []); + yield* orchestrator.dispatch({ + type: "thread.pull-request.watch", + commandId: CommandId.make("pr-watch-restart-history"), + threadId, + ...key, + watching: true, + }); + assert.deepEqual((yield* watchOf)?.reportedFailures, recorded.reportedFailures); + yield* orchestrator.dispatch({ + type: "thread.pull-request.watch", + commandId: CommandId.make("pr-watch-restart-stop"), + threadId, + ...key, + watching: false, + }); }), ); @@ -4621,7 +4642,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 runUrl = "https://github.com/pingdotgg/t3code/actions/runs/123"; const host = Layer.mergeAll( @@ -4710,6 +4731,39 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.owedChecks, [], ); + remainingStatus = "failure"; + yield* TestClock.adjust("10 minutes"); + yield* restarted.sweep; + yield* worker.drain(); + const failedWatch = (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch; + assert.isDefined(failedWatch); + if (failedWatch === undefined) return; + yield* orchestrator.dispatch({ + type: "thread.pull-request-watch.sync", + commandId: CommandId.make(`pr-watch-owed-legacy-${initialStatus}`), + threadId, + host: "github.com", + repository: "pingdotgg/t3code", + number: 12, + startedAt: failedWatch.startedAt, + watch: { ...failedWatch, reportedFailures: undefined }, + }); + yield* TestClock.adjust("10 minutes"); + yield* restarted.sweep; + yield* worker.drain(); + assert.deepEqual( + (yield* orchestrator.getThreadShell(threadId))?.pullRequests?.[0]?.watch?.reportedFailures, + failedWatch.reportedFailures, + ); + remainingStatus = "pending"; + yield* TestClock.adjust("10 minutes"); + yield* restarted.sweep; + yield* worker.drain(); + remainingStatus = "failure"; + yield* TestClock.adjust("10 minutes"); + yield* restarted.sweep; + yield* worker.drain(); + assert.deepEqual(yield* summaries, ["#12: checks passed", "#12: checks failed"]); yield* watch(false, "end"); }), ); diff --git a/packages/contracts/src/threadPullRequest.ts b/packages/contracts/src/threadPullRequest.ts index 0c8d7a61fe92..9f9ec3564f53 100644 --- a/packages/contracts/src/threadPullRequest.ts +++ b/packages/contracts/src/threadPullRequest.ts @@ -84,6 +84,8 @@ export const ThreadPullRequestWatch = Schema.Struct({ startedAt: IsoDateTime, headSha: Schema.NullOr(TrimmedNonEmptyString), failedChecks: Schema.Array(TrimmedNonEmptyString), + /** Consumed immutable job failures, retained across transient reads and rewatching. */ + reportedFailures: Schema.optional(Schema.Array(TrimmedNonEmptyString)), passed: Schema.Boolean, passedChecks: Schema.Array(TrimmedNonEmptyString).pipe( Schema.withDecodingDefault(Effect.succeed([])),