diff --git a/lib/plugins/__tests__/github-approve-on-green.test.ts b/lib/plugins/__tests__/github-approve-on-green.test.ts index 8206cc51..65887ac7 100644 --- a/lib/plugins/__tests__/github-approve-on-green.test.ts +++ b/lib/plugins/__tests__/github-approve-on-green.test.ts @@ -358,6 +358,8 @@ interface SweepPr { base?: string; /** HTTP status for a PUT /pulls/{n}/merge (default 200). */ mergeStatus?: number; + /** ISO created_at for the pulls listing (default: undefined — reads as "too young" for rescue). */ + createdAt?: string; } /** Stub GitHub REST for a reconciliation sweep over one repo's open PRs. */ @@ -386,7 +388,7 @@ function stubSweepFetch(prs: SweepPr[]): Captured { return json(prs.map((p) => ({ number: p.number, state: "open", draft: false, head: { sha: p.sha }, base: { ref: p.base ?? "feature/x" }, - title: "t", html_url: "u", user: { login: "dev" }, + title: "t", html_url: "u", user: { login: "dev" }, created_at: p.createdAt, }))); } if (url.match(/\/pulls\/\d+\/reviews/) && method === "GET") { @@ -437,24 +439,27 @@ function approvedGreenPr(number: number, sha: string, base: string): SweepPr { async function runSweepWithPlugin( prs: SweepPr[], - opts: { seedWatch?: { key: string; ageMs: number } } = {}, -): Promise<{ captured: Captured; plugin: GitHubPlugin }> { + opts: { seedWatch?: { key: string; ageMs: number }; plugin?: GitHubPlugin } = {}, +): Promise<{ captured: Captured; plugin: GitHubPlugin; dispatched: string[] }> { const captured = stubSweepFetch(prs); const registry = new ProjectRegistry(); // Registry is populated from a remote fetch in prod; stub the one accessor the // sweep reads so it iterates our fixture repo. (registry as unknown as { getGithubCoords: () => string[] }).getGithubCoords = () => [`${OWNER}/${REPO}`]; - const plugin = new GitHubPlugin("/tmp/nonexistent-ws", registry); + const plugin = opts.plugin ?? new GitHubPlugin("/tmp/nonexistent-ws", registry); if (opts.seedWatch) { (plugin as unknown as { mergeCompletionFirstSeen: Map }).mergeCompletionFirstSeen.set( opts.seedWatch.key, Date.now() - opts.seedWatch.ageMs, ); } + const bus = new InMemoryEventBus(); + const dispatched: string[] = []; + bus.subscribe("message.inbound.github.#", "t", (m) => { dispatched.push(m.topic); }); await (plugin as unknown as { - _reconcileApproveOnGreen: (g: () => Promise) => Promise; - })._reconcileApproveOnGreen(async () => "fake-token"); - return { captured, plugin }; + _reconcileApproveOnGreen: (b: InMemoryEventBus, g: () => Promise) => Promise; + })._reconcileApproveOnGreen(bus, async () => "fake-token"); + return { captured, plugin, dispatched }; } describe("_reconcileApproveOnGreen — level-triggered backstop (#879)", () => { @@ -547,6 +552,38 @@ describe("_reconcileApproveOnGreen — level-triggered backstop (#879)", () => { expect(watch2.size).toBe(1); // kept — retry next sweep }); + test("rescues an old unreviewed terminal PR with ONE dispatched review; second sweep is silent (ws-22l)", async () => { + const stranded: SweepPr = { + number: 906, sha: "los0906", reviews: [], createdAt: new Date(Date.now() - 30 * 60_000).toISOString(), + checkRuns: [{ status: "completed", conclusion: "success" }], + }; + const r1 = await runSweepWithPlugin([stranded]); + expect(r1.dispatched.length).toBe(1); // rescue review dispatched + expect(r1.captured.approvePosts.length).toBe(0); // never a blind approve + + // same plugin, next sweep — the rescue map holds, no duplicate + const r2 = await runSweepWithPlugin([stranded], { plugin: r1.plugin }); + expect(r2.dispatched.length).toBe(0); + }); + + test("does NOT rescue a young unreviewed PR (opened-dispatch may still be in flight)", async () => { + const fresh: SweepPr = { + number: 909, sha: "yng0909", reviews: [], createdAt: new Date().toISOString(), + checkRuns: [{ status: "completed", conclusion: "success" }], + }; + const { dispatched } = await runSweepWithPlugin([fresh]); + expect(dispatched.length).toBe(0); + }); + + test("does NOT rescue an unreviewed PR whose CI is still running", async () => { + const pending: SweepPr = { + number: 910, sha: "pnd0910", reviews: [], createdAt: new Date(Date.now() - 30 * 60_000).toISOString(), + checkRuns: [{ status: "in_progress" }], + }; + const { dispatched } = await runSweepWithPlugin([pending]); + expect(dispatched.length).toBe(0); + }); + test("covers a reviewed-but-UNREGISTERED repo (the approve-on-green gap fix)", async () => { // Registry is empty (repo not tagged / not in EXPLICIT), but Quinn reviewed // it this process → the sweep must still cover it via reviewedRepoCoords. @@ -560,8 +597,8 @@ describe("_reconcileApproveOnGreen — level-triggered backstop (#879)", () => { const plugin = new GitHubPlugin("/tmp/nonexistent-ws", registry); (plugin as unknown as { reviewedRepoCoords: Set }).reviewedRepoCoords.add(`${OWNER}/${REPO}`); await (plugin as unknown as { - _reconcileApproveOnGreen: (g: () => Promise) => Promise; - })._reconcileApproveOnGreen(async () => "fake-token"); + _reconcileApproveOnGreen: (b: InMemoryEventBus, g: () => Promise) => Promise; + })._reconcileApproveOnGreen(new InMemoryEventBus(), async () => "fake-token"); expect(captured.approvePosts.length).toBe(1); expect(captured.approvePosts[0]!.commit_id).toBe("por0038"); }); diff --git a/lib/plugins/github.ts b/lib/plugins/github.ts index 9106067e..73aa750f 100644 --- a/lib/plugins/github.ts +++ b/lib/plugins/github.ts @@ -522,7 +522,7 @@ export class GitHubPlugin implements Plugin { this.reconcileTimer = setInterval(() => { if (this.reconcileInFlight) return; this.reconcileInFlight = true; - void this._reconcileApproveOnGreen(getToken) + void this._reconcileApproveOnGreen(bus, getToken) .catch((err) => log.error("Approve-on-green reconciliation sweep failed", { err })) .finally(() => { this.reconcileInFlight = false; }); }, GitHubPlugin.RECONCILE_INTERVAL_MS); @@ -879,6 +879,36 @@ export class GitHubPlugin implements Plugin { private static readonly MERGE_COMPLETION_GRACE_MS = 2 * 60_000; private static readonly MERGE_COMPLETION_TTL_MS = 60 * 60_000; + /** + * PR@sha7 keys the sweep has already rescue-dispatched a review for + * (ws-22l). One rescue per head SHA per process — if the rescue run also + * dies, a restart or a new push retries. TTL-pruned alongside use. + */ + private readonly reviewRescueDispatched = new Map(); + + /** + * Claim the one rescue-review slot for a PR head. Lost opened-dispatch + * rescues (ws-22l) must dispatch once per head SHA per process — the + * dispatcher's 30s cooldown alone lets a 3-min sweep or a late CI re-run + * re-dispatch while the rescued review (~30-60s) is still in flight. + */ + private _claimRescue(owner: string, repo: string, prNumber: number, headSha: string): boolean { + const now = Date.now(); + for (const [k, t] of this.reviewRescueDispatched) { + if (now - t > GitHubPlugin.MERGE_COMPLETION_TTL_MS) this.reviewRescueDispatched.delete(k); + } + const key = `${owner}/${repo}#${prNumber}@${headSha.slice(0, 7)}`; + if (this.reviewRescueDispatched.has(key)) return false; + this.reviewRescueDispatched.set(key, now); + return true; + } + /** + * A PR younger than this can still have its opened-webhook review in + * flight; rescuing it would double-review. Lost-dispatch PRs are minutes + * old by the time CI is terminal, so the grace costs nothing real. + */ + private static readonly RESCUE_MIN_AGE_MS = 10 * 60_000; + private _handleAutoReview( event: string, payload: Record, @@ -1092,33 +1122,53 @@ export class GitHubPlugin implements Plugin { for (const pr of pulls) { const decision = await this._evaluateApproveOnGreen(event, owner, repo, pr, headSha, getToken); - if (decision !== "needs-review") continue; - - // Eligible + terminal but not auto-approvable — re-dispatch Quinn's - // pr_review LLM pass to upgrade the provisional COMMENT to a formal verdict. - const number = pr.number as number; - const ctx: GitHubEventContext = { - owner, - repo, - number, - title: (pr.title as string | undefined) ?? "", - url: (pr.html_url as string | undefined) ?? "", - body: (pr.body as string | undefined) ?? "", - author: ((pr.user as Record | undefined)?.login as string | undefined) ?? "", - }; - // Synthesize the shape _handleAutoReview reads (pull_request.draft + - // pull_request.head.sha). skipDedup: the all-terminal gate already ensures - // a single meaningful dispatch; the dispatcher's @sha7 cooldown collapses - // any workflow_run+check_suite co-arrival. - const synthPayload: Record = { action: "ci_completed", pull_request: pr }; - log.info(`CI-completion (${event}): re-dispatching pr_review for ${owner}/${repo}#${number} @${headSha.slice(0, 7)}`); - // event="pull_request" so the published topic is byte-identical to a - // normal auto-review (the routed path is proven); the "ci_completed" - // action just suppresses the opened-only leading comment. - this._handleAutoReview("pull_request", synthPayload, ctx, bus, getToken, { skipDedup: true }); + // needs-review: eligible + terminal but not auto-approvable — upgrade the + // provisional COMMENT to a formal verdict. unreviewed: the opened-webhook + // dispatch was lost (ws-22l) — same rescue, first review instead, once + // per head SHA (a CI re-run can outlive the dispatcher's 30s cooldown + // while the rescued review is still in flight). + if (decision === "needs-review") { + this._dispatchReviewFor(event, owner, repo, pr, headSha, bus, getToken); + } else if (decision === "unreviewed" && this._claimRescue(owner, repo, pr.number as number, headSha)) { + this._dispatchReviewFor(event, owner, repo, pr, headSha, bus, getToken); + } } } + /** + * Re-dispatch Quinn's pr_review LLM pass for a PR (the CI-completion + * upgrade path and the ws-22l unreviewed rescue). Synthesizes the shape + * `_handleAutoReview` reads (pull_request.draft + pull_request.head.sha). + * skipDedup: callers gate on all-terminal CI / rescue maps; the + * dispatcher's @sha7 cooldown collapses webhook co-arrival. + */ + private _dispatchReviewFor( + source: string, + owner: string, + repo: string, + pr: Record, + headSha: string, + bus: EventBus, + getToken: (owner: string, repo: string) => Promise, + ): void { + const number = pr.number as number; + const ctx: GitHubEventContext = { + owner, + repo, + number, + title: (pr.title as string | undefined) ?? "", + url: (pr.html_url as string | undefined) ?? "", + body: (pr.body as string | undefined) ?? "", + author: ((pr.user as Record | undefined)?.login as string | undefined) ?? "", + }; + const synthPayload: Record = { action: "ci_completed", pull_request: pr }; + log.info(`CI-completion (${source}): re-dispatching pr_review for ${owner}/${repo}#${number} @${headSha.slice(0, 7)}`); + // event="pull_request" so the published topic is byte-identical to a + // normal auto-review (the routed path is proven); the "ci_completed" + // action just suppresses the opened-only leading comment. + this._handleAutoReview("pull_request", synthPayload, ctx, bus, getToken, { skipDedup: true }); + } + /** * Deterministic approve-on-terminal-green decision for a single PR (#748, * #848). Shared by the edge-triggered CI-completion webhook and the @@ -1153,14 +1203,27 @@ export class GitHubPlugin implements Plugin { pr: Record, headSha: string, getToken: (owner: string, repo: string) => Promise, - ): Promise<"approved" | "merged" | "needs-review" | "skip"> { + ): Promise<"approved" | "merged" | "needs-review" | "unreviewed" | "skip"> { const number = pr.number as number; if (!prEligibleForCiReview(pr, headSha)) return "skip"; const reviews = (await this._ghGet(getToken, owner, repo, `/repos/${owner}/${repo}/pulls/${number}/reviews`)) as | Array> | null; - if (!quinnHasReviewed(reviews ?? undefined)) return "skip"; + if (!quinnHasReviewed(reviews ?? undefined)) { + // Never reviewed at all. The PR-opened webhook normally dispatched the + // review, but that in-process run dies with a container restart (the + // #906 strand, ws-22l) and nothing else re-triggers: the promote/merge + // machinery needs a prior review to act on. Signal "unreviewed" so the + // caller can dispatch a rescue review — only once CI is terminal (the + // meaningful review moment) and the PR is old enough that the original + // opened-dispatch can't still be in flight. + const createdAt = Date.parse((pr.created_at as string | undefined) ?? ""); + const oldEnough = Number.isFinite(createdAt) && Date.now() - createdAt > GitHubPlugin.RESCUE_MIN_AGE_MS; + if (!oldEnough) return "skip"; + if (!(await this._ciTerminal(getToken, owner, repo, headSha))) return "skip"; + return "unreviewed"; + } if (!(await this._ciTerminal(getToken, owner, repo, headSha))) { log.info(`CI-completion (${source}): ${owner}/${repo}#${number} — checks not all terminal yet, deferring`); return "skip"; @@ -1325,6 +1388,7 @@ export class GitHubPlugin implements Plugin { * GitHub's native auto-merge stays wedged. */ private async _reconcileApproveOnGreen( + bus: EventBus, getToken: (owner: string, repo: string) => Promise, ): Promise { // Union the registry with repos Quinn has reviewed this process. A repo @@ -1335,6 +1399,7 @@ export class GitHubPlugin implements Plugin { for (const c of this.reviewedRepoCoords) coords.add(c); let approved = 0; let merged = 0; + let rescued = 0; for (const coord of coords) { const slash = coord.indexOf("/"); if (slash <= 0) continue; @@ -1356,6 +1421,10 @@ export class GitHubPlugin implements Plugin { const decision = await this._evaluateApproveOnGreen("reconcile", owner, repo, pr, headSha, getToken); if (decision === "approved") approved++; else if (decision === "merged") merged++; + else if (decision === "unreviewed" && this._claimRescue(owner, repo, pr.number as number, headSha)) { + this._dispatchReviewFor("reconcile-rescue", owner, repo, pr, headSha, bus, getToken); + rescued++; + } } catch (err) { log.error( `Reconcile approve-on-green failed for ${owner}/${repo}#${pr.number}`, @@ -1364,10 +1433,10 @@ export class GitHubPlugin implements Plugin { } } } - if (approved > 0 || merged > 0) { + if (approved > 0 || merged > 0 || rescued > 0) { log.info( `Approve-on-green reconciliation sweep: ${approved} stranded PR(s) approved, ` + - `${merged} wedged merge(s) completed`, + `${merged} wedged merge(s) completed, ${rescued} unreviewed PR(s) rescue-dispatched`, ); } }