fix(agent): address CodeRabbit findings on direct-call dispatch (#4891)

[mmabrouk]

Why

PR #4891 (direct-call tools, Phase 2 sidecar dispatch) was merged with a green lgtm, but the CodeRabbit review that ran right after the hardening commit posted five actionable findings that never got folded in before the merge. This PR addresses all five in services/agent/src/tools/direct.ts, with tests.

The wire is unchanged and no resolver emits a call descriptor yet, so production behavior does not change. These are guard hardenings plus one functional-correctness trap.

What changed

#	CodeRabbit finding	Severity	Fix
1	Nested payload objects are not sanitized; root model args bypass the sanitizer via spread	Security, Major	deepMerge now recurses into every plain-object value, so a nested __proto__/constructor/prototype inside an untrusted subtree is stripped at every level. assembleBody deep-sanitizes the model args before they land at args_into or the root.
2	The SSRF guard silently widens when the callback mount cannot be derived	Security, Major	directCallUrl normalizes a trailing slash, then fails closed (throws) on any callback path that does not end with /tools/call, instead of deriving an empty mount.
3	Server logs leak the upstream response body	Privacy, Major	callDirect no longer reads or logs the upstream response body on the error path. It keeps the run's own internal URL and the status (no credentials; the authorization header is never logged), which are load-bearing for diagnosis.
4	A GET direct call silently drops model args	Functional, Major	callDirect fails fast when a GET carries a non-empty assembled body, instead of executing a call that quietly loses its inputs.
5	Stale test header (references the deferred dispatch.ts branch and an /api-relative rule)	Maintainability, Minor	Header now states relay-only with the dispatch branch deferred, and the mount-agnostic path rule.

Before / after (finding 3, server log on a non-2xx)

Before:

direct tool call POST https://host/api/workflows/invoke returned HTTP 500: <first 500 chars of the upstream body, which can contain user data>

After:

direct tool call POST https://host/api/workflows/invoke returned HTTP 500

Tests

Added coverage for nested-key sanitization (assembleBody + deepMerge), fail-closed mount derivation (and trailing-slash normalization), and the GET-with-body guard.

• cd services/agent && pnpm typecheck clean.
• pnpm test green: 316 tests across 31 files.

Not in this PR

The shallow fixed-wins overlay (a model can add a sibling key under a subtree that call.body only partly fixes) stays deferred, as noted on #4891. The durable fix there is strict request-schema enforcement on the assembled body, which is a larger, separate change. The prototype-pollution sanitization in this PR is distinct from that and is fully addressed.

https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agenta-documentation	Ready	Preview, Comment	Jun 28, 2026 12:27pm

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c640e4d8-dfca-4f6a-a27e-645254f62316

📥 Commits

Reviewing files that changed from the base of the PR and between d54a8b3 and 14ee3d5.

📒 Files selected for processing (2)

• services/agent/src/tools/direct.ts
• services/agent/tests/unit/tool-direct.test.ts

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Strengthened request handling for direct tool calls, including safer merging of model-provided parameters and improved protection against unsafe keys.
  • Tightened callback URL validation to reject unexpected endpoint shapes and handle trailing slashes correctly.
  • Added a guard so GET requests can’t carry hidden body data, preventing inputs from being dropped silently.
  • Improved error handling for failed requests by avoiding response-body exposure in logs and user-facing errors.

Walkthrough

Three security hardening changes to services/agent/src/tools/direct.ts: deepMerge sanitizes prototype-polluting keys at every recursion level; assembleBody uses deepMerge instead of shallow spread for model params; directCallUrl normalizes trailing slashes and throws on unexpected callback paths; callDirect rejects non-empty GET bodies and omits response body from error logs. Tests cover all cases.

Direct-call tool security hardening

Layer / File(s)	Summary
deepMerge prototype-pollution fix and assembleBody sanitization services/agent/src/tools/direct.ts, services/agent/tests/unit/tool-direct.test.ts	deepMerge computes an existing subtree before recursing to sanitize __proto__ keys at every level. assembleBody replaces shallow { ...args } spread with deepMerge({}, raw) for model params. Tests add nested __proto__ cases for both root and args_into merge paths, plus an edge case where the overlay lands on a non-object base key.
directCallUrl fail-closed SSRF guard services/agent/src/tools/direct.ts, services/agent/tests/unit/tool-direct.test.ts	Mount derivation is rewritten to trim trailing slashes from the callback pathname and throw if the path does not end with /tools/call, replacing the prior silent widening. Tests verify trailing-slash normalization and fail-closed rejection.
callDirect GET body guard and error path cleanup services/agent/src/tools/direct.ts, services/agent/tests/unit/tool-direct.test.ts	callDirect throws when a non-empty body is assembled for a GET descriptor. Non-OK response error path no longer reads or exposes the upstream response body; logs and thrown errors contain only HTTP status. Tests verify both the rejection and the allowed empty-body GET path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Agenta-AI/agenta#4891: Modifies the same deepMerge, assembleBody, directCallUrl, and callDirect functions in direct.ts and the same test file, making it a direct predecessor to these changes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: addressing CodeRabbit findings in direct-call dispatch.
Description check	✅ Passed	The description is directly about the same direct-call hardening changes and tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 60.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/4891-coderabbit-followups

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mmabrouk]

🤖 The AI agent says:

What this needs from you: a review of how I triaged CodeRabbit's five findings on the merged #4891, and confirmation on the two judgment calls below.

This is the follow-up the task asked for: #4891 merged with CodeRabbit's post-hardening review (five actionable findings) unaddressed. I fixed all five in services/agent/src/tools/direct.ts and added tests. Wire unchanged, no resolver emits call yet, so production behaviour is unchanged.

Two calls I want you to sanity-check:

1. Finding 3 (log hygiene) — partial by design. CodeRabbit wanted both the upstream response body and the internal URL out of the server logs. I dropped the response body (the genuine user-data leak) but kept the URL + method + status. The URL is the run's own internal Agenta endpoint; it carries no credentials (the authorization header is never logged) and it is the load-bearing detail for diagnosing this SSRF-guarded path. Tell me if you'd rather drop the URL too.

2. Finding 4 (GET with a body) — fail-fast, not query mapping. No resolver emits a GET call with inputs today, so I made callDirect throw on a GET with a non-empty body rather than build a query-string mapping now. The mapping can come with the first real GET descriptor. Confirm fail-fast is the right call for now.

The shallow fixed-wins overlay (sibling-key) stays deferred exactly as you noted on #4891 — that needs request-schema enforcement, a separate change. The prototype-pollution sanitization here is distinct and fully fixed.

Checks: runner tsc --noEmit clean; pnpm test green (316 tests, 31 files). Do not merge.

[mmabrouk]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[mmabrouk]

lgtm