feat(agent): client-tool round-trip backend (#4920)#4925
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. 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:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@ardaerzin — this is your counterpart for the FE round-trip design. The backend is done (see PR description). Your frontend slice needs:
The design file has the full detail: docs/design/agent-workflows/projects/agent-fe-roundtrip/design.md (section "Work split -> Arda (FE)"). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
services/agent/tests/unit/sandbox-agent-permissions.test.ts (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for the client-tool branch.
These stubs keep the expanded
Responderinterface satisfied, but the newattachPermissionResponderpath should also assert a request withtoolCall.spec.kind === "client"emitskind: "client_tool", parks without replying, and maps deny/output outcomes throughclientToolReply().Also applies to: 108-110, 147-149
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8737fc66-7334-4508-82bf-da22b81679d9
📒 Files selected for processing (11)
api/oss/src/apis/fastapi/tools/router.pyapi/oss/src/apis/fastapi/workflows/router.pyapi/oss/src/core/workflows/static_catalog.pysdks/python/agenta/sdk/agents/adapters/vercel/messages.pysdks/python/agenta/sdk/agents/platform/workflow.pyservices/agent/src/engines/sandbox_agent/permissions.tsservices/agent/src/responder.tsservices/agent/src/tools/dispatch.tsservices/agent/tests/unit/responder.test.tsservices/agent/tests/unit/sandbox-agent-orchestration.test.tsservices/agent/tests/unit/sandbox-agent-permissions.test.ts
| async def _emit_data_event( | ||
| *, | ||
| request: Request, | ||
| name: str, | ||
| data: dict, | ||
| ) -> None: | ||
| emit = getattr(request.state, "emit", None) or getattr( | ||
| request.state, "emit_event", None | ||
| ) | ||
| if not callable(emit): | ||
| return | ||
|
|
||
| result = emit({"type": "data", "name": name, "data": data}) | ||
| if isawaitable(result): | ||
| await result |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t let UI event delivery fail a successful tool call.
If the request-state emitter throws here, _call_workflow_tool() returns an error even though the workflow already ran and produced outputs. This helper should be best-effort for the same reason as the workflow router variant.
| await _emit_committed_revision_data_event( | ||
| request=request, | ||
| workflow_revision=workflow_revision, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Emit the same event from the normal commit endpoint.
This only covers create_workflow_revision(). The regular commit path at Line 1500 still returns without a committed-revision event, so follow-up commits will not trigger the frontend revision refresh that this PR is adding.
Minimal follow-up
# In commit_workflow_revision(), after invalidate_cache(...)
await _emit_committed_revision_data_event(
request=request,
workflow_revision=workflow_revision,
)| async def _emit_data_event( | ||
| *, | ||
| request: Request, | ||
| name: str, | ||
| data: dict, | ||
| ) -> None: | ||
| emit = getattr(request.state, "emit", None) or getattr( | ||
| request.state, "emit_event", None | ||
| ) | ||
| if not callable(emit): | ||
| return | ||
|
|
||
| result = emit({"type": "data", "name": name, "data": data}) | ||
| if isawaitable(result): | ||
| await result |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make data-event emission best-effort.
If request.state.emit fails here after the revision was already committed, the API turns a successful mutation into a 5xx and invites duplicate retries. Swallow/log emitter failures instead of letting them escape the request.
| if ( | ||
| tool_name is None | ||
| and ptype.startswith("tool-") | ||
| and ptype != TOOL_OUTPUT_AVAILABLE | ||
| and ptype not in TOOL_OUTPUT_AVAILABLE_TYPES | ||
| ): | ||
| tool_name = ptype[len("tool-") :] | ||
|
|
||
| blocks: List[ContentBlock] = [] | ||
| if ptype != TOOL_OUTPUT_AVAILABLE or "input" in part: | ||
| if ptype not in TOOL_OUTPUT_AVAILABLE_TYPES or "input" in part: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t synthesize tool names for generic result parts.
Line 100 only exempts tool-output-available. tool-output-error and tool-output-denied still fall through the tool- prefix path, so parts emitted by sdks/python/agenta/sdk/agents/adapters/vercel/stream.py:137-157 without toolName get parsed as tool_name="output-error" / "output-denied" and also produce a bogus tool_call at Line 105. That breaks the documented neutral tool_result mapping for non-success tool results.
Suggested fix
-TOOL_OUTPUT_AVAILABLE_TYPES = {TOOL_OUTPUT_AVAILABLE, "TOOL_OUTPUT_AVAILABLE"}
+TOOL_OUTPUT_AVAILABLE_TYPES = {TOOL_OUTPUT_AVAILABLE, "TOOL_OUTPUT_AVAILABLE"}
+GENERIC_TOOL_RESULT_TYPES = {
+ *TOOL_OUTPUT_AVAILABLE_TYPES,
+ "tool-output-error",
+ "tool-output-denied",
+}
@@
- if (
- tool_name is None
- and ptype.startswith("tool-")
- and ptype not in TOOL_OUTPUT_AVAILABLE_TYPES
- ):
+ if (
+ tool_name is None
+ and ptype.startswith("tool-")
+ and ptype not in GENERIC_TOOL_RESULT_TYPES
+ ):
tool_name = ptype[len("tool-") :]
@@
- if ptype not in TOOL_OUTPUT_AVAILABLE_TYPES or "input" in part:
+ if ptype not in GENERIC_TOOL_RESULT_TYPES or "input" in part:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| tool_name is None | |
| and ptype.startswith("tool-") | |
| and ptype != TOOL_OUTPUT_AVAILABLE | |
| and ptype not in TOOL_OUTPUT_AVAILABLE_TYPES | |
| ): | |
| tool_name = ptype[len("tool-") :] | |
| blocks: List[ContentBlock] = [] | |
| if ptype != TOOL_OUTPUT_AVAILABLE or "input" in part: | |
| if ptype not in TOOL_OUTPUT_AVAILABLE_TYPES or "input" in part: | |
| if ( | |
| tool_name is None | |
| and ptype.startswith("tool-") | |
| and ptype not in GENERIC_TOOL_RESULT_TYPES | |
| ): | |
| tool_name = ptype[len("tool-") :] | |
| blocks: List[ContentBlock] = [] | |
| if ptype not in GENERIC_TOOL_RESULT_TYPES or "input" in part: |
| def _is_request_connection_workflow(tool_config: ReferenceToolConfig) -> bool: | ||
| workflow = getattr(tool_config, "workflow", None) | ||
| if getattr(workflow, "slug", None) == REQUEST_CONNECTION_WORKFLOW_SLUG: | ||
| return True | ||
|
|
||
| call_ref = tool_config.call_ref | ||
| return ( | ||
| call_ref == f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}" | ||
| or call_ref.startswith(f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}.") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle environment-scoped request-connection references too.
Line 116 is checking tool_config.workflow.slug, but ReferenceToolConfig exposes slug directly. As written, the special case only matches workflow.variant... call refs, so ref_by="environment" tools targeting __ag__request_connection resolve as ordinary callback tools and bypass the client-tool flow.
Suggested fix
def _is_request_connection_workflow(tool_config: ReferenceToolConfig) -> bool:
- workflow = getattr(tool_config, "workflow", None)
- if getattr(workflow, "slug", None) == REQUEST_CONNECTION_WORKFLOW_SLUG:
+ if tool_config.slug == REQUEST_CONNECTION_WORKFLOW_SLUG:
return True
call_ref = tool_config.call_ref
return (
call_ref == f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}"
or call_ref.startswith(f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}.")
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _is_request_connection_workflow(tool_config: ReferenceToolConfig) -> bool: | |
| workflow = getattr(tool_config, "workflow", None) | |
| if getattr(workflow, "slug", None) == REQUEST_CONNECTION_WORKFLOW_SLUG: | |
| return True | |
| call_ref = tool_config.call_ref | |
| return ( | |
| call_ref == f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}" | |
| or call_ref.startswith(f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}.") | |
| ) | |
| def _is_request_connection_workflow(tool_config: ReferenceToolConfig) -> bool: | |
| if tool_config.slug == REQUEST_CONNECTION_WORKFLOW_SLUG: | |
| return True | |
| call_ref = tool_config.call_ref | |
| return ( | |
| call_ref == f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}" | |
| or call_ref.startswith(f"workflow.variant.{REQUEST_CONNECTION_WORKFLOW_SLUG}.") | |
| ) |
| private lookupClientTool(request: ClientToolRequest): { found: boolean; output?: unknown } { | ||
| for (const key of clientToolRequestKeys(request)) { | ||
| if (this.decisions.has(key)) { | ||
| const output = this.decisions.get(key); | ||
| if (!isPermissionDecision(output)) return { found: true, output }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Disambiguate permission decisions from client-tool outputs.
ApprovalDecisions now stores arbitrary client-tool outputs, but parkedCallResultOf() coerces any { approved: boolean } output into "allow"/"deny", and lookupClientTool() then drops stored "allow"/"deny" values as permission decisions. A valid client-tool output like { approved: false } or "deny" will not resume correctly. Store tagged entries, e.g. permission vs client-tool result, instead of using raw sentinel values.
Also applies to: 345-367
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🤖 The AI agent says: What to review on this PR:
|
RUNNER: add onClientTool to Responder; detect client specs in
permissions.ts and emit interaction_request(kind=client_tool) with park;
remove the throw in dispatch.ts (return pending signal instead);
rename approvalKey -> parkedCallKey everywhere; generalize
extractApprovalDecisions to handle both {approved} and structured output.
SDK: widen tool-output-available parsing in messages.py; add
__ag__request_connection static workflow resolving to ClientToolSpec
(kind=client, render={kind:connect}) in platform/workflow.py.
API: register __ag__request_connection in static_catalog; emit
data-committed-revision event after successful commit_revision in both
tools/router.py and workflows/router.py.
All checks: ruff format+check (API+SDK), tsc --noEmit, vitest (44 tests pass).
Frontend (message-part dispatcher, connect widget, resume predicate,
result UX, config refresh) is NOT included — owned by @ardaerzin.
mmabrouk
force-pushed
the
feat/client-tool-roundtrip-4920
branch
from
be5bd45 to
76b89f4
Compare
1 participant
Context
Some tools the agent uses require user interaction before they can complete. The OAuth connection flow is the main case: the agent asks the user to connect an integration (GitHub, Slack, etc.), the user finishes sign-in in the browser, and the run must resume with the result. The existing runner handled only
approved/deniedpermission decisions and had no path for structured output from a client-side tool call.This PR wires the backend side of that round-trip. The frontend slice (message-part dispatcher, connect widget, resume predicate, result UX, config refresh on commit) is a separate deliverable, coordinated with @ardaerzin.
Design:
docs/design/agent-workflows/projects/agent-fe-roundtrip/design.mdChanges
TS runner (
services/agent/):responder.ts—RespondergainsonClientTool(req).ApprovalDecisionsnow holds{ output: unknown }in addition to permission decisions.parkedCallKeyreplacesapprovalKeyeverywhere (the old name implied approval-only semantics).permissions.ts— Before dispatching a tool, checksspec.kind === "client". If so, emitsinteraction_request(kind: "client_tool")and parks viaresponder.onClientTool. The tool call stays parked until the frontend sends a result.dispatch.ts— Client tools no longer throw. They return a{type: "client_tool_pending"}signal; the permission responder owns the park.SDK (
sdks/python/):adapters/vercel/messages.py—TOOL_OUTPUT_AVAILABLE_TYPESis now a set containing both"tool-output-available"and"TOOL_OUTPUT_AVAILABLE". The previous check was case-exact and dropped uppercase variants from some clients.agents/platform/workflow.py— Registers__ag__request_connectionas a hard-coded client tool that resolves toClientToolSpec(kind="client", render={"kind": "connect"}). Agents that include this workflow as a tool get the connect affordance without any backend call.API:
core/workflows/static_catalog.py— Registers__ag__request_connectionunder URIclient:tool:request_connection:v0alongside the getting-started skill.apis/fastapi/tools/router.pyandworkflows/router.py— Bothcommit_revisionpaths now emit a one-waydata-committed-revisionevent{variantId, revisionId, version}after a successful commit. The frontend uses this to refresh the build-kit state without a separate poll.Scope / risk
This is the backend slice only. The interaction loop is incomplete until the frontend dispatches
client_toolinteraction events and sends results back. Until that lands, akind: "client"tool call will emit the event and park but never resume. The agent will appear to hang at that point.The
_emit_data_eventhelper in both API routers resolvesrequest.state.emitwith a fallback toemit_event. If neither attribute exists (e.g., outside a streaming context), it exits silently. Reviewers should confirm theemitattribute is reliably present onrequest.statein the tool-call and workflow-commit code paths, or that silent failure is the right behavior.The
parkedCallKeyrename is purely cosmetic. It does not change behavior. The test files were updated to match.Tests
responder.test.ts,sandbox-agent-orchestration.test.ts,sandbox-agent-permissions.test.tsupdated forparkedCallKeyrename and expandedResponderinterface.ruff format+checkclean on API + SDK.tsc --noEmitclean.How to QA
Prerequisites: local dev stack with the agent sidecar running. The frontend slice is not yet merged, so the round-trip cannot be exercised end-to-end. This PR can be verified at the unit-test level only.
Automated tests:
What to verify: Run the TS tests and confirm the
parkedCallKeyrename did not break any assertion. Confirm the API router tests still pass after the_emit_data_eventaddition.Edge cases: The
_emit_data_eventcall in the tools router readsoutputsas a dict and exits ifvariantId/revisionId/versionare absent. Confirm acommit_revisioncall that returns a non-dict output does not raise.https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc