fix: avoid Windows npm spawn failure in Pro install - #735
Merged
Conversation
rafaelscosta
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
May 14, 2026 15:03
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThis pull request centralizes npm command invocation in the installer wizard through shared helper functions that handle platform-specific execution. The Pro artifact installation steps are refactored to use the new abstraction, test coverage validates the cross-platform invocation resolution, and package versions are incremented across the root, installer, and compatibility wrapper packages. ChangesNPM Invocation Refactoring and Version Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
type: test
Contributor
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This was referenced May 14, 2026
rafaelscosta
added a commit
that referenced
this pull request
May 17, 2026
…json (#742) * fix(installer): isolate Pro install from ancestor workspaces/package.json Cohort students hit "Pro activation failed: Installed Pro artifact did not create node_modules/@aiox-squads/pro" even though npm install exits 0 and the upstream Pro license activation completes successfully. Root cause: installProArtifactIntoTarget runs `npm install <tgz>` in targetDir without --prefix or --workspaces=false. npm 10+ walks the directory tree, finds the first ancestor with a package.json (or one declaring workspaces), and installs node_modules there instead of in the target. The post-install integrity check then fails because the artifact is in the wrong place, not because it never downloaded. This was reproduced locally in 4 install topologies before patching, then proved by running the new test suite against pre-fix code — the two ancestor-hijack scenarios fail cleanly without the fix: 1. workspace declared in ancestor → npm installs at workspace root (FAILS pre-fix) 2. plain package.json in ancestor → npm installs at ancestor (FAILS pre-fix) 3. empty targetDir, clean ancestor → works (current happy path) 4. targetDir with own package.json → works (current happy path) Fixes applied: * installProArtifactIntoTarget: pass --prefix=targetDir + --workspaces=false + --include-workspace-root=false; create a synthetic anchor package.json if targetDir is empty, then clean it up after. * extractProArtifactToTemp: mirror the same flags to harden the temp extraction (it never failed in practice because os.tmpdir() has no ancestors with package.json, but the asymmetry was fragile). * acquireProArtifactSourceDir: when the target install fails, fall back to the extracted (sha256-verified) Pro source from tempRoot and emit a warning instead of bricking the wizard. The scaffolder copies files from the source dir, so the target install is a convenience, not a requirement. The warning is explicit that subsequent `aiox install` calls in the same directory will re-download Pro until the user runs from a fresh empty directory. * New diagnostic helper findAncestorNodeModulesPro walks up to 8 levels looking for a stray install, included in the user-facing error so students can see where npm actually put the package. Tests added: tests/installer/pro-setup-target-install.test.js — 8 regression tests covering all four install topologies, the synthetic anchor cleanup, the diagnostic helper (positive + negative cases), and the graceful fallback contract in acquireProArtifactSourceDir. Integration-style: builds a minimal fixture tarball at runtime via `npm pack`, no external dependencies, no Supabase credentials needed. CI added: .github/workflows/installer-smoke-matrix.yml — runs the regression suite + three end-to-end smoke installs (empty target, nested in workspace, nested under plain package.json) on Ubuntu/macOS/ Windows with Node 18/20/22 and npm 8/10/11. Triggers on packages/installer/** and tests/installer/** changes, breaking the "11 fixes to pro-setup.js in 30 days, each catching a new edge case" pattern by validating the install surface itself before publish. Cleared the dead fallback path in acquireProArtifactSourceDir that was already wired (`installedProSourceDir || extractedProSourceDir`) but unreachable because the throw in installProArtifactIntoTarget short- circuited it. Affected story: PRO-13.6 (signed artifact pipeline) Builds on: #735 (Windows npm spawn), PRO-13.5 (private distribution) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(installer-smoke): address CodeRabbit feedback on PR #742 * Use platform-aware npm binary (npm.cmd on Windows) in the test fixture tarball builder and the 3 inline smoke install scripts in installer-smoke-matrix.yml. Mirrors the pattern in pro-setup.js' resolveNpmInvocation. Without this, the Windows job fixed below would fail at the `execFileSync('npm', ...)` call. * Replace the cross-product matrix with an explicit `include:` list and drop combos that are structurally invalid (and were failing CI on the first run): Node 18 + npm 11 (npm 11 requires Node >= 20.17), Node 20 + npm 8 (npm 8 too old to honor --workspaces=false reliably), and the redundant npm 11 re-pin on Node 22 (already ships with npm 11; the re-pin via `npm install --global` conflicts with the bundled install on GitHub-hosted runners). Added a new `bundled` mode that skips the pin step entirely. * Coverage retained: Ubuntu (Node 18/20/22 × npm 10/11/bundled), macOS (Node 20/22), Windows (Node 20/22). 8 combos total, all valid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tuanmedeiros
pushed a commit
to tuanmedeiros/aios-core-synkraay
that referenced
this pull request
Jun 2, 2026
tuanmedeiros
pushed a commit
to tuanmedeiros/aios-core-synkraay
that referenced
this pull request
Jun 2, 2026
…json (SynkraAI#742) * fix(installer): isolate Pro install from ancestor workspaces/package.json Cohort students hit "Pro activation failed: Installed Pro artifact did not create node_modules/@aiox-squads/pro" even though npm install exits 0 and the upstream Pro license activation completes successfully. Root cause: installProArtifactIntoTarget runs `npm install <tgz>` in targetDir without --prefix or --workspaces=false. npm 10+ walks the directory tree, finds the first ancestor with a package.json (or one declaring workspaces), and installs node_modules there instead of in the target. The post-install integrity check then fails because the artifact is in the wrong place, not because it never downloaded. This was reproduced locally in 4 install topologies before patching, then proved by running the new test suite against pre-fix code — the two ancestor-hijack scenarios fail cleanly without the fix: 1. workspace declared in ancestor → npm installs at workspace root (FAILS pre-fix) 2. plain package.json in ancestor → npm installs at ancestor (FAILS pre-fix) 3. empty targetDir, clean ancestor → works (current happy path) 4. targetDir with own package.json → works (current happy path) Fixes applied: * installProArtifactIntoTarget: pass --prefix=targetDir + --workspaces=false + --include-workspace-root=false; create a synthetic anchor package.json if targetDir is empty, then clean it up after. * extractProArtifactToTemp: mirror the same flags to harden the temp extraction (it never failed in practice because os.tmpdir() has no ancestors with package.json, but the asymmetry was fragile). * acquireProArtifactSourceDir: when the target install fails, fall back to the extracted (sha256-verified) Pro source from tempRoot and emit a warning instead of bricking the wizard. The scaffolder copies files from the source dir, so the target install is a convenience, not a requirement. The warning is explicit that subsequent `aiox install` calls in the same directory will re-download Pro until the user runs from a fresh empty directory. * New diagnostic helper findAncestorNodeModulesPro walks up to 8 levels looking for a stray install, included in the user-facing error so students can see where npm actually put the package. Tests added: tests/installer/pro-setup-target-install.test.js — 8 regression tests covering all four install topologies, the synthetic anchor cleanup, the diagnostic helper (positive + negative cases), and the graceful fallback contract in acquireProArtifactSourceDir. Integration-style: builds a minimal fixture tarball at runtime via `npm pack`, no external dependencies, no Supabase credentials needed. CI added: .github/workflows/installer-smoke-matrix.yml — runs the regression suite + three end-to-end smoke installs (empty target, nested in workspace, nested under plain package.json) on Ubuntu/macOS/ Windows with Node 18/20/22 and npm 8/10/11. Triggers on packages/installer/** and tests/installer/** changes, breaking the "11 fixes to pro-setup.js in 30 days, each catching a new edge case" pattern by validating the install surface itself before publish. Cleared the dead fallback path in acquireProArtifactSourceDir that was already wired (`installedProSourceDir || extractedProSourceDir`) but unreachable because the throw in installProArtifactIntoTarget short- circuited it. Affected story: PRO-13.6 (signed artifact pipeline) Builds on: SynkraAI#735 (Windows npm spawn), PRO-13.5 (private distribution) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(installer-smoke): address CodeRabbit feedback on PR SynkraAI#742 * Use platform-aware npm binary (npm.cmd on Windows) in the test fixture tarball builder and the 3 inline smoke install scripts in installer-smoke-matrix.yml. Mirrors the pattern in pro-setup.js' resolveNpmInvocation. Without this, the Windows job fixed below would fail at the `execFileSync('npm', ...)` call. * Replace the cross-product matrix with an explicit `include:` list and drop combos that are structurally invalid (and were failing CI on the first run): Node 18 + npm 11 (npm 11 requires Node >= 20.17), Node 20 + npm 8 (npm 8 too old to honor --workspaces=false reliably), and the redundant npm 11 re-pin on Node 22 (already ships with npm 11; the re-pin via `npm install --global` conflicts with the bundled install on GitHub-hosted runners). Added a new `bundled` mode that skips the pin step entirely. * Coverage retained: Ubuntu (Node 18/20/22 × npm 10/11/bundled), macOS (Node 20/22), Windows (Node 20/22). 8 combos total, all valid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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\n- fix Pro artifact extraction/install npm invocation on Windows by preferring node + npm-cli.js\n- fall back to shell execution for npm.cmd when npm_execpath is unavailable\n- bump @aiox-squads/core/aiox-core to 5.2.4 and installer to 3.3.4\n\n## Validation\n- npm test -- tests/installer/pro-setup-auth.test.js --runInBand\n- npm run lint -- --no-cache packages/installer/src/wizard/pro-setup.js tests/installer/pro-setup-auth.test.js\n- npm test -- tests/installer/pro-setup-auth.test.js tests/unit/wizard/validation/dependency-validator.test.js --runInBand\n- npm run validate:publish
Summary by CodeRabbit
Chores
Bug Fixes
Tests