From 6c16acbe79ca2cc3fd46b8783fb150fdba80b9ee Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 07:17:43 +0000 Subject: [PATCH] chore(guard): extend the route-envelope check to the dispatcher domains (#4038 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The platform answers REST from two kinds of file. Route modules (*-routes.ts) call `res.json(...)`, which this scan counted. Dispatcher domains (runtime/src/domains/*.ts) RETURN `{ status, body }` for a central sender, so they never call `res.json` and were invisible to it by construction. That gap cost something real: /share-links emitted its payload under both `data` and a legacy `link`/`links` for as long as nothing looked, and it surfaced only because #3983's consumer sweep happened to walk past it (#4038, fixed in #4049). I noted the blind spot there; this closes it. The new table counts hand-built `response: { … }` literals per domain. A domain answering only through the deps.* helpers hand-builds zero and cannot drift; any hand-built one now needs a declared count plus a `note` saying why. Hand-building is not automatically wrong — four kinds show up and only the last is drift: 1. enveloped, but the helper cannot express it — deps.success hardcodes 200 so a 201 is assembled by hand, and deps.error carries no headers so a 405 with `Allow:` is too (keys.ts, share-links.ts, one in mcp.ts) 2. passthrough of a body this dispatcher does not own (mcp.ts, ai.ts) 3. a foreign wire format a client library requires — /auth answers better-auth's shapes because better-auth's client parses them (auth.ts x4) 4. drift 16 domains audited: 11 helper-only, 5 declaring hand-built responses, 1 ratcheted. The ratchet is ai.ts -> #4053, where GET /ai/agents is SDK-addressable but unenveloped and the SDK compensates by reading `.agents` off the raw body — converting it without changing cloud and the SDK together makes `client.ai.agents.list()` return an empty list silently, which is indistinguishable from the legitimate "no AI service configured" state. That warning now fails CI for whoever tries it, instead of sitting in an issue nobody reads. Five self-test assertions cover the new scan. One caught a real false positive while it was being written: matching the `response` key alone counted a PAYLOAD field named response (`deps.success({ response: … })`) as a hand-built response. It now requires `response` to be a sibling of `handled`, which is what makes the pair an HttpDispatcherResult rather than just an object with that key. Verified the guard actually bites: adding a hand-built response to a helper-only domain fails with the site and the four-kind classification prompt. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z --- ...route-envelope-guard-dispatcher-domains.md | 43 +++ scripts/check-route-envelope.mjs | 252 +++++++++++++++++- 2 files changed, 289 insertions(+), 6 deletions(-) create mode 100644 .changeset/route-envelope-guard-dispatcher-domains.md diff --git a/.changeset/route-envelope-guard-dispatcher-domains.md b/.changeset/route-envelope-guard-dispatcher-domains.md new file mode 100644 index 0000000000..5ba6f3cf5b --- /dev/null +++ b/.changeset/route-envelope-guard-dispatcher-domains.md @@ -0,0 +1,43 @@ +--- +--- + +Extend `scripts/check-route-envelope.mjs` to the dispatcher domains — the REST +surface it structurally could not see. Deliberately empty frontmatter: this is a +CI guard, no published behaviour, type, or wire shape changes. + +The platform answers REST from two kinds of file. Route modules (`*-routes.ts`) +call `res.json(...)`, which the scan counted. Dispatcher domains +(`runtime/src/domains/*.ts`) RETURN `{ status, body }` for a central sender, so +they never call `res.json` and were invisible to it by construction. + +That gap cost something real: `/share-links` emitted its payload under both `data` +and a legacy `link` / `links` for as long as nothing looked, and it surfaced only +because #3983's consumer sweep happened to walk past it (#4038, fixed in #4049). + +The new table counts hand-built `response: { … }` literals per domain. A domain +answering only through the `deps.success` / `deps.error` / `deps.routeNotFound` / +`deps.errorFromThrown` helpers hand-builds **zero** and cannot drift; any +hand-built response now has to be declared with a `note` saying why. Hand-building +is not automatically wrong — four kinds show up and only the last is drift: + +1. Enveloped, but the helper cannot express it — `deps.success` hardcodes status + 200, so a 201 must be assembled by hand, and `deps.error` carries no headers, + so a 405 with `Allow:` must be too (`keys.ts`, `share-links.ts`, one in `mcp.ts`). +2. Passthrough of a body this dispatcher does not own — an upstream Web Response, + another service's result (`mcp.ts`, `ai.ts`). +3. A foreign wire format a client library requires — `/auth` answers better-auth's + shapes because better-auth's client parses them (`auth.ts` ×4). +4. Drift. + +Result: **16 domains audited, 11 helper-only, 5 declaring hand-built responses, +1 ratcheted.** The ratchet is `ai.ts` → #4053: `GET /ai/agents` is SDK-addressable +but unenveloped, and the SDK compensates by reading `.agents` off the raw body, so +converting it without changing cloud and the SDK in the same batch makes +`client.ai.agents.list()` return an empty list silently. That warning now fails CI +for whoever tries, instead of sitting in an issue. + +Five self-test assertions cover the new scan, one of which caught a real +false positive while it was being written: matching the `response` key alone +counted a *payload field* named `response` (`deps.success({ response: … })`) as a +hand-built response. It now requires `response` to be a sibling of `handled` — +that pair is the `HttpDispatcherResult`. diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index 6b48e68989..711c28d211 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -18,14 +18,31 @@ * that was a bare string, so `body.error.message` read `undefined` * (#3636 → #3675 → #3689 → #3843). * + * ## Two surfaces + * + * The platform answers REST from two kinds of file, and this audits both: + * + * 1. **Route modules** (`*-routes.ts`) — call `res.json(...)`. Counted below + * in `MODULES`. + * 2. **Dispatcher domains** (`runtime/src/domains/*.ts`) — RETURN + * `{ status, body }` for a central sender, so they never call `res.json` + * and the first scan cannot see them. Counted in `DISPATCHER_DOMAINS`. + * + * Surface 2 was added after that blind spot cost something real: `/share-links` + * emitted its payload under both `data` and a legacy `link` / `links` for as + * long as nothing looked, and it was found by hand while sweeping consumers for + * #3983 rather than by any guard (#4038). + * * ## Why a whole-repo scan rather than a per-module test * - * The load-bearing check is structural, not per-route: it COUNTS the response - * write sites. When every body in a module goes through its `sendOk` / + * The load-bearing check is structural, not per-route: it COUNTS the sites where + * a response gets built. When every body in a module goes through its `sendOk` / * `sendError` pair, that count is fixed at two and does not grow with the route * list — so a *new* route that hand-rolls a body moves the count and fails here, * which is the one thing a driven-body test can never cover (it can only drive - * the routes that existed the day it was written). + * the routes that existed the day it was written). The domain table applies the + * same idea from the other end: a domain that answers only through the `deps.*` + * helpers hand-builds zero responses, so any hand-built one has to be classified. * * #3675 / #3689 shipped this as a regex block copied into each converted * package. Three copies was the signal it wanted lifting (#3843 option 3), and @@ -138,6 +155,88 @@ const MODULES = { /** Identifiers whose `.json()` READS a request rather than writing a response. */ const REQUEST_RECEIVERS = new Set(['req', 'request']); +// ── The dispatcher's OTHER surface ─────────────────────────────────────────── + +/** + * `packages/runtime/src/domains/*.ts` — the dispatcher domain handlers. + * + * These serve REST paths too, but they never call `res.json(...)`: they RETURN + * `{ status, body }` for a central sender. So the scan above cannot see them, by + * construction — which is not a theoretical gap. `/share-links` emitted the + * payload under both `data` and a legacy `link` / `links` for as long as that + * blind spot existed, and it was found by hand while sweeping consumers for + * #3983, not by any guard (#4038). + * + * The structural fact here is the mirror of `responses` above. A domain that + * routes every answer through the `deps.success` / `deps.error` / + * `deps.routeNotFound` / `deps.errorFromThrown` helpers hand-builds **zero** + * response literals, and cannot drift: the envelope lives in one place for all + * of them. So this counts hand-built `response: { … }` object literals per file, + * and a new one has to be classified rather than merged quietly. + * + * handBuilt — `response:` assigned an object literal instead of a `deps.*` call + * note — why those exist. REQUIRED whenever handBuilt > 0. + * ratchet — set when one of them is real, tracked drift. + * + * Hand-building is not automatically wrong. Four kinds show up, and only the + * last is drift: + * + * 1. Enveloped, but the helper cannot express the response. `deps.success` + * hardcodes status 200, so a 201 must be built by hand, and `deps.error` + * carries no headers, so a 405 with `Allow:` must be too. These emit the + * declared envelope — they just assemble it themselves. + * 2. Passthrough of a body this dispatcher does not own (an upstream Web + * Response, another service's result). The envelope does not govern bytes + * we are only relaying. + * 3. A foreign wire format a client library requires — `/auth` answers + * better-auth's shapes because better-auth's client parses them. + * 4. Drift. + */ +const DISPATCHER_DOMAIN_DIR = 'packages/runtime/src/domains'; + +const DISPATCHER_DOMAINS = { + 'actions.ts': { handBuilt: 0 }, + 'analytics.ts': { handBuilt: 0 }, + 'automation.ts': { handBuilt: 0 }, + 'data.ts': { handBuilt: 0 }, + 'i18n.ts': { handBuilt: 0 }, + 'meta.ts': { handBuilt: 0 }, + 'notifications.ts': { handBuilt: 0 }, + 'packages.ts': { handBuilt: 0 }, + 'security.ts': { handBuilt: 0 }, + 'storage.ts': { handBuilt: 0 }, + 'ui.ts': { handBuilt: 0 }, + + // Kind 1 — enveloped, hand-built only because the helper cannot say it. + 'keys.ts': { + handBuilt: 1, + note: 'POST /keys is a 201 and `deps.success` hardcodes 200; the body is the declared envelope', + }, + 'share-links.ts': { + handBuilt: 1, + note: 'POST /share-links is a 201, same reason as /keys (#4038 removed the duplicate key it also carried)', + }, + + // Kinds 1 and 2 together. + 'mcp.ts': { + handBuilt: 2, + note: 'one 405 that must carry an `Allow:` header (`deps.error` takes none) and emits the declared envelope; one passthrough of the upstream MCP Web Response', + }, + + // Kind 3 — a foreign wire format, all four in the mock/fallback path. + 'auth.ts': { + handBuilt: 4, + note: "bridges better-auth, whose client parses ITS shapes (`{ user, session }`, `{ session: null, user: null }`, `{ success: true }`) — BaseResponseSchema does not govern them", + }, + + // Kind 4 — real drift, tracked. + 'ai.ts': { + handBuilt: 2, + ratchet: '#4053', + note: 'one passthrough of the AI service result (kind 2); one is the unenveloped `{ agents: [] }` fallback for GET /ai/agents — an SDK-addressable route whose conversion must land with cloud and the SDK together or `ai.agents.list()` silently returns []', + }, +}; + /** * Count the envelope-relevant facts in one module's source. * @@ -213,6 +312,55 @@ export function scanSource(source, fileName = 'module.ts') { return found; } +/** + * Count hand-built response literals in one dispatcher domain's source. + * + * `response: { … }` is hand-built; `response: deps.success(…)` is not. Comments + * and strings are not tokens, so quoting either form in prose cannot move the + * count. + * + * Only a `response` that is a sibling of `handled` counts — that pair IS the + * `HttpDispatcherResult`. A payload field that happens to be named `response` + * (`deps.success({ response: … })`) is data, not a response, and the self-test + * pins the difference: matching the key alone counted it. + * + * @param {string} source TypeScript source text. + * @returns {{handBuilt: number, sites: string[]}} + */ +export function scanDomainSource(source, fileName = 'domain.ts') { + const sf = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true); + const found = { handBuilt: 0, sites: [] }; + + const isDispatcherResult = (obj) => + ts.isObjectLiteralExpression(obj) && + obj.properties.some( + (p) => ts.isPropertyAssignment(p) && ts.isIdentifier(p.name) && p.name.text === 'handled', + ); + + const visit = (node) => { + if ( + ts.isPropertyAssignment(node) && + ts.isIdentifier(node.name) && + node.name.text === 'response' && + ts.isObjectLiteralExpression(node.initializer) && + isDispatcherResult(node.parent) + ) { + found.handBuilt += 1; + found.sites.push(`${fileName}:${sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1}`); + } + ts.forEachChild(node, visit); + }; + visit(sf); + return found; +} + +/** Domain handler files, by basename. */ +function discoverDomains() { + return readdirSync(join(ROOT, DISPATCHER_DOMAIN_DIR)) + .filter((n) => n.endsWith('.ts') && !n.endsWith('.d.ts') && !n.includes('.test.') && !n.includes('.conformance.')) + .sort(); +} + /** Recursively collect candidate route-module paths under `packages/`. */ function discover() { const out = []; @@ -275,12 +423,57 @@ function audit() { } } + // ── The dispatcher domains ──────────────────────────────────────────────── + const domains = discoverDomains(); + + for (const name of domains) { + const declared = DISPATCHER_DOMAINS[name]; + if (!declared) { + problems.push( + `${DISPATCHER_DOMAIN_DIR}/${name}\n NOT DECLARED. Add it to DISPATCHER_DOMAINS in\n` + + ` scripts/check-route-envelope.mjs. A domain that answers only through the\n` + + ` \`deps.*\` helpers declares { handBuilt: 0 }; any hand-built response needs a\n` + + ` \`note\` saying which of the four kinds it is.`, + ); + continue; + } + const got = scanDomainSource(readFileSync(join(ROOT, DISPATCHER_DOMAIN_DIR, name), 'utf8'), name); + if (got.handBuilt !== declared.handBuilt) { + problems.push( + `${DISPATCHER_DOMAIN_DIR}/${name}\n handBuilt: found ${got.handBuilt}, declared ${declared.handBuilt}` + + (got.handBuilt > declared.handBuilt + ? `\n A handler is assembling its own response instead of returning a \`deps.*\`\n` + + ` envelope. If that is deliberate (a status or header the helper cannot\n` + + ` express, or a body this dispatcher only relays), raise the count and say\n` + + ` which in \`note\`. Sites: ${got.sites.join(', ')}` + : `\n Fewer than declared — if a hand-built response moved onto \`deps.*\`, lower\n` + + ` the count (and drop the \`ratchet\` if that was the drift it named).`) + + (declared.ratchet ? `\n (ratchet for ${declared.ratchet} — the declared count pins current drift)` : ''), + ); + } + if (declared.handBuilt > 0 && !declared.note) { + problems.push( + `${DISPATCHER_DOMAIN_DIR}/${name}\n declares handBuilt: ${declared.handBuilt} with no \`note\`.\n` + + ` A hand-built response has to say why it is not a \`deps.*\` call.`, + ); + } + } + + for (const name of Object.keys(DISPATCHER_DOMAINS)) { + if (!domains.includes(name)) { + problems.push( + `${DISPATCHER_DOMAIN_DIR}/${name}\n declared in DISPATCHER_DOMAINS but not found — moved or deleted?`, + ); + } + } + if (problems.length) { console.error('✗ Route-envelope conformance (#3843)\n'); for (const p of problems) console.error(' ' + p + '\n'); console.error( - 'Every REST body must be built by the module\'s sendOk / sendError pair, in the\n' + - 'envelope BaseResponseSchema declares. See scripts/check-route-envelope.mjs.', + 'Every REST body must be built in ONE place per surface, in the envelope\n' + + 'BaseResponseSchema declares — a route module\'s sendOk / sendError pair, or a\n' + + 'dispatcher domain\'s deps.* helpers. See scripts/check-route-envelope.mjs.', ); process.exit(1); } @@ -290,7 +483,7 @@ function audit() { const ratcheted = entries.filter(([, m]) => m.ratchet); const conformant = discovered.length - exempt.length - ratcheted.length; console.log( - `✓ Route-envelope conformance — ${discovered.length} module(s) audited: ` + + `✓ Route-envelope conformance — ${discovered.length} route module(s) audited: ` + `${conformant} conformant, ${ratcheted.length} ratcheted, ${exempt.length} exempt`, ); for (const [file, m] of ratcheted) { @@ -299,6 +492,18 @@ function audit() { for (const [file, m] of exempt) { console.log(` – exempt: ${file} — ${m.exempt}`); } + + const dEntries = Object.entries(DISPATCHER_DOMAINS); + const dRatcheted = dEntries.filter(([, m]) => m.ratchet); + const helperOnly = dEntries.filter(([, m]) => m.handBuilt === 0); + console.log( + `✓ Dispatcher domains — ${domains.length} audited: ` + + `${helperOnly.length} helper-only, ${dEntries.length - helperOnly.length} with declared hand-built responses ` + + `(${dRatcheted.length} ratcheted)`, + ); + for (const [name, m] of dRatcheted) { + console.log(` ⚠ ratchet ${m.ratchet}: ${DISPATCHER_DOMAIN_DIR}/${name} — ${m.note}`); + } } // ── Self-test ──────────────────────────────────────────────────────────────── @@ -357,6 +562,41 @@ function selfTest() { `ok passed as a helper's data must not count → ${JSON.stringify(r)}`, ); + // ── Dispatcher domains ──────────────────────────────────────────────────── + // A domain that answers only through the helpers hand-builds nothing. + let d = scanDomainSource(` + if (m === 'GET') return { handled: true, response: deps.success(rows) }; + if (m === 'DELETE') return { handled: true, response: deps.success({ ok: true }) }; + return { handled: true, response: deps.routeNotFound('/x') }; + `); + assert(d.handBuilt === 0, `helper-only domain → ${JSON.stringify(d)}`); + + // An assembled literal is what the count is for — this is how /share-links + // carried a duplicate `link` beside `data` unseen (#4038). + d = scanDomainSource(`return { handled: true, response: { status: 201, body: { success: true, data: link, link } } };`); + assert(d.handBuilt === 1, `hand-built response not caught → ${JSON.stringify(d)}`); + + // Both forms in one file, counted once each. + d = scanDomainSource(` + if (a) return { handled: true, response: deps.success(x) }; + if (b) return { handled: true, response: { status: 405, headers: { Allow: 'GET' }, body } }; + return { handled: true, response: { status: 200, body: { agents: [] } } }; + `); + assert(d.handBuilt === 2, `mixed domain miscounted → ${JSON.stringify(d)}`); + + // Prose quoting either form is not code — the note fields in the table above + // quote both, and must not move anyone's count. + d = scanDomainSource(` + /* was: response: { status: 200, body: { agents: [] } } — see #4053 */ + // return { handled: true, response: { status: 201, body } }; + return { handled: true, response: deps.success(x) }; + `); + assert(d.handBuilt === 0, `commented-out responses counted → ${JSON.stringify(d)}`); + + // A nested `response` key inside a payload is not a response assignment. + d = scanDomainSource(`return { handled: true, response: deps.success({ response: { status: 'queued' } }) };`); + assert(d.handBuilt === 0, `a data field named response must not count → ${JSON.stringify(d)}`); + console.log('✓ check-route-envelope self-test passed'); }