feat(sdk): typed agent tool resolution contracts#4763
Conversation
dosubot
Bot
added
the
size:XXL
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Reviewer guide: interesting code
|
| tool_callback = None | ||
| if gateway_configs: | ||
| if self._gateway_resolver is None: | ||
| raise UnsupportedToolProviderError(gateway_configs[0].provider) |
There was a problem hiding this comment.
This is the seam the architecture decision hangs on: a gateway config has no offline resolution, so an absent injected resolver raises here. The server-only Composio adapter attaches at this port.
| TOOL_SPEC_ADAPTER: TypeAdapter[ToolSpec] = TypeAdapter(ToolSpec) | ||
|
|
||
|
|
||
| def coerce_tool_spec(value: Any) -> ToolSpec: |
There was a problem hiding this comment.
coerce_tool_spec infers the kind discriminator when a caller omits it (callRef to callback, code to code, else client). This is what lets old runner dicts without a kind still validate into the new union.
| class EnvironmentToolSecretProvider: | ||
| """Read declared tool secrets from the current process environment.""" | ||
|
|
||
| async def get_many(self, names: Sequence[str]) -> Mapping[str, str]: |
There was a problem hiding this comment.
The default secret provider reads os.environ, which is why a standalone resolve needs no platform. Swapping this Protocol for a vault-backed provider is how the server injects real secrets without changing the resolver.
| tools: List[str] = Field(default_factory=list) | ||
|
|
||
| @model_validator(mode="after") | ||
| def _validate_transport(self) -> "MCPServerConfig": |
There was a problem hiding this comment.
The only MCP rule with teeth: stdio requires a command, http requires a url. Worth confirming this matches what the runner bridge can actually launch.
Railway Preview Environment
Updated at 2026-06-19T16:30:11.537Z |
|
Superseded. Replacing the path-based stack with PRs sliced by functional area showing final code only, so reviewers don't comment on intermediate scaffolding that a later PR rewrites. See the new set. |
1 participant
This PR is part of a stack. Review bottom-up.
Each PR's diff is only its own delta. Merge from the bottom. This PR's base is #4761 (merge that first).
Context
The agent service used to pass tools around as loose
List[Any]and loose dicts. Nothing typed a tool, nothing validated it, and the resolution logic that turns a stored tool into something a runner can execute lived only in the service. This PR moves the canonical tool and MCP contracts into the SDK and adds an offline resolver that runs with no network access. It sits onfeat/agent-harness-portand lands slice #9 (tool runtime) fromdocs/design/agent-workflows/pr-stack.md.What this changes
Two new SDK subsystems appear under
sdks/python/agenta/sdk/agents/:tools/andmcp/. Each one ships strict Pydantic models, a strict parser, a resolver, typed errors, and a wire serializer.A tool now has two shapes. A
ToolConfigis what a user stores: a discriminated union overbuiltin,gateway,code, andclient. AToolSpecis what a runner receives after resolution: a discriminated union overcallback,code, andclient. TheToolResolverwalks the configs, pulls declared secrets through an injected provider, and returns aResolvedToolSet. The defaultEnvironmentToolSecretProviderreadsos.environ, so a standalone resolve needs no platform. A gateway config has no offline resolution, so the resolver raisesUnsupportedToolProviderErrorunless the caller injects aGatewayToolResolver. That injection point is where the server-only Composio adapter attaches later.The DTOs change shape. Before,
SessionConfigand the harness configs carriedbuiltin_tools: List[str]andcustom_tools: List[Dict[str, Any]]. After, they carrybuiltin_names: List[str]andtool_specs: List[ToolSpec]. The old names survive as read aliases (AliasChoices) and as@propertyshims, so existing callers and stored payloads keep working.AgentConfig.toolsis now typedList[ToolConfig]and coerces legacy shapes on the way in.ToolCallbackmoves out ofdtos.pyintotools/models.py.The wire gains one field. Every resolved tool spec now serializes a
kinddiscriminator, so the runner reads the executor type directly instead of sniffing fields. The two golden/runrequest fixtures pick up"kind": "callback"on their callback tool, and that is the whole behavioral wire delta.MCP rides alongside as a sibling.
AgentConfiggainsmcp_servers: List[MCPServerConfig], harness configs emit amcpServerswire field throughwire_mcp(), and the payload omits that field when no servers are declared, so a tool-free run's wire stays byte-identical.Key architectural decision to review
The durable tool contract is the split between
ToolConfigandToolSpecintools/models.py. A stored config names a tool by reference (a builtin name, a gateway slug, a code body, a client declaration). A resolved spec carries a URI-like identity plus a JSON Schema plus either an execution body (code) or a delivery reference (callReffor callback). Resolution is the only bridge between them, and it stays offline in the SDK. The tradeoff: the SDK owns the shape and the secret plumbing, but it deliberately owns no gateway or vault logic. Those are server concerns that attach through theGatewayToolResolverandToolSecretProviderports. Scrutinize whether those two ports are the right seam, because every server-only piece lands behind them.The second thing to weigh is the alias-and-property compatibility layer on the DTOs (
dtos.py, around theSessionConfigandPiAgentConfigvalidators). It lets the rename ride in without breaking callers, but it means two names point at one field for a while. Decide whether that bridge should have a removal milestone.How to review this PR
Start with
sdks/python/agenta/sdk/agents/tools/models.py. Read the two unions and confirm the discriminators (typefor config,kindfor spec) and the alias handling onToolSpecBase. Then readtools/resolver.pyto see how a config becomes a spec and where secrets and the gateway port enter. Then readtools/compat.pyto see how legacy shapes (a bare string, acomposiotype, a gateway slug, an OpenAI-stylefunction) fold into the canonical union. Themcp/package mirrors thetools/package one-to-one, so skim it once you trusttools/.Skip
ui_messages.py; it is a compatibility re-export fromadapters/verceland carries no new logic. Checkdtos.pyfor the alias correctness, since a wrongAliasChoicesis the likely regression: a stored payload using the old key must still load. The golden fixtures andtest_wire_contract.pyguard the wire, so a diff there beyond the addedkindis a red flag.Tests / notes
New unit tests cover the tool models, parsing, resolver, and MCP resolver. A transport round-trip integration test exercises the resolved set end to end. The wire-contract test adds a case proving code, client, and MCP specs reach the runner intact and that
mcpServersis omitted when empty.