diff --git a/.changeset/review-budget-cap-clamp.md b/.changeset/review-budget-cap-clamp.md new file mode 100644 index 0000000..1898f30 --- /dev/null +++ b/.changeset/review-budget-cap-clamp.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Clamp the router's run budget to the effective per-run AI-credits cap, and make the cap visible to the run. The tier budget table is sized inside the workflow's assumed $10 ceiling, but the cap is enforced runner-side by the firewall api-proxy and was invisible in-container, so a consumer with a tighter `max-ai-credits` got soft targets that promised more work than the cap could pay for; the graceful-degradation path never fired and the run died at the api-proxy with findings in hand (observed on a 400-credit behavior test: three finders completed, ~390 credits spent, killed mid-persist before validation, nothing posted). The frontmatter now sets `max-ai-credits: 1000` explicitly and mirrors it into the agent environment as `REVIEW_MAX_AI_CREDITS` (kept in sync by consumers that override the cap); the router resolves the cap from that mirror (falling back to `GH_AW_MAX_AI_CREDITS`, a visible awf config, then gh-aw's 1000-credit default) and scales the selected tier's soft targets proportionally, floored at the trivial tier's values, marking `runBudget.capClamped` and `runBudget.effectiveCreditCap` in routing.json. The budget prompt gains a matching estimated-credits proxy (sum of in-band `subagent_tokens` over completed sub-agents, divided by 5,000) and two mandatory proxy checkpoints, the critical one immediately after the last finder returns and before claim validation, so a tight run sheds and lands a partial review instead of dying at the cap. diff --git a/workflows/review/eval/judge-live-model.ts b/workflows/review/eval/judge-live-model.ts index 002343f..e2ddd7b 100644 --- a/workflows/review/eval/judge-live-model.ts +++ b/workflows/review/eval/judge-live-model.ts @@ -45,7 +45,9 @@ const scoreOne = async (request: JudgeRequest): Promise => { "", `Evidence trace:\n${request.evidenceTrace.join("\n")}`, ].join("\n"); - let response: Response | undefined; + // Typed via the fetch signature: the repo's eslint no-undef does not + // know the fetch-API globals in type position. + let response: Awaited> | undefined; for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt += 1) { try { response = await fetch(API_URL, { diff --git a/workflows/review/lib/credit-cap.ts b/workflows/review/lib/credit-cap.ts new file mode 100644 index 0000000..0dae703 --- /dev/null +++ b/workflows/review/lib/credit-cap.ts @@ -0,0 +1,145 @@ +/** + * The effective per-run AI-credit cap: discovery from the environment and the + * clamp that resizes a tier budget to fit inside it. Split from `router.ts` + * by concern (and its max-lines budget); the router imports both functions + * and remains the single CLI entry point. + */ + +import type {RunBudget} from "./router"; +import {DEFAULT_TIER_BUDGETS} from "./router"; + +/** The read-only slice of the filesystem surface cap discovery needs. */ +export type CreditCapFs = { + readFileSync: (p: string, enc: "utf8") => string; + existsSync: (p: string) => boolean; +}; + +/** + * gh-aw's baked-in per-run AI-credits default (credits; 1 credit = $0.01), + * assumed when no cap is discoverable from the environment. + */ +export const DEFAULT_MAX_AI_CREDITS = 1000; + +/** + * The landing reserve: a clamped budget's soft targets aim at this fraction + * of the effective cap, never the cap itself. Spend is unobservable mid-run + * (the orchestrator works from proxies), and requests already in flight when + * a checkpoint passes still bill after it, so a soft target equal to the hard + * cap leaves no room to land: the second budget-shed acceptance run engaged + * the clamp and still died at 416/400 credits mid-skill-audit. A quarter of + * the cap is sized to absorb both the proxy estimation error and the + * in-flight overshoot observed there. + */ +export const LANDING_RESERVE_RATIO = 0.75; + +/** + * Clamp a tier budget to the effective per-run credit cap. The tier table is + * sized inside the workflow's assumed $10 ceiling; when a consumer sets a + * tighter `max-ai-credits`, the un-clamped soft targets promise more work + * than the hard cap can pay for, and the run dies at the api-proxy with + * findings in hand instead of shedding early (observed: a 400-credit run + * planned against the high tier's $10 targets and was killed mid-validation). + * The clamped soft dollar target is {@link LANDING_RESERVE_RATIO} of the cap, + * not the cap itself, so a run that sheds against its targets lands inside + * the hard ceiling. Scaling is proportional to spend, floored at the trivial + * tier's values so even a tiny cap yields the smallest designed review rather + * than zero work. A zero/negative cap means explicitly uncapped; undefined + * means unknown. + */ +export const clampBudgetToCreditCap = ( + budget: RunBudget, + capCredits: number | undefined, +): RunBudget => { + if ( + capCredits === undefined || + !Number.isFinite(capCredits) || + capCredits <= 0 + ) { + return budget; + } + const capUsd = capCredits / 100; + const softCapUsd = capUsd * LANDING_RESERVE_RATIO; + if (softCapUsd >= budget.maxUsd) { + // The tier's own soft target already leaves at least the reserve + // under the hard cap; record the cap and change nothing. + return {...budget, effectiveCreditCap: capCredits}; + } + const ratio = softCapUsd / budget.maxUsd; + const floor = DEFAULT_TIER_BUDGETS.trivial; + const scale = (value: number, min: number): number => + Math.max(min, Math.floor(value * ratio)); + return { + ...budget, + maxReviewerInvocations: scale( + budget.maxReviewerInvocations, + floor.maxReviewerInvocations, + ), + maxToolCallsPerFinding: scale( + budget.maxToolCallsPerFinding, + floor.maxToolCallsPerFinding, + ), + maxTotalToolCalls: scale( + budget.maxTotalToolCalls, + floor.maxTotalToolCalls, + ), + maxWallClockMinutes: scale( + budget.maxWallClockMinutes, + floor.maxWallClockMinutes, + ), + maxUsd: softCapUsd, + effectiveCreditCap: capCredits, + capClamped: true, + }; +}; + +/** + * Resolve the effective per-run AI-credit cap from the environment, most + * explicit source first: + * + * 1. `REVIEW_MAX_AI_CREDITS` — the frontmatter `env:` mirror of + * `max-ai-credits` (the cap itself is enforced runner-side by the + * firewall api-proxy and is not otherwise exported to the agent). + * 2. `GH_AW_MAX_AI_CREDITS` — in case a future gh-aw exports it directly. + * 3. The awf firewall config (`apiProxy.maxAiCredits`), when its file is + * visible from the agent container. + * 4. gh-aw's baked-in default ({@link DEFAULT_MAX_AI_CREDITS}). + * + * Returns the cap in credits; may be zero/negative when a source explicitly + * disables the cap (the clamp treats that as uncapped). + */ +export const resolveCreditCap = ( + env: Record, + fs: CreditCapFs, +): number => { + for (const name of ["REVIEW_MAX_AI_CREDITS", "GH_AW_MAX_AI_CREDITS"]) { + const raw = env[name]; + if (raw !== undefined && raw.trim() !== "") { + const parsed = Number(raw); + if (Number.isFinite(parsed)) { + return parsed; + } + } + } + const runnerTemp = env["RUNNER_TEMP"]; + const candidates = [ + ...(runnerTemp ? [`${runnerTemp}/gh-aw/awf-config.json`] : []), + "/tmp/gh-aw/awf-config.json", + ]; + for (const path of candidates) { + if (!fs.existsSync(path)) { + continue; + } + try { + const parsed = JSON.parse(fs.readFileSync(path, "utf8")) as { + apiProxy?: {maxAiCredits?: unknown}; + }; + const cap = parsed.apiProxy?.maxAiCredits; + if (typeof cap === "number" && Number.isFinite(cap)) { + return cap; + } + } catch { + // Unreadable or unparseable candidate: fall through to the next. + } + } + return DEFAULT_MAX_AI_CREDITS; +}; diff --git a/workflows/review/lib/router.test.ts b/workflows/review/lib/router.test.ts index cb31663..6ab460d 100644 --- a/workflows/review/lib/router.test.ts +++ b/workflows/review/lib/router.test.ts @@ -1,5 +1,11 @@ import {describe, it, expect} from "vitest"; +import { + clampBudgetToCreditCap, + DEFAULT_MAX_AI_CREDITS, + resolveCreditCap, +} from "./credit-cap"; + import { ALWAYS_ON_LENSES, computeRunBudget, @@ -386,6 +392,151 @@ describe("computeRunBudget: scaling", () => { }); }); +describe("clampBudgetToCreditCap", () => { + it("records but does not clamp a cap that covers the tier budget", () => { + const clamped = clampBudgetToCreditCap(DEFAULT_TIER_BUDGETS.high, 2500); + expect(clamped).toEqual({ + ...DEFAULT_TIER_BUDGETS.high, + effectiveCreditCap: 2500, + }); + expect(clamped.capClamped).toBeUndefined(); + }); + + it("scales every soft target to the reserve-adjusted cap, not the cap", () => { + // The budget-shed incident shape: the high tier ($10) planned inside a + // 400-credit ($4) cap, so the run died at the api-proxy mid-validation. + // The soft dollar target is 75% of the cap (the landing reserve): the + // acceptance re-run showed a soft target equal to the hard cap still + // dies at it (416/400), because spend is unobservable mid-run and + // in-flight work bills after the last observable checkpoint. + const clamped = clampBudgetToCreditCap(DEFAULT_TIER_BUDGETS.high, 400); + expect(clamped).toEqual({ + ...DEFAULT_TIER_BUDGETS.high, + maxReviewerInvocations: 3, // floor(12 * 0.3) + maxToolCallsPerFinding: 2, // floor(8 * 0.3) + maxTotalToolCalls: 36, // floor(120 * 0.3) + maxWallClockMinutes: 6, // floor(20 * 0.3) + maxUsd: 3, // 4 * 0.75 + effectiveCreditCap: 400, + capClamped: true, + }); + }); + + it("leaves a tier alone only when its target clears the reserve", () => { + // $10 tier under a 1400-credit ($14) cap: 75% of the cap is $10.50, + // above the tier's own $10 target, so nothing needs resizing. + const clamped = clampBudgetToCreditCap(DEFAULT_TIER_BUDGETS.high, 1400); + expect(clamped).toEqual({ + ...DEFAULT_TIER_BUDGETS.high, + effectiveCreditCap: 1400, + }); + // $10 tier under a 1200-credit ($12) cap: the reserve-adjusted target + // ($9) is below the tier's $10, so the clamp engages. + const tight = clampBudgetToCreditCap(DEFAULT_TIER_BUDGETS.high, 1200); + expect(tight.capClamped).toBe(true); + expect(tight.maxUsd).toBe(9); + }); + + it("never drops below the trivial tier's floors", () => { + const floor = DEFAULT_TIER_BUDGETS.trivial; + const clamped = clampBudgetToCreditCap(DEFAULT_TIER_BUDGETS.high, 10); + expect(clamped.maxReviewerInvocations).toBe( + floor.maxReviewerInvocations, + ); + expect(clamped.maxToolCallsPerFinding).toBe( + floor.maxToolCallsPerFinding, + ); + expect(clamped.maxTotalToolCalls).toBe(floor.maxTotalToolCalls); + expect(clamped.maxWallClockMinutes).toBe(floor.maxWallClockMinutes); + // The dollar target has no floor: a floor above the cap would defeat + // the cap. 75% of the $0.10 cap, within float precision. + expect(clamped.maxUsd).toBeCloseTo(0.075, 10); + expect(clamped.capClamped).toBe(true); + }); + + it("treats a zero or negative cap as explicitly uncapped", () => { + expect(clampBudgetToCreditCap(DEFAULT_TIER_BUDGETS.high, -1)).toEqual( + DEFAULT_TIER_BUDGETS.high, + ); + expect(clampBudgetToCreditCap(DEFAULT_TIER_BUDGETS.high, 0)).toEqual( + DEFAULT_TIER_BUDGETS.high, + ); + }); + + it("applies through computeRunBudget via config.maxAiCredits", () => { + const budget = computeRunBudget("high", false, { + ...baseConfig, + maxAiCredits: 400, + }); + expect(budget.tier).toBe("high"); + expect(budget.capClamped).toBe(true); + expect(budget.maxUsd).toBe(3); + }); +}); + +describe("resolveCreditCap", () => { + const noFs = { + existsSync: (): boolean => false, + readFileSync: (): string => { + throw new Error("unexpected read"); + }, + }; + + it("prefers the REVIEW_MAX_AI_CREDITS frontmatter mirror", () => { + expect( + resolveCreditCap( + {REVIEW_MAX_AI_CREDITS: "2500", GH_AW_MAX_AI_CREDITS: "400"}, + noFs, + ), + ).toBe(2500); + }); + + it("falls back to GH_AW_MAX_AI_CREDITS", () => { + expect(resolveCreditCap({GH_AW_MAX_AI_CREDITS: "400"}, noFs)).toBe(400); + }); + + it("ignores non-numeric and empty env values and keeps looking", () => { + expect( + resolveCreditCap( + {REVIEW_MAX_AI_CREDITS: "lots", GH_AW_MAX_AI_CREDITS: ""}, + noFs, + ), + ).toBe(DEFAULT_MAX_AI_CREDITS); + }); + + it("reads apiProxy.maxAiCredits from a visible awf config", () => { + const {fs} = fakeFs({ + "/tmp/gh-aw/awf-config.json": JSON.stringify({ + apiProxy: {maxAiCredits: 400}, + }), + }); + expect(resolveCreditCap({}, fs)).toBe(400); + }); + + it("prefers the RUNNER_TEMP awf config over the /tmp fallback", () => { + const {fs} = fakeFs({ + "/rt/gh-aw/awf-config.json": JSON.stringify({ + apiProxy: {maxAiCredits: 250}, + }), + "/tmp/gh-aw/awf-config.json": JSON.stringify({ + apiProxy: {maxAiCredits: 400}, + }), + }); + expect(resolveCreditCap({RUNNER_TEMP: "/rt"}, fs)).toBe(250); + }); + + it("survives an unparseable awf config and falls through", () => { + const {fs} = fakeFs({ + "/tmp/gh-aw/awf-config.json": "not json", + }); + expect(resolveCreditCap({}, fs)).toBe(DEFAULT_MAX_AI_CREDITS); + }); + + it("defaults to gh-aw's baked-in cap when nothing is discoverable", () => { + expect(resolveCreditCap({}, noFs)).toBe(DEFAULT_MAX_AI_CREDITS); + }); +}); + /* -------------------------------------------------------------------------- */ /* Misrouted floor */ /* -------------------------------------------------------------------------- */ @@ -606,6 +757,21 @@ describe("runCli", () => { expect(JSON.parse(written[ROUTING_OUT])).toEqual(json); }); + it("clamps the run budget to the credit cap from the environment", () => { + const {fs} = fakeFs({ + [FILES_PATH]: JSON.stringify([ + {path: "db/migrations/0001.sql", status: "added"}, + ]), + [ROUTING_CONFIG_PATH]: + "**/migrations/** tier=high lens=data-migrations", + }); + const json = runCli(fs, ".", {REVIEW_MAX_AI_CREDITS: "400"}); + expect(json.runBudget.tier).toBe("high"); + expect(json.runBudget.capClamped).toBe(true); + expect(json.runBudget.maxUsd).toBe(3); + expect(json.runBudget.effectiveCreditCap).toBe(400); + }); + it("accepts the {files:[...]} wrapper and degrades safely without a ROUTING config", () => { const {fs, written} = fakeFs({ [FILES_PATH]: JSON.stringify({ diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index 2899618..8b9692f 100644 --- a/workflows/review/lib/router.ts +++ b/workflows/review/lib/router.ts @@ -30,6 +30,7 @@ * sentence about the change itself. Prose stays with the lens sub-agents. */ +import {clampBudgetToCreditCap, resolveCreditCap} from "./credit-cap"; import {KNOWN_LENSES} from "./finding-schema"; import type {Lens} from "./finding-schema"; import { @@ -167,6 +168,19 @@ export type RunBudget = { maxWallClockMinutes: number; /** Soft spend ceiling (USD) the orchestrator should target. */ maxUsd: number; + /** + * The effective per-run AI-credit cap this budget was checked against + * (credits; 1 credit = $0.01), when one was known. Absent when the cap + * was unknown or explicitly uncapped. + */ + effectiveCreditCap?: number; + /** + * True when {@link effectiveCreditCap} is tighter than the tier's table + * budget and the soft targets above were scaled down to fit it. The + * orchestrator treats a clamped budget as already-tight: dispatch + * conservatively and expect to shed (review.md Step 3 Phase 3). + */ + capClamped?: boolean; }; export type RouterConfig = { @@ -188,6 +202,15 @@ export type RouterConfig = { misroutedFloorTier?: RiskTier; /** Tier assigned to a source file that matches no risk rule. Defaults to "low". */ defaultTier?: RiskTier; + /** + * Effective per-run AI-credit cap (credits; 1 credit = $0.01). When set + * and positive, the selected tier budget is clamped so the soft targets + * never promise more work than the hard cap can pay for + * ({@link clampBudgetToCreditCap}). Zero or negative means explicitly + * uncapped; undefined means unknown (no clamp). The CLI resolves this + * from the environment via {@link resolveCreditCap}. + */ + maxAiCredits?: number; }; /** Per-file routing decision. */ @@ -588,6 +611,7 @@ const tierForFile = ( * The single budget rule: scale by the highest touched tier, with one * floor for misrouted PRs. No per-lens knobs. When misrouted and the floor tier * outranks the touched tier, the floor's budget is used and `floored` is set. + * The selected budget is then clamped to `config.maxAiCredits` when known. */ export const computeRunBudget = ( highestTier: RiskTier, @@ -604,7 +628,10 @@ export const computeRunBudget = ( floored = true; } - return {...table[effectiveTier], tier: effectiveTier, floored}; + return clampBudgetToCreditCap( + {...table[effectiveTier], tier: effectiveTier, floored}, + config.maxAiCredits, + ); }; /* -------------------------------------------------------------------------- */ @@ -873,6 +900,7 @@ type FsLike = { * the {@link RoutingJson} shape. Factored out (fs injected) so it is testable * without touching the real filesystem. Returns the written JSON. */ + /** Accept either a bare `[{path,status}]` array or a `{files:[…]}` wrapper. */ const extractFileList = (raw: unknown): unknown[] => { if (Array.isArray(raw)) { @@ -882,7 +910,11 @@ const extractFileList = (raw: unknown): unknown[] => { return Array.isArray(wrapped) ? wrapped : []; }; -export const runCli = (fs: FsLike, repoRoot = "."): RoutingJson => { +export const runCli = ( + fs: FsLike, + repoRoot = ".", + env: Record = {}, +): RoutingJson => { const readText = (p: string): string => fs.readFileSync(p, "utf8"); const repoPath = (p: string): string => repoRoot === "." ? p : `${repoRoot}/${p}`; @@ -938,6 +970,7 @@ export const runCli = (fs: FsLike, repoRoot = "."): RoutingJson => { reviewerRules, lensRules: routingFileConfig.lensRules, riskRules: routingFileConfig.riskRules, + maxAiCredits: resolveCreditCap(env, fs), }); const json = toRoutingJson( result, @@ -956,5 +989,5 @@ export const runCli = (fs: FsLike, repoRoot = "."): RoutingJson => { // Run only when executed directly (review.md Step 3), never on import (tests). if (typeof require !== "undefined" && require.main === module) { const fs = require("node:fs") as FsLike; - runCli(fs, process.env.REVIEW_REPO_ROOT ?? "."); + runCli(fs, process.env.REVIEW_REPO_ROOT ?? ".", process.env); } diff --git a/workflows/review/review.md b/workflows/review/review.md index 5755f4c..500de9b 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -228,6 +228,15 @@ pre-agent-steps: # (-1) so reviews are never skipped on a busy PR day; the per-run cap below # still bounds the cost of any single review. max-daily-ai-credits: -1 +# Explicit per-run cap (matches the gh-aw default). The cap is enforced by the +# firewall api-proxy on the runner side and is not otherwise visible to the +# agent process, so it is mirrored into the agent's environment below; the +# router clamps its soft budget targets to the mirror so a run never plans +# more work than the hard cap can pay for. KEEP THE TWO VALUES IN SYNC — here +# and in any consumer override that changes `max-ai-credits`. +max-ai-credits: 1000 +env: + REVIEW_MAX_AI_CREDITS: "1000" --- # PR Reviewer @@ -769,13 +778,20 @@ claims anyway, and surface the gap as a skipped dimension (`claim validation`) w note in Step 6, so the author knows they were not double-checked this run. **Run out of budget gracefully: always land the review.** Two hard ceilings kill a -run that overruns: the per-run AI-credits cap (gh-aw's baked-in default; the -frontmatter only disables the *daily* cap) and the job's `timeout-minutes`. A run -that dies at a hard ceiling costs everything and delivers nothing, so a hard -ceiling must never be what stops you: treat the router's soft targets -(`runBudget`, Step 3) as the point to start landing. You cannot observe your own -credit spend (nothing reports credits consumed back to you mid-run), so never -estimate dollars; watch the signals you can observe, as spend proxies: +run that overruns: the per-run AI-credits cap (the frontmatter's +`max-ai-credits`; the daily cap is disabled separately) and the job's +`timeout-minutes`. A run that dies at a hard ceiling costs everything and +delivers nothing, so a hard ceiling must never be what stops you: treat the +router's soft targets (`runBudget`, Step 3) as the point to start landing. The +router clamps those targets to the effective credit cap (the +`REVIEW_MAX_AI_CREDITS` mirror of `max-ai-credits`) with a landing reserve +held back: the clamped `maxUsd` is 75% of the cap, not the cap itself, because +spend is unobservable mid-run and work already in flight bills after your last +checkpoint, so a run that sheds exactly at the cap still dies at it. When +`runBudget.capClamped` is true the cap is tighter than the tier's normal +budget — dispatch conservatively from the start and expect to shed. Treat +`maxUsd` as the landing target, never as money you may finish spending. Nothing reports exact credits consumed back to you +mid-run, so watch the signals you can observe, as spend proxies: - **Elapsed wall-clock** vs `runBudget.maxWallClockMinutes`: diff `date +%s` against the run start you recorded in Step 1 at each later checkpoint. This is @@ -783,11 +799,24 @@ estimate dollars; watch the signals you can observe, as spend proxies: the credits cap. - **Dispatch count** vs `runBudget.maxReviewerInvocations`: reviewers and lenses already dispatched plus still pending. +- **Estimated credits** vs `runBudget.maxUsd × 100`: every finished sub-agent + reports its tokens in-band (the `subagent_tokens` line of its result's + `` block). Estimated run credits ≈ the sum of `subagent_tokens` over + completed sub-agents ÷ 5,000. (Derivation: measured runs average roughly + 9,000 summed tokens per credit, and sub-agent tokens are only part of total + spend — your own orchestration turns are unmetered — so ÷5,000 folds in the + safety margin. An estimate, not an invoice: use it to shed, never to justify + spending more.) - **Run-wide investigation usage** vs `runBudget.maxTotalToolCalls`: one line per authorised call in `/tmp/gh-aw/review/investigation-journal.log` (`wc -l`). - **Trajectory**: an unusually large diff, many sub-agents still pending, many turns already spent. +Two checkpoints are mandatory, not judgment calls: recompute every proxy (1) +immediately after the last finder returns, BEFORE starting Phase 3 validation +— validation is itself model work, and dying there wastes findings already in +hand — and (2) before dispatching each additional wave of reviewers. + When any proxy passes roughly three-quarters of its soft target (or the trajectory is clearly expensive), stop starting new work and shed remaining work in this order: