From e31cdcc581a7320fc9d74529c87b49d0dd2f7fd2 Mon Sep 17 00:00:00 2001 From: israel Date: Sat, 29 Aug 2026 14:33:32 +0100 Subject: [PATCH] test(platform): guard the convex runtime boundary --- .agents/repo.md | 6 + services/platform/package.json | 2 +- .../scripts/check-convex-runtime.test.ts | 160 ++++++++++++++ .../platform/scripts/check-convex-runtime.ts | 207 ++++++++++++++++++ 4 files changed, 374 insertions(+), 1 deletion(-) create mode 100644 services/platform/scripts/check-convex-runtime.test.ts create mode 100644 services/platform/scripts/check-convex-runtime.ts diff --git a/.agents/repo.md b/.agents/repo.md index b0526e03a1..ff7d729143 100644 --- a/.agents/repo.md +++ b/.agents/repo.md @@ -34,6 +34,12 @@ Tale is a monorepo on Bun workspaces; every workspace script runs through scoped and queried per organization. Per-org knowledge routing is `getKnowledgePoolForOrg(orgSlug)`, never the deployment-default `getKnowledgePool()`; introducing a new cross-org shared surface is a defect. +- **A file under `convex/` that reaches a Node built-in declares `'use node'`** — Convex bundles + every file in that tree for its V8 runtime unless the file itself says otherwise, regardless of + who imports it, so an undeclared one fails the deploy with `Could not resolve "node:path"` and + the app stops starting. The reach is usually indirect, through a `lib/` barrel. `typecheck` runs + `check-convex-runtime` and prints the import chain; nothing else catches it, and in CI it + surfaces as every browser-test shard failing at once. - **A data-model or org-config schema change ships a migration** — versioned, reversible, idempotent; `migrations:check` fails without one. Scaffold with `bun run gen:migration` (the registries are generated — `migrations:sync`, never hand-edited) and follow the diff --git a/services/platform/package.json b/services/platform/package.json index 8f5526c787..eb60adda6d 100644 --- a/services/platform/package.json +++ b/services/platform/package.json @@ -11,7 +11,7 @@ "start": "bun --bun vite preview", "lint": "bunx oxlint --type-aware", "lint:fix": "bunx oxlint --type-aware --fix", - "typecheck": "bunx tsc --noEmit", + "typecheck": "bunx tsc --noEmit && bun scripts/check-convex-runtime.ts", "convex:dev": "CONVEX_AGENT_MODE=anonymous bunx convex dev", "convex:deploy": "CONVEX_AGENT_MODE=anonymous bunx convex deploy", "convex:codegen": "[ -z \"$CONVEX_DEPLOYMENT\" ] && echo 'Skipped convex codegen (no CONVEX_DEPLOYMENT)' || bunx convex codegen", diff --git a/services/platform/scripts/check-convex-runtime.test.ts b/services/platform/scripts/check-convex-runtime.test.ts new file mode 100644 index 0000000000..942ff4a6c4 --- /dev/null +++ b/services/platform/scripts/check-convex-runtime.test.ts @@ -0,0 +1,160 @@ +// The Convex runtime guard. +// +// Every case builds a small tree on disk rather than mocking the filesystem, +// because what the guard has to get right IS the filesystem walk: which files +// count, which imports count, and how far it follows them. + +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { afterEach, describe, expect, it } from 'vitest'; + +import { + declaresUseNode, + findOffenders, + importsOf, +} from './check-convex-runtime'; + +const roots: string[] = []; +afterEach(() => { + for (const dir of roots.splice(0)) + rmSync(dir, { recursive: true, force: true }); +}); + +/** A throwaway tree; `files` maps a relative path to its contents. */ +function tree(files: Record): string { + const root = mkdtempSync(path.join(tmpdir(), 'convex-runtime-')); + roots.push(root); + for (const [rel, body] of Object.entries(files)) { + const full = path.join(root, rel); + mkdirSync(path.dirname(full), { recursive: true }); + writeFileSync(full, body, 'utf8'); + } + return root; +} + +describe('declaresUseNode', () => { + it('accepts the directive on the first line', () => { + expect(declaresUseNode("'use node';\n\nexport const x = 1;")).toBe(true); + }); + + it('accepts it after a leading block comment', () => { + // A file whose licence or doc block comes first is still node-side. + expect(declaresUseNode("/* header */\n'use node';\n")).toBe(true); + }); + + it('rejects a file with no directive', () => { + expect(declaresUseNode('export const x = 1;')).toBe(false); + }); +}); + +describe('importsOf', () => { + it('finds a value import, a re-export and a dynamic import', () => { + const found = importsOf( + "import a from './a';\nexport { b } from './b';\nconst c = await import('./c');", + ); + expect(found).toEqual(['./a', './b', './c']); + }); + + it('ignores type-only imports and re-exports', () => { + // Erased before bundling, so they cannot break a deploy. Files on main do + // exactly this with `node:http` and deploy fine. + const found = importsOf( + "import type { X } from 'node:http';\nexport type { Y } from 'node:fs';", + ); + expect(found).toEqual([]); + }); +}); + +describe('findOffenders', () => { + it('flags a file that reaches a Node built-in with no directive', () => { + const root = tree({ + 'helper.ts': + "import { readFileSync } from 'node:fs';\nexport const x = readFileSync;", + }); + const { offenders } = findOffenders(root); + expect(offenders).toHaveLength(1); + expect(offenders[0]).toContain('node:fs'); + }); + + it('follows the chain out of the scanned tree, and reports it', () => { + // The real break was indirect and left the scanned tree: a file under + // convex/ imported a barrel in lib/, which re-exported a loader, which + // imported node:fs. Only the convex file can carry the directive, so the + // chain is what makes the report actionable — and the walk has to follow + // imports beyond the directory it is scanning. + const base = tree({ + 'convex/entry.ts': + "import { thing } from '../lib/index';\nexport const x = thing;", + 'lib/index.ts': "export { thing } from './loader';", + 'lib/loader.ts': "import fs from 'node:fs';\nexport const thing = fs;", + }); + const { offenders } = findOffenders(path.join(base, 'convex')); + expect(offenders).toHaveLength(1); + expect(offenders[0]).toContain('entry.ts'); + expect(offenders[0]).toContain('loader.ts'); + expect(offenders[0]).toContain('node:fs'); + }); + + it('accepts the same file once it declares itself node-side', () => { + const root = tree({ + 'helper.ts': + "'use node';\nimport fs from 'node:fs';\nexport const x = fs;", + }); + const { offenders, nodeSide } = findOffenders(root); + expect(offenders).toEqual([]); + expect(nodeSide).toBe(1); + }); + + it('ignores a test file, which is never deployed', () => { + const root = tree({ + 'thing.test.ts': "import fs from 'node:fs';\nexport const x = fs;", + }); + expect(findOffenders(root).offenders).toEqual([]); + }); + + it('ignores a type-only path to a built-in', () => { + const root = tree({ + 'entry.ts': + "import type { IncomingMessage } from 'node:http';\nexport type X = IncomingMessage;", + }); + expect(findOffenders(root).offenders).toEqual([]); + }); + + it('does not follow bare package specifiers', () => { + // A package that pulls Node in through its own dependencies is a hazard + // this cannot see. Guessing at packages would produce false alarms, and a + // guard that cries wolf gets switched off. + // + // The sibling file is named to collide with the package deliberately: if + // the walk ever treated a bare specifier as a relative path, it would + // resolve to `./zod.ts` and report a break that does not exist. + const root = tree({ + 'entry.ts': "import { z } from 'zod';\nexport const x = z;", + 'zod.ts': "import fs from 'node:fs';\nexport const z = fs;", + }); + const { offenders } = findOffenders(root); + // `zod.ts` is itself an offender — it is a real file reaching node:fs. What + // must NOT happen is `entry.ts` being reported through it. + expect(offenders).toHaveLength(1); + expect(offenders[0]).toContain('zod.ts'); + expect(offenders[0]).not.toContain('entry.ts'); + }); + + it('terminates on a circular import', () => { + const root = tree({ + 'a.ts': "import { b } from './b';\nexport const a = b;", + 'b.ts': "import { a } from './a';\nexport const b = a;", + }); + expect(findOffenders(root).offenders).toEqual([]); + }); + + it('finds every offender, not just the first', () => { + const root = tree({ + 'one.ts': "import fs from 'node:fs';\nexport const a = fs;", + 'two.ts': "import p from 'node:path';\nexport const b = p;", + }); + expect(findOffenders(root).offenders).toHaveLength(2); + }); +}); diff --git a/services/platform/scripts/check-convex-runtime.ts b/services/platform/scripts/check-convex-runtime.ts new file mode 100644 index 0000000000..6fc79fdc52 --- /dev/null +++ b/services/platform/scripts/check-convex-runtime.ts @@ -0,0 +1,207 @@ +/** + * Convex runtime guard: a file under `convex/` that reaches a Node built-in + * must declare `'use node'`, or the deploy fails and the app stops starting. + * + * Convex bundles EVERY file under `convex/` for its V8 runtime unless the file + * itself says otherwise — regardless of who imports it. So a helper that only + * node-side actions use is still bundled for V8, and if it reaches `node:fs` + * or `node:path` the push fails with: + * + * ✘ [ERROR] Could not resolve "node:path" + * ERROR Convex deploy failed (exit code: 1) + * + * Nothing else catches this. Format, lint, typecheck, knip and the whole test + * suite pass, because every one of them runs under Node where the import + * resolves. The failure surfaces only after a container build, as every + * browser-test shard failing at once — which reads like a flaky suite rather + * than a broken build. + * + * The walk follows LOCAL imports transitively, because the reach is usually + * indirect: `convex/knowledge/pii_gate.ts` imports `lib/pii`, whose barrel + * re-exports `data/loader.ts`, which imports `node:fs`. Only the first file in + * that chain is under `convex/` and only it can carry the directive. + * + * Bare npm packages are NOT followed. A package that pulls Node in through its + * own dependencies is a real hazard this cannot see; the `node:` prefix is the + * unambiguous signal, and a guard that guesses at packages would cry wolf. + */ + +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// `fileURLToPath`, not `import.meta.dir`: the latter is a Bun-ism and is +// undefined under vitest, which would break this module at import time in its +// own test. +const PLATFORM_ROOT = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '..', +); +const CONVEX_ROOT = path.join(PLATFORM_ROOT, 'convex'); + +/** Directories whose contents are generated or vendored, never authored. */ +const SKIP_DIRS = new Set(['_generated', 'node_modules']); + +/** + * Suffixes Convex does not deploy. A test may import `node:fs` freely — it + * runs under Node and never reaches the bundle. + */ +const SKIP_SUFFIXES = ['.test.ts', '.test.tsx', '.testkit.ts', '.bench.ts']; + +export function isDeployed(file: string): boolean { + return !SKIP_SUFFIXES.some((suffix) => file.endsWith(suffix)); +} + +/** + * `import x from 'y'`, `export … from 'y'`, and `await import('y')`. + * + * `import type` / `export type` are deliberately NOT matched: TypeScript + * erases them before anything is bundled, so `import type { IncomingMessage } + * from 'node:http'` is not a hazard. Several files on main do exactly that and + * deploy fine — a guard that flagged them would be wrong on day one and get + * switched off. + */ +const IMPORT_RE = + /(?:^|\n)\s*(?:import|export)\s+(?!type\s)[\s\S]*?from\s*['"]([^'"]+)['"]|\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)/g; + +export function listTsFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir)) { + if (SKIP_DIRS.has(entry)) continue; + const full = path.join(dir, entry); + if (statSync(full).isDirectory()) { + out.push(...listTsFiles(full)); + } else if (entry.endsWith('.ts') || entry.endsWith('.tsx')) { + out.push(full); + } + } + return out; +} + +/** Declared node-side. Matches the directive on the first non-empty line. */ +export function declaresUseNode(source: string): boolean { + return /^\s*(?:\/\*[\s\S]*?\*\/\s*)?['"]use node['"]/.test(source); +} + +export function importsOf(source: string): string[] { + const specifiers: string[] = []; + for (const match of source.matchAll(IMPORT_RE)) { + const specifier = match[1] ?? match[2]; + if (specifier !== undefined) specifiers.push(specifier); + } + return specifiers; +} + +/** Resolve a relative specifier to a file on disk, trying the usual endings. */ +export function resolveLocal( + fromFile: string, + specifier: string, +): string | null { + if (!specifier.startsWith('.')) return null; + const base = path.resolve(path.dirname(fromFile), specifier); + for (const candidate of [ + base, + `${base}.ts`, + `${base}.tsx`, + path.join(base, 'index.ts'), + path.join(base, 'index.tsx'), + ]) { + try { + if (statSync(candidate).isFile()) return candidate; + } catch { + // Not this ending; try the next. + } + } + return null; +} + +/** + * The chain from `file` to a `node:` import, or null when it reaches none. + * Depth-first so the reported path is one a reader can follow; `seen` makes a + * cycle terminate rather than recurse forever. + */ +export function nodeReach( + file: string, + seen: Set, + cache: Map, +): string[] | null { + const cached = cache.get(file); + if (cached !== undefined) return cached; + if (seen.has(file)) return null; + seen.add(file); + + let source: string; + try { + source = readFileSync(file, 'utf8'); + } catch { + return null; + } + + for (const specifier of importsOf(source)) { + if (specifier.startsWith('node:')) { + const chain = [path.relative(PLATFORM_ROOT, file), specifier]; + cache.set(file, chain); + return chain; + } + const next = resolveLocal(file, specifier); + if (next === null) continue; + const deeper = nodeReach(next, seen, cache); + if (deeper !== null) { + const chain = [path.relative(PLATFORM_ROOT, file), ...deeper]; + cache.set(file, chain); + return chain; + } + } + cache.set(file, null); + return null; +} + +/** + * Every file under `root` that reaches a Node built-in without declaring + * itself node-side, each with the import chain that gets there. + */ +export function findOffenders(root: string): { + offenders: string[]; + checked: number; + nodeSide: number; +} { + const files = listTsFiles(root); + const cache = new Map(); + const offenders: string[] = []; + let nodeSide = 0; + + for (const file of files) { + if (!isDeployed(file)) continue; + const source = readFileSync(file, 'utf8'); + if (declaresUseNode(source)) { + nodeSide += 1; + continue; + } + const chain = nodeReach(file, new Set(), cache); + if (chain !== null) { + offenders.push( + `${chain[0]} reaches ${chain[chain.length - 1]}\n via ${chain.join('\n → ')}`, + ); + } + } + return { offenders, checked: files.length, nodeSide }; +} + +function main(): void { + const { offenders, checked, nodeSide } = findOffenders(CONVEX_ROOT); + + if (offenders.length > 0) { + console.error( + `[check-convex-runtime] FAILED — ${offenders.length} file(s) under convex/ reach a Node built-in without "use node".\n` + + `Convex bundles them for its V8 runtime and the deploy will fail.\n - ` + + offenders.join('\n - '), + ); + process.exit(1); + } + console.log( + `[check-convex-runtime] OK — ${checked} file(s) under convex/ checked, ${nodeSide} declared node-side, none reach Node built-ins undeclared.`, + ); +} + +// Only when run as a script, so a test can import the pieces without the walk. +if (import.meta.main) main();