feat(agent): serve a BFF in-process with addBff() - #1876
Conversation
2 new issues
|
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (11)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
Tonours
left a comment
There was a problem hiding this comment.
Spec (PRD-1076): steps 8, 9 (its /bff half), 10, 11, 12, 15 and 16 are all delivered, and the two packaging decisions the ticket asked for by name — engines: { node: ">=22.12.0" } and the exact-pinned optional peer — match step 15 word for word, so neither is in question here. Two steps landed partially:
- Step 13 asked that a
buildBfffailure not leave a half-started agent, "the host is already serving/forestwhenstart()rejects, and a retry could duplicate hooks and subscriptions". Themountedflag and the audit-trail close are in, well reasoned. The retry half is not:subscribeToServerEvents()andonRefreshCustomizations()run before the failure point and there is no guard against a secondstart(). - Step 17 asked to "document that the
Promise.racecancels nothing: an action cut at 10s keeps running agent-side, and a retry can double the mutation". The propagation itself is complete and I traced it end to end —agentTimeoutMs→ config →resolveTransport→InProcessRequester→injectWithTimeout. The documentation is absent:git grep -i 'cancels nothing|keeps running|double the mutation'over both this PR and the docs PR returns nothing. It is a safety caveat about a doubled mutation, and embedded mode is what makes it reachable.
What I checked and found sound, so it does not get re-litigated: the bff-routes.ts helpers are correct on every input I threw at them, including /bff and /bff/ both stripping to / and never to '', /bffalo falling through, and query/fragment handling; the agent's auth chain genuinely still runs on the in-process path (the dispatcher injects into the mounted /forest router whose first root route installs jwt({ secret: authSecret }), and types.ts states the shared-secret invariant explicitly); getInProcessDispatcher() at builder time really is necessary and the comment explaining why is right; stop() leaks nothing, since agent-bff/src holds no timer; and the dependency direction between the two packages is now cleanly one-way.
One packaging mechanic worth a decision rather than a fix: engines: ">=22.12.0" is blocking under yarn 1 — an install failure, not a warning — and it ships here as a feat, so a minor bump. Any consumer still on Node 20 fails to install on a minor. The ticket asked for the field; it did not weigh that. Your call whether it wants a release note.
Eleven findings inline, three of them Must fix.
|
Pushed 271c47b. Eleven inline findings answered individually — nine fixed, two partly, and the reasoning is in each thread. On the two points in the review body itself: Step 13, the retry half. Addressed for the BFF: Step 17, the
Both suites green: agent-bff 82 suites / 1368 tests, agent 87 suites / 1587 tests, lint clean on both. |
Tonours
left a comment
There was a problem hiding this comment.
All fourteen findings verified on 271c47b: collidesWithBff() normalizes the way mcp-server does, so 'bff', '/bff/' and '/bff/ai' all throw, and the comment has the direction right; the tokenEncryptionKey doc no longer claims an access control it does not provide; the unit job ignores embedded-bff.e2e; the load failure keeps its cause; prepare() validates before mount and before the SSE subscription; counters reach the host logs while gauges stay dropped; the e2e runs its data contract over both transports, HTTP included; bff_stopped is told apart from bff_not_started; originalUrl is claimed before the rewrite; the five test gaps and the barrel export are closed. Keeping the handler mounted on stop and keeping features on /health are both argued positions I accept.
One note, not blocking: the build workflow has not run on this head — only Macroscope and qlty report. Worth a re-run before merge.
e1736d1 to
b72b6ab
Compare
271c47b to
42527f9
Compare
Running a BFF meant a second deployment: another process, another port, another set of secrets to keep in sync with the agent's. `addBff()` serves it at /bff on the agent's own port instead, on every mount target. The BFF reaches the agent through the in-process dispatcher the embedded MCP server already uses, so there is no socket, no agent url to guess per host framework, and no second listener. Everything it shares with the agent — the secrets, the Forest urls, the logger — is inherited rather than repeated. The dispatcher is registered in addBff() rather than at start(): its hook is pushed on first use and mount() only runs the hooks registered before it, so asking later would leave every BFF call throwing until the first restart. `/bff` answers 503 while the agent is starting and stops answering entirely once it stopped, since a host application keeps the middleware it registered. The search integration suite moves here from agent-bff, which loses its dev dependency on the agent and with it the build cycle that dependency would have created. It now covers the embedded path end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Without a sink `createReadModel` builds a console one, which reports its gauges at Info. That is what the standalone deployment wants; embedded it puts a schema-cache age line in the host's own logs on every read, for a number nobody reads there. `buildBff` now takes the sink, and the agent passes a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The /bff collision guard compared the raw mountAiMcpServer basePath to the
literal "/bff", but the MCP server normalizes its own: "bff", "/bff/" and
"/bff/ai" all landed inside /bff and slipped past. The BFF is registered at
builder time and wins the root middleware first-match, so in every one of
those cases the MCP server booted, logged success and was never reachable.
Normalize before comparing, and test containment rather than equality — the
comment above the error also had the victim backwards.
Configuration is now parsed in a prepare() step that runs before mount(),
not after it: everything it validates is what the caller handed to addBff(),
so a mistyped tokenEncryptionKey used to leave the host serving /forest with
/bff permanently answering 503 and no way back short of a restart. It also
moves that failure ahead of subscribeToServerEvents, so a retry cannot
duplicate the subscription.
Counters now reach the host logs. They are the schema cache and the
action-endpoint resolver only channel — neither takes a logger — and every
one reports a failure, so dropping them made a stale schema served to
third-party UIs completely silent. Gauges stay dropped, which is what the
original comment reasoned about.
Also: an Error in a log context is unfolded instead of serializing to {};
a package that fails to load keeps its reason and cause, since it resolves
from the host node_modules and "install it" is often the wrong advice;
a stopped BFF answers bff_stopped with a message rather than bff_not_started,
so a probe can tell shutdown from boot; originalUrl keeps the url the client
asked for; /health no longer reports ok on a dispatcher without an auth
secret, where the agent edge is a stub; the tokenEncryptionKey doc no longer
claims it closes the data surface (it gates the login flow, authSecret guards
the session bearer); the agentTimeoutMs doc says the timeout cancels nothing.
The unit job ignored search-agent.integration, a file this stack deleted, so
the e2e suite ran inside the fail-fast matrix as well as its own job. The
suite also now runs its data contract over both transports: the HTTP one had
no end-to-end coverage left anywhere in the repo, and it is the standalone
deployment only route to the agent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The in-process transport built its dispatch target from `IN_PROCESS_URL`, renamed to `IN_PROCESS_AGENT_URL` when it became a public export, and `setBffCallback` still passed a callback and a matcher as two arguments after the root registry started taking them paired. Both compiled in isolation and only broke once the stack sat in the order it merges in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`engines: { node: ">=22.12.0" }` is a hard install failure under yarn 1, not
a warning, and this ships as a minor — so every consumer still on Node 20
would fail to install the agent over a version bump that has nothing to do
with the BFF they never asked for.
The constraint belongs to the package that actually needs it. agent-bff keeps
its own `engines`, and it is an optional peer: a Node 20 host installs the
agent as before, and only hits the requirement if it opts into addBff() by
installing agent-bff.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lying it works Two things the embedded mode turns from academic into real. The allow-list only omitted a header for a disallowed origin and ran the request anyway, so a list was read and an action was executed for that origin — the browser merely discarded the answer it was never allowed to read. Once a host application sits in front of this app, its own permissive `cors()` answers the preflight with `*` and the real request arrives here regardless, which is measured in the e2e suite. A request carrying a disallowed `Origin` is now refused with `origin_not_allowed`; a caller sending no `Origin` at all — every server-to-server api-key call — is untouched. And `/health` called its map `features`, which reads as "these work". It never meant that: `oauth` is true as soon as an encryption key is set, so a deployment whose Forest server is unreachable answered 200 while advertising oauth and ai. Renamed to `configured`, which is what it has always reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The whitelist exempts a caller arriving over a loopback socket with no proxy hop, which is exactly what the in-process transport looks like, so every BFF request escapes it. That is the decision: propagating the caller's ip would make the embedded mode stricter than the standalone one — where the whitelist only ever sees the BFF's own host — and would refuse the browsers a third-party UI is made of. What it must not be is silent. An operator who turned the whitelist on to close a door had no way to learn this door is not part of it: nothing in the logs, nothing in /health. The warning names the consequence and what still protects the route, so it cannot be read as "the BFF is open". Its own read of the configuration rather than the one the IpWhitelist route already fetched: that route keeps it private, and reaching into it would put BFF concerns in an unrelated part of the agent. One round-trip at boot, and it never fails the boot — a warning is not worth refusing to serve over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b72b6ab to
f73332f
Compare
42527f9 to
ca5550f
Compare
The two in-process surfaces the agent hosts now tag their lines the same way, so a host scanning its own logs reads one convention rather than two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tonours
left a comment
There was a problem hiding this comment.
Delta since my last review (271c47b → 1e3ce47): rebase plus five commits — 5c8313d, a2c694d, 55875e0, ca5550f, 1e3ce47. One must-fix; the rest of the delta verified good (details at the end).
Must fix
The origin deny breaks same-origin mutating browser calls. createCorsMiddleware is installed unconditionally (build-bff.ts:526) and now refuses every request whose Origin is not in the allow-list (cors-middleware.ts:55-60, from 55875e0). Browsers send Origin on every same-origin POST/PUT/PATCH/DELETE, so a deployment whose own origin is not listed — the default, allowedOrigins starts empty — answers 403 origin_not_allowed to every list/count/action call from a same-origin UI. On main the same request was served (omit-header + log, #1868), and the README promises that contract: "Empty ⇒ no cross-origin browser access" (agent-bff/README.md:97) — same-origin implied working. This is also macroscopeapp's open thread on cors-middleware.ts:55; no test covers empty-list + Origin today.
Fix either way:
- treat a same-origin
Originas allowed (origin === ctx.origin, or Host comparison), or - make the new contract explicit: the deployment's own origin must be listed, with a test for empty-list + same-origin POST and the README sentence rewritten.
Verified good in this delta
- 5c8313d (drop
enginesfrom the agent): right call —enginesis a hard install failure under yarn 1 and this ships as a minor; the requirement stays on agent-bff, which is the optional peer. - ca5550f (whitelist startup warning): both branches tested; never fails boot; names what still protects the route.
- 55875e0 health rename
features→configured: honest naming, tests updated. Note the README in #1877 still documents the old key — flagged there. - a2c694d / 1e3ce47: rename reconciliation and log prefixing, no behavior change.
- Rebase drift on the three rewritten commits is upstream absorption (mountPath, RootHandler, IN_PROCESS_AGENT_URL), nothing new snuck in.
CI: only LLM Integration Tests (ai-proxy) fails, on all seven PRs of the stack alike — an Anthropic-side thinking.type.disabled 400 on claude-fable-5-1, unrelated to this code.
A same-origin call carries `Origin` too — the Fetch spec sends it on anything but GET and HEAD, and every BFF data route is a POST — so a host serving its own UI next to a `/bff` mount was refused on every request under the default empty allow-list, for an origin it has no reason to think it must name. The allow-list now applies to cross-origin callers only. Matched on host rather than full origin: a TLS-terminating proxy leaves ctx.protocol at http while the browser reports https, so comparing the scheme would work in development and fail in production. Nothing is weakened — a caller that can forge `Origin` can simply omit it, which was already allowed by design, and a cross-site request never carries this host as its origin. `hasAllRequired` no longer counts the encryption key. env-config already had a test named "it gates OAuth, not boot", and the next one pinned the opposite: a key-only deployment reported degraded, so a load balancer would restart a process serving its api-key and bearer traffic fine. warnMissingConfig never named the key either, so the 503 came with no explanation. Which optional surfaces are on is what `configured` reports. Metric tags are forwarded to the logger. `action_endpoint_error` and `action_endpoint_miss` carry the rendering, collection and action that failed; without them an embedded host learned only that something, somewhere, did not resolve. `addBff()` after `start()` throws instead of registering a BFF nothing will start: the dispatcher hook has no mount left to attach to, so `/bff` answered 503 for the rest of the process while every other route worked. A start() that failed still accepts it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`isSameOrigin` compared a URL-normalized host against the raw `Host` header, and the two normalize differently: `new URL()` drops a port that is the default for the scheme, while `ctx.host` keeps whatever the proxy sent. So `Host: app.example.com:443` against `Origin: https://app.example.com` read as cross-origin and 403ed — the very case the exemption exists for. Both spellings are accepted now, with the default port derived from the origin scheme. The addBff guard keyed on a flag set at the end of start(), but mount() drains the onFirstStart hooks partway through, so a call landing while start() is still in flight was already too late and slipped past. The flag is set on the first line instead, and cleared when startup fails so a failed start leaves the agent configurable. The README promised /health would report degraded without an encryption key. It reports `configured.oauth: false` and stays ok since that key gates OAuth and not boot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed 55bc6fa. Answering the The must-fix is done, taking your first option. Same-origin is exempt from the allow-list, which applies to cross-origin callers only. Your framing was the useful part: on main the same request was served, and Matched on host rather than on Macroscope then found two real holes in that first attempt, both now fixed in the same commit:
Also fixed a doc claim my earlier On your non-blocking note: the build workflow has now run on the head — everything green except Local: agent-bff 85 suites / 1440 tests, agent 88 / 1599, lint clean on both. |
`new URL()` lowercases the host it parses out of `Origin`; `ctx.host` is the raw `Host` header, spelled however the client or proxy sent it. So `Host: APP.EXAMPLE.COM` against `Origin: https://app.example.com` read as cross-origin and 403ed, though DNS hostnames are case-insensitive — the same shape as the default-port mismatch, on the other half of what `new URL()` normalizes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| const normalizedHost = host.toLowerCase(); | ||
| const defaultPort = url.protocol === 'https:' ? '443' : '80'; | ||
|
|
||
| return normalizedHost === url.host || normalizedHost === `${url.host}:${defaultPort}`; |
There was a problem hiding this comment.
🟠 High cors/cors-middleware.ts:39
isSameOrigin treats Origin: http://app.example.com as same-origin with an HTTPS request to https://app.example.com/bff, allowing it to bypass the configured allow-list and reach downstream routes. Compare the origin scheme with the request's effective protocol as well as the host (including the proxy's forwarded protocol).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/cors/cors-middleware.ts around line 39:
`isSameOrigin` treats `Origin: http://app.example.com` as same-origin with an HTTPS request to `https://app.example.com/bff`, allowing it to bypass the configured allow-list and reach downstream routes. Compare the origin scheme with the request's effective protocol as well as the host (including the proxy's forwarded protocol).
Tonours
left a comment
There was a problem hiding this comment.
Verified at 5a6593a. The must-fix is closed properly: the same-origin exemption matches on host, absorbing both normalizations new URL() and the raw Host header disagree on (default port spelled out or not, case) — a non-default port is still another origin and refused, and the whole class is pinned by unit tests on both sides plus the empty-allow-list same-origin POST case. The three open bot findings are closed too: /health stays ok without the encryption key (it gates OAuth, not boot — configured.oauth says what is off, and the env-table promise was resynced), metric tags reach the host logs, and addBff() past startup throws — hardened to mid-flight calls and failed starts, both tested. Approving.
Left open on purpose: the macroscopeapp thread on the scheme-mismatch in isSameOrigin — host-only matching is the documented tradeoff (TLS-terminating proxies; a forgeable Origin buys nothing over omitting it under bearer auth), but it is the author's call to answer.
Tonours
left a comment
There was a problem hiding this comment.
Verified at 5a6593a. The must-fix is closed properly: the same-origin exemption matches on host, absorbing both normalizations new URL() and the raw Host header disagree on (default port spelled out or not, case) — a non-default port is still another origin and refused, and the class is pinned by unit tests on both sides plus the empty-allow-list same-origin POST case. The three open bot findings are closed too: /health stays ok without the encryption key (it gates OAuth, not boot — configured.oauth says what is off, env-table promise resynced), metric tags reach the host logs, and addBff() past startup throws — hardened to mid-flight calls and failed starts, both tested. Approving.
Left open on purpose: the macroscopeapp thread on the scheme mismatch in isSameOrigin — host-only matching is the documented tradeoff (TLS-terminating proxies; a forgeable Origin buys nothing over omitting it under bearer auth), but it is the author's call to answer.

