feat(agent): runnable tools as agent configuration#4759
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 codeThe hunks that carry the design decisions, in reading order:
|
| # A slug segment is safe for the ``tools.{provider}.{integration}.{action}.{connection}`` | ||
| # call-ref. ``__`` is forbidden because ``/tools/call`` round-trips ``__`` <-> ``.`` when | ||
| # parsing function names, so a ``__`` inside a segment would corrupt the split. | ||
| _SLUG_SEGMENT_RE = re.compile(r"^[a-zA-Z0-9-]+(?:_[a-zA-Z0-9-]+)*$") |
There was a problem hiding this comment.
This regex forbids `` in a slug segment on purpose: `/tools/call` round-trips `` <-> `.` when parsing the OpenAI-style function name, so a `__` inside an integration or action name would corrupt the five-way split of the call-ref. The model-facing function name and the call-ref slug share one grammar.
| project_id=project_id, | ||
| provider_key=provider_key, | ||
| integration_key=integration_key, | ||
| is_active=None, |
There was a problem hiding this comment.
The `is_active=None` query (all, not active-only) is the reason this lookup can distinguish "inactive" from "not found". Querying active-only would collapse an inactive connection into an indistinguishable `ConnectionNotFoundError`.
| request.customTools ?? [], | ||
| request.toolCallback, | ||
| ); | ||
| const toolAllowlist = [ |
There was a problem hiding this comment.
Key coupling for review: Pi gates custom tools through the same `tools` allowlist as built-ins, so appending each customTool name here is required. An empty allowlist registers the customTool but never exposes it to the model.
| id: toolCallId, | ||
| type: "function", | ||
| // Arguments as an object (not a JSON string) to avoid double-encoding. | ||
| function: { name: callRef, arguments: params ?? {} }, |
There was a problem hiding this comment.
Arguments go as an object, not a JSON string, to avoid double-encoding on the `/tools/call` round-trip. A failure in this fetch throws, which Pi converts to a tool-error result so the loop continues rather than failing the whole run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33129cd3-25a8-4c72-95b0-b6e52f387a0d
📒 Files selected for processing (22)
api/oss/src/apis/fastapi/tools/models.pyapi/oss/src/apis/fastapi/tools/router.pyapi/oss/src/core/tools/dtos.pyapi/oss/src/core/tools/exceptions.pyapi/oss/src/core/tools/service.pydocs/design/agent-workflows/wp-1-pi-tracing/tracing-in-the-agent-service.mddocs/design/agent-workflows/wp-2-agent-service/implementation-plan.mddocs/design/agent-workflows/wp-2-agent-service/qa.mddocs/design/agent-workflows/wp-7-tools/README.mdhosting/docker-compose/ee/docker-compose.dev.ymlservices/agent/README.mdservices/agent/docker-compose.agent.ymlservices/agent/docker-compose.stack.ymlservices/agent/scripts/register_agent_app.pyservices/agent/src/runPi.tsservices/entrypoints/agent_main.pyservices/oss/src/agent.pyservices/oss/src/agent_pi/config.pyservices/oss/src/agent_pi/pi_harness.pyservices/oss/src/agent_pi/pi_http_harness.pyservices/oss/src/agent_pi/ports.pyservices/oss/src/agent_pi/schemas.py
💤 Files with no reviewable changes (4)
- services/agent/docker-compose.agent.yml
- services/entrypoints/agent_main.py
- services/agent/scripts/register_agent_app.py
- services/agent/docker-compose.stack.yml
| for ref in tools: | ||
| if isinstance(ref, AgentBuiltinTool): | ||
| if ref.name: | ||
| builtins.append(ref.name) | ||
| continue | ||
|
|
||
| if isinstance(ref, AgentComposioTool): | ||
| custom.append( | ||
| await self._resolve_composio_tool( | ||
| project_id=project_id, | ||
| ref=ref, | ||
| ) | ||
| ) | ||
|
|
||
| return AgentToolsResolution(builtins=builtins, custom=custom) |
There was a problem hiding this comment.
Enforce unique resolved tool names before returning the resolution.
Two Composio entries with different connection values currently default to the same name ({integration}__{action}), which creates ambiguous custom-tool registration/selection at runtime.
Proposed fix
async def resolve_agent_tools(
@@
- builtins: List[str] = []
- custom: List[ResolvedAgentTool] = []
+ builtins: List[str] = []
+ custom: List[ResolvedAgentTool] = []
+ seen_names: set[str] = set()
@@
if isinstance(ref, AgentBuiltinTool):
- if ref.name:
- builtins.append(ref.name)
+ if ref.name:
+ if ref.name in seen_names:
+ raise ToolSlugInvalidError(
+ slug=ref.name,
+ detail=f"Duplicate tool name: {ref.name!r}",
+ )
+ seen_names.add(ref.name)
+ builtins.append(ref.name)
continue
if isinstance(ref, AgentComposioTool):
- custom.append(
- await self._resolve_composio_tool(
- project_id=project_id,
- ref=ref,
- )
- )
+ resolved = await self._resolve_composio_tool(
+ project_id=project_id,
+ ref=ref,
+ )
+ if resolved.name in seen_names:
+ raise ToolSlugInvalidError(
+ slug=resolved.call_ref,
+ detail=f"Duplicate tool name: {resolved.name!r}",
+ )
+ seen_names.add(resolved.name)
+ custom.append(resolved)
@@
- name = ref.name or f"{ref.integration}__{ref.action}"
+ name = ref.name or f"{ref.integration}__{ref.action}__{ref.connection}"Also applies to: 559-563
| /** Per-tool budget for the /tools/call round-trip. Surfaced as a tool error on timeout. */ | ||
| const TOOL_CALL_TIMEOUT_MS = Number( | ||
| process.env.AGENTA_AGENT_TOOL_CALL_TIMEOUT_MS ?? 30000, | ||
| ); |
There was a problem hiding this comment.
Validate AGENTA_AGENT_TOOL_CALL_TIMEOUT_MS before using it.
Number(...) can produce NaN/invalid values; passing that into timeout construction can cause runtime failures on every tool call.
Proposed fix
-const TOOL_CALL_TIMEOUT_MS = Number(
- process.env.AGENTA_AGENT_TOOL_CALL_TIMEOUT_MS ?? 30000,
-);
+const parsedTimeoutMs = Number(
+ process.env.AGENTA_AGENT_TOOL_CALL_TIMEOUT_MS ?? 30000,
+);
+const TOOL_CALL_TIMEOUT_MS =
+ Number.isFinite(parsedTimeoutMs) && parsedTimeoutMs > 0
+ ? parsedTimeoutMs
+ : 30000;| if (!callback?.endpoint) { | ||
| log(`skipping ${specs.length} custom tool(s): missing toolCallback endpoint`); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Fail fast when customTools are provided without a callback endpoint.
Dropping tools silently here makes runs behave as tool-less without surfacing the configuration error.
Proposed fix
export function buildCustomTools(
specs: ResolvedToolSpec[],
callback: ToolCallbackContext | undefined,
): any[] {
if (specs.length === 0) return [];
if (!callback?.endpoint) {
- log(`skipping ${specs.length} custom tool(s): missing toolCallback endpoint`);
- return [];
+ throw new Error(
+ `customTools provided (${specs.length}) but toolCallback.endpoint is missing`,
+ );
}
|
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 #4758 (merge that first).
Context
An agent revision can now carry a list of tools in its config, and the runtime can execute them. This is the first cut of WP-7: a Composio action you configure on an agent becomes a runnable tool that Pi calls during a run, with the provider key kept server-side. It sits on top of
feat/agent-workflows(base branch) and is slice #9 (tool runtime) ofpr-stack.md.What this changes
Tools become part of the agent's saved configuration. Under an agent revision's
parameters, each tool entry is either a built-in name or a discriminated object:At invoke time the agent service resolves that list. Built-in names pass straight through to Pi's
tools: string[]. Composio references go to a new backend endpoint,POST /tools/resolve, which validates the connection and enriches each action from the catalog (description plus input schema). The resolved spec carries acall_refslug,tools.composio.{integration}.{action}.{connection}. The TS wrapper turns each spec into a PicustomToolwhoseexecuteposts that slug back to the existingPOST /tools/call. Pi drives the tool loop in the sandbox, but the Composio key and the connection auth never leave the backend.Before this PR the agent ran with no runnable tools and a fixed two-field config (
model,agents_md). After it, the playground renders the agent config as aprompt-template, so the model selector, system-message editor, and tool picker that chat and completion already use now drive the agent with no new frontend code (agent_pi/schemas.py). The handler reads the system message as the AGENTS.md and the picked tools fromllm_config.tools.call_toolno longer inlines its connection lookup. That logic moved intoToolsService.resolve_connection_by_slug, which both the execution endpoint and the new resolver share.Key architectural decision to review
The configured tool name must match Pi's session allowlist, or the call is hidden. In
services/agent/src/runPi.ts, custom tool names are appended to thecreateAgentSessiontoolsallowlist alongside the built-ins. Pi gates custom tools through the same allowlist, so an empty allowlist would register thecustomToolbut never expose it to the model. This couples the resolved tool name to the allowlist entry. CheckbuildCustomToolsand thetoolAllowlistassembly.The
call_refslug round-trips__<->., so slug segments forbid__._SLUG_SEGMENT_REinapi/oss/src/core/tools/service.pyrejects__inside any segment because/tools/callswaps__and.when it parses the OpenAI-style function name. A__inside an integration or action name would corrupt the five-way split. The tradeoff: the function name the model sees and the call-ref slug share one grammar. Confirm the regex matches the parsing in_parse_gateway_slug(services/oss/src/agent.py) and incall_tool.How to review this PR
api/oss/src/core/tools/service.pyfirst. Readresolve_agent_toolsand_resolve_composio_toolfor the resolution shape, thenresolve_connection_by_slugfor the extracted lookup. This is where the contract lives.services/oss/src/agent.py:_resolve_tools(the HTTP hop to/tools/resolve),_normalize_tool_refand_parse_gateway_slug(config and picker shapes into refs), and_request_authorization(the credential reuse).services/agent/src/runPi.ts:buildCustomToolsandcallAgentaToolfor the execution bridge.agent_main.pyand the two standalone composes are removed in favor of the integrated/agent/v0mount; theqa.mddoc records why._resolve_toolsreturns early on an empty ref list, which preserves the tool-less WP-2 path. Check that early return.Tests / notes
/tools/resolvebuilt the spec, Pi registered thegithub_whoamicustomTool and called it, the bridge executed the action through/tools/call, and the agent answered with live data (login, follower count). The trace nests the tool call:_agent -> invoke_agent -> turn 0 -> {chat, execute_tool} -> turn 1 -> chat.apibehind an HTTP endpoint the agent service calls. When WP-6 lands, its invoke path calls the sameToolsService.resolve_agent_tools(...)in-process and the HTTP hop drops out. Later PRs (feat(sdk): typed agent tool resolution contracts #4763/feat(agent): resolve typed tools through the service #4764/feat(agent): deliver resolved tools through the runner #4765) move resolution into the SDK and service and deliver it through the runner._resolve_toolsraises on any resolution failure, so a missing or inactive connection fails the invoke early with a clear message instead of surfacing as a mid-loop tool error.agent-pisidecar no longer loads the stackenv_file, so the sandbox cannot inheritCOMPOSIO_API_KEYand the other secrets (hosting/docker-compose/ee/docker-compose.dev.yml).