feat(core): wire StallAction dispatch into ReactAgent and runtime (#401) - #425
Conversation
- Add StallDetectedError exception for RETRY stall recovery path - Add stall_action parameter to ReactAgent (default: BLOCKER) - Wire configurable StallAction dispatch in react loop stall check: RETRY raises StallDetectedError, FAIL returns FAILED, BLOCKER creates blocker - Catch StallDetectedError in execute_agent() with 1 retry attempt - Add --stall-action CLI flag to work start and work batch run - Thread stall_action through conductor and subprocess calls - 24 tests covering all new integration points
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a StallDetectedError exception and a configurable stall_action (blocker/retry/fail) threaded from CLI through runtime, ReactAgent, and conductor/batch subprocess invocations, with runtime retry handling for RETRY and subprocess flag propagation for batch tasks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant CLI as CLI Parser
participant Runtime as Runtime
participant Agent as ReactAgent
participant Detector as StallDetector
participant Subproc as Task Subprocess
User->>CLI: cf work start --stall-action retry
CLI->>Runtime: execute_agent(..., stall_action="retry")
Runtime->>Agent: instantiate ReactAgent(stall_action=RETRY)
Agent->>Detector: start monitor
loop Agent tool loop
Agent->>Detector: record_activity()
Agent->>Detector: is_stalled()?
alt Stall detected
Detector->>Agent: stall event (elapsed_s, iterations, last_tool)
Agent->>Agent: branch on stall_action
alt RETRY
Agent->>Runtime: raise StallDetectedError(elapsed_s, iterations, last_tool)
Runtime->>Runtime: catch & retry (<= max retries)
else FAIL
Agent->>Agent: return FAILED
else BLOCKER
Agent->>Agent: create blocker & return BLOCKED
end
else No stall
Agent->>Agent: continue iterations
end
end
Note over Runtime,Subproc: For batch tasks, Conductor/Runtime pass --stall-action to Subproc
Runtime->>Subproc: run task --stall-action retry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Code Review: feat(core): wire StallAction dispatch into ReactAgent and runtimeThe PR is well-structured and the data flow is clean. The StallAction dispatch logic is clear and backwards-compatible (default stays BLOCKER). A few issues worth addressing: Bug / CorrectnessRedundant stop() call in except StallDetectedError (react_agent.py line 251) _react_loop() is called inside the inner try/finally block (lines 208-249). When StallDetectedError propagates out of _react_loop, the Input ValidationStallAction(stall_action) can raise a cryptic ValueError for bad CLI input (runtime.py line 679) If a user passes --stall-action typo, the raw enum ValueError propagates up through the CLI with no user-friendly message. The call should be wrapped with a try/except that provides a helpful error listing valid values. Alternatively, change the CLI parameter type from str to StallAction so Typer validates it automatically and auto-documents valid values in --help. Silent Flag / UX--stall-action is silently ignored when --engine plan is used The flag is wired into both work start and batch run, but the plan engine path in execute_agent never receives or uses it. A user running cf work start with --engine plan and --stall-action retry will get BLOCKER behavior with no warning. A short check or warning would prevent silent misuse. Design / Maintainabilitymax_stall_retries = 1 is hardcoded without explanation (runtime.py line 696) The retry count is intentionally capped at 1, but nothing documents why. A brief inline comment preserves intent and prevents future contributors from treating it as an oversight. Test CoverageBehavioral dispatch is not tested - only structural/signature checks TestReactAgentStallAction verifies that _stall_action is stored; TestExecuteAgentStallAction uses inspect.signature. Neither confirms that:
These are the most critical new behaviors. Mock-based unit tests for _react_loop with _stall_triggered pre-set for each action variant would close the gap without requiring a live LLM. Minorstate could be flagged as potentially unbound by static analysis (runtime.py) The for-loop in execute_agent always exits via break, so state is always set at runtime. However, mypy/pyright may warn that state could be unbound. A one-liner initialization before the loop eliminates the false positive. Summary: The BLOCKER -> RETRY/FAIL dispatch logic is correct and the parameter threading is mechanically sound. Addressing the redundant stop(), input validation for bad --stall-action values, and adding behavioral tests for the three dispatch paths would make this production-ready. |
|
PR Review for feat(core): wire StallAction dispatch into ReactAgent and runtime. Overall this is a clean, well-scoped change. The three-way dispatch (RETRY to exception, FAIL to status, BLOCKER to existing path) is easy to follow, and threading stall_action through the call chain is done consistently. ISSUE 1 - Redundant _stall_monitor.stop() in except StallDetectedError (react_agent.py lines 250-252): The inner try/finally (line 248-249) guarantees _stall_monitor.stop() is called before any exception propagates. StallMonitor.stop() is idempotent (_stop_event.set() is a no-op when already set, and the thread reference is nulled on first call), so this will not crash but the second stop() is dead code. The handler should just be: raise with a comment that finally already stopped the monitor. ISSUE 2 - No CLI-level validation of --stall-action value: Both work_start and batch_run accept stall_action as a plain str with no validation. An invalid value like --stall-action kaboom will not be caught until deep inside execute_agent() where StallAction(stall_action) raises a ValueError with a Python traceback instead of a user-friendly message. The Typer enum type approach is cleanest: import StallAction into app.py and use it as the parameter type so Typer shows valid choices automatically in --help. The subprocess path in batch mode makes this doubly important as invalid values will silently fail the subprocess. ISSUE 3 - max_stall_retries = 1 is a magic number (runtime.py line 696): Mentioned in the PR description but not obvious from the code. A named constant or clarifying comment would help future readers. ISSUE 4 - Tests are structural, not behavioral: The new test classes verify parameter existence and defaults (valuable for API stability) but the actual dispatch logic is not covered: that RETRY raises StallDetectedError, that FAIL returns AgentStatus.FAILED, that BLOCKER calls _create_text_blocker. These are testable by pre-setting _stall_triggered and _stall_event on an agent instance and calling _react_loop() directly. Worth tracking as a follow-up. Non-blocking notes: the _build_react_agent() closure captures outer variables cleanly; the retry loop logic is correct (verified); default stall_action=blocker preserves all existing behavior; init.py exports are consistent with the existing pattern. Summary: Items 1 and 2 are most important to address before merge. Items 3 and 4 are lower priority. The overall approach and data flow design are solid. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
codeframe/core/react_agent.py (1)
114-128:⚠️ Potential issue | 🟠 MajorHonor
stall_actionduring verification stalls too.This new field is only consulted in
_react_loop(). If the monitor fires during final verification, Line 497 still returns"stall_detected"and Lines 234-240 always turn that intoBLOCKED, so--stall-action fail|retryis ignored for long or stuck gate runs.Also applies to: 292-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/react_agent.py` around lines 114 - 128, The monitor "stall_detected" result is currently always mapped to BLOCKED in the reaction/verification flow, ignoring the configured stall_action; update the handling in _react_loop (and the similar handling at the other verification branch around the 292-315 region) to check self._stall_action (or stall_action) and act accordingly—i.e., when a verification monitor returns "stall_detected" choose between BLOCKED, FAIL, or RETRY per self._stall_action instead of hard-coding BLOCKED; ensure both the main loop path (_react_loop) and the separate verification path use the same decision logic so --stall-action fail|retry is honored.codeframe/core/conductor.py (1)
461-475:⚠️ Potential issue | 🟠 MajorPersist
stall_actionacrossBatchRunreloads.This only stores the new field on the in-memory object. The SQLite mapping in
_save_batch(),get_batch(),list_batches(), and_row_to_batch()still round-trips only throughengine, so any batch reloaded from the DB falls back to"blocker"/300. In practice,resume_batch()will ignore a non-default stall policy on the next attempt.🗄️ Minimal shape of the fix
- SELECT ..., results, engine + SELECT ..., results, engine, stall_timeout_s, stall_action - (id, workspace_id, task_ids, status, strategy, max_parallel, on_failure, - started_at, completed_at, results, engine) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + (id, workspace_id, task_ids, status, strategy, max_parallel, on_failure, + started_at, completed_at, results, engine, stall_timeout_s, stall_action) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) return BatchRun( ... engine=row[10] if len(row) > 10 and row[10] else "plan", + stall_timeout_s=row[11] if len(row) > 11 and row[11] is not None else 300, + stall_action=row[12] if len(row) > 12 and row[12] else "blocker", )This also needs the matching
batch_runsschema migration.Also applies to: 523-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/conductor.py` around lines 461 - 475, The BatchRun's stall_action (and stall_timeout_s) are only set on the in-memory object but not persisted or reloaded; update the SQLite persistence to include these fields: add columns to the batch_runs schema (migration), include stall_action and stall_timeout_s in _save_batch() INSERT/UPDATE, include them in SELECT projections used by get_batch() and list_batches(), and populate them in _row_to_batch() so reloaded BatchRun instances (and resume_batch()) retain non-default stall policies; ensure you keep sensible defaults when columns are absent for backward compatibility.
🤖 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/cli/app.py`:
- Around line 2006-2010: The CLI option currently defined as stall_action: str
should use the StallAction enum for parse-time validation: change the
typer.Option parameter type to StallAction (e.g., stall_action: StallAction =
typer.Option(StallAction.blocker, "--stall-action", help=...)) in both
work_start() and batch_run(), and remove the later runtime conversion
StallAction(stall_action) in execute_agent() so invalid values are rejected at
parsing and no IN_PROGRESS run is created; keep the same help text but set the
default to StallAction.blocker and update any downstream usage sites that expect
a string to accept the StallAction value (or call .value where a string is
required).
In `@codeframe/core/react_agent.py`:
- Around line 292-315: The FAIL branch currently returns AgentStatus.FAILED
which causes callers to report max_iterations_reached; change the FAIL branch to
raise StallDetectedError (same shape as the RETRY branch) with elapsed_s,
iterations, and last_tool so stalled runs are distinguishable; also ensure the
BLOCKER branch preserves diagnostics by passing iterations, last_tool and
elapsed_s into the blocker (e.g., include them in the stall_ctx passed to
_create_text_blocker or otherwise attach them) so iteration/last-tool/token
diagnostics are not lost (refer to symbols: StallAction.FAIL,
StallAction.BLOCKER, StallDetectedError, _create_text_blocker, and
recent_tool_signatures).
In `@tests/core/test_stall_detector.py`:
- Around line 14-17: Remove the unused import by deleting the `threading` import
statement from the top of the `tests/core/test_stall_detector.py` file (the line
that reads `import threading`) so the file only imports the used symbols
(`inspect`, `time`, and `MagicMock`), which resolves the Ruff F401 unused-import
error.
---
Outside diff comments:
In `@codeframe/core/conductor.py`:
- Around line 461-475: The BatchRun's stall_action (and stall_timeout_s) are
only set on the in-memory object but not persisted or reloaded; update the
SQLite persistence to include these fields: add columns to the batch_runs schema
(migration), include stall_action and stall_timeout_s in _save_batch()
INSERT/UPDATE, include them in SELECT projections used by get_batch() and
list_batches(), and populate them in _row_to_batch() so reloaded BatchRun
instances (and resume_batch()) retain non-default stall policies; ensure you
keep sensible defaults when columns are absent for backward compatibility.
In `@codeframe/core/react_agent.py`:
- Around line 114-128: The monitor "stall_detected" result is currently always
mapped to BLOCKED in the reaction/verification flow, ignoring the configured
stall_action; update the handling in _react_loop (and the similar handling at
the other verification branch around the 292-315 region) to check
self._stall_action (or stall_action) and act accordingly—i.e., when a
verification monitor returns "stall_detected" choose between BLOCKED, FAIL, or
RETRY per self._stall_action instead of hard-coding BLOCKED; ensure both the
main loop path (_react_loop) and the separate verification path use the same
decision logic so --stall-action fail|retry is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04783770-6f44-4b3d-a3ad-f5ef12eaa7ac
📒 Files selected for processing (7)
codeframe/cli/app.pycodeframe/core/__init__.pycodeframe/core/conductor.pycodeframe/core/react_agent.pycodeframe/core/runtime.pycodeframe/core/stall_detector.pytests/core/test_stall_detector.py
| stall_action: str = typer.Option( | ||
| "blocker", | ||
| "--stall-action", | ||
| help="Recovery action on stall: 'blocker' (default), 'retry', or 'fail'", | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and inspect the relevant lines in codeframe/cli/app.py
fd -n "app.py" | grep "cli"Repository: frankbria/codeframe
Length of output: 293
🏁 Script executed:
# Get the actual code at lines 2006-2010 and surrounding context
sed -n '2000,2020p' codeframe/cli/app.pyRepository: frankbria/codeframe
Length of output: 745
🏁 Script executed:
# Check if StallAction exists in codeframe/core/stall_detector
fd -n "stall_detector.py" codeframe/Repository: frankbria/codeframe
Length of output: 293
🏁 Script executed:
# Find stall_detector.py
find codeframe -name "stall_detector.py" -type fRepository: frankbria/codeframe
Length of output: 95
🏁 Script executed:
# Check batch_run() at lines 2883-2887
sed -n '2880,2900p' codeframe/cli/app.pyRepository: frankbria/codeframe
Length of output: 923
🏁 Script executed:
# Check work_start() function to see the line 2055 context and state mutation concern
sed -n '2050,2060p' codeframe/cli/app.pyRepository: frankbria/codeframe
Length of output: 507
🏁 Script executed:
# Examine the StallAction enum in stall_detector.py
cat codeframe/core/stall_detector.py | head -100Repository: frankbria/codeframe
Length of output: 2386
🏁 Script executed:
# Find where stall_action is validated/used in the codebase
rg "stall_action" -A 2 -B 2 codeframe/Repository: frankbria/codeframe
Length of output: 8462
🏁 Script executed:
# Check the full work_start() function signature and usage of stall_action
sed -n '1990,2070p' codeframe/cli/app.pyRepository: frankbria/codeframe
Length of output: 3079
🏁 Script executed:
# Check if there are existing Typer+Enum patterns in the codebase
rg "typer.Option" -A 3 codeframe/cli/app.py | grep -E "(Enum|enum|\[.*\])" -B 3Repository: frankbria/codeframe
Length of output: 6119
🏁 Script executed:
# Check what's imported at top of codeframe/cli/app.py
head -60 codeframe/cli/app.py | grep -E "^import|^from"Repository: frankbria/codeframe
Length of output: 333
🏁 Script executed:
# Verify: does work_start() call execute_agent with stall_action and what happens to it?
sed -n '2065,2075p' codeframe/cli/app.pyRepository: frankbria/codeframe
Length of output: 685
🏁 Script executed:
# Check what batch_run() does with stall_action - does it create state before validation?
sed -n '2850,2950p' codeframe/cli/app.py | head -100Repository: frankbria/codeframe
Length of output: 4100
🏁 Script executed:
# Check if Typer supports Enum type hints by searching for documentation patterns
rg "from enum import" codeframe/cli/Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Final check: verify the exact conversion logic in runtime.execute_agent()
rg "StallAction\(" -B 2 -A 2 codeframe/core/runtime.pyRepository: frankbria/codeframe
Length of output: 258
🏁 Script executed:
# Check if conductor.start_batch() creates persistent state immediately
rg "def start_batch" -A 15 codeframe/core/conductor.py | head -30Repository: frankbria/codeframe
Length of output: 504
🏁 Script executed:
# Verify: is BatchRun a database model that gets persisted on creation?
rg "class BatchRun" -A 10 codeframe/core/Repository: frankbria/codeframe
Length of output: 800
Use StallAction enum type for parse-time validation of --stall-action option.
The CLI currently accepts arbitrary strings and validates them only during execute_agent() via StallAction(stall_action) conversion. In work_start(), this creates a timing gap: the run is created at line 2055 before validation occurs, so an invalid --stall-action value leaves behind a dangling run in IN_PROGRESS state.
Suggested change
+from codeframe.core.stall_detector import StallAction
@@
- stall_action: str = typer.Option(
- "blocker",
+ stall_action: StallAction = typer.Option(
+ StallAction.BLOCKER,
"--stall-action",
help="Recovery action on stall: 'blocker' (default), 'retry', or 'fail'",
),Apply to both work_start() and batch_run().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stall_action: str = typer.Option( | |
| "blocker", | |
| "--stall-action", | |
| help="Recovery action on stall: 'blocker' (default), 'retry', or 'fail'", | |
| ), | |
| from codeframe.core.stall_detector import StallAction | |
| stall_action: StallAction = typer.Option( | |
| StallAction.BLOCKER, | |
| "--stall-action", | |
| help="Recovery action on stall: 'blocker' (default), 'retry', or 'fail'", | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/cli/app.py` around lines 2006 - 2010, The CLI option currently
defined as stall_action: str should use the StallAction enum for parse-time
validation: change the typer.Option parameter type to StallAction (e.g.,
stall_action: StallAction = typer.Option(StallAction.blocker, "--stall-action",
help=...)) in both work_start() and batch_run(), and remove the later runtime
conversion StallAction(stall_action) in execute_agent() so invalid values are
rejected at parsing and no IN_PROGRESS run is created; keep the same help text
but set the default to StallAction.blocker and update any downstream usage sites
that expect a string to accept the StallAction value (or call .value where a
string is required).
| elapsed_s = 0.0 | ||
| if self._stall_event: | ||
| elapsed_s = self._stall_event.elapsed_s | ||
| stall_ctx = ( | ||
| f"Agent stalled: no tool call for {self._stall_event.elapsed_s:.0f}s " | ||
| f"Agent stalled: no tool call for {elapsed_s:.0f}s " | ||
| f"(timeout: {self._stall_event.stall_timeout_s}s)" | ||
| ) | ||
| self._create_text_blocker( | ||
| stall_ctx or "Agent stalled with no tool activity", | ||
| "stall_detected", | ||
| ) | ||
| return AgentStatus.BLOCKED | ||
|
|
||
| if self._stall_action == StallAction.RETRY: | ||
| raise StallDetectedError( | ||
| elapsed_s=elapsed_s, | ||
| iterations=iterations, | ||
| last_tool=recent_tool_signatures[-1][0] if recent_tool_signatures else "", | ||
| ) | ||
| elif self._stall_action == StallAction.FAIL: | ||
| self._verbose_print(f"[ReactAgent] Stall → FAILED: {stall_ctx}") | ||
| return AgentStatus.FAILED | ||
| else: | ||
| # StallAction.BLOCKER (default) | ||
| self._create_text_blocker( | ||
| stall_ctx or "Agent stalled with no tool activity", | ||
| "stall_detected", | ||
| ) | ||
| return AgentStatus.BLOCKED |
There was a problem hiding this comment.
Keep stall failures distinct from max-iteration failures.
The FAIL branch returns only AgentStatus.FAILED, but Line 211 later emits reason="max_iterations_reached" for any failed _react_loop() result. That misreports stalled runs, and the BLOCKER branch also drops the iteration / last-tool / token diagnostics this feature is supposed to preserve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/react_agent.py` around lines 292 - 315, The FAIL branch
currently returns AgentStatus.FAILED which causes callers to report
max_iterations_reached; change the FAIL branch to raise StallDetectedError (same
shape as the RETRY branch) with elapsed_s, iterations, and last_tool so stalled
runs are distinguishable; also ensure the BLOCKER branch preserves diagnostics
by passing iterations, last_tool and elapsed_s into the blocker (e.g., include
them in the stall_ctx passed to _create_text_blocker or otherwise attach them)
so iteration/last-tool/token diagnostics are not lost (refer to symbols:
StallAction.FAIL, StallAction.BLOCKER, StallDetectedError, _create_text_blocker,
and recent_tool_signatures).
…reason - Verification stall check now dispatches via StallAction (RETRY/FAIL/BLOCKER) - FAILED status from react loop correctly reports stall_detected vs max_iterations
|
Follow-up Review after commit c8883ee. The third commit addresses two genuine gaps. Fixed: _run_final_verification now dispatches RETRY/FAIL/BLOCKER correctly on stall, and AGENT_FAILED events now emit stall_detected vs max_iterations_reached accurately. Both correct. Still open from prior reviews: (1) Redundant _stall_monitor.stop() in the except StallDetectedError block in react_agent.py line 250 - the inner try/finally already calls stop() before exception reaches there. (2) No CLI validation for --stall-action - both work_start and batch_run take stall_action as plain str, invalid values raise raw ValueError in runtime.py; use StallAction as the Typer type for free validation and --help documentation. (3) max_stall_retries equals 1 is undocumented in runtime.py line 696 - add a comment explaining the intentional cap. (4) Tests remain structural not behavioral - no coverage that RETRY raises StallDetectedError, FAIL returns AgentStatus.FAILED, or BLOCKER calls _create_text_blocker. Items 1-3 are small and addressable here; item 4 can be a follow-up. |
1 participant
Summary
Wires the
StallActionenum into the recovery flow so stall detection can trigger configurable recovery actions instead of always creating a blocker.StallDetectedErrorexception class for the RETRY recovery pathstall_actionparameter threaded through CLI → runtime → ReactAgent → conductor--stall-actionCLI flag onwork startandwork batch run(default:blocker)execute_agent()catchesStallDetectedErrorand retries up to 1 timeData flow
Closes #401
Test plan
Summary by CodeRabbit
New Features
User-facing
Tests