fix(npm): harden postinstall binary downloader

[jeremymcs]

What Changed

• Hardened the npm postinstall downloader (npm/scripts/postinstall.js) so an interrupted or failed download no longer leaves a truncated binary on disk: write/response error handlers now defer cleanup until the write stream closes, then rmSync the partial file before rejecting — preventing the next npm install from treating a corrupt partial as "binary already present".
• Added tests/postinstall.test.cjs covering ensureDir recursion, downloading into a missing bin/, partial-file cleanup on an interrupted download, and graceful rejection when the destination directory can't be created.
• Documented the partial-binary cleanup behavior in CHANGELOG.md.

Risk Assessment

✅ Low: The change is well-bounded hardening with thorough test coverage, and the previously-flagged cleanup race has been correctly resolved via the deferred close-event pattern; only a narrow, low-likelihood error-path gap remains.

Testing

Ran the existing node:test suite for postinstall.js (4/4 pass) and, since unit pass alone isn't sufficient evidence, wrote and ran an end-to-end demo driving the actual exported downloader against a real local HTTP server. The transcript shows each intended hardening behavior as an end user/installer would experience it: a fresh install auto-creates the missing bin/ and writes the binary after following a 302 redirect; an interrupted download is rejected and leaves no partial binary behind (the core fix); a blocked destination path is rejected gracefully with a clear prepare … failed message; and importing the module produces no download side effects (require.main guard). No UI surface is involved — this is a CLI/npm-lifecycle change, so the reviewer-visible evidence is the CLI transcript rather than a screenshot. Worktree left clean; temp download dirs were removed and only evidence files remain.

Evidence: End-to-end postinstall downloader demo transcript

--- Scenario 1: download into a MISSING bin/ directory --- bin/ exists before download? false bin/ created automatically? true binary written? true content matches server? true (followed 302 redirect -> 200, then wrote 34 bytes) --- Scenario 2: connection drops mid-download (interrupted) --- download rejected? true rejection message: socket hang up partial binary left behind? false <-- must be false (cleaned up) --- Scenario 3: destination dir cannot be created (path blocked by a file) --- download rejected gracefully? true rejection message: prepare .../s3/not-a-dir/gsd-browser-bin failed: EEXIST: file already exists, mkdir '.../s3/not-a-dir' --- Scenario 4: importing postinstall.js as a module is side-effect free --- exports available: downloadFile=function, ensureDir=function (no "postinstall failed" output above => main() did not run on require)

=== Hardened npm postinstall downloader — end-to-end demo ===
workdir: /tmp/claude-501/gsd-postinstall-demo-32QVCG

--- Scenario 1: download into a MISSING bin/ directory ---
(simulates a fresh `npm install` where bin/ does not exist yet)
bin/ exists before download? false
bin/ created automatically?  true
binary written?             true
content matches server?     true
(followed 302 redirect -> 200, then wrote 34 bytes)

--- Scenario 2: connection drops mid-download (interrupted) ---
(server promises 1024 bytes then kills the socket after 7 bytes)
download rejected?           true
rejection message:           socket hang up
partial binary left behind?  false  <-- must be false (cleaned up)

--- Scenario 3: destination dir cannot be created (path blocked by a file) ---
download rejected gracefully? true
rejection message:            prepare /tmp/claude-501/gsd-postinstall-demo-32QVCG/s3/not-a-dir/gsd-browser-bin failed: EEXIST: file already exists, mkdir '/tmp/claude-501/gsd-postinstall-demo-32QVCG/s3/not-a-dir'

--- Scenario 4: importing postinstall.js as a module is side-effect free ---
(require.main guard — tests can import without firing a real download)
exports available:           downloadFile=function, ensureDir=function
(no "postinstall failed" output above => main() did not run on require)

=== All scenarios completed ===

Evidence: Demo harness script

'use strict';
// End-to-end demonstration of the hardened npm postinstall downloader.
// Exercises the REAL exported functions against a real local HTTP server,
// mimicking how `npm install` downloads the gsd-browser binary from a GitHub
// release redirect — and shows the new safety behaviors via observable
// filesystem state, not assertions.

const fs = require('node:fs');
const http = require('node:http');
const os = require('node:os');
const path = require('node:path');

const REPO_ROOT = path.resolve(__dirname, '..', '..', '..');
// Resolve the worktree path passed via argv so this runs from the evidence dir.
const postinstallPath = process.argv[2];
const { downloadFile, ensureDir } = require(postinstallPath);

const line = (s = '') => process.stdout.write(s + '\n');

function startServer(handler) {
  return new Promise((resolve) => {
    const server = http.createServer(handler);
    server.listen(0, '127.0.0.1', () => resolve(server));
  });
}

async function main() {
  const work = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-postinstall-demo-'));
  line('=== Hardened npm postinstall downloader — end-to-end demo ===');
  line(`workdir: ${work}`);
  line('');

  // ---- Scenario 1: install into a brand-new, missing bin/ directory ----
  line('--- Scenario 1: download into a MISSING bin/ directory ---');
  line('(simulates a fresh `npm install` where bin/ does not exist yet)');
  const payload = Buffer.from('#!/fake/gsd-browser binary v0.2.1\n');
  let okServer = await startServer((req, res) => {
    // first hit a 302 redirect, like GitHub release asset URLs do
    if (req.url === '/release') {
      res.writeHead(302, { Location: `http://127.0.0.1:${okServer.address().port}/asset` });
      return res.end();
    }
    res.writeHead(200, { 'Content-Type': 'application/octet-stream' });
    res.end(payload);
  });
  const binDir1 = path.join(work, 's1', 'bin');
  const dest1 = path.join(binDir1, 'gsd-browser-bin');
  line(`bin/ exists before download? ${fs.existsSync(binDir1)}`);
  await downloadFile(`http://127.0.0.1:${okServer.address().port}/release`, dest1, http.get);
  line(`bin/ created automatically?  ${fs.existsSync(binDir1)}`);
  line(`binary written?             ${fs.existsSync(dest1)}`);
  line(`content matches server?     ${fs.readFileSync(dest1).toString() === payload.toString()}`);
  line(`(followed 302 redirect -> 200, then wrote ${fs.statSync(dest1).size} bytes)`);
  okServer.close();
  line('');

  // ---- Scenario 2: interrupted download leaves NO partial binary ----
  line('--- Scenario 2: connection drops mid-download (interrupted) ---');
  line('(server promises 1024 bytes then kills the socket after 7 bytes)');
  const partialServer = await startServer((req, res) => {
    res.writeHead(200, { 'Content-Type': 'application/octet-stream', 'Content-Length': '1024' });
    res.write(Buffer.from('partial'));
    res.socket.destroy();
  });
  const dest2 = path.join(work, 's2', 'bin', 'gsd-browser-bin');
  let s2err;
  try {
    await downloadFile(`http://127.0.0.1:${partialServer.address().port}/asset`, dest2, http.get);
  } catch (err) {
    s2err = err;
  }
  line(`download rejected?           ${Boolean(s2err)}`);
  line(`rejection message:           ${s2err && s2err.message}`);
  line(`partial binary left behind?  ${fs.existsSync(dest2)}  <-- must be false (cleaned up)`);
  partialServer.close();
  line('');

  // ---- Scenario 3: destination directory cannot be created ----
  line('--- Scenario 3: destination dir cannot be created (path blocked by a file) ---');
  const blockServer = await startServer((req, res) => {
    res.writeHead(200, { 'Content-Type': 'application/octet-stream' });
    res.end(payload);
  });
  const blocker = path.join(work, 's3', 'not-a-dir');
  fs.mkdirSync(path.dirname(blocker), { recursive: true });
  fs.writeFileSync(blocker, 'i am a file, not a directory');
  const dest3 = path.join(blocker, 'gsd-browser-bin');
  let s3err;
  try {
    await downloadFile(`http://127.0.0.1:${blockServer.address().port}/asset`, dest3, http.get);
  } catch (err) {
    s3err = err;
  }
  line(`download rejected gracefully? ${Boolean(s3err)}`);
  line(`rejection message:            ${s3err && s3err.message}`);
  blockServer.close();
  line('');

  // ---- Scenario 4: requiring the module does NOT trigger the downloader ----
  line('--- Scenario 4: importing postinstall.js as a module is side-effect free ---');
  line('(require.main guard — tests can import without firing a real download)');
  line(`exports available:           downloadFile=${typeof downloadFile}, ensureDir=${typeof ensureDir}`);
  line('(no "postinstall failed" output above => main() did not run on require)');
  line('');

  line('=== All scenarios completed ===');
  fs.rmSync(work, { recursive: true, force: true });
}

main().catch((e) => {
  line(`DEMO ERROR: ${e.stack}`);
  process.exit(1);
});

Pipeline

Updates from git push no-mistakes

⏭️ **intent** - skipped

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info

• ⚠️ npm/scripts/postinstall.js:65 - On a write error (file.on("error")) or interrupted download (res.on("error")), the partially-written file at dest is destroy()ed but never removed from disk. Because main() short-circuits on fs.existsSync(targetPath) and reports 'binary already present' (line 107-113), a truncated/corrupt binary left behind by a failed download is treated as valid on the next npm install, silently installing a broken executable. The fallback path within a single run truncates via createWriteStream, but a leftover partial from a fully-failed run persists across runs. Add fs.rmSync(dest, { force: true }) in both error handlers before rejecting.
• ℹ️ npm/scripts/postinstall.js:44 - On a 301/302 the redirect response res is not consumed/drained before recursing, leaving the socket holding an unread body. Pre-existing in both downloadFile and fetchJSON; minor since the redirect target is GitHub and connections are short-lived, but draining (res.resume()) before recursing would be cleaner.

🔧 Fix: remove partial binary on failed postinstall download
1 warning still open:

• ⚠️ npm/scripts/postinstall.js:72 - In both error handlers, file.destroy() is followed immediately by a synchronous cleanupPartial() (fs.rmSync(dest)). WriteStream.destroy() closes the underlying fd asynchronously, so rmSync runs while the fd is still open. On POSIX this is harmless (unlink of an open file succeeds), but on Windows (win32-x64 is a supported platform) removing a file with an open handle throws EBUSY, which cleanupPartial silently swallows. The partial binary then survives, and on the next npm install the fs.existsSync(targetPath) short-circuit in main() reports 'binary already present' — reintroducing exactly the stale-partial bug this change fixes, for Windows users. Additionally, rejecting before the fd closes means the fallback downloadFile to the same dest may also hit a write conflict. Fix: defer cleanup/settle until the stream actually closes, e.g. file.once('close', () => { cleanupPartial(); settle(reject, err); }); file.destroy();

🔧 Fix: defer partial cleanup until write stream closes
1 info still open:

• ℹ️ npm/scripts/postinstall.js:95 - The ClientRequest-level handler .on("error", reject) settles the promise directly, bypassing the settle/cleanupPartial/failing machinery used by the res.on("error") and file.on("error") paths. If a connection error surfaces on the request after the response callback has already created the write stream (possible for some socket-reset timings), the promise rejects but the partially-written file at dest is neither destroyed nor unlinked — the same stale-partial class of bug this change otherwise fixes, just via a less-common error path. In practice post-response socket errors usually surface on res (which is handled), so this is narrow; cleanly fixing it would require hoisting file/cleanupPartial to be reachable from the request error handler.

✅ **Test** - passed

✅ No issues found.

• node --test tests/postinstall.test.cjs — all 4 subtests pass (ensureDir recursion, download into missing bin/, partial-file cleanup on interrupted download, graceful rejection when dest dir can't be created)
• End-to-end demo node demo-postinstall.cjs <postinstall.js> exercising the real exported downloadFile/ensureDir against a live 127.0.0.1 HTTP server: 302→200 download into a missing bin/, mid-download socket drop, dir-creation blocked by a file, and module-import side-effect check

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

---

Note

Low Risk
Scoped to npm install lifecycle and download error paths; behavior change is defensive hardening with unit tests, though a narrow request-level error path may still leave a partial file.

Overview
Hardens the npm postinstall binary downloader so failed installs are less likely to leave a broken executable or skip a real download on the next run.

downloadFile now creates the destination parent directory immediately before opening the write stream (with clear prepare … failed errors), routes write/response failures through a single fail path that defers rmSync of partial files until the write stream closes (important on Windows), and waits for close before resolving successful writes. main() only runs when the script is executed directly (require.main === module); **ensureDir**, **downloadFile**, and **ensureDir** are exported for tests. Unused **execSync`** import was removed.

Adds tests/postinstall.test.cjs (four node:test cases) and an [Unreleased] CHANGELOG entry for these fixes.

^{Reviewed by Cursor Bugbot for commit deb8b17. Bugbot is set up for automated code reviews on this repo. Configure here.}

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is enabled.}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: Success close deletes downloaded file
  • Added a settled guard in the fail path close handler so cleanupPartial cannot run after the success handler has already resolved.

_{You can send follow-ups to the cloud agent here.}

^{Reviewed by Cursor Bugbot for commit 7d30bb5. Configure here.}