feat(agent-bff): let the agent transport be injected

[nbouliol]

Stacked on #1871.

Why

data-routes-middleware, agent-action-client and agent-capabilities-fetcher each took an agentUrl (plus a timeoutMs) and built their own HttpRequester. A socket was the only way to reach the agent, which is exactly what an agent embedding the BFF in its own process cannot use.

What

One AgentTransport replaces the { agentUrl, timeoutMs } pair everywhere:

• createHttpTransport — what the standalone BFF uses; identical behavior to before.
• createInProcessTransport — reaches an agent in the same process through an injected dispatcher, declared structurally so neither package needs a dependency on the other for six primitive fields.

InProcessRequester subclasses HttpRequester so the deserialization and the AgentHttpError shape stay identical to the HTTP path, and it escapes the path the way buildUrl does. That last point matters: agent-data-client passes raw segments on purpose (its comment says pre-encoding would double-encode), so the escaping has to happen in the transport or the two paths address records differently.

Worth noting for the review: escapeUrlSlug is encodeURI plus escaping of +?*, and encodeURI leaves # and ? alone — so a # inside a primary key is mishandled on both transports, not just in-process. That is a pre-existing issue tracked in PRD-1124, not something this PR fixes; what this PR guarantees is that the two transports behave the same.

stream() throws in-process: there is no socket to stream from, and nothing in the BFF streams today.

Tests

82 suites, 1362 tests. New in-process-transport suite covers the dispatched shape, the escaping (a space and a + are observable, unlike #), the leading-slash normalization, the timeout precedence, the error mapping and the stream refusal. The middleware suites now assert the transport reaches the client, and the timeout assertions moved down to the client suites where the transport is built.

Fixes PRD-1076

🤖 Generated with Claude Code

Note

Inject AgentTransport into agent-bff clients and add in-process transport

• Introduces AgentTransport interface with url and createRequester(token), replacing raw agentUrl/timeoutMs across all agent-bff clients, middlewares, and the OpenAPI source
• Adds createHttpTransport factory wrapping existing HTTP requester creation, and createInProcessTransport that dispatches queries via an AgentDispatcher without network I/O
• InProcessRequester extends HttpRequester, overrides query() to dispatch to the dispatcher, and explicitly throws on stream()
• Risk: all agent-bff client/middleware option types now require transport instead of agentUrl/timeoutMs; existing callers passing raw URL/timeout will fail to compile

Changes since #1872 opened

• Normalized InProcessRequester behavior to match HttpRequester by preprocessing paths through toDispatchTarget helper to ensure leading slashes and apply escapeUrlSlug, extracting and merging query parameters from the path with explicit query arguments and timezone, wrapping dispatcher invocations in a dispatch method that converts all thrown errors and rejections into AgentHttpError with status 500 using buildError to embed original error details, changing streaming rejection from raw Error to typed BffHttpError using streamingUnsupported with status 501 and type 'streaming_unsupported', and adding documentation to AgentDispatcher interface clarifying timeout semantics and rejection requirements [ca6056c]
• Added streamingUnsupported error factory function to bff-local-errors that returns a BffHttpError with status 501 and type 'streaming_unsupported' [ca6056c]
• Expanded test coverage to verify InProcessRequester matches HttpRequester path and query normalization using nock and HttpRequester for reference behavior, agent error shape parity for 4xx responses, conversion of dispatcher rejections to 500 AgentHttpError with embedded cause, and streaming rejection with typed 501 BffHttpError, and verified transport timeout configuration in buildBff and per-request client building with request-specific agent tokens in middleware tests [ca6056c]

📊 Macroscope summarized 813ae46. 9 files reviewed, 2 issues evaluated, 0 issues filtered, 2 comments posted

🗂️ Filtered Issues

[linear-code]

PRD-1076

[qltysh]

1 new issue

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

[macroscopeapp]

🟠 High src/build-bff.ts:318

BFF data and action routes always use an HTTP socket, so embedding callers cannot route requests through the in-process dispatcher. buildAgentRouteMiddlewares unconditionally creates createHttpTransport, while BuildBffOptions provides no transport override and createInProcessTransport is not exposed; accept and select a transport for embedded deployments, and export the in-process factory as needed.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/agent-bff/src/build-bff.ts around line 318:

BFF data and action routes always use an HTTP socket, so embedding callers cannot route requests through the in-process dispatcher. `buildAgentRouteMiddlewares` unconditionally creates `createHttpTransport`, while `BuildBffOptions` provides no transport override and `createInProcessTransport` is not exposed; accept and select a transport for embedded deployments, and export the in-process factory as needed.

[nbouliol]

Deliberate staging, not a gap: this PR only opens the seam, and feature/prd-1076-7-add-bff (commit 6e6c695d4, "feat(agent): serve a BFF in-process with addBff()") is the one that exports createInProcessTransport and lets BuildBffOptions carry a transport — accepting an override here would ship an option no caller in this branch can reach.

[macroscopeapp]

The current PR still has no reachable transport seam: BuildBffOptions has no override and route middleware hard-codes HTTP. A follow-up branch does not resolve this PR’s stated behavior, so this remains applicable here.

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (5)

Rating	File	% Diff	Uncovered Line #s
	packages/agent-bff/src/data/agent-data-client.ts	100.0%
	packages/agent-bff/src/build-bff.ts	100.0%
	packages/agent-bff/src/http/bff-local-errors.ts	100.0%
	packages/agent-bff/src/agent/agent-transport.ts	100.0%
	packages/agent-bff/src/agent/in-process-transport.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): step 3 is implemented, with one gap — the step says the in-process requester "must apply escapeUrlSlug to the path", and it does, but applying escapeUrlSlug alone is not what the HTTP path does, so the stated goal ("keeps a given record id addressing the same record over both transports") is not met. Details in the first finding.

