fix: detect npm.cmd on Windows in setup prerequisite check#2
Merged
DavidsonGomes merged 3 commits intoApr 10, 2026
Conversation
…/Git Bash, npm may only be available as npm.cmd and not directly as npm. Falls back to npm.cmd when the plain npm command fails. Also separates the error messages for node and npm so the user knows exactly which tool is missing instead of both being reported as "Node.js 18+".
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the setup prerequisite check to correctly detect npm on Windows (including Git Bash) by falling back to npm.cmd, and improves error reporting by distinguishing between missing node and missing npm. Flow diagram for npm detection fallback logic in check_prerequisitesflowchart TD
A[Start npm check] --> B[Set npm_cmd = npm]
B --> C[Run subprocess npm_cmd --version]
C --> D{subprocess succeeded
and returncode == 0?}
D -->|Yes| E[Print npm version as OK]
E --> Z[End npm check]
D -->|No| F[Raise FileNotFoundError]
F --> G[Fallback: set npm_cmd = npm.cmd]
G --> H[Run subprocess npm_cmd --version]
H --> I{returncode == 0?}
I -->|Yes| J[Print npm version as OK]
J --> Z
I -->|No| K[Append npm to errors]
H --> L{subprocess raised
FileNotFoundError or TimeoutExpired?}
L -->|Yes| K
K --> Z
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider limiting the
npm.cmdfallback to Windows platforms (e.g. usingos.name == 'nt'orplatform.system() == 'Windows') so non-Windows environments don't unnecessarily attempt to executenpm.cmdwhennpmexists but exits with a non-zero status for other reasons. - Instead of raising
FileNotFoundErrorwhennpmreturns a non-zero exit code, treat non-zero return codes separately from missing binaries so that genuinenpmerrors don't incorrectly trigger the fallback tonpm.cmd.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider limiting the `npm.cmd` fallback to Windows platforms (e.g. using `os.name == 'nt'` or `platform.system() == 'Windows'`) so non-Windows environments don't unnecessarily attempt to execute `npm.cmd` when `npm` exists but exits with a non-zero status for other reasons.
- Instead of raising `FileNotFoundError` when `npm` returns a non-zero exit code, treat non-zero return codes separately from missing binaries so that genuine `npm` errors don't incorrectly trigger the fallback to `npm.cmd`.
## Individual Comments
### Comment 1
<location path="setup.py" line_range="74-75" />
<code_context>
- print(f" {GREEN}✓{RESET} npm: {DIM}v{result.stdout.strip()}{RESET}")
- else:
- errors.append("npm")
+ result = subprocess.run([npm_cmd, "--version"], capture_output=True, text=True, timeout=5)
+ if result.returncode != 0:
+ raise FileNotFoundError
+ print(f" {GREEN}✓{RESET} npm: {DIM}v{result.stdout.strip()}{RESET}")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Treating any non-zero npm exit code as FileNotFound conflates command discovery with runtime errors.
Mapping any non-zero exit code from `npm` to `FileNotFoundError` causes the code to always fall back to `npm.cmd` and ultimately report npm as missing, even when it exists but failed for another reason (e.g., config/permission issues). Instead, only fall back to `npm.cmd` when an actual `FileNotFoundError` occurs, and treat non-zero exit codes separately (e.g., log the error and add "npm" to `errors` without changing the exception type).
</issue_to_address>
### Comment 2
<location path="setup.py" line_range="99-103" />
<code_context>
- if "node" in errors or "npm" in errors:
+ if "node" in errors:
print(f" {RED}•{RESET} Node.js 18+ — {BOLD}https://nodejs.org{RESET}")
+ if "npm" in errors:
+ print(f" {RED}•{RESET} npm not found (Node.js installed but npm missing from PATH) — {BOLD}https://nodejs.org{RESET}")
print()
print(f" {YELLOW}Install the missing tools and run setup again.{RESET}")
</code_context>
<issue_to_address>
**suggestion:** The npm error message can be misleading when Node.js is also missing.
This message assumes Node.js is present, but this branch can also run when `"node" in errors` is true (showing both lines). To avoid confusion, either use a neutral message like `npm not found (not installed or not in PATH)` or vary the text depending on whether `"node"` is also in `errors`.
```suggestion
if "node" in errors:
print(f" {RED}•{RESET} Node.js 18+ — {BOLD}https://nodejs.org{RESET}")
if "npm" in errors:
print(f" {RED}•{RESET} npm not found (not installed or not in PATH) — {BOLD}https://nodejs.org{RESET}")
print()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+74
to
+75
| result = subprocess.run([npm_cmd, "--version"], capture_output=True, text=True, timeout=5) | ||
| if result.returncode != 0: |
There was a problem hiding this comment.
suggestion (bug_risk): Treating any non-zero npm exit code as FileNotFound conflates command discovery with runtime errors.
Mapping any non-zero exit code from npm to FileNotFoundError causes the code to always fall back to npm.cmd and ultimately report npm as missing, even when it exists but failed for another reason (e.g., config/permission issues). Instead, only fall back to npm.cmd when an actual FileNotFoundError occurs, and treat non-zero exit codes separately (e.g., log the error and add "npm" to errors without changing the exception type).
Comment on lines
+99
to
103
| if "node" in errors: | ||
| print(f" {RED}•{RESET} Node.js 18+ — {BOLD}https://nodejs.org{RESET}") | ||
| if "npm" in errors: | ||
| print(f" {RED}•{RESET} npm not found (Node.js installed but npm missing from PATH) — {BOLD}https://nodejs.org{RESET}") | ||
| print() |
There was a problem hiding this comment.
suggestion: The npm error message can be misleading when Node.js is also missing.
This message assumes Node.js is present, but this branch can also run when "node" in errors is true (showing both lines). To avoid confusion, either use a neutral message like npm not found (not installed or not in PATH) or vary the text depending on whether "node" is also in errors.
Suggested change
| if "node" in errors: | |
| print(f" {RED}•{RESET} Node.js 18+ — {BOLD}https://nodejs.org{RESET}") | |
| if "npm" in errors: | |
| print(f" {RED}•{RESET} npm not found (Node.js installed but npm missing from PATH) — {BOLD}https://nodejs.org{RESET}") | |
| print() | |
| if "node" in errors: | |
| print(f" {RED}•{RESET} Node.js 18+ — {BOLD}https://nodejs.org{RESET}") | |
| if "npm" in errors: | |
| print(f" {RED}•{RESET} npm not found (not installed or not in PATH) — {BOLD}https://nodejs.org{RESET}") | |
| print() |
Prevents UnicodeEncodeError on systems where the default locale is not UTF-8 (e.g., Windows with cp1252). Also syncs package-lock.json with updated dependency tree.
Replace Unix-only PTY (pty/fcntl/termios) with a platform-aware implementation that uses pywinpty on Windows and the existing PTY stack on Unix. All session lifecycle operations (spawn, kill, resize, WebSocket I/O) now branch on sys.platform. Adds pywinpty>=3.0.3 as a conditional Windows dependency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DavidsonGomes
approved these changes
Apr 10, 2026
Member
DavidsonGomes
left a comment
DavidsonGomes
left a comment
There was a problem hiding this comment.
LGTM — suporte Windows (npm.cmd fallback + ConPTY). Ajustes menores serão feitos direto na develop.
8 tasks
5 tasks
This was referenced Apr 23, 2026
DavidsonGomes
added a commit
that referenced
this pull request
Apr 28, 2026
…tives (Step 1) Introduces dashboard/packages/ui as an npm workspace package that gives plugins direct access to the host design system without duplicating Tailwind config. Primitives: Button, Card/CardHeader/CardBody, Badge, Input/FormField, Select, Checkbox, Modal, ToastProvider/useToast, ErrorBoundary Schema-driven CRUD: - SchemaForm: driven by JSON Schema (string, number, boolean, enum, date widgets), client-side validation via bundled Ajv 8, no react-hook-form / zod dependency - SchemaTable: data + TableColumn[] with sorting and type-aware cell formatting Tailwind 4 tokens: packages/ui/src/tokens.css exports :root CSS custom properties (:root vars work via @import; @theme utilities require inlining in the plugin's CSS entry file — documented in tokens.css). Spike confirmed: @tailwindcss/vite does not process @theme from transitively-imported CSS files. Host wiring: - dashboard/package.json: npm workspaces root (["frontend", "packages/*"]) - frontend/tsconfig.app.json: customConditions: ["source"] for workspace resolution - frontend/vite.config.ts: resolve.conditions + @evonexus/ui/tokens.css alias - frontend/src/index.css: @import "@evonexus/ui/tokens.css" - frontend/src/App.tsx: /dev/ui-playground route - frontend/src/pages/UIPlayground.tsx: playground showing all primitives + evo-essentials schema Decisions recorded: - #1 Distribution: workspace internal (no publish). Zero friction for parallel dev. - #2 Tailwind preset: CSS-first via tokens.css :root block. @theme must be in root CSS. - #3 Version: 0.1.0, lockstep with host. - #5 Ajv: bundled inside @evonexus/ui, plugin authors do not install separately. - usePluginNavigation: name reserved in index.ts, implements in Step 2. Build: tsc -b -> 0 errors; vite build -> UIPlayground-yJlY1VKK.js (127KB) in dist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
9 tasks
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
2 participants
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.
fix: detect npm.cmd on Windows in setup prerequisite check
On Windows (especially when using Git Bash),
npmmay only be available asnpm.cmdrather thannpm.This change introduces a fallback mechanism:
npmnpm.cmdAdditionally, error handling has been improved:
nodeandnpmSummary by Sourcery
Improve prerequisite checks for Node.js tooling in setup script
Bug Fixes:
Enhancements: