fix(npm): detect GNU tar silent failure on Windows installer#134
Merged
jonathanpopham merged 1 commit intomainfrom Apr 15, 2026
Merged
fix(npm): detect GNU tar silent failure on Windows installer#134jonathanpopham merged 1 commit intomainfrom
jonathanpopham merged 1 commit intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthroughdownload() now waits for file descriptor release before proceeding. extractZip() on Windows prefers native tar.exe, falls back to a PowerShell Expand-Archive with a bounded retry loop, and verifies extraction by reading the temp dir — if no files are produced the fallback runs or the last error is thrown. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Installer as Install.js
participant Tar as tar.exe / tar (shell)
participant PowerShell as PowerShell Expand-Archive
participant FS as TempDir (readdirSync)
Client->>Installer: start download()
Installer->>Installer: write stream -> wait for 'close'
Installer->>Tar: attempt extraction (prefer native tar.exe on Windows)
Tar-->>Installer: exit code (0/err)
Installer->>FS: readdirSync(tmpDir)
alt tmpDir has files
Installer-->>Client: success (stop)
else tmpDir empty or read error
Installer->>PowerShell: run Expand-Archive with retry loop
PowerShell-->>Installer: exit/result
Installer->>FS: readdirSync(tmpDir)
alt tmpDir has files
Installer-->>Client: success
else
Installer-->>Client: throw last error / no-files error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
npm/install.test.js (1)
130-132: Optional: tighten the fallback assertion a bit more.You already assert
Expand-Archive; adding apowershellcheck here would make the intent of this specific path even clearer.Small assertion add-on
assert.equal(commands.length, 2, "should fall back to PowerShell when tar produced no files"); assert.ok(commands[0].startsWith("tar"), "first call should be tar"); + assert.ok(commands[1].includes("powershell"), "second call should be PowerShell"); assert.ok(commands[1].includes("Expand-Archive"), "fallback should use Expand-Archive");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@npm/install.test.js` around lines 130 - 132, The test asserts that the fallback uses Expand-Archive but doesn't explicitly verify PowerShell; update the assertion in npm/install.test.js that checks the fallback (the commands array used in the failing-tar path) to also assert that commands[1] includes a case-insensitive "powershell" token (or startsWith/contains "powershell -NoProfile -Command") so the intent is explicit; modify the assertion alongside the existing commands[1].includes("Expand-Archive") check to include the powershell check for clearer intent.npm/install.js (1)
58-60: Consider validating the expected artifact instead of onlyentries.length > 0.Line 60 can report success if
tmpDiralready contains unrelated files. A slightly safer check is verifyingsupermodel.exeexists after tar (or comparing before/after directory state).Suggested hardening
- let entries = []; - try { entries = fs.readdirSync(tmpDir); } catch { /* missing dir => failure */ } - tarOk = entries.length > 0; + try { + tarOk = fs.existsSync(path.join(tmpDir, "supermodel.exe")); + } catch { + tarOk = false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@npm/install.js` around lines 58 - 60, The current success check sets tarOk based solely on entries.length > 0 which can be fooled by unrelated files; instead verify the expected artifact (e.g., presence of "supermodel.exe") in tmpDir after extraction: after reading tmpDir with fs.readdirSync (entries) set tarOk = entries.includes("supermodel.exe") (or perform a direct fs.existsSync(path.join(tmpDir, "supermodel.exe"))) and optionally record the before/after state to ensure the file was newly created rather than pre-existing; update the logic that sets tarOk and any error handling to rely on this explicit artifact check rather than a non-empty directory test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@npm/install.js`:
- Around line 58-60: The current success check sets tarOk based solely on
entries.length > 0 which can be fooled by unrelated files; instead verify the
expected artifact (e.g., presence of "supermodel.exe") in tmpDir after
extraction: after reading tmpDir with fs.readdirSync (entries) set tarOk =
entries.includes("supermodel.exe") (or perform a direct
fs.existsSync(path.join(tmpDir, "supermodel.exe"))) and optionally record the
before/after state to ensure the file was newly created rather than
pre-existing; update the logic that sets tarOk and any error handling to rely on
this explicit artifact check rather than a non-empty directory test.
In `@npm/install.test.js`:
- Around line 130-132: The test asserts that the fallback uses Expand-Archive
but doesn't explicitly verify PowerShell; update the assertion in
npm/install.test.js that checks the fallback (the commands array used in the
failing-tar path) to also assert that commands[1] includes a case-insensitive
"powershell" token (or startsWith/contains "powershell -NoProfile -Command") so
the intent is explicit; modify the assertion alongside the existing
commands[1].includes("Expand-Archive") check to include the powershell check for
clearer intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ec562f0-789c-4174-b17b-5a3a6c15871d
📒 Files selected for processing (2)
npm/install.jsnpm/install.test.js
jonathanpopham
force-pushed
the
fix/windows-gnu-tar-silent-failure
branch
from
7e45459 to
4337945
Compare
Real failure chain on Windows was two-layered: 1. GNU tar (resolved from PATH under Git Bash, npm's postinstall shell) parses `C:\...` as rsh host:path, prints "Cannot connect to C: resolve failed", and exits 0 with nothing extracted. The try/catch PowerShell fallback never fired because tar didn't throw. 2. Even when the fallback did run, Windows Defender held a scan-lock on the freshly-downloaded .exe-containing zip; Expand-Archive kept failing with "The process cannot access the file ... because it is being used by another process". The existing retry loop silently swallowed all 10 retries and returned, so the caller never knew extraction failed. Fix: - Call the native Windows bsdtar directly at %SystemRoot%\System32\tar.exe (Windows 10+). bsdtar handles both zip format and `C:\` paths natively and isn't blocked by Defender the way Expand-Archive is. - Keep PowerShell Expand-Archive as a fallback for older Windows, with the retry loop bumped from 10 → 20 attempts and — critically — a `throw $lastErr` so exhausted retries surface instead of silently returning. - Always verify `tmpDir` is non-empty after extraction; if a tool exits 0 without writing anything, treat that as failure and try the next one. - Wait for the download stream's `close` event, not just `finish`, so the fd is truly released before any extractor runs. Tests rewritten to cover: first-extractor success, fallback on throw, fallback on silent-empty success, throw-on-exhaustion for both failure modes, PS retry-loop shape, and path propagation. Closes #133
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #133. The Windows installer failure on clean machines was two bugs compounding:
tarto GNU tar, which parsesC:\...as rshhost:path, printsCannot connect to C: resolve failed, and exits 0 with nothing extracted. The existingtry/catchPowerShell fallback never fired because tar didn't throw.Expand-Archivekept failing withThe process cannot access the file ... because it is being used by another process. The existing retry loop silently swallowed all 10 retries and returned, so the caller never knew.Changes
%SystemRoot%\System32\tar.exe(Windows 10+). It handles both zip format andC:\paths and isn't blocked by Defender the wayExpand-Archiveis.Expand-Archiveas a fallback for older Windows; bump retries 10→20 and — critically —throw $lastErrso exhausted retries surface instead of silently returning.tmpDiris non-empty after each extraction attempt; tools that exit 0 without writing anything (GNU tar) now get detected and fall through to the next attempt.closeevent (not justfinish) so the fd is actually released before extraction runs.Test plan
node --test npm/install.test.js— 7 tests pass, covering: first-extractor success, fallback on throw, fallback on silent-empty exit, throw-on-exhaustion for both modes, PS retry-loop shape, and path propagation./bin/supermodel.exe --versionprints0.6.5🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Tests