From ecb50600cb442387246aef82e44e0c5de0e93006 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:36:58 -0700 Subject: [PATCH] feat(ci): enforce that dispatch gates can say why they suppressed work (#9003) The #9003 invariant: every decision -- and every NON-decision -- records a reason. "Nothing happened and nothing says why" must be structurally impossible. It came out of the 2026-07-26 restart investigation, where a forceAiReview pass completed with no fresh review and zero audit events explaining why (#9000), costing hours of elimination -- while the disposition lane, which names every hold, answered the same class of question in minutes. Much of the issue is already built: #9000's fix landed resolvePublicAiReviewGateSkipReason and the audit on the suppress branch, the vision gates return typed { run, reason } results, and the reputation skip pushes a named finding. What was missing is the piece that keeps it true: the issue asks that "a bare-boolean gate on a dispatch path should fail a lint/unit convention test, the same way no-direct-octokit enforces the write chokepoint." scripts/check-dispatch-gate-reasons.ts is that test. It scans the dispatch-path modules for exported gate-shaped functions (should*/evaluate*Gate*/resolve*Gate*) returning bare boolean, and requires each to either return its reason with the decision, or be allowlisted naming its paired resolver. Both existing conventions pass as-is; what fails is the shape that produced #9000. The checker found one real instance immediately: shouldStartAiReviewForAdvisory -- the exact gate from the #9000 incident. Verified end-to-end that BOTH of its false paths are now named by the caller (the hard gate via resolvePublicAiReviewGateSkipReason, the reputation skip via maybeAddReputationSkipHold's ai_review_inconclusive finding), so its allowlist entry states that mechanism rather than dodging it. Two of my own bugs were caught by the fixture tests while writing this, both worth recording in the file: - the return-type regex was anchored to a line start, so it only matched multi-line signatures -- the whole check was passing VACUOUSLY, including against the real tree - a fixed-width signature slice read past a gate's own body into its neighbour's `): boolean {`, wrongly flagging a reason-returning gate. Now bounded by the function's own opening brace, paren-depth-aware for inline object-type params. Also deliberately rejected: "a resolve*SkipReason anywhere in the module exempts the module" -- one resolver would exempt every future gate beside it, the same neighbour false negative check-regate-sort-key.ts had to kill with brace-bounding. Pairing must be stated per gate. 10 direct tests, including both formatting shapes, exact-name allowlisting, the neighbour non-exemption, and the real tree staying green. Wired into test:ci and the validate-code job. --- .github/workflows/ci.yml | 7 + package.json | 3 +- scripts/check-dispatch-gate-reasons.ts | 167 ++++++++++++++++++ ...check-dispatch-gate-reasons-script.test.ts | 95 ++++++++++ 4 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 scripts/check-dispatch-gate-reasons.ts create mode 100644 test/unit/check-dispatch-gate-reasons-script.test.ts 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([]); + }); +});