refactor(agent): serve root-mounted handlers through one registry

[nbouliol]

Stacked on #1874. Touches only packages/agent, so it can be reviewed independently of the agent-bff PRs below it in the stack.

Why

McpMiddleware held one callback and one matcher. That was enough while the MCP server was the only thing the agent served at the root of a host application, next to its own /{prefix}/forest router. The embedded BFF needs the same treatment on /bff/*, and two singletons wired into five mount points is how mount points drift apart.

What

RootMiddleware keeps handlers by name, each with the matcher declaring which urls it claims. Unclaimed urls fall through to the host untouched, exactly as before. A handler and its matcher travel together as RootHandler, so there is no default pattern set to claim host urls by accident: makeIsMcpRoute(basePath) is the only matcher MCP is ever registered with.

Nothing deregisters a handler today — stop() leaves it in place — but if one ever is, its urls fall through to the host's 404, which reads exactly like a route that was never registered. That, and two overlapping matchers shadowing each other, is why dispatch logs a Debug line naming the handler that claimed the url.

The five mount points (mountOnExpress, mountOnFastify, mountOnKoa, mountOnNestJs, and the connect callback mountOnStandaloneServer uses) change only in the type they call.

One behavior does tighten: on the connect and Express paths the MCP callback used to be handed every request and trusted to filter itself, while only the Koa path applied the matcher. The matcher now applies everywhere. makeIsMcpRoute is what the MCP server filters on internally, so the claimed set is the same.

Nothing registers a second handler yet — the BFF arrives in the next PR.

Tests

82 suites, 1502 tests in @forestadmin/agent. The old mcp-middleware suite becomes root-middleware, plus coverage for what is new: dispatch to the right handler among several, and a null callback while nothing is registered so the host keeps its untouched path.

... (truncated)

Note

Serve root-mounted handlers through one RootMiddleware registry in agent

• Replaces the MCP-specific McpMiddleware with a general RootMiddleware that registers named handlers by URL matcher and dispatches the first match, falling through to the host or agent when no handler claims the URL
• Agent.initializeMcpServer now returns a single RootHandler (callback plus matcher) instead of separate values; FrameworkMounter.setMcpCallback registers it by name in the shared registry
• All framework mount paths (Express, Fastify, Koa, NestJS, standalone) mount the generic root middleware at the host root before the agent's prefixed forest routes
• Behavioral Change: root requests are now dispatched to the first registered handler whose RouteMatcher accepts the URL; unclaimed requests fall through to the agent router as before — FrameworkMounter.setMcpCallback and Agent.getRouter callers that passed separate callback/matcher arguments must switch to the combined RootHandler

^{Macroscope summarized f73332f.}

[linear-code]

PRD-1076

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (3)

Rating	File	% Diff	Uncovered Line #s
	packages/agent/src/agent.ts	100.0%
	packages/agent/src/framework-mounter.ts	100.0%
	packages/agent/src/root-middleware.ts	100.0%
	Total	100.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.

[Tonours]

Spec (PRD-1076): this is the first half of step 9 — the generic registry — with the /bff wiring deferred to #1876, which the PR says plainly. No objection to the split. One deviation inside the half that did land: step 9 asks for the matcher to be copied from makeIsMcpRoute, in these words —

Copy the matcher from makeIsMcpRoute (mcp-paths.ts:47-51): it already handles the query string and the segment boundary, and does not match /bffx.

— and the new mcp-routes.ts does the opposite, keeping the old startsWith form. First finding below.

Iso-behaviour: confirmed. This was the thing worth checking, since the old getExpressMiddleware() did not filter at all and the new one consults a matcher first. They are equivalent, because agent.ts builds the gate as makeIsMcpRoute(this.mcpBasePath) and the MCP server's own internal gate is makeIsMcpRoute(this.basePath) from the same value — same factory, same normalized prefix, and normalizeMountPath is idempotent. Express, Fastify and NestJS all reach the middleware with an unmodified req.url, so both gates see the same string. Verified across all five mount targets plus the nested standalone path, where getCallback() returning null on an empty registry preserves the pre-PR "no MCP, straight to the handler" path exactly.

Two things are actually better than the merge base and worth saying: the Koa gate now keys on handlerFor(url) !== null rather than consulting a matcher even when the callback was null, so a host request no longer takes a pointless trip through expressToKoa; and getCallback() is null-when-empty rather than always allocating.

One correction to something a reader might expect to be a finding and is not: RootMiddleware dispatches on req.url while the Koa gate reads ctx.url, but Koa's ctx.url delegates to req.url through a getter/setter pair, so they cannot diverge at runtime — only in a unit test, which is why makeCtx now sets both.

[Tonours]

Claude Opus 5 (claude-opus-5): Should fix

This matcher is dead today, over-claims when it is reached, and is the one thing step 9 explicitly told this PR not to write.

startsWith with no segment boundary and no query stripping, over the bare no-prefix path set. What it claims that it should not:

• /.well-known/ unconditionally — so a host's ACME renewal on /.well-known/acme-challenge/… gets the MCP app's 404 instead of the host's handler. buildMcpPaths narrows well-known to /.well-known/oauth-authorization-server<prefix> when a prefix is set, precisely so a prefixed deployment does not claim the host's root metadata.
• /oauth/ unconditionally — a host's own /oauth/callback login leg is answered by the agent. The customer sees their login break with an MCP-shaped error and nothing in the agent logs mentions /oauth/.
• /mcp without a boundary — /mcp-dashboard and /mcpanel are claimed. The canonical matcher guards exactly this and says so: "on a segment boundary, so /mcp?x=1 still matches and /ai/mcp does not shadow /ai/mcp-dashboard".

Not reachable at this SHA — setMcpCallback's only two callers always pass mcpIsMcpRoute, and when MCP is off the callback is null so set() takes the delete branch. But the merge base's identical crude default fed only the Koa gate, and getExpressMiddleware ignored it outright; this PR promotes it to the default for all five mount targets, so the first caller written without a matcher gets a root middleware that swallows host traffic.

Cheapest fix, and it removes the duplication rather than fixing it twice: delete this module and make routeMatcher non-optional on setMcpCallback. Both callers already comply, so it is a compile-time guarantee instead of a silent wrong default, and mcp-paths.ts stays the sole owner. It also collapses RouteMatcher and McpRouteMatcher, the same signature declared twice.

Found by three independent passes.

[nbouliol]

Fixed by deleting the module: the matcher now travels with the callback as RootHandler { callback, matches }, built in initializeMcpServer from makeIsMcpRoute(basePath) and threaded through setMcpCallback — registering a root handler without its matcher no longer compiles, and McpRouteMatcher/RouteMatcher collapse into one type in types.ts.

[Tonours]

Claude Opus 5 (claude-opus-5): Should fix

Two handlers with overlapping matchers shadow each other with no error at registration and no trace at dispatch, and there is no way to detect it today — no registration diagnostic, no dispatch log, no test.

set() keys on name and never inspects matches; handlerFor returns the first insertion-ordered hit. Inert at this SHA, since 'mcp' is the only registration — but this PR exists so that a second handler can be added, and the overlap is not hypothetical: with no basePath, the MCP matcher claims the whole of /oauth/ and the whole of /.well-known/, so a BFF wanting any /oauth/... path would receive zero requests.

What makes it a silent failure rather than a bug: set() returns void and succeeds, startup logs mount normally, and at dispatch the request is answered — by the wrong handler, with that handler's own 404 or 401 body. The developer reads "route not registered" and goes looking in the wrong package; the cause is a Map iteration order in this file.

Registration-time overlap detection is undecidable for opaque predicates, so don't attempt it — make dispatch observable instead. RootMiddleware takes no logger today and FrameworkMounter has one to pass; one debug line naming the winning handler turns this from unfalsifiable into one grep.

One forward note for the next PR, since it bears on precedence: set(name, null, …) deletes, and Map.set only preserves position for a key that already exists. So an MCP-disabled cycle would re-append 'mcp' after anything registered in between and flip the order. Not reachable while mcpEnabled never returns to false, but any precedence claim made in comments should be checked against insertion order rather than source order.

[nbouliol]

Took the observability half: RootMiddleware now receives the mounter's logger and dispatch emits one Debug line naming the winner (Root middleware: 'mcp' claims /mcp), so a request answered by the wrong handler is one grep away instead of unfalsifiable — registration-time detection stays out, the predicates are opaque. Insertion-order note noted: nothing in the file claims precedence, and the delete-then-set re-append is unreachable while mcpEnabled never returns to false.

[Tonours]

Claude Opus 5 (claude-opus-5): Violates conventions — skills/conventions/testing.md#Assert behavior with args and outputs

The rule: "A spy or mock must assert which arguments the call was made with — never bare called-ness […] What is banned is stopping at that it ran." Three sites here stop at that it ran — this line, and the two expect(claimed).toHaveBeenCalled() in the Koa and getCallback blocks. (not.toHaveBeenCalled() is exempt by the rule's own last sentence, so those five are fine.)

This matters concretely for one argument. The only thing getExpressMiddleware does for a claimed url is handler(req, res, next), and nothing asserts next is forwarded. Drop it from that call and every test still passes — while on Koa a declining handler would then hang the request forever, since expressToKoa resolves only on next() or a finished response. The Koa-boundary case has the same exposure from the other side: expressToKoa hands the callback ctx.req / ctx.res, not the Koa ctx, and asserting only called-ness means a future change handing over the Koa context passes CI and fails on the first real MCP request.

expect(bff).toHaveBeenCalledWith(req, res, next) with the three values in locals covers all of it.

Separately, a coverage loss with the deleted suite: its negative case — an explicit matcher startsWith('/custom') registered, /mcp must not reach the callback — has no replacement. It pinned the explicit matcher as an override of the default rather than an addition to it. Every case in the new suite registers a matcher that claims the url it then asserts on, so nothing fails if a default pattern set is ever OR-ed in alongside. One case restores it. (The other deleted case, the default-matcher fallback, disappears with the first finding if you take it.)

[nbouliol]

Fixed: the three sites now assert arguments — toHaveBeenCalledWith(req, res, next) on the Express and getCallback paths, and toHaveBeenCalledWith(ctx.req, ctx.res, expect.any(Function)) on the Koa boundary, which pins the node request/response rather than the Koa context. The negative case is back too: a handler registered with startsWith('/custom') must leave /mcp to the host.

[Tonours]

Claude Opus 5 (claude-opus-5): Preferential

Two small contract-versus-code mismatches in this file, neither reachable today.

The defensive 404 disappeared. At the merge base an unclaimed request still reached the MCP callback, which owned the decision and had an explicit last resort when next was missing — res.writeHead(404, …) under a "this shouldn't happen in normal usage" comment. next?.() replaces that with a no-op: no status, no log, the socket open until the client times out. I enumerated all five mount points plus the nested standalone path and every one supplies a next, so this is not observable — it is worth a line only because the optional-call is now the only thing between a future next-less caller and a hung connection.

The class comment claims a removal path no code takes. It says a handler can be removed on "a restart, a stop()". restart() only ever replaces, and FrameworkMounter.stop() runs its onStop tasks and nothing else — no path calls set(name, null, …) on stop, so after agent.stop() a still-serving Express or NestJS host keeps routing /mcp into a stopped MCP callback. The behaviour is unchanged from the merge base; only the comment asserting otherwise is new, and it sits in the one file a maintainer will read while diagnosing exactly that. Either wire stop() to deregister, or cut the parenthetical.

Worth a sentence in the PR body either way: if a handler is ever deregistered while the host keeps the middleware, its requests fall through to the host's 404 — a reasonable contract, but indistinguishable from "the route was never registered", so the deregistration wants a log.

[nbouliol]

Comment fixed — the parenthetical now says "on a restart" only, since stop() runs its onStop tasks and never deregisters. Not restoring the 404: the middleware's contract is that an unclaimed url is not its business, and the old 404 belonged to the MCP callback, not to the dispatcher — having RootMiddleware answer a request no handler claimed is exactly the behaviour this PR removes from the connect path. All five mount points plus the nested standalone path supply a next; if a terminal, next-less caller is ever added it needs to say so at construction rather than have the middleware guess per request.

[nbouliol]

On the spec deviation: rather than copying makeIsMcpRoute into the agent, mcp-routes.ts is gone and makeIsMcpRoute(basePath) is now the only matcher the MCP handler is ever registered with — it travels with the callback as a RootHandler, so there is no second implementation to drift and no default that could claim host urls. Inline findings answered one by one.

[Tonours]

All four findings verified on e1736d1: the crude matcher module is gone, a handler and its matcher travel together as RootHandler so registering one without the other does not compile; dispatch names the winner in a Debug line; the three called-ness assertions now pass args, the Koa one pinning ctx.req/ctx.res, and the negative explicit-matcher case is back; the class comment no longer claims a removal path stop() does not take. Not restoring the 404 is fine — it belonged to the MCP callback, not the dispatcher.

[Tonours]

Own content is identical to the reviewed 46447d8/e1736d1 (range-diff clean) — only the base stack moved under it. Approving.