-
Notifications
You must be signed in to change notification settings - Fork 12
feat(mcp-server): dispatch mounted tool calls to the agent in-process #1743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8c7b962
c476514
c26d56b
0507fb9
0327a3c
a1bcaf1
4ba880e
d6a46e2
fec6e70
ae3a5c2
e30cfe4
8abefd1
929a1c8
9db0616
9a1a1c5
ac2d6bb
c801d5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -244,6 +244,10 @@ export default class Agent<S extends TSchema = TSchema> extends FrameworkMounter | |
| * Enable MCP (Model Context Protocol) server support. | ||
| * This allows AI assistants to interact with your Forest Admin data. | ||
| * | ||
| * Tool calls reach your data in-process (no socket), so they skip any host-app middleware you | ||
| * mounted in front of the agent (rate limiting, request logging, WAF). The agent's own JWT auth | ||
| * and permission checks still run on them, and the inbound MCP request itself is unaffected. | ||
| * | ||
| * @see {@link https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/ai/mcp-server} | ||
| * @example | ||
| * agent.mountAiMcpServer(); | ||
|
|
@@ -425,12 +429,17 @@ export default class Agent<S extends TSchema = TSchema> extends FrameworkMounter | |
| forestServerClient, | ||
| enabledTools: this.mcpEnabledTools, | ||
| basePath: this.mcpBasePath, | ||
| agentDispatcher: this.getInProcessDispatcher(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium MCP tool calls dispatched via 🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — this is real. Worth noting the pre-existing behaviour: over HTTP the Options, and I think this needs a product/security call rather than a silent fix:
Leaning (1). @albanb — your call on which way to take it; I can implement whichever.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 in-process flag lands in a follow-up commit here once #1744 merges. So this thread is addressed by #1744.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }); | ||
|
|
||
| const httpCallback = await mcpServer.getHttpCallback(); | ||
| const isMcpRoute = makeIsMcpRoute(this.mcpBasePath); | ||
|
|
||
| mcpLogger('Info', 'Server initialized successfully'); | ||
| mcpLogger( | ||
| 'Info', | ||
| 'Tool calls dispatch in-process and skip any middleware mounted in front of the agent', | ||
| ); | ||
|
|
||
| return { httpCallback, isMcpRoute }; | ||
| } catch (error) { | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium agent-nodejs/packages/agent/src/framework-mounter.ts Lines 73 to 74 in fec6e70
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:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional — not resetting to On |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| import type { Logger } from '@forestadmin/datasource-toolkit'; | ||
| import type { | ||
| InProcessAgentDispatcher, | ||
| InProcessDispatchRequest, | ||
| InProcessDispatchResponse, | ||
| } from '@forestadmin/mcp-server'; | ||
| import type { RequestListener } from 'http'; | ||
|
|
||
| import inject, { type InjectOptions, type InjectPayload } from 'light-my-request'; | ||
|
|
||
| // Single-sourced from mcp-server (the dispatcher contract) — no re-declaration to drift. | ||
| export type InProcessRequest = InProcessDispatchRequest; | ||
| export type InProcessResponse = InProcessDispatchResponse; | ||
|
|
||
| // Matches superagent's HTTP path, which times out after 10s (HttpRequester.query). | ||
| const DEFAULT_TIMEOUT_MS = 10_000; | ||
|
|
||
| function toStringQuery(query: Record<string, unknown>): Record<string, string> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are always scalars in practice. agent-client builds the query via |
||
| const result: Record<string, string> = {}; | ||
|
|
||
| for (const [key, value] of Object.entries(query)) { | ||
| if (value !== undefined) result[key] = String(value); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Dispatches an agent-client request into the agent's own Koa stack in-memory (no socket), so a | ||
| * mounted MCP server's tool calls run through the real auth/permission/logging middleware. | ||
| * `setHandler` is refreshed on every (re)mount, so a captured reference never goes stale. | ||
| */ | ||
| export default class InProcessDispatcher implements InProcessAgentDispatcher { | ||
| private handler: RequestListener | null = null; | ||
|
|
||
| constructor(private readonly logger?: Logger) {} | ||
|
|
||
| setHandler(handler: RequestListener | null): void { | ||
| this.handler = handler; | ||
| } | ||
|
|
||
| async request({ | ||
| method, | ||
| path, | ||
| headers, | ||
| query, | ||
| payload, | ||
| timeoutMs = DEFAULT_TIMEOUT_MS, | ||
| }: InProcessRequest): Promise<InProcessResponse> { | ||
| if (!this.handler) { | ||
| throw new Error('Cannot dispatch to the agent in-process: it is not mounted yet.'); | ||
| } | ||
|
|
||
| // No socket to abort, so a hung agent handler would hang the tool call forever without this. | ||
| const response = await this.injectWithTimeout( | ||
| { | ||
| method: method.toUpperCase() as InjectOptions['method'], | ||
| url: path, | ||
| headers, | ||
| ...(query && { query: toStringQuery(query) }), | ||
| ...(payload !== undefined && { payload: payload as InjectPayload }), | ||
| }, | ||
| timeoutMs, | ||
| ); | ||
|
|
||
| return { | ||
| status: response.statusCode, | ||
| body: this.parseBody(response.payload, response.statusCode), | ||
| text: response.payload, | ||
| }; | ||
| } | ||
|
|
||
| private async injectWithTimeout(options: InjectOptions, timeoutMs: number) { | ||
| let timer: NodeJS.Timeout | undefined; | ||
| const timeout = new Promise<never>((_, reject) => { | ||
| timer = setTimeout( | ||
| () => reject(new Error(`In-process dispatch timed out after ${timeoutMs}ms`)), | ||
| timeoutMs, | ||
| ); | ||
| }); | ||
|
|
||
| const injection = inject(this.handler as RequestListener, options); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium When the agent handler throws before the timeout fires, 🚀 Reply "fix it for me" or copy this AI Prompt for your agent: |
||
| // If the timeout wins the race, `injection` is left unobserved; swallow+log a late settlement so | ||
| // a handler that rejects after the timeout can't surface as an unhandledRejection. | ||
| injection.catch(error => | ||
| this.logger?.( | ||
| 'Warn', | ||
| `In-process handler settled after the ${timeoutMs}ms timeout: ${error}`, | ||
| ), | ||
| ); | ||
|
|
||
| try { | ||
| return await Promise.race([injection, timeout]); | ||
| } finally { | ||
| if (timer) clearTimeout(timer); | ||
| } | ||
| } | ||
|
|
||
| private parseBody(payload: string, status: number): unknown { | ||
| if (payload === '') return undefined; | ||
|
|
||
| try { | ||
| return JSON.parse(payload); | ||
| } catch { | ||
| const preview = payload.length > 200 ? `${payload.slice(0, 200)}…` : payload; | ||
| const message = `In-process dispatch received a non-JSON ${status} response body: ${preview}`; | ||
|
|
||
| // A 2xx must carry a JSON:API body here; a non-JSON success body means misbehaving middleware | ||
| // (HTML, redirect) — fail loudly rather than hand the tool `undefined`. On 4xx+ the raw text | ||
| // is preserved by the error path (buildError falls back to parseJson(text)), so drop the body. | ||
| if (status < 400) throw new Error(message); | ||
|
|
||
| this.logger?.('Warn', message); | ||
|
|
||
| return undefined; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import type { RequestListener } from 'http'; | ||
|
|
||
| const mockInject = jest.fn(); | ||
|
|
||
| jest.mock('light-my-request', () => ({ | ||
| __esModule: true, | ||
| default: (...args: unknown[]) => mockInject(...args), | ||
| })); | ||
|
|
||
| // eslint-disable-next-line import/first | ||
| import InProcessDispatcher from '../src/mcp-in-process-dispatcher'; | ||
|
|
||
| describe('InProcessDispatcher — injection rejection safety', () => { | ||
| it('observes a late injection rejection (logs it) instead of leaking an unhandled rejection', async () => { | ||
| const logger = jest.fn(); | ||
| // inject() rejects after the timeout has already won the race — the losing promise must not | ||
| // become an unhandledRejection. | ||
| mockInject.mockReturnValue( | ||
| new Promise((_resolve, reject) => { | ||
| setTimeout(() => reject(new Error('inject blew up')), 30); | ||
| }), | ||
| ); | ||
| const dispatcher = new InProcessDispatcher(logger); | ||
| dispatcher.setHandler((() => {}) as unknown as RequestListener); | ||
|
|
||
| await expect( | ||
| dispatcher.request({ method: 'get', path: '/x', headers: {}, timeoutMs: 10 }), | ||
| ).rejects.toThrow(/timed out after 10ms/); | ||
|
|
||
| await new Promise(resolve => { | ||
| setTimeout(resolve, 40); | ||
| }); | ||
|
|
||
| expect(logger).toHaveBeenCalledWith( | ||
| 'Warn', | ||
| expect.stringContaining('settled after the 10ms timeout'), | ||
| ); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.