feat(ci): enforce that dispatch gates can say why they suppressed work (#9003)

.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

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

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<string, string> = 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<string, string>;
+  } = {},
+): 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 resolve<X>SkipReason 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();

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<string, string>() };
+}
+
+// #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<boolean> is caught too", () => {
+    const violations = findDispatchGatesWithoutReasons(
+      fakeModule("src/queue/ai-review-orchestration.ts", "export async function shouldDispatch(args: X): Promise<boolean> {\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([]);
+  });
+});