From 6a8bd8df014231a1311160ddcdef5e7ce6d6d31c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:51:34 -0700 Subject: [PATCH] fix(review): forced re-run steals an orphaned AI-review lock instead of silently no-op'ing; re-run checkbox ignores closed/merged PRs Two related fixes to the panel re-run mechanism: #9008 -- a forced re-run (the panel checkbox / `@loopover review`) previously died silently behind the AI-review lock: claimAiReviewLock had no way to bypass a still-held (possibly orphaned, from a self-host process that died mid-review) lock, so the contended path returned the same inconclusive placeholder a genuine race would, with zero audit trail, and that placeholder's plain-prose notes masqueraded as a real review summary (hasPublicReviewAssessment had no way to tell it apart), suppressing the one alarm meant to catch this. shouldStartAiReviewForAdvisory also silently ignored forceAiReview entirely, so a low-reputation author's forced re-run could skip for a completely different, unrelated reason. Adds a `steal` option to claimTransientLock (cache path: unconditional `set` instead of `claim`; SubmissionLock DO: same overwrite semantics), threaded through claimAiReviewLock and used whenever `forceAiReview: true`. Names the contended path in the audit trail unconditionally (forced or not). Fixes the masquerade by checking `persistable === false` directly -- the one signal aiReviewLockContendedResult uniquely produces. Threads forceAiReview into shouldStartAiReviewForAdvisory to bypass ONLY the reputation skip, never a maintainer-configured hard gate. #9020 -- the panel comment is deliberately preserved after merge/close and its re-run checkbox stays interactive on GitHub forever, but maybeProcessPrPanelRetrigger never checked pr.state, so a post-merge click ran deep into the pipeline (real GitHub/DB spend) before dying at the freshness guard with contradictory telemetry and no user feedback. Adds the same `pr.state !== "open"` guard reReviewStoredPullRequest already has. Separately, createOrUpdateSkippedGateCheckRun had no updateExisting mode, so its no-checkRunId lookup path could PATCH an already-`completed` Gate run to `skipped` -- falsifying a fully-evaluated, merged PR's historical verdict. Scoped to updateExisting: "in_progress_only", matching this call's actual purpose (finalize an in-progress evaluation that never finished). Closes #9008, #9020 Tests: 4 new transient-lock steal tests (cache + DO paths, both success and fail-open), a forceAiReview-bypasses-reputation test, two full-pipeline integration tests proving the steal + audit event + masquerade fix end to end (including the audit write's own fail-open branch), a pr_not_open skip test, and a completed-check-run-never-demoted test. 100% line+branch coverage on every changed line across all 5 touched source files. --- src/github/app.ts | 6 ++ src/queue/ai-review-orchestration.ts | 10 ++- src/queue/processors.ts | 51 +++++++++++- src/queue/submission-lock.ts | 6 +- src/queue/transient-locks.ts | 19 ++++- test/unit/ai-review-advisory.test.ts | 18 +++++ test/unit/github-app.test.ts | 57 ++++++++++++++ test/unit/queue-4.test.ts | 57 ++++++++++++++ test/unit/queue.test.ts | 96 +++++++++++++++++++++++ test/unit/transient-locks.test.ts | 112 +++++++++++++++++++++++++++ 10 files changed, 427 insertions(+), 5 deletions(-) diff --git a/src/github/app.ts b/src/github/app.ts index 410fbe6824..be7a392e22 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -827,6 +827,12 @@ export async function createOrUpdateSkippedGateCheckRun( summary: reason, text: "LoopOver does not post late first comments on closed or merged pull requests.", }, + // #9020: without this, the no-checkRunId lookup path's default (effectively "any") lets this call PATCH + // an already-`completed` Gate run (e.g. a fully-evaluated, green run on a since-merged head) down to + // `skipped` -- falsifying the historical verdict. This call's own purpose is narrower: finalize an + // in-progress "evaluating" run when the PR closed before evaluation finished, which "in_progress_only" + // already covers correctly; it must never touch a run that already reached a real terminal conclusion. + updateExisting: "in_progress_only", supersedeLegacyNames: [GITTENSORY_LEGACY_GATE_CHECK_NAME, GITTENSORY_LEGACY_ORB_GATE_CHECK_NAME], mode, }, diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 0b447c2249..5948ab2dfa 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -116,11 +116,13 @@ export async function claimAiReviewLock( prNumber: number, headSha: string, mode: string, + options?: { steal?: boolean }, ): Promise { return claimTransientLock( env, aiReviewLockKey(repoFullName, prNumber, headSha, mode), AI_REVIEW_LOCK_TTL_SECONDS, + options, ); } @@ -185,10 +187,16 @@ export async function shouldStartAiReviewForAdvisory( // no second REPUTATION_WINDOW_ROW_CAP-bounded review_targets scan when the caller already ran one this pass. // Absent (every existing/direct caller) ⇒ computed here exactly as before. preComputedReputationSkip?: boolean | undefined; + // #9008: true for a maintainer's explicit forced re-run (the PR-panel checkbox / `@loopover review`). + // Bypasses ONLY the reputation anti-abuse skip below -- a transient, heuristic gate about spend, not a + // configuration decision. It deliberately does NOT bypass shouldRequirePublicAiReviewForAdvisory's hard + // gates (aiReviewMode off, an ineligible author under aiReviewConfirmedContributorsOnly, no AI binding, + // ...): those are maintainer-configured policy a click cannot override. Absent/false ⇒ byte-identical. + forceAiReview?: boolean | undefined; }, ): Promise { if (!shouldRequirePublicAiReviewForAdvisory(env, args)) return false; - if (args.settings.aiReviewAllAuthors) return true; + if (args.settings.aiReviewAllAuthors || args.forceAiReview) return true; if (!(isReputationEnabled(env) && isConvergenceRepoAllowed(env, args.repoFullName))) return true; const reputationSkip = args.preComputedReputationSkip ?? diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d96a8d77d8..893fa86e24 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -9998,6 +9998,7 @@ async function maybePublishPrPublicSurface( confirmedContributor, skipAiReview: webhook.skipAiReview, preComputedReputationSkip, + forceAiReview: webhook.forceAiReview, })); aiReviewExpected = aiReviewWillRun; if (isFrozenForManualReview) { @@ -10185,8 +10186,31 @@ async function maybePublishPrPublicSurface( pr.number, aiReviewHeadSha, settings.aiReviewMode, + // #9008: a maintainer's explicit forced re-run (the panel checkbox / `@loopover review`) steals this + // lock instead of deferring to whatever currently holds it. Without this, an orphaned lock left behind + // by a self-host process that died mid-review made the re-run button permanently inert for the full + // TTL (confirmed live: 15+ minutes) -- a duplicate LLM call is the explicitly accepted cost. + { steal: webhook.forceAiReview === true }, ); if (!aiReviewLock.acquired) { + // #9008: this branch used to be completely silent -- no audit event, so a killed process's orphaned + // lock made every subsequent pass (forced or not) vanish with zero trace. Named unconditionally; a + // FORCED pass landing here at all is itself notable (steal should make that unreachable barring a + // cache error, so `forced: true` here is a signal worth alerting on, not just informational). + await recordAuditEvent(env, { + eventType: "github_app.ai_review_lock_contended", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: "Another pass already holds the AI-review lock for this exact PR head; deferring rather than duplicating the LLM call.", + metadata: { + deliveryId: webhook.deliveryId, + repoFullName, + headSha: aiReviewHeadSha, + aiReviewMode: settings.aiReviewMode, + forced: webhook.forceAiReview === true, + }, + }).catch(() => undefined); aiReview = aiReviewLockContendedResult(advisory); } else { try { @@ -10577,7 +10601,14 @@ async function maybePublishPrPublicSurface( }, ); } - if (aiReviewExpected && !hasPublicReviewAssessment(aiReview?.notes)) { + // #9008: `aiReview?.persistable === false` is produced ONLY by aiReviewLockContendedResult + // (ai-review-orchestration.ts) — its plain-prose "AI review is already running..." notes have no + // Blockers/Nits section, so hasPublicReviewAssessment previously read them as a legitimate summary and + // silently suppressed this exact alarm — a lock-contended pass (forced or not) masqueraded as a real, + // completed review with a public assessment, defeating the one safety net meant to catch "no fresh review + // happened." Checking persistable directly (rather than special-casing the notes string) is precise: no + // other producer ever sets it false. + if (aiReviewExpected && (aiReview?.persistable === false || !hasPublicReviewAssessment(aiReview?.notes))) { const message = "AI review did not produce a public summary; publishing deterministic PR surface without AI notes"; await recordAuditEvent(env, { @@ -13056,6 +13087,24 @@ async function maybeProcessPrPanelRetrigger( ); return true; } + // #9020: the panel comment is deliberately preserved after merge/close (#preserve-review-on-close) and its + // re-run checkbox stays interactive on GitHub forever, but every downstream step below assumes an OPEN PR -- + // prReadyForReview returns true unconditionally for a non-open PR (it has no CI/mergeable state left to wait + // on), so a post-merge click used to sail straight through readiness, spend a real gate evaluation + live CI + // reads, and only die deep in the pipeline at the freshness guard (which records a contradictory "denied" on + // top of this call's own "completed" outcome) -- real spend, no user-visible feedback, checkbox left ticked. + // Mirrors reReviewStoredPullRequest's identical `pr.state !== "open"` guard (the sibling entry point). + if (pr.state !== "open") { + await recordPrPanelRetriggerSkip( + env, + deliveryId, + repoFullName, + targetKey, + actor, + "pr_not_open", + ); + return true; + } const { authorization } = await authorizePrActionActor({ env, diff --git a/src/queue/submission-lock.ts b/src/queue/submission-lock.ts index ae218f6871..912394c948 100644 --- a/src/queue/submission-lock.ts +++ b/src/queue/submission-lock.ts @@ -14,6 +14,9 @@ type LockRecord = { type ClaimBody = { ownerToken?: unknown; ttlSeconds?: unknown; + // #9008: a forced re-run intentionally takes ownership even from a still-live holder — mirrors the + // cache-path steal in claimTransientLock (transient-locks.ts), kept consistent across both lock backends. + steal?: unknown; }; type ReleaseBody = { @@ -44,9 +47,10 @@ export class SubmissionLock extends DurableObject { return Response.json({ error: "invalid_claim" }, { status: 400 }); } + const steal = body?.steal === true; const now = Date.now(); const existing = await this.ctx.storage.get(STORAGE_KEY); - if (existing && existing.expiresAt > now && existing.ownerToken !== ownerToken) { + if (!steal && existing && existing.expiresAt > now && existing.ownerToken !== ownerToken) { return Response.json({ acquired: false }); } diff --git a/src/queue/transient-locks.ts b/src/queue/transient-locks.ts index 837af1833c..9f5aca53ae 100644 --- a/src/queue/transient-locks.ts +++ b/src/queue/transient-locks.ts @@ -44,8 +44,9 @@ export async function claimTransientLock( env: Env, key: string, ttlSeconds: number, + options?: { steal?: boolean }, ): Promise { - const viaDo = await claimSubmissionLockIfBound(env, key, ttlSeconds); + const viaDo = await claimSubmissionLockIfBound(env, key, ttlSeconds, options); if (viaDo !== null) return viaDo; const cache = env.SELFHOST_TRANSIENT_CACHE; @@ -55,6 +56,19 @@ export async function claimTransientLock( // calling claim() so misconfigured test/custom adapters never acquire an unreleasable lock (#2129/#3153). if (!cache.releaseIfValue) return { acquired: true, ownerToken: null }; const ownerToken = randomUUID(); + // #9008: a `steal` caller (a maintainer's explicit forced re-run) intentionally takes ownership even from a + // still-live holder — a duplicate LLM call is the accepted cost, and a self-host process that died mid-review + // otherwise orphans this key for the FULL TTL with no recovery path (confirmed live: a lock outlived the + // process that claimed it by 15+ minutes). `set` unconditionally overwrites (unlike `claim`'s SET-NX), so a + // steal can never itself contend — that is the entire point. + if (options?.steal) { + try { + await cache.set(key, ownerToken, ttlSeconds); + return { acquired: true, ownerToken }; + } catch { + return { acquired: true, ownerToken: null }; // fail open — same posture as every other claim failure + } + } try { const acquired = await cache.claim(key, ownerToken, ttlSeconds); return { acquired, ownerToken: acquired ? ownerToken : null }; @@ -85,6 +99,7 @@ async function claimSubmissionLockIfBound( env: Env, key: string, ttlSeconds: number, + options?: { steal?: boolean }, ): Promise { const ns = env.SUBMISSION_LOCK; if (!ns) return null; @@ -94,7 +109,7 @@ async function claimSubmissionLockIfBound( const response = await ns.get(id).fetch("https://submission-lock/claim", { method: "POST", headers: { "content-type": "application/json" }, - body: JSON.stringify({ ownerToken, ttlSeconds }), + body: JSON.stringify({ ownerToken, ttlSeconds, steal: options?.steal === true }), }); const body = (await response.json().catch(() => null)) as { acquired?: unknown } | null; if (typeof body?.acquired !== "boolean") return { acquired: true, ownerToken: null }; // fail open diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 2eb0d80325..90c0b2c56f 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -191,6 +191,24 @@ describe("shouldStartAiReviewForAdvisory", () => { }), ).resolves.toBe(true); }); + + // #9008: a maintainer clicking the panel's re-run checkbox wants a fresh opinion regardless of the + // reputation heuristic -- before this, forceAiReview was silently ignored here, so a forced re-run for a + // low-reputation author would end with NO review and no explanation, the exact "silent skip" the issue + // reported. + it("#9008: forceAiReview bypasses ONLY the reputation skip, not the hard entry gates", async () => { + const env = createTestEnv({ AI: { run: vi.fn() } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", LOOPOVER_REVIEW_REPUTATION: "true", LOOPOVER_REVIEW_REPOS: "acme/widgets" }); + await env.DB.prepare("INSERT INTO submitter_stats (project, submitter, submissions, merged, closed, manual, last_seen) VALUES (?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)").bind("acme/widgets", "alice", 8, 0, 8, 0).run(); + // Without force, reputation still skips (pinned above) -- WITH force, the same low-reputation author is + // reviewed. + await expect(shouldStartAiReviewForAdvisory(env, { ...base, forceAiReview: true })).resolves.toBe(true); + // But force never overrides a HARD, maintainer-configured gate: aiReviewMode: "off" still returns false, + // and an explicit skipAiReview (a different caller-level suppression) still wins too. + await expect( + shouldStartAiReviewForAdvisory(env, { ...base, forceAiReview: true, settings: { aiReviewMode: "off" } as RepositorySettings }), + ).resolves.toBe(false); + await expect(shouldStartAiReviewForAdvisory(env, { ...base, forceAiReview: true, skipAiReview: true })).resolves.toBe(false); + }); }); describe("runAiReviewForAdvisory", () => { diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 512b1b55b0..279954f210 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -1864,6 +1864,63 @@ describe("GitHub check runs", () => { ); }); + // #9020: a late-arriving panel re-run click on an ALREADY-MERGED PR used to reach this call with the head's + // Gate check run already `completed` (a real, fully-evaluated verdict) -- and demote it to `skipped`, + // falsifying the historical result. `updateExisting: "in_progress_only"` means this call may only finalize a + // still-`in_progress` run; an already-completed one is left alone (a second, separate run is posted instead, + // so the call still does its job without touching the terminal one). + it("#9020: never demotes an already-completed Gate check run to skipped -- posts a separate run instead", async () => { + const privateKey = await generatePrivateKeyPem(); + let patchedExisting = false; + let newRunPosted = false; + vi.stubGlobal( + "fetch", + async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url.includes("/access_tokens")) + return Response.json({ token: "installation-token" }); + if (url.includes("/commits/merged123/check-runs")) { + return Response.json({ + total_count: 1, + check_runs: [ + { + id: 999, + name: "LoopOver Orb Review Agent", + status: "completed", + conclusion: "success", + }, + ], + }); + } + if (method === "PATCH" && url.includes("/check-runs/999")) { + patchedExisting = true; + return Response.json({ id: 999, html_url: "https://github.com/checks/999" }); + } + if (method === "POST" && url.includes("/check-runs")) { + newRunPosted = true; + return Response.json( + { id: 1000, html_url: "https://github.com/checks/1000" }, + { status: 201 }, + ); + } + return new Response("not found", { status: 404 }); + }, + ); + + const result = await createOrUpdateSkippedGateCheckRun( + createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), + 123, + "JSONbored/gittensory", + gateAdvisory("merged123"), + "Merged before LoopOver finished.", + ); + + expect(patchedExisting).toBe(false); // the completed run was never touched + expect(newRunPosted).toBe(true); // the call still completes its own job + expect(result).toMatchObject({ kind: "published", id: 1000 }); + }); + it("reposts a known check-run id when the old run belongs to a prior App", async () => { const privateKey = await generatePrivateKeyPem(); const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index ae63feabef..68a881855c 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -2527,6 +2527,63 @@ describe("queue processors", () => { }); }); + // #9020: the panel comment is deliberately preserved after merge/close (#preserve-review-on-close) and its + // re-run checkbox stays interactive on a closed/merged PR forever. Before this guard, a post-merge click + // sailed past every other check (bot_author/missing_repo_pr_or_installation/cached_pr_missing/authorization), + // ran deep into the pipeline, and only died later at the freshness guard -- real spend, contradictory + // telemetry, and no user-visible feedback. Mirrors reReviewStoredPullRequest's own `pr.state !== "open"` guard. + it("#9020: a re-run click on an already-merged PR is skipped immediately with pr_not_open, no live GitHub calls", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", {}); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 49, + title: "Already merged", + state: "closed", + merged_at: "2026-07-26T00:00:00.000Z", + user: { login: "contributor" }, + author_association: "CONTRIBUTOR", + head: { sha: "merged-sha" }, + labels: [], + body: "Validation: npm test", + }); + const checkedPanel = [ + "", + "", + "- [x] Re-run LoopOver review", + ].join("\n"); + let fetchCalls = 0; + vi.stubGlobal("fetch", async () => { + fetchCalls += 1; + return new Response("unexpected fetch", { status: 500 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "panel-retrigger-merged-pr", + eventName: "issue_comment", + payload: { + action: "edited", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 49, title: "Already merged", state: "closed", user: { login: "contributor" }, pull_request: {} }, + comment: { id: 781, body: checkedPanel, user: { login: "loopover-orb[bot]", type: "Bot" } }, + sender: { login: "maintainer", type: "User" }, + }, + }); + + expect(fetchCalls).toBe(0); // never reaches authorization/readiness/gate at all + const audit = await env.DB.prepare("select actor, target_key, detail from audit_events where event_type = ?") + .bind("github_app.pr_panel_retrigger_skipped") + .first<{ actor: string | null; target_key: string; detail: string }>(); + expect(audit).toMatchObject({ target_key: "JSONbored/gittensory#49", detail: "pr_not_open" }); + // No contradictory "completed" retrigger outcome either -- this is the ONLY audit row for the delivery. + const retriggered = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?") + .bind("github_app.pr_panel_retriggered") + .first<{ n: number }>(); + expect(retriggered?.n).toBe(0); + }); + it("ignores invalid rerun task edits and audits skipped rerun requests", async () => { const env = createTestEnv(); const checkedPanel = [ diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c8139ffe27..3e9372c936 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -7403,6 +7403,102 @@ describe("queue processors", () => { expect(bypassAudit?.outcome).toBe("completed"); }); + it("#9008: a forced re-run steals an orphaned AI-review lock instead of silently landing on the contended placeholder", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 95, title: "Held behind an orphaned AI-review lock", state: "open", user: { login: "contributor" }, head: { sha: "a95" }, labels: [], body: "Closes #1" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/pulls/95/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/95")) return Response.json({ number: 95, title: "Held behind an orphaned AI-review lock", state: "open", user: { login: "contributor" }, head: { sha: "a95" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + // Simulate the exact production scenario: a self-host process claimed the AI-review lock for this PR's + // head and died before releasing it -- the lock now sits held for the full TTL with no live pass behind + // it, per aiReviewLockKey's (repo, PR, head, mode) shape. + const orphan = await claimAiReviewLock(env, "JSONbored/gittensory", 95, "a95", "block"); + expect(orphan.acquired).toBe(true); + + // A normal (non-forced) pass correctly defers to the (orphaned, but indistinguishable from live) lock -- + // this used to be completely silent; it must now both name itself in the audit trail AND trip the + // missing-summary alarm instead of the placeholder masquerading as a real review. + await processJob(env, { type: "agent-regate-pr", deliveryId: "unforced-hits-orphan", repoFullName: "JSONbored/gittensory", prNumber: 95, installationId: 123 }); + expect(aiCalls).toBe(0); + const contendedAudit = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_lock_contended", "JSONbored/gittensory#95") + .first<{ detail: string; metadata_json: string }>(); + expect(contendedAudit).toBeTruthy(); + expect(JSON.parse(contendedAudit!.metadata_json)).toMatchObject({ forced: false, headSha: "a95" }); + const missingSummaryAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_public_summary_missing", "JSONbored/gittensory#95") + .first<{ n: number }>(); + expect(missingSummaryAudit?.n).toBeGreaterThan(0); // the masquerade no longer suppresses this alarm + + // A FORCED pass against the SAME still-held lock steals it and spends a genuinely fresh review -- + // it must NOT land on the contended path at all. + await processJob(env, { type: "agent-regate-pr", deliveryId: "forced-steals-orphan", repoFullName: "JSONbored/gittensory", prNumber: 95, installationId: 123, force: true }); + expect(aiCalls).toBeGreaterThan(0); + const secondContendedCount = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ? and detail = detail").bind("github_app.ai_review_lock_contended", "JSONbored/gittensory#95").first<{ n: number }>(); + expect(secondContendedCount?.n).toBe(1); // still just the ONE unforced-pass row from above -- the forced pass never hit this branch + }); + + it("#9008: a failing lock-contended audit write is best-effort and never crashes the deferred pass", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => ({ response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }) } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 96, title: "Held behind an orphaned lock, audit write down", state: "open", user: { login: "contributor" }, head: { sha: "a96" }, labels: [], body: "Closes #1" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/pulls/96/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/96")) return Response.json({ number: 96, title: "Held behind an orphaned lock, audit write down", state: "open", user: { login: "contributor" }, head: { sha: "a96" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + const orphan = await claimAiReviewLock(env, "JSONbored/gittensory", 96, "a96", "block"); + expect(orphan.acquired).toBe(true); + const originalRecordAuditEvent = repositoriesModule.recordAuditEvent; + const auditSpy = vi.spyOn(repositoriesModule, "recordAuditEvent").mockImplementation(async (auditEnv, event) => { + if (event.eventType === "github_app.ai_review_lock_contended") throw new Error("audit DB down"); + await originalRecordAuditEvent(auditEnv, event); + }); + + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "unforced-hits-orphan-audit-fails", repoFullName: "JSONbored/gittensory", prNumber: 96, installationId: 123 }), + ).resolves.toBeUndefined(); + + auditSpy.mockRestore(); + // The write failed, so no row exists for it -- confirming the failure was genuinely swallowed rather + // than this test accidentally not exercising the branch at all. + const contendedCount = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_lock_contended", "JSONbored/gittensory#96") + .first<{ n: number }>(); + expect(contendedCount?.n).toBe(0); + }); + describe("one-shot AI review cadence (#one-shot-review-cadence)", () => { it("default (one_shot, no manual-review label): a genuinely NEW push does not spend a fresh main-review AI call -- reuses the prior published review", async () => { let aiCalls = 0; diff --git a/test/unit/transient-locks.test.ts b/test/unit/transient-locks.test.ts index e69bf09764..22c33adcbf 100644 --- a/test/unit/transient-locks.test.ts +++ b/test/unit/transient-locks.test.ts @@ -350,6 +350,118 @@ describe("claimTransientLock / releaseTransientLockIfOwner — cache fallback wi }); }); +describe("claimTransientLock — steal option (#9008)", () => { + it("cache path: steal overwrites a still-live holder's claim instead of contending against it", async () => { + const held = new Map(); + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async (key) => held.get(key) ?? null, + set: async (key, value) => { + held.set(key, value); + }, + claim: async (key, value) => { + if (held.has(key)) return false; + held.set(key, value); + return true; + }, + releaseIfValue: async (key, value) => { + if (held.get(key) !== value) return false; + held.delete(key); + return true; + }, + }, + }); + delete env.SUBMISSION_LOCK; + + const original = await claimTransientLock(env, "steal-key", 30); + expect(original.acquired).toBe(true); + + // A plain (non-steal) claim against the same key still contends normally and loses. + const contended = await claimTransientLock(env, "steal-key", 30); + expect(contended.acquired).toBe(false); + + const stolen = await claimTransientLock(env, "steal-key", 30, { steal: true }); + expect(stolen.acquired).toBe(true); + expect(stolen.ownerToken).toEqual(expect.any(String)); + expect(stolen.ownerToken).not.toBe(original.ownerToken); + expect(held.get("steal-key")).toBe(stolen.ownerToken); + + // The stealer's own token now genuinely owns the key: it releases cleanly... + await releaseTransientLockIfOwner(env, "steal-key", stolen.ownerToken); + expect(held.has("steal-key")).toBe(false); + // ...and the ORIGINAL holder's stale release (its token no longer matches) is a safe no-op, exactly like + // the existing stale-actuation-lock-holder regression above. + held.set("steal-key", "someone-else"); + await releaseTransientLockIfOwner(env, "steal-key", original.ownerToken); + expect(held.get("steal-key")).toBe("someone-else"); + }); + + it("cache path: steal fails open (acquired: true, ownerToken: null) when the underlying set() throws", async () => { + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async () => null, + set: async () => { + throw new Error("cache unavailable"); + }, + claim: async () => true, + releaseIfValue: async () => true, + }, + }); + delete env.SUBMISSION_LOCK; + + await expect(claimTransientLock(env, "steal-key", 30, { steal: true })).resolves.toEqual({ + acquired: true, + ownerToken: null, + }); + }); + + it("cache path: a non-steal call is byte-identical to before -- steal:false/absent never touches set()", async () => { + let setCalls = 0; + const held = new Map(); + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async (key) => held.get(key) ?? null, + set: async (key, value) => { + setCalls += 1; + held.set(key, value); + }, + claim: async (key, value) => { + if (held.has(key)) return false; + held.set(key, value); + return true; + }, + releaseIfValue: async () => true, + }, + }); + delete env.SUBMISSION_LOCK; + + await claimTransientLock(env, "steal-key", 30); + await claimTransientLock(env, "steal-key", 30, { steal: false }); + expect(setCalls).toBe(0); + }); + + it("DO path: steal overwrites a still-live claim; a non-steal claim against the same key still loses", async () => { + const ns = submissionLockNamespace(); + const env = createTestEnv({ SUBMISSION_LOCK: ns as unknown as DurableObjectNamespace }); + delete env.SELFHOST_TRANSIENT_CACHE; + + const original = await claimTransientLock(env, "do-steal-key", 60); + expect(original.acquired).toBe(true); + + const contended = await claimTransientLock(env, "do-steal-key", 60); + expect(contended.acquired).toBe(false); + + const stolen = await claimTransientLock(env, "do-steal-key", 60, { steal: true }); + expect(stolen.acquired).toBe(true); + expect(stolen.ownerToken).not.toBe(original.ownerToken); + + // The stealer's release now works; the original holder's own token no longer matches what's stored. + await releaseTransientLockIfOwner(env, "do-steal-key", stolen.ownerToken); + const reclaim = await claimTransientLock(env, "do-steal-key", 60); + expect(reclaim.acquired).toBe(true); + }); +}); + describe("domain wrappers + PrActuationLockContendedError (#8896)", () => { it("routes claim/release for PR actuation and contributor-cap through the same lock helpers", async () => { const ns = submissionLockNamespace();