Skip to content

Cleanup: address low-priority review items from Bun→Node migration #990

@BYK

Description

@BYK

Context

During the Bun → Node.js migration (PRs #967, #970, #984, #986), several low-priority review comments were intentionally deferred. These are not regressions — they're pre-existing patterns or accepted trade-offs — but worth tracking for future cleanup.

Items

1. patchedDependencies name-only keys may silently skip on version bump

File: package.json
PR: #967

pnpm's patchedDependencies uses name-only keys (e.g., "@stricli/core" instead of "@stricli/core@1.2.5"). If the patched package is bumped to a version where the patch doesn't apply cleanly, pnpm may silently skip it. Consider pinning exact versions or adding a CI check.

2. whichSync has no timeout — can block event loop indefinitely

File: src/lib/which.ts
PR: #984

execFileSync has no timeout. If /bin/sh or where.exe hangs (unlikely but possible with broken PATH or network mounts), it blocks indefinitely. Consider adding a timeout: 5000 option.

3. scanner.ts stat-after-readFile TOCTOU

File: src/lib/dsn/scanner.ts
PR: #984

readFile() then stat() on the same file creates a race window — if the file is modified between the two calls, the cached mtime won't match the content. Could use fs.promises.open() for a single handle. Low impact since this only affects .env file DSN scanning.

4. bspatch.ts stream bridge has no cancel/destroy handler

File: src/lib/bspatch.ts
PR: #986

The Node-to-Web stream bridge in createZstdStreamReader doesn't implement cancel() or pull() on the ReadableStream, so the underlying Node stream isn't destroyed on error paths. Not a regression (old code had same characteristic). Acceptable for bspatch use case (~100 MB binary).

5. Remove node-polyfills.ts and bunSqlitePlugin dead code

Files: script/node-polyfills.ts, script/bundle.ts

The SQLite polyfill in node-polyfills.ts and the bunSqlitePlugin in bundle.ts are now dead code — source imports from src/lib/db/sqlite.ts instead of bun:sqlite. The rest of node-polyfills.ts (Bun.file, Bun.write, etc.) is also dead since all Bun APIs were replaced. These should be removed as part of Phase 5 cleanup.

Metadata

Metadata

Assignees

Labels

No labels
No labels
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions