mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-01-30 14:22:02 +00:00
fix: address code review feedback for node-finder
- Fix PATH collision detection using proper path segment matching instead of substring includes() which could cause false positives - Reorder fnm Windows paths to prioritize canonical installation path over shell shims (fnm_multishells) - Make Windows path test platform-aware since path.dirname handles backslash paths differently on non-Windows systems 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -180,13 +180,14 @@ function findNodeWindows(homeDir: string): NodeFinderResult | null {
|
||||
return { nodePath: nvmNode, source: 'nvm-windows' };
|
||||
}
|
||||
|
||||
// fnm on Windows
|
||||
// fnm on Windows (prioritize canonical installation path over shell shims)
|
||||
const fnmWindowsPaths = [
|
||||
path.join(homeDir, '.fnm', 'node-versions'),
|
||||
path.join(
|
||||
process.env.LOCALAPPDATA || path.join(homeDir, 'AppData', 'Local'),
|
||||
'fnm_multishells'
|
||||
'fnm',
|
||||
'node-versions'
|
||||
),
|
||||
path.join(homeDir, '.fnm', 'node-versions'),
|
||||
];
|
||||
|
||||
for (const fnmBasePath of fnmWindowsPaths) {
|
||||
@@ -332,7 +333,9 @@ export function buildEnhancedPath(nodePath: string, currentPath: string = ''): s
|
||||
const nodeDir = path.dirname(nodePath);
|
||||
|
||||
// Don't add if already present or if it's just '.'
|
||||
if (nodeDir === '.' || currentPath.includes(nodeDir)) {
|
||||
// Use path segment matching to avoid false positives (e.g., /opt/node vs /opt/node-v18)
|
||||
const pathSegments = currentPath.split(path.delimiter);
|
||||
if (nodeDir === '.' || pathSegments.includes(nodeDir)) {
|
||||
return currentPath;
|
||||
}
|
||||
|
||||
|
||||
@@ -96,12 +96,21 @@ describe('node-finder', () => {
|
||||
});
|
||||
|
||||
it('should handle Windows-style paths', () => {
|
||||
// On Windows, path.dirname recognizes backslash paths
|
||||
// On other platforms, backslash is not a path separator
|
||||
const nodePath = 'C:\\Program Files\\nodejs\\node.exe';
|
||||
const currentPath = 'C:\\Windows\\System32';
|
||||
|
||||
const result = buildEnhancedPath(nodePath, currentPath);
|
||||
|
||||
expect(result).toBe(`C:\\Program Files\\nodejs${delimiter}${currentPath}`);
|
||||
if (process.platform === 'win32') {
|
||||
// On Windows, should prepend the node directory
|
||||
expect(result).toBe(`C:\\Program Files\\nodejs${delimiter}${currentPath}`);
|
||||
} else {
|
||||
// On non-Windows, backslash paths are treated as relative paths
|
||||
// path.dirname returns '.' so the function returns currentPath unchanged
|
||||
expect(result).toBe(currentPath);
|
||||
}
|
||||
});
|
||||
|
||||
it('should use default empty string for currentPath', () => {
|
||||
|
||||
Reference in New Issue
Block a user