Repository navigation
fix(subagent): harden child failure routing - #106
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a FatalToolExecutionError type, makes session ownership conflicts fatal, updates parallel executor and toolset flows to re-raise fatal errors, and hardens subagent dispatch/cleanup to emit deterministic, model-visible summaries for interrupts, run exceptions, and adapter cleanup failures; tests and ADR docs are updated accordingly. ChangesFatal Error Propagation & Subagent Hardening
Sequence DiagramsequenceDiagram
participant Client as Client/Caller
participant Toolset as Toolset
participant Parallel as ParallelExecutor
participant Subagent as Subagent/Tool
participant Parent as Parent Session / Publisher
Note over Client,Parent: FatalToolExecutionError propagation & deterministic subagent summaries
Client->>Toolset: execute_tools(tool_calls)
Toolset->>Parallel: dispatch batch
Parallel->>Subagent: run tool (concurrent)
alt Fatal (SessionOwnershipConflictError)
Subagent-->>Parallel: raise FatalToolExecutionError
Parallel-->>Toolset: re-raise fatal (taskgroup propagates)
Toolset-->>Client: exception escapes (no wrapped result)
else Non-fatal run error
Subagent-->>Parallel: raises Exception
Parallel-->>Parallel: capture -> json error result
Parallel-->>Toolset: return error result
Toolset-->>Client: wrapped error result
end
Note over Subagent,Parent: Subagent outcome publishing
Subagent->>Subagent: run_turn / cleanup
alt Interrupted or Run Exception
Subagent->>Parent: publish deterministic "interrupted" / "failed" summary (model-visible)
Subagent->>Parent: append hidden child tape entry (meta.skip_context/meta.subagent_child)
else Cleanup Failure
Subagent->>Parent: publish composed cleanup error summary (includes timeout context if any)
Subagent->>Parent: append hidden child tape entry
end
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)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/src/coding_agent/plugins/parallel_executor.py (1)
128-158:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
asyncio.gatherdoes not cancel sibling tasks whenFatalToolExecutionErrorpropagates — other tools continue executing after an ownership conflict.When
return_exceptions=False(the default), the first raised exception is immediately propagated to the awaiting caller, but other awaitables in the sequence won't be cancelled and will continue to run.This means when one
_run_onecoroutine raisesSessionOwnershipConflictError, theexecute_batchcall sees the exception immediately, but the other coroutines in the same batch keep executing as background tasks. SinceDependencyAnalyzerallows twofile_writeoperations on different files in the same batch, a write-to-b.pycan commit its side effects even after an ownership conflict was detected on thea.pywrite.This contradicts the PR's stated "fail-fast" objective.
asyncio.TaskGroup(Python 3.11+) is the recommended alternative and provides stronger safety guarantees: if any task raises,TaskGroupcancels the remaining scheduled tasks.♻️ Proposed fix using `asyncio.TaskGroup`
- batch_results = await asyncio.gather( - *[_run_one(idx, call) for idx, call in batch] - ) - for idx, result in batch_results: - results_by_index[idx] = result + task_items = list(enumerate([_run_one(idx, call) for idx, call in batch])) + async with asyncio.TaskGroup() as tg: + tasks = [tg.create_task(coro) for _, coro in task_items] + for task, (_, _) in zip(tasks, task_items): + idx, result = task.result() + results_by_index[idx] = resultNote:
TaskGroupraisesExceptionGroupon failure. If the callsite needs to unwrapFatalToolExecutionErrorfrom it, add anexcept*handler or unwrap inexecute_batch.If
TaskGroupis not available (Python < 3.11), an alternative is to create explicitTaskobjects and cancel them onFatalToolExecutionError:tasks = [asyncio.ensure_future(_run_one(idx, call)) for idx, call in batch] try: batch_results = await asyncio.gather(*tasks) except FatalToolExecutionError: for t in tasks: t.cancel() raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/plugins/parallel_executor.py` around lines 128 - 158, The current use of asyncio.gather inside the batch loop lets sibling _run_one tasks continue running when one raises FatalToolExecutionError; change execute_batch's batch execution so that sibling tasks are cancelled when a FatalToolExecutionError occurs: preferably replace asyncio.gather with an asyncio.TaskGroup (Python 3.11+) to let the group cancel remaining tasks on first failure (handle ExceptionGroup/FatalToolExecutionError as needed), or if TaskGroup isn't available, create explicit asyncio Tasks for each _run_one, await them with asyncio.gather inside a try/except that on FatalToolExecutionError iterates over the created tasks and calls cancel() before re-raising; ensure the semantics around _semaphore and results_by_index remain unchanged.
🧹 Nitpick comments (1)
coding-agent/tests/coding_agent/tools/test_subagent.py (1)
477-487: 💤 Low valueConsider tracking
close()calls to document expected cleanup behavior forINTERRUPTED.
RaisingAdapterin the adjacent run_exception test explicitly verifiesevents == ["adapter-close"]to assert thatclose()is called after a thrown exception.InterruptedAdapterhas noclose()at all, so the test is silent on whether close is expected to run (or be skipped) for theINTERRUPTEDpath — an outcome that the PR explicitly adds cleanup handling for. Adding a minimal tracking stub would either confirm parity with the exception path or explicitly document that interruption skips close.🔧 Suggested addition
+ events: list[str] = [] + class InterruptedAdapter: def __init__(self, **_kwargs: Any) -> None: pass async def run_turn(self, _goal: str) -> TurnOutcome: return TurnOutcome( stop_reason=StopReason.INTERRUPTED, error="Cancelled by user", ) + + async def close(self) -> None: + events.append("adapter-close")Then add an assertion at the end of the test documenting the expected behaviour, e.g.:
# Assert close IS called (or assert events == [] if INTERRUPTED skips close) assert events == ["adapter-close"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/coding_agent/tools/test_subagent.py` around lines 477 - 487, The InterruptedAdapter in the test lacks a close() implementation so the test doesn't assert whether adapter.close() is invoked for StopReason.INTERRUPTED; add a minimal close(self) async or sync method on InterruptedAdapter that appends e.g. "adapter-close" to the shared events list (same mechanism used by RaisingAdapter), keep run_turn returning StopReason.INTERRUPTED, and then add a final assertion (e.g. assert events == ["adapter-close"] or assert events == [] depending on the intended behavior) to explicitly document the expected cleanup behavior for InterruptedAdapter and ensure parity with the run_exception test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/coding_agent/tools/subagent.py`:
- Around line 314-332: When closing resources in the finally block, if await
child_consumer.close() raises asyncio.CancelledError the code currently
re-raises immediately and skips awaiting
_close_adapter_if_supported(child_adapter); change the logic to catch
asyncio.CancelledError from child_consumer.close(), store the exception (e.g.,
saved_cancel = exc or saved_cancel = True), continue to run the second
try/except that awaits _close_adapter_if_supported(child_adapter) and aggregate
any adapter_close_error into cleanup_error as currently done, then after both
cleanup attempts re-raise the original CancelledError so both consumer and
adapter close are attempted but cancellation is still propagated; reference
child_consumer.close(), _close_adapter_if_supported(child_adapter),
cleanup_error and adapter_close_error when implementing this control-flow
change.
---
Outside diff comments:
In `@coding-agent/src/coding_agent/plugins/parallel_executor.py`:
- Around line 128-158: The current use of asyncio.gather inside the batch loop
lets sibling _run_one tasks continue running when one raises
FatalToolExecutionError; change execute_batch's batch execution so that sibling
tasks are cancelled when a FatalToolExecutionError occurs: preferably replace
asyncio.gather with an asyncio.TaskGroup (Python 3.11+) to let the group cancel
remaining tasks on first failure (handle ExceptionGroup/FatalToolExecutionError
as needed), or if TaskGroup isn't available, create explicit asyncio Tasks for
each _run_one, await them with asyncio.gather inside a try/except that on
FatalToolExecutionError iterates over the created tasks and calls cancel()
before re-raising; ensure the semantics around _semaphore and results_by_index
remain unchanged.
---
Nitpick comments:
In `@coding-agent/tests/coding_agent/tools/test_subagent.py`:
- Around line 477-487: The InterruptedAdapter in the test lacks a close()
implementation so the test doesn't assert whether adapter.close() is invoked for
StopReason.INTERRUPTED; add a minimal close(self) async or sync method on
InterruptedAdapter that appends e.g. "adapter-close" to the shared events list
(same mechanism used by RaisingAdapter), keep run_turn returning
StopReason.INTERRUPTED, and then add a final assertion (e.g. assert events ==
["adapter-close"] or assert events == [] depending on the intended behavior) to
explicitly document the expected cleanup behavior for InterruptedAdapter and
ensure parity with the run_exception test.
🪄 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: 0c2abb41-cc8e-4308-a165-69c3fd2461f9
📒 Files selected for processing (9)
coding-agent/.opencode/prompts/tasks/adr-0018-pr2-cancellation-failure-summary-routing.mdcoding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.mdcoding-agent/src/agentkit/tools/__init__.pycoding-agent/src/agentkit/tools/toolset.pycoding-agent/src/coding_agent/plugins/parallel_executor.pycoding-agent/src/coding_agent/tools/subagent.pycoding-agent/src/coding_agent/ui/session_owner_store.pycoding-agent/tests/coding_agent/plugins/test_parallel_executor.pycoding-agent/tests/coding_agent/tools/test_subagent.py
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
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/tools/subagent.py`:
- Around line 310-313: The except block in Subagent.run_turn (inside
coding_agent/tools/subagent.py) is currently catching all Exceptions and
converting them into a TurnOutcome, which swallows fatal owner-fencing
conflicts; update the error handling so that SessionOwnershipConflictError is
explicitly re-raised (i.e., add an except SessionOwnershipConflictError: raise
before the generic except Exception) so the parent sees and fails fast, leaving
the existing TurnOutcome creation only for non-fencing exceptions.
🪄 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: 4c52d27d-d673-42e5-aea8-e6e31f7d8e3f
📒 Files selected for processing (4)
coding-agent/src/coding_agent/plugins/parallel_executor.pycoding-agent/src/coding_agent/tools/subagent.pycoding-agent/tests/coding_agent/plugins/test_parallel_executor.pycoding-agent/tests/coding_agent/tools/test_subagent.py
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary
Verification
uv run pytest tests/agentkit/tools/test_toolset.py tests/coding_agent/plugins/test_parallel_executor.py tests/coding_agent/tools/test_subagent.py tests/coding_agent/subagents/test_coordinator.py -vuv run pytest tests/coding_agent/test_bootstrap.py tests/ui/test_session_manager_public_api.py tests/ui/test_session_manager_owner_checks.py -k "subagent or cloud" -vuv run python -m compileall -q src tests/agentkit tests/coding_agent tests/uiReview
Summary by CodeRabbit
Bug Fixes
Tests