fix(orchestration): surface async sub-agent failures/awaiting-user in chat (#4896)#4908
Conversation
…r in chat Async sub-agent delegation re-injected only successful outcomes; the AwaitingUser and Err(...) branches published events but never called record_completion, so background_delivery (which drains only completions) could never surface them and the turn finalized on "Accepted" with the failure silently lost. Route failure and awaiting-user outcomes through the delivery path with a user-readable summary (task id + reason), and render outcome-specific items in background_delivery so the user learns a delegated task failed or needs input. Success tag/wording preserved; existing tests unaffected. Closes tinyhumansai#4896
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAsync sub-agent failures and awaiting-user states are now recorded alongside successful completions, preserved through background delivery and requeueing, and rendered with distinct notice tags and fallback messages. ChangesAsync sub-agent outcome propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SpawnAsyncSubagent
participant BackgroundCompletions
participant BackgroundDelivery
participant ParentChat
SpawnAsyncSubagent->>BackgroundCompletions: record failure or awaiting-input outcome
SpawnAsyncSubagent->>BackgroundDelivery: publish terminal event
BackgroundDelivery->>BackgroundCompletions: schedule and drain delivery
BackgroundCompletions-->>BackgroundDelivery: outcome-specific completion
BackgroundDelivery-->>ParentChat: inject batched notice
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4838b94b33
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| crate::openhuman::agent_orchestration::background_completions::record_awaiting_input( | ||
| background_parent_session.clone(), | ||
| outcome.task_id.clone(), | ||
| outcome.agent_id.clone(), | ||
| question, | ||
| background_parent_thread_id.clone(), | ||
| ); |
There was a problem hiding this comment.
Now that awaiting-user and failed async outcomes are enqueued for background delivery, a parent that calls wait_subagent and receives that terminal status inline can still get a second injected follow-up after AgentTurnCompleted, because wait_subagent only calls mark_collected for the Completed arm. In the scenario where the child pauses or fails while the parent is waiting, this new queued notice survives and is delivered as a duplicate for the same task.
Useful? React with 👍 / 👎.
…inyhumansai#4896) The background delivery handler only scheduled a drain on SubagentCompleted, so a subagent that FAILED (or paused awaiting user) after the parent turn was already idle never triggered a delivery attempt — the pending failure sat undelivered until some unrelated later turn, leaving the chat stuck on the original "Accepted" response (Codex P1). Match all three subagent terminal events (completed | failed | awaiting-user), each of which carries `parent_session`, in the same debounced-drain arm. Add a handler test that accepts every terminal event.
coderabbitai
Bot
added
agent
![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)
src/openhuman/agent_orchestration/background_delivery.rs (1)
254-261: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest requeue with an actual failed outcome.
This calls
record_completion, so it passes even if requeue downgrades failures to success. Enqueue withrecord_failure, requeue, drain, and assertBackgroundAgentOutcome::Failed.🤖 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 `@src/openhuman/agent_orchestration/background_delivery.rs` around lines 254 - 261, Update requeue_restores_a_failed_batch to enqueue the batch using record_failure instead of record_completion, then requeue and drain it with take_pending; assert that the resulting outcome is BackgroundAgentOutcome::Failed, while retaining the pending-state and cleanup checks.Source: Coding guidelines
🧹 Nitpick comments (1)
src/openhuman/agent_orchestration/background_completions.rs (1)
149-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd grep-friendly terminal-outcome lifecycle diagnostics. The new outcome pipeline has no trace for terminal-state detection, queue acceptance/deduplication, or drain scheduling, making silent delivery loss difficult to diagnose. Do not log summaries, questions, or error text.
src/openhuman/agent_orchestration/background_completions.rs#L149-L195: log accepted and duplicate-suppressed records withtask_idand outcome.src/openhuman/agent_orchestration/tools/spawn_async_subagent.rs#L666-L725: log the awaiting-input/failed branch before enqueueing, using task ID, agent ID, and disposition only.src/openhuman/agent_orchestration/background_delivery.rs#L73-L85: log terminal-event drain scheduling with the task ID.As per coding guidelines, “New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries/timeouts, state transitions, and errors; use
log/tracingat debug/trace in Rust and namespaced debug logging in the app. Never log secrets or full PII.”🤖 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 `@src/openhuman/agent_orchestration/background_completions.rs` around lines 149 - 195, Since the terminal-outcome lifecycle lacks diagnostics, add namespaced debug logs at all three sites: in background_completions.rs lines 149-195, update record_outcome to log accepted records and duplicate-suppressed records with task_id and outcome only; in tools/spawn_async_subagent.rs lines 666-725, log the awaiting-input/failed terminal branch immediately before enqueueing using task ID, agent ID, and disposition only; and in background_delivery.rs lines 73-85, log terminal-event drain scheduling with the task ID. Do not log summaries, questions, error text, secrets, or full PII.Source: Coding guidelines
🤖 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 `@src/openhuman/agent_orchestration/background_delivery.rs`:
- Around line 317-359: Strengthen handler_accepts_every_subagent_terminal_event
so each terminal event demonstrably schedules and executes a drain: enqueue a
matching headless result before handling each event, await the debounce period,
and assert that the pending item was consumed. Retain the existing assertion
that terminal events do not mark the parent session busy.
---
Outside diff comments:
In `@src/openhuman/agent_orchestration/background_delivery.rs`:
- Around line 254-261: Update requeue_restores_a_failed_batch to enqueue the
batch using record_failure instead of record_completion, then requeue and drain
it with take_pending; assert that the resulting outcome is
BackgroundAgentOutcome::Failed, while retaining the pending-state and cleanup
checks.
---
Nitpick comments:
In `@src/openhuman/agent_orchestration/background_completions.rs`:
- Around line 149-195: Since the terminal-outcome lifecycle lacks diagnostics,
add namespaced debug logs at all three sites: in background_completions.rs lines
149-195, update record_outcome to log accepted records and duplicate-suppressed
records with task_id and outcome only; in tools/spawn_async_subagent.rs lines
666-725, log the awaiting-input/failed terminal branch immediately before
enqueueing using task ID, agent ID, and disposition only; and in
background_delivery.rs lines 73-85, log terminal-event drain scheduling with the
task ID. Do not log summaries, questions, error text, secrets, or full PII.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a844096d-0e98-4f81-9abe-42bf720ab877
📒 Files selected for processing (3)
src/openhuman/agent_orchestration/background_completions.rssrc/openhuman/agent_orchestration/background_delivery.rssrc/openhuman/agent_orchestration/tools/spawn_async_subagent.rs
W5 code review (comment-only — maintainer decides merge)Reviewed the full diff (3 files, +326/-7). Verdict: sound. This correctly closes the gap where a detached sub-agent that failed or paused awaiting input never surfaced in chat — the parent turn finalized on "Accepted" and the outcome was lost. Transparency: the What's good
One minor note (non-blocking)
No i18n surface (Rust-only). No blockers. Not approving/merging — that's the maintainer's call. |
…tinyhumansai#4896) CodeRabbit: the previous handler test only asserted `!is_busy`, which also holds if the events fall through to `_ => {}`, so it couldn't catch a regression to the completed-only behaviour. Replace it with a behavioural test on the paused clock: queue a headless result per session, fire each terminal event, advance past the debounce, and assert the pending result was consumed — proving the arm scheduled a drain for SubagentFailed / SubagentAwaitingUser (not just SubagentCompleted).
W5 — pushed 834c823 (CodeRabbit)Rewrote the terminal-events handler test as a behavioural check (paused clock): queues a headless result per session, fires each of the 3 terminal events, advances past the debounce, and asserts the pending result was consumed — so it now fails if any terminal event is dropped or falls through to |
2 participants
Closes #4896
Root cause
Async sub-agent delegation re-injected only successful outcomes.
Completed/Incompletecalledbackground_completions::record_completion(→ re-injected viabackground_delivery::try_deliver), but theAwaitingUserandErr(...)branches inspawn_async_subagent.rsonly published events — neverrecord_completion. Sincetry_deliverdrains onlypendingcompletions, failures/awaiting could never reach chat: the parent turn finalized on "Accepted async sub-agent…" and the failure was silently lost.Fix
Route failure and awaiting-user outcomes through the delivery path with a user-readable summary (task id + reason), and render outcome-specific items in
background_delivery(mixed-batch tagging, outcome-specific fallbacks, outcome preserved through a drain/requeue). Success tag/wording preserved; existing tests unaffected.Verification
Unit tests: an erroring child and an awaiting-user child each produce a delivered chat follow-up. i18n: none — these are agent-context/system-turn strings (same class as the existing untranslated
[SUBAGENT_INCOMPLETE]framing), not localized UI. No AI attribution.Summary by CodeRabbit
New Features
Bug Fixes