feat(agent): resolve typed tools through the service#4764
Conversation
|
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 |
dosubot
Bot
added
size:XXL
Reviewer guide: interesting code
|
| secret_provider=secret_provider, | ||
| gateway_resolver=AgentaGatewayToolResolver(), | ||
| missing_secret_policy=MissingSecretPolicy.ERROR, | ||
| ).resolve(tool_configs) |
There was a problem hiding this comment.
Composition root: the offline SDK ToolResolver receives the gateway resolver and vault secret provider as ports, with a fail-fast MissingSecretPolicy.ERROR. The resolver itself never knows it runs on a server, which is what keeps it usable standalone.
| ) -> GatewayToolResolution: | ||
| for tool_config in tools: | ||
| if tool_config.provider != "composio": | ||
| raise UnsupportedToolProviderError(tool_config.provider) |
There was a problem hiding this comment.
The gateway adapter rejects non-Composio providers up front and raises on every count/duplicate/shape mismatch below, so it never returns partial specs. Worth confirming each branch raises rather than degrades, since the model would otherwise see a tool with no working callback.
| data = response.json() or {} | ||
| except Exception: # pylint: disable=broad-except | ||
| log.warning("agent: named-secret resolve failed for %s", names, exc_info=True) | ||
| return {} |
There was a problem hiding this comment.
Best-effort by design: a transport error returns {} and only logs the missing names. The hard failure for a declared-but-unresolved secret comes from the SDK resolver under MissingSecretPolicy.ERROR, not here. That split decides whether a vault outage fails fast or yields an agent missing its secret.
| unsupported = [ | ||
| config | ||
| for config in configs | ||
| if not isinstance(config, (BuiltinToolConfig, GatewayToolConfig)) |
There was a problem hiding this comment.
This coercion is what keeps /tools/resolve the gateway-only endpoint: it runs the shared coerce_tool_configs and rejects anything other than builtin or gateway, so code/client/MCP tools cannot leak into this path.
|
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 #4763 (merge that first).
Context
The base PR
feat/sdk-local-tools-contractsmoved tool resolution into an offline SDK resolver: it parses tool configs, applies a missing-secret policy, and builds code, client, and callback specs without any network call. This PR is slice #9 (tool runtime) indocs/design/agent-workflows/pr-stack.md. It does the service-side composition: it wires that offline resolver to the two concerns only the server can satisfy, the Composio gateway and the project vault.What this changes
Before,
services/oss/src/agent/tools.pywas one module that parsed tool references inline, calledPOST /tools/resolve, and returned a bare 3-tuple of(builtins, custom_tools, tool_callback). It only understood builtin and Composio tools, and it skipped anything it could not parse.After, that module is gone. A
services/oss/src/agent/tools/package now composes the SDKToolResolverwith two service adapters and returns a typedResolvedAgentResources(aResolvedToolSetplus resolved MCP servers).AgentaGatewayToolResolveris the gateway port: it maps tool configs to gateway references, callsPOST /tools/resolve, and turns the response into callback specs.VaultToolSecretProvideris the secret port: it resolves named secrets throughPOST /secrets/resolve. The handler inapp.pynow callsresolve_agent_resources(tools=..., mcp_servers=...)and readsresources.tools.builtin_names,resources.tools.tool_specs, andresources.tools.tool_callbackontoSessionConfig.On the contract side, the API now reuses the SDK tool-config models instead of its own.
api/oss/src/core/tools/dtos.pyaliasesAgentBuiltinToolandAgentComposioToolto the SDKBuiltinToolConfigandGatewayToolConfig.ToolResolveRequestcoerces incoming tools through the sharedcoerce_tool_configsand rejects anything other than builtin or gateway, so/tools/resolvestays the gateway-only endpoint.The agent config schema also collapses to one catalog type.
schemas.pydrops its hand-writtenagent_configproperty block and emits a thinx-ag-type-ref: agent_config; the field shape now comes fromAgentConfigSchemain the SDK, which addsmcp_serversand renamesinstructionstoagents_md. The playground gains anMcpServerItemControland an updatedAgentConfigControl.Key architectural decision to review
The resolver composition in
services/oss/src/agent/tools/resolver.pyis the decision to scrutinize. The SDK resolver stays offline and pure; every networked concern enters through a port. The gateway resolver and the vault secret provider are injected intoToolResolver, so the resolver itself never knows it is on a server. The tradeoff is one more layer of indirection in exchange for an SDK resolver that runs standalone and a server that owns all I/O.The second decision is the secret failure policy. The resolver injects
MissingSecretPolicy.ERRORfor both tools and MCP. A declared secret that the vault cannot return fails the run rather than silently dropping. Watch the seam:VaultToolSecretProvider.get_manyreturns{}on a transport error and only logs the missing names, so the hard failure comes from the SDK resolver when a declared secret is absent from the returned map, not from the adapter. Confirm that split is what you want, because it decides whether a vault outage fails fast or yields an agent missing its secrets.How to review this PR
Start with
services/oss/src/agent/tools/resolver.py. It is small and names every adapter and policy, so it frames the rest. Then readgateway.pyfor the/tools/resolvemapping and its strict one-spec-per-ref and duplicate-ref checks, andsecrets.pyfor the/secrets/resolvecall and its best-effort error handling. Then readapp.pyto see the call site move from the 3-tuple toResolvedAgentResources.Check that the unsupported-provider, duplicate-reference, and count-mismatch branches in the gateway adapter all raise rather than return partial results. Check that
_mcp_enabledstill gates MCP resolution behindAGENTA_AGENT_ENABLE_MCP. You can skip theweb/controls and the test conftests on a first pass; they follow the contract rather than define it.Likely regression: a caller that still imports
resolve_toolsexpecting the old 3-tuple. The package keeps aresolve_toolsshim, but it now returns aResolvedToolSet, so any unported caller breaks at the unpack.Tests / notes
Unit tests cover code-tool scoped env, the named-secret fail-fast, and the gateway reference mapping. Integration tests run the gateway and secret adapters against mocked
/tools/resolveand/secrets/resolveresponses.api/oss/tests/pytest/unit/tools/test_agent_resolution.pycovers the request coercion and the builtin-or-gateway-only rejection.