Skip to content

Commit 33f1ba0

Browse files
authored
fix(maintainability): put the three paid advisories' universal spend stops behind one predicate (#9557) (#9558)
1 parent 3be31d3 commit 33f1ba0

5 files changed

Lines changed: 141 additions & 7 deletions

File tree

0

Whitespace-only changes.

src/queue/advisory-spend-gate.ts

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

src/queue/processors.ts

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

src/queue/slop-detection.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
// -- it would otherwise make the two files circularly dependent on each other for one line of logic.
77

88
import { getCachedAiSlopAdvisory, getDecryptedRepositoryAiKey, type listPullRequestFiles, putCachedAiSlopAdvisory, recordAuditEvent } from "../db/repositories";
9+
import { advisorySpendStopReason } from "./advisory-spend-gate";
910
import { buildPullRequestAdvisory } from "../rules/advisory";
1011
import { buildAiReviewDiff } from "../review/review-diff";
1112
import { aiSlopCacheInputFingerprint } from "../review/ai-slop-cache-input";
@@ -62,15 +63,24 @@ export async function runAiSlopForAdvisory(
6263
): Promise<void> {
6364
// Confirmed-contributor gate (matches runAiReviewForAdvisory): no AI spend — free OR BYOK — on a PR from
6465
// an unconfirmed author. The deterministic slop core still ran for everyone; only the AI layer is gated.
65-
if (args.mode === "paused" || !args.confirmedContributor || !args.advisory.headSha) return;
66-
if (args.commitThresholdReached) {
66+
// Kept HERE rather than in advisorySpendStopReason: ai_review deliberately reviews some unconfirmed
67+
// authors, so the author rule is per-feature by design (see that module's header).
68+
if (!args.confirmedContributor) return;
69+
// #9541: the universal stops (paused / no head / commit cap) now come from ONE place, so a fourth paid
70+
// advisory cannot be written without them — which is exactly how #9491's missing cap happened.
71+
const spendStop = advisorySpendStopReason({ mode: args.mode, headSha: args.advisory.headSha, commitThresholdReached: args.commitThresholdReached });
72+
if (spendStop !== null && spendStop !== "commit_threshold_reached") return;
73+
if (spendStop === "commit_threshold_reached") {
6774
await recordAuditEvent(env, {
6875
eventType: "github_app.ai_slop_auto_review_skipped",
6976
actor: args.author,
7077
targetKey: `${args.repoFullName}#${args.pr.number}`,
7178
outcome: "completed",
7279
detail: "slop advisory paused (commit threshold); this head has already been reviewed enough times",
73-
metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha },
80+
/* v8 ignore next -- the `?? null` arm is unreachable by construction: advisorySpendStopReason checks
81+
headSha BEFORE the commit cap, so reaching this branch proves headSha is a non-empty string. It exists
82+
only because TypeScript cannot narrow through that call, and `undefined` is not a JsonValue. */
83+
metadata: { repoFullName: args.repoFullName, headSha: args.advisory.headSha ?? null },
7484
}).catch(
7585
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */
7686
() => undefined,
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
import { readFileSync } from "node:fs";
2+
import { describe, expect, it } from "vitest";
3+
import { advisorySpendStopReason } from "../../src/queue/advisory-spend-gate";
4+
5+
// #9541 (deliverable 2) / #9491: three features each make a paid LLM call per PR head, each grew its own
6+
// hand-written guard line, and they drifted — the linked-issue advisory was the one member of the family with
7+
// NO per-PR commit cap, so a long-lived PR kept paying on every push after its siblings had stopped. Nobody
8+
// could have caught that by reading one function: the guards lived in three files and the incomplete one
9+
// looked complete on its own.
10+
describe("advisorySpendStopReason (#9541)", () => {
11+
const proceed = { mode: "live" as const, headSha: "abc123", commitThresholdReached: false };
12+
13+
it("returns null when every universal precondition passes", () => {
14+
expect(advisorySpendStopReason(proceed)).toBeNull();
15+
});
16+
17+
it("REGRESSION: stops on the per-PR commit cap — the stop the linked-issue advisory shipped without", () => {
18+
expect(advisorySpendStopReason({ ...proceed, commitThresholdReached: true })).toBe("commit_threshold_reached");
19+
});
20+
21+
it("stops a paused repo, independent of the feature's own mode setting", () => {
22+
expect(advisorySpendStopReason({ ...proceed, mode: "paused" })).toBe("paused");
23+
});
24+
25+
it("stops when there is no head SHA to attribute the spend to", () => {
26+
for (const headSha of [null, undefined, ""]) {
27+
expect(advisorySpendStopReason({ ...proceed, headSha }), String(headSha)).toBe("no_head_sha");
28+
}
29+
});
30+
31+
it("INVARIANT: precedence is paused → no_head_sha → commit cap, matching the hand-written guards it replaces", () => {
32+
// Order is load-bearing, not cosmetic: adopting this function had to be behaviour-preserving, and the
33+
// reason a given PR reports decides which audit event it emits. A re-ordering would silently relabel
34+
// real production history.
35+
expect(advisorySpendStopReason({ mode: "paused", headSha: null, commitThresholdReached: true })).toBe("paused");
36+
expect(advisorySpendStopReason({ mode: "live", headSha: null, commitThresholdReached: true })).toBe("no_head_sha");
37+
});
38+
39+
it("is PURE — same input, same answer, and the input is never mutated", () => {
40+
const input = { ...proceed, commitThresholdReached: true };
41+
const snapshot = JSON.stringify(input);
42+
expect(advisorySpendStopReason(input)).toBe(advisorySpendStopReason(input));
43+
expect(JSON.stringify(input)).toBe(snapshot);
44+
});
45+
46+
// The point of the module is that a FOURTH advisory cannot be written without these stops. That only holds
47+
// while the existing three actually route through it, so the adoption itself is pinned at the source —
48+
// the same producer-drift-guard convention db-parsers.test.ts uses for STALE_RECHECK_DENIAL_DETAIL_PATTERN.
49+
describe("adoption is real, not aspirational", () => {
50+
it.each([
51+
["src/queue/slop-detection.ts", "runAiSlopForAdvisory"],
52+
["src/queue/processors.ts", "runLinkedIssueSatisfactionForAdvisory"],
53+
])("%s routes its paid advisory through the shared gate", (file) => {
54+
expect(readFileSync(file, "utf8")).toContain("advisorySpendStopReason(");
55+
});
56+
57+
it("neither adopter re-implements the commit-cap stop inline, which is what allowed the two to disagree", () => {
58+
for (const file of ["src/queue/slop-detection.ts", "src/queue/processors.ts"]) {
59+
const source = readFileSync(file, "utf8");
60+
expect(source, file).not.toContain("if (args.commitThresholdReached) return");
61+
}
62+
});
63+
64+
it("INVARIANT: the author rule is deliberately NOT unified — ai_review reviews some unconfirmed authors", () => {
65+
// Folding `confirmedContributor` in would either narrow ai_review's audience or widen the other two's.
66+
// A 'shared' rule that is wrong for one caller is how the next #9491 gets written, so the module must
67+
// keep saying so and must not grow the field.
68+
const gate = readFileSync("src/queue/advisory-spend-gate.ts", "utf8");
69+
expect(gate).not.toContain("confirmedContributor:");
70+
expect(gate).toContain("resolveAiReviewableAuthor");
71+
});
72+
});
73+
});

0 commit comments

Comments
 (0)