fix: max_steps=0 falsy fallback and planning remaining_steps override#2526
Open
ErenAta16 wants to merge 1 commit into
Open
fix: max_steps=0 falsy fallback and planning remaining_steps override#2526ErenAta16 wants to merge 1 commit into
ErenAta16 wants to merge 1 commit into
Conversation
…huggingface#2458, huggingface#2510) Two related bugs in max_steps handling: 1. run(max_steps=0) silently fell back to the agent constructor default via `max_steps = max_steps or self.max_steps` (0 is falsy). max_steps is typed `int | None = None`, so 0 is a legitimate, distinct value from not-provided, changed to an explicit is-not-None check. Fixing this exposed an existing edge case: with zero steps taken, the loop body that assigns action_step never runs, and the max-steps-reached branch unconditionally yielded it, raising UnboundLocalError. Initialized action_step to None before the loop and guarded the yield. 2. Planning updates render the literal text "{remaining_steps}" instead of a number, and even once fixed, the value comes from self.max_steps (the constructor default) rather than the effective per-run budget when run(max_steps=N) overrides it. populate_template uses Jinja2 with StrictUndefined, which requires {{ }} for interpolation; the templates used single-brace {remaining_steps}, which Jinja2 never substitutes, so it reached the model as literal text. Fixed the three built-in planning templates (code_agent, structured_code_agent, toolcalling_agent) to use {{ remaining_steps }}, and threaded the effective max_steps through _run_stream -> _generate_planning_step so remaining_steps is computed from the actual per-run budget. Testing: added test_run_max_steps_zero_is_respected and test_planning_step_remaining_steps_uses_per_run_max_steps_override, updated the existing test_planning_step call site for the new _generate_planning_step signature. Ran the full tests/test_agents.py suite before and after against the unfixed code to confirm: all remaining failures (missing numpy/transformers, from_folder version compat, an ImportError, one flaky timing assertion) are pre-existing and unrelated, present identically on unfixed main. Fixes huggingface#2458 Fixes huggingface#2510 Signed-off-by: ErenAta16 <erena6466@gmail.com>
This was referenced Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2458 and #2510, two related bugs in how
max_stepspropagates through a run.#2458:
max_steps=0silently ignoredrun()doesmax_steps = max_steps or self.max_steps. Sincemax_steps: int | None = Noneand0is falsy,run(task, max_steps=0)silently falls back to the agent's constructor default instead of running zero steps.Fixing this exposed a pre-existing edge case: when the loop body never executes (zero steps),
action_stepwas never assigned, but the max-steps-reached branch unconditionally didyield action_step, raisingUnboundLocalError. Initializedaction_step = Nonebefore the loop and guarded the yield.#2510: planning
remaining_stepsplaceholder never interpolated, and uses the wrong budgetTwo stacked issues on the same value:
code_agent.yaml,structured_code_agent.yaml,toolcalling_agent.yaml) use{remaining_steps}(single brace).populate_templaterenders with Jinja2 (Template(..., undefined=StrictUndefined)), which requires{{ }}for interpolation, single braces are literal text. So the model receives the literal string{remaining_steps}instead of a number.self.max_steps(the constructor default) inside_generate_planning_step, not the effective per-run budget. Whenrun(task, max_steps=N)overrides the default, the execution loop correctly usesN, but planning was still reading the constructor value. Threadedmax_stepsthrough_run_stream→_generate_planning_stepso both agree.Testing
test_run_max_steps_zero_is_respected:run(..., max_steps=0)takes zero action steps and returns cleanly (noUnboundLocalError).test_planning_step_remaining_steps_uses_per_run_max_steps_override: planning text contains the per-run override's remaining-steps number, not the constructor default's, and never contains the literal{remaining_steps}placeholder.test_planning_step's call site for the new_generate_planning_step(..., max_steps=...)signature.Ran the full
tests/test_agents.pysuite before and after against unfixed code to isolate what's actually caused by this change: the same ~15 failures are present identically on unfixedmain(missingnumpy/transformersextras,from_folderversion-compat fixtures, oneImportError, one flaky timing assertion, all unrelated tomax_steps), confirming this diff doesn't introduce any new failures.