fix(maintainability): put the three paid advisories' universal spend stops behind one predicate (#9557)

src/queue/advisory-spend-gate.ts

@@ -0,0 +1,47 @@
+// #9541 (deliverable 2) / #9491: the ONE precondition check every paid per-PR advisory shares.
+//
+// Three features each make a paid LLM call per PR head — `ai_slop` (slop-detection.ts), `ai_review`
+// (ai-review-orchestration.ts) and the linked-issue satisfaction advisory (processors.ts). Each grew its own
+// hand-written guard line, and they drifted: #9491 found the linked-issue advisory was the one member of the
+// family with NO per-PR commit cap at all, so a long-lived PR kept paying for a fresh assessment on every
+// push long after its two siblings had stopped. That is not a coding mistake anyone could have caught by
+// reading one function — the guards live in three files, and the missing one looked complete on its own.
+//
+// This module owns the stops that are UNIVERSAL, so a fourth advisory cannot be written without them.
+//
+// WHAT IS DELIBERATELY *NOT* HERE: the eligible-author rule. It looks shared (`!confirmedContributor` appears
+// in two of the three) but genuinely is not — `ai_review` reviews some UNCONFIRMED authors on purpose, via
+// `resolveAiReviewableAuthor`'s `aiReviewAllAuthors` / gate-pack policy. Folding that into a common predicate
+// would either silently narrow `ai_review`'s audience or silently widen the other two's, and a "shared" rule
+// that is wrong for one caller is how the next #9491 gets written. Each feature keeps its own author gate;
+// only the stops that are true for every paid advisory live here.
+import type { AgentActionMode } from "../settings/agent-execution";
+
+/** Why a paid advisory must not spend, or `null` when it may proceed. A NAMED reason rather than a boolean,
+ *  mirroring `resolvePublicAiReviewGateSkipReason`'s own shape — the caller usually wants to audit or log
+ *  which stop fired, and a bare `false` forces every call site to re-derive that. */
+export type AdvisorySpendStopReason = "paused" | "no_head_sha" | "commit_threshold_reached";
+
+export type AdvisorySpendPreconditions = {
+  /** A paused repo must never reach a paid LLM call, independent of the feature's own mode setting. */
+  mode: AgentActionMode;
+  /** The advisory's resolved head SHA. Absent ⇒ there is no specific head to attribute the spend to. */
+  headSha: string | null | undefined;
+  /** The per-PR reviewed-commit cap the whole family honors (#9491). */
+  commitThresholdReached: boolean;
+};
+
+/**
+ * The universal spend stops, in the order the existing hand-written guards applied them — so adopting this
+ * function is behaviour-preserving for every current caller, not a re-ordering that changes which audit
+ * event a given PR emits.
+ *
+ * PURE: no IO, no clock. The caller decides what to do with a non-null reason (return silently, record an
+ * audit event, log) — those responses legitimately differ per feature and are not unified here.
+ */
+export function advisorySpendStopReason(preconditions: AdvisorySpendPreconditions): AdvisorySpendStopReason | null {
+  if (preconditions.mode === "paused") return "paused";
+  if (!preconditions.headSha) return "no_head_sha";
+  if (preconditions.commitThresholdReached) return "commit_threshold_reached";
+  return null;
+}

src/queue/processors.ts

@@ -304,6 +304,7 @@ import {
   pendingClosureLabelApplied,
 } from "../services/agent-action-executor";
 import { activeMergeBlockedSha } from "../services/merge-failure";
+import { advisorySpendStopReason } from "./advisory-spend-gate";
 import { applyLowConfidenceHoldCap } from "../review/low-confidence-hold-cap";
 import { recordPendingClosureFlag } from "../review/pending-closure-watchdog";
 import { loadIssueQualityReportMap } from "../services/issue-quality";
@@ -8535,10 +8536,13 @@ export async function runLinkedIssueSatisfactionForAdvisory(
     commitThresholdReached: boolean;
   },
 ): Promise<{ status: "addressed" | "partial" | "unaddressed"; rationale: string } | null> {
-  if (args.mode === "paused" || !args.confirmedContributor || !args.advisory.headSha) return null;
-  // #9491: honor the cap the siblings honor. Deliberately no reuse-for-display fallback, matching
-  // slop-detection's identical early return -- past the cap this feature simply stops spending.
-  if (args.commitThresholdReached) return null;
+  // The author rule stays per-feature by design (ai_review deliberately reviews some unconfirmed authors --
+  // see advisory-spend-gate.ts's header for why it is NOT unified).
+  if (!args.confirmedContributor) return null;
+  // #9541/#9491: the universal spend stops (paused / no head / commit cap) come from ONE place. This advisory
+  // was the family member that shipped WITHOUT the cap, which is precisely the drift a shared predicate
+  // makes structurally impossible for the next one.
+  if (advisorySpendStopReason({ mode: args.mode, headSha: args.advisory.headSha, commitThresholdReached: args.commitThresholdReached }) !== null) return null;
   const primaryIssueNumber = args.pr.linkedIssues[0];
   if (primaryIssueNumber === undefined) return null;
   try {

src/queue/slop-detection.ts

@@ -6,6 +6,7 @@
 // -- it would otherwise make the two files circularly dependent on each other for one line of logic.
 
 import { getCachedAiSlopAdvisory, getDecryptedRepositoryAiKey, type listPullRequestFiles, putCachedAiSlopAdvisory, recordAuditEvent } from "../db/repositories";
+import { advisorySpendStopReason } from "./advisory-spend-gate";
 import { buildPullRequestAdvisory } from "../rules/advisory";
 import { buildAiReviewDiff } from "../review/review-diff";
 import { aiSlopCacheInputFingerprint } from "../review/ai-slop-cache-input";
@@ -62,15 +63,24 @@ export async function runAiSlopForAdvisory(
 ): Promise<void> {
   // Confirmed-contributor gate (matches runAiReviewForAdvisory): no AI spend — free OR BYOK — on a PR from
   // an unconfirmed author. The deterministic slop core still ran for everyone; only the AI layer is gated.
-  if (args.mode === "paused" || !args.confirmedContributor || !args.advisory.headSha) return;
-  if (args.commitThresholdReached) {
+  // Kept HERE rather than in advisorySpendStopReason: ai_review deliberately reviews some unconfirmed
+  // authors, so the author rule is per-feature by design (see that module's header).
+  if (!args.confirmedContributor) return;
+  // #9541: the universal stops (paused / no head / commit cap) now come from ONE place, so a fourth paid
+  // advisory cannot be written without them — which is exactly how #9491's missing cap happened.
+  const spendStop = advisorySpendStopReason({ mode: args.mode, headSha: args.advisory.headSha, commitThresholdReached: args.commitThresholdReached });
+  if (spendStop !== null && spendStop !== "commit_threshold_reached") return;
+  if (spendStop === "commit_threshold_reached") {
     await recordAuditEvent(env, {
       eventType: "github_app.ai_slop_auto_review_skipped",
       actor: args.author,
       targetKey: `${args.repoFullName}#${args.pr.number}`,
       outcome: "completed",
       detail: "slop advisory paused (commit threshold); this head has already been reviewed enough times",
-      metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha },
+      /* v8 ignore next -- the `?? null` arm is unreachable by construction: advisorySpendStopReason checks
+         headSha BEFORE the commit cap, so reaching this branch proves headSha is a non-empty string. It exists
+         only because TypeScript cannot narrow through that call, and `undefined` is not a JsonValue. */
+      metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null },
     }).catch(
       /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */
       () => undefined,

test/unit/advisory-spend-gate.test.ts

@@ -0,0 +1,73 @@
+import { readFileSync } from "node:fs";
+import { describe, expect, it } from "vitest";
+import { advisorySpendStopReason } from "../../src/queue/advisory-spend-gate";
+
+// #9541 (deliverable 2) / #9491: three features each make a paid LLM call per PR head, each grew its own
+// hand-written guard line, and they drifted — the linked-issue advisory was the one member of the family with
+// NO per-PR commit cap, so a long-lived PR kept paying on every push after its siblings had stopped. Nobody
+// could have caught that by reading one function: the guards lived in three files and the incomplete one
+// looked complete on its own.
+describe("advisorySpendStopReason (#9541)", () => {
+  const proceed = { mode: "live" as const, headSha: "abc123", commitThresholdReached: false };
+
+  it("returns null when every universal precondition passes", () => {
+    expect(advisorySpendStopReason(proceed)).toBeNull();
+  });
+
+  it("REGRESSION: stops on the per-PR commit cap — the stop the linked-issue advisory shipped without", () => {
+    expect(advisorySpendStopReason({ ...proceed, commitThresholdReached: true })).toBe("commit_threshold_reached");
+  });
+
+  it("stops a paused repo, independent of the feature's own mode setting", () => {
+    expect(advisorySpendStopReason({ ...proceed, mode: "paused" })).toBe("paused");
+  });
+
+  it("stops when there is no head SHA to attribute the spend to", () => {
+    for (const headSha of [null, undefined, ""]) {
+      expect(advisorySpendStopReason({ ...proceed, headSha }), String(headSha)).toBe("no_head_sha");
+    }
+  });
+
+  it("INVARIANT: precedence is paused → no_head_sha → commit cap, matching the hand-written guards it replaces", () => {
+    // Order is load-bearing, not cosmetic: adopting this function had to be behaviour-preserving, and the
+    // reason a given PR reports decides which audit event it emits. A re-ordering would silently relabel
+    // real production history.
+    expect(advisorySpendStopReason({ mode: "paused", headSha: null, commitThresholdReached: true })).toBe("paused");
+    expect(advisorySpendStopReason({ mode: "live", headSha: null, commitThresholdReached: true })).toBe("no_head_sha");
+  });
+
+  it("is PURE — same input, same answer, and the input is never mutated", () => {
+    const input = { ...proceed, commitThresholdReached: true };
+    const snapshot = JSON.stringify(input);
+    expect(advisorySpendStopReason(input)).toBe(advisorySpendStopReason(input));
+    expect(JSON.stringify(input)).toBe(snapshot);
+  });
+
+  // The point of the module is that a FOURTH advisory cannot be written without these stops. That only holds
+  // while the existing three actually route through it, so the adoption itself is pinned at the source —
+  // the same producer-drift-guard convention db-parsers.test.ts uses for STALE_RECHECK_DENIAL_DETAIL_PATTERN.
+  describe("adoption is real, not aspirational", () => {
+    it.each([
+      ["src/queue/slop-detection.ts", "runAiSlopForAdvisory"],
+      ["src/queue/processors.ts", "runLinkedIssueSatisfactionForAdvisory"],
+    ])("%s routes its paid advisory through the shared gate", (file) => {
+      expect(readFileSync(file, "utf8")).toContain("advisorySpendStopReason(");
+    });
+
+    it("neither adopter re-implements the commit-cap stop inline, which is what allowed the two to disagree", () => {
+      for (const file of ["src/queue/slop-detection.ts", "src/queue/processors.ts"]) {
+        const source = readFileSync(file, "utf8");
+        expect(source, file).not.toContain("if (args.commitThresholdReached) return");
+      }
+    });
+
+    it("INVARIANT: the author rule is deliberately NOT unified — ai_review reviews some unconfirmed authors", () => {
+      // Folding `confirmedContributor` in would either narrow ai_review's audience or widen the other two's.
+      // A 'shared' rule that is wrong for one caller is how the next #9491 gets written, so the module must
+      // keep saying so and must not grow the field.
+      const gate = readFileSync("src/queue/advisory-spend-gate.ts", "utf8");
+      expect(gate).not.toContain("confirmedContributor:");
+      expect(gate).toContain("resolveAiReviewableAuthor");
+    });
+  });
+});