fix(stall): validate --stall-action at CLI and cleanup (#401) - #426
Conversation
…date docstring - Add click.Choice validation to --stall-action (rejects invalid values at CLI) - Remove redundant stall_monitor.stop() in StallDetectedError handler (finally already handles it) - Add stall_timeout_s and stall_action to execute_agent docstring
WalkthroughAdded Click input validation for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/core/runtime.py`:
- Around line 617-618: Update the docstring to state that stall_timeout_s and
stall_action apply only to the React engine; specifically mention that
execute_agent() consumes stall_timeout_s and stall_action only when engine ==
"react" (they are not used by the legacy plan engine). Reference the parameters
stall_timeout_s and stall_action and the function execute_agent so readers know
where the restriction applies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00e8099a-3041-476b-a951-64b4ad248701
📒 Files selected for processing (3)
codeframe/cli/app.pycodeframe/core/react_agent.pycodeframe/core/runtime.py
| stall_timeout_s: Seconds without tool activity before stall detection (0 = disabled) | ||
| stall_action: Recovery action on stall ("blocker", "retry", or "fail") |
There was a problem hiding this comment.
Clarify that these stall controls are React-only.
execute_agent() only consumes stall_timeout_s and stall_action in the engine == "react" path, so this docstring currently implies support that the legacy plan engine doesn't have.
📝 Suggested wording
- stall_timeout_s: Seconds without tool activity before stall detection (0 = disabled)
- stall_action: Recovery action on stall ("blocker", "retry", or "fail")
+ stall_timeout_s: React engine only. Seconds without tool activity before stall detection (0 = disabled)
+ stall_action: React engine only. Recovery action on stall ("blocker", "retry", or "fail")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/runtime.py` around lines 617 - 618, Update the docstring to
state that stall_timeout_s and stall_action apply only to the React engine;
specifically mention that execute_agent() consumes stall_timeout_s and
stall_action only when engine == "react" (they are not used by the legacy plan
engine). Reference the parameters stall_timeout_s and stall_action and the
function execute_agent so readers know where the restriction applies.
Summary
Follow-up polish from code review on #425:
--stall-actionat CLI layer withclick.Choice(prevents unhelpful ValueError deep in runtime)stall_monitor.stop()inStallDetectedErrorhandler (finally block already handles it)execute_agentdocstring withstall_timeout_sandstall_actionparametersTest plan
Summary by CodeRabbit
Release Notes
New Features
Improvements