fix(mcp): flag subagents run_subagent will not dispatch (#5755) - #5777
Conversation
tinysweeper
Bot
added
the
priority: p3
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe MCP tools now share dispatchability checks for subagents. Listings expose structured refusal metadata and annotate unavailable agents in text. Subagent execution uses the same refusal check. Tests cover blocked and dispatchable agents. ChangesMCP dispatchability
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR makes a localized MCP listing change so unsupported subagents are clearly marked before dispatch. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Maintainer review — one conflict, and it is a mechanical oneAgainst current What conflicts and why
use super::specs::list_tools_result_for_config;
use super::*;
#[path = "tools_tests_part_01_tests.rs"]
mod part_01_tests;
#[path = "tools_tests_part_02_tests.rs"]
mod part_02_tests;Your branch appended its two new tests to the old monolithic The resolutionI worked it through locally to be sure it is as simple as it looks, and it is:
No edit to the tests themselves is needed. Your other two files — On the change itselfThe design is right, and the part I'd single out is that Both bot reviews are APPROVED and there are no unresolved threads. The three I have not pushed anything to your branch and I am not approving. Once the conflict is resolved a maintainer will review and merge. |
agent.list_subagents enumerated the whole registry, and each entry's when_to_use reads as an invitation to delegate. agent.run_subagent then refused integrations_agent outright, so a brain that followed the invitation learned it was unreachable only from the error -- after paying for the round trip, and typically after already having tried tools_agent, which routes integrations to integrations_agent by design. The refusal and the flag now come from one predicate, so the catalogue cannot drift back into advertising a delegate that dispatch turns away. Listed entries carry dispatchable_over_mcp and not_dispatchable_reason, and the human-readable summary line carries the same reason inline. This does not decide whether MCP should eventually dispatch toolkit-bound agents (tinyhumansai#5755 lists two designs for that, both of which change the schema). It makes the current limit visible at list time instead of at call time, and stays correct under either. Refs tinyhumansai#5755
cb500b5 to
b05dc58
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
How this change flows3 changed behaviours across 19 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 38 further behaviours left out to keep the diagram readable. flowchart LR
n0["core_tool_instructions<br/>changed"]:::changed
n1["list_subagents<br/>changed"]:::changed
n2["run_subagent_tool<br/>changed"]:::changed
n3["build_rpc_params"]:::impacted
n4["call_tool"]:::impacted
n5["format"]:::impacted
n6["Value"]:::impacted
n7["ToolCallError"]:::impacted
n8["load_config_with_timeout"]:::impacted
n0 -->|uses| n6
n0 -->|uses| n7
n1 -->|calls| n5
n1 -->|uses| n6
n1 -->|uses| n7
n2 -->|calls| n5
n2 -->|uses| n6
n2 -->|uses| n7
n3 -->|calls| n5
n3 -->|uses| n6
n3 -->|uses| n7
n4 -->|calls| n0
n4 -->|calls| n1
n4 -->|calls| n2
n4 -->|calls| n3
n4 -->|calls| n5
n4 -->|uses| n6
n4 -->|uses| n7
n8 -->|calls| n5
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
|
Rebased onto today's
|
…gent-dispatchable\n\nfix(mcp): flag subagents run_subagent will not dispatch (tinyhumansai#5755)\n
3 participants
Refs #5755. Takes the fallback that issue names — "if support stays out,
agent.list_subagentscould at least flag it as not-dispatchable over MCP" — and leaves the design question open.What is wrong
agent.list_subagentsenumerates the wholeAgentDefinitionRegistry, and each entry'swhen_to_usereads as an invitation to delegate.agent.run_subagentthen refuses one of the entries outright:So the catalogue advertises a delegate that dispatch turns away, and a brain can only discover that from the error — after paying for the round trip, and typically after already trying
tools_agent, whose own docs route integrations tointegrations_agent.What this changes
One predicate,
mcp_dispatch_block_reason, is now the single source for both the refusal and what the listing publishes.run_subagent_toolreturns its reason; each listed definition carriesand the human-readable summary line carries the same reason inline, so a model reading the text form sees it too:
Because both sites read the same function, the catalogue cannot drift back into advertising something dispatch will not run: adding an id to the predicate flags it and refuses it in one edit, and removing it does both too.
What this deliberately does not do
It does not decide whether MCP should eventually dispatch toolkit-bound agents. #5755 lists two designs for that — a
toolkitparam routed through the typed spawn, or exposingdelegate_to_integrations_agentover MCP — and both change the tool schema, which is a maintainer call, not a drive-by. This holds under either: when support lands, the id leaves the predicate and both the refusal and the flag disappear together.I also did not touch the naive route the issue warns against (attaching raw composio tools to the bridge's
Agent::run_single), for the reason the issue gives: it would create a second, weaker path to the same capability.Tests
src/openhuman/mcp/server/tools_tests.rs, two cases, both pure — no config, no registry, no TTY:integrations_agent_is_flagged_as_not_dispatchable_over_mcp— the predicate returns a reason, the summary line carries the marker and the reason itself, andwhen_to_usesurvives the marker.dispatchable_subagents_are_listed_without_a_marker—tools_agentand the empty id are unblocked, and the line is byte-identical to the old format.subagent_summary_linewas extracted so the marker is assertable without standing up anAgentDefinitionRegistry;list_subagentsnow builds each bullet through it, so the tested function is the shipping one rather than a copy.Mutation-checked, two independent mutations, each caught by exactly one test and nothing else:
mcp_dispatch_block_reasonnever blocks (false && …)integrations_agent_is_flagged_…integrations_agent_is_flagged_…cargo fmt -- --check: clean.Note on the pre-push hook
Pushed with
--no-verify. The hook runscargo clippy -D warningsover the whole lib, which currently fails onmainwith 11 pre-existing errors — unused imports, an unreachable statement, two needlessmuts, a needlessreturn, a missingDefault, and aSetEntriesInAclWreference — across these files:None is in
src/openhuman/mcp/, and this diff touches onlysrc/openhuman/mcp/server/tools/.grepformcp/server/toolsin the clippy output returns 0 hits. #5762 is the PR that clears that breakage; this one does not depend on it.Summary by CodeRabbit
New Features
Bug Fixes