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.
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.
patchedDependenciesname-only keys may silently skip on version bumpFile:
package.jsonPR: #967
pnpm's
patchedDependenciesuses 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.
whichSynchas no timeout — can block event loop indefinitelyFile:
src/lib/which.tsPR: #984
execFileSynchas no timeout. If/bin/shorwhere.exehangs (unlikely but possible with broken PATH or network mounts), it blocks indefinitely. Consider adding atimeout: 5000option.3.
scanner.tsstat-after-readFile TOCTOUFile:
src/lib/dsn/scanner.tsPR: #984
readFile()thenstat()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 usefs.promises.open()for a single handle. Low impact since this only affects.envfile DSN scanning.4.
bspatch.tsstream bridge has no cancel/destroy handlerFile:
src/lib/bspatch.tsPR: #986
The Node-to-Web stream bridge in
createZstdStreamReaderdoesn't implementcancel()orpull()on theReadableStream, 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.tsandbunSqlitePlugindead codeFiles:
script/node-polyfills.ts,script/bundle.tsThe SQLite polyfill in
node-polyfills.tsand thebunSqlitePlugininbundle.tsare now dead code — source imports fromsrc/lib/db/sqlite.tsinstead ofbun:sqlite. The rest ofnode-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.