diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5ba6a5b47..204afe19b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -383,6 +383,13 @@ jobs: - name: Command redelivery-guard check if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} run: npm run command-redelivery-guards:check + + # #9003: a gate that can suppress a review or an action must be able to say WHY. A forceAiReview pass + # once finished with no fresh review and no audit event explaining it (#9000), costing hours of + # elimination -- this makes "nothing happened and nothing says why" fail CI instead. + - name: Dispatch gate-reason check + if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} + run: npm run dispatch-gate-reasons:check # Mechanical drift tripwire for the hand-duplicated src/{review,settings,signals} <-> loopover-engine # twin files, plus a version-skew check on the installed @loopover/engine (#4260). Same # local-only-until-now gap as the drift checks above. Also gated on `miner`: the reverse-direction diff --git a/package.json b/package.json index 48c951d7d..24b355dc8 100644 --- a/package.json +++ b/package.json @@ -74,6 +74,7 @@ "dead-source-files:check": "node --experimental-strip-types scripts/check-dead-source-files.ts", "regate-sort-key:check": "node --experimental-strip-types scripts/check-regate-sort-key.ts", "command-redelivery-guards:check": "node --experimental-strip-types scripts/check-command-redelivery-guards.ts", + "dispatch-gate-reasons:check": "node --experimental-strip-types scripts/check-dispatch-gate-reasons.ts", "replay-runner-manifest": "tsx scripts/replay-runner-image-manifest.ts", "replay-runner-manifest:write": "tsx scripts/replay-runner-image-manifest.ts --write", "replay-runner-manifest:check": "tsx scripts/replay-runner-image-manifest.ts --check", @@ -121,7 +122,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run dead-source-files:check && npm run regate-sort-key:check && npm run command-redelivery-guards:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run dead-source-files:check && npm run regate-sort-key:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/scripts/check-dispatch-gate-reasons.ts b/scripts/check-dispatch-gate-reasons.ts new file mode 100644 index 000000000..741d99ad6 --- /dev/null +++ b/scripts/check-dispatch-gate-reasons.ts @@ -0,0 +1,167 @@ +#!/usr/bin/env node +// #9003: a gate that can suppress a review or an action must be able to say WHY. +// +// The invariant, from the issue: "every decision -- and every NON-decision -- must record a reason. A pass +// that declines to review, declines to act, downgrades, defers, or drops work must emit an audit event naming +// the specific gate that made the call. 'Nothing happened and nothing says why' must be structurally +// impossible." +// +// This came out of the 2026-07-26 restart investigation, where a `forceAiReview: true` pass -- whose entire +// purpose is to spend a fresh review -- completed with no fresh review and ZERO audit events explaining why +// (#9000). Hours went into reverse-engineering by elimination. The disposition lane, which already names every +// hold via agentHoldAuditDetail, took minutes for the same class of question. +// +// WHAT THIS ENFORCES, and why it is shaped this way. It does not demand one particular return type. The +// codebase has already converged on two working answers, and either satisfies this check: +// +// 1. Return the reason directly. evaluateVisualVisionGate and evaluateScreenshotTableVisionGate return +// `{ run: false, reason: "low_reputation" }` -- the gate and its explanation are one value, so they +// cannot drift apart. +// 2. Pair the boolean with a resolver. shouldRequirePublicAiReviewForAdvisory stays boolean for the fast +// path, and resolvePublicAiReviewGateSkipReason names the reason on the suppress path. Cheaper when the +// hot path runs thousands of times and the reason is only needed when it says no. +// +// What is NOT acceptable is a bare boolean with no way to recover the reason, on a path where `false` silently +// suppresses work. That is exactly the shape that produced #9000. +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +/** + * The dispatch-path modules this applies to. Deliberately a named list rather than all of src/**: the rule is + * about gates that suppress a REVIEW or an ACTION, and applying it to every boolean helper in the tree would + * bury the few that matter under hundreds that do not -- the same reasoning src/selfhost/inert-config.ts uses + * for scoping its own report. + */ +const DISPATCH_PATH_MODULES = [ + "src/queue/ai-review-orchestration.ts", + "src/review/visual/visual-findings.ts", + "src/review/visual/screenshot-table-vision.ts", +] as const; + +/** Exported gate-shaped functions: the ones whose answer decides whether work happens. */ +const GATE_DECLARATION = /^export (?:async )?function (should\w+|evaluate\w*Gate\w*|resolve\w*Gate\w*)\s*\(/gm; + +/** + * A return type that carries its own reason satisfies the rule outright; this matches only the bare-boolean + * shape that does not. + * + * Deliberately NOT anchored to a line start. The first version was `/^\s*\)\s*:.../m`, which only ever matched + * a multi-line signature -- so every single-line gate passed silently and the whole check was vacuous, + * including against the real tree. Caught by the fixture tests, which is exactly what they are for. + */ +const BARE_BOOLEAN_RETURN = /\)\s*:\s*(?:Promise<\s*)?boolean\s*>?\s*\{/; + +/** + * Gates that stay boolean WITH a stated reason. Each entry names the paired resolver (option 2 above) or the + * argument for why `false` here cannot silently suppress work. + */ +const ALLOWED_BARE_BOOLEAN: ReadonlyMap = new Map([ + [ + "shouldStartAiReviewForAdvisory", + "Both of its `false` paths are named by the caller, verified end-to-end. (1) The hard-gate path: processors.ts calls resolvePublicAiReviewGateSkipReason on the suppress branch and audits the reason -- that call is #9000's own fix. (2) The reputation-skip path: maybeAddReputationSkipHold pushes a named `ai_review_inconclusive` finding into the advisory before this point is reached. So a `false` here is never silent; the reason simply lives with the caller rather than in the return type.", + ], + [ + "shouldRequirePublicAiReviewForAdvisory", + "Paired with resolvePublicAiReviewGateSkipReason in the same module, which names the reason on the suppress path (processors.ts audits it when aiReviewWillRun is false). The boolean is the hot path; the resolver is only called when it says no.", + ], +]); + +export type DispatchGateViolation = { file: string; gate: string; reason: string }; + +/** + * The declaration text from `start` through this function's own body-opening brace, or null if unbalanced. + * + * Tracks paren depth to find the END of the parameter list first, because a parameter with an inline object + * type (`args: { repoFullName: string }`) contains a `{` that is not the body's. + */ +function signatureOf(source: string, start: number): string | null { + let depth = 0; + let seenParen = false; + for (let i = start; i < source.length; i += 1) { + const ch = source[i]; + if (ch === "(") { + depth += 1; + seenParen = true; + } else if (ch === ")") { + depth -= 1; + } else if (ch === "{" && seenParen && depth === 0) { + return source.slice(start, i + 1); + } + } + return null; +} + +/** + * Pure over its inputs. `readFile` is injectable so tests can drive a synthetic offender without touching the + * tree, matching check-dead-source-files.ts and check-command-redelivery-guards.ts. + */ +export function findDispatchGatesWithoutReasons( + options: { + modules?: readonly string[]; + readFile?: (file: string) => string; + allowedBareBoolean?: ReadonlyMap; + } = {}, +): DispatchGateViolation[] { + const { + modules = DISPATCH_PATH_MODULES, + readFile = (file: string) => readFileSync(file, "utf8"), + allowedBareBoolean = ALLOWED_BARE_BOOLEAN, + } = options; + + const violations: DispatchGateViolation[] = []; + for (const file of modules) { + let source: string; + try { + source = readFile(file); + } catch { + // A module that does not exist in this checkout is not a violation -- report nothing for it. + continue; + } + for (const match of source.matchAll(GATE_DECLARATION)) { + const gate = match[1]; + if (gate === undefined || allowedBareBoolean.has(gate)) continue; + // The signature must end at THIS function's own opening brace. A fixed-width slice reads on into the + // next declaration, so a reason-returning gate followed by a boolean one inherits its neighbour's + // `): boolean {` and is wrongly flagged -- the mirror image of the neighbour false-NEGATIVE noted below, + // and caught by the same fixture tests. + const signature = signatureOf(source, match.index ?? 0); + if (signature === null || !BARE_BOOLEAN_RETURN.test(signature)) continue; + // NOTE: deliberately NOT "does this module contain any resolve*SkipReason". That heuristic was the first + // shape of this check and it is unsound: one resolver anywhere in a file would exempt every future gate + // added beside it -- the same "a neighbour satisfies the scan for the one actually missing it" false + // negative that check-regate-sort-key.ts had to replace brace-bounding for. Pairing must be STATED, per + // gate, in ALLOWED_BARE_BOOLEAN, naming the resolver. + violations.push({ + file, + gate, + reason: "returns a bare boolean with no paired reason resolver — a `false` here suppresses work with nothing recording why", + }); + } + } + return violations.sort((a, b) => (a.file === b.file ? a.gate.localeCompare(b.gate) : a.file.localeCompare(b.file))); +} + +function main(): void { + const violations = findDispatchGatesWithoutReasons(); + if (violations.length === 0) { + process.stdout.write("dispatch gate reasons: OK\n"); + return; + } + process.stderr.write(`Found ${violations.length} dispatch gate(s) that cannot say why they suppressed work (#9003):\n`); + for (const violation of violations) { + process.stderr.write(` ${violation.file}: ${violation.gate} — ${violation.reason}\n`); + } + process.stderr.write( + "\nA `forceAiReview: true` pass once completed with no fresh review and no audit event explaining why\n" + + "(#9000); the investigation took hours of elimination. Either:\n\n" + + " • return the reason with the decision, as evaluateVisualVisionGate does\n" + + " -> { run: false, reason: \"low_reputation\" }\n" + + " • or add a paired resolveSkipReason in the same module and audit it on the suppress path,\n" + + " as shouldRequirePublicAiReviewForAdvisory does\n\n" + + "...or, if a `false` here genuinely cannot suppress work, add the gate to ALLOWED_BARE_BOOLEAN in\n" + + "scripts/check-dispatch-gate-reasons.ts WITH that argument.\n", + ); + process.exit(1); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/test/unit/check-dispatch-gate-reasons-script.test.ts b/test/unit/check-dispatch-gate-reasons-script.test.ts new file mode 100644 index 000000000..479998229 --- /dev/null +++ b/test/unit/check-dispatch-gate-reasons-script.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it } from "vitest"; +import { findDispatchGatesWithoutReasons } from "../../scripts/check-dispatch-gate-reasons"; + +/** One module's source, keyed by path — the injectable seam every checker in scripts/ exposes. */ +function fakeModule(path: string, source: string) { + return { modules: [path], readFile: () => source, allowedBareBoolean: new Map() }; +} + +// #9003: a `forceAiReview: true` pass — whose entire purpose is to spend a fresh review — completed with no +// fresh review and ZERO audit events explaining why (#9000). The investigation took hours of elimination, +// while the disposition lane, which names every hold, answered the same class of question in minutes. This +// check makes "nothing happened and nothing says why" fail CI instead of costing an investigation. +describe("check-dispatch-gate-reasons script (#9003)", () => { + it("REGRESSION: flags a bare-boolean gate on a dispatch path — the shape that produced #9000", () => { + const violations = findDispatchGatesWithoutReasons( + fakeModule("src/queue/ai-review-orchestration.ts", "export function shouldStartReview(args: X): boolean {\n return true;\n}\n"), + ); + expect(violations).toEqual([ + { + file: "src/queue/ai-review-orchestration.ts", + gate: "shouldStartReview", + reason: expect.stringContaining("no paired reason resolver"), + }, + ]); + }); + + it("INVARIANT: a gate returning its reason WITH the decision passes — the evaluateVisualVisionGate shape", () => { + // { run: false, reason: "low_reputation" } cannot drift from its explanation, because they are one value. + const violations = findDispatchGatesWithoutReasons( + fakeModule("src/review/visual/visual-findings.ts", "export function evaluateVisualVisionGate(input: X): VisualVisionGateResult {\n return { run: true };\n}\n"), + ); + expect(violations).toEqual([]); + }); + + it("INVARIANT: an async gate returning Promise is caught too", () => { + const violations = findDispatchGatesWithoutReasons( + fakeModule("src/queue/ai-review-orchestration.ts", "export async function shouldDispatch(args: X): Promise {\n return true;\n}\n"), + ); + expect(violations.map((violation) => violation.gate)).toEqual(["shouldDispatch"]); + }); + + it("INVARIANT: a multi-line signature is caught — formatting must not decide whether a rule applies", () => { + // The #9541 lesson: a family written in two formattings is a family a grep only half-sees. + const source = "export function shouldStartReview(\n env: Env,\n args: Args,\n): boolean {\n return true;\n}\n"; + const violations = findDispatchGatesWithoutReasons(fakeModule("src/queue/ai-review-orchestration.ts", source)); + expect(violations.map((violation) => violation.gate)).toEqual(["shouldStartReview"]); + }); + + it("INVARIANT: an allowlisted gate is exempt, but only by EXACT name", () => { + const source = "export function shouldRequire(a: X): boolean {\n return true;\n}\nexport function shouldRequireTwin(a: X): boolean {\n return true;\n}\n"; + const violations = findDispatchGatesWithoutReasons({ + modules: ["src/queue/ai-review-orchestration.ts"], + readFile: () => source, + allowedBareBoolean: new Map([["shouldRequire", "paired with resolveXSkipReason"]]), + }); + // The twin is NOT covered by its prefix-sharing sibling's entry — an exemption is a claim about one gate. + expect(violations.map((violation) => violation.gate)).toEqual(["shouldRequireTwin"]); + }); + + it("REGRESSION: a resolver ELSEWHERE in the module does not exempt an unrelated gate", () => { + // This was the check's first shape and it is unsound: one resolver anywhere in a file would exempt every + // future gate added beside it — the same "a neighbour satisfies the scan for the one actually missing it" + // false negative check-regate-sort-key.ts had to adopt brace-bounding to kill. Pairing must be STATED. + const source = + "export function resolvePublicAiReviewGateSkipReason(a: X): Reason | null {\n return null;\n}\n" + + "export function shouldSomethingElse(a: X): boolean {\n return true;\n}\n"; + const violations = findDispatchGatesWithoutReasons(fakeModule("src/queue/ai-review-orchestration.ts", source)); + expect(violations.map((violation) => violation.gate)).toEqual(["shouldSomethingElse"]); + }); + + it("INVARIANT: a non-gate export is ignored — the rule is about suppressing work, not every boolean", () => { + const source = "export function formatSummary(a: X): boolean {\n return true;\n}\n"; + expect(findDispatchGatesWithoutReasons(fakeModule("src/queue/ai-review-orchestration.ts", source))).toEqual([]); + }); + + it("INVARIANT: a module absent from this checkout reports nothing rather than throwing", () => { + const violations = findDispatchGatesWithoutReasons({ + modules: ["src/queue/does-not-exist.ts"], + readFile: () => { + throw new Error("ENOENT"); + }, + }); + expect(violations).toEqual([]); + }); + + it("reports violations sorted by file then gate, so the failure output is stable", () => { + const source = "export function shouldZeta(a: X): boolean {\n return true;\n}\nexport function shouldAlpha(a: X): boolean {\n return true;\n}\n"; + const violations = findDispatchGatesWithoutReasons(fakeModule("src/queue/ai-review-orchestration.ts", source)); + expect(violations.map((violation) => violation.gate)).toEqual(["shouldAlpha", "shouldZeta"]); + }); + + it("the REAL dispatch paths are clean — this check runs in CI and must stay green", () => { + expect(findDispatchGatesWithoutReasons()).toEqual([]); + }); +});