Repository navigation
feat: extract runtime turn error handler - #458
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors local daemon turn error handling by extracting fatal, cancelled, and generic error flows from ChangesLocal Daemon Turn Error Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/runs/lifecycle.py`:
- Around line 352-360: The handler handle_cancelled should set the session state
to "cancelled" before finalizing the run so the persisted session reflects the
cancellation; modify RuntimeTurnErrorSession handling in handle_cancelled to
assign session.turn_status = "cancelled" (after invoking
self.cancel_observation() if present) and then call
self.turn_run.finish_if_started(session, status="cancelled", result={},
error="cancelled") so the persisted state is correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c0b7e4f-9671-40b1-bd9c-e77501563ed8
📒 Files selected for processing (6)
coding-agent/.opencode/prompts/tasks/local-daemon-pr35-turn-error-handler.mdcoding-agent/docs/adr/0058-local-daemon-control-plane-executor-architecture.mdcoding-agent/src/coding_agent/runs/__init__.pycoding-agent/src/coding_agent/runs/lifecycle.pycoding-agent/src/coding_agent/server/session_manager.pycoding-agent/tests/coding_agent/test_run_lifecycle.py
Comment on lines
+352
to
+360
| async def handle_cancelled(self, session: RuntimeTurnErrorSession) -> None: | ||
| if self.cancel_observation is not None: | ||
| self.cancel_observation() | ||
| await self.turn_run.finish_if_started( | ||
| session, | ||
| status="cancelled", | ||
| result={}, | ||
| error="cancelled", | ||
| ) |
Contributor
There was a problem hiding this comment.
Set the session state to cancelled here.
handle_cancelled() updates the run record, but it never updates session.turn_status. After this refactor, SessionManager.run_agent() persists whatever state the handler leaves behind, so user-initiated cancels now stick at "cancelling" (and some other cancellation paths can fall back to "idle" in the finally block) instead of ending as "cancelled".
Proposed fix
async def handle_cancelled(self, session: RuntimeTurnErrorSession) -> None:
if self.cancel_observation is not None:
self.cancel_observation()
await self.turn_run.finish_if_started(
session,
status="cancelled",
result={},
error="cancelled",
)
+ session.turn_status = "cancelled"
+ session.last_failure_details = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@coding-agent/src/coding_agent/runs/lifecycle.py` around lines 352 - 360, The
handler handle_cancelled should set the session state to "cancelled" before
finalizing the run so the persisted session reflects the cancellation; modify
RuntimeTurnErrorSession handling in handle_cancelled to assign
session.turn_status = "cancelled" (after invoking self.cancel_observation() if
present) and then call self.turn_run.finish_if_started(session,
status="cancelled", result={}, error="cancelled") so the persisted state is
correct.
Owner
Author
|
Local review fallback because CodeRabbit remained pending without producing an actionable review. Reviewer: Epicurus (local subagent) Findings:
Review notes:
Additional verification from reviewer:
Residual risk:
Merge decision under fallback policy: OK to merge. |
2 tasks done
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
RuntimeTurnErrorHandlerto own local daemon fatal/cancelled/generic turn error actionsSessionManageras an injected callback sorunsdoes not depend on wire protocol typesrun_agenterror action bodies with lifecycle helper callsVerification
uv run pytest tests/coding_agent/test_run_lifecycle.py -v-> 14 passeduv run pytest tests/ui/test_session_manager_runtime.py -k "run_agent_marks_agent_run_failed_when_turn_outcome_errors or run_agent_records_error_outcome_without_runtime_store or run_agent_reraises_fatal_tool_execution_error_without_sending_error_turn or run_agent_persists_agent_run_lifecycle_when_store_configured" -v-> 4 passeduv run ruff check src/coding_agent/runs src/coding_agent/server/session_manager.py tests/coding_agent/test_run_lifecycle.py tests/ui/test_session_manager_runtime.py-> passeduv run ruff format --check src/coding_agent/runs/lifecycle.py src/coding_agent/runs/__init__.py src/coding_agent/server/session_manager.py tests/coding_agent/test_run_lifecycle.py-> passedgit diff --check-> cleanReview policy
If CodeRabbit is rate-limited, this PR requires local subagent P1/P2 review before merge. Do not merge on a rate-limited CodeRabbit placeholder alone.
Summary by CodeRabbit
Refactor
Tests