feat: add cost budget enforcement with audit/enforce modes - #212
Conversation
Add budget_usd and budget_mode to workflow limits configuration, implementing LOA Pattern 2.4 (Token Budget Throttle) as a runtime safety mechanism for agentic workflows. Graduation path for users: 1. No config (default) - no budget tracking, existing behavior unchanged 2. budget_usd with audit mode - emits budget_exceeded event, logs warning, workflow continues. Discover cost profiles before enforcing. 3. budget_usd with enforce mode - emits event, saves checkpoint, stops workflow with BudgetExceededError. Resumable via conductor resume. Changes: - config/schema.py: budget_usd (float|None) and budget_mode (audit|enforce) on LimitsConfig - exceptions.py: BudgetExceededError with budget_usd, spent_usd, and current_agent attributes - engine/limits.py: check_budget() on LimitEnforcer with first-time detection flag; from_dict() accepts budget fields for resume parity - engine/workflow.py: _check_budget() helper called at all 5 enforcement points alongside check_timeout(); budget_exceeded event emission; workflow_failed enrichment for BudgetExceededError - cli/run.py: pass budget fields to LimitEnforcer.from_dict() on resume - docs/configuration.md: document budget fields and graduation path - AGENTS.md: add test fixture patterns and resume/checkpoint parity notes - tests/test_engine/test_budget.py: 21 tests covering schema defaults, LimitEnforcer unit tests, all 3 graduation steps, and error attributes
- limits.py: remove unused BudgetExceededError import (F401)
- workflow.py: collapse split f-string per ruff format
- test_budget.py:
- sort import block (I001)
- remove unused UsageTracker import (F401)
- narrow pytest.raises(Exception) to pytest.raises(ValidationError) for
schema-validation tests (B017)
- drop two dead 'original_execute = provider.execute' bindings (F841)
No behavior change. make check now passes on the budget-enforcement diff.
The original budget enforcement commit (6975aaf) added budget docs to docs/configuration.md but missed two surfaces: - docs/workflow-syntax.md 'Limits and Safety' section did not list budget_usd/budget_mode alongside max_iterations/timeout_seconds, even though it is the canonical reference users skim when authoring workflows. Adds the fields to the top-of-file limits snippet, the expanded Limits and Safety section, and a new 'Cost Budget' subsection mirroring the configuration.md graduation path. - CHANGELOG.md [Unreleased] had no entry for the feature. Adds an entry under Added describing the audit/enforce graduation and linking to the new docs sections.
a413e84 to
9bc92f3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #212 +/- ##
=======================================
Coverage ? 86.35%
=======================================
Files ? 67
Lines ? 11657
Branches ? 0
=======================================
Hits ? 10066
Misses ? 1591
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jrob5756
left a comment
There was a problem hiding this comment.
Wow, I'm really sorry. I had my review sitting here as "Pending". Not sure how that happened but regardless, he is my review. The PR is well-scoped, well-tested at the unit level, and follows project conventions (Pydantic v2, exception hierarchy, run/resume parity, AGENTS.md fixture patterns). Verified all 5 check_timeout() sites are correctly paired with _check_budget(); the > boundary semantics, one-shot emission flag, and workflow_failed enrichment are correct; 21/21 tests pass; lint clean.
Strengths worth calling out: split-commit hygiene (feature / lint cleanup / doc follow-up) is reviewer-friendly; the AGENTS.md "Test Fixture Patterns" and "Resume / Checkpoint Parity" sections added here are accurate and useful beyond this PR; zero behavior change when budget_usd is unset is a true zero-overhead default.
In terms of improvements, there are a couple of things that I caught during my review. Please see them as inline comments, and let me know if you have any questions. My apologies again on the delay.
| Subsequent overshoots in ``audit`` mode are silent (no repeated | ||
| events or warnings). | ||
| """ | ||
| summary = self.usage_tracker.get_summary() |
There was a problem hiding this comment.
[critical] Sub-workflow spend bypasses the parent budget.
self.usage_tracker only sees costs incurred by this engine's agents. Sub-workflows execute through a freshly constructed child WorkflowEngine (see _execute_subworkflow at line ~1086) which has its own UsageTracker. The child's usage is read once at line ~1313 to populate the subworkflow_completed event, but is never merged back into the parent's tracker.
Consequence: a workflow with budget_usd: 10, budget_mode: enforce whose work delegates entirely to a type: workflow step can spend $1000 inside the child and _check_budget() here will see spent = 0.0 and never trip. The for-each-of-subworkflows path at line ~3956 already captures child_usage but only uses it for a per-item event — not fed into self.usage_tracker either.
Fix: add UsageTracker.merge(other) and call it after each sub-workflow returns; OR explicitly document the limitation in docs/workflow-syntax.md and emit a startup warning when budget_usd is set and the workflow contains type: workflow steps.
| events or warnings). | ||
| """ | ||
| summary = self.usage_tracker.get_summary() | ||
| spent = summary.total_cost_usd or 0.0 |
There was a problem hiding this comment.
[critical] Silent disable for unpriced models.
WorkflowUsage.total_cost_usd (usage.py:79-85) returns None when no agent has pricing data, and otherwise sums only the agents that had pricing. So or 0.0 here silently collapses two distinct states into "$0 spent":
- Workflow uses a custom MCP provider or new model missing from the pricing table →
spent = 0.0forever, budget never trips. - Mixed pricing (9 priced agents at $5 total + 1 unknown-priced custom model that actually cost $50) →
spent = $5, the $50 is invisible.
The codebase already knows about this state — cli/run.py:878 prints "Cost data unavailable (unknown model pricing)". The budget check ignores that signal.
User impact: configures budget_usd: 50, budget_mode: enforce on a workflow that uses an unpriced model, believes they are protected, the provider charges $500. No warning, no event.
Suggested fix:
summary = self.usage_tracker.get_summary()
if summary.total_cost_usd is None and any(
(a.input_tokens or a.output_tokens) for a in summary.agents
):
# Tokens flowed but no pricing data — emit one-time degraded warning
...
return
spent = summary.total_cost_usd or 0.0| return | ||
|
|
||
| budget = self.limits.budget_usd # guaranteed non-None when exceeded | ||
| assert budget is not None # for type narrowing |
There was a problem hiding this comment.
[suggestion] assert is stripped under python -O.
Conductor is distributed via uv tool install; anyone running the resulting venv with PYTHONOPTIMIZE=1 (or python -O) loses this assertion. The "guaranteed non-None" invariant is also unenforced — a future LimitEnforcer.check_budget change (e.g., a "soft budget" tier that returns exceeded=True when budget_usd is None) would silently break it.
If the invariant breaks under -O, the f-string at line 523 raises TypeError: unsupported format string passed to NoneType.__format__, which then propagates to the generic except Exception at line 2695 and surfaces as a cryptic workflow_failed event — masking the real bug.
| assert budget is not None # for type narrowing | |
| budget = self.limits.budget_usd | |
| if budget is None: | |
| # Defensive: check_budget should never return exceeded=True | |
| # without a configured budget. Raised explicitly so it survives -O. | |
| raise RuntimeError( | |
| "Internal invariant violated: LimitEnforcer.check_budget " | |
| "returned exceeded=True but budget_usd is None." | |
| ) |
Or replace with the NamedTuple-result pattern I suggest at limits.py:258 to eliminate the need for narrowing entirely.
| assert budget is not None # for type narrowing | ||
|
|
||
| if first_time: | ||
| self._emit( |
There was a problem hiding this comment.
[important] This event has no user-visible rendering.
ConsoleEventSubscriber.on_event in cli/run.py:712-813 has no budget_exceeded branch (compare line 738's agent_timeout handler that calls verbose_log_agent_timeout). The dashboard frontend has zero handling either — grep -rn "budget" src/conductor/web/frontend/src/ returns nothing.
In audit mode the only signal is the logger.warning at line 530, which falls through to Python's logging.lastResort (no basicConfig in src/conductor/). That writes to stderr unstyled and is invisible in --web-bg mode (lands in .bg.stderr.log under $TMPDIR/conductor/).
So the docstring at schema.py:323 ("emit a budget_exceeded event and log a warning") is technically true, but neither reaches users via the interfaces they're actually watching. Suggest:
- Add a
budget_exceededbranch toConsoleEventSubscriber.on_eventcalling a newverbose_log_budget_exceeded(spent, budget, mode, agent). - Add a
budget_exceededhandler to the dashboard frontend store/UI (banner for audit, fatal status for enforce). - Drop the bare
logger.warningonce the event reaches both subscribers.
| if first_time: | ||
| logger.warning( | ||
| "Budget exceeded (audit mode): spent $%.4f of $%.2f budget", | ||
| spent, | ||
| budget, | ||
| ) |
There was a problem hiding this comment.
[important] Two issues with this audit-mode warning.
(1) Fire-once-then-silent UX. Because _budget_exceeded_emitted latches at limits.py:283, this warning fires exactly once per run. A long-running audit-mode workflow that trips at $11 vs. a $10 budget then runs for another hour and spends $200 more — with zero further signal to the user. See my comment at limits.py:73 for the suggested re-emission strategy.
(2) Missing current_agent. The event payload at line 517 and BudgetExceededError at line 526 both include current_agent. This warning does not, so users with only stderr to go on can't tell which agent triggered the overshoot:
| if first_time: | |
| logger.warning( | |
| "Budget exceeded (audit mode): spent $%.4f of $%.2f budget", | |
| spent, | |
| budget, | |
| ) | |
| if first_time: | |
| logger.warning( | |
| "Budget exceeded (audit mode) after agent %r: spent $%.4f of $%.2f budget", | |
| self.limits.current_agent or "<unknown>", | |
| spent, | |
| budget, | |
| ) |
| a hard time limit. | ||
| """ | ||
|
|
||
| budget_usd: float | None = Field(default=None, ge=0.0) |
There was a problem hiding this comment.
[suggestion] ge=0.0 admits budget_usd=0.0, which has odd semantics.
check_budget uses strict spent > self.budget_usd (limits.py:280). With budget_usd=0.0, the check only trips when a non-zero charge lands — i.e. after one wasted execution. That's a confusing way to spell "fail fast on any spend."
test_budget_usd_zero_allowed (test_budget.py:55) makes this look intentional, but it's almost certainly a user error case ("I set budget=0 to disable tracking" — they meant None).
| budget_usd: float | None = Field(default=None, ge=0.0) | |
| budget_usd: float | None = Field(default=None, gt=0.0) |
And drop test_budget_usd_zero_allowed. If you have a real reason to admit zero, document the "fires after first spend" semantics in the docstring below.
| - `budget_mode: enforce` emits a `budget_exceeded` event, saves a checkpoint, | ||
| and stops the workflow with `BudgetExceededError`. Resume after increasing | ||
| the budget with `conductor resume <workflow.yaml>`. |
There was a problem hiding this comment.
[important] "Resume after increasing the budget" contradicts the code.
The engine resets UsageTracker on resume (workflow.py:366 rebuilds it fresh), so spent starts at $0 and the resumed run gets a fresh budget window for the remaining work. Users can conductor resume without raising the budget. This wording — echoed in CHANGELOG.md:14-16, docs/configuration.md:288-289, and the auto-generated BudgetExceededError.suggestion — implies the opposite.
Either reconcile the docs/suggestion text:
| - `budget_mode: enforce` emits a `budget_exceeded` event, saves a checkpoint, | |
| and stops the workflow with `BudgetExceededError`. Resume after increasing | |
| the budget with `conductor resume <workflow.yaml>`. | |
| - `budget_mode: enforce` emits a `budget_exceeded` event, saves a checkpoint, | |
| and stops the workflow with `BudgetExceededError`. Resume with | |
| `conductor resume <workflow.yaml>` to continue with a fresh budget window | |
| for the remaining work (raise `budget_usd` first if the remainder needs | |
| more headroom). |
… OR restore prior cumulative spend on resume so the existing guidance becomes true. Pick a side and apply it everywhere.
| See [configuration.md](configuration.md) for an end-to-end example and | ||
| notes on how budget tracking integrates with the provider usage callbacks. |
There was a problem hiding this comment.
[suggestion] Dangling reference.
docs/configuration.md has the budget_usd / budget_mode subsection (which is good), but it doesn't include "an end-to-end example" or "notes on how budget tracking integrates with the provider usage callbacks" — that content doesn't exist there. The link points readers at something they won't find.
Either drop this line, or actually add the example + provider-usage-callback notes to configuration.md and keep the cross-link.
| ) | ||
|
|
||
|
|
||
| class TestBudgetGraduationStep0: |
There was a problem hiding this comment.
[critical] Coverage gap: only 1 of 5 enforcement sites is exercised.
The PR adds self._check_budget() at 5 distinct sites in workflow.py:
| Line | Site | Tested here? |
|---|---|---|
| 2097 | after for-each group | no |
| 2169 | after parallel group | no |
| 2414 | after script step | no |
| 2506 | after sub-workflow step | no |
| 2639 | after main agent loop | yes |
All four integration tests in this file use the single-agent _make_config(...) helper. Coverage report confirms workflow.py lines 2058-2124, 2129-2196, 2302-2446, 2450-2538 are entirely uncovered by test_budget.py. Removing any of those four _check_budget() calls would not fail any test.
Also missing:
LimitEnforcer.from_dict(budget_usd=..., budget_mode=...)coverage. No test reconstructs an enforcer with the new kwargs; extendTestLimitEnforcerFromDictintest_context_serialization.py(mirror the existingtest_timeout_from_parameterpattern).- CLI resume integration test.
cli/run.py:1748readsconfig.workflow.limits.budget_usd/budget_modeand forwards them; nothing tests this end-to-end. If those two kwargs get dropped from thefrom_dictcall, a resumed enforce-mode workflow silently loses its cap. current_agentassertions.test_enforce_mode_raises_budget_exceededcheckserror_type,budget_usd,spent_usdin theworkflow_failedevent — but the engine also populatescurrent_agenton the exception, thebudget_exceededevent, andworkflow_failed. None of those are asserted by an integration test.- Sub-workflow cost-isolation pin. Worth a test that pins the contract (in either direction) for whether parent
budget_usdcovers child spend. See my critical comment onworkflow.py:500— today the answer is "no" but it's not pinned by any test.
Suggested additions (one per enforcement site, plus the resume tests):
test_enforce_mode_in_parallel_group_raisestest_enforce_mode_in_for_each_group_raisestest_enforce_mode_in_subworkflow_step_raisestest_enforce_mode_in_script_step_raises(or a comment if scripts don't accrue model cost)test_budget_kwargs_round_trip_from_dict(intest_context_serialization.py)test_resume_preserves_budget_enforcement(CLI level)
| def mock_handler(agent, prompt, context): | ||
| return expensive.content | ||
|
|
There was a problem hiding this comment.
[suggestion] mock_handler body is dead.
provider.execute = patched_execute at line 196 replaces the execution path before engine.run() is awaited, so mock_handler is never called. Verified by reading CopilotProvider.__init__ and the SDK call sites — the argument itself is load-bearing (gates _validate_reasoning_effort_for_model, _max_prompt_tokens_for_model, validate_connection) but the body is unreachable.
Same pattern is duplicated at lines 215-217, 273-274, 308-310, 347-349. Collapse each to a sentinel:
| def mock_handler(agent, prompt, context): | |
| return expensive.content | |
| provider = CopilotProvider(mock_handler=lambda *_a, **_kw: {}) |
(remove the surrounding def mock_handler block and the now-orphaned mock_handler=mock_handler kwarg on the next CopilotProvider(...) line).
Criticals: - C1: merge sub-workflow usage into parent UsageTracker so delegated spend counts against a parent budget (UsageTracker.merge) - C2: one-time degraded warning when a budget is set but models are unpriced (no silent disable) - C3: add per-call-site / re-emission / from_dict round-trip / merge tests Important: - I1: render budget_exceeded in the console subscriber and the web dashboard activity log - I2: include current_agent in the audit warning - I3: shared BudgetMode Literal alias for type safety - I4: threshold re-emission (per budget increment) instead of a one-shot latch - I5: make from_dict transient kwargs required keyword-only; update all callers - I6/I7: reconcile resume budget semantics (fresh window) across exception, CHANGELOG and docs Suggestions: - S1/S3: BudgetCheckResult NamedTuple removes the assert at the call site - S2: BudgetExceededError Args docstring + forward current_agent to agent_name - S4: budget_usd must be strictly positive (gt=0) - S5: drop dangling configuration.md reference - S6: collapse dead mock_handler bodies in tests
|
Thanks for the thorough review, @jrob5756 — I've pushed a commit (18d55e4) addressing all of it. Criticals
Important
Suggestions
Full local gate is green (ruff, ruff format, ty on src, and pytest — only the known pre-existing Windows-only environmental failures remain). |
…ement # Conflicts: # docs/workflow-syntax.md # src/conductor/cli/run.py # src/conductor/config/schema.py # src/conductor/engine/workflow.py # src/conductor/web/static/assets/index-DGEak2SH.js # src/conductor/web/static/index.html
Add TestBudgetSubworkflowRollup covering the parent-level usage merge (self.usage_tracker.merge(child_engine.usage_tracker.get_summary())) that had to be re-applied on top of main's _run_child_engine refactor. Two tests: child spend appears in the parent usage summary, and an enforce-mode parent budget trips on cost incurred only by the delegated child. Mutation-verified: both fail if the merge call is removed.
Bump version 0.1.19 -> 0.1.20 and finalize the changelog. Changelog (0.1.20): - Added: Hermes provider (#235), cost budget enforcement (#212), external-workflow-friction knobs — output_mode / max_parse_recovery_attempts / gate-respond CLI / Windows paths (#234), templated reasoning.effort & context_tier (#263), scoped applyTo instruction loading (#238). - Fixed: Copilot model attribution for auto-routed runs (#268), claude-agent-sdk default tool preset when tools: omitted (#269). Re-locked uv.lock to record 0.1.20. Quality gates green locally (ruff, ty, pytest excl. real_api/performance: 3673 passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…266) (#276) Several current models had no DEFAULT_PRICING entry, so get_pricing returned None and they were silently costed at $0 in the Token Usage Summary / cost breakdown. Dotted version suffixes (e.g. claude-opus-4.8) are not bridged by the `-`-delimited fuzzy fallback, so exact keys are required. Add entries (grounded in the table's existing per-family rates): - claude-opus-4.7, claude-opus-4.8 (5/25, cache 0.50/6.25) - claude-sonnet-5 (3/15, cache 0.30/3.75) - gpt-5.5, gpt-5.4, gpt-5.3-codex (2/8) - gpt-5-mini, gpt-5.4-mini (0.15/0.60) - gemini-3.5-flash (0.30/2.50) Also add a regression test asserting each new model resolves to non-None pricing, and repoint the #137 cross-family test off claude-opus-4.7 (now a real key) to a synthetic claude-opus-4.123 so it still exercises the delimiter guard. The related sub-workflow usage under-reporting reported in the same issue was already fixed in #212. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
feat: add cost budget enforcement with audit/enforce modes
Branch:
feature/budget-enforcement→mainType: Feature + hygiene cleanup + docs follow-up
Risk: Low — additive only; default behavior unchanged (no budget tracking unless
budget_usdis set).Summary
Adds
budget_usdandbudget_modeto workflowlimits, implementing LOA Pattern 2.4 (Token Budget Throttle) as a runtime safety mechanism for agentic workflows that can otherwise burn unbounded cost in loops or recursive sub-workflows.The graduation path is the headline UX:
budget_usd+auditmode — emitsbudget_exceededevent, logs a warning, workflow continues. Use this to discover cost profiles without breaking real workflows.budget_usd+enforcemode — emits event, saves checkpoint, stops the workflow withBudgetExceededError. Resumable viaconductor resumeafter raising the budget.Commits
6975aaf feat: add cost budget enforcement with audit/enforce modesd3fb868 chore: fix pre-existing ruff errors in budget code9bc92f3 docs(budget): document limits.budget_* in workflow-syntax + CHANGELOGWhat changed (feature commit)
src/conductor/config/schema.py—LimitsConfiggainsbudget_usd: float | None(must be ≥ 0) andbudget_mode: Literal["audit", "enforce"](default"audit"). Pydantic v2 raisesValidationErrorfor negative numbers or invalid mode strings.src/conductor/exceptions.py— newBudgetExceededErrorcarryingbudget_usd,spent_usd, andcurrent_agentfor diagnostics and workflow_failed enrichment.src/conductor/engine/limits.py—LimitEnforcer.check_budget()with a first-time-overshoot flag so audit mode emits exactly one event per run.from_dict()now accepts the new fields for resume parity.src/conductor/engine/workflow.py—_check_budget()helper called at all five existing limit-check points alongsidecheck_timeout(). Emitsbudget_exceededevent. In enforce mode raises the new exception, which the workflow_failed handler enriches with budget context.src/conductor/cli/run.py— passes budget fields throughLimitEnforcer.from_dict()on resume so a resumed workflow re-applies the cap from the original config.docs/configuration.md— full feature reference with graduation guidance.AGENTS.md— test fixture patterns + resume/checkpoint parity rules (the latter is generally applicable, not budget-specific, but was learned while wiring the resume path).tests/test_engine/test_budget.py— 21 tests covering schema defaults, validation rejection paths,LimitEnforcerunit behavior (zero, audit first-time flag, enforce raising), and all three graduation modes against aCopilotProviderwith a mocked execute.Hygiene cleanup (separate commit)
Lint errors that pre-existed on the branch base or crept in during feature work. Split into its own commit (
d3fb868) to keep the feature diff reviewable:engine/limits.py: remove unusedBudgetExceededErrorimport (F401).engine/workflow.py: collapse split f-string perruff format.tests/test_engine/test_budget.py:UsageTrackerimport (F401),pytest.raises(Exception)topytest.raises(ValidationError)for schema validation tests (B017 — pydantic v2 is the precise exception these tests intend to catch),original_execute = provider.executebindings (F841).No behavior change.
make checkis clean on the full branch diff.Documentation follow-up (separate commit)
The feature commit updated
docs/configuration.mdbut two surfaces were missed:docs/workflow-syntax.md"Limits and Safety" — the canonical syntax reference users skim when authoring workflows. It listedmax_iterationsandtimeout_secondsbut notbudget_usd/budget_mode. Added to both the top-of-file snippet and the expanded section, plus a new "Cost Budget" subsection mirroring the graduation path.CHANGELOG.md[Unreleased] — had no entry for the feature.Captured in commit
9bc92f3rather than amending6975aafso the timeline shows the gap was caught and closed, rather than rewriting published history.Verification
make check(ruff + ruff format + ty) — passes.uv run pytest tests/test_engine/test_budget.py -q— 21 / 21 pass.uv run pytest) — passes (modulo the 11 pre-existing failures onmainunrelated to this branch: registry TOML parse + event-log tests).Backwards compatibility
Zero behavior change when
budget_usdis unset. Existing workflows, tests, and CI pipelines need no modification.Reviewer guidance
engine/workflow.pyis the load-bearing piece. Confirm_check_budget()is called at everycheck_timeout()site and that no new call sites were added without budget coverage.LimitEnforceris the source of the event-emission contract — if you change it, updatetest_audit_mode_emits_event_and_continuesaccordingly.LimitEnforcer.from_dict()accepts budget fields as transient config (sourced from the current workflow YAML at resume time, not the checkpoint). This is intentional per the AGENTS.md "Transient vs persistent" rule — users may want to raise the cap before resuming.