diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index 40ef85196..540b058f5 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -582,7 +582,13 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy // automatically — NEVER a failure, so a contributor PR is never auto-CLOSED because a model hiccupped. This // is evaluated AFTER the deterministic blockers above, so a real violation (secret_leak, duplicate, // missing-issue, slop, quality) still blocks: an inconclusive AI can no longer bury a blocked PR in a hold. - if (advisoryResult.findings.some((finding) => finding.code === "ai_review_inconclusive")) { + // #10016: mirrors isEvaluationBlocker's CLA case -- the finding is produced in every aiReviewGateMode (the + // two producers in ai-review-orchestration.ts don't consult the mode), so only "block" should ever HOLD the + // gate on it. "advisory"/"off" mode's whole contract is "surface findings, never affect the verdict"; + // unconditionally holding here diverted a clean, green advisory-mode PR to manual review over a race or a + // transient model failure the repo never opted into blocking on. The finding still reaches the panel via + // `gateWarnings` below either way -- only the HOLD is mode-gated, never the visibility. + if (gatePolicyBlocks(effective.aiReviewGateMode, "advisory") && advisoryResult.findings.some((finding) => finding.code === "ai_review_inconclusive")) { return { enabled: true, conclusion: "neutral", diff --git a/packages/loopover-engine/test/gate-advisory-ai-inconclusive-hold.test.ts b/packages/loopover-engine/test/gate-advisory-ai-inconclusive-hold.test.ts new file mode 100644 index 000000000..4de9c1061 --- /dev/null +++ b/packages/loopover-engine/test/gate-advisory-ai-inconclusive-hold.test.ts @@ -0,0 +1,69 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { evaluateGateCheck } from "../dist/advisory/gate-advisory.js"; +import type { Advisory, AdvisoryFinding } from "../dist/types/predicted-gate-types.js"; + +function inconclusiveAdvisory(extraFindings: AdvisoryFinding[] = []): Advisory { + const finding: AdvisoryFinding = { + code: "ai_review_inconclusive", + title: "AI review could not complete for this PR head", + severity: "warning", + detail: "The AI review attempt did not produce a result.", + action: "The review is retried automatically after a short cooldown.", + }; + return { + id: "advisory-1", + targetType: "pull_request", + targetKey: "JSONbored/loopover#1", + repoFullName: "JSONbored/loopover", + conclusion: "success", + severity: "warning", + title: "LoopOver review", + summary: "", + findings: [finding, ...extraFindings], + generatedAt: "2026-07-31T00:00:00.000Z", + }; +} + +// #10016 (gate-decision twin of src/rules/advisory.ts, kept in sync per checkGateDecisionVersionBump): mirrors +// the host copy's own regression tests for the ai_review_inconclusive hold's aiReviewGateMode gating. + +test("REGRESSION: an inconclusive review does not hold an advisory-mode repo's otherwise-clean gate", () => { + const evaluation = evaluateGateCheck(inconclusiveAdvisory()); + assert.equal(evaluation.conclusion, "success"); + assert.ok(evaluation.warnings.some((finding) => finding.code === "ai_review_inconclusive")); +}); + +test("stays non-blocking under an explicit advisory or off mode", () => { + const advisory = inconclusiveAdvisory(); + const advisoryMode = evaluateGateCheck(advisory, { aiReviewGateMode: "advisory" }); + assert.equal(advisoryMode.conclusion, "success"); + assert.ok(advisoryMode.warnings.some((finding) => finding.code === "ai_review_inconclusive")); + + const offMode = evaluateGateCheck(advisory, { aiReviewGateMode: "off" }); + assert.equal(offMode.conclusion, "success"); + assert.ok(offMode.warnings.some((finding) => finding.code === "ai_review_inconclusive")); +}); + +test("still HOLDS the gate (neutral) under aiReviewGateMode: block, with the unchanged title", () => { + const evaluation = evaluateGateCheck(inconclusiveAdvisory(), { aiReviewGateMode: "block" }); + assert.equal(evaluation.conclusion, "neutral"); + assert.equal(evaluation.title, "LoopOver Orb Review Agent — held for human review"); + assert.equal(evaluation.blockers.length, 0); +}); + +test("the unconditional secret_scan_incomplete hold still fires once the AI hold no longer does", () => { + const advisory = inconclusiveAdvisory([ + { + code: "secret_scan_incomplete", + title: "Patch-less file(s) could not be fully scanned for secrets (1)", + severity: "critical", + detail: "GitHub omitted inline diff for: secrets.env.", + action: "Ensure patch-less files are within scan limits or split the change so secrets can be verified.", + }, + ]); + const evaluation = evaluateGateCheck(advisory); + assert.equal(evaluation.conclusion, "neutral"); + assert.equal(evaluation.blockers.length, 0); +}); diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index d76363c3a..904cd2a35 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -838,7 +838,16 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy // automatically — NEVER a failure, so a contributor PR is never auto-CLOSED because a model hiccupped. This // is evaluated AFTER the deterministic blockers above, so a real violation (secret_leak, duplicate, // missing-issue, slop, quality) still blocks: an inconclusive AI can no longer bury a blocked PR in a hold. - if (advisoryResult.findings.some((finding) => finding.code === "ai_review_inconclusive")) { + // #10016: mirrors isEvaluationBlocker's CLA case -- the finding is produced in every aiReviewGateMode (the + // two producers in ai-review-orchestration.ts don't consult the mode), so only "block" should ever HOLD the + // gate on it. "advisory"/"off" mode's whole contract is "surface findings, never affect the verdict"; + // unconditionally holding here diverted a clean, green advisory-mode PR to manual review over a race or a + // transient model failure the repo never opted into blocking on. The finding still reaches the panel via + // `gateWarnings` below either way -- only the HOLD is mode-gated, never the visibility. + if ( + gateMode(effective.aiReviewGateMode ?? "advisory") === "block" && + advisoryResult.findings.some((finding) => finding.code === "ai_review_inconclusive") + ) { return { enabled: true, conclusion: "neutral", diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index 4f7df80c0..832697954 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -157,12 +157,14 @@ describe(".loopover.yml settings override (resolveEffectiveSettings)", () => { }); describe("AI fail-closed hold (#ai-fail-closed)", () => { - it("holds the gate NEUTRAL (held for human, never a failure-close) when an AI review is inconclusive", () => { + it("holds the gate NEUTRAL (held for human, never a failure-close) when an AI review is inconclusive under aiReviewGateMode: block", () => { const adv: Advisory = { ...missingIssueAdvisory(), findings: [{ code: "ai_review_inconclusive", title: "AI review could not be completed", severity: "warning", detail: "no usable verdict", action: "held for human" }], }; - const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + // #10016: the hold is gated on aiReviewGateMode -- this suite's own repo default is unset (resolves + // "advisory"), so the block-mode hold this describe block exercises must opt in explicitly. + const result = evaluateGateCheck(adv, gateCheckPolicy(settings({ aiReviewMode: "block" }), null, true)); expect(result.conclusion).toBe("neutral"); expect(result.blockers).toEqual([]); }); @@ -175,13 +177,13 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { { code: "ai_review_inconclusive", title: "AI review could not be completed", severity: "warning", detail: "no usable verdict", action: "held for human" }, ], }; - const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + const result = evaluateGateCheck(adv, gateCheckPolicy(settings({ aiReviewMode: "block" }), null, true)); // An inconclusive AI can no longer bury a real violation in a "held" state — the secret_leak still hard-blocks. expect(result.conclusion).toBe("failure"); expect(result.blockers.map((blocker) => blocker.code)).toContain("secret_leak"); }); - it("holds the gate NEUTRAL (never a failure-close) when the AI review lock is held by another in-flight pass (#confirmed-bug)", () => { + it("holds the gate NEUTRAL (never a failure-close) when the AI review lock is held by another in-flight pass under aiReviewGateMode: block (#confirmed-bug)", () => { // Same code, different finding text — the lock-contention finding constructed by runAiReviewForAdvisory's // new claim-failure branch. advisory.ts only keys on `code`, so this proves the mechanism end-to-end for // the new finding shape without needing to touch advisory.ts. @@ -197,7 +199,7 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { }, ], }; - const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + const result = evaluateGateCheck(adv, gateCheckPolicy(settings({ aiReviewMode: "block" }), null, true)); expect(result.conclusion).toBe("neutral"); expect(result.blockers).toEqual([]); }); @@ -216,11 +218,21 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { }, ], }; - const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + const result = evaluateGateCheck(adv, gateCheckPolicy(settings({ aiReviewMode: "block" }), null, true)); expect(result.conclusion).toBe("failure"); expect(result.blockers.map((blocker) => blocker.code)).toContain("secret_leak"); }); + it("REGRESSION (#10016): an inconclusive AI review no longer holds the gate when aiReviewGateMode resolves to advisory (the repo default)", () => { + const adv: Advisory = { + ...missingIssueAdvisory(), + findings: [{ code: "ai_review_inconclusive", title: "AI review could not be completed", severity: "warning", detail: "no usable verdict", action: "held for human" }], + }; + const result = evaluateGateCheck(adv, gateCheckPolicy(settings(), null, true)); + expect(result.conclusion).toBe("success"); + expect(result.warnings.map((finding) => finding.code)).toContain("ai_review_inconclusive"); + }); + it("an enforced pre-merge check (pre_merge_check_required) hard-blocks; the advisory variant never does (#review-pre-merge-checks)", () => { const enforced: Advisory = { ...missingIssueAdvisory(), diff --git a/test/unit/predicted-gate-engine.test.ts b/test/unit/predicted-gate-engine.test.ts index 4e225bdf1..a47ac846b 100644 --- a/test/unit/predicted-gate-engine.test.ts +++ b/test/unit/predicted-gate-engine.test.ts @@ -684,7 +684,8 @@ describe("predicted-gate engine module coverage (#2283)", () => { findings: [{ code: "ai_review_inconclusive", severity: "warning", title: "ai", detail: "ai" }], generatedAt: "2026-01-01T00:00:00.000Z", }, - {}, + // #10016: the hold only fires under aiReviewGateMode: block now -- an unset mode resolves "advisory". + { aiReviewGateMode: "block" }, ); expect(aiHold.conclusion).toBe("neutral"); diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index 130c64de5..d16fbee9b 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -17,7 +17,7 @@ import { resolveAiReviewLowConfidenceHold, } from "../../src/rules/advisory"; import type { CollisionReport } from "../../src/signals/engine"; -import type { IssueRecord, PullRequestRecord, PullRequestFileRecord, RepositoryRecord } from "../../src/types"; +import type { Advisory, IssueRecord, PullRequestRecord, PullRequestFileRecord, RepositoryRecord } from "../../src/types"; const repo: RepositoryRecord = { fullName: "JSONbored/loopover", @@ -738,6 +738,67 @@ describe("advisory rules", () => { expect(evaluateGateCheck(splitAdvisory).conclusion).toBe("success"); }); + describe("ai_review_inconclusive hold is gated by aiReviewGateMode (#10016)", () => { + const inconclusiveAdvisory = (extraFindings: Advisory["findings"] = []) => ({ + ...buildPullRequestAdvisory(repo, null), + findings: [ + { + code: "ai_review_inconclusive" as const, + title: "AI review could not complete for this PR head", + severity: "warning" as const, + detail: "The AI review attempt did not produce a result.", + action: "The review is retried automatically after a short cooldown.", + }, + ...extraFindings, + ], + }); + + it("REGRESSION: an inconclusive review does not hold an advisory-mode repo's otherwise-clean gate", () => { + const result = evaluateGateCheck(inconclusiveAdvisory()); + expect(result.conclusion).toBe("success"); + expect(result.warnings.map((finding) => finding.code)).toContain("ai_review_inconclusive"); + }); + + it("stays non-blocking under an explicit advisory or off mode", () => { + const advisory = inconclusiveAdvisory(); + const advisoryMode = evaluateGateCheck(advisory, { aiReviewGateMode: "advisory" }); + expect(advisoryMode.conclusion).toBe("success"); + expect(advisoryMode.warnings.map((finding) => finding.code)).toContain("ai_review_inconclusive"); + + const offMode = evaluateGateCheck(advisory, { aiReviewGateMode: "off" }); + expect(offMode.conclusion).toBe("success"); + expect(offMode.warnings.map((finding) => finding.code)).toContain("ai_review_inconclusive"); + }); + + it("still HOLDS the gate (neutral) under aiReviewGateMode: block, with the unchanged title", () => { + const result = evaluateGateCheck(inconclusiveAdvisory(), { aiReviewGateMode: "block" }); + expect(result.conclusion).toBe("neutral"); + expect(result.title).toBe("LoopOver Orb Review Agent — held for human review"); + expect(result.blockers).toEqual([]); + }); + + it("the unconditional secret_scan_incomplete hold still fires once the AI hold no longer does", () => { + const advisory = inconclusiveAdvisory([ + { + code: "secret_scan_incomplete", + title: "Patch-less file(s) could not be fully scanned for secrets (1)", + severity: "critical", + detail: "GitHub omitted inline diff for: secrets.env.", + action: "Ensure patch-less files are within scan limits or split the change so secrets can be verified.", + }, + ]); + const result = evaluateGateCheck(advisory); + expect(result.conclusion).toBe("neutral"); + expect(result.blockers).toEqual([]); + }); + + it("dry-run: displayConclusion previews the block-mode hold while the posted conclusion stays success", () => { + const result = evaluateGateCheck(inconclusiveAdvisory(), { aiReviewGateMode: "advisory", dryRun: true }); + expect(result.conclusion).toBe("success"); + expect(result.displayConclusion).toBe("neutral"); + }); + }); + describe("aiReviewLowConfidenceDisposition (#4603)", () => { const consensusAdvisory = (confidence: number) => ({ ...buildPullRequestAdvisory(repo, null),