From c635c537c904b55eca2bdc054fb4d984e869ebac Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Sat, 29 Aug 2026 22:42:51 +0530 Subject: [PATCH] fix(edge): stop agent-edge 404ing every real /api/* route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agent-edge.mjs` gained a `path.startsWith('/api/')` catch-all in 4c67733 that returned a JSON 404 *before* `worker.mjs` ever delegated to OpenNext. Only `/api/ai` sat above it, so every other `GET /api/*` was answered by the edge and never reached Next.js. Live on starboard.codevetter.com: GET /api/discover -> 404 "Unknown API path" POST /api/discover -> 405 (the real Next.js handler — the route exists) GET /api/health -> 404 GET /api/auth/session, /api/auth/callback/github -> 404 That is 30 of the repo's 31 route handlers, plus the entire NextAuth surface (session, csrf, providers, signin, the OAuth callback), plus the four tools of the public Starboard MCP connector. The fix inverts who decides existence. The edge keeps a pre-handler for the surfaces it genuinely owns (`/llms.txt`, `/llms-full.txt`, `/index.md`, `/openapi.json`, `/api/ai`, homepage markdown negotiation) and returns `null` for everything else. The JSON-404 envelope moves to `withApiJsonNotFound`, a post-handler applied to the response coming back from OpenNext: only when *Next.js itself* reports 404 for an `/api/*` path does the HTML error page get swapped for JSON. The edge never asserts that a path does not exist, so adding a route handler under `src/app/api/` needs no edge change and the shadowing cannot come back. The markdown-404 branch now also skips `/api/*` — an `Accept` header must not divert a real API request away from its handler. Regression coverage at two levels, both proven to fail before this change: - `src/__tests__/agent-edge-api-routing.test.ts` drives the real `worker.mjs` with a stubbed OpenNext handler (aliased in `vitest.config.ts`, since `.open-next/` is gitignored). The guarded route list is read off the filesystem, so new handlers are covered the moment they land. Before: 35 failed / 7 passed. After: 42 passed. - `e2e/public-app.spec.ts` asserts the same through real workerd via `wrangler dev`. Before: failed on `/api/health`. After: passed. Needs a deploy to take effect in production. Co-Authored-By: Claude Opus 5 (1M context) --- agent-edge.mjs | 56 ++++- docs/development/conventions.md | 8 + e2e/public-app.spec.ts | 25 +++ src/__tests__/agent-edge-api-routing.test.ts | 195 ++++++++++++++++++ .../fixtures/open-next-worker-stub.mjs | 34 +++ vitest.config.ts | 13 +- worker.mjs | 12 +- 7 files changed, 327 insertions(+), 16 deletions(-) create mode 100644 src/__tests__/agent-edge-api-routing.test.ts create mode 100644 src/__tests__/fixtures/open-next-worker-stub.mjs diff --git a/agent-edge.mjs b/agent-edge.mjs index 0d64de2..0f98d6b 100644 --- a/agent-edge.mjs +++ b/agent-edge.mjs @@ -3,9 +3,11 @@ * Spec: foundry/ops/docs/agent-indexing-standard.md * * Usage in worker.mjs (before openNext.fetch): - * import { handleAgentEdge } from './agent-edge.mjs' + * import { handleAgentEdge, withApiJsonNotFound } from './agent-edge.mjs' * const agent = handleAgentEdge(request) * if (agent) return agent + * // ...and on the way back out, so unknown /api/* paths answer in JSON: + * return withApiJsonNotFound(request, await openNext.fetch(request, env, ctx)) */ /** @type {{ name: string, url: string, llmsTxt: string, llmsFullTxt?: string, indexMd: string, catalog: object }} */ @@ -88,6 +90,18 @@ export const AGENT_SURFACE = { }; /** + * Paths under `/api/` that the edge itself serves. Everything else under + * `/api/` belongs to Next.js and MUST fall through — see `withApiJsonNotFound`. + */ +const EDGE_OWNED_API_PATHS = new Set(['/api/ai']); + +/** + * Pre-handler: answers only for surfaces the edge itself owns. + * + * Returns `null` for everything else so the request reaches OpenNext/Next.js. + * This function must never decide that a path does *not* exist — only Next.js + * knows the route table. + * * @param {Request} request * @returns {Response | null} */ @@ -105,7 +119,7 @@ export function handleAgentEdge(request) { if (path === '/index.md') { return text(AGENT_SURFACE.indexMd, 'text/markdown; charset=utf-8'); } - if (path === '/api/ai') { + if (EDGE_OWNED_API_PATHS.has(path)) { // Re-bind origin so preview/custom domains stay correct const catalog = { ...AGENT_SURFACE.catalog, @@ -129,11 +143,6 @@ export function handleAgentEdge(request) { return json(openapiSpecForOrigin(url.origin)); } - // JSON error for unknown /api/* paths - if (path.startsWith('/api/')) { - return jsonError(404, 'not_found', `Unknown API path: ${path}`, path); - } - // Homepage markdown negotiation if ((path === '/' || path === '') && wantsMarkdown(request)) { return text(AGENT_SURFACE.indexMd, 'text/markdown; charset=utf-8', { @@ -142,14 +151,43 @@ export function handleAgentEdge(request) { }); } - // Agent-friendly 404: markdown body for Accept: text/markdown - if (wantsMarkdown(request) && !path.includes('.')) { + // Agent-friendly 404: markdown body for Accept: text/markdown. + // `/api/*` is excluded: those paths are Next.js route handlers, and an + // Accept header must never stop a real API request from reaching them. + // Unknown API paths are shaped into JSON by `withApiJsonNotFound` instead. + if (!isApiPath(path) && wantsMarkdown(request) && !path.includes('.')) { return markdown404(path, url.origin); } return null; } +/** + * Post-handler: shape Next.js's own 404 for `/api/*` into a JSON error body. + * + * The edge deliberately does not know which API routes exist — Next.js does. + * We call it, and only if *it* reports 404 do we swap the HTML error page for + * the machine-readable JSON envelope. That is why this cannot rot the way the + * previous allow-list did: adding a route handler under `src/app/api/` makes it + * reachable with no edge change, because the edge never asserts non-existence. + * + * @param {Request} request + * @param {Response} response Response from the downstream Next.js handler. + * @returns {Response} + */ +export function withApiJsonNotFound(request, response) { + if (request.method !== 'GET' && request.method !== 'HEAD') return response; + if (response.status !== 404) return response; + const path = new URL(request.url).pathname; + if (!isApiPath(path)) return response; + if ((response.headers.get('content-type') || '').includes('application/json')) return response; + return jsonError(404, 'not_found', `Unknown API path: ${path}`, path); +} + +function isApiPath(pathname) { + return pathname.startsWith('/api/'); +} + function wantsMarkdown(request) { const accept = (request.headers.get('accept') || '').toLowerCase(); if (!accept.includes('text/markdown')) return false; diff --git a/docs/development/conventions.md b/docs/development/conventions.md index c5dc1a9..c819c25 100644 --- a/docs/development/conventions.md +++ b/docs/development/conventions.md @@ -64,6 +64,14 @@ Code style and repo conventions. The executable source of truth is - `worker.mjs` — OpenNext-generated Worker entry. - `cloudflare-env.d.ts` — generated by `wrangler types`. +**Carry-forward when regenerating `agent-edge.mjs`:** the upstream template's +`/api/*` catch-all 404 shadowed every real Next.js route handler in production +(only `/api/ai` was allow-listed above it). The local file replaces that +pre-emptive catch-all with `withApiJsonNotFound`, a post-handler that shapes +*Next.js's own* 404 into JSON. Regenerating from an unfixed template +reintroduces the outage — `src/__tests__/agent-edge-api-routing.test.ts` fails +loudly if it comes back. + ## Agent skills / plugins — do not modify Do not modify, move, rename, or delete agent skills, plugins, or agent-profile diff --git a/e2e/public-app.spec.ts b/e2e/public-app.spec.ts index 66d80dd..e858904 100644 --- a/e2e/public-app.spec.ts +++ b/e2e/public-app.spec.ts @@ -322,3 +322,28 @@ test('uncataloged public preview asks for sign-in without presenting a rate-limi await expect(page.getByRole('link', { name: 'Sign in to preview' })).toBeVisible(); await expect(page.getByText(/rate limit/i)).toHaveCount(0); }); + +test('public API routes are reachable through the Worker edge on GET', async ({ request }) => { + // Regression guard for the `agent-edge.mjs` `/api/*` catch-all that 404'd + // every real route handler before OpenNext ever saw the request. Statuses + // vary with the e2e database contents, so assert only that these are NOT the + // edge's "Unknown API path" 404. + for (const path of ['/api/health', '/api/discover', '/api/tools', '/api/auth/session']) { + const response = await request.get(path, { maxRedirects: 0 }); + expect( + response.status(), + `${path} was 404'd — the edge is shadowing the Next.js route again` + ).not.toBe(404); + } + + // The edge's own surface still works... + const catalog = await request.get('/api/ai'); + expect(catalog.status()).toBe(200); + expect((await catalog.json()).name).toBe('Starboard'); + + // ...and a genuinely unknown API path still answers in JSON, not HTML. + const unknown = await request.get('/api/definitely-not-a-real-path'); + expect(unknown.status()).toBe(404); + expect(unknown.headers()['content-type']).toContain('application/json'); + expect((await unknown.json()).error.code).toBe('not_found'); +}); diff --git a/src/__tests__/agent-edge-api-routing.test.ts b/src/__tests__/agent-edge-api-routing.test.ts new file mode 100644 index 0000000..017a15b --- /dev/null +++ b/src/__tests__/agent-edge-api-routing.test.ts @@ -0,0 +1,195 @@ +/** + * Regression guard for the edge shadowing real API routes. + * + * Commit 4c67733 gave `agent-edge.mjs` a `path.startsWith('/api/')` catch-all + * that answered 404 *before* OpenNext ever saw the request, with only + * `/api/ai` allow-listed above it. Every other `GET /api/*` — Discover, health, + * tools, the whole NextAuth surface — was 404'd at the edge in production for + * six days. + * + * These tests drive the real `worker.mjs` entrypoint with a stubbed OpenNext + * handler (see `fixtures/open-next-worker-stub.mjs`), so they cover the actual + * wiring, not a reimplementation of it. + * + * The route list is read off the filesystem rather than hard-coded: a route + * handler added under `src/app/api/` is covered the moment it lands, which is + * exactly the rot that the original hand-maintained allow-list suffered. + */ +import { readdirSync } from 'node:fs'; +import { join, resolve } from 'node:path'; +import { beforeEach, describe, expect, it } from 'vitest'; + +import { openNextStub } from './fixtures/open-next-worker-stub.mjs'; +import worker from '../../worker.mjs'; + +const API_DIR = resolve(__dirname, '../app/api'); + +/** Turn `src/app/api/repos/[repoId]/route.ts` into a concrete `/api/repos/sample`. */ +function collectApiRoutePaths(dir: string, prefix = '/api'): string[] { + const paths: string[] = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (entry.isDirectory()) { + const segment = entry.name.startsWith('[') + ? // Dynamic and catch-all segments get a concrete stand-in value. + entry.name.startsWith('[...') || entry.name.startsWith('[[...') + ? 'sample/segment' + : 'sample' + : entry.name; + paths.push(...collectApiRoutePaths(join(dir, entry.name), `${prefix}/${segment}`)); + } else if (entry.name === 'route.ts' || entry.name === 'route.tsx') { + paths.push(prefix); + } + } + return paths; +} + +const API_ROUTE_PATHS = collectApiRoutePaths(API_DIR); + +const ctx = { waitUntil: () => undefined, passThroughOnException: () => undefined }; +const env = {} as Record; + +function get(path: string, headers: Record = {}) { + return worker.fetch( + new Request(`https://starboard.codevetter.com${path}`, { headers }), + env, + ctx + ); +} + +beforeEach(() => { + openNextStub.reset(); +}); + +describe('edge does not shadow Next.js API routes', () => { + it('found the route handlers to guard', () => { + // Sanity check: if this ever hits zero the suite below is vacuous. + expect(API_ROUTE_PATHS.length).toBeGreaterThan(20); + expect(API_ROUTE_PATHS).toContain('/api/discover'); + expect(API_ROUTE_PATHS).toContain('/api/health'); + }); + + it('reaches a real API route through the edge on GET', async () => { + openNextStub.handler = () => + new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + + const response = await get('/api/discover'); + + expect(openNextStub.calls).toEqual(['/api/discover']); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ ok: true }); + }); + + it.each(API_ROUTE_PATHS)('lets GET %s reach Next.js', async (path) => { + openNextStub.handler = () => new Response('routed', { status: 200 }); + + const response = await get(path); + + expect(openNextStub.calls).toEqual([path]); + expect(response.status).toBe(200); + }); + + it('does not let an Accept header divert an API route to the markdown 404', async () => { + openNextStub.handler = () => new Response('routed', { status: 200 }); + + const response = await get('/api/discover', { accept: 'text/markdown' }); + + expect(openNextStub.calls).toEqual(['/api/discover']); + expect(response.status).toBe(200); + }); +}); + +describe('unknown API paths still answer with the JSON 404 envelope', () => { + it('replaces the Next.js HTML 404 with JSON', async () => { + openNextStub.handler = () => + new Response('404', { + status: 404, + headers: { 'content-type': 'text/html; charset=utf-8' }, + }); + + const response = await get('/api/definitely-not-a-real-path'); + + expect(openNextStub.calls).toEqual(['/api/definitely-not-a-real-path']); + expect(response.status).toBe(404); + expect(response.headers.get('content-type')).toContain('application/json'); + expect(await response.json()).toEqual({ + error: { + code: 'not_found', + message: 'Unknown API path: /api/definitely-not-a-real-path', + path: '/api/definitely-not-a-real-path', + }, + }); + }); + + it("leaves a route handler's own 404 JSON body untouched", async () => { + openNextStub.handler = () => + new Response(JSON.stringify({ error: 'repo not found' }), { + status: 404, + headers: { 'content-type': 'application/json' }, + }); + + const response = await get('/api/repos/999999'); + + expect(await response.json()).toEqual({ error: 'repo not found' }); + }); + + it('leaves non-API 404s alone so the HTML error page still renders', async () => { + openNextStub.handler = () => + new Response('404', { + status: 404, + headers: { 'content-type': 'text/html; charset=utf-8' }, + }); + + // Deliberately not a cacheable document path — the edge HTML cache branch + // needs `caches.default`, which only exists in workerd. + const response = await get('/no-such-page'); + + expect(openNextStub.calls).toEqual(['/no-such-page']); + expect(response.status).toBe(404); + expect(response.headers.get('content-type')).toContain('text/html'); + }); +}); + +describe('edge-owned surfaces still work', () => { + it('serves the agent catalog at /api/ai without touching Next.js', async () => { + const response = await get('/api/ai'); + + expect(openNextStub.calls).toEqual([]); + expect(response.status).toBe(200); + const catalog = (await response.json()) as { name: string; openapi: string }; + expect(catalog.name).toBe('Starboard'); + expect(catalog.openapi).toBe('https://starboard.codevetter.com/openapi.json'); + }); + + it('serves the OpenAPI spec', async () => { + const response = await get('/openapi.json'); + + expect(openNextStub.calls).toEqual([]); + expect(response.status).toBe(200); + const spec = (await response.json()) as { openapi: string }; + expect(spec.openapi).toBe('3.1.0'); + }); + + it('serves llms.txt and index.md', async () => { + expect((await get('/llms.txt')).status).toBe(200); + expect((await get('/index.md')).status).toBe(200); + expect(openNextStub.calls).toEqual([]); + }); + + it('still negotiates markdown on the homepage', async () => { + const response = await get('/', { accept: 'text/markdown' }); + + expect(openNextStub.calls).toEqual([]); + expect(response.headers.get('content-type')).toContain('text/markdown'); + }); + + it('still serves the markdown 404 for unknown non-API pages', async () => { + const response = await get('/no-such-page', { accept: 'text/markdown' }); + + expect(openNextStub.calls).toEqual([]); + expect(response.status).toBe(404); + expect(response.headers.get('content-type')).toContain('text/markdown'); + }); +}); diff --git a/src/__tests__/fixtures/open-next-worker-stub.mjs b/src/__tests__/fixtures/open-next-worker-stub.mjs new file mode 100644 index 0000000..13544a9 --- /dev/null +++ b/src/__tests__/fixtures/open-next-worker-stub.mjs @@ -0,0 +1,34 @@ +/** + * Stand-in for `.open-next/worker.js` (a build artifact, gitignored, so it is + * absent in CI). `vitest.config.ts` aliases the OpenNext worker import to this + * module so `worker.mjs` — the real Cloudflare entrypoint — can be imported and + * exercised in a plain Node test. + * + * Tests set `openNextStub.handler` to decide what "Next.js" answers, and read + * `openNextStub.calls` to assert that a request actually reached it. + */ + +export const openNextStub = { + /** @type {(request: Request) => Response | Promise} */ + handler: () => new Response('stub handler not configured', { status: 500 }), + /** @type {string[]} */ + calls: [], + reset() { + openNextStub.calls = []; + openNextStub.handler = () => new Response('stub handler not configured', { status: 500 }); + }, +}; + +const worker = { + async fetch(request) { + openNextStub.calls.push(new URL(request.url).pathname); + return await openNextStub.handler(request); + }, +}; + +export default worker; + +// `worker.mjs` re-exports these Durable Object classes from the OpenNext entry. +export class DOQueueHandler {} +export class DOShardedTagCache {} +export class BucketCachePurge {} diff --git a/vitest.config.ts b/vitest.config.ts index ff18d8b..6f632d0 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -34,8 +34,15 @@ export default defineConfig({ }, }, resolve: { - alias: { - '@': resolve(__dirname, 'src'), - }, + alias: [ + // `.open-next/worker.js` is a gitignored build artifact, so it does not + // exist in CI. Alias it to a stub so `worker.mjs` — the real Cloudflare + // entrypoint — can be imported and exercised by unit tests. + { + find: /^\.\/\.open-next\/worker\.js$/, + replacement: resolve(__dirname, 'src/__tests__/fixtures/open-next-worker-stub.mjs'), + }, + { find: '@', replacement: resolve(__dirname, 'src') }, + ], }, }); diff --git a/worker.mjs b/worker.mjs index 7a3af6a..88aa982 100644 --- a/worker.mjs +++ b/worker.mjs @@ -13,7 +13,7 @@ import openNext from './.open-next/worker.js'; import { withTiming } from './timing.mjs'; -import { handleAgentEdge } from './agent-edge.mjs'; +import { handleAgentEdge, withApiJsonNotFound } from './agent-edge.mjs'; // Durable Objects must be re-exported from the entry that wrangler.toml // points at, otherwise the bindings can't resolve them at deploy time. @@ -68,18 +68,22 @@ function cacheKeyFor(request, versionId) { const worker = { fetch: withTiming(async function fetch(request, env, ctx) { - // Agent / LLM indexing surfaces (fleet GEO standard) + // Agent / LLM indexing surfaces (fleet GEO standard). This only answers + // for paths the edge itself owns; everything else falls through below. { const agent = handleAgentEdge(request); if (agent) return agent; } try { + // `withApiJsonNotFound` is applied on the way back out: Next.js owns the + // route table, so only its 404 (not an edge guess) turns into a JSON + // error body for `/api/*`. if (request.method !== 'GET') { - return openNext.fetch(request, env, ctx); + return withApiJsonNotFound(request, await openNext.fetch(request, env, ctx)); } const url = new URL(request.url); if (!isCacheableDocumentPath(url.pathname)) { - return openNext.fetch(request, env, ctx); + return withApiJsonNotFound(request, await openNext.fetch(request, env, ctx)); } // Auth-bearing requests pass straight through; the user is likely // going to be redirected by middleware to /library or /dashboard.