feat(agent): serve a BFF in-process with addBff() - #1876
Conversation
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>
2 new issues
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (9)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
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>
Tonours
left a comment
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.
| throw new Error('addBff can only be called once.'); | ||
| } | ||
|
|
||
| if (this.mcpBasePath === BFF_PREFIX) { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Must fix
The collision guard is defeated by any non-literal spelling of /bff, and when it is, the entire MCP surface goes silently dark — the server initialises, logs Server initialized successfully, and is never reachable.
Both guards compare the raw option string to '/bff', while mcp-server normalises its basePath (trim, add the leading slash, collapse and strip trailing slashes) before building its paths. Executed against the shipped helpers:
basePath="/bff" guard trips=true
basePath="/bff/" guard trips=false <-- MCP shadowed silently
basePath="/bff/ai" guard trips=false <-- MCP shadowed silently
basePath="bff" guard trips=false <-- MCP shadowed silently
'bff' and '/bff/' both normalise to /bff inside the MCP server, so it claims exactly /bff/oauth/ and /bff/mcp — the collision this error exists to reject. '/bff/ai' is accepted by normalizeMountPath's allowlist (it permits multi-segment prefixes) and lands wholly under /bff. In every case isBffRoute claims the path, so POST /bff/mcp returns the BFF's 404.
And the comment above BFF_MCP_COLLISION has the direction backwards: "the MCP server … is consulted first, so it would shadow two of the BFF's own routes". It is the other way round. addBff() calls setBffCallback at builder time; setMcpCallback runs inside start(); handlerFor iterates a Map in insertion order and returns the first match, so 'bff' wins. Map.set on an existing key preserves position, so restart() does not reorder it. The throw is still right — it is the stated victim that is wrong, which matters because the next person reasoning about a new handler will reason from a false premise.
Normalise before comparing, and test containment rather than equality — either path being a prefix of the other is a collision, whichever is consulted first. isBffRoute(${normalized}/mcp) covers all four rows above.
There was a problem hiding this comment.
Fixed, and you are right on both counts. collidesWithBff() in bff-routes.ts normalizes the way normalizeMountPath does, then tests containment with isBffRoute(${normalized}/mcp) — so all four of your rows now throw, and the error names the basePath the caller actually passed instead of a hardcoded /bff. The normalization is mirrored rather than imported because mcp-server keeps it internal and exporting it would widen this PR into a third package. The comment is rewritten with the direction the right way round: the BFF registers at builder time, wins the Map first-match, and the MCP surface is the one that goes dark. Cases in bff-routes.test.ts and agent-bff.test.ts.
| export type BffEmbedOptions = { | ||
| /** | ||
| * Base64-encoded 32-byte AES-256 key encrypting stored refresh tokens. Enables OAuth (mode 1), | ||
| * and with it the AI relay. Absent, the BFF serves API-key callers only. |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Must fix
This sentence makes a security claim that the code does not honour, on public API surface.
Absent, the BFF serves API-key callers only.
It does not. createAuthModeMiddleware is mounted whenever forestAuthSecret exists, and it verifies a bff_access JWT with authSecret alone — no session store, no tokenEncryptionKey — then issues the agent token from that principal. So with the key omitted, every data route still answers to anyone who can sign a bff_access JWT with the agent's authSecret. This PR's own e2e demonstrates it: addBff({ allowedOrigins: [...] }) with no key reports features.oauth: false, and POST /bff/agent/v1/books/list with a bff_access token returns 200.
What the key actually gates is the login/refresh flow and the AI relay. The consequence of the current wording is the dangerous direction: an integrator who omits it believing they have closed session access has in fact left the data surface open.
Reword to say what it gates, and say plainly that the session-bearer path is governed by authSecret, not by this key.
There was a problem hiding this comment.
Fixed. The doc now says what the key gates — the OAuth login/refresh flow and the AI relay — and says plainly that it is not an access control: the data routes accept a bff_access bearer either way, because that token is verified against authSecret alone. Also corrected the same sentence in the PR description.
| @@ -153,7 +153,7 @@ jobs: | |||
| # Boots a real agent on a local HTTP port, so it stays out of the unit job. Unlike the LLM suite | |||
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Must fix
Applies to: .github/workflows/build.yml:114 (the unit job's ignore pattern, not in this diff) — anchored on the comment this diff makes false.
This comment now says the opposite of what happens. The unit job runs yarn test --coverage --testPathIgnorePatterns='llm.integration|search-agent.integration'. The deleted file matched search-agent.integration; the new one is test/bff/embedded-bff.e2e.test.ts, which matches neither pattern, and packages/agent/jest.config.ts collects test/**/*.test.ts.
So the suite — two full agents booted, a mock Forest server, a 30s beforeAll budget — now runs inside the fail-fast matrix's agent cell at timeout-minutes: 10, in addition to its own dedicated job. A slow runner takes the whole matrix down with it, and the failure will look like an unrelated unit regression.
One word: --testPathIgnorePatterns='llm.integration|embedded-bff.e2e'.
There was a problem hiding this comment.
Fixed: --testPathIgnorePatterns='llm.integration|embedded-bff.e2e'. Also updated the dedicated job comment and name, since it now gates both transports rather than only the embedded one.
| * Dynamically load the optional @forestadmin/agent-bff package. Deferred so agents that embed no | ||
| * BFF never load its code at startup. | ||
| */ | ||
| private async importPackage() { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
error is bound and never used, so every way of failing to load a package that is installed collapses into "install it". The developer installs, reinstalls, and the message does not move.
Realistic causes here, since the package loads from the customer's own node_modules: a missing or ESM-only transitive dependency of agent-bff (the message then names the wrong package), a SyntaxError from a partially written install or a bundler that rewrote the dist, a throw during agent-bff's own module evaluation, ESM/CJS interop under a type: module host, or a Node version below the newly declared engines — which is a resolution failure, not a helpful error.
This is the class PR #1834 already fixed in this repo for mcp-server, and the MCP path sits one file over doing it right: it logs Failed to initialize MCP server: ${message} and rethrows the original.
Keep the hint, keep the cause — engines now guarantees Node ≥22.12, so cause is available:
const { message } = error as Error;
throw new Error(
`Cannot load '@forestadmin/agent-bff' (required by addBff()): ${message}. ` +
'If it is not installed: `npm install @forestadmin/agent-bff`.',
{ cause: error },
);There was a problem hiding this comment.
Fixed, with the cause kept: the message now carries the load failure verbatim, and cause is set by assignment rather than through the constructor options bag — the repo targets ES2020, so new Error(msg, { cause }) does not typecheck even though the declared engines guarantee a runtime that reads it. Three cases in agent-bff-missing-dep.test.ts.
|
|
||
| const { buildBff, parseConfig, IN_PROCESS_AGENT_URL } = await this.importPackage(); | ||
|
|
||
| const config = parseConfig({ |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
Everything this line validates is data the caller already handed to addBff(), but it is validated at start() — after mount(). So a mistyped key does not degrade the BFF, it kills the agent, with the host left half-serving.
parseConfig throws synchronously on a tokenEncryptionKey that is not base64/32 bytes (the likeliest typo), a non-IANA defaultTimezone, a zero or out-of-range timeout, and a malformed Forest URL. Resulting state for the documented Express pattern, where app.listen() precedes agent.start(): /forest live and serving; /bff/* claimed since builder time and answering 503 bff_not_started for the rest of the process; start() rejected. There is no path back without a process restart — embedOptions is never cleared and setBffCallback(null) is never called.
Retrying start() is worse, and this is step 13's unaddressed half: subscribeToServerEvents() and onRefreshCustomizations() run before the failure point, with no guard against a second start(), so a retry opens a second SSE subscription and adds a second restart listener (the handler is a bare eventEmitter.on, no dedupe). The duplicate is then masked by the isRestarting guard, which only logs at Debug.
Call parseConfig in addBff() and leave only buildBff — which genuinely needs the mounted stack — in start(). That turns "app half-up, /bff bricked" into a throw on the line the developer wrote.
There was a problem hiding this comment.
Fixed. parseConfig moved into a new EmbeddedBff.prepare(), which start() calls first — before buildRouterAndSendSchema(), before subscribeToServerEvents(), before mount(). So a mistyped key throws with nothing mounted and nothing subscribed, instead of leaving a live /forest next to a bricked /bff; and since the failure is now ahead of the subscription, a retry cannot duplicate it either. buildBff stays in start(dispatcher), which genuinely needs the mounted stack. Asserted in agent-bff-lifecycle.test.ts. The general "second start() on an agent with no BFF" guard is pre-existing Agent.start() behaviour on every failure path, so I left it out of this PR.
| fail-on-cache-miss: true | ||
| - name: Run BFF integration tests | ||
| run: yarn workspace @forestadmin/agent-bff test --testPathPattern='search-agent.integration' | ||
| run: yarn workspace @forestadmin/agent test --testPathPattern='bff/embedded-bff.e2e' |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
Applies to: packages/agent-bff/test/data/search-agent.integration.test.ts (deleted by this PR) — anchored on the job that used to gate it.
After this PR no end-to-end coverage of the HTTP transport survives anywhere in the repo, and that transport is the standalone deployment's only route to the agent — the configuration shipping today. The deleted suite was the only test booting a real agent on a real port and driving createHttpTransport through a socket; the replacement uses the in-process dispatcher exclusively. Every remaining createHttpTransport test mocks @forestadmin/agent-client away, and agent-bff can no longer boot an agent in its own suite at all, having dropped those devDeps.
The job is still named "BFF Integration Tests", so a reader assumes it gates the standalone path. It no longer does.
What that leaves unguarded on the HTTP path specifically: HttpRequester header and auth-token construction, query-string encoding, JSON:API deserialization over the wire, the socket timeout, HTTP status → BFF error mapping, and AGENT_URL/prefix joining.
Separately, thirteen assertions became five, and five behaviours now have no case in any package: relation crossing via the author.name: query syntax without searchExtended (whose deleted case commented that it was "pinned here so it cannot change unnoticed"), the 422 relation_field_not_supported contrast for the same path in a filter, search ∩ filter intersecting rather than replacing, whitespace-only search listing everything, and the count search refusal.
The ticket sanctions moving the suite (step 7), but it says the deleted file's unique value moves — an equivalent that swaps the transport is not an equivalent for the transport it left behind. packages/agent/test/bff/ is now the natural home for both: it already has the agent and supertest, so pointing a createHttpTransport BFF at mountOnStandaloneServer costs one describe block and restores the gate the job name still claims.
There was a problem hiding this comment.
Fixed. embedded-bff.e2e.test.ts now runs its data contract twice through one shared itServesTheDataContract(): once over the in-process dispatcher, once over a buildBff({ AGENT_URL }) pointed at the express app listening on a real port — so createHttpTransport, header and token construction, query encoding, JSON:API over the wire and the status mapping are gated again. All five behaviours you listed as orphaned are back in that contract, so each is now asserted on both transports (13 cases x 2, plus the health//forest//bffalo/stopped cases — 30 in the suite). Job renamed to say it gates both.
| * Stop answering. The host application keeps whatever middleware it registered, so without this | ||
| * a stopped agent would go on serving BFF data through a dispatcher pointing at a dead stack. | ||
| */ | ||
| stop(): void { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
stop() sets bff = null and nothing else, so embedOptions stays set and handle answers 503 bff_not_started for the rest of the process lifetime. Three consequences:
Boot and shutdown become indistinguishable. A k8s or ALB probe on /bff/health gets the same 503 bff_not_started while the agent is booting and after a deliberate stop(). During boot the right action is to wait; after stop it is to take the pod out of rotation. EmbeddedBff knows which state it is in — one flag separates bff_not_started from bff_stopped.
The type misleads. "not started" for a BFF that started, served traffic, and was stopped.
/bff is permanently occupied. RootMiddleware.set supports removal and setBffCallback already accepts null; neither is used on stop. So a host that stops the agent and mounts its own /bff handler, or a test harness cycling agents on one Express app, can never get the path back — its own routes stay shadowed by a 503. setBffCallback(null) in stop() is a one-liner and makes the middleware genuinely fall through.
Minor, same body: it omits message, unlike every other BFF error, so a client rendering error.message shows blank.
While here: the if (!this.embedOptions) next?.() branch just above is unreachable — configure() runs two lines before setBffCallback(bff.handle) — so its comment describes a fall-through that does not exist, while the state that should fall through (stopped) 503s instead.
There was a problem hiding this comment.
Half fixed. A stopped BFF now answers bff_stopped with a message, and boot answers bff_not_started — a probe can tell shutdown from boot, which was the substance of the finding. The unreachable !embedOptions branch is gone: embedOptions moved to the constructor, so the state cannot exist.
I did not call setBffCallback(null). It contradicts the other half of your finding — with the handler removed there is no way to answer bff_stopped at all, and a stopped agent falling through to the host 404 is the "wrong url" reading this 503 exists to avoid. agent.stop() is process shutdown, not a handover, so a host reclaiming /bff afterwards is not a flow we support. Happy to revisit if you have a concrete case where it is.
| return; | ||
| } | ||
|
|
||
| req.url = stripBffPrefix(req.url ?? BFF_PREFIX); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
req.url is mutated in place and never restored, so on a Koa host every access log and metric of the customer's own middleware records the stripped path.
Express hosts are mostly fine — Express sets req.originalUrl at app entry and morgan reads req.originalUrl || req.url. Koa has no originalUrl, and ctx.url / ctx.path read req.url live: expressToKoa hands ctx.req straight through and resolves on finish, so any host middleware registered before the agent's that logs after await next() sees /health or /agent/v1/books/list. A /bff/health probe appears in their log as /health — colliding with paths the host may itself serve. Same on the raw-http and standalone paths, and for any res.on('finish') handler the host registered on any mount target.
The repo already has the pattern, one file over in this package: fastify-adapter.ts:127-132 saves originalUrl and restores it in the next callback. There is no fall-through to restore in here, but the mutation still escapes to everything holding a reference to req. Restore it after callback returns, or set req.originalUrl ??= req.url before stripping.
There was a problem hiding this comment.
Partly fixed. req.originalUrl ??= req.url before the rewrite, so anything reading originalUrl reports the url the client asked for.
On the Koa half: ctx.originalUrl is not missing — koa/lib/application.js:215 sets context.originalUrl = request.originalUrl = req.url in createContext, at host entry, before any middleware, so it stays correct. What is genuinely live is ctx.url / ctx.path, and neither restoring after callback() nor on finish fixes those: Koa reads req.url lazily while routing, and a host finish listener registered before the agent middleware fires before any restore of ours. The only thing that would work is handing the BFF a Proxy over req with url overridden, and proxying a live IncomingMessage stream through the body parser is more risk than the log fidelity is worth. Asserted the originalUrl behaviour in agent-bff-lifecycle.test.ts; say the word if you want the proxy anyway.
| this.setMcpCallback(mcpHttpCallback ?? null, mcpIsMcpRoute); | ||
| await this.remount(router); | ||
| // A restart means the customizations changed, so the schema the BFF read is stale. | ||
| this.embeddedBff?.invalidate(); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Violates conventions — skills/conventions/testing.md#Cover error and edge paths, not only the happy path and skills/conventions/agent.md#A module's public API is its curated barrel
Four new reachable paths have no case, and one public type is missing from the barrel.
invalidate() on restart — this line. restart() is public and documented as the runtime-remount entry point; no test calls it on an agent with an embedded BFF. The one line whose job is to stop the BFF serving a schema that no longer matches the customizations is unguarded: delete it and every test passes, so the regression ships as a stale schema answered to third-party UIs.
The missing-package path — untested, though the sibling feature proves it costs nothing: test/agent-workflow-executor-missing-dep.test.ts mocks the module to throw and asserts start() rejects with the actionable message. The BFF has no equivalent, so a refactor of importPackage can turn its message into a generic startup failure with nothing failing.
buildBff's dispatcher branch — dispatcher is a new public option on buildBff, and no test in packages/agent-bff/test/ passes one. test/agent/in-process-transport.test.ts covers the transport unit, never the wiring that selects it. So the entire embedded path is unverified inside the package that owns the code.
healthy: dispatcher !== undefined || config.hasAllRequired — the deliberate new "embedded is always healthy" semantic, asserted nowhere in agent-bff; flipping the operands keeps that suite green.
Barrel — BffEmbedOptions is the parameter type of a public method on the exported Agent, and index.ts exports only AgentOptions and WorkflowExecutorEmbedOptions. The sibling feature's embed-options type is in the barrel; this one is not, so a consumer naming the options has to import from @forestadmin/agent/dist/types — the bypass the barrel exists to prevent.
There was a problem hiding this comment.
All five fixed.
invalidate()on restart:agent-bff-lifecycle.test.tsassertsrestart()invalidates once and does not rebuild.- Missing package:
agent-bff-missing-dep.test.ts, mirroring the workflow-executor one, plus a case on the preservedcause. buildBff'sdispatcherbranch:build-bff-dispatcher.test.tsdrives a real list through the dispatcher (path, bearer, timeout, records) and asserts the no-dispatcher/no-AGENT_URLconfig falls back to the 501 stub.healthy: both operands asserted in the same file.- Barrel:
BffEmbedOptionsexported fromindex.tsnext toWorkflowExecutorEmbedOptions.
| ctx.status = config.hasAllRequired ? 200 : 503; | ||
| ctx.body = { status: config.hasAllRequired ? 'ok' : 'degraded', version }; | ||
| ctx.status = healthy ? 200 : 503; | ||
| ctx.body = { status: healthy ? 'ok' : 'degraded', version, features }; |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Preferential
/health is unauthenticated — it sits second in the middleware array, long before createAuthModeMiddleware — and now returns features: { oauth, ai, cors, openapi }, where oauth and ai are Boolean(config.tokenEncryptionKey). So an anonymous caller learns which capabilities the deployment configured.
No secret leaks and the values are useful, so this may well be the right trade. What changes it is the mount: embedded, this lands on /bff/health of the customer's own public application, where standalone it lived on a separately deployed port they could firewall. Worth deciding deliberately rather than inheriting — a features block gated behind the same credential the rest of the surface uses would keep the operational value without the anonymous reconnaissance.
What is not a judgment call: test/http/bff-http-server.test.ts has a case named should never expose config presence or secret values in the response body, and it stays green because it only greps for the secret literals. The invariant its name asserts is now false. Either the disclosure goes, or that test's name and body do.
Related in the same file's suite: the /health case was weakened from toEqual to toMatchObject({ status, version }) and renamed nothing, so its "with 200 ok and the version only" is also false now that features is in the body — and the four flags have no assertion at all on the BFFHttpServer path (build-bff.test.ts does pin them for the buildBff path).
There was a problem hiding this comment.
Test names fixed, disclosure kept.
The /health case is renamed and now asserts the full body including all four flags, plus a second case pinning them on a partial config — the BFFHttpServer path had none. The invariant test is renamed to what it actually guarantees ("secret values or the config itself"), which is the claim that survives.
On the trade: I am keeping features. It names capabilities, never values, and it is what makes an embedded /bff/health useful to a load balancer or to support — which is the deployment where the caller cannot reach a second port to ask. Gating it behind the same credential as the rest would leave a probe with nothing to read. Worth reopening if you would rather have it behind auth.
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>
|
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. |
2 participants
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
addBff()toAgentto serve an embedded BFF in-processAgent.addBff()mounts a BFF at/bffusing an in-process dispatcher, managed alongside the agent lifecycle (start/stop/restart) in agent.ts and embedded-bff.tsagent-bffgainsresolveTransportto choose between in-process and HTTP transport; when neither a dispatcher norAGENT_URLis set, data and action routes fall back to stubs in build-bff.ts/healthendpoint now returns afeaturesobject (oauth, ai, cors, openapi) and derives status from an explicithealthyboolean in health-route.ts@forestadmin/agent; the oldagent-bffintegration test file is deletedengines.node: >=22.12.0and declares@forestadmin/agent-bffas an optional dependency; if the package is missing at runtime,EmbeddedBffthrows a guidance error.Agent.mountAiMcpServer()now throws on MCP basePath/bffcollisionChanges since #1876 opened
EmbeddedBffclass to implement a two-phase lifecycle withprepare()for early configuration validation andstart()for building the BFF server, forwarding counter metrics to the host logger at Warn level while dropping gauges, preservingoriginalUrland rewritingreq.urlwhen handling requests under/bff, returning 503 responses with distinct error types (bff_not_startedorbff_stopped) when not serving, and wrapping import failures of@forestadmin/agent-bffwith clearer error messages that retain the original cause [271c47b]Agent.start()to callthis.embeddedBff?.prepare()before building routes or subscribing to events for early BFF configuration validation, and updatedAgent.addBff()andAgent.mountAiMcpServer()to use a newcollidesWithBff()utility that normalizes base paths and detects collisions with any path under/bff[271c47b]/healthendpoint inbuildBff()to require a non-emptyFOREST_AUTH_SECRETwhen using an in-process dispatcher, returning degraded status (503) if the secret is missing, and added afeaturesblock to health responses containing OAuth, AI, CORS, and OpenAPI configuration flags [271c47b]collidesWithBff()utility function inbff-routesthat normalizes a provided base path by trimming whitespace, ensuring a leading slash, collapsing multiple slashes, and dropping trailing slashes, then returns true if the normalized path with/mcpappended would land under/bff[271c47b]embedded-bff.e2e.test.tsto extract a reusableitServesTheDataContractsuite that runs over both in-process and HTTP transports, added assertions for search behavior including plain search, relation-extended search, query syntax crossing relations, filter vs search semantics, whitespace handling, and counts, and updated health checks and stopped-agent behavior assertions to match new 503 error types [271c47b]buildBff()behavior with an in-processAgentDispatcher, lifecycle behavior including restart invalidation, early failure on invalid options, 503 responses with distinct error types pre-start and post-stop, URL prefix handling withoriginalUrlpreservation, metrics forwarding, error context serialization in logs, and missing dependency scenarios [271c47b]BffEmbedOptionstype frompackages/agent/src/index.tsand expanded documentation comments for its fields to clarify OAuth gating, CORS semantics, agent timeout implications, and OpenAPI exposure [271c47b]llm.integration|search-agent.integrationtollm.integration|embedded-bff.e2eand revised comments and job names to reflect both embedded and HTTP transports for BFF integration tests [271c47b]Macroscope summarized 339fda7.