From c56b8887023d38524516accf50ce7fc3ef94e47b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 01:58:34 -0700 Subject: [PATCH] fix(maintainability): put the three paid advisories' universal spend stops behind one predicate (#9557) --- 0 | 0 src/queue/advisory-spend-gate.ts | 47 +++++++++++++++++ src/queue/processors.ts | 12 +++-- src/queue/slop-detection.ts | 16 ++++-- test/unit/advisory-spend-gate.test.ts | 73 +++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 0 create mode 100644 src/queue/advisory-spend-gate.ts create mode 100644 test/unit/advisory-spend-gate.test.ts diff --git a/0 b/0 new file mode 100644 index 000000000..e69de29bb diff --git a/src/queue/advisory-spend-gate.ts b/src/queue/advisory-spend-gate.ts new file mode 100644 index 000000000..b14299e41 --- /dev/null +++ b/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; +} diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 74ae0c856..3231d1fc9 100644 --- a/src/queue/processors.ts +++ b/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 { diff --git a/src/queue/slop-detection.ts b/src/queue/slop-detection.ts index 4a1ecdf85..e853ffe38 100644 --- a/src/queue/slop-detection.ts +++ b/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 { // 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, diff --git a/test/unit/advisory-spend-gate.test.ts b/test/unit/advisory-spend-gate.test.ts new file mode 100644 index 000000000..2b848b54e --- /dev/null +++ b/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"); + }); + }); +});