refactor(agent-bff): assemble the server behind a single buildBff - #1870
refactor(agent-bff): assemble the server behind a single buildBff#1870nbouliol wants to merge 2 commits into
Conversation
2 new issues
|
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (8)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
Spec (PRD-1076): conforms. Step 1 is delivered as written — buildBff({ config, logger }) -> { callback } exists, runCli decomposes into parseConfig + buildBff + listen, /health and X-Forest-Bff-Version moved into the handler, cli-dispatch follows resolveOAuthConfig / resolveUnfoldSource to their new module, and the public surface is additive only. The invalidate half of the ticket's { callback, invalidate } signature belongs to step 6 and is not expected here.
Iso-behaviour verified against the merge base: all eleven extracted helpers are byte-identical, and the effective middleware order is unchanged (version header, /health, CORS, scoped error middleware, body parser, OAuth, docs, agent chain). Nothing dropped. BFFHttpServer's existing constructor is not broken — version was already required before this PR, and callback is new, so an existing consumer never reaches the ignored-middlewares path.
Four findings inline, none blocking.
| expect(response.headers['x-forest-bff-version']).toBe(version); | ||
| }); | ||
|
|
||
| it('should mount the agent edge behind the health route', async () => { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Violates conventions — skills/conventions/testing.md#Test name states the exact behavior it asserts
A reader of this file will believe the health/agent mount order is covered. It is not: swap the first two entries of buildBff's middleware array and this test still passes green, so the ordering invariant the commit message calls out is unguarded.
A 401 on /agent/... proves the auth chain is mounted; it says nothing about the health route sitting in front of it, because agentScoped never lets the agent chain touch /health and the two paths never collide.
Either rename it to what it verifies (should answer 401 on an unauthenticated agent route), or make the ordering observable — assert that GET /health returns 200 and carries x-forest-bff-version, which only holds if the version header is mounted ahead of the health route, since the health route short-circuits without calling next().
There was a problem hiding this comment.
Fair — agentScoped skips /health and the health route skips /agent/..., so the two never collide and the old name claimed coverage the assertion could not give. Renamed to should answer 401 on an unauthenticated agent route, and the ordering invariant is now pinned by a separate test asserting x-forest-bff-version on the /health 200: I verified it goes red (assertion failure, not timeout) when the header middleware and the health route are swapped.
| const { config, version } = this.options; | ||
| ctx.status = config.hasAllRequired ? 200 : 503; | ||
| ctx.body = { status: config.hasAllRequired ? 'ok' : 'degraded', version }; | ||
| this.handler = options.callback ?? BFFHttpServer.buildHandler(options); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
A host that adds callback without removing middlewares boots successfully, logs Forest BFF started, and then 404s every one of its own routes from bare Koa — with no warning and nothing correlating the 404s to the dropped array. The combination could not exist before this PR, since callback is new.
Both fields are optional on BFFHttpServerOptions, so passing both compiles, and options.callback ?? buildHandler(options) silently discards the array. The doc comment says so; nothing at runtime does. Same shape of problem one field over: version stays required but is dead in the callback branch, so a caller of the new public API has to pass a value that does nothing.
A discriminated union on the options type would make the invalid combination unrepresentable instead of documented. Failing that, a Warn naming the number of dropped middlewares — plus a test pinning the documented behaviour, since test/http/bff-http-server.test.ts only exercises the middlewares branch today.
There was a problem hiding this comment.
Made unrepresentable rather than documented: BFFHttpServerOptions is now a union of AssembledOptions (version required, middlewares optional, callback?: never) and PrebuiltOptions (callback required, version?: never, middlewares?: never). Both invalid combinations — callback + middlewares and callback + version — are compile errors now, verified with a throwaway tsc probe, so the dead-version half is gone too and runCli no longer passes it. Existing consumers passing { port, version, config, logger, middlewares } still fit the assembled variant unchanged. Added a test that the prebuilt handler is served as-is.
| * deployment modes must share — and hand back the request handler. `runCli` puts it behind a | ||
| * listener; an embedding host mounts it on its own server. | ||
| */ | ||
| export default async function buildBff({ |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
Applies to: packages/agent-bff/src/http/bff-http-server.ts:67 (not in this diff) — anchored here because buildBff is where the fix belongs.
The log line that names which required keys are missing lives in BFFHttpServer.start(), which a host mounting callback on its own server never calls. buildBff's own warnings say required configuration is missing without naming a variable, so the two deployment modes now give different diagnostics for the same misconfiguration — in the PR whose stated purpose is that the modes cannot diverge.
Bounded today: the caller builds config itself and has config.presence in hand, so this is a lost convenience rather than blindness. It stops being bounded at addBff(), where the agent calls parseConfig internally and the host never sees presence at all.
The presence diff needs only config.presence and the logger, both already in hand here. Move it into buildBff and drop it from start(), otherwise the CLI logs it twice.
There was a problem hiding this comment.
Agreed — extracted to src/config/missing-config-warning.ts and dropped from start(). One difference from your suggestion: I kept a call site in BFFHttpServer, in the assembled branch of the constructor, otherwise the legacy middlewares path (a published export) would have lost the diagnostic entirely. The branches are exclusive so the CLI still logs it once, and both modes now warn at assembly time rather than after listen. Tests cover both: the assembled branch warns naming the keys, the prebuilt branch stays silent.
|
|
||
| /** | ||
| * Assemble the whole BFF — `/health`, the version header, and every middleware in the one order both | ||
| * deployment modes must share — and hand back the request handler. `runCli` puts it behind a |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Preferential
"an embedding host mounts it on its own server" promises more than the handler delivers, and buildBff is published in index.ts as of this PR — so the first embedder discovers the constraint rather than reading it.
The callback is a terminal Koa handler: it answers 404 itself and never yields to a host next(). A host that routes every path into it loses its own routes; a host that mounts it under a prefix gets /health and /agent/... at the wrong absolute paths. The basePath support that makes the sentence true arrives in a later PR of this stack, but the export ships now.
Worth one clause naming the constraint — mounted at the root, absolute paths, terminal handler.
There was a problem hiding this comment.
Right, app.callback() is terminal. The doc comment now names the constraint: mounted at the root of the host server, terminal handler that answers 404 itself instead of yielding to a host next(), absolute paths, so a prefix mount would move /health and /agent/... off the paths the frontend calls — until basePath lands later in the stack.
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
All four findings verified on 23ccdc5: the ordering test now pins x-forest-bff-version on the /health 200 and the old test is renamed to what it asserts; BFFHttpServerOptions is a union so callback + middlewares/version no longer compiles; warnMissingConfig is shared by both modes and gone from start(); the buildBff doc names the terminal-handler and absolute-path constraint.
`runCli` owned the middleware order, so an embedding host had no way to get the same stack without a listener. Move the whole assembly into `buildBff`, which returns the request handler; `runCli` is now parseConfig + buildBff + listen. `/health` and the version header move with it, so they belong to the handler rather than to the listener. `BFFHttpServer` keeps its `middlewares` constructor working for external consumers and gains an optional `callback`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up on the buildBff extraction. `callback` and `middlewares` were both optional on `BFFHttpServerOptions`, so a host passing both compiled, booted, logged `Forest BFF started` and then 404'd every one of its own routes from bare Koa. The options type is now a union — `callback` forbids `version` and `middlewares` instead of ignoring them — so the invalid combination no longer type-checks. The log naming *which* required keys are absent lived in `start()`, which a host mounting the handler on its own server never calls: same misconfiguration, two different diagnostics, in the change whose point is that the modes cannot drift. It moves to `warnMissingConfig`, called at assembly time by `buildBff` and by the server's own legacy branch. `buildBff`'s doc comment promised a mount it does not support: the handler is terminal and its paths are absolute, so only a root mount works until `basePath` lands. `should mount the agent edge behind the health route` asserted a 401 on `/agent/...`, which the health route can never affect — the two paths never collide. It is renamed to what it checks, and the ordering invariant is now pinned by asserting the version header on `/health`, which only holds while the header sits ahead of the short-circuiting health route. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
23ccdc5 to
72fc3a1
Compare
2 participants
First of the stack that lets an agent embed the BFF in-process (
agent.addBff()).Why
runCliowned the middleware order — cors, error, body parser, oauth, docs, then the agent-scoped chain — so a host wanting the same stack without a listener had no entry point. Rebuilding that order on the agent side would have made the two deployment modes drift silently, and a divergence there is CORS or auth quietly wrong, not a failing test.What
buildBff({ config, logger })returns the request handler and owns the whole assembly.runClibecomesparseConfig+buildBff+listen./healthandX-Forest-Bff-Versionmove into the handler, where they belong: they are part of what the BFF serves, not of how it listens. Both are extracted intocreateHealthRouteandcreateVersionHeaderMiddleware, used bybuildBffand by the legacyBFFHttpServerpath — one implementation, two call sites.BFFHttpServeris a published export, so its{ port, version, config, logger, middlewares }constructor keeps working unchanged; it just gains an optionalcallback.resolveOAuthConfigandresolveUnfoldSourcemove alongside the rest, socli-core.tsno longer reaches intoopenapi/— the mount-invariant allow-list gets tighter, not looser.No behavior change: same middlewares, same order, same responses.
Tests
yarn workspace @forestadmin/agent-bff test— 80 suites, 1350 tests. The existingcli-coresuite still exercises the composed stack end to end throughrunCli; the newbuild-bffsuite covers what moved (health ok/degraded, version header, the agent edge sitting behind the health route, the malformed-origins warning).Fixes PRD-1076
🤖 Generated with Claude Code
Note
Assemble the agent BFF server behind a single
buildBffcallbackcli-core.tsandBFFHttpServerinto the newbuildBfffunction in build-bff.ts, which returns a ready-to-serve Koa callbackBFFHttpServerin bff-http-server.ts now accepts either a prebuilt callback or the legacy internally-assembled options; a prebuilt callback is served without adding extra health, version, or route middlewarecreateHealthRouteandcreateVersionHeaderMiddlewaremiddleware, and a sharedwarnMissingConfighelper that logs missing required keys during constructionbuildBffandBffCallback/BuildBffOptionstypes from the package entrypoint in index.tslistentime; consumers usingbuildBffget the callback directly and bypassBFFHttpServer's internal assembly path entirelyMacroscope summarized 72fc3a1.