From ad912b21d8438533fd15a81281f8b1f619678650 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 13:48:30 +0000 Subject: [PATCH] fix(queue): wire lock-heartbeat onLost at both actuation and AI-review call sites A holder whose transient lock renewal reports it no longer owns the key (TTL lapse + re-claim, or a maintainer's forced-re-run steal) never learned about it -- onLost was documented but no caller supplied it, so a losing pass kept running to completion: the actuation pass could still merge/close/comment after another pass took over, and the AI-review pass could still overwrite the winner's cached verdict. Both heartbeats now pass onLost, which aborts the publish-and-maintain unit before any further GitHub mutation and discards the AI-review result in favor of the lock-contended placeholder instead of persisting it. --- src/queue/processors.ts | 233 ++++++++++------- test/unit/lock-heartbeat-onlost.test.ts | 316 ++++++++++++++++++++++++ 2 files changed, 462 insertions(+), 87 deletions(-) create mode 100644 test/unit/lock-heartbeat-onlost.test.ts diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a5051037e..54a21d248 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -4490,11 +4490,30 @@ export async function reReviewStoredPullRequest( // and the AI review alone can outlive the 600s TTL. Renew it while the work runs so a slow-but-healthy pass // cannot have its lock claimed out from under it mid-flight. Compare-and-extend, so if this pass has already // lost the key it learns that instead of extending the new owner's lock. + let actuationLockLost = false; const actuationHeartbeat = startLockHeartbeat( env, prActuationLockKeyForHeartbeat(repoFullName, pr.number), actuationLock.ownerToken, PR_ACTUATION_LOCK_TTL_SECONDS, + { + // #10019: a renewal that comes back "not ours" means another pass already re-claimed this PR's + // actuation lock (TTL lapse + re-claim, or a maintainer's forced-re-run steal, #9008). The maintenance + // pass below performs the irreversible merge/close/comment actuation, so this pass must not reach it -- + // the check right after the publish call throws before that happens. + onLost: () => { + actuationLockLost = true; + console.error( + JSON.stringify({ + level: "error", + event: "pr_actuation_lock_lost", + repoFullName, + pullNumber: pr.number, + deliveryId, + }), + ); + }, + }, ); let gate: ReturnType | undefined; try { @@ -4542,6 +4561,13 @@ export async function reReviewStoredPullRequest( ); return undefined; }); + // #10019: mirrors the initial contention check above (throw, uncaught, so the queue's attempt-free retry + // handles it) -- this pass learned mid-flight that it no longer owns the actuation lock, so it must abort + // before the maintenance pass below performs any merge/close/comment mutation instead of racing the new + // owner. + if (actuationLockLost) { + throw new PrActuationLockContendedError(repoFullName, pr.number, "actuation-lock-lost"); + } await withReviewPipelineSpan( "selfhost.review.maintenance", { @@ -10731,6 +10757,11 @@ async function maybePublishPrPublicSurface( let inlineCommentsPerCategoryForReview: number | null = null; let aiReviewExpected = false; let aiReviewWasReused = false; + // #10019: set by the AI-review lock heartbeat's onLost callback below when a renewal reports this pass no + // longer owns the key. Declared here (not inside the `if (aiReviewWillRun)` block that starts the heartbeat) + // so aiReviewCacheReadDecideAndRun -- a sibling function in this same scope -- can read it before writing + // ai_review_cache. + let aiReviewLockLost = false; let gateFinalized = false; // #6685: hoisted the same way aiReviewExpected/aiReviewWasReused are above -- assigned inside the try block // below, read at the draft-republish skip check past it (autoReviewSkipReason itself is try-block-scoped). @@ -11839,6 +11870,25 @@ async function maybePublishPrPublicSurface( aiReviewHeadSha, settings.aiReviewMode, aiReviewLock.ownerToken, + { + // #10019: a renewal that comes back "not ours" means another pass already re-claimed this lock + // (TTL lapse + re-claim, or a maintainer's forced-re-run steal, #9008) -- this pass's eventual + // verdict must not overwrite the new owner's. aiReviewCacheReadDecideAndRun checks this flag + // before persisting anything. + onLost: () => { + aiReviewLockLost = true; + console.error( + JSON.stringify({ + level: "error", + event: "ai_review_lock_lost", + repoFullName, + pullNumber: pr.number, + headSha: aiReviewHeadSha, + aiReviewMode: settings.aiReviewMode, + }), + ); + }, + }, ); try { await aiReviewCacheReadDecideAndRun(aiReviewLock); @@ -12142,95 +12192,104 @@ async function maybePublishPrPublicSurface( deliveryId: webhook.deliveryId, preComputedReputationSkip, }); - // #9016 (security): a FRESH verdict (this branch only runs on a genuine cache miss — never for a - // reused cache hit, which is not a new independent roll) is checked against the PR's flip history. - // The AI reviewer is non-deterministic, so a contributor can otherwise force re-rolls (a no-op - // recommit, or a same-head retry after the non-cacheable cooldown lapses) until a lucky CLEAN roll - // auto-merges a PR another roll flagged as blocked. Scoped to block mode only — advisory mode never - // gates on the AI verdict, so there is nothing to shop for there. Best-effort/fail-open by - // construction (recordVerdictFlip never throws); a persistable placeholder never counts as a roll. - if (aiReview && aiReview.persistable !== false && settings.aiReviewMode === "block") { - const verdictFlip = await recordVerdictFlip(env, repoFullName, pr.number, aiReview.findings ?? [], inputFingerprint); - if (verdictFlip.escalate) { - advisory.findings.push({ - code: "ai_review_inconclusive", - severity: "warning", - title: "AI review verdict has flip-flopped too many times", - detail: `This PR's AI review result has changed direction ${verdictFlip.flipCount} times across recent re-reviews of the same or similar content. Repeated re-rolls of a non-deterministic reviewer are held for a human instead of trusting the newest roll.`, - action: "A maintainer should review this PR directly, or push a substantive fix so the next review reflects real content change.", - }); - incr("loopover_ai_review_verdict_flip_escalated_total"); - await recordAuditEvent(env, { - eventType: "github_app.ai_review_verdict_flip_escalated", - actor: author, - targetKey: `${repoFullName}#${pr.number}`, - outcome: "completed", - detail: `verdict flipped ${verdictFlip.flipCount} times; held for human review`, - metadata: { deliveryId: webhook.deliveryId, repoFullName, headSha: advisory.headSha ?? null, flipCount: verdictFlip.flipCount }, - }).catch(() => undefined); - } - } - // `persistable === false` (only the lock-contention placeholder — see runAiReviewForAdvisory's return - // type doc comment) is excluded from EVERY write, not just the durable one: it describes a transient - // scheduling race, not a real AI opinion, and the concurrent pass it deferred to persists the real - // result within seconds — writing this placeholder (even non-durably) could replay a stale "another - // pass is running" message for the rest of the cooldown window, well after that race resolved. - if (aiReview && aiReview.persistable !== false) { - // A dynamic-context result is never durably cacheable (see the comment above); otherwise defer to - // the review's own verdict (consensus defect / inconclusive → false). - const cacheableForStorage = !dynamicReviewContextActive && aiReview.cacheable !== false; - if (!cacheableForStorage) { - incr("loopover_ai_review_non_cacheable_total"); - await recordAuditEvent(env, { - eventType: "github_app.ai_review_non_cacheable", - actor: author, - targetKey: `${repoFullName}#${pr.number}`, - outcome: "completed", - detail: "AI review outcome is not durably cacheable; persisted for bounded-cooldown reuse only", - metadata: { deliveryId: webhook.deliveryId, repoFullName, /* v8 ignore next -- reached only inside aiReviewWillRun (which requires a truthy advisory.headSha) or the publish-skip guard's own `advisory.headSha &&` check; the `?? null` is a type-level fallback for an unreachable branch. */ headSha: advisory.headSha ?? null }, - }).catch(() => undefined); + // #10019: the heartbeat learned mid-review that this pass no longer owns the lock (renewIfValue + // reported someone else re-claimed the key). The winning pass persists the real verdict within + // seconds, so this pass's own result must be discarded rather than racing that write -- same + // shape (and same reason) as the lock-contended placeholder a pass that never acquired the lock + // returns above. + if (aiReviewLockLost) { + aiReview = aiReviewLockContendedResult(advisory); + } else { + // #9016 (security): a FRESH verdict (this branch only runs on a genuine cache miss — never for a + // reused cache hit, which is not a new independent roll) is checked against the PR's flip history. + // The AI reviewer is non-deterministic, so a contributor can otherwise force re-rolls (a no-op + // recommit, or a same-head retry after the non-cacheable cooldown lapses) until a lucky CLEAN roll + // auto-merges a PR another roll flagged as blocked. Scoped to block mode only — advisory mode never + // gates on the AI verdict, so there is nothing to shop for there. Best-effort/fail-open by + // construction (recordVerdictFlip never throws); a persistable placeholder never counts as a roll. + if (aiReview && aiReview.persistable !== false && settings.aiReviewMode === "block") { + const verdictFlip = await recordVerdictFlip(env, repoFullName, pr.number, aiReview.findings ?? [], inputFingerprint); + if (verdictFlip.escalate) { + advisory.findings.push({ + code: "ai_review_inconclusive", + severity: "warning", + title: "AI review verdict has flip-flopped too many times", + detail: `This PR's AI review result has changed direction ${verdictFlip.flipCount} times across recent re-reviews of the same or similar content. Repeated re-rolls of a non-deterministic reviewer are held for a human instead of trusting the newest roll.`, + action: "A maintainer should review this PR directly, or push a substantive fix so the next review reflects real content change.", + }); + incr("loopover_ai_review_verdict_flip_escalated_total"); + await recordAuditEvent(env, { + eventType: "github_app.ai_review_verdict_flip_escalated", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: `verdict flipped ${verdictFlip.flipCount} times; held for human review`, + metadata: { deliveryId: webhook.deliveryId, repoFullName, headSha: advisory.headSha ?? null, flipCount: verdictFlip.flipCount }, + }).catch(() => undefined); + } } - await putCachedAiReview( - env, - repoFullName, - pr.number, - advisory.headSha, - settings.aiReviewMode, - { - ...aiReview, - cacheable: cacheableForStorage, - metadata: { - /* v8 ignore next -- runAiReviewForAdvisory (the sole path reaching here) always sets metadata on its "ok" returns; the nullish fallback is a type-level (optional field) safeguard, not a reachable runtime path. */ - ...(aiReview.metadata ?? {}), - inputFingerprint, - // #9019: `cacheable=0` conflates TWO independent, unrelated reasons -- (a) a dynamic review - // context (grounding/RAG), where the verdict itself is perfectly CONCLUSIVE but simply not - // durable across time, and (b) the review's own verdict being inconclusive/consensus- - // disputed. Only (b) should be retried; (a) is correctly reused once published (#2119). - // Recording the review's OWN verdict here keeps the two separable at read time without - // needing a schema migration, since `cacheable` alone can no longer tell them apart. - inconclusive: aiReview.cacheable === false, - // Persist line-anchored findings for post-submission MCP readback (#4519). Inline comments - // themselves are still only posted on a fresh review (see inlineFindings hoisting above); - // this metadata is read-only structured output, not a cache-replay trigger. - ...(aiReview.inlineFindings && aiReview.inlineFindings.length > 0 - ? { inlineFindings: aiReview.inlineFindings } - : {}), + // `persistable === false` (only the lock-contention placeholder — see runAiReviewForAdvisory's return + // type doc comment) is excluded from EVERY write, not just the durable one: it describes a transient + // scheduling race, not a real AI opinion, and the concurrent pass it deferred to persists the real + // result within seconds — writing this placeholder (even non-durably) could replay a stale "another + // pass is running" message for the rest of the cooldown window, well after that race resolved. + if (aiReview && aiReview.persistable !== false) { + // A dynamic-context result is never durably cacheable (see the comment above); otherwise defer to + // the review's own verdict (consensus defect / inconclusive → false). + const cacheableForStorage = !dynamicReviewContextActive && aiReview.cacheable !== false; + if (!cacheableForStorage) { + incr("loopover_ai_review_non_cacheable_total"); + await recordAuditEvent(env, { + eventType: "github_app.ai_review_non_cacheable", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: "AI review outcome is not durably cacheable; persisted for bounded-cooldown reuse only", + metadata: { deliveryId: webhook.deliveryId, repoFullName, /* v8 ignore next -- reached only inside aiReviewWillRun (which requires a truthy advisory.headSha) or the publish-skip guard's own `advisory.headSha &&` check; the `?? null` is a type-level fallback for an unreachable branch. */ headSha: advisory.headSha ?? null }, + }).catch(() => undefined); + } + await putCachedAiReview( + env, + repoFullName, + pr.number, + advisory.headSha, + settings.aiReviewMode, + { + ...aiReview, + cacheable: cacheableForStorage, + metadata: { + /* v8 ignore next -- runAiReviewForAdvisory (the sole path reaching here) always sets metadata on its "ok" returns; the nullish fallback is a type-level (optional field) safeguard, not a reachable runtime path. */ + ...(aiReview.metadata ?? {}), + inputFingerprint, + // #9019: `cacheable=0` conflates TWO independent, unrelated reasons -- (a) a dynamic review + // context (grounding/RAG), where the verdict itself is perfectly CONCLUSIVE but simply not + // durable across time, and (b) the review's own verdict being inconclusive/consensus- + // disputed. Only (b) should be retried; (a) is correctly reused once published (#2119). + // Recording the review's OWN verdict here keeps the two separable at read time without + // needing a schema migration, since `cacheable` alone can no longer tell them apart. + inconclusive: aiReview.cacheable === false, + // Persist line-anchored findings for post-submission MCP readback (#4519). Inline comments + // themselves are still only posted on a fresh review (see inlineFindings hoisting above); + // this metadata is read-only structured output, not a cache-replay trigger. + ...(aiReview.inlineFindings && aiReview.inlineFindings.length > 0 + ? { inlineFindings: aiReview.inlineFindings } + : {}), + }, }, - }, - ).catch((error) => { - // #regate-churn (req 3/9): a swallowed write failure here is exactly how the cache goes silently - // stale in production — make it observable instead of a bare no-op catch. - incr("loopover_ai_review_cache_write_error_total"); - return recordAuditEvent(env, { - eventType: "github_app.ai_review_cache_write_error", - actor: author, - targetKey: `${repoFullName}#${pr.number}`, - outcome: "error", - detail: errorMessage(error), - metadata: { deliveryId: webhook.deliveryId, repoFullName, /* v8 ignore next -- reached only inside aiReviewWillRun (which requires a truthy advisory.headSha) or the publish-skip guard's own `advisory.headSha &&` check; the `?? null` is a type-level fallback for an unreachable branch. */ headSha: advisory.headSha ?? null }, - }).catch(() => undefined); - }); + ).catch((error) => { + // #regate-churn (req 3/9): a swallowed write failure here is exactly how the cache goes silently + // stale in production — make it observable instead of a bare no-op catch. + incr("loopover_ai_review_cache_write_error_total"); + return recordAuditEvent(env, { + eventType: "github_app.ai_review_cache_write_error", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "error", + detail: errorMessage(error), + metadata: { deliveryId: webhook.deliveryId, repoFullName, /* v8 ignore next -- reached only inside aiReviewWillRun (which requires a truthy advisory.headSha) or the publish-skip guard's own `advisory.headSha &&` check; the `?? null` is a type-level fallback for an unreachable branch. */ headSha: advisory.headSha ?? null }, + }).catch(() => undefined); + }); + } } } }, diff --git a/test/unit/lock-heartbeat-onlost.test.ts b/test/unit/lock-heartbeat-onlost.test.ts new file mode 100644 index 000000000..06e5c18d5 --- /dev/null +++ b/test/unit/lock-heartbeat-onlost.test.ts @@ -0,0 +1,316 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { clearInstallationTokenCacheForTest } from "../../src/github/app"; +import { clearReviewSuppressionCacheForTest } from "../../src/review/review-memory-wire"; +import * as backfillModule from "../../src/github/backfill"; +import * as repositoriesModule from "../../src/db/repositories"; +import { + upsertInstallation, + upsertPullRequestFromGitHub, + upsertRepositoryFromGitHub, + upsertRepositorySettings, +} from "../../src/db/repositories"; +import { processJob } from "../../src/queue/processors"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; +import { fetchPullRequestFreshness } from "../../src/github/pr-freshness"; +import { createTestEnv } from "../helpers/d1"; +import { generatePrivateKeyPem } from "../helpers/github-app-key"; + +// #10019: `startLockHeartbeat`'s own renew/interval/fail-open mechanics are already exhaustively covered by +// `test/unit/transient-locks.test.ts` (INVARIANT/REGRESSION/fail-open cases). What was never wired -- and what +// this file tests -- is the CALLER side: do the two production heartbeats at src/queue/processors.ts actually +// pass an `onLost` handler, and does that handler make the pass abort/discard instead of actuating? Driving +// this through a real `setInterval` would require advancing real (600s/1800s) TTL-derived intervals, which +// `startLockHeartbeat`'s own tests already do with fake timers -- doing it again here would only re-test the +// heartbeat, not the wiring. Mocking `startLockHeartbeat` down to a synchronous `onLost()` trigger isolates +// exactly the new code: the flag it sets, and what the pass does with that flag. +const h = vi.hoisted(() => ({ + fireActuationOnLost: false, + fireAiReviewOnLost: false, +})); + +vi.mock("../../src/queue/transient-locks", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + startLockHeartbeat: vi.fn( + ( + _env: Env, + key: string, + _ownerToken: string | null, + _ttlSeconds: number, + options?: { onLost?: () => void }, + ) => { + if (key.startsWith("pr-actuation-lock:") && h.fireActuationOnLost) options?.onLost?.(); + if (key.startsWith("ai-review-lock:") && h.fireAiReviewOnLost) options?.onLost?.(); + return { stop: vi.fn() }; + }, + ), + }; +}); + +vi.mock("../../src/github/pr-freshness", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fetchPullRequestFreshness: vi.fn(async (_env: Env, args: { expectedHeadSha?: string | null }) => ({ + status: "current" as const, + liveHeadSha: args.expectedHeadSha ?? null, + liveState: "open", + liveLabels: [] as string[], + })), + }; +}); + +describe("lock-heartbeat onLost wiring at the production call sites (#10019)", () => { + beforeEach(() => { + clearInstallationTokenCacheForTest(); + clearReviewSuppressionCacheForTest(); + vi.mocked(fetchPullRequestFreshness).mockClear(); + h.fireActuationOnLost = false; + h.fireAiReviewOnLost = false; + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-05-28T00:00:00.000Z")); + // Deterministic "CI green" for every scenario here -- none of these tests are about CI-aggregation + // behavior, and leaving it to the raw check-runs/status/check-suites fetch mocks below is exactly the kind + // of incidental coupling that makes an unrelated gate input flip the merge/no-merge outcome. + vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + advisoryHoldDetails: [], + ignoredCheckDetails: [], + ciCompletenessWarning: null, + }); + // The planning pass's CI read above is separate from the live re-check agent-action-executor.ts does right + // before actuating a merge (a check flipping between planning and actuation must invalidate the plan) -- + // both need to agree "passed" or the merge step itself denies the action, independent of the lock behavior + // these tests are actually about. + vi.spyOn(backfillModule, "fetchLiveCiAggregate").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + advisoryHoldDetails: [], + ignoredCheckDetails: [], + ciCompletenessWarning: null, + }); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + }); + + it("actuation lock lost mid-pass: the publish-and-maintain unit throws PrActuationLockContendedError instead of merging", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + await upsertInstallation(env, { + action: "created", + installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { metadata: "read", contents: "write", pull_requests: "write", issues: "write" }, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "act-repo", full_name: "owner/act-repo", private: false, owner: { login: "owner" } }, 9001); + await upsertRepositorySettings(env, { repoFullName: "owner/act-repo", autonomy: { merge: "auto" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/act-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off", reviewCheckMode: "required", autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } } }); + await upsertPullRequestFromGitHub(env, "owner/act-repo", { number: 41, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a41" }, labels: [], body: "Closes #1" }); + let mergeCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/41/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/41/merge")) { + mergeCalls += 1; + return new Response(null, { status: 204 }); + } + if (url.includes("/pulls/41/reviews") && init?.method === "POST") return Response.json({ id: 1 }); + if (url.includes("/pulls/41/reviews")) return Response.json([]); + if (/\/pulls\/41(\?|$)/.test(url)) return Response.json({ number: 41, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a41" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a41/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a41/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/a41/check-suites")) return Response.json({ check_suites: [] }); + 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: [] } } }); + if (url.includes(".loopover.yml")) return new Response("Not Found", { status: 404 }); + if (url.endsWith("/check-runs") && init?.method === "POST") return Response.json({ id: 1 }); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.endsWith("/graphql")) return Response.json({ data: {} }); + return Response.json({}); + }); + + h.fireActuationOnLost = true; + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "actuation-lock-lost", repoFullName: "owner/act-repo", prNumber: 41, installationId: 9001 }), + ).rejects.toMatchObject({ name: "PrActuationLockContendedError", retryKind: "pr_actuation_lock_contended" }); + + // Lost mid-flight, not merely contended at claim time: the lock WAS acquired (publish ran), but the + // heartbeat's onLost aborted the pass before the maintenance section's irreversible merge/close mutation. + expect(mergeCalls).toBe(0); + expect( + errorSpy.mock.calls.some(([line]) => typeof line === "string" && line.includes('"event":"pr_actuation_lock_lost"') && line.includes("owner/act-repo")), + ).toBe(true); + }); + + it("actuation lock NOT lost (regression): the heartbeat firing never / onLost absent leaves the pass merging exactly as before", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + action: "created", + installation: { id: 9002, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { metadata: "read", contents: "write", pull_requests: "write", issues: "write" }, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "act-repo-2", full_name: "owner/act-repo-2", private: false, owner: { login: "owner" } }, 9002); + await upsertRepositorySettings(env, { repoFullName: "owner/act-repo-2", autonomy: { merge: "auto" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/act-repo-2", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off", reviewCheckMode: "required", autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } } }); + await upsertPullRequestFromGitHub(env, "owner/act-repo-2", { number: 42, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a42" }, labels: [], body: "Closes #1" }); + let mergeCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/42/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/42/merge")) { + mergeCalls += 1; + return new Response(null, { status: 204 }); + } + if (url.includes("/pulls/42/reviews") && init?.method === "POST") return Response.json({ id: 1 }); + if (url.includes("/pulls/42/reviews")) return Response.json([]); + if (/\/pulls\/42(\?|$)/.test(url)) return Response.json({ number: 42, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a42" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a42/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a42/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/a42/check-suites")) return Response.json({ check_suites: [] }); + 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: [] } } }); + if (url.includes(".loopover.yml")) return new Response("Not Found", { status: 404 }); + if (url.endsWith("/check-runs") && init?.method === "POST") return Response.json({ id: 1 }); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.endsWith("/graphql")) return Response.json({ data: {} }); + return Response.json({}); + }); + + // h.fireActuationOnLost stays false (default) -- the fail-open posture (an adapter without renewIfValue, or + // a renewal that keeps confirming ownership) never calls onLost. Nothing here should differ from the + // pre-#10019 behavior: the pass completes and actuates normally. + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "actuation-lock-not-lost", repoFullName: "owner/act-repo-2", prNumber: 42, installationId: 9002 }), + ).resolves.toBeUndefined(); + expect(mergeCalls).toBeGreaterThan(0); + }); + + it("AI-review lock lost mid-review: the pass discards its verdict for the lock-contended placeholder and never writes ai_review_cache", 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", + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const putCachedAiReviewSpy = vi.spyOn(repositoriesModule, "putCachedAiReview"); + await upsertInstallation(env, { + action: "created", + installation: { id: 9003, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { metadata: "read", contents: "write", pull_requests: "write", issues: "write" }, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "ai-repo", full_name: "owner/ai-repo", private: false, owner: { login: "owner" } }, 9003); + await upsertRepositorySettings(env, { repoFullName: "owner/ai-repo", autonomy: { close: "auto", merge: "auto" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/ai-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required", aiReviewMode: "block", autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } } }); + await upsertPullRequestFromGitHub(env, "owner/ai-repo", { number: 55, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a55" }, labels: [], body: "Closes #1" }); + let mergeCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/55/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/55/merge")) { + mergeCalls += 1; + return new Response(null, { status: 204 }); + } + if (url.includes("/pulls/55/reviews") && method === "POST") return Response.json({ id: 1 }); + if (url.includes("/pulls/55/reviews")) return Response.json([]); + if (/\/pulls\/55(\?|$)/.test(url)) return Response.json({ number: 55, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a55" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a55/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a55/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/a55/check-suites")) return Response.json({ check_suites: [] }); + 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: [] } } }); + if (url.includes(".loopover.yml")) return new Response("Not Found", { status: 404 }); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 1 }); + if (url.endsWith("/graphql")) return Response.json({ data: {} }); + return Response.json({}); + }); + + h.fireAiReviewOnLost = true; + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "ai-review-lock-lost", repoFullName: "owner/ai-repo", prNumber: 55, installationId: 9003 }), + ).resolves.toBeUndefined(); + + // The LLM call genuinely ran (this is the fresh-review path, not the never-acquired short-circuit), but its + // result was discarded once the heartbeat reported the lock lost -- so nothing was ever written to the cache. + expect(aiCalls).toBeGreaterThan(0); + expect(putCachedAiReviewSpy).not.toHaveBeenCalled(); + // The lock-contended placeholder holds the gate, so the losing pass never merges either. + expect(mergeCalls).toBe(0); + expect( + errorSpy.mock.calls.some(([line]) => typeof line === "string" && line.includes('"event":"ai_review_lock_lost"') && line.includes("owner/ai-repo")), + ).toBe(true); + }); + + it("AI-review lock NOT lost (regression): a completed pass with no onLost fired still writes ai_review_cache exactly as before", 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", + }); + const putCachedAiReviewSpy = vi.spyOn(repositoriesModule, "putCachedAiReview"); + await upsertInstallation(env, { + action: "created", + installation: { id: 9004, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { metadata: "read", contents: "write", pull_requests: "write", issues: "write" }, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "ai-repo-2", full_name: "owner/ai-repo-2", private: false, owner: { login: "owner" } }, 9004); + await upsertRepositorySettings(env, { repoFullName: "owner/ai-repo-2", autonomy: { close: "auto", merge: "auto" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/ai-repo-2", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required", aiReviewMode: "block", autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } } }); + await upsertPullRequestFromGitHub(env, "owner/ai-repo-2", { number: 56, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a56" }, labels: [], body: "Closes #1" }); + let mergeCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/56/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/56/merge")) { + mergeCalls += 1; + return new Response(null, { status: 204 }); + } + if (url.includes("/pulls/56/reviews") && method === "POST") return Response.json({ id: 1 }); + if (url.includes("/pulls/56/reviews")) return Response.json([]); + if (/\/pulls\/56(\?|$)/.test(url)) return Response.json({ number: 56, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a56" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a56/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a56/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/a56/check-suites")) return Response.json({ check_suites: [] }); + 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: [] } } }); + if (url.includes(".loopover.yml")) return new Response("Not Found", { status: 404 }); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 1 }); + if (url.endsWith("/graphql")) return Response.json({ data: {} }); + return Response.json({}); + }); + + // Both flags stay false: neither heartbeat's onLost fires, so this must behave byte-identically to the + // pre-#10019 pass -- a fresh review that persists, and a gate-clean merge. + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "ai-review-lock-not-lost", repoFullName: "owner/ai-repo-2", prNumber: 56, installationId: 9004 }), + ).resolves.toBeUndefined(); + + expect(aiCalls).toBeGreaterThan(0); + expect(putCachedAiReviewSpy).toHaveBeenCalledTimes(1); + expect(mergeCalls).toBeGreaterThan(0); + }); +});