diff --git a/apps/server/src/orchestration-v2/Orchestrator.ts b/apps/server/src/orchestration-v2/Orchestrator.ts index 61929131e9d72..e9e464212ff8e 100644 --- a/apps/server/src/orchestration-v2/Orchestrator.ts +++ b/apps/server/src/orchestration-v2/Orchestrator.ts @@ -5081,6 +5081,9 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio ...(existing.endedWatch?.owedChecks !== undefined && { owedChecks: existing.endedWatch.owedChecks, }), + ...(existing.endedWatch?.passedCheckRuns !== undefined && { + passedCheckRuns: existing.endedWatch.passedCheckRuns, + }), ...(existing.endedWatch?.awaitingApproval !== undefined && { awaitingApproval: existing.endedWatch.awaitingApproval, }), diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index 5b69a7efa3377..1758f5917bdda 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -207,6 +207,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc left.failedChecks.join("\n") === right.failedChecks.join("\n") && left.passed === right.passed && left.passedChecks.join("\n") === right.passedChecks.join("\n") && + JSON.stringify(left.passedCheckRuns ?? []) === JSON.stringify(right.passedCheckRuns ?? []) && (left.owedChecks ?? []).join("\n") === (right.owedChecks ?? []).join("\n") && (left.awaitingApproval ?? []).join("\n") === (right.awaitingApproval ?? []).join("\n") && (left.approvalAnnounced ?? false) === (right.approvalAnnounced ?? false) && diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index 5a836898bb66c..a9fb464ee4950 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -570,6 +570,149 @@ describe("evaluatePullRequestWatch", () => { assert.deepEqual(evaluatePullRequestWatch(green.next, botDone, noRemarks).changes, []); }); + it("reports an owed all-checks result when a rerun also settles the others", () => { + const required = (url: string) => ({ + ...check("test", "success"), + url, + required: true, + }); + const e2e = (status: PullRequestCheck["status"], url = "https://ci.example/e2e") => ({ + ...check("e2e", status), + url, + }); + const first = evaluatePullRequestWatch( + watch(), + detail({ checks: [required(run(1)), e2e("pending")] }), + noRemarks, + ); + assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 1, required: true }]); + assert.deepEqual(first.next.owedChecks, ["e2e"]); + + // The required rerun and the owed advisory settle in the same read: deliver one completion. + const settled = detail({ checks: [required(run(2)), e2e("success")] }); + const final = evaluatePullRequestWatch(first.next, settled, noRemarks); + assert.deepEqual(final.changes, [{ kind: "checks-passed", count: 2, required: false }]); + assert.deepEqual(final.next.owedChecks, []); + assert.deepEqual(evaluatePullRequestWatch(final.next, settled, noRemarks).changes, []); + + const expanded = detail({ + checks: [ + required(run(2)), + e2e("success"), + { + ...check("new gate", "success"), + required: true, + }, + ], + }); + const expandedRead = evaluatePullRequestWatch(first.next, expanded, noRemarks); + assert.deepEqual(expandedRead.changes, [{ kind: "checks-passed", count: 3, required: false }]); + assert.deepEqual(expandedRead.next.owedChecks, []); + assert.deepEqual(evaluatePullRequestWatch(expandedRead.next, expanded, noRemarks).changes, []); + + // A rerun before the owed check settles keeps its debt for the later wake. + const rerun = evaluatePullRequestWatch( + first.next, + detail({ checks: [required(run(3)), e2e("pending")] }), + noRemarks, + ); + assert.deepEqual(rerun.changes, [{ kind: "checks-passed", count: 1, required: true }]); + assert.deepEqual(rerun.next.owedChecks, ["e2e"]); + const failedRerun = evaluatePullRequestWatch( + first.next, + detail({ checks: [required(run(3)), e2e("failure")] }), + noRemarks, + ); + assert.deepEqual(failedRerun.next.owedChecks, ["e2e"]); + assert.deepEqual( + evaluatePullRequestWatch( + failedRerun.next, + detail({ checks: [required(run(3)), e2e("success")] }), + noRemarks, + ).changes, + [{ kind: "checks-passed", count: 2, required: false }], + ); + const settledRerun = detail({ checks: [required(run(3)), e2e("success")] }); + assert.deepEqual(evaluatePullRequestWatch(rerun.next, settledRerun, noRemarks).changes, [ + { kind: "checks-passed", count: 2, required: false }, + ]); + }); + + it("does not wake again for a stable duplicate check name with distinct URLs", () => { + const pipeline = (url: string): PullRequestCheck => ({ ...check("Pipeline", "success"), url }); + const firstUrl = "https://gitlab.example/pipelines/1"; + const secondUrl = "https://gitlab.example/pipelines/2"; + const checks = [pipeline(firstUrl), pipeline(secondUrl)]; + const first = evaluatePullRequestWatch(watch(), detail({ checks }), noRemarks); + assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 2, required: false }]); + assert.deepEqual(first.next.passedCheckRuns, [ + { name: "Pipeline", url: firstUrl }, + { name: "Pipeline", url: secondUrl }, + ]); + const again = evaluatePullRequestWatch(first.next, detail({ checks }), noRemarks); + assert.deepEqual(again.changes, []); + assert.deepEqual(again.next.passedCheckRuns, first.next.passedCheckRuns); + + // A new advisory name has no baseline identity, so it is not a rerun of one. + const grown = detail({ checks: [...checks, check("downstream", "success")] }); + const growth = evaluatePullRequestWatch(again.next, grown, noRemarks); + assert.deepEqual(growth.changes, []); + assert.deepEqual(evaluatePullRequestWatch(growth.next, grown, noRemarks).changes, []); + + // One duplicate reruns: its new URL is news once, then quiet. + const rerun = [pipeline("https://gitlab.example/pipelines/3"), pipeline(secondUrl)]; + const rerunRead = evaluatePullRequestWatch(growth.next, detail({ checks: rerun }), noRemarks); + assert.deepEqual(rerunRead.changes, [{ kind: "checks-passed", count: 2, required: false }]); + assert.deepEqual( + evaluatePullRequestWatch(rerunRead.next, detail({ checks: rerun }), noRemarks).changes, + [], + ); + }); + + it("keeps stage baselines through a partial pipeline read so a later rerun still wakes", () => { + const stage = (job: string, url: string): PullRequestCheck => ({ + ...check(`build / ${job}`, "success"), + url, + }); + const testUrl = "https://gitlab.example/jobs/1"; + const deployUrl = "https://gitlab.example/jobs/2"; + const full = detail({ checks: [stage("test", testUrl), stage("deploy", deployUrl)] }); + const first = evaluatePullRequestWatch(watch(), full, noRemarks); + assert.deepEqual(first.changes, [{ kind: "checks-passed", count: 2, required: false }]); + + // A URL-less partial read cannot identify a rerun, so it keeps every baseline. + const unaddressed = evaluatePullRequestWatch( + first.next, + detail({ checks: [{ ...check("Pipeline", "success"), url: null }] }), + noRemarks, + ); + assert.deepEqual(unaddressed.changes, []); + assert.deepEqual(unaddressed.next.passedCheckRuns, first.next.passedCheckRuns); + + // The stage read falls back to one passing pipeline check, which changes no known name. + const fallback = detail({ + checks: [{ ...check("Pipeline", "success"), url: "https://gitlab.example/pipelines/9" }], + }); + const fallbackRead = evaluatePullRequestWatch(unaddressed.next, fallback, noRemarks); + assert.deepEqual(fallbackRead.changes, []); + assert.deepEqual(fallbackRead.next.passedCheckRuns, [ + { name: "build / test", url: testUrl }, + { name: "build / deploy", url: deployUrl }, + { name: "Pipeline", url: "https://gitlab.example/pipelines/9" }, + ]); + const fallbackAgain = evaluatePullRequestWatch(fallbackRead.next, fallback, noRemarks); + assert.deepEqual(fallbackAgain.changes, []); + assert.lengthOf(fallbackAgain.next.passedCheckRuns ?? [], 3); + + // Full jobs return after a rerun of one existing name: its new URL is a successful rerun. + const rerun = detail({ + checks: [stage("test", "https://gitlab.example/jobs/3"), stage("deploy", deployUrl)], + }); + const rerunRead = evaluatePullRequestWatch(fallbackAgain.next, rerun, noRemarks); + assert.deepEqual(rerunRead.changes, [{ kind: "checks-passed", count: 2, required: false }]); + assert.deepEqual(evaluatePullRequestWatch(rerunRead.next, rerun, noRemarks).changes, []); + }); + it("reports an owed non-required failure once after the required notice", () => { const required = { ...check("test", "success"), required: true }; const early = detail({ checks: [required, check("e2e", "pending")] }); diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index a6603aad97c3f..98cdb5a7c4b43 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -33,6 +33,66 @@ export interface PullRequestWatchReport { const isFailedCheck = (check: PullRequestCheck) => check.status === "failure" || check.status === "cancelled"; +type CheckRun = { readonly name: string; readonly url: string }; + +// A rerun of a known check replaces its URL; extra or missing names are not reruns. Keeps a +// partial read from making a full read of an unchanged job look new. +function reranKnownCheck( + previous: ReadonlyArray, + gate: ReadonlyArray, +): boolean { + const urls = new Map>(); + for (const run of previous) { + const known = urls.get(run.name) ?? new Set(); + known.add(run.url); + urls.set(run.name, known); + } + return gate.some((check) => { + if (!check.url) return false; + const known = urls.get(check.name); + return known !== undefined && !known.has(check.url); + }); +} + +// Partial reads omit stages and URL-less checks, so their identities keep their last URLs. +// Observed names take this read's URLs instead, which bounds the history across retries. +function nextPassedCheckRuns( + previous: ReadonlyArray, + checks: ReadonlyArray, +): Array { + const observed = new Map>(); + for (const check of checks) { + if (!check.url) continue; + const runs = observed.get(check.name) ?? []; + if (!runs.some((run) => run.url === check.url)) { + runs.push({ name: check.name, url: check.url }); + } + observed.set(check.name, runs); + } + const next: Array = []; + const seen = new Set(); + const add = (run: CheckRun) => { + const key = JSON.stringify([run.name, run.url]); + if (seen.has(key)) return; + seen.add(key); + next.push(run); + }; + const replaced = new Set(); + for (const run of previous) { + const runs = observed.get(run.name); + if (!runs) { + add(run); + } else if (!replaced.has(run.name)) { + replaced.add(run.name); + runs.forEach(add); + } + } + for (const [name, runs] of observed) { + if (!replaced.has(name)) runs.forEach(add); + } + return next; +} + // 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; @@ -63,6 +123,7 @@ export function evaluatePullRequestWatch( let failedChecks = headMoved ? [] : watch.failedChecks; let passed = headMoved ? false : watch.passed; let passedChecks = headMoved ? [] : watch.passedChecks; + let passedCheckRuns = headMoved ? [] : (watch.passedCheckRuns ?? []); let owedChecks = headMoved ? [] : (watch.owedChecks ?? []); let awaitingApproval = headMoved ? [] : (watch.awaitingApproval ?? []); let approvalAnnounced = headMoved ? false : (watch.approvalAnnounced ?? false); @@ -128,6 +189,7 @@ export function evaluatePullRequestWatch( 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. const gateGrew = required.length > 0 && gateNames.some((name) => !told.includes(name)); + const runChanged = reranKnownCheck(passedCheckRuns, gate); // Other checks still pending when required ones passed are owed one wake when all settle. // Only those are owed, so later advisory churn never wakes. One turned required is gate news. const owed = @@ -141,22 +203,39 @@ export function evaluatePullRequestWatch( awaitingApproval.length === 0 && checks.every((check) => check.status !== "pending"); owedChecks = owed; - if (passedNow && (!passed || gateGrew)) { - if (!checksBaseline) { - changes.push({ kind: "checks-passed", count: gate.length, required: required.length > 0 }); + if (passedNow && (!passed || gateGrew || runChanged)) { + // A rerun that lands with the owed checks settling is one all-checks result, not two. + if (runChanged && settled && owed.length > 0) { + changes.push({ kind: "checks-passed", count: checks.length, required: false }); + owedChecks = []; + } else { + if (!checksBaseline) { + changes.push({ + kind: "checks-passed", + count: gate.length, + required: required.length > 0, + }); + } + owedChecks = + required.length > 0 + ? checks + .filter( + (check) => + check.required !== true && + (check.status === "pending" || (runChanged && owed.includes(check.name))), + ) + .map((check) => check.name) + : []; } - owedChecks = - required.length > 0 - ? 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: checks.length, required: false }); owedChecks = []; } passed = passedNow; passedChecks = passedNow ? gateNames : []; + if (passedNow) { + passedCheckRuns = nextPassedCheckRuns(passedCheckRuns, gate); + } } // Only a head never read with checks waits for them; a legacy watch's clock starts at its next @@ -240,6 +319,7 @@ export function evaluatePullRequestWatch( failedChecks, passed, passedChecks, + passedCheckRuns, owedChecks, awaitingApproval, approvalAnnounced, diff --git a/apps/server/src/orchestration-v2/runtimeLayer.test.ts b/apps/server/src/orchestration-v2/runtimeLayer.test.ts index d488c76f313b1..549db5d279c3a 100644 --- a/apps/server/src/orchestration-v2/runtimeLayer.test.ts +++ b/apps/server/src/orchestration-v2/runtimeLayer.test.ts @@ -4230,10 +4230,15 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { yield* watch(true, "start"); let headSha = "ad70160aaa"; + let runId = 1; + let required = true; const host = Layer.mergeAll( NodeServices.layer, NodeCrypto.layer, Layer.mock(PullRequestService.PullRequestService)({ + watchFingerprint: () => + Effect.succeed({ status: `${headSha} run-${runId}`, remarks: "unchanged" }), + invalidate: () => Effect.void, detail: () => Effect.sync(() => ({ ...watchedPullRequestDetail({ @@ -4242,12 +4247,12 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { at: "2026-10-02T12:00:00.000Z", }), headSha, - checks: ["lint", "test"].map((name) => ({ + checks: ["lint", "test"].map((name, index) => ({ name, status: "success" as const, description: null, - url: null, - required: true, + url: `https://github.com/pingdotgg/t3code/actions/runs/${runId}/job/${runId * 10 + index}`, + required, })), })), activity: () => @@ -4295,11 +4300,32 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { yield* worker.drain(); assert.deepEqual(yield* summaries, ["#12: checks passed"]); - headSha = "cccccccccc"; + // A complete same-head run can fall between reads, without any pending detail. + runId = 2; yield* TestClock.adjust("10 minutes"); yield* restarted.sweep; yield* worker.drain(); assert.deepEqual(yield* summaries, ["#12: checks passed", "#12: checks passed"]); + + const afterRerun = yield* PullRequestWatchReactor.make.pipe(Effect.provide(host)); + yield* afterRerun.sweep; + yield* watch(false, "stop-rerun"); + yield* watch(true, "again-rerun"); + yield* afterRerun.sweep; + assert.deepEqual(yield* summaries, ["#12: checks passed", "#12: checks passed"]); + + required = false; + runId = 3; + yield* TestClock.adjust("2 minutes"); + yield* afterRerun.sweep; + assert.equal((yield* summaries).length, 3); + yield* afterRerun.sweep; + assert.equal((yield* summaries).length, 3); + + headSha = "cccccccccc"; + yield* TestClock.adjust("10 minutes"); + yield* afterRerun.sweep; + assert.equal((yield* summaries).length, 4); yield* watch(false, "end"); }), ); diff --git a/packages/contracts/src/threadPullRequest.test.ts b/packages/contracts/src/threadPullRequest.test.ts index 334b85d5480af..c12cdb764e2df 100644 --- a/packages/contracts/src/threadPullRequest.test.ts +++ b/packages/contracts/src/threadPullRequest.test.ts @@ -32,5 +32,6 @@ it.effect("decodes a watch saved before passed checks were recorded", () => ]); assert.deepStrictEqual(link?.watch?.passedChecks, []); + assert.isUndefined(link?.watch?.passedCheckRuns); }), ); diff --git a/packages/contracts/src/threadPullRequest.ts b/packages/contracts/src/threadPullRequest.ts index 0c8d7a61fe92d..c36f7d90dfb8b 100644 --- a/packages/contracts/src/threadPullRequest.ts +++ b/packages/contracts/src/threadPullRequest.ts @@ -88,6 +88,10 @@ export const ThreadPullRequestWatch = Schema.Struct({ passedChecks: Schema.Array(TrimmedNonEmptyString).pipe( Schema.withDecodingDefault(Effect.succeed([])), ), + /** Job URLs distinguish successful reruns that finish between watch reads. */ + passedCheckRuns: Schema.optional( + Schema.Array(Schema.Struct({ name: TrimmedNonEmptyString, url: Schema.String })), + ), /** Checks still pending when required ones passed early, owed one wake once all settle. */ owedChecks: Schema.optional(Schema.Array(TrimmedNonEmptyString)), /** Approval holds keyed by workflow run URL, or check name when no run URL is known. */