review: clamp the run budget to the effective credit cap; land before the cap#249
review: clamp the run budget to the effective credit cap; land before the cap#249jwbron wants to merge 9 commits into
Conversation
🦋 Changeset detectedLatest commit: 03960c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ve credit cap; land before the cap The tier budget table assumes the workflow's $10 ceiling, but the per-run credit cap is enforced runner-side and was invisible to the agent, so a tighter consumer cap produced soft targets the hard cap could not pay for: the 400-credit budget-shed behavior test planned against the high tier's $10 targets, spent 390 credits on the finder phase, and was killed by the api-proxy mid-persist with findings in hand and nothing posted. Three changes close the gap: - The frontmatter sets max-ai-credits: 1000 explicitly and mirrors it into the agent environment as REVIEW_MAX_AI_CREDITS (consumers that override the cap keep the two in sync). - The router resolves the effective cap (mirror, GH_AW_MAX_AI_CREDITS, a visible awf config, then the 1000-credit gh-aw default) and clamps the selected tier budget proportionally, floored at the trivial tier's values; routing.json carries capClamped and effectiveCreditCap so the orchestrator and post-hoc audits see the clamp. - The budget prompt adds an estimated-credits proxy (in-band subagent_tokens sum / 5000) and two mandatory checkpoints, the critical one after the last finder returns and before claim validation, the exact boundary the observed run died at.
4370d5b to
78e501e
Compare
|
Restacked for a linear chain: this PR now bases on jwbron/review-skill-rule-quote (#248), so the full order is #243 -> #244 -> #245 -> #247 -> #248 -> #249 -> #250, with #246 (mode dial) the one sibling off #247 while its eval updates land. Content unchanged (cherry-picks 78e501e / 45cd748); reset any local state to origin. Acceptance re-run mechanics for this PR (the budget-shed behavior test, per the Khan/webapp#40737 record): the old scaffold base branch (review-eval/budget-shed-base) was deleted, but webapp's review-bot-v1.4.0-preview branch is still alive. Recreate: branch off review-bot-v1.4.0-preview, re-pin review-preview.md's pre-agent-steps ref and source: to jwies/review-budget-cap (branch ref), set max-ai-credits 400, gh aw compile, push; stack a copy of the seeded PR on it and confirm the run lands a verdict with skipped-dimension notes instead of dying at the cap. |
router.ts crossed the 1000-line lint budget with the clamp addition; the cap discovery (resolveCreditCap), the clamp (clampBudgetToCreditCap), and the gh-aw default constant move to their own module by concern. The router imports both and stays the single CLI entry point; tests for the moved functions import from the new module. Prettier fix in router.test.ts rides along.
Review live A/BBaseline:
Adversarial hard gate: PASSED on the candidate arm. Agent failures
Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. |
Behavior-test record: budget shed re-run (jwies/review-budget-cap pin, max-ai-credits 400)Run: 29056727919, agent job failure, ~8m agent runtime. Cost: 394.5 of 400 credits (proxy denial at 416.8/400 in-flight). No review posted. Outcome vs pass criteria: FAIL, but the failure moved one layer up.
Scaffold ( |
…edit cap The acceptance re-run showed the clamp engaging and the run still dying at the hard cap (416/400 credits, mid-skill-audit): the clamped soft target equaled the cap, spend is unobservable mid-run, and requests in flight at the last checkpoint bill after it. The clamp now sets the soft dollar target to 75% of the effective cap (LANDING_RESERVE_RATIO) and scales the other targets to match, so shedding against the targets lands the run inside the ceiling. The clamp also engages whenever the tier's own target sits above the reserve-adjusted cap, not only above the raw cap. review.md tells the orchestrator maxUsd is a landing target, never money it may finish spending. Acceptance: re-run the budget-shed test on the kept-alive webapp scaffold at cap 400.
…ies/review-budget-cap
… tmp/skill-rule-quote
…ies/review-budget-cap
Review Guidancegithub-actions (3 files)
|
| * 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; |
There was a problem hiding this comment.
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:114—resolveCreditCapchecks the hand-syncedREVIEW_MAX_AI_CREDITSmirror 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.mdoverridesmax-ai-credits: 2500with noREVIEW_MAX_AI_CREDITSmirror, 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.
| apiProxy?: {maxAiCredits?: unknown}; | ||
| }; | ||
| const cap = parsed.apiProxy?.maxAiCredits; | ||
| if (typeof cap === "number" && Number.isFinite(cap)) { |
There was a problem hiding this comment.
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);
});| * the {@link RoutingJson} shape. Factored out (fs injected) so it is testable | ||
| * without touching the real filesystem. Returns the written JSON. | ||
| */ | ||
|
|
There was a problem hiding this comment.
nitpick (non-blocking): This PR inserts a blank line after the runCli doc-comment's */, so that doc block now floats above extractFileList (which has its own comment) rather than sitting on runCli. Move the Read + parse the staged inputs... block down to sit immediately above export const runCli.
| ): RunBudget => { | ||
| if ( | ||
| capCredits === undefined || | ||
| !Number.isFinite(capCredits) || |
There was a problem hiding this comment.
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.
Phase 1 of the live A/B eval plan (#232): the eval corpus learns to carry real change content so a future live producer can run the actual model sub-agents against it. ## Format A case may now carry an opt-in `live` block (`corpus/live.ts`, re-exported through the loader): - `prContext`: PR title/description/author/base branch, mirroring production `pr-context.json`. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff. - An on-disk post-change file tree, via a new `<id>/case.json` + `<id>/tree/` layout coexisting with flat `<id>.json`. A directory containing `case.json` is one case; its tree is never parsed as corpus JSON. - `mustCatchSpecs` / `mustNotFlagSpecs`: labeled defects as (path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window plus mechanism rather than id; the Phase 3 matcher consumes these. Enforced invariants: the `live` tag and the block imply each other; a live case needs a cleanly-parseable diff (fail-open is fine in production, an authoring error here); spec paths must appear in `changedFiles` and the diff; every non-removed changed file must exist in the tree. `loadLiveCorpus()` returns the subset. ## Cases Ten cases converted with hand-authored real diffs and trees: the five smoke incidents (money rounding, auth bypass, cache key, race condition, missing index), both clean cases, the adversarial injection case (payload is a code comment in the diff instructing the reviewer to approve), the golden authz holdout, and the money-payments synthetic mutation. Recorded line anchors are rewritten to the authored defect lines; natural files beat content padded to synthetic line numbers, and every anchor is asserted to be an added line so the provenance gate (now active on these cases) keeps them. The trial-content port landed as #235, stacked on this PR. One acceptance-driven change also landed here: the phase 4 runs missed `incident-sql-missing-index` in all four arms because live cases carried no `routerConfig` lens rules, so specialist lenses never spawned; every live case whose ground truth belongs to a specialist lens now routes that lens on the finding's file (see the "route the specialist lens on each live case" commit). ## Test plan: - `pnpm run test --run`: 659 tests green, including 17 new loader tests (memfs) covering the live block, both layouts, tree validation, and the tree-JSON exclusion. - The deterministic suite runs the ten converted cases unchanged (same verdicts, comment counts, must-catch sets), now with the provenance gate active on them. - `pnpm run typecheck` and eslint clean on the three touched TS files. ## Next steps (human) 1. Review the ten authored diffs and trees for realism; they are synthetic content a live model will read, so plausibility matters more than in ordinary fixtures. Spot-check that each case's recorded finding still describes the authored defect (anchors were rewritten to the authored lines). 2. This is the root of the stack: review and undraft first; #234 and #235 rebase onto it. 3. No live validation needed here; the deterministic suite (`pnpm run test --run`) fully gates it. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #233
# Conflicts: # workflows/review/eval/corpus/live.ts
Closes the budget-shed gap the v1.4.0-preview behavior test exposed (Khan/webapp#40737, run 29051903691): the tier budget table is sized inside the workflow's assumed $10 ceiling, but the per-run credit cap is enforced runner-side by the firewall api-proxy and is invisible in-container. A consumer with a tighter
max-ai-creditstherefore got soft targets the hard cap could not pay for: the 400-credit test run planned against the high tier's $10 targets (its own words: the cap was 'unobservable to me mid-run'), shed only the opt-in reviewers, spent 390 credits on the finder phase, and was killed by the api-proxy mid-persist with findings in hand and nothing posted.Changes
max-ai-credits: 1000is now explicit (matches the gh-aw default) and mirrored into the agent environment asenv: REVIEW_MAX_AI_CREDITS, with a keep-in-sync note for consumer overrides. gh-aw compilesenv:to workflow-level env and the firewall's--env-allforwards it into the agent container (verified against a scratch compile and the v0.81.6 lock).lib/router.ts):resolveCreditCapresolves the effective cap (mirror env,GH_AW_MAX_AI_CREDITS, a visible awf config'sapiProxy.maxAiCredits, then the 1000-credit default);clampBudgetToCreditCapscales the selected tier's soft targets proportionally, floored at the trivial tier's values, and marksrunBudget.capClamped+runBudget.effectiveCreditCapin routing.json. Zero/negative caps mean explicitly uncapped; the pureroute()core takes the cap as config, so non-CLI callers (eval suite) are unaffected.capClampedmeans dispatch-conservatively; adds an estimated-credits proxy (sum of in-bandsubagent_tokensover completed sub-agents divided by 5,000, derivation documented inline from measured runs at ~9,000 summed tokens per credit); makes two proxy checkpoints mandatory, the critical one immediately after the last finder returns and before Phase 3 validation, the exact boundary the observed run died at.With the clamp, the 400-credit scenario's targets become 4 invocations / 8 minutes / $4, so the existing three-quarters rule trips during the finder phase and the run sheds stages 2-3 and lands a partial review instead of dying.
Test plan
pnpm run test --run: 708 tests green (new: clamp scaling incl. the 400-credit incident shape, trivial floors, uncapped sentinels, resolveCreditCap source priority and fallbacks, runCli env integration).pnpm run typecheck: clean.Stacked on #243 (batch two).
Human steps (non-review)
review-bot-v1.4.0-preview(still alive), re-pinreview-preview.md'spre-agent-stepsref andsource:tojwies/review-budget-cap, setmax-ai-credits: 400,gh aw compile, push, and stack a copy of the seeded PR on it. Pass = the run lands a verdict with skipped-dimension notes instead of dying at the cap. ~$4 of credits.Update 2026-07-09: landing reserve
The acceptance re-run showed the clamp engaging and the run still dying at the hard cap (416/400 mid-skill-audit): the clamped soft target equaled the cap, and spend that is unobservable mid-run plus in-flight work billing after the last checkpoint left no room to land. The clamp now sets the soft dollar target to 75% of the effective cap (
LANDING_RESERVE_RATIO) and scales the other targets to match, engages whenever the tier target sits above the reserve-adjusted cap (not only above the raw cap), and review.md now tells the orchestrator thatmaxUsdis a landing target, never money it may finish spending. Acceptance: re-run the budget-shed test on the kept-alive webapp scaffold at cap 400; expected shape is shed-early, land inside 400.