fix: correct cross-platform PATH splitting and dead code in executor - #1
Merged
Conversation
resolver.ts: findOnPath() was splitting PATH using path.delimiter (which is ":" on Linux/macOS even when IS_WINDOWS is true in tests). Windows paths like "C:\dir" contain a literal ":" that the Linux delimiter would split on, causing the PATH walk to silently fail and fall through to the literal "commandcode" fallback. Fix: use IS_WINDOWS ? ";" : delimiter so the split character always matches the target platform, not the host OS. This makes the three Windows PATH-walk resolver tests pass on Linux/macOS. executor.ts: remove the unused IS_WINDOWS import (leftover from when spawn used shell:true on Windows) and correct the JSDoc that still claimed shell:true/false split by OS. https://claude.ai/code/session_01Qkq1tdzF4drY3DSg61CSAx
Removes stale "peer": true metadata fields from several lockfile entries (npm v10 no longer emits this field for non-peer deps). https://claude.ai/code/session_01Qkq1tdzF4drY3DSg61CSAx
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by nemotron-3-super-120b-a12b-20230311:free · 174,723 tokens |
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.
Summary
src/resolver.ts:findOnPath()was splittingPATHusingpath.delimiter, which is":"on Linux/macOS even whenIS_WINDOWSis mocked totruein tests. Windows-style paths like"C:\dir"contain a literal":"that the Linux delimiter splits on, silently breaking the PATH walk and falling through to the literal"commandcode"fallback. Fix: useIS_WINDOWS ? ";" : delimiterso the split character always matches the target platform, not the host OS. This fixes the three Windows PATH-walk resolver tests that were failing on Linux/macOS.src/executor.ts: Remove the unusedIS_WINDOWSimport (leftover from whenspawnusedshell: trueon Windows) and correct the JSDoc that still claimed ashell: true/falseOS split.Test plan
npm run build— TypeScript compiles cleanlynpm test— all 108 tests pass (including the 3 Windows PATH-walk tests that were previously failing on Linux/macOS)node scripts/smoke.mjs— version banner prints, no deprecation warningshttps://claude.ai/code/session_01Qkq1tdzF4drY3DSg61CSAx
Generated by Claude Code