feat(agent): CostBudget - hard per-run USD spend ceiling - #10
Conversation
Every retry/iteration cap in the loop is bounded individually (API retries 3, compression 3, length-continues 3, empty-content 3, invalid-JSON 6, iterations via IterationBudget) but their PRODUCT had no dollar bound — during the 2026-07-24 incident one chat turn burned ~$35 producing nothing, at a $455/hr account-level peak. CostBudget mirrors IterationBudget: the caller creates one, the parent agent and every delegate_task subagent share it, and spend accumulates from estimate_usage_cost after each successful API call. When the ceiling is crossed the loop stops making tool calls and produces a final no-tools summary of the work already paid for (the same graceful exit as the iteration cap — never a hard error). A run that STARTS over-budget (recovery re-prompt on the same agent) returns a static message with zero API calls. Opt-in: cost_budget=None (the default) preserves existing behavior for every current caller. The result dict gains cost_limited so callers can log/surface the stop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F1 — run_conversation unconditionally replaced the iteration budget, silently handing every delegate child a fresh full budget: the documented shared cap never constrained children (pre-existing; found while verifying the CostBudget's 'mirrors IterationBudget' claim). The reset is now ownership-guarded: self-created budgets keep the per-turn refresh (CLI/gateway unchanged); externally-provided shared pools survive, so children genuinely drain the parent's cap — same rule the CostBudget already followed. F3 — the salvage-summary fallback strings said 'iteration limit' / 'maximum iterations' even on the cost path; now reason-aware. F4 — the salvage summary call's own usage was invisible to the session counters and the CostBudget; it is now metered (best-effort, never breaks the summary path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Self-review (adversarial pass, requested by Aman)F1 (fixed in 6a5b8a3) — shared IterationBudget was silently broken. F3 (fixed) — salvage fallback copy said 'iteration limit' on the cost path. Now reason-aware. F4 (fixed) — the salvage summary call's own usage was invisible to session counters and the CostBudget. Now metered across all API-mode variants, best-effort, never breaks the summary path. Locked by Suite: 16 cost-budget tests + 239 adjacent (run_agent / guardrails / 1630 / 413 / subagent-interrupt) all passing. |
Code review — approve ✅Verified the PR head ( Findings (all low severity)
Cross-PR note (from the #11 review): Tests: 🤖 Generated with Claude Code |
…ost-limited completed semantics Cross-PR composition fix with #11 (feat/usage-listener): the salvage summary call's spend was metered into session counters and the CostBudget but never reached the per-call delta stream that hermes's UsageFlusher consumes. _meter_summary_usage now fires the listener with a payload matching #11's main-loop delta field-for-field, behind a getattr guard so it stays a no-op until both PRs land together, and with the same swallow-and-debug-log protection (a raising listener can never break the salvage flow). Also documents why cost-limited runs deliberately report completed=True (suppresses client-side whole-turn retries; consumers must check cost_limited), unlike iteration-capped runs. Tests: TestSummaryUsageListener (2) — exact-payload contract via an attribute-injected mock listener, and raising-listener resilience. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up pushed (eb5ed0e)Cross-PR composition fix (#10 × #11): Finding 3: added a comment at the Finding 1 (early-return dicts missing Tests: new 🤖 Generated with Claude Code |
GurneeshBudhiraja
left a comment
There was a problem hiding this comment.
Code Review — PR #10: feat(agent): CostBudget — hard per-run USD spend ceiling
Overview
Adds an opt-in, thread-safe CostBudget that mirrors IterationBudget: the caller creates one USD ceiling, the parent agent and every delegate_task subagent share it, and spend accrues from estimate_usage_cost after each priced API call. When the ceiling is crossed, the loop stops issuing tool calls and falls into the existing salvage-summary path (_handle_max_iterations), returning cost_limited=True. Motivated by the 2026-07-24 incident where individually-bounded caps multiplied into ~$35/turn of unbounded burn.
I read the diff plus the head versions of run_agent.py, the summary/metering path, and delegate_tool.py. The change is careful, narrowly scoped, and genuinely well-tested. No correctness or security blockers found. Notes below are mostly design/coordination.
Findings
🟢 The salvage-summary call(s) are not themselves gated by the budget (run_agent.py _handle_max_iterations, ~L5311+)
Once the ceiling trips, the exit path makes one summary call — and up to a second on the retry branch — that are metered after the fact but never blocked, even when already far over budget. On a large context these are real input-token spend, so the effective ceiling can be overshot by ~1–2 large calls. This is documented ("one bounded no-tools call") and is the same graceful-exit contract as the iteration cap, so it's acceptable; flagging only because the whole feature exists to bound spend. Metering them (F4) is the right call.
🟢 completed=True for cost-limited runs is a consumer footgun (run_agent.py ~L7321–7358)
Deliberate and clearly documented (suppresses client-side whole-turn retries that would re-spend the budget), but it diverges from the iteration-cap path, which reports completed=False. Any caller that keys off completed alone — not cost_limited — will treat a truncated salvage summary (or the zero-call "stopped before it started" message) as a fully successful turn. The consuming hermes PR (CHAT_MAX_COST_USD) must check cost_limited; worth calling out to any other consumer of the result dict too.
🟢 No validation on max_usd (CostBudget.__init__, ~L35)
A 0 or negative ceiling makes exceeded true immediately (_spent_usd >= max_usd), silently forcing every run to zero API calls and a static "stopped before it started" reply. A misconfigured/negative CHAT_MAX_COST_USD would disable the agent rather than fail loudly. Consider a max_usd > 0 guard (or at least a warning).
🟢 Best-effort overshoot within a single iteration (design, documented)
The ceiling is only checked at the top of the loop, so a single iteration that fans out to many parallel subagent API calls can overshoot before the next check. Mitigated because each subagent re-checks the shared budget in its own loop, and it's explicitly documented as best-effort. No action needed.
🟢 Minor: logging.debug vs logger.debug mix inside _meter_summary_usage — the outer swallow uses module-level logging.debug, the listener guard uses logger.debug. Harmless, just inconsistent.
Positives worth noting
- F1 is a real pre-existing bug fix, not just scaffolding.
run_conversationunconditionally rebuiltiteration_budgetevery turn, so delegate children each got a fresh full budget and the "shared cap" never constrained them. The ownership guard (_owns_iteration_budget) fixes that and is covered byTestIterationBudgetOwnership. - Opt-in default (
cost_budget=None) means zero behavior change for existing callers, and the top-of-loop check correctly catches both mid-run breach (summarize) and a pre-exhausted budget (zero API calls). - Placement of the check — before
api_call_count += 1/iteration_budget.consume(), and re-evaluated after everycontinue(compression/length/empty-content retries) — is exactly right. - Thread-safety is real (single lock, monotonic accumulator) and smoke-tested; unpriced calls correctly add nothing.
- Test coverage is strong: class semantics, mid-run breach → summary +
cost_limited, pre-exhausted → zero calls, unpriced no-trip, salvage metering into session counters + budget, reason-aware fallback copy, delegate inheritance, and the #11 usage_listener cross-PR contract behind a guardedgetattr. - No secrets logged; the warning line logs only spend/max/api_calls/model.
api_keyreachesestimate_usage_costvia the samegetattr(..., "")pattern as the existing main-loop call.
Verdict
Solid, defensive, well-tested fix that closes a genuine cost-safety gap and fixes a real pre-existing shared-budget bug along the way. No blockers. Suggest (1) a max_usd > 0 guard and (2) ensuring the hermes consumer keys off cost_limited rather than completed. The requested merge commit (not squash) is appropriate given the hermes submodule pin.
Conflict: delegate_tool.py child-agent constructor — kept both kwargs (usage_listener + cost_budget). Updated the #10-era composition test: with both PRs together a cost-limited run now emits two deltas (main-loop call + salvage summary), which is the intended composed behavior the getattr guard in _meter_summary_usage was built for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Why
2026-07-24 cost incident: OpenRouter burn peaked at $455/hr, with one chat turn burning ~$35 producing nothing. Every retry/iteration cap in the loop is bounded individually (API retries 3, compression 3, length-continues 3, empty-content 3, invalid-JSON 6, iterations via IterationBudget) — but their product had no dollar bound.
What
CostBudget— thread-safe shared USD ceiling, mirroringIterationBudget: caller creates one, the parent agent and everydelegate_tasksubagent share it, so delegation can't bypass it.estimate_usage_costafter each successful API call (unpriced calls add nothing — best-effort bound).cost_limitedso callers can log/surface the stop.cost_budget=Nonedefault → zero behavior change for every existing caller.Tests
tests/test_cost_budget.py— budget semantics (incl. thread-safety), normal completion under budget, mid-run breach → summary +cost_limited, pre-exhausted budget → zero API calls, unpriced calls don't trip, delegate child inherits the parent's budget. Also rantest_run_agent.py,test_agent_guardrails.py,test_1630_context_overflow_loop.py,test_413_compression.py,test_cli_interrupt_subagent.py— 250 passed, 12 skipped (baseline).Consumed by the hermes PR that sets
CHAT_MAX_COST_USD(default $5/turn) for interactive chat. Please use a merge commit (not squash) so the hermes submodule pin can point at a reachable SHA.🤖 Generated with Claude Code