Skip to content

feat(mcp-server): dispatch mounted tool calls to the agent in-process#1743

Open
Scra3 wants to merge 17 commits into
mainfrom
feat/mcp-server-in-process-dispatch
Open

feat(mcp-server): dispatch mounted tool calls to the agent in-process#1743
Scra3 wants to merge 17 commits into
mainfrom
feat/mcp-server-in-process-dispatch

Conversation

@Scra3

@Scra3 Scra3 commented Jul 7, 2026

Copy link
Copy Markdown
Member

⚠️ Stacked on #1740 — base is feat/mcp-server-internal-agent-url, not main. Review/merge #1740 first, then this diff collapses to the in-process work only.

What

When the MCP server is mounted in an agent (agent.mountAiMcpServer()), tool calls now reach the agent's data layer in-process — dispatched into the agent's own Koa stack via light-my-request (in-memory, no socket) — instead of calling back over the public api_endpoint.

This removes the need for agentUrl in mounted mode (the feedback that motivated #1740). agentUrl stays for the standalone server (FOREST_AGENT_URL), where there is no agent process to dispatch into.

Why

Self-hosted setups (Swaive) observed every mounted tool call leaving over the public internet and coming back, even though the MCP server and the agent run in the same process. In-process dispatch keeps that hop internal by construction — nothing to configure.

How

  • agent-client: extracted shared response parsing (buildAgentHttpError, deserializeAgentBody) and let createRemoteAgentClient accept an injected httpRequester.
  • agent: InProcessDispatcher injects a request into the agent's real router (auth / permissions / serializer all run); getInProcessDispatcher() re-registers the handler on every remount so a captured reference never goes stale.
  • mcp-server: tools now receive a single ToolContext; when a dispatcher is present, buildClient builds an InProcessHttpRequester that reuses the agent-client parse helpers — so the deserialized result and AgentHttpError shape are byte-identical to the HTTP path, keeping approval-gate detection and error handling unchanged. CSV export (stream) is not supported in-process and throws explicitly.

Tests

  • mcp-server: InProcessHttpRequester unit tests + agent-caller wiring (requester built only when a dispatcher is present) — 624 passing.
  • agent: constructor-spy that mountAiMcpServer passes an agentDispatcher; and a full-stack guard that a real OAuth-authed tools/call reaches the agent through the InProcessDispatcher across all 8 host frameworks — 982 passing.
  • The existing should retrieve users via MCP tools/call integration test now runs through the in-process path and still returns the seeded records.

🤖 Generated with Claude Code

Note

Dispatch MCP server tool calls to the agent in-process instead of over HTTP

  • Introduces InProcessDispatcher in the agent and InProcessHttpRequester in the MCP server so that tool calls bypass the network and go directly into the agent's Koa stack via light-my-request.
  • Adds agentDispatcher to ForestMCPServerOptions and a shared ToolContext; all declare*Tool functions are refactored to accept this context so they can route calls in-process when a dispatcher is available.
  • Adds agentUrl support (FOREST_AGENT_URL env var) for standalone mode, normalised via a new normalizeAgentUrl utility, and plumbed through ForestOAuthProvider so extra.environmentApiEndpoint reflects the agent URL.
  • Extracts buildAgentHttpError and deserializeAgentBody helpers from agent-client and extends createRemoteAgentClient to accept an injected HttpRequester, enabling the in-process path to share the same error shapes and deserialization logic.
  • Risk: in-process tool calls bypass any middleware mounted in front of the agent; only the agent's own auth and permission checks apply.

Changes since #1743 opened

  • Updated documentation for agent.Agent.mountAiMcpServer method to clarify middleware and authentication behavior [929a1c8]
  • Centralized type definitions for in-process agent dispatching by moving InProcessDispatchRequest and InProcessDispatchResponse types from @forestadmin/agent to @forestadmin/mcp-server [9db0616]
  • Removed descriptive comments from ToolContext interface, createClient function in agent-caller module, and updated comment on DEFAULT_TIMEOUT_MS constant to reference HttpRequester.query [9db0616]
  • Refactored buildAgentHttpError and deserializeAgentBody utility functions into protected methods within HttpRequester class [9a1a1c5]
  • Added InProcessHttpRequester.stream() method that throws an error indicating CSV streaming is not supported over the in-process MCP transport [ac2d6bb]
  • Added test coverage for InProcessHttpRequester.stream() error behavior and updated agent caller test to expect InProcessHttpRequester wiring when environmentApiEndpoint is absent [ac2d6bb]
  • Modified InProcessDispatcher.parseBody to reject with an Error for successful HTTP responses (status < 400) containing non-JSON payloads and to log a warning while returning undefined for error responses (status >= 400) with non-JSON payloads [c801d5b]
  • Fixed unhandled rejection issue in InProcessDispatcher.injectWithTimeout by capturing the injection promise separately and attaching a catch handler to log warnings when the handler rejects after the timeout has fired [c801d5b]
  • Added test coverage in mcp-in-process-dispatcher.rejection.test.ts to verify that late injection rejections after timeout are logged as warnings without causing unhandled rejections [c801d5b]
  • Updated test expectations in mcp-in-process-dispatcher.test.ts to validate that non-JSON 2xx responses throw errors and non-JSON 4xx/5xx responses log warnings while returning undefined body [c801d5b]

Macroscope summarized 8abefd1.

alban bertolini and others added 9 commits July 7, 2026 12:27
MCP tools call back into the agent's data layer over HTTP. By default the URL
is the environment's public api_endpoint, so tool calls leave over the public
internet even when the MCP server is mounted inside the agent. Add an opt-in
agentUrl option (mountAiMcpServer, ForestMCPServerOptions, FOREST_AGENT_URL for
the CLI) that overrides only the internal tool->agent channel — the advertised
OAuth discovery URLs stay public so external MCP clients still authenticate.

Requested by a self-hosted customer (Swaive) who wants MCP callbacks to stay on
their private network.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-ups on the agentUrl option:
- normalizeAgentUrl now parses once, rejects non-http(s) schemes and URLs with
  a query string or fragment (which would swallow the request path in
  agent-client's `${url}${path}` join), and returns the parsed, whitespace-
  normalized form instead of the raw string. Previously such inputs passed
  validation and silently misrouted every tool call.
- Add an end-to-end test proving a `tools/call` actually targets agentUrl and
  not the environment's public api_endpoint.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ctor

Extract normalizeAgentUrl to a shared helper and call it from the
ForestMCPServer constructor, so a malformed agentUrl / FOREST_AGENT_URL throws
at construction — proximate to the misconfiguration — instead of later inside
ForestOAuthProvider during run()/start(). Add a verifyAccessToken test for the
fallback path (no agentUrl -> environment api_endpoint).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ForestMCPServer is now the sole validator/normalizer of agentUrl; the
internal ForestOAuthProvider accepts a pre-normalized value instead of
re-running normalizeAgentUrl. Move the validation/normalization unit tests
(invalid inputs, trailing slash, http/https, path preservation) into a
dedicated normalize-agent-url.test.ts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the agentUrl option from agent.mountAiMcpServer() — mounted mode will
dispatch tool calls to the agent in-process (follow-up), making a callback URL
unnecessary. agentUrl stays available for the standalone MCP server via
FOREST_AGENT_URL / ForestMCPServerOptions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ter override

Factor HttpRequester's success-deserialize and error-mapping tail into exported
buildAgentHttpError/deserializeAgentBody helpers (HTTP path unchanged), and let
createRemoteAgentClient accept a pre-built httpRequester. Groundwork for an
in-process transport that reuses the exact same AgentHttpError shape (so the
approval-gate detection stays identical).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add InProcessDispatcher, which runs an agent-client request through the agent's
own /forest Koa stack in-memory via light-my-request (no socket) and returns a
superagent-shaped {status, body, text}. FrameworkMounter.getInProcessDispatcher
rebuilds the handler on every (re)mount (stable object, mutable target — same
pattern as McpMiddleware); the /forest prefix is fixed since agent-client
hardcodes those paths. Not yet wired to the MCP server (next increment).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the MCP server is mounted in an agent, tool calls now reach the
agent's data layer in-process (via the agent's InProcessDispatcher)
instead of over the public api_endpoint, removing the need for agentUrl
in mounted mode. agentUrl stays for standalone (FOREST_AGENT_URL).

Tools now receive a single ToolContext; buildClient builds an
InProcessHttpRequester (reusing agent-client's shared parse helpers so
the deserialized result and AgentHttpError shape stay byte-identical to
the HTTP path) when a dispatcher is present.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Guards against a regression to the HTTP path: a real OAuth-authed
tools/call must reach the agent through the InProcessDispatcher, across
every host framework.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@qltysh

qltysh Bot commented Jul 7, 2026

Copy link
Copy Markdown

1 new issue

Tool Category Rule Count
qlty Duplication Found 17 lines of identical code in 2 locations (mass = 68) 1

@Scra3 Scra3 changed the base branch from feat/mcp-server-internal-agent-url to main July 7, 2026 18:11
@qltysh

qltysh Bot commented Jul 7, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

⬆️ Merging this pull request will increase total coverage on main by 0.06%.

Modified Files with Diff Coverage (20)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/associate.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/execute-action.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/utils/agent-caller.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/list.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/update.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/get-action-form.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/agent.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/forest-oauth-provider.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent-client/src/http-requester.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/create.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/delete.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/describe-collection.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/dissociate.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent-client/src/index.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/framework-mounter.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/server.ts100.0%
Coverage rating: A Coverage rating: A
packages/mcp-server/src/tools/list-related.ts100.0%
New Coverage rating: A
packages/mcp-server/src/in-process-http-requester.ts100.0%
New Coverage rating: A
packages/agent/src/mcp-in-process-dispatcher.ts100.0%
New Coverage rating: A
packages/mcp-server/src/utils/normalize-agent-url.ts100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

Comment thread packages/agent/src/agent.ts

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

protected async remount(router: Router): Promise<void> {
for (const task of this.onEachStart) await task(router); // eslint-disable-line no-await-in-loop

getInProcessDispatcher() returns the InProcessDispatcher immediately, but its handler is only populated by an onEachStart hook that runs later inside mount()/remount(). Any MCP tools/call that arrives before those hooks fire hits a dispatcher whose handler is still null and throws it is not mounted yet. During restart, the handler can still reference the previous router and serve stale /forest routes/schema. Consider resetting the handler to null at the start of remount() and/or blocking in-process calls until the hook has run.

  protected async remount(router: Router): Promise<void> {
+    this.inProcessDispatcher.setHandler(null);
    for (const task of this.onEachStart) await task(router); // eslint-disable-line no-await-in-loop
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent/src/framework-mounter.ts around lines 73-74:

`getInProcessDispatcher()` returns the `InProcessDispatcher` immediately, but its handler is only populated by an `onEachStart` hook that runs later inside `mount()`/`remount()`. Any MCP `tools/call` that arrives before those hooks fire hits a dispatcher whose handler is still `null` and throws `it is not mounted yet`. During restart, the handler can still reference the previous router and serve stale `/forest` routes/schema. Consider resetting the handler to `null` at the start of `remount()` and/or blocking in-process calls until the hook has run.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional — not resetting to null. The onEachStart hook is registered during initializeMcpServer(), which runs inside buildRouterAndSendSchema() before mount() iterates onEachStart, so the handler is set before the server accepts traffic; the "not mounted yet" throw is only reachable before start() completes (no listener yet).

On restart(), setHandler is a single atomic reference swap: in-flight dispatches keep the old Koa app via closure and new calls get the new router — the same ride-out semantics an in-flight HTTP request has during a remount. Resetting to null at the top of remount() would instead open a window where in-process calls throw "not mounted yet" mid-restart, which is strictly worse. Covered by the "reflects a handler swap (remount)" test.

// Superagent-shaped so agent-client's HttpRequester helpers parse it identically.
export type InProcessResponse = { status: number; body: unknown; text: string };

function toStringQuery(query: Record<string, unknown>): Record<string, string> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/mcp-in-process-dispatcher.ts:16

toStringQuery() stringifies every query value with String(value), so array and object query parameters are corrupted before reaching the Koa router. For example, { ids: ['1','2'] } becomes ids=1,2 and { filter: { ... } } becomes filter=[object Object], which does not match the normal HTTP encoding used by HttpRequester.query(). Any MCP tool or in-process caller that sends array/object query params will silently hit the agent with different semantics and get wrong results. Consider using URLSearchParams or qs-style serialization so multi-value and nested params encode the same way they would over real HTTP.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent/src/mcp-in-process-dispatcher.ts around line 16:

`toStringQuery()` stringifies every query value with `String(value)`, so array and object query parameters are corrupted before reaching the Koa router. For example, `{ ids: ['1','2'] }` becomes `ids=1,2` and `{ filter: { ... } }` becomes `filter=[object Object]`, which does not match the normal HTTP encoding used by `HttpRequester.query()`. Any MCP tool or in-process caller that sends array/object query params will silently hit the agent with different semantics and get wrong results. Consider using `URLSearchParams` or `qs`-style serialization so multi-value and nested params encode the same way they would over real HTTP.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are always scalars in practice. agent-client builds the query via QuerySerializer, which emits only strings/numbers/booleans — filters/sort are JSON.stringify'd to strings, and fields[...] projections are comma-joined deliberately (the Ruby agent's Rack collapses repeated params to the last value). So arrays/objects never reach toStringQuery, and String(value) matches the wire encoding for the shapes actually sent. Switching to URLSearchParams multi-value would in fact diverge from the comma-join contract the Ruby backend expects. undefined is dropped, same as superagent.

Comment thread packages/mcp-server/src/in-process-http-requester.ts
- Bound in-process dispatch with a timeout (default 10s, honors
  maxTimeAllowed) so a hung agent handler can't hang a tool call forever,
  matching the HTTP path's superagent timeout.
- Thread a logger into InProcessDispatcher; distinguish an expected empty
  (204) body from a corrupted non-JSON 2xx body, which is now logged
  instead of silently returning undefined.
- Pin the cross-package contract: InProcessDispatcher now
  `implements InProcessAgentDispatcher` (exported from mcp-server), so
  drift fails to compile at the definition, not just at one wiring line.
- Make ToolContext.collectionNames required and default it once in the
  server instead of `= []` in all 10 tool factories.
- Document on mountAiMcpServer() + log at startup that in-process tool
  calls bypass any middleware mounted in front of the agent.

Tests: approval-gate 403 error-shape parity, maxTimeAllowed forwarding,
timeout, 204/non-JSON body handling, payload+content-type forwarding
through a real handler, buildClientWithActions dispatcher wiring, and a
status assertion on the in-process integration test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
body: responseBody,
text,
} = await this.dispatcher.request({
method,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/in-process-http-requester.ts:45

InProcessHttpRequester.query() forwards path directly to the dispatcher without calling HttpRequester.escapeUrlSlug(), so collection names, record ids, or action endpoints containing reserved URI characters (spaces, +, ?) are sent unescaped. This causes the agent's router to fail or hit the wrong route for valid names — for example /forest/Foo Bar or /forest/orders/1/relationships/line?items are sent literally, with ?items truncated as a query string. Consider applying HttpRequester.escapeUrlSlug() to path before dispatching, matching the HTTP path's behavior.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/mcp-server/src/in-process-http-requester.ts around line 45:

`InProcessHttpRequester.query()` forwards `path` directly to the dispatcher without calling `HttpRequester.escapeUrlSlug()`, so collection names, record ids, or action endpoints containing reserved URI characters (spaces, `+`, `?`) are sent unescaped. This causes the agent's router to fail or hit the wrong route for valid names — for example `/forest/Foo Bar` or `/forest/orders/1/relationships/line?items` are sent literally, with `?items` truncated as a query string. Consider applying `HttpRequester.escapeUrlSlug()` to `path` before dispatching, matching the HTTP path's behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real but rare, left as a known limitation. escapeUrlSlug does encodeURI(path).replace(/([+?*])/g, '\\$1') — the \+?* backslash convention targets the agent's koa/path-to-regexp router + the HTTP wire, and can't be replayed through light-my-request (its WHATWG new URL parsing treats \ as /), so applying it blindly would break more than it fixes. Spaces/non-ASCII encode identically both ways; the only divergence is slug names containing literal + ? * (e.g. a record id with ?), which are already fragile over HTTP. Deliberately not changing this under this PR to avoid a riskier fix — noting it as a known edge.

alban bertolini and others added 2 commits July 8, 2026 08:19
…dyParser

Mirrors FrameworkMounter.getInProcessDispatcher()'s wrapping and asserts a
mutating POST body reaches the route parsed — the driver router's
`router.use(bodyParser)` (agent.ts) is included in `.routes()`, so mounted
mutating tool calls (create/update/…) get a parsed body, same as HTTP.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…HttpRequester

No MCP tool exports CSV / calls stream(), so the override guarded a path that
cannot happen. Removed it (and its test) per YAGNI; query() is the only method
the in-process requester needs to override.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread packages/mcp-server/src/in-process-http-requester.ts
Comment thread packages/agent/src/agent.ts Outdated
Comment on lines +247 to +249
* Tool calls reach your data in-process (no socket), so they bypass any middleware you mounted
* in front of the agent (rate limiting, request logging, WAF): only the agent's own JWT auth and
* permission checks run on them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double check with someone more competent to see if this is OK

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's accurate, and it's OK — here's the double-check.

What the note describes. When the MCP server is mounted in the agent, a tool call reaches the data layer via the in-process dispatcher, which wraps the agent's own driver router (new Router({ prefix: '/forest' }).use(driverRouter.routes())). That stack still includes the agent's cors + bodyParser + JWT auth + permission checks — it just doesn't re-run any middleware the host app registered around the mount (app.use(rateLimiter), request logging, a WAF/proxy layer).

Why it's not a security concern. The external entry point is unchanged: an inbound POST /mcp still traverses the host app's middleware and the OAuth bearer check before any tool runs. Only the internal hop that the already-authenticated tool makes to /forest is in-process — and it still carries the agent's JWT + permissions. So nothing becomes reachable without auth; there's no bypass of an access gate.

The one real consequence (which the note is there to flag): previously the tool call went out to the public api_endpoint and came back in through the front door, so host-level middleware saw it a second time. In-process, that second pass is gone — a client who counts or logs /forest traffic at the host layer won't see MCP-driven data access there anymore. That's the deliberate trade-off of keeping the hop internal (it's the whole point of the feature: no public round-trip). It's verified by the dispatches the tool call to the agent in-process integration test (all 8 host frameworks) + the preserves router-level middleware (bodyParser) test.

If you'd rather, I can soften the wording to make explicit that agent auth/permissions still apply (so "bypass" doesn't read as "unauthenticated") — e.g. "bypass host-app middleware; the agent's own auth/permission checks still run." Happy to push that.

@ShohanRahman

Copy link
Copy Markdown
Contributor

Comments describe what, not why — violates project CLAUDE.md style

  • packages/mcp-server/src/utils/agent-caller.ts:43 — // Mounted in an agent: reach it in-process. Standalone: ... restates the ternary directly below it.
  • packages/mcp-server/src/tool-context.ts:9 — // Present only when... duplicates what the ? optional plus the server.ts JSDoc already say.
  • mcp-in-process-dispatcher.ts:19 — // Matches superagent's HTTP path, which times out after 10s (HttpRequester.query/stream) — the /stream reference is misleading (stream is not dispatched in-process). Trim to HttpRequester.query.

@ShohanRahman

Copy link
Copy Markdown
Contributor

Single source of truth for the dispatcher contract

The structural InProcessAgentDispatcher interface (mcp-server) re-declares the same shape as InProcessRequest/InProcessResponse (agent), with no mechanical sync. The agent already imports InProcessAgentDispatcher from mcp-server, so exporting named InProcessDispatchRequest/Response types from mcp-server and importing them in the agent would eliminate the duplication with zero new dependency-graph edges. Drift is currently caught only by implements on existing fields, not by additions.

…st middleware is skipped

Addresses Shohan's review: reword so "bypass" doesn't read as "unauthenticated".
In-process tool calls skip host-app middleware (rate limit/logging/WAF) but the
agent's own JWT auth + permission checks still run, and the inbound MCP request
is unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
forestServerClient,
enabledTools: this.mcpEnabledTools,
basePath: this.mcpBasePath,
agentDispatcher: this.getInProcessDispatcher(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/agent.ts:432

MCP tool calls dispatched via agentDispatcher: this.getInProcessDispatcher() bypass the agent's IP-whitelist enforcement. IpWhitelist authorizes /forest requests using x-forwarded-for or context.request.ip, but the in-process dispatch path never forwards the original MCP request's IP or forwarded headers, so every tool call is evaluated against the injector's local address rather than the real client. With IP whitelisting enabled, legitimate MCP requests are rejected as 403, and the whitelist no longer enforces the actual caller IP. Consider forwarding the original request's IP and x-forwarded-for headers into the in-process dispatch context so the whitelist check sees the real client.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent/src/agent.ts around line 432:

MCP tool calls dispatched via `agentDispatcher: this.getInProcessDispatcher()` bypass the agent's IP-whitelist enforcement. `IpWhitelist` authorizes `/forest` requests using `x-forwarded-for` or `context.request.ip`, but the in-process dispatch path never forwards the original MCP request's IP or forwarded headers, so every tool call is evaluated against the injector's local address rather than the real client. With IP whitelisting enabled, legitimate MCP requests are rejected as 403, and the whitelist no longer enforces the actual caller IP. Consider forwarding the original request's IP and `x-forwarded-for` headers into the in-process dispatch context so the whitelist check sees the real client.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed — this is real. IpWhitelist.checkIp (routes/security/ip-whitelist.ts:27) reads x-forwarded-for ?? context.request.ip; the in-process dispatch injects via light-my-request with no x-forwarded-for and a default remote address of 127.0.0.1. So with the whitelist enabled and loopback not in the rules, every mounted MCP tool call would 403.

Worth noting the pre-existing behaviour: over HTTP the /forest hop already carried the mcp-server's source IP (or the proxy's x-forwarded-for), not the end MCP client's — so the whitelist never saw the real MCP caller. The in-process change turns that into a hard 403 (127.0.0.1) instead of "checks the wrong IP".

Options, and I think this needs a product/security call rather than a silent fix:

  1. Forward the inbound /mcp request's IP + x-forwarded-for down through the dispatch so the whitelist sees the real client (cleanest, but plumbs the IP across mcp-server → agent-client → dispatcher).
  2. Exempt in-process /forest calls from IP whitelist — the /mcp entry is already OAuth-gated, so the check is redundant on the internal hop (but it's silently skipping a security control).
  3. Document mounted-MCP + IP-whitelist as incompatible for now.

Leaning (1). @albanb — your call on which way to take it; I can implement whichever.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: went the other way from my earlier "leaning (1)". We do not forward the IP inward (impossible for the executor anyway — no client IP there). Instead, in #1744:

  • the agent's IP whitelist exempts authenticated internal callers (in-process dispatch flag / step-execution executor JWT from a loopback socket) — never trusting x-forwarded-for;
  • the whitelist is enforced at the /mcp perimeter (real client IP), fail-closed.

The in-process flag lands in a follow-up commit here once #1744 merges. So this thread is addressed by #1744.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Décision finale (remplace mon message précédent). Après discussion produit/sécu (thread Slack) : on ne rend pas le MCP soumis à l'IP whitelist pour l'instant.

Constat partagé : c'est bien un trou dans la raquette (restreindre l'accès data par IP via le front devrait valoir aussi pour le MCP), mais ~11 projets seulement utilisent la feature et aucun ne l'a remonté → on attend qu'un client le demande.

Conséquences, assumées et suivies dans PRD-752 (décision + implémentation) et PRD-753 (doc) :

Ce finding est donc acté comme limitation connue, pas corrigé dans cette PR. Rien à changer ici côté in-process.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…ating comments

Review (Shohan):
- Export named InProcessDispatchRequest/InProcessDispatchResponse from mcp-server
  and import them in the agent's InProcessDispatcher instead of re-declaring the
  shape — one source of truth, no manual sync (adds no dependency-graph edge, the
  agent already imports from mcp-server).
- Remove comments that restate the code (agent-caller.ts branch note,
  tool-context.ts agentDispatcher note) and fix the misleading `/stream` mention
  in the timeout note (stream isn't dispatched in-process) per CLAUDE.md style.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Scra3

Scra3 commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

@ShohanRahman both addressed in 9db061606:

Comment style (what-not-why). Removed the two comments that restate the code:

  • agent-caller.ts — the "Mounted in an agent / Standalone" note above the ternary (the ternary says it).
  • tool-context.ts — the agentDispatcher "present only when mounted" note (the ? + the server.ts JSDoc cover it).
  • mcp-in-process-dispatcher.ts — kept the timeout note (it explains why 10s = match the HTTP path) but trimmed the misleading HttpRequester.query/streamHttpRequester.query, since stream isn't dispatched in-process.

Single source of truth for the dispatcher contract. Exactly your suggestion: InProcessDispatchRequest / InProcessDispatchResponse are now named, exported from @forestadmin/mcp-server, and the agent's InProcessDispatcher imports them (aliased to its local InProcessRequest/InProcessResponse) instead of re-declaring the shape. The shape now lives once; a field addition can't silently drift (no new dependency edge — the agent already imported InProcessAgentDispatcher). Build + tests green (mcp-server 626, agent dispatcher suite).

…, not exports

buildAgentHttpError and deserializeAgentBody were free functions exported from
agent-client only so InProcessHttpRequester (mcp-server) could rebuild the exact
same AgentHttpError shape on the in-process transport. Since that requester
already extends HttpRequester, fold them into protected methods (buildError /
deserialize) and drop the two public exports — the shared error/deserialize
contract now travels through the subclass, not agent-client's public surface.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ShohanRahman

Copy link
Copy Markdown
Contributor

Weak assertion in the dispatcher-present test

agent-caller.test.ts:151-157 only uses .not.toThrow(). It should also assert the client was wired for in-process:
expect(mockCreateRemoteAgentClient).toHaveBeenCalledWith(
expect.objectContaining({ httpRequester: expect.any(InProcessHttpRequester), url: '' }),
);
Otherwise the test passes even if the dispatcher was silently dropped.

@ShohanRahman

Copy link
Copy Markdown
Contributor

The stream()/CSV story is now internally contradictory

  • The PR description says CSV export "throws explicitly" — but commit 8abefd1 removed the throwing stream() override.
  • InProcessHttpRequester now inherits the base stream(), which uses the sentinel URL http://in-process.agent. If ever called, it fires a real network request to a non-existent host → ~10s hang → confusing error. It does not throw.
  • Actions: fix the false PR description; fix the inaccurate comment at in-process-http-requester.ts:19 ("Base url is unused" — it is used by the inherited stream()); recommended: restore the one-line throwing stream() override for cheap insurance.

…atcher test

Review (Shohan):
- InProcessHttpRequester inherited the base stream(), which would fire a real
  request at the sentinel host (http://in-process.agent) → ~10s hang. Restore a
  throwing override so CSV export fails fast in-process (matches the PR
  description's "throws explicitly") and fix the stale "base url is unused"
  comment (it is a never-resolved sentinel now that both query() and stream() are
  overridden). Covered by a new stream()-throws test.
- Strengthen the dispatcher-present test: assert createRemoteAgentClient is wired
  with an InProcessHttpRequester + empty url, instead of only .not.toThrow() (a
  silently dropped dispatcher would have passed before).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Scra3

Scra3 commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

@ShohanRahman les deux points traités dans ac2d6bb6d :

1. Assertion faible (dispatcher-present test). Le test .not.toThrow() est remplacé : il appelle maintenant buildClient(request, agentDispatcher) et vérifie que createRemoteAgentClient est bien câblé en in-process —

expect(mockCreateRemoteAgentClient).toHaveBeenCalledWith(
  expect.objectContaining({ httpRequester: expect.any(InProcessHttpRequester), url: '' }),
);

Un dispatcher silencieusement perdu échoue désormais le test.

2. stream()/CSV contradictoire. J'ai suivi ta reco (restaurer l'override) — c'est le choix qui rend tout cohérent d'un coup :

  • override stream() restauré → throw immédiat au lieu du hang ~10s sur le host sentinelle ; la description PR (« CSV export … throws explicitly ») redevient vraie, pas besoin de la corriger.
  • commentaire :16 corrigé : ce n'est plus « base url is unused » mais une sentinelle jamais résolue (query() dispatche in-process, stream() throw).
  • nouveau test qui couvre le throw de stream().

…+ silent non-JSON 2xx

Review (Shohan):
- injectWithTimeout raced inject() against a timer with no handler on the
  injection promise. If the timeout won and inject() later rejected, it surfaced
  as an unhandledRejection. Attach a catch() that logs the late settlement before
  racing, so the losing promise is always observed.
- parseBody returned undefined for any non-JSON body (warn only). On a 2xx that
  handed the tool no data and no error. Now throw on a non-JSON 2xx (misbehaving
  middleware — HTML/redirect), and include a truncated body preview in both the
  thrown error and the 4xx+ warn log. 4xx+ still returns undefined (buildError
  falls back to the raw text).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
);
});

const injection = inject(this.handler as RequestListener, options);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/mcp-in-process-dispatcher.ts:82

When the agent handler throws before the timeout fires, injectWithTimeout logs it as "settled after the ${timeoutMs}ms timeout" even though the timeout never elapsed. This emits a bogus timeout warning that obscures the real handler failure. The .catch on injection fires unconditionally on rejection, so any normal-path exception is mislabeled as a late settlement. Consider attaching the late-settlement logger only after the timeout actually wins the race.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent/src/mcp-in-process-dispatcher.ts around line 82:

When the agent handler throws before the timeout fires, `injectWithTimeout` logs it as `"settled after the ${timeoutMs}ms timeout"` even though the timeout never elapsed. This emits a bogus timeout warning that obscures the real handler failure. The `.catch` on `injection` fires unconditionally on rejection, so any normal-path exception is mislabeled as a late settlement. Consider attaching the late-settlement logger only after the timeout actually wins the race.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants