diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 07bfcb531..6f3f74876 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -193,6 +193,7 @@ jobs: - run: bun run typecheck - run: bun run check:deps - run: bun run check:errors + - run: bun run check:patches test-unit: name: Unit Tests diff --git a/.lore.md b/.lore.md index 43c4ba4a7..de0fcbdca 100644 --- a/.lore.md +++ b/.lore.md @@ -93,7 +93,7 @@ * **ubuntu-latest defaults to Node 20 (EOL) — E2E jobs must use actions/setup-node**: Trap: \`ubuntu-latest\` GitHub Actions runner ships Node 20 (EOL). E2E jobs that run the npm bundle via \`node -e "..."\` silently use Node 20 unless \`actions/setup-node\` is explicitly added. This looks fine because the job succeeds, but it violates \`engines.node >=22.12\`. Fix: add \`actions/setup-node\` with \`node-version: 22\` to any job that invokes \`node\` directly. The \`build-npm\` job already does this correctly; E2E jobs need the same treatment. Never add feature-detection workarounds for Node 20 — it's EOL and unsupported. -* **whichSync must use 'command -v' not 'which' for PATH-restricted lookups**: (pattern) Bun→Node.js API replacements: \`Bun.which(cmd, {PATH})\` → \`whichSync()\` from \`src/lib/which.ts\` (uses \`command -v\` shell builtin, NOT \`which\` binary — \`which\` fails when PATH is restricted to a test dir). \`Bun.spawn\` → \`spawn\` from \`node:child\_process\`; wrap \`.exited\` as \`new Promise(r => proc.on('close', code => r(code ?? 1)))\`. \*\*CRITICAL: always attach \`proc.on('error', () => {})\` — Node crashes on unhandled spawn errors; Bun did not.\*\* \`Bun.spawnSync\` → \`spawnSync\`. \`Bun.sleep(ms)\` → \`import { setTimeout as sleepMs } from 'node:timers/promises'\`. \`new Bun.Glob(p).match(i)\` → \`picomatch(p, { dot: true })(i)\` — pre-compile matchers outside hot loops. \`Bun.randomUUIDv7()\` → \`uuidv7()\`. \`Bun.semver.order()\` → \`compare()\` from \`semver\`. Only change actual API call sites — never comments. Update type annotations. +* **Bun.which polyfill uses 'command -v' with 5s timeout for PATH-restricted lookups**: (pattern) Bun→Node.js API replacements live in \`script/node-polyfills.ts\` (injected at bundle time via esbuild). \`Bun.which(cmd, {PATH})\` polyfill uses \`command -v\` shell builtin (NOT \`which\` binary — \`which\` fails when PATH is restricted to a test dir) with \`timeout: 5000\` to prevent indefinite blocking. Windows uses \`where\`. \`Bun.spawn\` → \`spawn\` from \`node:child\_process\`; wrap \`.exited\` as \`new Promise(r => proc.on('close', code => r(code ?? 1)))\`. \*\*CRITICAL: always attach \`proc.on('error', () => {})\` — Node crashes on unhandled spawn errors; Bun did not.\*\* \`Bun.spawnSync\` → \`spawnSync\`. \`Bun.sleep(ms)\` → \`setTimeout\` promise. \`new Bun.Glob(p).match(i)\` → \`picomatch(p, { dot: true })(i)\` — pre-compile matchers outside hot loops. \`Bun.randomUUIDv7()\` → \`uuidv7()\`. \`Bun.semver.order()\` → \`compare()\` from \`semver\`. SQLite handled by \`src/lib/db/sqlite.ts\` (not the polyfill). Only change actual API call sites — never comments. Update type annotations. * **Whole-buffer matchAll slower than split+test when aggregated over many files**: Grep/scan traps in \`src/lib/scan/\`: (1) Whole-buffer \`regex.exec\` 12× faster per-file but ~1.6× SLOWER over 10k files — early-exit at \`maxResults\` via \`mapFilesConcurrent.onResult\` wins. (2) Literal prefilter is FILE-LEVEL gate (\`indexOf\`→skip); per-line verify breaks cross-newline patterns and Unicode length-changing \`toLowerCase\` (Turkish \`İ\`→\`i̇\`). (3) Extractor \`hasTopLevelAlternation\`+\`skipGroup\` must call \`skipCharacterClass\` (PCRE \`\[]abc]\` ≠ JS empty class). (4) Wake-latch race: naive \`let notify=null; await new Promise(r=>notify=r)\` loses signals — use latched \`pendingWake\` flag. (5) \`mapFilesConcurrent\` filters \`null\` but NOT \`\[]\` — return \`null\` for no-op files. (6) \`collectGlob\`/\`collectGrep\` must NOT forward \`maxResults\` to iterator; drain uncapped, set \`truncated=true\`. diff --git a/package.json b/package.json index 16ea6e9ef..2d7f738eb 100644 --- a/package.json +++ b/package.json @@ -112,6 +112,7 @@ "check:fragments": "bun run script/check-fragments.ts", "check:deps": "bun run script/check-no-deps.ts", "check:errors": "bun run script/check-error-patterns.ts", + "check:patches": "bun run script/check-patches.ts", "check:docs-sections": "bun run script/generate-docs-sections.ts --check" }, "type": "module", diff --git a/script/bundle.ts b/script/bundle.ts index 21b9f895e..85bc7e8b9 100644 --- a/script/bundle.ts +++ b/script/bundle.ts @@ -19,34 +19,6 @@ if (!SENTRY_CLIENT_ID) { process.exit(1); } -// Regex patterns for esbuild plugin (must be top-level for performance) -const BUN_SQLITE_FILTER = /^bun:sqlite$/; -const ANY_FILTER = /.*/; - -/** Plugin to replace bun:sqlite with our node:sqlite polyfill. */ -const bunSqlitePlugin: Plugin = { - name: "bun-sqlite-polyfill", - setup(pluginBuild) { - pluginBuild.onResolve({ filter: BUN_SQLITE_FILTER }, () => ({ - path: "bun:sqlite", - namespace: "bun-sqlite-polyfill", - })); - - pluginBuild.onLoad( - { filter: ANY_FILTER, namespace: "bun-sqlite-polyfill" }, - () => ({ - contents: ` - // Use the polyfill injected by node-polyfills.ts - const polyfill = globalThis.__bun_sqlite_polyfill; - export const Database = polyfill.Database; - export default polyfill; - `, - loader: "js", - }) - ); - }, -}; - type InjectedFile = { jsPath: string; mapPath: string; debugId: string }; /** Delete .map files after a successful upload — they shouldn't ship to users. */ @@ -180,11 +152,7 @@ const sentrySourcemapPlugin: Plugin = { }; // Always inject debug IDs (even without auth token); upload is gated inside the plugin -const plugins: Plugin[] = [ - bunSqlitePlugin, - sentrySourcemapPlugin, - textImportPlugin, -]; +const plugins: Plugin[] = [sentrySourcemapPlugin, textImportPlugin]; if (process.env.SENTRY_AUTH_TOKEN) { console.log(" Sentry auth token found, source maps will be uploaded"); @@ -226,6 +194,10 @@ const result = await build({ // from trying to resolve these packages in the main bundle graph. external: [ "node:*", + // bun:sqlite is referenced as a fallback in src/lib/db/sqlite.ts (never + // reached on Node 22+ where node:sqlite is available). Mark external so + // esbuild doesn't fail trying to resolve a Bun-only module. + "bun:sqlite", "ink", "ink-spinner", "react", diff --git a/script/check-patches.ts b/script/check-patches.ts new file mode 100644 index 000000000..51aa84cab --- /dev/null +++ b/script/check-patches.ts @@ -0,0 +1,85 @@ +#!/usr/bin/env bun +/** + * Check Patched Dependency Versions + * + * Verifies that pnpm patchedDependencies target versions match installed versions. + * Name-only keys in patchedDependencies (pnpm 10+ catalog style) apply patches + * to whatever version resolves. If the installed version doesn't match the + * patch file's target version, the patch may silently fail to apply. + * + * Mismatches are surfaced as warnings (not hard failures) because pnpm 10 + * name-only keys intentionally support version-agnostic patching — patches + * often apply cleanly across minor/patch bumps. The warning ensures engineers + * notice and can regenerate the patch if needed. + * + * Usage: + * bun run script/check-patches.ts + * + * Exit codes: + * 0 - All patch versions match, or mismatches are non-critical (warnings) + * 1 - A patched package is missing entirely + */ + +const pkg: { + pnpm?: { patchedDependencies?: Record }; +} = await Bun.file("package.json").json(); + +const patches = pkg.pnpm?.patchedDependencies ?? {}; +const warnings: string[] = []; +const errors: string[] = []; +export {}; + +for (const [name, patchPath] of Object.entries(patches)) { + // Extract version from patch path: "patches/@stricli%2Fcore@1.2.5.patch" → "1.2.5" + // Handles pre-release versions like "1.2.3-beta.1" by matching everything after @M.N.P until .patch + const versionMatch = patchPath.match(/@(\d+\.\d+\.\d+[^@]*)\.patch$/); + if (!versionMatch) { + warnings.push( + ` ? ${name}: could not extract version from patch path "${patchPath}"` + ); + continue; + } + const patchVersion = versionMatch[1]; + + // Resolve installed version + const pkgJsonPath = `node_modules/${name}/package.json`; + try { + const installed: { version: string } = await Bun.file(pkgJsonPath).json(); + if (installed.version !== patchVersion) { + warnings.push( + ` ${name}: patch targets ${patchVersion}, installed ${installed.version} — regenerate with: pnpm patch ${name}` + ); + } + } catch { + errors.push(` ${name}: not installed (expected ${patchVersion})`); + } +} + +// Emit GitHub Actions annotations for CI visibility +const isCI = !!process.env.CI; +for (const w of warnings) { + if (isCI) { + console.log(`::warning::Patch version mismatch:${w.trim()}`); + } else { + console.warn(`⚠ ${w}`); + } +} + +if (errors.length > 0) { + console.error("✗ Missing patched dependencies:"); + console.error(""); + for (const e of errors) { + console.error(e); + } + console.error(""); + console.error("Run pnpm install to install missing dependencies."); + process.exit(1); +} + +if (warnings.length === 0) { + console.log("✓ All patched dependency versions match installed versions"); +} else { + console.log( + `✓ Patches applied (${warnings.length} version mismatch warning(s) — consider regenerating)` + ); +} diff --git a/script/node-polyfills.ts b/script/node-polyfills.ts index 170b829c4..ec21a9a96 100644 --- a/script/node-polyfills.ts +++ b/script/node-polyfills.ts @@ -2,7 +2,7 @@ * Node.js polyfills for Bun APIs. Injected at bundle time via esbuild. */ import { - execSync, + execFileSync, spawn as nodeSpawn, spawnSync as nodeSpawnSync, } from "node:child_process"; @@ -12,9 +12,6 @@ import { promisify } from "node:util"; // biome-ignore lint/performance/noNamespaceImport: runtime access to optional `zstdCompress`/`zstdDecompress` exports import * as zlibModule from "node:zlib"; import { constants as zlibConstants } from "node:zlib"; -// node:sqlite is imported lazily inside NodeDatabasePolyfill to avoid -// crashing on Node.js versions without node:sqlite support when the -// bundle is loaded as a library (the consumer may never use SQLite). import picomatch from "picomatch"; import { compare as semverCompare } from "semver"; @@ -25,85 +22,6 @@ declare global { var Bun: typeof BunPolyfill; } -type SqliteValue = string | number | bigint | null | Uint8Array; - -/** Lazy-loaded node:sqlite DatabaseSync constructor. */ -function getNodeSqlite(): typeof import("node:sqlite").DatabaseSync { - // eslint-disable-next-line @typescript-eslint/no-require-imports - return require("node:sqlite").DatabaseSync; -} - -/** Wraps node:sqlite StatementSync to match bun:sqlite query() API. */ -class NodeStatementPolyfill { - // biome-ignore lint/suspicious/noExplicitAny: node:sqlite types loaded lazily - private readonly stmt: any; - - // biome-ignore lint/suspicious/noExplicitAny: node:sqlite types loaded lazily - constructor(stmt: any) { - this.stmt = stmt; - } - - get(...params: SqliteValue[]): Record | undefined { - return this.stmt.get(...params) as Record | undefined; - } - - all(...params: SqliteValue[]): Record[] { - return this.stmt.all(...params) as Record[]; - } - - run(...params: SqliteValue[]): void { - this.stmt.run(...params); - } -} - -/** Wraps node:sqlite DatabaseSync to match bun:sqlite Database API. */ -class NodeDatabasePolyfill { - // biome-ignore lint/suspicious/noExplicitAny: node:sqlite types loaded lazily - private readonly db: any; - - constructor(path: string) { - // SQLite configuration (busy_timeout, foreign_keys, WAL mode) is applied - // via PRAGMA statements in src/lib/db/index.ts after construction - const DatabaseSync = getNodeSqlite(); - this.db = new DatabaseSync(path); - } - - exec(sql: string): void { - this.db.exec(sql); - } - - query(sql: string): NodeStatementPolyfill { - return new NodeStatementPolyfill(this.db.prepare(sql)); - } - - close(): void { - this.db.close(); - } - - /** - * Wraps a function in a transaction. Returns a callable that executes - * the function within BEGIN/COMMIT, with ROLLBACK on error. - * Matches Bun's db.transaction() API. - */ - transaction(fn: () => T): () => T { - return () => { - this.db.exec("BEGIN"); - try { - const result = fn(); - this.db.exec("COMMIT"); - return result; - } catch (error) { - this.db.exec("ROLLBACK"); - throw error; - } - }; - } -} - -const bunSqlitePolyfill = { Database: NodeDatabasePolyfill }; -(globalThis as Record).__bun_sqlite_polyfill = - bunSqlitePolyfill; - const BunPolyfill = { file(path: string) { return { @@ -162,22 +80,39 @@ const BunPolyfill = { which(command: string, opts?: { PATH?: string }): string | null { try { const isWindows = process.platform === "win32"; - const cmd = isWindows ? `where ${command}` : `which ${command}`; // If a custom PATH is provided, override it in the subprocess env. // Use !== undefined (not truthy) so empty-string PATH is respected. const env = opts?.PATH !== undefined ? { ...process.env, PATH: opts.PATH } : undefined; - return ( - execSync(cmd, { + + let stdout: string; + if (isWindows) { + // execFileSync bypasses the shell entirely — no injection risk + stdout = execFileSync("where.exe", [command], { encoding: "utf-8", stdio: ["pipe", "pipe", "ignore"], env, - }) - .trim() - .split("\n")[0] || null - ); + timeout: 5000, + }); + } else { + // Pass command as a positional arg ($1) so it's never interpolated + // into the shell string. `command -v` is a POSIX builtin — works + // even when PATH is overridden to a restricted set of directories. + stdout = execFileSync( + "/bin/sh", + ["-c", 'command -v "$1"', "--", command], + { + encoding: "utf-8", + stdio: ["pipe", "pipe", "ignore"], + env, + timeout: 5000, + } + ); + } + + return stdout.trim().split("\n")[0] || null; } catch { return null; } diff --git a/src/lib/bspatch.ts b/src/lib/bspatch.ts index 87b70061e..8a8a69882 100644 --- a/src/lib/bspatch.ts +++ b/src/lib/bspatch.ts @@ -183,6 +183,20 @@ class BufferedStreamReader { return output; } + + /** Release the underlying stream reader, cancelling any pending reads. */ + async cancel(): Promise { + try { + await this.reader.cancel(); + } catch { + // Stream may already be closed or errored — safe to ignore + } + try { + this.reader.releaseLock(); + } catch { + // Lock may already be released + } + } } /** @@ -213,6 +227,11 @@ function createZstdStreamReader(compressed: Uint8Array): BufferedStreamReader { controller.error(err); }); }, + cancel() { + // Destroy the underlying Node.js stream so buffered data events + // don't fire controller.enqueue() on the now-closed controller. + nodeStream.destroy(); + }, }); return new BufferedStreamReader( @@ -270,6 +289,38 @@ async function loadOldBinary(oldPath: string): Promise { } } +/** Resources to clean up after patch application. */ +type PatchResources = { + writer: import("node:fs").WriteStream; + /** Returns the latest write error (live reference, not a snapshot). */ + getWriteError: () => Error | undefined; + diffReader: BufferedStreamReader; + extraReader: BufferedStreamReader; + cleanupOldFile: () => void | Promise; +}; + +/** + * Clean up all patch resources: flush the output stream, cancel decompression + * readers, and remove temp files. Each step runs regardless of prior failures. + */ +async function cleanupPatchResources(r: PatchResources): Promise { + try { + await new Promise((resolve, reject) => { + r.writer.end((err?: Error | null) => { + const finalErr = err ?? r.getWriteError(); + if (finalErr) { + reject(finalErr); + } else { + resolve(); + } + }); + }); + } finally { + await Promise.all([r.diffReader.cancel(), r.extraReader.cancel()]); + await r.cleanupOldFile(); + } +} + /** * Apply a TRDIFF10 binary patch with streaming I/O for minimal memory usage. * @@ -367,20 +418,13 @@ export async function applyPatch( oldpos += seekBy; } } finally { - try { - await new Promise((resolve, reject) => { - writer.end((err?: Error | null) => { - const finalErr = err ?? writeError; - if (finalErr) { - reject(finalErr); - } else { - resolve(); - } - }); - }); - } finally { - await cleanupOldFile(); - } + await cleanupPatchResources({ + writer, + getWriteError: () => writeError, + diffReader, + extraReader, + cleanupOldFile, + }); } // Validate output size matches header diff --git a/src/lib/dsn/scanner.ts b/src/lib/dsn/scanner.ts index c792def7e..7459ff503 100644 --- a/src/lib/dsn/scanner.ts +++ b/src/lib/dsn/scanner.ts @@ -5,7 +5,7 @@ * Used by env-file detection for scanning .env file variants. */ -import { readFile, stat } from "node:fs/promises"; +import { open } from "node:fs/promises"; import { join } from "node:path"; import { handleFileError, isRegularFile } from "./fs-utils.js"; import type { DetectedDsn } from "./types.js"; @@ -72,26 +72,38 @@ export async function scanSpecificFiles( try { // Guard: skip non-regular files (FIFOs, sockets, etc.) that would block. - // 1Password streams secrets via symlinked named pipes; Bun.file().text() - // blocks indefinitely on these. + // 1Password streams secrets via symlinked named pipes; open() on a FIFO + // blocks indefinitely waiting for a writer. if (!(await isRegularFile(filepath, "scanSpecificFiles.stat"))) { continue; } - const content = await readFile(filepath, "utf-8"); - const result = processFile(filename, content); - if (result?.dsn) { - const detected = createDsn(result.dsn, filename, result.metadata); - if (detected) { - dsns.push(detected); - // Record mtime for cache invalidation - const stats = await stat(filepath); - sourceMtimes[filename] = Math.floor(stats.mtimeMs); + // Use a single file handle for atomic read + stat to avoid TOCTOU: + // reading content and mtime from the same open handle ensures they + // correspond to the same file version. + const fh = await open(filepath, "r"); + try { + const [content, stats] = await Promise.all([ + fh.readFile("utf-8"), + fh.stat(), + ]); + const result = processFile(filename, content); - if (stopOnFirst) { - return { dsns, sourceMtimes }; + if (result?.dsn) { + const detected = createDsn(result.dsn, filename, result.metadata); + if (detected) { + dsns.push(detected); + // Record mtime for cache invalidation (from same handle as content). + // Floor to integer — all mtime comparisons in dsn-cache.ts use Math.floor. + sourceMtimes[filename] = Math.floor(stats.mtimeMs); + + if (stopOnFirst) { + return { dsns, sourceMtimes }; + } } } + } finally { + await fh.close(); } } catch (error) { handleFileError(error, {