fix: address low-priority review items from Bun→Node migration (#990)

1. Remove dead SQLite polyfill code (NodeDatabasePolyfill + bunSqlitePlugin)
   — replaced by src/lib/db/sqlite.ts

2. Fix which polyfill: use 'command -v' (POSIX builtin) instead of 'which'
   binary, add 5s timeout to prevent indefinite blocking

3. Add stream cleanup to bspatch BufferedStreamReader: cancel() method +
   cleanup in applyPatch finally block

4. Fix scanner TOCTOU: use single fs.promises.open() file handle for
   atomic read + stat (content and mtime from same handle)

5. Add CI check (check:patches) to detect when pnpm patchedDependencies
   target versions diverge from installed versions

Closes #990

Author: BYK

.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

.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.
 
 <!-- lore:019e464f-8fcb-7d16-b7ee-f3b157e1565e -->
-* **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.
 
 <!-- lore:019db0c9-9cc7-7352-b1f2-61b34b87b252 -->
 * **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\`.

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",

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");

script/check-patches.ts

@@ -0,0 +1,84 @@
+#!/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<string, string> };
+} = 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"
+  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)`
+  );
+}

script/node-polyfills.ts

@@ -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<string, SqliteValue> | undefined {
-    return this.stmt.get(...params) as Record<string, SqliteValue> | undefined;
-  }
-
-  all(...params: SqliteValue[]): Record<string, SqliteValue>[] {
-    return this.stmt.all(...params) as Record<string, SqliteValue>[];
-  }
-
-  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<T>(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<string, unknown>).__bun_sqlite_polyfill =
-  bunSqlitePolyfill;
-
 const BunPolyfill = {
   file(path: string) {
     return {
@@ -162,7 +80,9 @@ const BunPolyfill = {
   which(command: string, opts?: { PATH?: string }): string | null {
     try {
       const isWindows = process.platform === "win32";
-      const cmd = isWindows ? `where ${command}` : `which ${command}`;
+      // `command -v` is a POSIX shell builtin that works even with restricted
+      // PATH (unlike the `which` binary). Windows has no equivalent — use `where`.
+      const cmd = isWindows ? `where ${command}` : `command -v ${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 =
@@ -174,6 +94,7 @@ const BunPolyfill = {
           encoding: "utf-8",
           stdio: ["pipe", "pipe", "ignore"],
           env,
+          timeout: 5000,
         })
           .trim()
           .split("\n")[0] || null

src/lib/bspatch.ts

@@ -183,6 +183,20 @@ class BufferedStreamReader {
 
     return output;
   }
+
+  /** Release the underlying stream reader, cancelling any pending reads. */
+  async cancel(): Promise<void> {
+    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
+    }
+  }
 }
 
 /**
@@ -270,6 +284,37 @@ async function loadOldBinary(oldPath: string): Promise<OldFileHandle> {
   }
 }
 
+/** Resources to clean up after patch application. */
+type PatchResources = {
+  writer: import("node:fs").WriteStream;
+  writeError: Error | undefined;
+  diffReader: BufferedStreamReader;
+  extraReader: BufferedStreamReader;
+  cleanupOldFile: () => void | Promise<void>;
+};
+
+/**
+ * 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<void> {
+  try {
+    await new Promise<void>((resolve, reject) => {
+      r.writer.end((err?: Error | null) => {
+        const finalErr = err ?? r.writeError;
+        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 +412,13 @@ export async function applyPatch(
       oldpos += seekBy;
     }
   } finally {
-    try {
-      await new Promise<void>((resolve, reject) => {
-        writer.end((err?: Error | null) => {
-          const finalErr = err ?? writeError;
-          if (finalErr) {
-            reject(finalErr);
-          } else {
-            resolve();
-          }
-        });
-      });
-    } finally {
-      await cleanupOldFile();
-    }
+    await cleanupPatchResources({
+      writer,
+      writeError,
+      diffReader,
+      extraReader,
+      cleanupOldFile,
+    });
   }
 
   // Validate output size matches header