feat: Agent stall detection and recovery (#399) - #423
Conversation
Add StallMonitor watchdog that detects when agent stops making progress (no tool calls for configurable duration). Thread-based daemon polls at regular intervals and fires callback on stall. - StallMonitor class with start/stop/notify_tool_executed API - StallEvent dataclass for stall context - stall_timeout_s config field (default 300s, 0=disabled) - AGENT_STALL_DETECTED event type - 17 unit tests covering timing, threading, config
Wire StallMonitor into the agent execution pipeline: - ReactAgent: start/stop monitor around _react_loop, check stall flag at top of each iteration and verification retry, notify on tool success - execute_agent: accept and forward stall_timeout_s parameter - CLI: --stall-timeout flag on work start and batch run (default 300s) - Conductor: forward stall_timeout_s through BatchRun and subprocess cmd - On stall: creates blocker with context and returns BLOCKED status
Add stall detection section to Agent System docs covering CLI flag, config key, and default behavior.
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds stall detection to agent execution: a background StallMonitor watches for periods with no tool calls and, after a configurable timeout (default 300s), triggers a stall event that transitions execution to BLOCKED and creates a blocker. Timeout configurable via CLI Changes
Sequence DiagramsequenceDiagram
participant Agent as ReactAgent
participant Monitor as StallMonitor
participant LLM as LLM Provider
participant Watch as Watchdog Thread
participant Blocker as Blocker Creator
Agent->>Monitor: start(task_id)
Monitor->>Watch: spawn watchdog (daemon)
Agent->>LLM: request next action
LLM->>Agent: return action
Agent->>Agent: execute tool
Agent->>Monitor: notify_tool_executed(task_id, iteration)
Monitor->>Monitor: reset last_activity
Note over LLM,Agent: LLM hangs or agent loops without tool calls
Watch->>Monitor: poll elapsed time
Monitor->>Monitor: elapsed > stall_timeout_s?
Monitor->>Agent: invoke on_stall(StallEvent)
Agent->>Blocker: create blocker with stall context
Agent->>Agent: set _stall_triggered, emit AGENT_STALL_DETECTED
Agent->>Agent: return BLOCKED status
Agent->>Monitor: stop()
Monitor->>Watch: terminate watchdog
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 - Agent Stall Detection and Recovery. Overall this is a clean, well-scoped implementation that follows the project core-first headless architecture. The StallMonitor is a standalone daemon thread with correct use of threading primitives, 22 unit tests cover the main behaviors, and conductor/CLI wiring is consistent. Issues: (1) Medium - stall detection does not interrupt a hanging LLM call. _stall_triggered is set from the watchdog thread but the main loop only checks it at the top of each iteration. If the LLM hangs for 600s the watchdog fires at 300s but the agent does not return BLOCKED until the LLM call completes. The stall timeout is a lower bound on detection not an upper bound on execution time - worth documenting in AGENTS.md. (2) Minor - only successful tool calls reset the timer. Error tool calls do not call notify_tool_executed, so an agent in an error-recovery loop still has its stall timer counting down. If this is intentional the name notify_tool_executed is misleading and a comment explaining the design choice would help. (3) Minor - stall_timeout_s type is inconsistent: AgentBudgetConfig uses int, StallMonitor and ReactAgent use float, runtime.execute_agent uses int. Python coerces these transparently but float throughout would match the sub-second usage in tests. (4) Minor - config.yaml integration may not be fully wired. AgentBudgetConfig.stall_timeout_s is defined but the diff does not show it flowing into execute_agent. The CLI flag path is wired, but the config file path may be a silent no-op. Nits: the 5 _execute_task_subprocess call sites in conductor.py are all updated consistently; stop() join timeout could in theory leave a ghost daemon thread on slow machines; timing-based tests with 0.2-0.3s windows may flake under heavy CI load. Strengths: daemon thread will not block exit, lock usage is minimal and correct, _stop_event.wait avoids busy-waiting, idempotent stop is tested, AGENTS.md docs are clear, 1404 tests pass. Most important to resolve before merge: issue 1 (document LLM-hang behavior) and issue 4 (verify config.yaml wiring). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/core/conductor.py (1)
460-520:⚠️ Potential issue | 🟠 MajorPersist
stall_timeout_swith the batch record.Lines 460 and 520 only carry the timeout in memory. Reloaded batches are reconstructed via
get_batch()/_row_to_batch(), but_save_batch()and the batch SELECTs still serialize only throughengine, so any resumed or reloaded batch falls back to the dataclass default of300. A batch started with--stall-timeout 0or a custom value will therefore resume with the wrong stall behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/conductor.py` around lines 460 - 520, The batch's stall_timeout_s is only set in memory in start_batch (BatchRun(..., stall_timeout_s=...)) but not persisted; update persistence so resumed batches keep the configured timeout: modify _save_batch to include stall_timeout_s when writing the BatchRun record, update any INSERT/UPDATE SQL/ORM fields and the SELECTs used by get_batch/_row_to_batch to read stall_timeout_s, and ensure _row_to_batch maps the stored stall_timeout_s into the BatchRun dataclass; also add any necessary DB schema change or default handling so older rows get a sensible value.
🧹 Nitpick comments (1)
tests/core/test_stall_monitor.py (1)
91-145: These sleep-based timing assertions will be flaky in CI.The sub-second
sleep()windows here leave very little headroom for scheduler jitter in a background-thread test. Prefer waiting on a callback-ownedthreading.Event(or polling until a deadline) so these monitor tests stay stable on slower runners.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_stall_monitor.py` around lines 91 - 145, Replace fragile time.sleep assertions in these tests by having the MagicMock callback set a threading.Event (or use a small polling loop with deadline) and waiting on that event with a generous timeout; specifically, in tests using StallMonitor (test_stall_fires_callback_after_timeout, test_notify_prevents_stall, test_disabled_when_timeout_zero, test_callback_receives_correct_iteration_count) attach an Event to the on_stall callback so the test waits event.wait(timeout=...) instead of time.sleep, then assert event.is_set() (or not set) and inspect callback.call_args for the StallEvent and iteration count; ensure notify_tool_executed("task-1", iteration=...) and monitor.start/stop logic remain unchanged.
🤖 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 2001-2005: The CLI is forcing a concrete default for stall_timeout
which overrides workspace config.agent_budget.stall_timeout_s; change the typer
Option declarations for stall_timeout to use an "unset" sentinel (e.g., default
None / Optional[int]) so omitted flags are distinguishable from an explicit 300,
and propagate None downstream so core code can resolve using config defaults
(agent_budget.stall_timeout_s). Update every occurrence of the stall_timeout CLI
Option (the current stall_timeout parameters in app.py and the other occurrences
you noted) to be optional/unset at the boundary and ensure callers that forward
this value treat None as “use config” rather than a numeric timeout.
In `@codeframe/core/react_agent.py`:
- Around line 202-206: The stall monitor is stopped immediately after
_react_loop returns, so _run_final_verification (and any gates.run() or
verification retry loops) can hang without triggering stall recovery; keep the
monitor running until verification completes and ensure verification stalls are
routed through the same blocker/BLOCKED flow used by the main loop. Concretely:
move the call to self._stall_monitor.stop() to after _run_final_verification()
completes (or wrap both _react_loop(...) and self._run_final_verification(...)
inside the same try/finally guarded by self._stall_monitor.start(task_id)), and
update the verification path (the code that calls _run_final_verification and
any gates.run() loops) to create blocker entries and set the BLOCKED state using
the same blocker creation flow as the main loop so that the stall handler can
detect and recover from verification hangs.
- Around line 370-373: The stall monitor is only notified on successful tool
results (checks result.is_error) causing failing tool calls like
run_tests/run_command to not reset the stall timer; change the logic in
react_agent.py so that
self._stall_monitor.notify_tool_executed(self._current_task_id, iterations) is
called for every completed tool invocation (regardless of result.is_error)—i.e.,
move the notify_tool_executed call out of the result.is_error conditional so all
tool completions trigger the stall monitor update.
In `@codeframe/core/stall_monitor.py`:
- Around line 78-80: The start() initialization currently sets
self._last_activity = datetime.now(...), causing first-iteration stalls to
report a fake timestamp; change the initialization in start() (and the similar
block at lines 118-123) to set self._last_activity = None so "never executed a
tool" is preserved, and ensure the code paths that emit StallEvent (and any uses
of _last_activity) handle None and translate it to last_tool_call_at = None;
also ensure the actual tool-run path(s) (methods that update activity, e.g., the
function that records tool invocations) set self._last_activity to
datetime.now(timezone.utc) when a tool runs.
---
Outside diff comments:
In `@codeframe/core/conductor.py`:
- Around line 460-520: The batch's stall_timeout_s is only set in memory in
start_batch (BatchRun(..., stall_timeout_s=...)) but not persisted; update
persistence so resumed batches keep the configured timeout: modify _save_batch
to include stall_timeout_s when writing the BatchRun record, update any
INSERT/UPDATE SQL/ORM fields and the SELECTs used by get_batch/_row_to_batch to
read stall_timeout_s, and ensure _row_to_batch maps the stored stall_timeout_s
into the BatchRun dataclass; also add any necessary DB schema change or default
handling so older rows get a sensible value.
---
Nitpick comments:
In `@tests/core/test_stall_monitor.py`:
- Around line 91-145: Replace fragile time.sleep assertions in these tests by
having the MagicMock callback set a threading.Event (or use a small polling loop
with deadline) and waiting on that event with a generous timeout; specifically,
in tests using StallMonitor (test_stall_fires_callback_after_timeout,
test_notify_prevents_stall, test_disabled_when_timeout_zero,
test_callback_receives_correct_iteration_count) attach an Event to the on_stall
callback so the test waits event.wait(timeout=...) instead of time.sleep, then
assert event.is_set() (or not set) and inspect callback.call_args for the
StallEvent and iteration count; ensure notify_tool_executed("task-1",
iteration=...) and monitor.start/stop logic remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbe58d1b-b653-4e80-98b9-9496505c57e2
📒 Files selected for processing (9)
AGENTS.mdcodeframe/cli/app.pycodeframe/core/conductor.pycodeframe/core/config.pycodeframe/core/events.pycodeframe/core/react_agent.pycodeframe/core/runtime.pycodeframe/core/stall_monitor.pytests/core/test_stall_monitor.py
| stall_timeout: int = typer.Option( | ||
| 300, | ||
| "--stall-timeout", | ||
| help="Seconds without a tool call before agent is considered stalled (0 = disabled)", | ||
| ), |
There was a problem hiding this comment.
The CLI default currently overrides workspace config.
Both commands give stall_timeout a concrete default and always pass it downstream, so omitting --stall-timeout is indistinguishable from explicitly choosing 300. That makes .codeframe/config.yaml's agent_budget.stall_timeout_s ineffective on the main CLI execution paths. Use an "unset" sentinel at the CLI boundary and let core resolve config/defaults.
Also applies to: 2068-2070, 2872-2876, 2965-2975
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/cli/app.py` around lines 2001 - 2005, The CLI is forcing a concrete
default for stall_timeout which overrides workspace
config.agent_budget.stall_timeout_s; change the typer Option declarations for
stall_timeout to use an "unset" sentinel (e.g., default None / Optional[int]) so
omitted flags are distinguishable from an explicit 300, and propagate None
downstream so core code can resolve using config defaults
(agent_budget.stall_timeout_s). Update every occurrence of the stall_timeout CLI
Option (the current stall_timeout parameters in app.py and the other occurrences
you noted) to be optional/unset at the boundary and ensure callers that forward
this value treat None as “use config” rather than a numeric timeout.
There was a problem hiding this comment.
Valid concern. The CLI default pattern matches how --engine works today (also has a concrete default that overrides config). For now this is acceptable — the config precedence layer (issue #399 step 6 in the plan) was intentionally deferred as YAGNI. If users actually set stall_timeout_s in config and expect it to be respected from CLI, we can add sentinel-based detection in a follow-up.
There was a problem hiding this comment.
@frankbria, understood — the concrete default matches the existing --engine precedence pattern and the config-first resolution is a planned future concern. I'll note this for future reviews.
Would you like me to open a follow-up issue to track the sentinel-based CLI/config precedence work (config → CLI default → explicit CLI flag) when the time comes?
✏️ Learnings added
Learnt from: frankbria
Repo: frankbria/codeframe PR: 423
File: codeframe/cli/app.py:2001-2005
Timestamp: 2026-03-08T05:05:03.452Z
Learning: In codeframe/cli/app.py, the `--stall-timeout` CLI option (and `--engine`) intentionally uses a concrete default (300s) that overrides workspace config (`agent_budget.stall_timeout_s`). Config-first precedence resolution (sentinel-based detection where `None` means "use config") is deferred as YAGNI and tracked as a follow-up for a future PR. Do not flag this pattern as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:24:27.822Z
Learning: Applies to codeframe/core/runtime.py : Default execution engine for agent tasks must be ReAct (Reasoning + Acting) with iterative tool-use loop (observe → think → act). Legacy plan-based engine available via `--engine plan` flag only.
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T17:24:51.290Z
Learning: Applies to codeframe/cli/app.py : Use `--engine react` (default ReAct engine) for most tasks; `--engine plan` (legacy) only for well-defined, predictable tasks
| if not result.is_error: | ||
| self._stall_monitor.notify_tool_executed( | ||
| self._current_task_id, iterations, | ||
| ) |
There was a problem hiding this comment.
Failing tool calls still count as activity.
The feature defines a stall as "no tool calls", but the timer is only reset for non-error results here. If the agent is actively iterating on failing run_tests or run_command calls, it will eventually be misclassified as stalled and blocked. Notify the monitor after every completed tool call, not only successful ones.
Minimal fix
result = self._execute_tool_with_lint(tc)
-
- if not result.is_error:
- self._stall_monitor.notify_tool_executed(
- self._current_task_id, iterations,
- )
+ self._stall_monitor.notify_tool_executed(
+ self._current_task_id,
+ iterations,
+ )🤖 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 370 - 373, The stall monitor is
only notified on successful tool results (checks result.is_error) causing
failing tool calls like run_tests/run_command to not reset the stall timer;
change the logic in react_agent.py so that
self._stall_monitor.notify_tool_executed(self._current_task_id, iterations) is
called for every completed tool invocation (regardless of result.is_error)—i.e.,
move the notify_tool_executed call out of the result.is_error conditional so all
tool completions trigger the stall monitor update.
There was a problem hiding this comment.
This is intentional design. A stall means the agent is not doing anything (e.g., LLM API hangs or agent loops without acting). When the agent is actively executing tools — even failing ones — it IS making progress (discovering errors, trying fixes). The existing fix_tracker and escalation system handles repeated failures. Stall detection is specifically for the case where nothing happens at all.
There was a problem hiding this comment.
@frankbria, understood — that's a clear and well-reasoned separation of concerns. Stall detection (no tool calls at all, e.g., LLM hang) vs. repeated failure recovery (fix_tracker + escalation) are orthogonal problems handled by distinct systems. I'll withdraw the concern.
✏️ Learnings added
Learnt from: frankbria
Repo: frankbria/codeframe PR: 423
File: codeframe/core/react_agent.py:370-373
Timestamp: 2026-03-08T05:05:00.413Z
Learning: In `codeframe/core/react_agent.py`, `StallMonitor.notify_tool_executed()` is intentionally called only on *successful* tool results (`if not result.is_error`). A "stall" is defined as the agent executing zero tool calls (e.g., LLM API hang or agent looping without acting). Failing tool calls still constitute activity and are handled by the separate `fix_tracker` / escalation system. Do not flag this conditional as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T17:24:51.290Z
Learning: Applies to codeframe/core/react_agent.py : ReAct agent must implement the observe → think → act loop using the 7 defined tools: read_file, edit_file, create_file, run_command, run_tests, search_codebase, list_files
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:24:27.822Z
Learning: Applies to codeframe/core/{react_agent,tools}.py : ReactAgent implementation must use 7 structured tools: `read_file`, `edit_file`, `create_file`, `run_command`, `run_tests`, `search_codebase`, `list_files`. These tools are defined in `core/tools.py` and must be called via tool-use protocol.
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:24:27.822Z
Learning: Applies to codeframe/core/runtime.py : Default execution engine for agent tasks must be ReAct (Reasoning + Acting) with iterative tool-use loop (observe → think → act). Legacy plan-based engine available via `--engine plan` flag only.
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T17:24:51.290Z
Learning: Applies to codeframe/core/{agent,react_agent,runtime}.py : Agent state transitions (IDLE, PLANNING, EXECUTING, BLOCKED, COMPLETED, FAILED) must be managed by the Agent class; runtime handles TaskStatus transitions (BACKLOG, READY, IN_PROGRESS, DONE, BLOCKED, FAILED). Agent must NOT call `tasks.update_status()` directly.
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T17:24:51.290Z
Learning: Applies to codeframe/core/agent.py : Do NOT update task status from agent.py; runtime handles all TaskStatus transitions based on agent state
Learnt from: frankbria
Repo: frankbria/codeframe PR: 360
File: codeframe/core/tools.py:651-735
Timestamp: 2026-02-09T02:52:03.968Z
Learning: In codeframe/core/tools.py, agent tool functions (read_file, list_files, search_codebase, edit_file, create_file, run_tests, run_command) must be stateless with signature (input_data: dict, workspace_path: Path, tool_call_id: str) -> ToolResult and must not emit events. They are separate from verification gates in codeframe/core/gates.py. Gate diagnostic events (GATES_STARTED, GATES_COMPLETED) belong in the verification pipeline (gates.run()), not inside individual tools. If tool observability is required, implement it at the execute_tool dispatcher level, not within the individual tool implementations.
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:24:27.822Z
Learning: Applies to codeframe/core/{gates,agent,react_agent}.py : Verification gates must run incrementally after file changes (ruff) and finally (pytest, ruff, BUILD). Self-correction loop retries failed gates up to 5 times for ReAct and 3 times for plan engine with pattern-based quick fixes before LLM intervention.
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:24:27.822Z
Learning: Applies to codeframe/core/{diagnostics,diagnostic_agent}.py : Failed task diagnosis via `cf work diagnose <task-id>` must use AI-powered analysis via `core/diagnostic_agent.py`. Analyze error logs, failed verification gates, and previous attempts to suggest root causes and recovery actions.
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:24:27.822Z
Learning: Applies to codeframe/core/{fix_tracker,quick_fixes}.py : Self-correction loop must track fix attempts via `core/fix_tracker.py` to prevent repeating failed fixes. Normalize errors for comparison and detect escalation patterns (same error/file 3+ times). Use pattern-based quick fixes via `core/quick_fixes.py` before LLM intervention.
Learnt from: frankbria
Repo: frankbria/codeframe PR: 128
File: tests/agents/test_bottleneck_detection.py:486-500
Timestamp: 2025-12-17T19:21:40.014Z
Learning: In tests/agents/test_bottleneck_detection.py, test_skip_agents_below_threshold should be async and mock _get_agent_workload to return workload below AGENT_OVERLOAD_THRESHOLD (5) while providing non-empty tasks list to prevent early return in detect_bottlenecks().
- Extend stall monitor lifetime to cover final verification (not just the react loop), so hangs during gates.run() are also detected - Track last_tool_call_at separately from last_activity so initial stalls correctly report None for last tool call timestamp - Handle stall_detected reason from final verification
PR #423 Review: Agent Stall Detection and RecoveryOverall this is a clean, well-scoped implementation. The architecture follows established patterns (headless core, daemon thread, event emission) and test coverage is solid. A few issues worth addressing before merge. Issues1. Config file to runtime wiring appears incomplete
2. Stall timer only resets on successful tool calls In if not result.is_error:
self._stall_monitor.notify_tool_executed(...)If the agent is actively attempting error recovery (retrying a failing tool, running ruff fixes, etc.), the stall timer will not reset even though the agent is making real progress. This risks false stall detections during intensive self-correction loops. Consider resetting on any tool call attempt, since a stall should mean "no tool calls attempted", not "no successful tool calls". 3. Type inconsistency: float vs int for stall_timeout_s
Pick one. If sub-second granularity is needed (the tests use 0.2, 0.3), Minor NotesTiming-sensitive tests may be flaky in CI
No stall support for the plan engine Clearly intentional (plan engine is legacy), but worth a note in AGENTS.md stating stall detection applies to the ReAct engine only. What is Working Well
The critical items are issue 2 (false stalls during error recovery) and confirming issue 1 (config wiring). Type consistency is a polish item. |
1 participant
Summary
Implements #399: Agent Stall Detection and Recovery
finally. Stall check at top of each iteration and verification retry. Creates blocker with context on stall → returns BLOCKED.--stall-timeoutflag onwork startandwork batch run(default 300s, 0=disabled)agent_budget.stall_timeout_sin.codeframe/config.yamlAGENT_STALL_DETECTEDevent type for monitoring--stall-timeoutto subprocess commandsAcceptance Criteria
Test Plan
Implementation Notes
threading.Lock--stall-timeoutvia subprocess CLI args (same pattern as--engine)Closes #399
Summary by CodeRabbit
New Features
Tests
Documentation