Skip to content

fix(agent): budget exhaustion is a resumable blocker, not a bare failure (#1117) - #1128

Merged
frankbria merged 4 commits into
mainfrom
fix/1117-iteration-budget-outcome
Aug 10, 2026
Merged

fix(agent): budget exhaustion is a resumable blocker, not a bare failure (#1117)#1128
frankbria merged 4 commits into
mainfrom
fix/1117-iteration-budget-outcome

Conversation

@frankbria

Copy link
Copy Markdown
Owner

Closes #1117.

What was wrong

A run that produced a substantially complete implementation — 15 create_file,
8 edit_file, 12 run_command, 6 run_tests — hit its iteration cap while
adding a Dockerfile and example clients, and told the user, in full:

Task execution failed

The task flipped to FAILED, no blocker was created, and nothing indicated that
most of the deliverable was sitting in the working tree. The cap appeared only in
.codeframe/runs/<id>/output.log as Iteration 45/45.

The change

Running out of budget is not an error. It is the same shape as the cost cap,
which already blocks rather than fails — and whose existing comment says exactly
why: "the work is not wrong, it needs a human decision, which is what a blocker
is for."
Exhaustion now takes that path.

That one change satisfies four of the acceptance criteria at once: it is a
distinct named outcome, it creates a blocker, it is resumable rather than
terminal, and BLOCKED reads differently from FAILED in cf tasks list.

The blocker the user actually gets:

Run stopped: iteration_budget_exhausted

Used the full budget of 45 iteration(s) without declaring the task complete.

This is not an error. Partial work from this run is already in your working
tree — review it with `git status` before re-running.

To continue, raise the budget and re-run:
  CODEFRAME_MAX_ITERATIONS=90 cf work start 019cf894 --execute
(or set agent.max_iterations in .codeframe/config.yaml), or split the task up.

What the run did: `cf work diagnose 019cf894`

CODEFRAME_MAX_ITERATIONS had to be built, not just referenced

The AC asks the message to name "the flag/env var that raises it." Neither
--max-iterations nor CODEFRAME_MAX_ITERATIONS existed
— the obvious message
would have pointed the user at nothing. Only .codeframe/config.yaml's
agent.max_iterations worked, which means editing a file inside the repo.

So the env var is now real: an exact budget that beats both the config file and
the complexity multiplier (matching the LLM-provider precedence chain), with a
bad value warning rather than aborting a Golden Path command. Five tests cover
it, including that it wins over the multiplier — raising only a ceiling the
multiplier then scales away from would have bought nothing.

Also fixed

_create_text_blocker prefixed every blocker with "Agent detected a blocker:".
For a runtime-initiated stop that is backwards — "Agent detected a blocker:
iteration_budget_exhausted" reads as though the agent found a problem in the
work. Runtime stops now read "Run stopped: ..." and carry BlockerOrigin "system" rather than "agent".

Acceptance criteria

  • Distinct, named outcome — BLOCKED + iteration_budget_exhausted
  • States the cap, how to raise it, and that partial work is present
  • Distinguishes "incomplete" from "failed", including in cf tasks list
  • A blocker is created, so the run is resumable
  • Points at cf work diagnose <id>
  • A test asserts the message is distinct from the generic failure

15 new tests; tests/core/ + tests/cli/ 4343 passed.

Judgment calls

  • The outcome changes, not just the text. If you'd rather it stay FAILED
    with better wording, that's a small revert — but BLOCKED is what makes it
    resumable, and it reuses the cost-cap path rather than inventing a state.
  • No --max-iterations CLI flag. The env var covers the message's need
    without threading a new parameter through runtime.execute_agent → engine →
    agent. Happy to add the flag on top if you want it discoverable in --help.
  • Message kept under 500 characters because _create_text_blocker truncates
    its context at that length — the cf work diagnose pointer was being cut off
    in my first draft.

Known limitations

  • If the blocker write fails (e.g. no database), the run still degrades to
    FAILED with no message. That is pre-existing behaviour for every blocker
    reason, not new here; test_error_event_and_stream_close_on_max_iterations now
    documents that path explicitly.
  • The count is iterations, not "how close to done" — nothing here judges whether
    the partial work is nearly complete or barely started. The message says to
    review the tree rather than claiming the work is nearly done.

…ure (#1117)

A run that produced a substantially complete implementation — 15 create_file,
8 edit_file, 12 run_command, 6 run_tests — hit its iteration cap while adding
extras and told the user, in full:

    Task execution failed

The task flipped to FAILED, no blocker was created, and nothing indicated that
most of the deliverable was sitting in the working tree. The cap appeared only
in .codeframe/runs/<id>/output.log as "Iteration 45/45".

Running out of budget is not an error. It is the same shape as the cost cap,
whose existing comment already says it: "the work is not wrong, it needs a human
decision, which is what a blocker is for." So exhaustion now takes that path.

- New _REASON_ITERATION_BUDGET_EXHAUSTED, in _BLOCKED_REASONS. The loop returns
  BLOCKED with a blocker instead of FAILED, so the run is resumable and reads
  differently from a genuine error in `cf tasks list`.
- The blocker names the cap that was hit, says partial work is already in the
  tree (review it before re-running), gives the command to raise the budget, and
  points at `cf work diagnose`. Kept under _create_text_blocker's 500-char cap so
  that pointer is not truncated away.
- CODEFRAME_MAX_ITERATIONS now exists. Neither it nor a --max-iterations flag
  did, so the obvious message would have pointed the user at nothing. It is an
  exact budget that beats the config file and the complexity multiplier, matching
  the LLM-provider precedence chain; a bad value warns rather than aborting.
- _create_text_blocker no longer says "Agent detected a blocker" for stops the
  runtime made. "Agent detected a blocker: iteration_budget_exhausted" reads as
  though the agent found a problem in the work, which is the opposite of what
  happened. Those also get BlockerOrigin "system" rather than "agent".

test_error_event_and_stream_close_on_max_iterations asserted the old FAILED
outcome. Its fixture workspace has no database, so the blocker write fails and
the run degrades to FAILED — it now documents that degradation explicitly, which
is worth pinning, and the BLOCKED contract is covered against a real workspace
in the new test module.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @frankbria's task in 4m 43s —— View job


Bug-hunting review — ✅ GLM review: no defects found

Reviewed the three hunks in react_agent.py against their callers/callees for logic errors, error-path crashes, origin-mapping consistency, and the new env-var precedence. Nothing concrete to flag.

Verified:

  • _iteration_budget_override() (react_agent.py:82) — os (L13) and logger (L50) are both in scope before the function runs. Bad values ("0", "-3", non-integer) warn and return None rather than crashing, matching the docstring's "a typo must not abort the run."
  • Exhaustion path (react_agent.py:808-825) — _create_text_blocker sits outside any try/except inside _react_loop, so a failed blocker write (e.g. no DB) propagates up to run()'s except Exception (L384) → FAILED. This is exactly the documented limitation and what test_error_event_and_stream_close_on_max_iterations now pins.
  • Origin mapping (react_agent.py:1450) — _SYSTEM_DETECTED_REASONS = {stall_detected, iteration_budget_exhausted} preserves prior behavior: stall still → "system", cost cap still → "agent" (no regression), budget exhaustion → "system". Both "system" and "agent" resolve to valid BlockerOrigin members, so blockers.create(created_by=origin) won't raise.
  • Precedence — _calculate_adaptive_budget (L1225) returns the override before the multiplier/clamp runs, and run() always assigns it to self.max_iterations before _react_loop. The only caller of _react_loop is L318, so the env var genuinely wins over config.yaml in every production path. The override intentionally bypasses [min, max] clamping (documented inline).
  • No second reader/writer of CODEFRAME_MAX_ITERATIONS elsewhere in codeframe/ to conflict with the new precedence.

Out of scope (left to the general review, not defects):

  • The cost-cap blocker keeps origin "agent" while conceptually runtime-detected — pre-existing, not changed here.
  • The AGENT_FAILED event + stream error emitted on the BLOCKED path (L333-339) is shared with the existing cost-cap path, not introduced here.
  • The 8-char task-id prefix in the help text relies on the CLI accepting id prefixes.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review

Solid, well-scoped fix. Reusing the cost-cap "blocked, not failed" shape for iteration exhaustion is the right call, and the new CODEFRAME_MAX_ITERATIONS env var is genuinely necessary rather than decorative — without it the blocker message would point users at nothing. I verified a few of the PR's specific claims rather than taking them on faith:

  • The blocker text for a typical case (45 iterations, 8-char task id) is 439 chars, comfortably under _create_text_blocker's 500-char truncation — the cf work diagnose pointer really does survive.
  • cf work start/cf work diagnose accept unique prefixes via tasks.find_by_prefix (codeframe/cli/app.py), so the shortened 8-char task id in the message is actually usable, not just illustrative.
  • .codeframe/config.yaml's agent.max_iterations really does act as an exact override today (config.py sets both base_iterations and max_iterations to the same value, so the complexity multiplier's result clamps right back down to it) — the message's "or set agent.max_iterations" alternative is accurate, not misleading.
  • BlockerOrigin("system") is a pre-existing valid value (already used for stall detection), so no schema/enum risk there.
  • New tests use real LLMResponse/ToolCall objects instead of MagicMock for the provider, specifically to survive the loop's JSON serialization — good instinct, and the comment explaining why is appreciated.

Minor / nit findings

  1. Stale docstrings. _react_loop's docstring (codeframe/core/react_agent.py, "Returns AgentStatus.FAILED when max_iterations is reached") and run()'s docstring ("AgentStatus.FAILED — max iterations or verification exhausted") both still describe the pre-[P1.40] Iteration-budget exhaustion is reported as a bare 'Task execution failed', hiding a nearly-complete implementation #1117 behavior. Worth a one-line update since this is exactly the kind of docstring a future reader trusts at face value.

  2. Now-dead fallback branch. In run(), the else: reason = "max_iterations_reached" branch (around line 325) was reachable before this change whenever _react_loop exhausted its budget naturally. After this PR, that natural-exhaustion path always returns BLOCKED (via the new _create_text_blocker call), so _react_loop can only still return FAILED via the stall FAIL action (which sets _stall_triggered) or loop detection (which sets _early_termination_reason) — both already handled by the earlier branches. "max_iterations_reached" no longer appears anywhere in the test suite either. Not a bug, but it's vestigial and could mislead someone into thinking that reason string is still live.

  3. Inconsistent wording left on the cost-cap sibling. _SYSTEM_DETECTED_REASONS now covers stall detection and iteration-budget exhaustion, but not cost_cap_exceeded — so hitting the spend cap still produces "Agent detected a blocker: cost_cap_exceeded" with origin "agent", even though the same reasoning this PR applies ("the runtime detected this, not the agent") arguably applies there too. Given the PR explicitly frames iteration exhaustion as reusing the cost-cap's existing pattern, this leaves the two siblings inconsistent. Might be intentionally out of scope for [P1.40] Iteration-budget exhaustion is reported as a bare 'Task execution failed', hiding a nearly-complete implementation #1117, but flagging in case it wasn't a deliberate choice — a natural follow-up if not.

None of these block the change; all are polish. The core behavior change, precedence logic, and test coverage look correct and match the PR description's claims.

…idated (#1117)

Both from PR review.

_react_loop and run() still documented "FAILED when max_iterations is reached",
which is exactly what this branch changes — the kind of docstring a later reader
trusts at face value.

The `else: reason = "max_iterations_reached"` default in run() is now
unreachable: natural exhaustion returns BLOCKED, and the two remaining FAILED
paths (stall with --stall-action fail, loop detection) both set a flag handled
by the branches above it. Kept as a defensive default rather than deleted, but
relabelled "unknown" so it cannot report a budget stop that did not happen.
@frankbria

Copy link
Copy Markdown
Owner Author

Both findings fixed.

1. Stale docstrings — correct, and exactly the kind a later reader trusts. _react_loop and run() now describe the new outcomes: BLOCKED on budget exhaustion, FAILED only for a stall with --stall-action fail or loop detection.

2. Dead max_iterations_reached branch — verified your reasoning: natural exhaustion returns BLOCKED now, and the two remaining FAILED paths both set _early_termination_reason or _stall_triggered, so the else cannot be reached. I kept it as a defensive default rather than deleting it — if a future path returns FAILED without setting either flag, an else is better than an unbound reason — but relabelled it "unknown" so it can no longer report a budget stop that did not happen.

Thanks also for independently checking the 439-char blocker length and that find_by_prefix makes the 8-char task id in the message actually usable — both were things I'd asserted and you verified rather than took on trust.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @frankbria's task in 3m 42s —— View job


Bug-hunting review — ✅ GLM review: no defects found

Re-reviewed the three react_agent.py hunks after the f66f72b docstring/dead-reason follow-up. Confirmed the two earlier findings are fixed and found no new concrete defects (logic, error-path, security, data-loss, or contract).

Verified independently (against the file, not the diff):

  • _iteration_budget_override() (react_agent.py:82-100) — int() parse with ValueErrorNone, <=0None, empty/unset (not raw)→None. No crash path; os/logger in scope.
  • Loop boundary (react_agent.py:542,600) — while iterations < self.max_iterations with iterations += 1 after each LLM call yields exactly max_iterations calls before falling through to the exhaustion path at :813, so the blocker's reported count is correct.
  • Origin mapping (react_agent.py:122-125,1460-1468) — both "system" and "agent" are valid for blockers.create(created_by=...) (Union[BlockerOrigin, str]; enum has SYSTEM/AGENT/HUMAN). Cost cap staying on "agent" is the already-flagged out-of-scope nit, not a regression.
  • Precedence (react_agent.py:1235-1247) — the override returns before the complexity multiplier/clamp, and run() always assigns it to self.max_iterations before _react_loop. CODEFRAME_MAX_ITERATIONS is read only here (the other max_iterations hits in streaming.py / agents_config.py / config.py are unrelated knobs), so no precedence conflict.
  • The agent.max_iterations config alternative the message names is real: config.py:561-566 sets both base_iterations and max_iterations to it, so it produces an exact budget — the message is accurate, not misleading.
  • Blocker text is ~439 chars at a realistic N=45, under _create_text_blocker's text[:500] cap, so the cf work diagnose pointer survives.

Earlier findings, now resolved in f66f72b:

  • _react_loop / run() docstrings now describe BLOCKED-on-exhaustion.
  • ✅ The unreachable else in run() is relabelled "unknown" (defensive default kept), and test_error_event_and_stream_close_on_max_iterations was relaxed to a truthy error_events[0].error to match the blocker-write-fails → FAILED degradation path it now documents.

Benign edge cases considered and rejected (not defects):

  • If the final LLM call both exhausts iterations and crosses the cost cap, the loop exits before the top-of-loop cost check, so the reason reads iteration_budget_exhausted rather than cost_cap_exceeded. Both are BLOCKED + resumable, so the outcome is correct; only the label is imprecise, and the structure predates this PR.
  • CODEFRAME_MAX_ITERATIONS=1 bypasses min_iterations (15) — explicit user override, documented inline.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review

This has already been through a solid two-round review (the automated GLM bug-hunt and a prior claude pass), and both findings from that pass — the stale FAILED-only docstrings and the now-dead max_iterations_reached branch — are correctly fixed in the second commit (f66f72b). I re-verified those fixes directly against the current diff rather than taking the PR's word for it:

  • run() and _react_loop() docstrings now correctly describe BLOCKED-on-exhaustion.
  • The else branch in run() is relabelled "unknown" and confirmed genuinely unreachable today (the only two remaining FAILED paths from _react_loop — stall with --stall-action fail, and loop detection — both set _stall_triggered/_early_termination_reason before returning, so the earlier elifs catch them first).

I did my own independent pass focused on tracing the change end-to-end rather than re-litigating what's already been reviewed:

  • CODEFRAME_MAX_ITERATIONS precedence is real. _calculate_adaptive_budget returns the override before the multiplier/clamp runs, and it's the only place run() sets self.max_iterations before _react_loop. Grepped for other readers of the env var — none — so there's no conflicting precedence source.
  • No stale references to the removed reason string. max_iterations_reached doesn't appear anywhere else in the codebase (CLI, UI, tests), so relabelling it didn't silently break a consumer that pattern-matched on it.
  • BlockerOrigin("system")/("agent") are both pre-existing valid enum members (codeframe/core/blockers.py), so the new origin-mapping logic can't raise.
  • Traced the actual user-facing surface, not just the unit tests. codeframe/cli/app.py prints state.blocker.question on AgentStatus.BLOCKED for cf work start --execute, and runtime.py's execute_agent maps the adapter's "blocked" status through correctly — so this genuinely replaces the bare Task execution failed a real CLI user sees, not just what a test asserts against _react_loop() directly. One nuance worth noting (not a defect in this PR, and not new — it applies identically to the already-shipped stall/cost-cap blockers): BuiltinReactAdapter._map_status never populates AgentResult.blocker_question, so state.blocker stays None for react-engine blocks and the inline CLI print falls back to the generic "Task blocked - human input needed" + "Use codeframe blocker list" line rather than showing the crafted message text immediately. The full message is still there — the user just needs the one extra cf blocker list/cf work diagnose step to see it, exactly as the existing print statement tells them to. Since this is pre-existing plumbing shared by every ReactAgent-originated blocker, it's out of scope here, but flagging since it's directly adjacent to this PR's value proposition.
  • Blocker dedup ([P1.8] Deduplicate blocker creation (adapter + runtime each create one → double blockers/webhooks) #735) isn't at risk. blockers.create dedupes on (task_id, question, OPEN) to guard against the adapter and runtime.execute_agent both creating a blocker for the same event — confirmed this doesn't produce a different duplicate here, since the react adapter path never reaches the runtime-side create call at all (per the point above).
  • Confirmed _iteration_budget_override()'s bad-value handling (non-integer, zero, negative) all warn-and-fall-through rather than raising, matching the "a typo must not abort the run" intent, and that os/logger are both in scope at module load.

Nothing new to flag. The behavior change, precedence logic, and test coverage hold up under an independent trace of the full path from _react_loop through runtime.py to the CLI's printed output.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @frankbria's task in 4m 36s —— View job


Bug-hunting review — ✅ GLM review: no defects found

Third pass on PR #1128, re-triggered by the origin/main merge in d45424f. The merge brought in tasks.py (#1115) and 16 other files but zero changes into react_agent.py (empty three-way diff on the merge commit) and left no conflict markers — so the two earlier GLM passes and the f66f72b docstring/dead-reason fixes still stand. Verification re-run against the current file, not the diff.

Independently verified:

  • _iteration_budget_override() (react_agent.py:82-100) — int() parse, ValueError/<=0/unset → None; os/logger in scope. No crash path.
  • Loop boundary (react_agent.py:542,600) — post-increment while iterations < self.max_iterations yields exactly max_iterations LLM calls before the exhaustion fall-through (:813), where iterations == self.max_iterations, so the reported cap is accurate.
  • Blocker text ~439 chars at N=45 (under 500 for any realistic budget) — cf work diagnose pointer survives _create_text_blocker's text[:500] cap.
  • Origin mapping (react_agent.py:1460-1468) — both "system" and "agent" are valid BlockerOrigin members; cost cap staying on "agent" is the already-flagged out-of-scope nit, not a regression.
  • Precedence (react_agent.py:1235-1237) — override returns before multiplier/clamp; only production caller of the budget; CODEFRAME_MAX_ITERATIONS read nowhere else.
  • blockers.create (blockers.py:97-99) dedups and returns (never raises) on a duplicate, so a re-run exhausting at the same budget re-uses the OPEN blocker and still returns BLOCKED — benign.
  • run() BLOCKED branch (react_agent.py:340-346) emits AGENT_FAILED+stream error identical to the pre-existing cost-cap/stall path — no new stream behavior.
  • f66f72b fixes confirmed in place: docstrings describe BLOCKED-on-exhaustion; unreachable else relabelled "unknown"; relaxed test assertion matches the documented no-DB → FAILED degradation.

No concrete defect with a reproducible failure scenario. No inline comments posted.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review

No new commits since my last pass (f66f72bf) — the current HEAD only adds a main merge commit, no code changes. Both minor findings from my two earlier rounds (stale FAILED-only docstrings, the now-dead max_iterations_reached branch) are still fixed, and nothing in the diff has regressed since. I re-traced the core logic once more rather than re-litigating what's already covered:

  • _calculate_adaptive_budget still applies CODEFRAME_MAX_ITERATIONS before the complexity multiplier/clamp, with no other reader of the env var to conflict with it.
  • _SYSTEM_DETECTED_REASONS / _BLOCKED_REASONS / origin mapping in _create_text_blocker are consistent and don't collide with the other call sites (escalated_to_blocker, requirements/access tool-error blockers, cost cap) — those keep their existing "Agent detected a blocker: ..." wording and agent origin as expected.
  • If blockers.create itself fails (no DB, etc.), the exception still propagates out through run()'s outer except Exception and degrades to FAILED with reason "exception" — matches the PR's stated "Known limitations" and is pre-existing behavior for every blocker reason, not new here.

One small, non-blocking nit I hadn't raised before: tests/core/test_react_agent.py's test_error_event_and_stream_close_on_max_iterations was loosened from asserting an exact error_events[0].error == "max_iterations_reached" to just assert error_events[0].error (truthy). That's reasonable given the new code path no longer produces a stable string in that no-DB fixture (it now falls through to "exception"), and the docstring update explains why — but since the value actually is deterministic ("exception"), asserting that exact string would keep the test as precise as it was before rather than trading precision for truthiness. Not worth blocking on.

Everything else — the BLOCKED-not-FAILED outcome, the new env var and its precedence, the blocker message content/length, and test coverage — holds up as already confirmed in my prior two reviews. No new issues found.

@github-actions

Copy link
Copy Markdown
Contributor

Bug-hunting review — ✅ GLM review: no defects found

Third pass, re-triggered by the origin/main merge in d45424f. The merge brought in tasks.py (#1115 PRD decomposition) and 16 other files but zero changes into react_agent.py (the three-way diff on the merge commit is empty) and left no conflict markers — so the two earlier GLM passes and the f66f72b docstring/dead-reason fixes still stand. I re-ran the verification against the current file, not the diff.

Independently verified:

  • _iteration_budget_override() (react_agent.py:82-100) — int() parse with ValueError→None, <=0→None, empty/unset→None; os/logger in scope. No crash path.
  • Loop boundary (react_agent.py:542,600) — while iterations < self.max_iterations with post-increment yields exactly max_iterations LLM calls before falling through to exhaustion (:813), where iterations == self.max_iterations, so the blocker's reported cap is accurate.
  • Exhaustion blocker text is ~439 chars at N=45 (and stays well under 500 for any realistic budget), so the cf work diagnose pointer survives _create_text_blocker's text[:500] cap.
  • Origin mapping (react_agent.py:1460-1468) — _SYSTEM_DETECTED_REASONS = {stall_detected, iteration_budget_exhausted} → origin "system"; cost cap stays "agent" (pre-existing, out of scope). Both are valid BlockerOrigin members, so BlockerOrigin(created_by) can't raise.
  • Precedence (react_agent.py:1235-1237) — the override returns before the multiplier/clamp; _calculate_adaptive_budget is the only production caller feeding self.max_iterations before _react_loop, and CODEFRAME_MAX_ITERATIONS is read nowhere else, so no precedence conflict.
  • blockers.create (blockers.py:97-99) dedups on (workspace, question, OPEN, task_id) and returns the existing blocker on a duplicate (never raises), so a re-run that exhausts at the same budget re-uses the existing OPEN blocker and still returns BLOCKED — benign.
  • run() BLOCKED branch (react_agent.py:340-346) emits AGENT_FAILED+stream error — identical to the pre-existing cost-cap/stall path, so no new stream behavior; the only change vs. pre-PR is the reason string ("blocked" vs. the old "max_iterations_reached").
  • f66f72b fixes confirmed in place: _react_loop/run() docstrings now describe BLOCKED-on-exhaustion; the unreachable else is relabelled "unknown"; the relaxed assertion (error_events[0].error truthy) matches the no-DB → blocker-write-fails → FAILED degradation that test now documents.

No concrete defect with a reproducible failure scenario.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @frankbria's task in 3m 6s —— View job


Bug-hunting review — ✅ GLM review: no defects found

Fourth pass, re-triggered by the origin/main merge in c53efe4. That merge (like d45424f before it) pulled in tasks.py (#1115) and other files but zero changes into react_agent.py — the three-way diff on the merge commit is empty and there are no conflict markers — so this is the same react_agent.py the prior passes verified. I re-ran verification against the checked-out file, not the diff.

Independently verified:

  • _iteration_budget_override() (react_agent.py:82-100) — int() parse with ValueError/<=0/unset → None; os (L13) and logger (L50) in scope. No crash path.
  • Loop boundary (react_agent.py:542,600) — post-increment while iterations < self.max_iterations yields exactly max_iterations LLM calls before the exhaustion fall-through (:813), where iterations == self.max_iterations, so the blocker's reported cap is accurate.
  • Exhaustion path (react_agent.py:818-835) — _create_text_blocker(...) then return BLOCKED; sits inside run()'s try (L287), so a blocker-write failure propagates to except Exception (L391) → FAILED with reason "exception". No "BLOCKED without a blocker" state — matches the documented limitation.
  • Origin mapping (react_agent.py:122-125,1460-1468) — _SYSTEM_DETECTED_REASONS = {stall_detected, iteration_budget_exhausted}"system"; cost cap stays "agent" (the already-flagged out-of-scope nit, not a regression). BlockerOrigin("system")/("agent") are both valid (blockers.py:38-40), so BlockerOrigin(created_by) can't raise.
  • Precedence (react_agent.py:1230-1237) — override returns before the multiplier/clamp; _calculate_adaptive_budget is the only production caller feeding self.max_iterations before _react_loop (run() L298-304), and CODEFRAME_MAX_ITERATIONS is read nowhere else in codeframe//tests/ outside this PR's files.
  • Dedup (blockers.py:97-99) — create() returns the existing OPEN blocker on a (task_id, question, OPEN) match, never raises. The exhaustion question embeds the exact budget (Used the full budget of N iteration(s) + CODEFRAME_MAX_ITERATIONS={N*2}), so a re-run at a different budget produces a different question and is not falsely deduped; a re-run at the same budget reuses the OPEN blocker and still returns BLOCKED — benign either way.
  • run() BLOCKED branch (react_agent.py:340-346) emits AGENT_FAILED + stream error identical to the pre-existing cost-cap/stall path — no new stream behavior; only the reason string differs.
  • f66f72b fixes still in place: _react_loop/run() docstrings describe BLOCKED-on-exhaustion; the unreachable else (L326-332) is relabelled "unknown" (kept as a defensive default).
  • New tests (test_iteration_budget_outcome_1117.py) assert exact values (== 77, == 40, == 45), use real LLMResponse/ToolCall objects to survive the loop's JSON serialization, and the genuine-error test confirms a real failure is not dressed up as a blocker.

Edge cases considered and rejected (not defects):

  • If the final LLM call both exhausts iterations and crosses the cost cap, the loop exits via the exhaustion fall-through before the next top-of-loop cost check, so the reason reads iteration_budget_exhausted rather than cost_cap_exceeded. Both are BLOCKED + resumable — the outcome is correct, only the label is imprecise, and the structure predates this PR.
  • CODEFRAME_MAX_ITERATIONS=1 bypasses min_iterations (15) — explicit user override, documented inline at react_agent.py:1230-1234.
  • The cf work start {short_id} suggestion in the blocker relies on prefix matching; short_id is best-effort guidance text, not a code path.

No concrete defect with a reproducible failure scenario. No inline comments posted.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review

I re-verified the current state of codeframe/core/react_agent.py directly (not just the diff) against the claims in this PR and the five prior review rounds (three GLM bug-hunt passes + two claude passes). Everything I checked still holds and I found nothing new:

  • _iteration_budget_override() (react_agent.py:82-100) — unset/non-integer/<=0 all warn-and-return-None rather than raising; os/logger are in scope.
  • _BLOCKED_REASONS/_SYSTEM_DETECTED_REASONS (react_agent.py:110-125) and the exhaustion path (react_agent.py:813-835) match the PR description: iteration_budget_exhaustedBLOCKED, origin "system", message includes the cap, the git status pointer, the raise-and-rerun command, and cf work diagnose.
  • _create_text_blocker (react_agent.py:1448-1475) — system-detected reasons get "Run stopped: ..." + origin "system"; agent-detected reasons keep the original "Agent detected a blocker: ..." + origin "agent". Confirms the two prior findings (stale docstrings, dead max_iterations_reached branch) are genuinely fixed: run()'s docstring now correctly describes BLOCKED-on-exhaustion (react_agent.py:520-524), and the old else is relabelled "unknown" (react_agent.py:332).
  • _calculate_adaptive_budget (react_agent.py:1210-1247) — CODEFRAME_MAX_ITERATIONS returns before the complexity multiplier/clamp runs, matching the stated precedence.

One thing worth flagging only for the record, not as a defect (and this is consistent with the 439-char measurement already reported in a prior round): _create_text_blocker's text[:500] cap applies to the text argument only — the final stored question is f"Run stopped: {reason}\n\n{text[:500]}", so the total string can exceed 500 chars by the length of that prefix. At realistic iteration counts this is harmless (already measured well under budget), but it's the prefix, not the full message, that's guaranteed to fit — worth keeping in mind if reason strings ever get longer.

No new issues. This has had unusually thorough review coverage already (logic, error paths, origin mapping, precedence, blocker dedup, CLI surface, docstring/dead-code follow-ups) and my independent read of the current file confirms the implementation matches what was verified in those rounds.

@frankbria
frankbria merged commit 0dd195d into main Aug 10, 2026
13 checks passed
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.

[P1.40] Iteration-budget exhaustion is reported as a bare 'Task execution failed', hiding a nearly-complete implementation

1 participant