Three things worth stating because they came out clean and they were the risky ones:

• Auth is not bypassed. The per-request agent JWT travels on every dispatch, and the only AgentDispatcher implementation injects into the agent's real Koa RequestListener via light-my-request, so the agent's own auth and permission middleware still runs. The "permissions are enforced by the agent, not the BFF" contract holds over both transports.
• Subclassing HttpRequester to inherit buildError / deserialize is the right call. It is why the error shape is preserved rather than re-derived, and why the findings below are about paths and timeouts and not about a dozen response-shape divergences.
• The # half of the PR description is correct. new URL drops the fragment on the HTTP side and the dispatcher's own url parse drops it on the in-process side, so # really does behave the same. It is +, ? and * that diverge — the three characters escapeUrlSlug singles out.

One note on PRD-1124 for whoever owns it: its impact section says "only over the in-process transport: the same call over HTTP is correct". That is not right — on HTTP, escapeUrlSlug's backslash is rewritten into a path separator by new URL, so HTTP mangles these keys too, just differently. This PR's description is closer to the truth than the ticket it cites.

---

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

Applies to: the PR as a whole.

Worth an ADR: reaching an embedded agent by injecting into its Koa stack through a dispatcher, rather than by a loopback HTTP call from the BFF to itself. Hard to reverse (the transport seam and its structural interface are now the contract between two packages), surprising without context, and a real trade-off — no socket and no second port, against a request path that no longer benefits from anything the HTTP stack gave for free, which is precisely what three of the findings below are about. The ADR search returns nothing covering it.

[Tonours]

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

A record whose primary key contains +, ? or * resolves over one transport and fails over the other — same click, two different answers. parentId and the record id come straight from user-supplied keys (src/data/agent-data-client.ts:38-39), and an email with a plus alias or a base64-ish id are the realistic carriers.

Mechanism: this line escapes and stops. The HTTP path escapes and then goes through new URL(...) (packages/agent-client/src/http-requester.ts:144). escapeUrlSlug prefixes + ? * with a literal backslash, and the WHATWG parser treats \ in an http: URL as a path separator, so new URL splits the segment while this line ships the backslash. Executed, not reasoned:

