feat(agent-bff): let the emitted paths carry a mount prefix - #1873
feat(agent-bff): let the emitted paths carry a mount prefix#1873nbouliol wants to merge 2 commits into
Conversation
The docs page embeds absolute paths for the document and the redoc bundle,
and the OpenAPI document declares `servers: [{ url: '/' }]`. Both assume the
BFF owns the origin root, which stops being true the moment a host serves it
under a prefix: the page would fetch a document that is not there, and a
generated client would target the wrong base url.
`basePath` is emitted, never routed on — the host strips its prefix before the
request lands here, so the routes stay exactly where they were.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 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 (3)
🛟 Help
|
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
Spec (PRD-1076): step 4 is half implemented. Its first sentence — thread basePath into the two absolute paths in docs-routes.ts and into the OpenAPI servers — landed, and landed well. Its second sentence did not:
Derive the emitted base from the request when the host framework exposes it (
req.originalUrlunder Express), falling back tobasePath: an agent mounted underapp.use('/api', router)routes correctly (Express strips the mount path) but would otherwise emit/bff/...where the browser needs/api/bff/....
git grep originalUrl -- packages/agent-bff/src is empty at this SHA and the diff mentions neither req. nor originalUrl. So the case the ticket wrote that requirement for — a host mounting the agent under its own prefix — still emits the wrong base. Worth saying whether it is deferred on purpose; the static half is a prerequisite for it either way, so this is not a blocker for the PR, just a gap the ticket named.
What is right, and it is the part I expected to be wrong. The prefix lands in servers[].url exactly once and the operation paths stay unprefixed, so the absolute URL composes correctly with no double-count and no drop. Both document forms get it symmetrically (generic via openapi-routes.ts, unfolded via unfolded-document.ts). Route matching is untouched — ctx.path comparisons still use the unprefixed constants — which is what the host-strips design requires, and the test at test/build-bff.test.ts guards it.
Claude Opus 5 (claude-opus-5): Preferential
Applies to: the PR as a whole — coordination with #1866.
#1866 (publish BFF_PUBLIC_URL as the openapi server url, still open on main) writes the same servers[0].url this PR writes. They are not just a textual conflict: the two values must be composed, not chosen. For a BFF embedded under /bff behind https://app.example.com, the correct value is https://app.example.com/bff — basePath alone loses the host, BFF_PUBLIC_URL alone loses the mount prefix.
Neither PR's tests exercise both options at once, so whichever merges second can resolve the conflict by keeping its own value and no test will fail. PRD-1076 never mentions BFF_PUBLIC_URL at all, so the ticket does not arbitrate it either. Whoever lands second wants one assertion: basePath: '/bff' + BFF_PUBLIC_URL: 'https://host' → servers[0].url === 'https://host/bff'.
|
|
||
| const page = bundle ? renderDocsPage(documentPath, DOCS_BUNDLE_PATH) : undefined; | ||
| const page = bundle | ||
| ? renderDocsPage(`${basePath}${documentPath}`, `${basePath}${DOCS_BUNDLE_PATH}`) |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Must fix
Applies to: packages/agent-bff/src/docs/docs-samples.ts:330 and :309 (not in this diff) — anchored here because this is the line that prefixes the other emitted URLs on the same page, and the samples are the one site the prefix was not threaded into.
Every copy-pasteable snippet on the docs page of a prefixed deployment targets the wrong URL. Under basePath: '/bff' the page is served at https://host/bff/docs, renders a document whose paths are right, and emits curl -X POST 'https://host/agent/v1/<collection>/list' — 404, or worse a 200 from whatever unrelated app owns the origin root. All three languages, since curlSample / nodeSample / rubySample share the origin.
Mechanism: decorateWithSamples(spec, window.location.origin) — location.origin is scheme + host and never a path — composed with the operation path, which this PR deliberately leaves unprefixed. spec.servers[0].url, the one place the prefix now lives, is never read. Redoc's own base-URL display is correct while the snippet next to it is wrong, which is the worst version of this.
The module's own header comment is the reasoning that hides it: "the document declares servers: [{ url: '/' }], so a sample built into it could only hold a placeholder host, while the page knows where it is served from". After this PR the document does not declare /, and the page does not know where it is served from. That comment needs updating with the fix.
One line, and it makes the samples self-consistent with the servers entry this PR adds rather than adding plumbing:
var mount = ((spec.servers || [])[0] || {}).url || '/';
return decorateWithSamples(spec, window.location.origin + (mount === '/' ? '' : mount));Found independently by three separate review passes; the test gap that let it through is that test/build-bff.test.ts asserts the document and bundle URLs carry /bff and nothing asserts a sample URL.
There was a problem hiding this comment.
Fixed: withSamples now builds on sampleBase(spec), which composes window.location.origin with the document's own servers[0].url (a server url that already carries a host is taken as it stands, / means the origin root), the module header comment now states that composition instead of the old servers: [{ url: '/' }] reasoning, and docs-page.test.ts asserts the emitted sample urls for /bff, / and https://public.example.com/bff/.
| export default async function buildBff({ | ||
| config, | ||
| logger = createConsoleLogger(), | ||
| basePath = '', |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
basePath is a public option on the exported BuildBffOptions, so its value is whatever an embedding host — often an env var — hands over, and it is interpolated raw at every site with no trim, no leading-slash enforcement and no trailing-slash collapse. Executed, not reasoned (resolved from a page at https://host/docs):
basePath emitted doc url resolves to
"/bff" /bff/agent/openapi.json https://host/bff/agent/openapi.json correct
"/" //agent/openapi.json https://agent/openapi.json <-- "agent" read as the HOST
"/bff/" /bff//agent/openapi.json https://host/bff//agent/openapi.json 404 after the host strips /bff
"bff" bff/agent/openapi.json https://host/bff/agent/openapi.json page ok, but servers="bff" is relative
'/' is the input that hurts most: //agent/openapi.json is an RFC 3986 network-path reference, so the browser resolves agent as a hostname. Same for <script src="//docs/redoc.standalone.js"> → https://docs/.... The page renders blank with a cross-origin failure. 'bff' is the likeliest typo: the page happens to resolve, but servers: [{ url: 'bff' }] is a relative server URL that generators resolve against the document location, so every generated client targets a base nobody chose — and the two disagree silently.
The precedent is in this monorepo, for this same concept: packages/mcp-server/src/mcp-paths.ts:3-23. normalizeMountPath trims, maps '/' to '', prepends the leading slash, collapses duplicate and trailing slashes, allowlists the characters, and throws Invalid MCP mount path "…": use a plain path prefix like "/mcp". Its comment gives the reason directly: the prefix is interpolated raw into route patterns and resolved through new URL(), so both interpretations must agree. That is exactly the failure set above.
It also holds the new input to a lower bar than the old one in the same function body: build-bff.ts warns and lists the malformed BFF_ALLOWED_ORIGINS entries a few lines down. Normalise once here, before anything is interpolated, and reject with a message naming the value.
There was a problem hiding this comment.
Fixed: normalizeBasePath (src/base-path.ts, modelled on mcp-paths.ts) trims, maps ''/'/' to '', prepends the leading slash, collapses duplicate and trailing slashes, and throws ConfigurationError('Invalid BFF base path "…": use a plain path prefix like "/bff" …') on anything else — called as the first statement of buildBff, before any value is interpolated, so a bad mount fails at boot rather than as a docs page that cannot load itself.
| } ${SHARED_DESCRIPTION}`, | ||
| }, | ||
| servers: [{ url: '/' }], | ||
| servers: [{ url: basePath || '/' }], |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
Applies to: packages/agent-bff/src/cli-dispatch.ts:99 and :104 (not in this diff) — anchored on the line whose value the export omits.
The CLI export and the live route now describe the same deployment differently, and both succeed. forest-bff openapi calls generateOpenApiDocument(version, { hasAiQueryRoute }) and buildUnfoldedDocument(…) without basePath, and no env key carries one — basePath exists only as a code-level option. So for a deployment served under /bff, the live /agent/openapi.json emits servers: [{ url: '/bff' }] while the document a CI job commits and generates an SDK from emits servers: [{ url: '/' }].
Who finds out: not whoever ran the export — exit 0, no warning — but whoever consumes the generated client later and gets 404s against /agent/…. New with this PR: before it there was no prefix to disagree about.
Either thread a BFF_BASE_PATH env key through the export, or have the export state in its output that it assumes the origin root.
There was a problem hiding this comment.
Not changing this, and I don't think the two disagree: basePath reaches the BFF only through buildBff({ basePath }), and the CLI's single build site is cli-core.ts:15 — buildBff({ config, logger }), no prefix — so every deployment forest-bff can serve owns the origin root, and the document it exports (servers: [{ url: '/' }]) is exactly what that same binary serves. The host that does set a prefix sets it in code (the embedded agent passes the constant /bff), so no env key could carry its value anyway, and adding BFF_BASE_PATH for the export alone would create the divergence it is meant to close — that only makes sense as one key threaded through both cli-core and renderOpenApi, which is a separate change.
| basePath: '/bff', | ||
| }); | ||
|
|
||
| const response = await request(callback).get('/docs'); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix
This assertion cannot fail on the bug it exists to catch. toContain('/bff/agent/openapi.json') is satisfied by "/bff/bff/agent/openapi.json" — verified, includes returns true — so a regression that applies the prefix twice passes green.
The sibling case for the unprefixed deployment gets this right by anchoring the delimiters ('"/agent/openapi.json"', 'src="/docs/redoc.standalone.js"'), which is exactly why it can fail on a stray prefix. Use the same anchored form here: '"/bff/agent/openapi.json"' and 'src="/bff/docs/redoc.standalone.js"'.
There was a problem hiding this comment.
Fixed: both assertions now anchor their delimiters ('"/bff/agent/openapi.json"' and 'src="/bff/docs/redoc.standalone.js"'), so a doubled prefix fails instead of passing.
| const { document, unfolding } = await buildUnfoldedDocument(source, readModel, token, { | ||
| version, | ||
| hasAiQueryRoute, | ||
| basePath, |
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
The prefixed variant of the served document has no case, and the untested branch is the production one. The only basePath assertion on servers is the pure-function unit test in test/openapi/openapi-document.test.ts; nothing at the routes level asserts the emitted servers under a prefix, and the new prefixed block in test/build-bff.test.ts only requests /docs, never /agent/openapi.json.
So dropping or mistyping this basePath forward — the unfolded branch, which is what a deployment with an agent URL actually serves — leaves the whole suite green while an embedded deployment publishes a document whose servers is /. Every client generated from it then targets the host root, and this repo treats generated clients as the contract (test/openapi/openapi-generated-client.test.ts exists for that reason), so all data and action calls 404.
Smallest fix: one case asserting JSON.parse(response.text).servers equals [{ url: '/bff' }] for an authenticated caller with basePath: '/bff'.
Same shape one line up in unfolded-document.ts: deleting basePath from that destructure also leaves every assertion in this PR green.
There was a problem hiding this comment.
Fixed: openapi-routes.test.ts now asserts servers equals [{ url: '/bff' }] on the served document in both branches — unfolded through the read-model store, and generic — plus that the operation paths stay unprefixed, so dropping the basePath forward in either openapi-routes.ts or unfolded-document.ts now fails.
| * it strips the prefix before the request lands here — but the paths the BFF *emits* (the OpenAPI | ||
| * `servers` entry, the docs page's asset and document urls) must carry it. | ||
| */ | ||
| basePath?: string; |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Preferential
basePath is now part of the published surface of @forestadmin/agent-bff and is documented nowhere outside this doc comment — not in the package README, not in the docs. Given the normalisation findings above, the two things a host needs stated are the accepted shape (leading slash, no trailing slash) and that it changes only what the BFF emits, never what it serves, because the host is expected to strip the prefix before dispatching.
The README lands in a later PR of this stack, so this is a note rather than a request — worth making sure it covers this option when it does.
There was a problem hiding this comment.
Noted, and no change here: I'll make sure the docs PR of the stack (#1877) states the accepted shape (a plain path prefix, leading slash optional, trailing slash and / collapsed, anything else rejected at boot) and that basePath only changes what the BFF emits, never what it routes on.
basePath was interpolated raw into every emitted url, so the two values a host is most likely to hand over broke the very page it exists to fix: '/' emitted //agent/openapi.json, which a browser resolves with "agent" as the HOST, and a bare 'bff' emitted a relative servers url, which a generator resolves against wherever it read the document from rather than against the origin. Normalize once at the entry - as mcp-paths.ts already does for the MCP mount - and reject anything that is not a plain path prefix at boot, naming the value. The docs page's code samples were the one site the prefix never reached: they composed location.origin with the operation path, so a prefixed deployment published copy-pasteable commands that 404, or worse reach whatever owns the origin root. They now read the document's own servers[0].url, which is also what keeps them consistent with a generated client. Two assertions could not fail on the bug they guard: the prefixed page test was unanchored, so a doubled prefix passed, and nothing asserted the servers entry of the SERVED document - dropping the forward in openapi-routes.ts or unfolded-document.ts left the suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On the two points in the review body:
|
2 participants
Stacked on #1872.
Why
Two places assume the BFF owns the origin root:
renderDocsPageembeds absolute paths —/agent/openapi.jsonand/docs/redoc.standalone.js— and a browser resolves those against the origin, not against the page.servers: [{ url: '/' }], which is what a generated client uses as its base url.Serve the BFF under
/bffand both are wrong: the viewer fetches a document that is not there, and a generated client targets paths that 404.What
buildBff({ basePath }), default'', threaded into the docs page and the document'sservers.basePathis emitted, never routed on: the host strips its own prefix before the request reaches this app, so every route stays exactly where it was — there is a test for that, since it is the part that would be easy to get wrong later.basePathis normalized once, at the entry (normalizeBasePath, modelled onmcp-server'snormalizeMountPath):''and'/'mean the origin root, a leading slash is optional, duplicate and trailing slashes collapse, and anything that is not a plain path prefix throws at boot.'/'is the input that mattered — interpolated raw it emits//agent/openapi.json, which a browser resolves withagentas the host.The docs page's code samples read the prefix off the document's own
servers[0].urlrather than assuminglocation.originis the base, so the snippets and a generated client cannot disagree.Tests
83 suites, 1390 tests.
build-bffcovers the prefixed and unprefixed page (delimiters anchored, so a doubled prefix fails), the'/'case, a rejected prefix, and the invariant that routing is unaffected;openapi-routesassertsserverson the served document in both the unfolded and generic branches;base-pathcovers normalization and rejection;docs-pageasserts the emitted sample urls for a prefix, the root, and a server url that already carries a host.Fixes PRD-1076
🤖 Generated with Claude Code
Note
Add
basePathoption toagent-bffso emitted paths carry a mount prefixbasePath?: stringonBuildBffOptionsand threads it throughbuildAgentMiddlewares,createOpenApiRoutes,createDocsRoutes, andbuildUnfoldedDocument.generateOpenApiDocumentnow sets the OpenAPIserversfield to[{ url: basePath || '/' }]instead of always/, so generated clients target the prefixed URL.createDocsRoutesprefixes the document and Redoc bundle URLs in the served HTML whenbasePathis provided.servers, docs asset links) reflectbasePath. Default remains''(unprefixed), so existing callers see no difference unless they passbasePath.Changes since #1873 opened
normalizeBasePathfunction to validate and normalize base path configuration [951f8ba]buildBfffunction to normalize base path and propagate it through middleware and routes [951f8ba]serversentry with normalized base path [951f8ba]servers[0].urlinstead of page origin [951f8ba]📊 Macroscope summarized fcca9df. 5 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues