fix: bound while-loop iterations without ToolCall by the step limit#142
fix: bound while-loop iterations without ToolCall by the step limit#142Mingye-Lu wants to merge 2 commits into
Conversation
execute_while_loop only called check_limits() per iteration, and step_counter was only ever incremented in execute_node's ToolCall arm. A loop body containing only Assign/Expression re-evaluation (no ToolCall) never advanced step_counter, so such a loop was bounded only by the wall-clock timeout (up to max_timeout_secs), not max_steps -- it could spin unthrottled, burning CPU, until the much larger wall-clock deadline caught it. Increment step_counter on every while iteration regardless of whether a tool call occurred. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3a2da851c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // that only does Assign/Expression re-evaluation never | ||
| // advances step_counter, and this loop is bounded only by | ||
| // the wall-clock timeout in check_limits(), not max_steps. | ||
| self.step_counter.fetch_add(1, Ordering::Relaxed); |
There was a problem hiding this comment.
When a while body contains a ToolCall, this increment is followed by execute_node incrementing step_counter again in the ToolCall arm, so these loops now consume two steps per browser action. A script that pages through results with one click/read_content per while iteration and limits.max_steps: 50 will abort after roughly 25 tool calls, unlike the same work in a for/for_each loop or straight-line script where each tool call consumes one step; consider only applying the synthetic iteration charge when the body did not otherwise advance the counter, or centralizing counting consistently per executed node.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23a90e5 — the synthetic iteration charge is now conditional: only applied when the while-loop body did not already advance step_counter (i.e., no ToolCall ran). While loops with ToolCall bodies now consume one step per iteration, matching for/for_each and straight-line behavior.
… bodies Before this fix, execute_while_loop unconditionally incremented step_counter on every iteration, and execute_node's ToolCall arm also incremented it. This meant each iteration with a browser action consumed 2 steps. Now the synthetic iteration charge is only applied when the body did NOT already advance the counter (i.e., no ToolCall ran), so while loops with ToolCall bodies consume one step per iteration like for/for_each loops and straight-line scripts. PR #142 — resolves review comment 3566182334
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Summary
execute_while_looponly calledcheck_limits()per iteration;step_counterwas only ever incremented inexecute_node'sToolCallarm. A loop body containing onlyAssign/Expressionre-evaluation (noToolCall) never advancedstep_counter, so such a loop was bounded only by the wall-clock timeout (max_timeout_secs), notmax_steps— it could spin unthrottled, burning CPU, until the much larger wall-clock deadline caught it.step_counteron everywhileiteration regardless of whether a tool call occurred, somax_stepsbounds it as intended.Test plan
cargo fmt --package agentcargo test -p agent(629 passed, including new regression testwhile_loop_without_tool_calls_is_bounded_by_step_limit, which uses a tool-call-free body and a shortmax_timeout_secsso the test fails fast rather than hanging if this regresses)cargo clippy -p agent --all-targets -- -D warnings(clean)