diff --git a/server/package.json b/server/package.json index 646541042a..2219c6c833 100644 --- a/server/package.json +++ b/server/package.json @@ -35,7 +35,7 @@ "dev": "tsx src/index.ts", "dev:watch": "cross-env PAPERCLIP_MIGRATION_PROMPT=never PAPERCLIP_MIGRATION_AUTO_APPLY=true tsx ./scripts/dev-watch.ts", "prepare:ui-dist": "bash ../scripts/prepare-server-ui-dist.sh", - "build": "pnpm run prepare:runner-vendor && tsc && mkdir -p dist/onboarding-assets dist/built-ins dist/services/scripts dist/vendor/paperclip-runner && cp -R src/onboarding-assets/. dist/onboarding-assets/ && cp -R src/built-ins/. dist/built-ins/ && cp -R src/services/scripts/. dist/services/scripts/ && cp -R ../packages/paperclip-runner/dist/. dist/vendor/paperclip-runner/ && node scripts/write-build-stamp.mjs", + "build": "pnpm run prepare:runner-vendor && node scripts/verify-runner-vendor-dependencies.mjs && tsc && mkdir -p dist/onboarding-assets dist/built-ins dist/services/scripts dist/vendor/paperclip-runner && cp -R src/onboarding-assets/. dist/onboarding-assets/ && cp -R src/built-ins/. dist/built-ins/ && cp -R src/services/scripts/. dist/services/scripts/ && cp -R ../packages/paperclip-runner/dist/. dist/vendor/paperclip-runner/ && node scripts/write-build-stamp.mjs", "prepack": "pnpm run prepare:ui-dist && pnpm run build", "postpack": "rm -rf ui-dist", "clean": "rm -rf dist", @@ -88,6 +88,7 @@ "pino-http": "^11.0.0", "pino-pretty": "^13.1.3", "sharp": "^0.35.4", + "smol-toml": "^1.4.2", "ssh2": "^1.17.0", "ws": "^8.21.3", "zod": "^4.4.3" diff --git a/server/scripts/verify-runner-vendor-dependencies.mjs b/server/scripts/verify-runner-vendor-dependencies.mjs new file mode 100644 index 0000000000..16634ae755 --- /dev/null +++ b/server/scripts/verify-runner-vendor-dependencies.mjs @@ -0,0 +1,145 @@ +// Verify every npm package the vendored paperclip-runner actually imports +// at runtime is also declared as a direct dependency of server/package.json. +// +// packages/paperclip-runner is private and never published, so the server +// build vendors its compiled dist/ tree wholesale with `cp -R` (see the +// `build` script's `... dist/vendor/paperclip-runner/` step) -- code only, +// no node_modules alongside it. Every npm package the vendored code +// actually imports is therefore also a runtime dependency of server once +// vendored, and has to be declared there too (see acpx, ajv). That mirror +// step is easy to forget -- it silently missed smol-toml in #13110, and CI +// stayed green while production crash-looped 3 seconds into every start +// (#13116) -- because nothing enforced it. +// +// This derives the required set from esbuild's own module-resolution scan +// of the two entry points server actually imports (index.js, testing.js), +// with `write: false` so nothing is written to disk and `packages: +// "external"` so npm imports are reported, not inlined. That is precise: +// paperclip-runner declares dependencies (react-markdown, the codex/opencode +// CLI packages, ...) that only its unrelated ./react and ./browser export +// subpaths use, which server never imports, so requiring *every* declared +// dependency to be mirrored would be over-broad and demand dependencies +// server does not actually need. +// +// This intentionally only analyzes the module graph; it does not bundle or +// rewrite anything on disk. Several runner modules resolve sibling build +// artifacts -- the native runnerd binary, the ACPX/OpenCode CJS sidecar +// scripts in dist/cli, JSON replay fixtures under a top-level protocol/ +// directory -- via `import.meta.url`-relative filesystem paths rather than +// JS imports, each at whatever nesting depth its source file happens to +// sit at. Actually bundling those entry points (an earlier version of this +// fix did) collapses and rearranges that layout, silently breaking those +// lookups. Preserving the original `cp -R` tree 1:1 is what keeps all of +// them working, so this script only verifies; it never restructures. + +import { build } from "esbuild"; +import { existsSync, readFileSync } from "node:fs"; +import { builtinModules } from "node:module"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +const serverRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); +const runnerRoot = resolve(serverRoot, "../packages/paperclip-runner"); +const runnerDist = resolve(runnerRoot, "dist"); + +// The only entry points server/src actually imports from the vendored +// runner (server/src/**/*.ts import "../vendor/paperclip-runner/index.js" +// or ".../testing.js"). +const ENTRY_POINT_NAMES = ["index.js", "testing.js"]; + +const NODE_BUILTINS = new Set([ + ...builtinModules, + ...builtinModules.map((name) => `node:${name}`), +]); + +/** The npm package name a bare import specifier resolves to, honoring scoped packages and subpaths. */ +function packageNameFromSpecifier(specifier) { + const segments = specifier.split("/"); + return specifier.startsWith("@") ? segments.slice(0, 2).join("/") : segments[0]; +} + +export function findMissingVendorDependencies(importedPackageNames, declaredDependencyNames) { + return [...importedPackageNames] + .filter((name) => !declaredDependencyNames.has(name)) + .sort(); +} + +export async function findRunnerExternalPackages(entryPoints) { + for (const entryPoint of entryPoints) { + if (!existsSync(entryPoint)) { + throw new Error( + `paperclip-runner vendor check: expected build output at ${entryPoint}. ` + + `Run "pnpm --filter @paperclipai/paperclip-runner build" first.`, + ); + } + } + + // write: false means this never touches disk -- it's a module-graph scan, + // not a real bundle. Vendoring itself still happens via `cp -R` elsewhere + // in the build script. `outdir` is required by esbuild for multiple entry + // points but nothing is ever written there, so any sibling path will do. + const result = await build({ + entryPoints, + outdir: resolve(dirname(entryPoints[0]), ".vendor-dependency-scan"), + bundle: true, + write: false, + platform: "node", + format: "esm", + packages: "external", + metafile: true, + logLevel: "silent", + }); + + const externalPackageNames = new Set(); + for (const output of Object.values(result.metafile.outputs)) { + for (const imported of output.imports) { + if (!imported.external) continue; + if (imported.path.startsWith(".") || NODE_BUILTINS.has(imported.path)) continue; + externalPackageNames.add(packageNameFromSpecifier(imported.path)); + } + } + return externalPackageNames; +} + +function readDependencyNames(packageJsonPath) { + const packageJson = JSON.parse(readFileSync(packageJsonPath, "utf8")); + return new Map(Object.entries(packageJson.dependencies ?? {})); +} + +function explainMissingDependencies(missing, runnerDependencyNames) { + const lines = missing.map((name) => { + const range = runnerDependencyNames.get(name); + return range + ? ` "${name}": "${range}" (matches packages/paperclip-runner/package.json)` + : ` "${name}" (not declared as a paperclip-runner dependency either -- check for a missing or mistyped dependency there first)`; + }); + return ( + `paperclip-runner vendor check: server/package.json is missing the runtime ` + + `${missing.length === 1 ? "dependency" : "dependencies"} the vendored runner ` + + `imports at runtime:\n${lines.join("\n")}\n\n` + + `packages/paperclip-runner/dist is copied into server's own published package ` + + `without its node_modules, so every npm package the runner imports must also be ` + + `a direct dependency of server so it resolves once vendored. Add ` + + `${missing.length === 1 ? "it" : "them"} to server/package.json's "dependencies".` + ); +} + +async function main() { + const entryPoints = ENTRY_POINT_NAMES.map((name) => resolve(runnerDist, name)); + const externalPackageNames = await findRunnerExternalPackages(entryPoints); + const serverDependencyNames = readDependencyNames(resolve(serverRoot, "package.json")); + + const missing = findMissingVendorDependencies( + externalPackageNames, + new Set(serverDependencyNames.keys()), + ); + if (missing.length > 0) { + const runnerDependencyNames = readDependencyNames(resolve(runnerRoot, "package.json")); + throw new Error(explainMissingDependencies(missing, runnerDependencyNames)); + } +} + +// Only run when invoked directly (`node scripts/verify-runner-vendor-dependencies.mjs`), +// not when the vitest suite imports findMissingVendorDependencies for a unit test. +const isMain = process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href; +if (isMain) await main(); diff --git a/server/scripts/verify-runner-vendor-dependencies.test.mjs b/server/scripts/verify-runner-vendor-dependencies.test.mjs new file mode 100644 index 0000000000..07bbbcef3b --- /dev/null +++ b/server/scripts/verify-runner-vendor-dependencies.test.mjs @@ -0,0 +1,98 @@ +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +import { + findMissingVendorDependencies, + findRunnerExternalPackages, +} from "./verify-runner-vendor-dependencies.mjs"; + +describe("findMissingVendorDependencies", () => { + it("returns nothing when every runner dependency is already declared on server", () => { + const missing = findMissingVendorDependencies( + new Set(["acpx", "ajv", "smol-toml"]), + new Set(["acpx", "ajv", "smol-toml", "express"]), + ); + + expect(missing).toEqual([]); + }); + + it("flags a runner dependency that isn't mirrored into server/package.json", () => { + // This is the exact shape of the incident this check exists to catch: + // packages/paperclip-runner/package.json grew a new runtime dependency + // (smol-toml) that never got mirrored into server/package.json, so the + // vendored `cp -R` copy failed to resolve it at runtime (#13110, #13116). + const missing = findMissingVendorDependencies( + new Set(["acpx", "ajv", "smol-toml"]), + new Set(["acpx", "ajv"]), + ); + + expect(missing).toEqual(["smol-toml"]); + }); + + it("sorts multiple missing dependencies for a stable error message", () => { + const missing = findMissingVendorDependencies( + new Set(["smol-toml", "ajv-formats", "acpx"]), + new Set(), + ); + + expect(missing).toEqual(["acpx", "ajv-formats", "smol-toml"]); + }); +}); + +describe("findRunnerExternalPackages", () => { + // Fixture-level coverage for the actual esbuild scan, not just the diff + // function: a real dist/index.js + dist/testing.js on disk, structurally + // matching packages/paperclip-runner's shape (testing.js re-exports + // index.js, which imports another local module that imports a bare npm + // specifier), plus package.json dependency noise that should be ignored + // because nothing reachable from these entry points imports it. + let fixtureDir; + + afterEach(() => { + if (fixtureDir) rmSync(fixtureDir, { recursive: true, force: true }); + fixtureDir = undefined; + }); + + function writeFixture() { + fixtureDir = mkdtempSync(join(tmpdir(), "paperclip-runner-vendor-fixture-")); + writeFileSync( + join(fixtureDir, "internal.js"), + 'import { parse } from "smol-toml";\n' + + "export function parseSomething(text) { return parse(text); }\n", + ); + writeFileSync( + join(fixtureDir, "index.js"), + 'export * from "./internal.js";\nexport const marker = "index";\n', + ); + writeFileSync( + join(fixtureDir, "testing.js"), + 'export * from "./index.js";\nexport const testingMarker = "testing";\n', + ); + return [join(fixtureDir, "index.js"), join(fixtureDir, "testing.js")]; + } + + it("reports only the npm packages actually reachable from the entry points", async () => { + const entryPoints = writeFixture(); + + const externalPackageNames = await findRunnerExternalPackages(entryPoints); + + // smol-toml is reachable through internal.js -> index.js -> testing.js + // and must be reported. Nothing else was imported anywhere in the + // fixture, so this also proves the scan doesn't fall back to "every + // dependency the package declares" (which is what made the check + // over-broad before -- see the module header). + expect(externalPackageNames).toEqual(new Set(["smol-toml"])); + }); + + it("throws a clear, actionable error when an entry point is missing", async () => { + fixtureDir = mkdtempSync(join(tmpdir(), "paperclip-runner-vendor-fixture-")); + const missingEntryPoint = join(fixtureDir, "index.js"); + + await expect(findRunnerExternalPackages([missingEntryPoint])).rejects.toThrow( + /expected build output at .*index\.js.*Run "pnpm --filter @paperclipai\/paperclip-runner build" first/s, + ); + }); +}); diff --git a/server/src/__tests__/server-package-build-script.test.ts b/server/src/__tests__/server-package-build-script.test.ts index 9e142cdcb0..b4cfe4ff38 100644 --- a/server/src/__tests__/server-package-build-script.test.ts +++ b/server/src/__tests__/server-package-build-script.test.ts @@ -62,6 +62,22 @@ describe("server package build script", () => { ); }); + it("verifies vendored runner dependencies are mirrored before building", () => { + const packageJson = JSON.parse(readFileSync(packageJsonPath, "utf8")) as { + scripts?: Record; + }; + + // See scripts/verify-runner-vendor-dependencies.mjs: packages/paperclip-runner + // is vendored with a raw `cp -R` of its compiled dist/, so every runtime + // dependency it imports must also be a direct dependency of server. This + // check derives that requirement from an esbuild scan of the vendored + // entry points instead of relying on a human to have kept a hand-copied + // list in sync (the smol-toml incident in #13110/#13116). + expect(packageJson.scripts?.build).toContain( + "node scripts/verify-runner-vendor-dependencies.mjs", + ); + }); + it("loads runner source when the source server starts before workspace builds", () => { const shim = readFileSync(runnerShimPath, "utf8"); diff --git a/server/vitest.config.ts b/server/vitest.config.ts index f87ac41efe..239bce1bc3 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -15,7 +15,7 @@ export default defineConfig({ }, test: { environment: "node", - include: ["src/**/*.test.ts"], + include: ["src/**/*.test.ts", "scripts/**/*.test.mjs"], // Each server suite boots + tears down its own embedded Postgres in // beforeAll/afterAll. Under the loaded serial shard (maxWorkers=1) the // graceful shutdown can occasionally cross vitest's default 10s hookTimeout,