Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/review-budget-cap-clamp.md
Original file line number Diff line number Diff line change
@@ -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.
145 changes: 145 additions & 0 deletions workflows/review/lib/credit-cap.ts
Original file line number Diff line number Diff line change
@@ -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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note (non-blocking): LANDING_RESERVE_RATIO = 0.75 reads as the reserve held back, but it is the fraction of the cap the soft target aims at (softCapUsd = capUsd * LANDING_RESERVE_RATIO, line 78); the reserve is the complementary 0.25, as the "A quarter of the cap" docstring says. No behavioral defect — the tests pin 0.75 — but the inverted name invites a future "fix" toward 0.25 that would quietly quarter every clamped budget. A name like LANDING_TARGET_RATIO would match the arithmetic.

Lower-confidence observations (non-blocking)
  • credit-cap.ts:114resolveCreditCap checks the hand-synced REVIEW_MAX_AI_CREDITS mirror above the machine-enforced awf-config. No defect today (the in-repo consumer omits the mirror and falls through to the awf-config's true cap), but a consumer that set a stale mirror would clamp to the wrong cap; a min-across-sources or awf-first order would be desync-safe.
  • review.md:244 — the in-repo consumer .github/workflows/review.md overrides max-ai-credits: 2500 with no REVIEW_MAX_AI_CREDITS mirror, so the KEEP-IN-SYNC contract is already unheld in-repo; it resolves correctly only via the awf-config fall-through.
  • review.md:807 — the 0.75 clamp reserve, the ~0.75 shed trigger, and the ÷5,000 token proxy compound to roughly 3× conservatism, so a tight-cap run may start shedding at ~1⁄3 of the cap. Consider carrying the margin in one place.


/**
* 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) ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): The !Number.isFinite(capCredits) guard is untested (the zero/negative branch on the same condition is). Low value in practice since resolveCreditCap filters non-finite values before the clamp, so this is only reachable via a direct call to the exported function.

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<string, string | undefined>,
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)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): No test covers a valid-JSON awf config whose apiProxy.maxAiCredits is absent or non-numeric — the exact case this typeof cap === "number" guard defends. The suite covers unparseable and numeric configs but not this fall-through, so a regression that dropped the type check wouldn't be caught. A small addition would close it:

it("falls through when a valid awf config lacks a numeric maxAiCredits", () => {
    const {fs} = fakeFs({"/tmp/gh-aw/awf-config.json": JSON.stringify({apiProxy: {}})});
    expect(resolveCreditCap({}, fs)).toBe(DEFAULT_MAX_AI_CREDITS);
});

return cap;
}
} catch {
// Unreadable or unparseable candidate: fall through to the next.
}
}
return DEFAULT_MAX_AI_CREDITS;
};
166 changes: 166 additions & 0 deletions workflows/review/lib/router.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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 */
/* -------------------------------------------------------------------------- */
Expand Down Expand Up @@ -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({
Expand Down
Loading
Loading