input                     HTTP (new URL pathname)   in-process (raw path)
/forest/orders/A B+c      /forest/orders/A%20B/+c   /forest/orders/A%20B\+c
/forest/orders/a*b        /forest/orders/a/*b       /forest/orders/a\*b
/forest/orders/q?x        /forest/orders/q/         /forest/orders/q\?x
/forest/users/../admin    /forest/admin             /forest/users/../admin

Concretely on GET /agent/v1/users/relations/posts/list with parentId = "a+b": over HTTP the extra segment makes the agent's relationship route regex miss → 404 → not_found; in-process the route matches with a parentId containing a backslash → a record lookup that fails differently, or a 500 → agent_unavailable. Secondary: new URL collapses .. and this line does not, so the dispatcher receives traversal paths the HTTP transport would have normalised away.

Don't reimplement half of buildUrl — reuse its construction so equality holds by construction rather than by comment:

path: new URL(`${IN_PROCESS_URL}${HttpRequester.escapeUrlSlug(normalized)}`).pathname

[nbouliol]

Fixed as suggested — the dispatched path is now new URL(IN_PROCESS_URL + escapeUrlSlug(normalized)).pathname, with the query that parse splits off merged into the dispatched query, and the test measures parity instead of asserting it (a real HttpRequester through nock, five inputs including ..).

One correction to the reasoning, which is why I made the two identical rather than making this side right: the agent registers escapeUrlSlug(collectionName) as a path-to-regexp pattern (packages/agent/src/routes/base-route.ts:22), and pathToRegexp('/forest/a\\+b') yields /^(?:\/forest\/a\+b)(?:\/$)?$/i, which matches the raw /forest/a+b and rejects HTTP's /forest/a/+b — so HTTP is the broken side and mcp-server's raw pass-through is accidentally the correct one. Fixing that means changing escapeUrlSlug/buildUrl in agent-client, which is PRD-1124's and fixes both transports at once; what this PR owes is equality, which now holds by construction.

[Tonours]

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

This is the only evidence offered for the parity claim, and it certifies the opposite of its own name. The test is called should escape it exactly as buildUrl does on the HTTP path, feeds /forest/orders/A B+c, and asserts /forest/orders/A%20B\+c — while buildUrl actually yields /forest/orders/A%20B/+c for that input. Green, on the example it chose itself.

That is worse than having no test: the next person reads the name, trusts the invariant, and never checks. Either compare against a real buildUrl call so the two cannot drift, or rename it to describe the escaping it does perform.

Same theme in the same file at line 83 — should throw the same AgentHttpError shape as the HTTP path asserts only { status: 403 }. The fixture sets up detail: 'Forbidden' and nothing asserts it, so neither the type nor the message is checked, while error-shape parity is the whole premise of subclassing HttpRequester (skills/conventions/testing.md#Test name states the exact behavior it asserts).

[nbouliol]

Both fixed. The escaping test no longer asserts a hand-written string: it drives a real HttpRequester at nock, captures the path superagent actually sends and asserts the dispatcher got the same path and the same query params, over five inputs (space, +, *, ?, ..) — the two cannot drift without the test failing. The 403 test now builds the same 403 over both transports and asserts the in-process error against the HTTP one: AgentHttpError, same message, status, body and responseText.

[Tonours]

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

Delete this line and the whole suite stays green, while every agent call silently falls back to superagent's 10 s default instead of the configured BFF_AGENT_TIMEOUT_MS.

The rewiring dropped the assertions that used to protect it: the tests that asserted timeoutMs: 2500 and timeoutMs: undefined reached the client were replaced by expect.objectContaining({ transport: TRANSPORT }) — object identity on a module constant passed straight through, which cannot fail. git grep agentTimeoutMs over packages/agent-bff/test now hits only config/env-config.test.ts (parsing) and cli-ai-wiring.test.ts (the AI timeout), so the config.agentTimeoutMs → createHttpTransport → maxTimeAllowed link is asserted nowhere.

One assertion on the constructed transport's timeoutMs, in either the data or the action middleware suite, restores what the old tests covered.

[nbouliol]

Fixed: test/build-bff.test.ts now spies on createHttpTransport (a requireActual passthrough, so the rest of the suite keeps the real one) and asserts that every transport buildBff constructs carries { agentUrl, timeoutMs: 2500 } — both call sites, since asserting only one of them would have been satisfied by the toUnfoldSource call while the middleware line dropped it.

[Tonours]

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

Applies to: packages/agent-bff/src/http/agent-error-mapper.ts:152-166 (not in this diff) — anchored on the unguarded dispatch, which is where the fix belongs.

A customer's TypeError in an action hook, a datasource driver throw, an assertion in a customizer — anything escaping the agent — reaches the end user as 502 { type: 'network_error', message: 'The agent could not be reached' }, for an agent that is in the same process and perfectly reachable. The developer gets one Warn line carrying error.message and no stack. The message actively misdirects: they check the network, the proxy, the agent URL, none of which exist on this path.

Mechanism: query has no try/catch, so a dispatcher.request rejection propagates raw into mapAgentError, whose non-AgentHttpError branch is the transport-failure bucket. That bucket is right on HTTP — no response really does mean unreachable, and the generic message deliberately hides internal topology. In-process there is no network, so the only things that can land there are a dispatcher bug or an exception from the agent itself.

Secondary hazard on the same path: parseJsonApiFromMessage runs JSON.parse(error.message) first, so a customizer that throws new Error(JSON.stringify(...)) is mapped to 400 invalid_request — blaming the caller for an agent bug.

Wrap the dispatch, log at Error with the stack, and throw buildError(500, …) so it routes to 503 agent_unavailable — the honest bucket for "the agent broke". While there, say on AgentDispatcher whether an implementor is expected to reject or to return a status; specifying neither is how this gap got in.

[nbouliol]

Fixed: the dispatch is wrapped, and a rejection is rethrown as buildError(500, { errors: [{ detail: stack }] }) — so mapAgentError takes its AgentHttpError 5xx branch, logs the real cause with the stack, and answers 503 agent_unavailable instead of 502 "the agent could not be reached". That also closes the secondary hazard you noted: parseJsonApiFromMessage is no longer reached on this path, so a customizer throwing JSON.stringify(...) can't be mapped to 400. Logging stays in the mapper rather than moving into the requester (which has no logger); the stack rides in detail, which for a 5xx is only ever logged, never sent to the client. The interface now says a failed dispatch must reject.

[Tonours]

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

The 10 s ceiling the HTTP path always has (.timeout(maxTimeAllowed ?? 10_000)) exists in-process only by the grace of the dispatcher: this line passes timeoutMs as advisory data and then awaits with no race and no timer, and when BFF_AGENT_TIMEOUT_MS is unset it hands over undefined — which the sibling test freezes as the expected value.

To be fair to the design: it is not actually unbounded today. packages/agent/src/mcp-in-process-dispatcher.ts defaults to 10 s, races the injection, and even swallows a late settlement so a post-timeout rejection cannot surface as an unhandledRejection — with a comment saying it matches the HTTP bound. That is careful work.

The finding is that AgentDispatcher as declared here places no such obligation on an implementor, so the parity rests on a convention across a package boundary rather than on the contract. A second dispatcher hangs a BFF request forever where HTTP would have returned 502. Either ?? 10_000 on this line, or one sentence on the interface making the bound the implementor's stated duty.

[nbouliol]

Took the contract option: AgentDispatcher.request now states that bounding the call is the implementor's duty, that an absent timeoutMs means applying the 10s ceiling HttpRequester.query always has, and that a failed dispatch — timeout included — must reject. Not ?? 10_000 here: this side has nothing to race and no socket to abort, so the number would still be advisory and a dispatcher that ignores it would hang exactly as before — enforcement can only live where the injection happens, which is what the sentence now makes an obligation instead of a convention.

[Tonours]

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

This class is a near-verbatim copy of packages/mcp-server/src/in-process-http-requester.ts — same sentinel URL, same header block, same timezone default, same status >= 400 branch, same stream() refusal — and the copies already disagree on the one line that matters: mcp-server passes path raw, this one escapes it. Both dispatch into the same InProcessDispatcher, so an MCP tool call and a BFF call for the same record id now reach the agent's Koa stack with different path strings. One of the two is wrong, and after the finding above, arguably both are.

To be clear about what is not the finding: re-declaring AgentDispatcher / AgentDispatchRequest / AgentDispatchResponse structurally instead of importing mcp-server's is a deliberate, documented decision — PRD-1076's "Decisions taken" argues it explicitly, and I am not reopening it. The interface duplication is a choice; the requester duplication with divergent escaping is not covered by that rationale.

Cheapest shape given that createInProcessTransport has no production caller in this diff: fix the escaping once in a shared requester, rather than land a second copy ahead of the PR that needs it.

[nbouliol]

Not deduping, and the divergence is now deliberate rather than accidental. Executed: the agent registers escapeUrlSlug(collectionName) as a path-to-regexp pattern, and pathToRegexp('/forest/a\\+b').regexp matches /forest/a+b and rejects /forest/a/+b — so mcp-server's raw pass-through is the form the agent's routes actually match, and HTTP is the broken one. This transport's invariant is HTTP parity (the BFF serves the same routes over both, and a record must not resolve differently depending on how the BFF was deployed), so aligning it on mcp-server would reintroduce exactly the "same click, two answers" you flagged above. The single fix is escapeUrlSlug/buildUrl in agent-client under PRD-1124, which corrects HTTP, in-process and MCP at once; landing it from a stacked BFF PR would change MCP behavior in a package this branch does not touch. Happy to hoist the shared requester into agent-client as part of that ticket.

[Tonours]

Claude Opus 5 (claude-opus-5): Violates conventions — skills/conventions/ts-server.md#Throw typed errors; no silent catches

The rule: "Throw from the stack's BusinessError / ValidationError hierarchy, never a raw Error … a typed error carries the semantics the HTTP boundary needs to map a response."

Refusing is the right behaviour — stream()'s only callers are exportCsv and the segment export in agent-client, and no BFF route reaches them today, so this is latent rather than live. But a raw Error lands in the same mapAgentError fallback as the finding above, so the day a route does stream over this transport the user gets 502 "The agent could not be reached" for a capability gap, and the actual sentence survives only inside a Warn context field. A connectivity message for a not-implemented condition sends whoever debugs it to the wrong system.

BffHttpError(501, …) is returned as-is by mapAgentError's first branch, and bff-local-errors.ts already has the 501 shape.

[nbouliol]

Fixed: stream() now throws streamingUnsupported(...) — a new BffHttpError(501, 'streaming_unsupported', …) in bff-local-errors.ts — which mapAgentError returns as-is, so the day a route does stream over this transport the client is told what is missing instead of being sent to look for a network the request never crossed.

[Tonours]

Claude Opus 5 (claude-opus-5): Violates conventions — skills/conventions/testing.md#Test name states the exact behavior it asserts

should build one client per request with the configured transport fires a single request and asserts objectContaining({ transport: TRANSPORT }). It proves neither half of its name: no toHaveBeenCalledTimes, no second request, and the transport assertion duplicates line 137, which already exact-matches the full options object. Net effect — hoisting client creation out of the request handler would break no test, so the per-request invariant is unguarded.

Same defect one file over: test/action/action-routes-middleware.test.ts:187, leaves the agent token on the client options for every call, issues one request and asserts one objectContaining. "For every call" is unverified, so a regression that reused a first-request client — stale token on the second call — stays green.

Both want either a second request with a distinct token, or a name matching the single assertion.

[nbouliol]

Both fixed: each test now fires two requests carrying different agent tokens (the token comes off a header instead of a constant) and asserts toHaveBeenCalledTimes(2) plus the options of each call — so hoisting client creation out of the request handler, or reusing a first-request client with its stale token, fails the test the name promises.

[nbouliol]

On the ADR (Preferential): declining here, not disputing the substance. The search came back empty because the repo has no ADR practice to add to — no docs/, no adr/ or decisions/ directory anywhere outside node_modules — so an ADR in this PR would be starting a convention as a side effect of a stacked BFF change. The decision stays recorded where it already is: this PR's body (why a dispatcher rather than a loopback call, and what it costs) plus PRD-1076's "Decisions taken". Happy to open a docs/adr/ and seed it with this one as its own change if we want the practice.