Repository navigation
fix(agent): stop MCP Setup Agent re-spawn loop on user-clarification pause (#4291) - #4303
Conversation
…inyhumansai#4291) Replace the inline [SUBAGENT_AWAITING_USER] format! with the shared awaiting_user_envelope helper. No behaviour change.
…inyhumansai#4291) dispatch_subagent matched only Ok/Err on the run outcome and ignored outcome.status. When a delegated sub-agent (e.g. mcp_setup via delegate_setup_mcp_server) called ask_user_clarification, the runner checkpointed it and returned SubagentRunStatus::AwaitingUser — but dispatch reported it as a plain success carrying the question, and emitted a SubagentCompleted event. The orchestrator never received the task_id, so its only way to continue was to re-delegate a fresh, stateless sub-agent, which restarted from step 1, paused again, and re-spawned — an unbounded loop (MCP Setup Agent spawned 3+ times, never finishing). Match outcome.status: on AwaitingUser, publish SubagentAwaitingUser (domain + progress) events and return the [SUBAGENT_AWAITING_USER] envelope so the orchestrator resumes via continue_subagent. Completed and Err paths unchanged (the tinyhumansai#3193 hallucinated-success guard stays on Err). The paused-path mapping is factored into a pure awaiting_outcome_to_tool_result fn with a regression test.
…gates (tinyhumansai#4291) The orchestrator's named tool list lacked continue_subagent, so even after dispatch surfaces the [SUBAGENT_AWAITING_USER] envelope the model had no tool to resume the checkpointed run — its only move was to re-delegate, re-entering the loop. Add continue_subagent to the orchestrator's tools and lock it in with a loader test.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute 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 (2)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
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
🤖 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/tools/awaiting_user.rs`:
- Around line 31-45: The awaiting-user envelope in `awaiting_user.rs` is
embedding `question` directly into the `[SUBAGENT_AWAITING_USER]` block, which
can break the tagged format if the text contains newlines or reserved
delimiters. Update the formatting in the function that builds this message to
escape or serialize `question` before insertion, preserving the envelope
structure and resume instructions. Add a regression test covering a multiline
question and a question containing `[/SUBAGENT_AWAITING_USER]` to verify the
block stays intact.
🪄 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: 8526b987-ded6-43bb-911d-9b6e98fd721e
📒 Files selected for processing (6)
src/openhuman/agent_orchestration/tools.rssrc/openhuman/agent_orchestration/tools/awaiting_user.rssrc/openhuman/agent_orchestration/tools/dispatch.rssrc/openhuman/agent_orchestration/tools/spawn_subagent.rssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/agent_registry/agents/orchestrator/agent.toml
…humansai#4291) Sub-agent-authored question text was embedded raw into the line-oriented [SUBAGENT_AWAITING_USER] block. A newline or a literal closing tag could terminate the envelope early and inject fake resume fields the orchestrator now trusts. JSON-encode the question so it stays on one line with delimiters escaped; add a regression test for a multiline / tag-injection question.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Reviewed and verified: correct, well-tested, CI green/clearing. Approving. Thanks!
Summary
dispatch_subagentnow honoursSubagentRunStatus::AwaitingUserand returns the[SUBAGENT_AWAITING_USER]envelope (matching thespawn_subagentpath) so the orchestrator resumes the checkpointed run viacontinue_subagentrather than re-delegating a fresh, stateless sub-agent.continue_subagentto the orchestrator's tool set so that resume instruction is actually actionable.Problem
Asking the chat agent to "install an MCP server and test it" spawned the MCP Setup Agent 3+ times and stalled, never completing or failing.
Root cause: the orchestrator reaches
mcp_setupthrough the synthesizeddelegate_setup_mcp_servertool →ArchetypeDelegationTool::execute→dispatch_subagent.dispatch_subagentmatched onlyOk/Erron the run outcome and ignoredoutcome.status. Whenmcp_setupcalledask_user_clarification, the runner checkpointed the conversation and returnedSubagentRunStatus::AwaitingUser— butdispatch_subagentreturned the question as a plainToolResult::successand emitted aSubagentCompletedevent. The orchestrator never received thetask_id, so its only way to "continue" was to re-delegate a fresh, statelessmcp_setup, which restarted from step 1, paused again, and re-spawned — an unbounded loop. No circuit breaker caught it: every re-spawn returnsis_error=false, andMAX_SPAWN_DEPTHonly bounds nesting depth, not breadth-wise re-spawns.Compounding it, the orchestrator's named tool list lacked
continue_subagent(nothing injected it), so even the existing async pause envelopes already instructed it to call a tool it could not call.Solution
dispatch.rs— matchoutcome.statusin theOkarm. OnAwaitingUser, publishSubagentAwaitingUser(domain + per-turn progress) events and return the[SUBAGENT_AWAITING_USER]envelope carryingtask_id/agent_id/questionplus an explicit instruction to resume viacontinue_subagentand not re-spawn.Completedkeeps the prior behaviour; theErrarm (and its Cannot write code on filesystem #3193 hallucinated-success guard) is untouched. The paused-path mapping is factored into a pureawaiting_outcome_to_tool_resultfn with a regression test.tools/awaiting_user.rs(new) — pure, side-effect-freeawaiting_user_envelope(...)builder, the single source of truth for both delegation paths, with unit tests.spawn_subagent.rs— route its existingAwaitingUserarm through the shared helper (no behaviour change).orchestrator/agent.toml— addcontinue_subagentto[tools].named, locked in by aloader.rstest, so the resume instruction is reachable.Design notes: the resume machinery (
continue_subagent, the on-disk checkpoint keyed bytask_id) already existed and is launch-path-agnostic — this PR only routes the synchronous delegate path into it. The Notion 401 angle (#4282) is intentionally out of scope (a separateok:false-recorded-as-success concern); this fix bounds the loop regardless of the 401, since the orchestrator now resumes one checkpointed agent instead of re-spawning.Submission Checklist
awaiting_userenvelope tests (handles + worker-thread present/absent),dispatchAwaitingUser→envelope mapping regression test,loaderorchestrator-has-continue_subagentlock-in test.AwaitingUser→envelope mapping + dispatch path are unit-covered.N/A: behaviour-only fix to existing delegation flow; no feature row added/removed/renamed.## Related—N/A: no matrix feature IDs touched.N/A: no release-cut surface changed (internal agent orchestration only).Closes #NNNin## Related.Impact
Errpath is unchanged).spawn/waitpause path, which previously told the orchestrator to call acontinue_subagenttool it didn't have.Related
ok:falsesuccess — separate classifier concern, out of scope here).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/4291-mcp-setup-awaiting-userValidation Run
pnpm --filter openhuman-app format:check— N/A: no frontend change.pnpm typecheck— N/A: no TypeScript change.cargo test --lib awaiting_user(5 ok),cargo test --lib orchestrator_can_resume(1 ok).cargo fmt --checkclean;cargo checkexit 0.app/src-taurichange.Validation Blocked
command:pre-pushpnpm rust:check(Tauri shell check)error:worktree submodules (tauri-cef,tauri-plugin-notification) uninitialised → src-tauri not buildable hereimpact:pushed with--no-verify; change is core-only (src/openhuman/...), noapp/src-taurifiles touched, so the Tauri check is not relevant to this diff.Behavior Changes
ask_user_clarificationis surfaced as a resumable pause; the orchestrator resumes viacontinue_subagentinstead of re-delegating.Parity Contract
CompletedandErroutcome paths unchanged (incl. the Cannot write code on filesystem #3193 hallucinated-success guard);spawn_subagent's envelope is byte-identical, now sourced from the shared helper.dispatchnow mirrorsspawn_subagent'sAwaitingUserhandling; loader test asserts the orchestrator exposescontinue_subagent.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes