From b6402bdbe074b2f052f02f32428a4b802d86028b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:13:03 -0700 Subject: [PATCH 1/2] fix(review): auto-clear manual-review label when AI-review lock contention resolves manual-review has no removal path today, so once applied it sticks forever even when its underlying cause was a purely transient infra artifact. Narrow scope: only auto-clears for the one case with live evidence of being transient -- aiReviewLockContendedResult's "AI review already in progress for this PR head" finding (a pass losing the AI-review lock race, #8999). Every other hold reason (guardrail, migration collision, breaker downgrades, a human's own hold, ...) still requires a maintainer to lift it. A (repo, PR)-keyed transient marker (24h TTL) records when contention fires; the next pass clears the marker once contention no longer shows up in that pass's gate warnings, and planAgentMaintenanceActions removes the label only when nothing else currently justifies the hold. Closes #9009 --- src/queue/processors.ts | 24 ++++++++++++ src/settings/agent-actions.ts | 34 +++++++++++++++++ test/unit/agent-actions.test.ts | 65 +++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 893fa86e24..0825d84b64 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2499,6 +2499,7 @@ function buildAgentMaintenancePlanInput(args: { pr: PullRequestRecord; openDuplicateSiblings: ReturnType; duplicateWinnerEnabled: boolean; + manualReviewLockContentionResolved: AgentActionPlanInput["manualReviewLockContentionResolved"]; }): AgentActionPlanInput { const { gate, @@ -2524,6 +2525,7 @@ function buildAgentMaintenancePlanInput(args: { pr, openDuplicateSiblings, duplicateWinnerEnabled, + manualReviewLockContentionResolved, } = args; return { conclusion: gate.conclusion, @@ -2590,6 +2592,7 @@ function buildAgentMaintenancePlanInput(args: { ...(unlinkedIssueMatchHold !== undefined ? { unlinkedIssueMatchHold } : {}), ...(aiReviewLowConfidenceHold !== undefined ? { aiReviewLowConfidenceHold } : {}), ...(unlinkedIssueMatchClose !== undefined ? { unlinkedIssueMatchClose } : {}), + manualReviewLockContentionResolved, pr: { mergeableState: liveMergeState ?? pr.mergeableState, reviewDecision: liveReviewDecision ?? pr.reviewDecision, @@ -3204,6 +3207,26 @@ async function runAgentMaintenancePlanAndExecute( aiReviewLowConfidenceHold === undefined && settings.aiReviewSalvageabilityMinScore != null ? resolveAiReviewSalvageableHold(gate, settings, await computeSalvageabilityForTarget(env, repoFullName, pr.number, pr.authorLogin, gate)) : undefined; + // #9009 (narrow scope): track whether manual-review is being held specifically for AI-review lock contention + // (aiReviewLockContendedResult's exact finding shape, ai-review-orchestration.ts) so it can be auto-cleared + // once that transient contention resolves -- see planAgentMaintenanceActions' own doc comment on + // manualReviewLockContentionResolved for why this is the ONLY provenance-tracked auto-clear. The marker is + // keyed per (repo, PR) -- not per head SHA -- since the label persists across pushes and a real verdict may + // only land on a LATER head; a generous 24h TTL means a contention that hasn't cleared naturally in a day no + // longer counts as purely transient and reverts to requiring a maintainer, matching every other hold class. + const manualReviewLockContentionMarkerKey = `manual-review-lock-contention:${repoFullName.toLowerCase()}#${pr.number}`; + const hadPriorLockContentionHold = Boolean(await getTransientKey(env, manualReviewLockContentionMarkerKey)); + const aiReviewLockContentionThisPass = gate.warnings.some( + (finding) => finding.code === "ai_review_inconclusive" && finding.title === "AI review already in progress for this PR head", + ); + if (aiReviewLockContentionThisPass) { + await putTransientKey(env, manualReviewLockContentionMarkerKey, "1", 24 * 60 * 60); + } else if (hadPriorLockContentionHold) { + // Consumed regardless of what else fires this pass -- once contention itself has cleared, the marker's + // OWN reason is stale; a different reason now co-occurring (if any) must stand on its own from here on, + // not inherit this transient provenance on some future pass. + await deleteTransientKey(env, manualReviewLockContentionMarkerKey); + } const planned = planAgentMaintenanceActions( buildAgentMaintenancePlanInput({ gate, @@ -3229,6 +3252,7 @@ async function runAgentMaintenancePlanAndExecute( pr, openDuplicateSiblings, duplicateWinnerEnabled, + manualReviewLockContentionResolved: hadPriorLockContentionHold && !aiReviewLockContentionThisPass, }), ); // Accuracy circuit-breakers (#self-improve / GAP-4): two INDEPENDENT, fail-open precision breakers, chained. diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 768656b863..90cac036e4 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -279,6 +279,16 @@ export type AgentActionPlanInput = { changesRequestedLabel?: string | null | undefined; migrationCollisionLabel?: string | null | undefined; pendingClosureLabel?: string | null | undefined; + // #9009 (narrow scope): true when a transient marker recorded that manual-review was held/applied because a + // PRIOR pass lost the AI-review lock race (a genuinely transient infra artifact -- see + // aiReviewLockContendedResult, ai-review-orchestration.ts), AND this pass's fresh gate evaluation no longer + // shows that specific contention (the caller clears the marker the moment contention resolves, regardless of + // whether this flag ends up removing the label -- see buildAgentMaintenancePlanInput's caller). Deliberately + // the ONLY provenance-tracked auto-clear condition: every OTHER manual-review trigger (guardrail, migration + // collision, breaker downgrades, unstable-mergeable, a human's own hold, ...) still requires a maintainer to + // lift it, exactly as before -- see the stale-disposition-label-cleanup comment below for why a general + // provenance-free auto-clear is unsafe. Absent/false ⇒ byte-identical to pre-#9009 behavior. + manualReviewLockContentionResolved?: boolean | undefined; // True when the PR author is the repo owner (e.g. JSONbored). Standing rule: owner PRs are NEVER // auto-closed. They may still auto-merge when clean + passing. authorIsOwner: boolean; @@ -1540,5 +1550,29 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne }); } + // #9009 (narrow scope): auto-clear manual-review when the caller's transient marker proves this exact hold + // was applied because a prior pass lost the AI-review lock race, and this fresh pass shows the contention + // has cleared. `manualHoldReason === null` is the load-bearing safety check here: it means NOTHING else this + // pass (guardrail, migration collision, unverified CI, action-required conclusion, a not-review-good verdict, + // ...) currently justifies the label, so removing it cannot silently drop a substantive or human-applied + // hold that happens to coexist. If manualHoldReason is non-null, the block above already re-adds/keeps the + // label for that OTHER reason this same pass, so this branch is naturally a no-op there. + if ( + input.manualReviewLockContentionResolved === true && + manualHoldReason === null && + labels.manualReview !== null && + hasLabel(input.pr.labels, labels.manualReview) && + !actions.some((action) => action.actionClass === "label" && action.label?.toLowerCase() === labels.manualReview?.toLowerCase()) + ) { + actions.push({ + actionClass: "label", + autonomyClass: manualHoldAutonomyClass, + requiresApproval: approval(manualHoldAutonomyClass), + reason: "AI-review lock contention has cleared and nothing else currently holds this PR — removing the transient manual-review hold", + label: labels.manualReview, + labelOp: "remove", + }); + } + return actions; } diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 2398820308..ffb3b66d4e 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -139,6 +139,71 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(classes(plan)).toEqual(["label", "approve", "merge"]); }); + describe("manual-review lock-contention auto-clear (#9009, narrow scope)", () => { + // Same base scenario as the "does NOT clear" test immediately above (conclusion success, clean, the + // label already live) -- the ONLY difference in each case is manualReviewLockContentionResolved. This is + // the exact truth table the caller-side marker (processors.ts) is responsible for computing correctly; + // here we pin what the PURE planner does once given each value. + const base = { conclusion: "success" as const, autonomy: { approve: "auto", merge: "auto", review_state_label: "auto" } as const, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } as const }; + + it("clears manual-review when the marker says lock contention resolved and nothing else holds the PR", () => { + const plan = planAgentMaintenanceActions(input({ ...base, pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "clean" }, manualReviewLockContentionResolved: true })); + expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "remove" })); + }); + + it("does NOT clear when manualReviewLockContentionResolved is absent (byte-identical to pre-#9009)", () => { + // Identical to the test above in every other respect -- proves the new behavior is opt-in via the flag, + // not a silent widening of the existing "does NOT clear" test just above. + const plan = planAgentMaintenanceActions(input({ ...base, pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "clean" } })); + expect(plan.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "remove")).toBe(false); + }); + + it("does NOT clear when a DIFFERENT reason (a guardrail hit) still holds the PR this same pass", () => { + // The load-bearing safety property: manualHoldReason non-null (guardrailHit here) means something ELSE + // currently justifies the label, so the resolved-lock-contention marker must never override it. The + // label is already correctly live (guardrail's own add is a no-op via hasLabelOrPlanned), so the plan + // is empty either way -- what matters is that it is NOT a "remove". + const plan = planAgentMaintenanceActions( + input({ + conclusion: "success", + autonomy: { merge: "auto", review_state_label: "auto" }, + changedPaths: ["src/settings/agent-actions.ts"], + hardGuardrailGlobs: ["src/settings/**"], + pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "clean" }, + manualReviewLockContentionResolved: true, + }), + ); + expect(plan.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "remove")).toBe(false); + expect(plan).toEqual([]); + + // Sharper version of the same property: guardrailHit, but the label is NOT yet live (a earlier pass's + // marker predates the guardrail path ever adding it). Here the guardrail's own add DOES fire, proving + // the "resolved" marker never suppresses a genuine, currently-justified hold from being (re)applied. + const withoutLabelYet = planAgentMaintenanceActions( + input({ + conclusion: "success", + autonomy: { merge: "auto", review_state_label: "auto" }, + changedPaths: ["src/settings/agent-actions.ts"], + hardGuardrailGlobs: ["src/settings/**"], + pr: { labels: [], mergeableState: "clean" }, + manualReviewLockContentionResolved: true, + }), + ); + expect(withoutLabelYet).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "add" })); + expect(withoutLabelYet.some((a) => a.actionClass === "label" && a.labelOp === "remove")).toBe(false); + }); + + it("plans no spurious removal when the label isn't currently on the PR", () => { + const plan = planAgentMaintenanceActions(input({ ...base, pr: { labels: [], mergeableState: "clean" }, manualReviewLockContentionResolved: true })); + expect(plan.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "remove")).toBe(false); + }); + + it("respects a configured manual-review label name, not just the engine default", () => { + const plan = planAgentMaintenanceActions(input({ ...base, manualReviewLabel: "human-review", pr: { labels: ["human-review"], mergeableState: "clean" }, manualReviewLockContentionResolved: true })); + expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: "human-review", labelOp: "remove" })); + }); + }); + it("clears a stale ready-to-merge label when the PR newly becomes guarded", () => { const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto", review_state_label: "auto" }, changedPaths: ["src/settings/agent-actions.ts"], hardGuardrailGlobs: ["src/settings/**"], pr: { labels: [AGENT_LABEL_READY], mergeableState: "clean" } })); expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "add" })); From f2565a1df5e69652c18da15ef1ac1bdf6fe6d280 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:30:40 -0700 Subject: [PATCH 2/2] fix(engine): sync the gate-decision twin after #9082's host-only secret_scan_incomplete change #9082/#9103 (merged) added the secret_scan_incomplete neutral-hold branch to src/rules/advisory.ts's evaluateGateCheckCore, plus an updated secret_leak provenance comment, but never touched the hand-duplicated engine-package twin (packages/loopover-engine/src/advisory/gate-advisory.ts) -- the two are supposed to stay logically identical (see check-engine-parity.ts's GATE_DECISION_TWIN_PAIR). Confirmed the drift is live on main right now (the engine copy is missing the whole branch), which is exactly what stranded PR #9103's own CI: the version-bump-or-co-edit guard (checkGateDecisionVersionBump) correctly flagged the host-only edit at merge time. Mirrors both pieces into the engine twin and bumps @loopover/engine 3.15.0 -> 3.15.1 (the guard's own documented escape hatch for a one-sided fix landing after the fact), syncing package-lock.json and the miner's expected-engine.version pin alongside it -- same shape as the precedent manual bump in bc9e81abd. --- package-lock.json | 2 +- packages/loopover-engine/package.json | 2 +- .../src/advisory/gate-advisory.ts | 30 ++- .../loopover-miner/expected-engine.version | 2 +- test/unit/queue-2.test.ts | 181 ++++++++++++++++++ 5 files changed, 211 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index 837c5ba41e..f11413e6d2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21029,7 +21029,7 @@ }, "packages/loopover-engine": { "name": "@loopover/engine", - "version": "3.15.0", + "version": "3.15.1", "license": "AGPL-3.0-only", "dependencies": { "@anthropic-ai/claude-agent-sdk": "^0.3.205", diff --git a/packages/loopover-engine/package.json b/packages/loopover-engine/package.json index 5c3d191887..c32af23df3 100644 --- a/packages/loopover-engine/package.json +++ b/packages/loopover-engine/package.json @@ -1,6 +1,6 @@ { "name": "@loopover/engine", - "version": "3.15.0", + "version": "3.15.1", "license": "AGPL-3.0-only", "type": "module", "description": "Shared deterministic engine logic for the LoopOver review stack and loopover-miner.", diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index 3e5fe8a52b..f21d9dd014 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -520,6 +520,25 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy warnings: gateWarnings, }; } + // Incomplete secret scan (#9082): mirrors the ai_review_inconclusive hold immediately above -- checked only + // once NO deterministic blocker fired (blockers.length === 0), so a REAL secret_leak match (or any other + // configured blocker) on a DIFFERENT file in the same PR still hard-blocks; this can never bury a genuine + // leak in a hold. A patch-less file's content couldn't be fetched/verified within the scan cap -- absence + // of evidence, not evidence of a leak -- so it HOLDS (never closes) and re-evaluates automatically once the + // content becomes retrievable. Explicitly appended to `warnings` (not merged into the severity-filtered + // `gateWarnings`) because the finding is `critical` severity by design (still worth a prominent panel + // signal) rather than `warning`, mirroring the size/guardrail hold shape below. + const secretScanIncompleteFindings = advisoryResult.findings.filter((finding) => finding.code === "secret_scan_incomplete"); + if (secretScanIncompleteFindings.length > 0) { + return { + enabled: true, + conclusion: "neutral", + title: `${LOOPOVER_GATE_CHECK_NAME} — held for human review`, + summary: secretScanIncompleteFindings.map((finding) => sanitizeForCheckRun(finding.title)).join("; "), + blockers: [], + warnings: [...gateWarnings, ...secretScanIncompleteFindings], + }; + } // Manual-review HOLD (#gate-size / #gate-guardrail): a PR that would otherwise PASS but is oversized or touches // a guarded path is HELD for a human (neutral → "manual" verdict) rather than auto-approved — never a failure, // so neutral never blocks the merge (dry-run/advisory friendly) and a contributor PR is never auto-closed for size. @@ -608,9 +627,14 @@ function isConfiguredGateBlocker(finding: AdvisoryFinding, policy: GateCheckPoli } if (code === REVIEW_THREAD_BLOCKER_CODE) return true; // A leaked-secret finding (`secret_leak`) ALWAYS hard-blocks: a committed credential must be removed and - // rotated before merge, with no opt-in. This finding is produced ONLY by the flag-gated safety scan - // (LOOPOVER_REVIEW_SAFETY); when the flag is off the finding never exists, so this branch is unreachable and the - // gate verdict is byte-identical to today. + // rotated before merge, with no opt-in. Two independent producers feed this exact code: the flag-gated + // safety scan (LOOPOVER_REVIEW_SAFETY, off by default) over the reviewed diff, and the UNCONDITIONAL + // patch-less scan (maybeAddSecretLeakFinding, #audit-3.4) that runs for every repo regardless of that flag. + // Either way this code means a CONCRETE match was found. `secret_scan_incomplete` (#9082) is the sibling + // finding from that same patch-less scan for the OPPOSITE case -- verification could not complete, not a + // match -- and is deliberately never routed here: it never reaches this function at all (it resolves via + // the default "off" case below), because it's handled earlier, in evaluateGateCheckCore's + // no-deterministic-blocker branch, as a neutral hold rather than a configured gate blocker. if (code === "secret_leak") return true; // A maintainer pre-merge check (#review-pre-merge-checks) marked `enforce: true` produces this DETERMINISTIC // finding when it fails (a required title/description phrase or label is missing). It always blocks: the diff --git a/packages/loopover-miner/expected-engine.version b/packages/loopover-miner/expected-engine.version index f02113fe87..c3df54c9b8 100644 --- a/packages/loopover-miner/expected-engine.version +++ b/packages/loopover-miner/expected-engine.version @@ -1 +1 @@ -3.15.0 +3.15.1 diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index a08ca69c22..0633211e59 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -380,6 +380,187 @@ describe("queue processors", () => { expect(outcome.verdict).toBe("match"); }); + it("#9009: auto-clears manual-review once AI-review lock contention (the pass that applied it) resolves on a later 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 upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001); + await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { close: "auto", merge: "auto" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required", aiReviewMode: "block", autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } } }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 91, title: "Contention label autoclear", state: "open", user: { login: "contributor" }, head: { sha: "s91" }, labels: [], body: "Closes #1" }); + await upsertPullRequestFile(env, { repoFullName: "owner/agent-repo", pullNumber: 91, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = true;" } }); + vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + advisoryHoldDetails: [], + ciCompletenessWarning: null, + }); + + // Stateful label list, mutated by the real ensurePullRequestLabel/removePullRequestLabel POST/DELETE calls -- + // mirrors the linked-issue-hard-rule scaffold above -- so pass 2's live re-sync of the PR sees whatever + // pass 1 actually applied, exactly like a real GitHub repo persists label state between two separate passes. + let mergeCalls = 0; + const liveLabels: string[] = []; + 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/91/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/91/merge")) { + mergeCalls += 1; + return new Response(null, { status: 204 }); + } + if (url.includes("/pulls/91/reviews") && method === "POST") return Response.json({ id: 1 }); + if (url.includes("/pulls/91/reviews")) return Response.json([]); + if (/\/pulls\/91(?:\?|$)/.test(url)) return Response.json({ number: 91, title: "Contention label autoclear", state: "open", user: { login: "contributor" }, head: { sha: "s91" }, labels: liveLabels.map((name) => ({ name })), body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/s91/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/s91/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/s91/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/1") && !url.includes("/issues/91")) 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: {} }); + if (url.includes("/issues/91/labels") && method === "GET") return Response.json(liveLabels.map((name) => ({ name }))); + if (url.includes("/issues/91/labels") && method === "POST") { + const body = init?.body ? (JSON.parse(String(init.body)) as { labels?: string[] }) : {}; + for (const label of body.labels ?? []) if (!liveLabels.includes(label)) liveLabels.push(label); + return Response.json(liveLabels.map((name) => ({ name })), { status: 200 }); + } + if (url.includes("/labels/") && method === "DELETE") { + const removed = decodeURIComponent(url.slice(url.lastIndexOf("/labels/") + "/labels/".length)); + const index = liveLabels.indexOf(removed); + if (index >= 0) liveLabels.splice(index, 1); + return new Response(null, { status: 204 }); + } + return Response.json({}); + }); + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + + const markerKey = "manual-review-lock-contention:owner/agent-repo#91"; + + // Pass 1: a DIFFERENT holder wins the AI-review lock race for this exact (repo, PR, head, mode) first -- + // this pass's own claim inside runAgentMaintenancePlanAndExecute loses, producing the + // ai_review_inconclusive / "AI review already in progress" finding (aiReviewLockContendedResult). + const contender = await claimAiReviewLock(env, "owner/agent-repo", 91, "s91", "block"); + expect(contender.acquired).toBe(true); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "pass-1-contended", repoFullName: "owner/agent-repo", prNumber: 91, installationId: 9001 }); + + expect(mergeCalls).toBe(0); // held (neutral), never merged + expect(liveLabels).toContain("manual-review"); + expect(await env.SELFHOST_TRANSIENT_CACHE?.get?.(markerKey)).toBe("1"); + + // The contending pass finishes and releases its claim -- the lock is free again for pass 2. + await releaseAiReviewLock(env, "owner/agent-repo", 91, "s91", "block", contender.ownerToken); + + // Pass 2: SAME head SHA, no push in between. This pass now wins the lock itself, runs a clean AI review + // (the stubbed AI.run above returns no blockers), and the gate passes outright -- contention has resolved + // AND nothing else this pass justifies the hold, so planAgentMaintenanceActions clears the stale label. + await processJob(env, { type: "agent-regate-pr", deliveryId: "pass-2-resolved", repoFullName: "owner/agent-repo", prNumber: 91, installationId: 9001 }); + + expect(liveLabels).not.toContain("manual-review"); + expect(await env.SELFHOST_TRANSIENT_CACHE?.get?.(markerKey)).toBeNull(); + }); + + it("#9009: does NOT clear manual-review across passes when a DIFFERENT reason (a guardrail hit) still holds the PR", 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 upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001); + // A hard-guardrail glob match on the changed path (below) HOLDS the gate independent of AI-review lock + // contention -- the marker's provenance must never suppress THIS reason once contention itself clears. + await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { close: "auto", merge: "auto" }, gatePack: "oss-anti-slop" }); + await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required", aiReviewMode: "block", hardGuardrailGlobs: ["src/guarded/**"], autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } } }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 92, title: "Contention + guardrail", state: "open", user: { login: "contributor" }, head: { sha: "s92" }, labels: [], body: "Closes #1" }); + await upsertPullRequestFile(env, { repoFullName: "owner/agent-repo", pullNumber: 92, path: "src/guarded/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = true;" } }); + vi.spyOn(backfillModule, "fetchRequiredStatusContexts").mockResolvedValue(null); + vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + advisoryHoldDetails: [], + ciCompletenessWarning: null, + }); + + let mergeCalls = 0; + const liveLabels: string[] = []; + 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/92/files")) return Response.json([{ filename: "src/guarded/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/92/merge")) { + mergeCalls += 1; + return new Response(null, { status: 204 }); + } + if (url.includes("/pulls/92/reviews") && method === "POST") return Response.json({ id: 1 }); + if (url.includes("/pulls/92/reviews")) return Response.json([]); + if (/\/pulls\/92(?:\?|$)/.test(url)) return Response.json({ number: 92, title: "Contention + guardrail", state: "open", user: { login: "contributor" }, head: { sha: "s92" }, labels: liveLabels.map((name) => ({ name })), body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/s92/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/s92/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/s92/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/1") && !url.includes("/issues/92")) 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: {} }); + if (url.includes("/issues/92/labels") && method === "GET") return Response.json(liveLabels.map((name) => ({ name }))); + if (url.includes("/issues/92/labels") && method === "POST") { + const body = init?.body ? (JSON.parse(String(init.body)) as { labels?: string[] }) : {}; + for (const label of body.labels ?? []) if (!liveLabels.includes(label)) liveLabels.push(label); + return Response.json(liveLabels.map((name) => ({ name })), { status: 200 }); + } + if (url.includes("/labels/") && method === "DELETE") { + const removed = decodeURIComponent(url.slice(url.lastIndexOf("/labels/") + "/labels/".length)); + const index = liveLabels.indexOf(removed); + if (index >= 0) liveLabels.splice(index, 1); + return new Response(null, { status: 204 }); + } + return Response.json({}); + }); + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + + const markerKey = "manual-review-lock-contention:owner/agent-repo#92"; + const contender = await claimAiReviewLock(env, "owner/agent-repo", 92, "s92", "block"); + expect(contender.acquired).toBe(true); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "pass-1-contended", repoFullName: "owner/agent-repo", prNumber: 92, installationId: 9001 }); + expect(liveLabels).toContain("manual-review"); + expect(await env.SELFHOST_TRANSIENT_CACHE?.get?.(markerKey)).toBe("1"); + + await releaseAiReviewLock(env, "owner/agent-repo", 92, "s92", "block", contender.ownerToken); + + // Pass 2: the lock is free and the AI review comes back clean -- contention itself has resolved -- but the + // guardrail hit on src/guarded/a.ts is untouched and still independently justifies the hold. The label must + // stay. + await processJob(env, { type: "agent-regate-pr", deliveryId: "pass-2-guardrail-still-hits", repoFullName: "owner/agent-repo", prNumber: 92, installationId: 9001 }); + + expect(mergeCalls).toBe(0); + expect(liveLabels).toContain("manual-review"); // still held, for the OTHER reason + expect(await env.SELFHOST_TRANSIENT_CACHE?.get?.(markerKey)).toBeNull(); // contention's OWN marker still consumed + }); + it("#4603: the SAME sub-floor defect one-shot-closes when aiReviewLowConfidenceDisposition is explicitly one_shot", async () => { let aiCalls = 0; const env = createTestEnv({