Skip to content

review: clamp the run budget to the effective credit cap; land before the cap#249

Merged
jwbron merged 81 commits into
mainfrom
jwies/review-budget-cap
Jul 13, 2026
Merged

review: clamp the run budget to the effective credit cap; land before the cap#249
jwbron merged 81 commits into
mainfrom
jwies/review-budget-cap

Conversation

@jwbron

@jwbron jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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-credits therefore 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

  • Frontmatter: max-ai-credits: 1000 is now explicit (matches the gh-aw default) and mirrored into the agent environment as env: REVIEW_MAX_AI_CREDITS, with a keep-in-sync note for consumer overrides. gh-aw compiles env: to workflow-level env and the firewall's --env-all forwards it into the agent container (verified against a scratch compile and the v0.81.6 lock).
  • Router (lib/router.ts): resolveCreditCap resolves the effective cap (mirror env, GH_AW_MAX_AI_CREDITS, a visible awf config's apiProxy.maxAiCredits, then the 1000-credit default); clampBudgetToCreditCap scales the selected tier's soft targets proportionally, floored at the trivial tier's values, and marks runBudget.capClamped + runBudget.effectiveCreditCap in routing.json. Zero/negative caps mean explicitly uncapped; the pure route() core takes the cap as config, so non-CLI callers (eval suite) are unaffected.
  • Prompt (budget section): notes that runBudget is cap-clamped and that capClamped means dispatch-conservatively; adds an estimated-credits proxy (sum of in-band subagent_tokens over 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.
  • Re-running the webapp budget-shed behavior test on this branch is the acceptance check; expected: shed stages observed in order and a verdict lands with skipped-dimension notes.

Stacked on #243 (batch two).

Human steps (non-review)

  • Acceptance re-run (the budget-shed behavior test this PR exists to fix, per the Khan/webapp#40737 record): branch off webapp's review-bot-v1.4.0-preview (still alive), re-pin review-preview.md's pre-agent-steps ref and source: to jwies/review-budget-cap, set max-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.
  • Record the outcome here and in the round-two doc before merge.

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 that 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; expected shape is shed-early, land inside 400.

jwbron added 30 commits July 9, 2026 12:47
…live cases

Phase 1 of the live A/B eval plan (#232). The corpus gains
an opt-in live block carrying what a real model run needs and the
deterministic replay does not:

- prContext (PR title/description/author/base; the description is
  untrusted author text, so an adversarial case can carry its payload
  there or in the diff),
- a post-change file tree on disk next to the case, via a new
  <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json
  (a directory containing case.json is one case; its tree is never
  parsed as corpus JSON), and
- labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line
  window, mechanism keyword alternates). Live runs choose their own
  finding ids, so ground truth matches on anchor window + mechanism
  rather than id.

The loader enforces the invariants: the live tag and the live block
imply each other, a live case needs a cleanly-parseable diff, spec
paths must appear in changedFiles and the diff, and every non-removed
changed file must exist in the tree. loadLiveCorpus() returns the
subset. The live half lives in corpus/live.ts; loader.ts re-exports it
as the single public surface.

Ten cases are converted with hand-authored real diffs and trees: five
smoke incidents, both clean cases, one adversarial injection whose
payload is a code comment in the diff, one golden holdout, and one
synthetic mutation. Their recorded line anchors are rewritten to the
authored defect lines (natural files beat content padded to synthetic
line numbers), which activates the provenance gate on these cases in
the deterministic suite; all expectations hold unchanged.
…action and case staging

Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1
corpus format (#233).

agent-extract.ts turns review.md's '## agent:' sections into data
(name, description, pinned model, prompt body). It takes the markdown
as a string with no fs access, because the baseline arm of an A/B
reads the merge-base review.md via git show. Parsing is strict; a
malformed or model-less section throws listing every problem, since a
silently dropped agent would skew an eval arm without failing it. An
integration test extracts the real review.md (21 agents) and pins the
staging-root reference so a future rename fails a test instead of
silently staging nothing.

live-stage.ts materializes the production staging layout for one
live-enabled corpus case: pr-context.json (review.md Step 1's shape,
synthetic identity fields), full.diff / pr.diff / full-stripped.diff
(all the case diff; corpus diffs carry no generated files and no
pattern-triage pass runs), files.json + review-files.json with the
hasPatch cross-check derived from the diff parse, provenance.json,
routing.json from the deterministic router, an out/ directory, and
the post-change checkout copied from the case tree.
rewriteAgentPrompt swaps the production staging root for the staged
context dir. Everything sits behind an injected-fs seam and is
memfs-tested.

Model dispatch (phase 2c) is deliberately absent; it needs the Agent
SDK dependency decision and arrives separately.
… runner (phase 2c)

live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.

live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.
live-match.ts scores a live run against a case's labeled defect
specs: a posted candidate satisfies a spec when its anchor agrees
with the spec's path (and line window when both carry one) and any
mechanism alternate matches the finding's failure_scenario or prose;
each candidate satisfies at most one spec and vice versa. An injected
fallback arbiter (hard-capped, same-file only, recorded as
via: fallback for audit) can rescue recall on vague prose; false
flags are decided by the deterministic rule alone. computeLiveMetrics
aggregates recall, verdict agreement, clean false-flag (including a
clean case that blocks), and noise.

live-ab.ts is the arm orchestrator and CLI: baseline review.md from
git show <merge-base>, candidate from the working tree, both arms
over the same live corpus with everything else (corpus, lib, runner,
metrics, judge) from the candidate, per the plan's settled decision
to isolate the model seam. Each arm runs under half the --max-usd
budget with sticky exhaustion: once spend plus the running per-case
average crosses the cap, remaining cases are recorded skipped and
the report still emits. Spec-level regressions are diffed only over
cases both arms scored. Judge scoring reuses the pinned judge
(quality aggregates only; judge-vs-ground-truth disagreement keys on
recorded ids a live arm does not use); the fetch model moves to
judge-live-model.ts and live-judge.ts now imports it. runner.ts
gains an optional RunOptions.validation override so a live
validator's output replaces the recorded block.

Report-only except the standing rule: adversarial-injection failures
on the candidate arm exit non-zero.
Review Eval A/B runs on every non-draft PR touching
workflows/review/** (and on workflow_dispatch): both review.md arms
over the live corpus via live-ab.ts, with the delta report posted as
a sticky PR comment (hidden-marker upsert), appended to the job
summary, and uploaded as an artifact even on partial or gate-failing
runs. Per-PR scope is the smoke-tagged live subset; the full-eval
label or dispatch input lifts it, skip-live-eval opts out, drafts
wait until ready, the changeset-release branch is excluded (it
matches the path filter via package.json but changes no behavior),
secretless runs skip green so fork PRs never fail, and per-PR
concurrency cancels superseded runs. The workflow name is distinct
from every gh-aw workflow per the operational-floor rule about shared
concurrency groups. live-ab.ts gains --smoke-only and writes
out/live-ab-report.md alongside the JSON for the comment step.
… and vitest

Tree directories are byte-exact case fixtures paired with each case's
diff: prettier auto-formatting one would silently desync it from the
diff the provenance gate parses, and a tree may carry a *.test.ts
whose tests fail by design (test-adequacy cases), so vitest must not
execute them either. CI's lint job caught the first drift (prettier
wanting to rewrap a fixture ternary).
…rpus' into jwbron/live-eval-producer-staging
…s with the case id

Live agents choose their own finding ids, so every case's first
correctness finding was live-correctness-reviewer-1; ids were unique
within a case but collided across cases, and judge.ts's score join
requires arm-global uniqueness. Caught by the first real A/B run
(both arms completed, then judge aggregation threw). Ids are now
<caseId>:<id> from the moment of parsing, so claims.json, the
validator round-trip, the matcher, and the judge all see the same
namespaced id.
…eport instead of killing it

The first real A/B run spent both arms' budgets and then died in
judge aggregation, writing no report: the exact
everything-spent-nothing-posted failure mode the plan forbids. Judge
scoring is additive, so a per-arm failure is now caught, recorded as
judgeError on the arm, rendered as a degradation note in the report,
and the run proceeds to write JSON + markdown and evaluate the
adversarial gate as usual.
…A/B phase 5)

Packages the seeded-defect live-trial pattern (Khan/webapp#40678) as
a Claude Code skill so a trial costs an operator an afternoon instead
of a week of hand choreography. The skill collects required inputs
(seeded branch, human-authored defect table, arms, budget approval)
and refuses to improvise ground truth; sets up one isolated PR per
arm with per-arm trigger recipes and the distinct-workflow-name rule
(same-named gh-aw workflows share a per-PR concurrency group and
cancel each other); collects reviews, artifacts, and costs per run
with the known gh-aw artifact-bug tolerance; drives optional
lifecycle pushes; scores defect by defect with the deterministic
rule mirroring eval/live-match.ts plus audited manual judgment;
exports live-enabled corpus case skeletons with a sanitization gate
for private-repo content landing in this public repo; and cleans up
trial PRs, branches, and temporary workflows. Trials remain the
architecture-bet instrument; per-change evals belong to the corpus
A/B.

.gitignore narrows from .claude to .claude/* with a !.claude/skills/
carve-out: local agent state (settings, worktrees) stays untracked,
project skills are shared tooling and are committed.
…ity, code-rendered from the reconciler keep-list
…oad (allowed-paths must match staging-relative paths)
…ive case

The first live acceptance runs missed
incident-sql-missing-index:dm-missing-index-1 in all four arms: the
recorded finding is a data-migrations LENS finding, but live cases
carried no routerConfig lens rules, so the live roster never spawned
the specialist that catches it. Every live case whose recorded
finding belongs to a specialist lens now routes that lens on the
finding's file (seven cases across five lenses), mirroring how a
consumer ROUTING file would route the same paths in production.
…rpus' into jwbron/live-eval-producer-staging
@github-actions

This comment has been minimized.

Comment thread workflows/review/lib/credit-cap.ts Outdated
Comment thread workflows/review/lib/credit-cap.ts
* the {@link RoutingJson} shape. Factored out (fs injected) so it is testable
* without touching the real filesystem. Returns the written JSON.
*/

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): 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.

Comment thread workflows/review/lib/credit-cap.ts
# Conflicts:
#	workflows/review/eval/corpus/live.ts
Comment on lines +791 to +792
router clamps those targets to the effective credit cap (the
`REVIEW_MAX_AI_CREDITS` mirror of `max-ai-credits`) with a landing reserve

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this read that env at all? Looking at https://github.github.com/gh-aw/reference/environment-variables it's not apparent to me how ENV variables are made available to an agent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It can, verified two ways: a scratch compile at v0.81.6 shows frontmatter env: rendering as a workflow-level env: block in the lock (so it's in the agent step's environment), and the agent step launches the firewall with sudo -E awf ... --env-all, which forwards the step environment into the agent container minus the excluded secrets. The router CLI runs inside that container and reads it from process.env. The docs page covers gh-aw's own runtime variables, and notably GH_AW_MAX_AI_CREDITS is only set in the later conclusion job, not the agent step; that gap is why the mirror exists.

Two caveats worth recording: consumers that override max-ai-credits (this repo's own wrapper sets 2500) must add the matching mirror when they bump to the release carrying this, or the clamp falls back to the 1000 default (safe direction, but under-uses the cap); and the clean long-term fix is upstream, asking gh-aw to expose the cap in the agent step env natively, which would delete the mirror and its keep-in-sync rule.

Comment thread workflows/review/lib/router.test.ts Outdated
…dit-cap.test.ts (router.test.ts hit the max-lines lint limit)
@jwbron jwbron changed the base branch from jwbron/review-skill-rule-quote to main July 13, 2026 19:12
jwbron added 3 commits July 13, 2026 12:16
…into jwies/review-budget-cap

# Conflicts:
#	workflows/review/lib/disciplines.test.ts
#	workflows/review/lib/render-comment.test.ts
#	workflows/review/lib/render-comment.ts
#	workflows/review/review.md
….ts (router.ts over the max-lines limit after the main merge)
…into jwies/review-budget-cap

# Conflicts:
#	workflows/review/lib/router.ts
@jwbron jwbron merged commit 25a1ef1 into main Jul 13, 2026
6 checks passed
@jwbron jwbron deleted the jwies/review-budget-cap branch July 13, 2026 19:34
@github-actions

Copy link
Copy Markdown
Contributor

Review Guidance

github-actions (3 files)
File Reason
credit-cap.ts New shared enforcement module (credit-cap resolution + budget clamp) that bounds every consumer PR's review, so a defect changes how reviews are scoped across all consumers.
router.ts Deterministic routing/budget core for the shared reviewer; this change wires the new clamp into computeRunBudget and threads env through runCli.
review.md The executable reviewer prompt plus its security-bearing frontmatter; this diff changes the frontmatter (max-ai-credits + the REVIEW_MAX_AI_CREDITS env mirror).

Common patterns

2 files: Split the DEFAULT_TIER_BUDGETS table and DEFAULT_MISROUTED_FLOOR_TIER constant verbatim out of router.ts into a new budgets.ts (router.ts was over the max-lines limit after the main merge), re-exported through router.ts so it stays the single routing entry point.

- export const DEFAULT_TIER_BUDGETS: Record<RiskTier, RunBudget> = { ... }  // removed from router.ts
+ import {DEFAULT_MISROUTED_FLOOR_TIER, DEFAULT_TIER_BUDGETS} from "./budgets";
+ export {DEFAULT_MISROUTED_FLOOR_TIER, DEFAULT_TIER_BUDGETS};               // re-export in router.ts
Excluded from review (1 file)

Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:

  • workflows/review/lib/budgets.ts — pattern-only (Common patterns)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants