fix(review): auto-clear stale manual-review lock-contention hold; sync engine gate-decision twin

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.",

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

packages/loopover-miner/expected-engine.version

@@ -1 +1 @@
-3.15.0
+3.15.1

src/queue/processors.ts

@@ -2499,6 +2499,7 @@ function buildAgentMaintenancePlanInput(args: {
   pr: PullRequestRecord;
   openDuplicateSiblings: ReturnType<typeof linkedIssueDuplicatePullRequestRecordsForGate>;
   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.

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;
 }

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" }));