[api][plan][python] Expose sub-agents to chat models as callable tools - #1114
yunfengzhou-hub wants to merge 6 commits into
Conversation
680cf2d to
574795c
Compare
alnzng
left a comment
There was a problem hiding this comment.
Thanks for making the changes.
| # Tools are forbidden to register under this prefix, so a prefixed callable | ||
| # name unambiguously addresses a sub-agent and the executing side routes it to | ||
| # the AGENT namespace. | ||
| CALLABLE_NAME_PREFIX = "subagent_" |
There was a problem hiding this comment.
if this is intended to be internal only, I would prefer using "subagent" as prefix to keep consistent with existing patterns, e.g. _TOOL_CALL_CONTEXT in chat_model_action.py
There was a problem hiding this comment.
Done — renamed to _subagent_ in both languages.
| String inputSchema = setup.getInputSchema(); | ||
| if (inputSchema == null) { | ||
| // Unlike a bridge handle this is a sub-agent the caller could have described, so | ||
| // it is dropped with a warning rather than failing the job: the rest of the |
There was a problem hiding this comment.
I wonder why we decided to drop sub-agent silently, not fail the job directly.
There was a problem hiding this comment.
It was supposed that we may have some built-in sub-agents that may not be exposed to LLM, which means they do not need schemas. But this assumption is not that solid so I agree to reject this corner case for now.
| List<ToolCallExecution> toolExecutions = new ArrayList<>(); | ||
| for (ToolCallExecution execution : executions) { | ||
| if (execution.agent != null) { | ||
| dispatchAgentExecution(execution, ctx, success, error, responses); |
There was a problem hiding this comment.
It looks like all the sub-agent tool calls not run parallel, this is different with the regular tool call execution, right? any reason behind this?
There was a problem hiding this comment.
Thanks for the reminder. This code was developed before the batch tool-call feature, along with #938, so we missed this feature. I've added support for parallel tool calls.
574795c to
85c5bf8
Compare
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for taking this on. A few questions inline.
| raise TypeError(msg) | ||
|
|
||
|
|
||
| def _check_tool_name_not_reserved(name: str) -> None: |
There was a problem hiding this comment.
This guard runs at :341 for the decorator and :386 for add_resource, but I don't see it on the MCP discovery loop at :486-492, which registers each tool under the name the remote server chose (name=tool.name).
Java looks covered. MCP discovery there goes through addResourceProvider, which calls checkToolNameNotReserved (AgentPlan.java:694). I walked the other Python TOOL paths, and YAML funnels into add_resource, so this looked like the only gap. I may have missed an entry point though.
So a server advertising _subagent_lookup registers a TOOL, and dispatch then routes it to the AGENT namespace where it can never resolve. Contract 6 says a tool under the prefix is rejected at plan-construction, which holds when the name is ours to choose. Here it isn't.
Adding _check_tool_name_not_reserved(tool.name) to that loop would close it, but then one badly named remote tool fails the whole plan. Which would you prefer, failing at discovery, or skipping just that tool with a warning so the rest of the server still works?
There was a problem hiding this comment.
Fixed. the MCP discovery loop now runs _check_tool_name_not_reserved(tool.name) and raises, so a remote advertising _subagent_* fails at plan-construction. I went with failing at discovery rather than skipping with a warning.
| } | ||
|
|
||
| /** One request carrying two sub-agent calls, so the batched path has more than one to run. */ | ||
| private static ToolRequestEvent twoSubagentRequest(String first, String second) { |
There was a problem hiding this comment.
Both tests that enable the batched path use this builder, so every call in them is a sub-agent and toolExecutions ends up empty. recordOutcome(toolExecutions.get(i), outcomes.get(i), ...) at ToolCallAction.java:251 therefore never runs against a non-empty list.
That pairing is what keeps each tool's result on its own call id once sub-agents are filtered out of the batch. If the two lists drift, one call's result lands on another call's id and both tests still pass.
A mixed call also seems like the common case, since a model with both tools and sub-agents available can call one of each in a turn.
Would a third parallel case, one _subagent_ plus one tool, checking each id gets its own result, be worth adding? withParallelToolCalls() and durableExecuteAllAsync are already in the fixture, so it looks mostly like a new request builder.
There was a problem hiding this comment.
Added — parallelDispatchKeepsToolAndSubagentResultsOnTheirOwnIds runs one _subagent_ call alongside two tools under withParallelToolCalls(), and asserts call-1/2/3 each land on their own result ("agent-result", "alpha called", "beta called").
|
|
||
| private static JsonNode render(Class<?> type) { | ||
| try { | ||
| return MAPPER.generateJsonSchema(type).getSchemaNode(); |
There was a problem hiding this comment.
I ran the pinned jackson-databind 2.18.2 (pom.xml:48) and pydantic 2.11.4 over the Review type this PR mirrors on both sides.
Java:
{"type":"object","properties":{"path":{"type":"string"},"lines":{"type":"integer"}}}
Python:
{"properties":{"path":{"title":"Path","type":"string"},"lines":{"default":0,"title":"Lines","type":"integer"}},"required":["path"],"title":"Review","type":"object"}
The two sides express required differently. Jackson's legacy generator writes it per property, and only under @JsonProperty(required = true). Nothing on this path sets that, so Java marks no field required at all. Pydantic writes the object-level "required":["path"]. Same declaration, two different messages to the model about which arguments it must send.
The mirror tests check type plus the two property types (SubagentSetupTest.java:156-166, test_subagent.py:98-104), which is exactly where the two outputs agree. So contract 12 passes while the schemas differ on the field that changes what the model sends.
Is that deliberate? If not, would you rather fill required from the non-defaulted properties on the Java side, or have both mirror tests assert the whole schema so the next drift fails a build?
nit: from the same run, a byte[] field renders in Java as {"type":"array","items":{"type":"byte"}}, and byte is not a JSON Schema type. Python gives {"type":"string","format":"binary"}.
There was a problem hiding this comment.
Updated. I fixed the behavior in Java so that it now returns a json schema with the same required property like Python. It also renders byte[] as {"type":"string","format":"binary"} instead of an array of the non-standard byte type. Both mirror tests now assert required too, so a future drift fails the build.
| // tool-error response and driving a further chat call off it. | ||
| Thread.currentThread().interrupt(); | ||
| throw e; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Both sub-agent dispatch paths catch InterruptedException then Exception, but neither has a catch (Error e). The tool paths do, at :264 and :327.
ToolResultUtils.requireJsonCompatible walks Map, List and arrays with no cycle guard and no depth limit (ToolResultUtils.java:120-143), so a result that contains itself recurses until the stack runs out. That StackOverflowError is an Error, so it passes these catches and fails the job. Python's same recursion raises RecursionError, which is an Exception, so dispatch absorbs it into a ToolResponse error. Contract 9 says an inexpressible result is reported as a failed delegation, not made a job failure.
InputSchemas.render already carries a catch (StackOverflowError) for this same shape (InputSchemas.java:74-87).
I can't say how likely a cyclic result is in practice. Neither suite has a cycle or a deep-nesting case though, and the body calls result normalization the highest-risk area. Would the same clause here work, or would you rather thread an identity set through requireJsonCompatible?
There was a problem hiding this comment.
I've added both guarantees. One the one hand, ToolResultUtils.requireJsonCompatible checks for pontential loops and fail early in that case. On the other hand, catch StackOverflowError is also added to invoker's try-catch blocked.
| * to register under this prefix, so a prefixed callable name unambiguously addresses a | ||
| * sub-agent and the executing side routes it to the {@code AGENT} namespace. | ||
| */ | ||
| public static final String CALLABLE_NAME_PREFIX = "_subagent_"; |
There was a problem hiding this comment.
nit: the prefix is _subagent_ here, but the description still says subagent_ throughout, including the runtime-flow paragraph and contract 1.
Two behavior claims also read the other way now. Contract 2 says a sub-agent with no usable schema "is not offered, and does not stop the others", and the table row says "dropped with a warning; job continues". BaseChatModelSetup.java:149 fails open() through checkState instead, and chat_model.py:451-457 raises to match.
Worth a pass over the description before merge? It is the part people read later, and the contract list is detailed enough that the stale rows stand out.
There was a problem hiding this comment.
Done. The description has been updated to match the latest code behavior.
A sub-agent setup now carries the routing information a caller needs to expose it as a model callable: a description and a JSON Schema for its arguments, defaulting to a single required prompt field and rejecting a blank schema at construction. Both sides also define the reserved subagent_ callable-name prefix, under which a sub-agent is exposed to a chat model and which tools are forbidden to register under. Co-Authored-By: Qoder <noreply@qoder.com> AI-Contributed/Feature: 87/87 AI-Contributed/UT: 0/160 Generated-by: Qoder 1.29.0 (Qwen3.8-Max)
…lables A chat model setup takes the sub-agents it may delegate to under the subagents argument and derives one callable per sub-agent, named under the reserved subagent_ prefix, with a description marking it as a sub-agent; no separate listing message is injected. Tool registration rejects the reserved prefix at plan-construction time, so a tool and a sub-agent may share a resource name and each callable name resolves to exactly one namespace. AI-Contributed/Feature: 0/343 AI-Contributed/UT: 0/667 Generated-by: Qoder 1.29.0 (Qwen3.8-Max)
…re reason exposed Tool-call dispatch resolves a callable name under the reserved subagent_ prefix in the AGENT namespace and any other name in the TOOL namespace, so the two kinds never collide. A sub-agent call hands the model arguments to the setup unchanged, normalizes its result into the tool-response content, and reports a failed delegation back to the model with the reason, so the model can correct the call instead of repeating it blindly. AI-Contributed/Feature: 0/583 AI-Contributed/UT: 0/903 Generated-by: Qoder 1.29.0 (Qwen3.8-Max)
…nd dispatch Sub-agent callables are named under the reserved _subagent_ prefix, whose leading underscore marks an internal namespace that tool names cannot take. A sub-agent that declares neither an input schema nor an input type is rejected when a chat model sets up rather than dropped, so a declaration with no arguments for the model to build a call from surfaces immediately. In the batched tool-call path sub-agent calls are dispatched concurrently: every call is submitted before any is awaited, so the runs overlap like the batched tools instead of proceeding one by one. Generated-by: Qoder 1.29.0 (Qwen3.8-Max) AI-Contributed/Feature: 0/190 AI-Contributed/UT: 0/366
A sub-agent call whose durable execution is cancelled surfaces an InterruptedException from submit()/await(). Both dispatch paths now propagate it the way the tool paths do since apache#1111, so processToolRequest skips sendEvent and no further chat call is driven off a cancelled delegation. The concurrent path additionally cancels every handle submitted but no longer awaited, so a mid-batch cancellation leaves no in-flight remote run dangling. Generated-by: Qoder 1.29.0 (Qwen3.8-Max) Co-Authored-By: Qoder <noreply@qoder.com> AI-Contributed/Feature: 38/38 AI-Contributed/UT: 116/116
…and MCP naming
Result normalization:
- Refuse a cyclic sub-agent result through an identity-based set of the
containers on the current path, so a cycle is reported as an
IllegalArgumentException before the walk can overflow the stack. A
StackOverflowError is an Error that escapes catch(Exception) and would fail
the job, so both the serial and the batched dispatch paths also fold a
residual overflow from a result too deep to walk into a failed delegation.
- Depth on its own is not a reason to refuse a result: a deep but finite tree
still has a JSON form and the stack is the real bound, so no fixed nesting
cap is imposed. A guard test pins that a 200-deep result still normalizes.
Cross-language input schema:
- Render byte[] as {"type":"string","format":"binary"}, the standard JSON Schema
form and what pydantic gives a bytes field, and fill in object-level required
to match; mirror tests on both sides pin the contract.
- Treat a non-object schema as the programming error it is and let the cast fail
fast rather than passing it through silently.
MCP tool naming (Python):
- Run the reserved-prefix check on the MCP discovery loop too, matching Java's
centralized checkToolNameNotReserved: a remote tool named _subagent_* would
land in the TOOL map while dispatch routes any prefixed call to AGENT, so it
could never be called.
Co-Authored-By: Qoder <noreply@qoder.com>
Generated-by: Qoder (Qoder Auto)
AI-Model: Qoder Auto
AI-Contributed/Feature: 194/194
AI-Contributed/UT: 387/387
118d8d6 to
6660f59
Compare
alnzng
left a comment
There was a problem hiding this comment.
LGTM.
Let's please add end-to-end example and docs to explain the usage of sub-agents.
Linked issue: #1112
Purpose of change
A chat model can now delegate to a sub-agent by issuing a tool call. The setup declares each sub-agent it names as one callable under the reserved
_subagent_prefix; at execution a prefixed call is routed to that sub-agent and its result is handed back to the model. Which sub-agent to call, and with what arguments, becomes the model's decision — previously a sub-agent could only be invoked from action code.A sub-agent the caller names but gives no usable input shape is rejected when the model sets up, and a delegation that fails tells the model why, so it can correct the call rather than repeat it blindly.
Runtime flow
open()walks thesubagentsargument after the tools and, for each name resolving to aSubagentSetupwith a usable input schema, adds a metadata-onlySubagentToolnamed_subagent_<name>. The model builds a function call against that schema.ToolCallActionreads the prefix off the callable name, resolves the AGENT resource once, and passes the model's arguments tosubmit(...).await()unchanged; the result is normalized to JSON-generic form and rendered as the tool-message content. In the batched tool-call path sub-agent calls are dispatched concurrently — every call is submitted before any is awaited, so the runs overlap like the batched tools instead of proceeding one by one. Python mirrors this in its setup and tool-call action.Key decisions
The reserved prefix, not a distinct tool type, separates the namespaces: tool registration rejects
_subagent_at plan-construction, so a prefixed name can only address a sub-agent, and a tool and a sub-agent may share a resource name. No listing message is injected — every sub-agent description ends with a marker, so the model learns a callable is a delegation from its description alone.SubagentToolis metadata-only (call()throws); dispatch resolves the AGENT resource at execution, keeping the sub-agent's own durable execution as the single source of the result and avoiding nested durable cursors. An input schema is derived from the declared input type through the same Jackson generator the ReAct output schema uses, then aligned to the cross-language form (object-levelrequired,byte[]as string/binary), adding no dependency.Behavioral Semantics
Interaction decisions
At dispatch a
_subagent_name resolves in the AGENT namespace and a plain name in TOOL; a success returns normalized JSON content, and any failure returns an error carrying the reason.Behavioral contracts
_subagent_<name>, after the tool callables, carrying its description and schema.open()fails throughcheckState— rather than dropped, so a declaration with no arguments for the model to build a call from surfaces immediately.Failure behavior
Construction and plan-building raise; call-time failures are absorbed into the tool response.
Tests
The new suites run offline, with no live model or external agent. Java: BaseChatModelSetupSubagentTest, SubagentSetupTest, AgentPlanSubagentResourceTest, ToolResultUtilsTest, ToolCallActionSubagentTest; Python: the mirrored test_chat_model_subagents, test_subagent, test_agent_plan, test_tool_result_utils, test_tool_call_action_subagent. Every contract is pinned on both sides:
Highest risk is result normalization (9): non-string map keys, non-finite numbers, a POJO inside a JSON tree, arrays walked by index, a cyclic result refused at its path, and a 200-deep acyclic result still normalized, each asserted on the path in the message where one applies. The derived input schema (12) is pinned whole on both sides — object-level
requiredand each property's type/format, including abyte[]/bytesfield rendered as string/binary — so a divergence fails a build rather than passing contract 12 while the schemas differ.Not verified: no test drives a live model or a real external sub-agent, so these pin what the framework declares, dispatches, and reports, never that a provider accepts the derived schema. A sub-agent owned by the other language is checked only up to the rejection at open(); cross-language delegation end-to-end is out of scope. Parallel-mode routing of a sub-agent beside tools, and each call id landing on its own result under the batch, are covered; concurrency timing is not.
Implementation invariants (not caller-observable)
API
Additive, aligned across Java / Python / YAML, building on the AGENT resource type and SubagentSetup:
_subagent_).One thing changes for a caller who does nothing differently: a tool named
_subagent_*is now rejected at plan-construction, including a name an MCP server advertises. Otherwise nothing changes unless the new subagents argument is set, and a plain tool call routes as before. A sub-agent owned by the other language cannot yet be exposed as a callable — declaring one fails open().Documentation
doc-needed but deferred, matching the AGENT-resource change: document the subagents chat-model argument and the sub-agent-as-tool flow once the internal sub-agent lands and the API stabilizes.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Qoder 1.29.0 (Qwen3.8-Max)