feat(harness): add a max_cost_usd budget cap for agent runs - #6588
feat(harness): add a max_cost_usd budget cap for agent runs#6588guyoron1 wants to merge 10 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Site previewPreview: https://e41be1ba-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
06e1f49 to
69300c0
Compare
PR Summary by QodoAdd max_cost_usd budget caps for harness runs
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if !aggMetrics.OverBudget && exceedsCostBudget(aggMetrics.TotalCostUSD, h.MaxCostUSD) { | ||
| aggMetrics.OverBudget = true | ||
| printer.StepWarn(fmt.Sprintf("Over max_cost_usd budget ($%.4f > $%.4f) — halting further iterations", aggMetrics.TotalCostUSD, h.MaxCostUSD)) |
There was a problem hiding this comment.
1. runagent budget halt untested 📘 Rule violation ▣ Testability
No behavioral test verifies that exceeding max_cost_usd prevents a subsequent validation-loop iteration. Regressions in the marker assignment or loop guard could therefore pass the current predicate and serialization tests.
Agent Prompt
## Issue description
The new budget-halting path lacks a behavioral test proving that a subsequent validation-loop iteration is not started after aggregate cost exceeds `max_cost_usd`.
## Issue Context
Existing tests cover the budget predicate and metrics serialization independently, but not their integration with the `runAgent` retry loop.
## Fix Focus Areas
- internal/cli/run.go[1921-1923]
- internal/cli/run.go[2043-2045]
- internal/cli/run_test.go[4552-4585]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if aggMetrics.OverBudget { | ||
| break | ||
| } |
There was a problem hiding this comment.
2. make e2e-test not evidenced 📘 Rule violation ▣ Testability
The PR changes internal/cli but provides no evidence that the required make e2e-test run passed. The PR description reports package tests, build, and vet results, but not the required end-to-end suite.
Agent Prompt
## Issue description
Changes under `internal/cli` require `make e2e-test` to run and pass, but the PR provides no successful E2E result.
## Issue Context
The repository workflow classifies changed Go files as E2E-relevant and invokes `make e2e-test`. Run that suite and record the passing result in the PR or ensure the corresponding required CI check succeeds.
## Fix Focus Areas
- internal/cli/run.go[1921-1923]
- .github/workflows/e2e.yml[140-170]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 69300c0 |
|
/review |
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
|
/agentic_review |
| # Hard timeout enforced by the runner. The sandbox is killed after this. | ||
| timeout_minutes: 30 | ||
|
|
||
| # A hard cost cap (max_cost_usd) was added later — see ADR 0097. |
There was a problem hiding this comment.
1. adr 0024 edit unannounced 📘 Rule violation § Compliance
The PR modifies accepted ADR 0024, but the PR description does not identify that ADR or summarize the cross-reference addition. Accepted ADR edits must be explicitly called out even when the edit itself is a permitted annotation.
Agent Prompt
## Issue description
The PR description does not explicitly identify the edit to accepted ADR 0024 or summarize the new ADR 0097 cross-reference.
## Issue Context
Update the PR description to mention `ADR 0024` and state that a cross-reference to ADR 0097 was added; no source-code change is required.
## Fix Focus Areas
- docs/ADRs/0024-harness-definitions.md[422-422]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 2ddefc1 |
|
Hello!
|
|
/fs-reivew |
|
Also rebase to solve the Build Site problem. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit e4a5843 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head e4a5843. Two MEDIUM findings posted inline (ADR 0097 line 33, internal/cli/run.go line 2080). Neither overlaps the existing threads.
| [operational-observability.md](../problems/operational-observability.md)). | ||
|
|
||
| Claude Code reports `total_cost_usd` once, in the final result event of a | ||
| completed iteration, so no mechanism at this layer can interrupt an |
There was a problem hiding this comment.
MEDIUM — Design premise "no mechanism can interrupt an in-flight iteration" is false for the Claude runtime (native --max-budget-usd exists and is unused)
Verified at head e4a5843. Three in-diff passages state as fact that in-flight interruption is unavailable: this ADR's Context ("no mechanism at this layer can interrupt an iteration already in flight; the only enforceable boundary is between iterations"), docs/normative/harness-budget/v1/README.md lines 45-47 ("an iteration already in flight is never interrupted; the cap is soft by at most one iteration"), and docs/reference/harness-reference.md line 147 ("it cannot interrupt one long iteration still in progress"). The PR body repeats it.
But Claude Code ships --max-budget-usd <amount> ("Maximum dollar amount to spend on API calls (only works with --print)") — present in claude --help on 2.1.258 and in the anthropics/claude-code CHANGELOG: added in 2.0.28 ("SDK: added --max-budget-usd flag"), subagent enforcement fixed in 2.1.217. The harness sandbox image pins CLAUDE_CODE_VERSION=2.1.252 (images/sandbox/Containerfile:54), so the flag is available in every fleet run. buildRunCommand (internal/runtime/claude.go:302-310) already runs claude --print --verbose --output-format stream-json, and git grep -i max-budget-usd over the head tree returns nothing, so the native per-iteration cap was neither used nor evaluated. The "soft by one iteration" consequence in the ADR rests on that unverified premise.
Suggestion: Either
(a) pass --max-budget-usd <max_cost_usd - aggregate so far> from buildRunCommand (via RunParams) for the Claude runtime whenever max_cost_usd > 0, so one long iteration cannot blow past the cap. A budget trip yields an error result in the transcript, which the existing is_error path (run.go ~2091) already turns into a failed iteration, and the between-iteration latch then suppresses the retry with no new loop logic; or
(b) keep the between-iteration-only design but rewrite the three passages (ADR 0097 Context/Consequences, normative README "Enforcement boundary", harness-reference.md max_cost_usd entry) to say fullsend deliberately does not use the runtime's native cap and why (e.g. runtime neutrality — pi/opencode have no equivalent).
Either way, the docs should not assert that the capability does not exist.
There was a problem hiding this comment.
Fixed in 61d0964 — all three passages reworded: the iteration boundary is the runtime-agnostic choice (pi has no in-flight control), and Claude Code's native --max-budget-usd is named as an unused, possible tighter complement. Not wired here — docs stay truthful, feature stays out of a review-fix commit.
| // anyway. Cancelling the run's context here instead would reassign | ||
| // the ctx that the status-comment defer closes over, making every | ||
| // run report "cancelled". | ||
| if !budgetExhausted && exceedsCostBudget(aggMetrics.TotalCostUSD, maxCostUSD) { |
There was a problem hiding this comment.
MEDIUM — Cap is silently inert when the runtime reports zero cost; docs call it a "hard budget" without stating it depends on runtime-reported cost
Verified at head e4a5843. exceedsCostBudget (run.go:3192) is maxCostUSD > 0 && totalCostUSD >= maxCostUSD, and the only trigger is aggMetrics.TotalCostUSD, which aggregateRunMetrics (run.go:3173) sums from each iteration's runtime-reported value. Nothing in run.go distinguishes "cost $0" from "cost unknown" — there is no StepWarn on that path.
Under-count vectors confirmed in the tree:
- The Claude runtime sets
TotalCostUSDonly from the finalresultevent (claude.go:136,claude_progress.go:345), and the runtime's own test asserts 0 cost when no result event arrives (claude_progress_test.go:631) — so a crashed/killed iteration contributes $0 to the aggregate even though tokens were spent. - The pi runtime sums
msg.Usage.Cost.Total(pi_progress.go:336) from a plainpiCost{Total float64}struct (pi_progress.go:34), so a provider entry without pricing reports 0 per message. docs/guides/infrastructure/distributed-tracing.md:192already states fullsend has no pricing table and "accepts the reported total" as-is.
Yet the new normative README line 19 calls the field "A hard budget in USD for one run" with no qualification, and the runtime-coverage caveat appears nowhere in the PR's docs — only Claude Code is named. A repo that sets max_cost_usd on a pi/opencode harness with an unpriced provider, or whose Claude iterations crash, gets no enforcement and no signal.
Suggestion: After aggregateRunMetrics (line 2055), when maxCostUSD > 0 && metrics.NumTurns > 0 && metrics.TotalCostUSD == 0, emit a StepWarn such as "runtime reported no cost for this iteration; max_cost_usd cannot be enforced against it" (optionally add a cost_unreported marker in metrics.json / the root span). In docs/normative/harness-budget/v1/README.md, qualify "hard budget" as enforced against the runtime's self-reported cost estimate and link the cost data contract in distributed-tracing.md, noting per-runtime coverage (Claude: final result event only, zero when the iteration does not produce one; pi/opencode: only when the provider/model entry carries pricing).
There was a problem hiding this comment.
Fixed in 61d0964 — fullsend run now warns when a cap is set and a completed iteration reports no cost, and the normative spec gained a Cost reporting section naming the under-count vectors; "hard budget" is qualified as enforced against self-reported cost. Test added.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head e4a5843, supplementary to my earlier review: two MEDIUM findings from the second reviewer that were not relayed the first time (ADR 0097 relates_to line 5, internal/cli/budget_run_test.go line 58). Neither overlaps an existing thread.
| title: "97. Harness-level max_cost_usd budget cap" | ||
| status: Accepted | ||
| relates_to: | ||
| - operational-observability |
There was a problem hiding this comment.
MEDIUM — ADR 0097 decides the threat model's open "hard stop vs. human-in-the-loop" cost-budget question, but neither problem doc points back to it
Verified at head e4a5843. docs/problems/security-threat-model.md line 383 proposes cost budgets that "require human approval before further agent invocations", and its Open questions (line 402) ask "Should cost budgets trigger a hard stop or a human-in-the-loop approval flow?". This ADR answers that at per-run granularity (hard stop between iterations, no approval flow), yet security-threat-model.md is not listed under relates_to: and neither it nor operational-observability.md (the one doc that is listed, whose "How much does it cost?" section at lines 74-84 is the other passage that discusses this) carries any pointer to ADR 0097. git grep 0097 docs/problems/ on the head tree returns nothing.
The repo convention for an Accepted ADR is an inline pointer at the open question it decides, not a rewrite: see the same Open questions section of operational-observability.md (lines 191-196), where ADR 0041, 0021, 0087 and 0050 are each linked from the question they resolved (struck through when fully decided, annotated when partial). Without that, a reader of the threat model's DOS section cannot discover that the decision was made, and the "hard stop vs. approval" question stays open in the living doc after it was closed here.
Suggestion: add security-threat-model to relates_to:; annotate the line-402 open question ("Per-run hard stop decided in ADR 0097; per-repo/per-org budgets and an approval flow remain open"); and add a one-line pointer under the cost section of operational-observability.md (e.g. after line 84: "Per-run cap: ADR 0097"). This is the same pattern ADR 0050 used for its "bootstrapping" question.
There was a problem hiding this comment.
Fixed in 61d0964 — security-threat-model added to ADR 0097's relates_to with a Context mention, and the threat model's open question now carries the partial answer in place (per-run hard stop decided; per-repo/org budgets and approval flows remain open).
| " esac ;;\n" + | ||
| // `sandbox download <name> <remote> <local>`: create the local | ||
| // destination so SafeDownload's sanitize walk sees a directory. | ||
| " 'sandbox download') mkdir -p \"$5\"; exit 0 ;;\n" + |
There was a problem hiding this comment.
MEDIUM — The extraction-failure over_budget path is untested: the stub makes sandbox download always succeed, so the top-of-loop guard that exists for that path is never exercised
Verified at head e4a5843. docs/normative/harness-budget/v1/README.md lines 51-54 make the failed-repo-extraction case a first-class part of the contract ("The suppressed retry can follow a failed validation or a failed repository extraction"). In internal/cli/run.go both extraction-failure branches (forceRemoveAll pre-clear at ~2184 and SafeDownload at ~2207) continue past the bottom-of-loop retry check at ~2250-2258, so the only place over_budget can be recorded for them is the top-of-loop guard at ~1958-1961 — whose own comment says "This guard is what halts the extraction-failure continue paths".
None of the three tests in this file reach it. This stub line answers every sandbox download with mkdir -p; exit 0, so extraction always succeeds and TestRunAgent_BudgetHaltsValidationRetries / …CrossedOnFinalIterationIsNotMarked / …UnderBudgetStillRetries all stop via the bottom-of-loop break after a failed validation script. If the top-of-loop guard were deleted (or its aggMetrics.OverBudget = true dropped), all three tests still pass, and an extraction-failure run over budget would silently start another iteration and never write the marker.
Suggestion: parameterise useBudgetRunStub (or add a sibling) so sandbox download can exit 1, then add one case with max_iterations: 3, cost over the cap on iteration 1, and a failing download: assert exactly one RUN line in the log and over_budget: true in metrics.json. That pins the guard the normative contract depends on.
There was a problem hiding this comment.
Fixed in 61d0964 — the stub can now fail sandbox download; TestRunAgent_BudgetHaltsExtractionFailureRetries drives the continue path with the budget exhausted and asserts one invocation + over_budget: true.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 61d0964 |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 8b72440 |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit c06fdba |
c06fdba to
315e193
Compare
|
@rh-hemartin Going through these:
Head is 315e193. |
|
build passed on 315e193. All checks green now, blocked only on required review. |
|
No need for papel trail, just wanting someone with a use case or someone that will use it. We keep adding things that no one asked for. I'm fine with this, no worries. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass. One finding posted inline. Two additional MEDIUM findings below have no in-diff anchor (the specific lines fall outside this PR's diff hunks / outside files it touches), so they're included here instead:
MEDIUM — Merge-rules table omits max_cost_usd's presence-aware 0 semantics (docs/reference/harness-reference.md:179)
The field-details prose earlier in the same file (line 147) correctly states max_cost_usd merges by presence (absent inherits the base's cap; an explicit 0 in a child disables an inherited cap as unlimited). But the "Field merge rules" table further down the file has no row for max_cost_usd — it falls under the generic "Scalars (model, pre_script, policy, image, etc.)" row (line 179), which states the opposite rule (value-based, not presence-based, treating 0 as empty/no-override rather than an explicit override). A reader who only consults the table (not the prose above it) will misconfigure a child harness's cost cap.
Suggestion: add an explicit max_cost_usd row/footnote to the merge-rules table clarifying it merges by presence (nil vs. set), not by non-empty value, and that an explicit 0 means unlimited rather than inherit.
MEDIUM — New harness-level max_cost_usd collides in name with the pre-existing eval-case max_cost_usd threshold
docs/testing/functional-tests.md:165 already documents max_cost_usd as a per-eval-case post-run judge threshold (compared against metrics.json by the max_cost deterministic judge in eval.yaml). This PR adds a same-named but semantically different harness-level field (a mid-run hard stop that suppresses further retries) without touching functional-tests.md or the two problem docs that reference the pre-existing field: docs/problems/cross-run-memory.md:99 and docs/problems/flapping-convergence.md:51, both of which say cost is "already enforced via max_cost_usd in the functional test framework" — a sentence that is now ambiguous about which max_cost_usd is meant, since the harness-level field also exists. Confirmed via diff that this PR does not touch any of these three files, so the collision is a side effect of introducing the new field name.
Suggestion: disambiguate the two mechanisms wherever both could be confused — e.g. name them "harness max_cost_usd (hard stop, per-run)" vs. "eval max_cost_usd threshold (post-run judge)" in the harness reference, functional-tests.md, and the two problem-doc passages. A field rename isn't necessary since this is a documentation/mental-model collision, not a schema conflict.
| // run report "cancelled". | ||
| if !budgetExhausted && exceedsCostBudget(aggMetrics.TotalCostUSD, maxCostUSD) { | ||
| budgetExhausted = true | ||
| printer.StepWarn(fmt.Sprintf("Reached max_cost_usd budget ($%.4f of $%.4f) — no further iterations will start", aggMetrics.TotalCostUSD, maxCostUSD)) |
There was a problem hiding this comment.
MEDIUM — Budget StepWarn fires even when no retry is actually suppressed
When aggregate cost first crosses max_cost_usd, this line unconditionally logs StepWarn("Reached max_cost_usd budget ... — no further iterations will start"). aggMetrics.OverBudget (the metrics.json / OTEL over_budget marker) is deliberately latched only at two later points: the top-of-loop guard (~line 2107-2109) and the bottom-of-loop retry-suppression guard (~line 2415-2424) — by design, per the comment above this block, so the marker records an actually-suppressed retry.
But if the very same iteration that crosses the cap then passes validation, the loop breaks at line 2401 before ever reaching the retry-suppression check at line 2415, so OverBudget never gets set even though the StepWarn already printed "no further iterations will start." This is distinct from the already-fixed zero-cost StepWarn on this same line range (fixed in 61d0964, which addressed under-counting, not this timing gap).
Suggestion: either move/gate this StepWarn so it only fires when a retry is genuinely about to be suppressed (next to the two OverBudget = true sites), or reword it to something like "cap reached; remaining retries, if any, will be skipped" so it doesn't imply the run was halted by budget when metrics.json's over_budget stays false.
A runaway agent had no cost ceiling: max_iterations bounds how many times it retries, but nothing bounds what those retries cost. max_cost_usd is an optional per-harness cap (0 = unlimited, inherited by composition like timeout_minutes) checked against the aggregate cost after each iteration. Claude Code reports total_cost_usd only in the final result event of a completed iteration, so the cap cannot interrupt work already in flight — it stops the loop from starting another iteration. The check is at the top of the loop rather than the bottom so the two mid-iteration continue paths cannot buy an iteration the budget has already refused. The halt sets over_budget in metrics.json so a post-script can tell a deliberate stop from a crash. It records why the run stopped retrying and does not imply success: with a validation_loop the halt is only reachable after an iteration failed validation, so over_budget commonly accompanies a non-nil error. metrics.json also gains duration_seconds, which post-scripts can surface alongside cost. TestRunAgent_DoesNotCancelTheRunContext pins a constraint that is easy to reintroduce: cancelling the run's own ctx to stop the loop would rebind the variable the status-comment defer closes over, and defers run LIFO, so every run would report 'cancelled'. The test reads the source because the failure is invisible at the package boundary — the run still succeeds, only the reported status is wrong — and .codecov.yml excludes run.go from patch coverage. Signed-off-by: guy oron <goron@redhat.com>
Correctness: - Treat an aggregate cost exactly equal to max_cost_usd as exhausted (>=): a budget that is exactly spent buys no further iteration. - Reject non-finite max_cost_usd values (NaN, +/-Inf) during harness validation; either would silently disable the cap. - Make Harness.MaxCostUSD presence-aware (*float64) so a child harness can override an inherited positive cap with an explicit 0 (unlimited); absent still inherits, and runtime treats nil and 0 identically. - Set the over_budget marker only when the cap actually suppresses a retry that was otherwise due, instead of whenever a completed iteration's total crosses the cap, so the marker means "the cap stopped this run", not "the run was expensive". - Persist aggregate metrics on every exit after iteration metrics have been aggregated via one deferred best-effort writer, so fatal repository-extraction paths no longer lose cost/duration/over_budget; write failures are logged. Tests: - Behavioral coverage of the runAgent loop through an openshell stub that streams canned cost results: the loop must not start another iteration once the cap is reached, must not mark a run that was ending anyway, and must keep retrying while under budget. - Replace the source-text ctx-rebinding regression scan with a Go AST check confined to runAgent's body scope that rejects rebinding ctx from any cancellable context constructor, whatever the cancel variable is named. - Update budget boundary, validation, and compose tests for the new semantics; add YAML .nan/.inf/-.inf loading tests. Docs: - Record the cost-cap decision as ADR 0097 and reduce the addition to accepted ADR 0024 to a one-line cross-reference. - Document max_cost_usd classification and presence-aware merge rules in the harness field reference; document duration_seconds and over_budget in the metrics.json field table; update architecture.md and the harness reference for the final semantics. Signed-off-by: guy oron <goron@redhat.com>
Address the second review pass on the budget cap: - The field-level contract (validation, base: inheritance, enforcement boundary, over_budget semantics) moves to docs/normative/harness-budget/v1, following ADR 0015 and the shape of the prescript-output spec; ADR 0097 keeps the decision and links the spec instead of restating it. - Correct the over_budget prose in docs/cli/run.md and the run.go field comment: the suppressed retry can follow a failed validation or a failed repository extraction (whose continue path never reaches validation), and the marker implies nothing about the final validation state — the post-loop sweep may still pass a completed iteration. Signed-off-by: guy oron <goron@redhat.com>
Address the third review pass on the budget cap: - Correct the false premise that no mechanism can interrupt an in-flight iteration: the boundary is between iterations because that is the runtime-agnostic layer (pi offers no in-flight budget control), while Claude Code's native per-invocation --max-budget-usd flag exists, is unused today, and could later complement the cap as a tighter in-flight bound. Reworded in ADR 0097, the normative spec, and the harness reference; wiring the flag is out of scope here. - Warn when a cap is set and a completed iteration reports zero cost: enforcement relies on runtime-self-reported cost, so a crashed stream or an unpriced provider silently under-counts the aggregate. The normative spec and docs/cli/run.md now state the dependency and link the cost data contract; a test covers the warning. - Cross-reference the threat model: ADR 0097 answers its open 'hard stop vs human-in-the-loop' cost-budget question at per-run granularity, so relates_to gains security-threat-model, the ADR context mentions it, and the open question is annotated in place (per-repo/per-org budgets and approval flows remain open). - Pin the extraction-failure over_budget path: the openshell test stub can now fail sandbox download, exercising the top-of-loop guard that suppresses a retry after a failed extraction — previously only the validation-failure halt was tested. Signed-off-by: guy oron <goron@redhat.com>
Signed-off-by: guy oron <goron@redhat.com>
…orted cost The soft-by-one-iteration bound only holds when every iteration reports its cost; crashed, killed, or unpriced iterations contribute $0 and can widen the overshoot. Say so in ADR 0097 and the normative contract instead of overstating the guarantee. Signed-off-by: guy oron <goron@redhat.com>
metrics.json already carries over_budget; the root span didn't, so a budget-halted run was invisible to trace-based alerting. Signed-off-by: guy oron <goron@redhat.com>
The merge-rules table had no max_cost_usd row, so the field fell under the
generic scalar row ("child wins if non-empty") — the opposite of the rule
the field details state a few sections above, where an explicit 0 is an
override to unlimited and only an absent field inherits. A reader who
consults the table alone would misconfigure a child harness. Add the row
and a note that this is the one scalar the generic rule does not cover.
The harness field also shares its name with the eval-case max_cost_usd
threshold in the functional test framework: a post-run judge that grades a
finished run against metrics.json, where this one is a mid-run stop. Say
which is which in the harness reference, in functional-tests.md, and in
the two problem docs whose "already enforced via max_cost_usd" sentence
now reads ambiguously.
While there: name codex in the runtime cost-coverage notes — it reports no
cost at all, so a cap on a codex harness never trips; record in ADR 0097
that the one runtime shipping a native per-invocation budget flag does not
get it passed through, and why forwarding it is its own decision; and
point operational-observability's cost section back at the ADR, as the
threat model already does.
Signed-off-by: guy oron <goron@redhat.com>
The warning fired the moment aggregate cost reached the cap and claimed "no further iterations will start", but at that point the loop does not yet know whether anything will be suppressed: the same iteration may pass validation and break out, or be the last one. In both cases nothing is skipped and metrics.json keeps over_budget false, so the log contradicted the marker. Reword it to the only thing crossing the cap guarantees — remaining retries, if there are any, will be skipped. The suppression sites are where a halt can honestly be announced, and the top-of-loop guard (the one that stops an extraction-failure retry) was breaking silently. Give it the same line the bottom-of-loop guard prints, so every run that the budget really stopped says so. The two suppression tests now assert the announcement, and the crossed-on-the-final-iteration test asserts the absence of any halt claim. Signed-off-by: guy oron <goron@redhat.com>
…ebase Rebasing onto fullsend-ai#7049 brought a second pre-return metrics write, in the path that fails to clear a stale iteration deadline. The deferred writer registered before the loop already persists metrics.json on every fatal exit once iterations have started, so the inline write was a duplicate; drop it so there is one writer, as the comment above the loop says. The budget contract now also states that an iteration killed at timeout_minutes ends the run before any retry is due (ADR 0105), so it is never marked over_budget, whatever its cost. Signed-off-by: guy oron <goron@redhat.com>
315e193 to
5972b8f
Compare
|
@waynesun09 @rh-hemartin: rebased onto main through #7049;
Deferred: two qodo threads wanting PR-description edits. |
3 participants
Heyaa : )
Kept noticing that
max_iterationsbounds how many times an agent retries, but nothing bounds what those retries cost — a runaway run had no ceiling.max_cost_usd: an optional per-harness cap (0= unlimited, inherited by composition exactly liketimeout_minutes), checked against aggregate cost after each iteration.What it can and cannot do:
total_cost_usdonly in the final result event of a completed iteration, so the cap can't interrupt in-flight work — it stops the loop from starting another iteration. Documented where the field is configured, not implied away.continuepaths can't buy an iteration the budget already refused.over_budgetinmetrics.json— it records why the run stopped retrying, not success (with avalidation_loopit commonly accompanies a non-nil error, and the field doc says so). The same flag is now also set as afullsend.over_budgetattribute on the run's root span, so a budget-halted run is visible in traces, not justmetrics.json.metrics.jsonalso gains a top-levelduration_seconds— total wall-clock across all iterations, summed the same way cost is. Independent of the cap; nothing else surfaces run duration today.TestRunAgent_DoesNotCancelTheRunContextpins a constraint that's genuinely easy to reintroduce: cancelling the run's ownctxrebinds the variable the status-comment defer closes over — defers run LIFO, so every run would report "cancelled". The test reads the source because the failure is invisible at the package boundary and.codecov.ymlexcludesrun.gofrom patch coverage (#2831). Negative-checked: reintroducing the exact pattern fails it.Tests: validation (negative/zero/positive), composition inheritance and child override,
exceedsCostBudgetboundaries, duration accumulation, theover_budgetmarker (metrics.jsonand now the root span).go build,go vet,internal/cliandinternal/harnesspass in full.Docs note: the cost-cap decision is recorded as new ADR 0097 with the field-level contract in
docs/normative/harness-budget/v1; accepted ADR 0024 gets only a one-line cross-reference annotation pointing at it — kept rather than reverted, since it's the pointer a future ADR-0024 reader needs to find where the cap actually lives.No tracking issue — this came out of ad-hoc auditing (see PR body above), not a filed bug. Happy to open one if you want a paper trail.