Stacked on #1875. This is where the stack becomes a feature.
Why
Running a BFF meant a second deployment: another process, another port, another set of secrets to keep in sync with the agent's — for a component whose only job is to sit in front of that same agent.
What
Served at
/bffon the agent's own port, on every mount target. The BFF reaches the agent through the in-process dispatcher the embedded MCP server already uses: no socket, no agent url to guess per host framework, no second listener.authSecret,envSecret,forestServerUrl,forestAppUrland the logger are inherited — a divergentauthSecretwould make the agent reject the very tokens the BFF mints, as an opaque 401. What is left are features the BFF switches on:tokenEncryptionKey(the OAuth login/refresh flow, and with it the AI relay — it gates that flow, not the data surface, which answers to anybff_accessbearer signed withauthSecret),allowedOrigins(browser access),openapiEnabled(the docs, off by default when embedded because the document is not filtered per caller).Three lifecycle details that are easy to get wrong, and are tested:
addBff(), not atstart().getInProcessDispatcher()pushes its hook the first time it is called, andmount()only runs the hooks registered before it — asked for later, every BFF call would thrownot mounted yetuntil the first restart./bffanswers 503 while the agent is starting, rather than falling through to the host's 404, which would read as "wrong url" instead of "not started".stop()stops answering. The host application keeps whatever middleware it registered, so without it a stopped agent would keep serving BFF data through a dispatcher pointing at a dead stack.restart()invalidates what the BFF read from the SaaS — a restart means the customizations changed.addBff()refuses a second call, and refuses to coexist with an MCP server mounted under/bffin either order — including the spellingsbff,/bff/and/bff/ai, which the MCP server normalizes onto the same prefix. The BFF is registered at builder time and wins the root middleware's first-match, so the MCP surface would boot, log success and never be reachable.The build cycle
agentgains an optional peer dependency (exact pin, like every internal dep — multi-semantic-release rewrites those ranges on release) plus a dev dependency on@forestadmin/agent-bff, which would close the cycleagent → agent-bff → agent-testing → agentthatlerna run buildsorts on. It is broken by moving the search integration suite out of agent-bff, which loses its dev dependency on the agent.engines: { node: ">=22.12.0" }is declared on the agent too:addBff()pulls in a package that requires it, and the agent declared nothing.Tests
agent-bff 82 suites / 1368 tests, agent 87 suites / 1587 tests.
The moved suite becomes
test/bff/embedded-bff.e2e.test.ts: a realAgentover a real datasource, mounted on Express, with a real BFF in front — list, search, relation-extended search, count, and an agent-side refusal surfacing as the BFF error contract. It also pins what the unit tests cannot see:/foreststill answers next to it,/bffalois not claimed, and a stopped agent answers 503. Its data contract runs twice — over the in-process dispatcher, and over a real socket against a listening agent, so the HTTP transport the standalone deployment uses keeps a gate. The CI job that ran the old suite now runs this one, and the unit job ignores it.Fixes PRD-1076
🤖 Generated with Claude Code
Note
Add in-process BFF serving via
Agent.addBff()addBff()chainable builder method toAgent, which dynamically loads@forestadmin/agent-bffduringstart()and builds the BFF against the agent's in-process dispatcher at the fixed/bffroute.EmbeddedBffclass to manage the BFF lifecycle (prepare, start, stop, invalidate) and route matching inbff-routes.ts; requests under/bffreturn a JSON 503 when the BFF is stopped or not yet started.buildBfftransport resolution to prefer an injected in-process dispatcher overAGENT_URL, enabling socketless deployments, and adds a metrics sink input.corsMiddlewareto return HTTP 403 with anorigin_not_allowederror for disallowed non-OPTIONS requests, stopping downstream execution.collidesWithBffnormalization;Agent.restart()invalidates the embedded BFF;@forestadmin/agent-bffandsupertestare added as dependencies to the agent package while removed as direct dependencies fromagent-bff.Changes since #1876 opened
formatLogutility function for all execution paths [1e3ce47]cors.createCorsMiddlewaremiddleware to allow same-origin requests to proceed even when the origin is not in the CORS allowlist [aaaaf58]agent.Agent.addBffmethod preventing calls afteragent.Agent.starthas completed [aaaaf58]config.parseConfigutility to no longer requireBFF_TOKEN_ENCRYPTION_KEYforhasAllRequireddetermination [aaaaf58]embedded-bff.EmbeddedBff.preparemethod to include tags in metrics increment logging [aaaaf58]Agentclass startup state tracking fromstartedflag (set at end ofstart()) tostartupBegunflag (set at beginning ofstart()and cleared on failure), and modifiedAgent.addBff()to reject calls once startup has begun rather than once startup has completed [55bc6fa]isSameOriginutility in@forestadmin/agent-bffCORS middleware to treat requests as same-origin when theHostheader includes the scheme's default port while theOriginheader omits it [55bc6fa]@forestadmin/agent-bffdocumentation [55bc6fa]isSameOriginutility withinagent-bffCORS middleware to perform case-insensitive comparison of the Host header by normalizing it to lowercase before matching against the Origin host [5a6593a]Macroscope summarized ca5550f.