From d3005393dfc7dfffaf6f4aee7aa9c07d2564eacc Mon Sep 17 00:00:00 2001
From: Kacper <kacperlachowiczwp.pl@wp.pl>
Date: Sun, 21 Dec 2025 14:49:19 +0100
Subject: [PATCH] fix: address code review feedback for node-finder
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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>
---
 libs/platform/src/node-finder.ts        | 11 +++++++----
 libs/platform/tests/node-finder.test.ts | 11 ++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/libs/platform/src/node-finder.ts b/libs/platform/src/node-finder.ts
index 8dabfa3b..9b45d9e8 100644
--- a/libs/platform/src/node-finder.ts
+++ b/libs/platform/src/node-finder.ts
@@ -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;
   }
 
diff --git a/libs/platform/tests/node-finder.test.ts b/libs/platform/tests/node-finder.test.ts
index 98521636..f37d0d85 100644
--- a/libs/platform/tests/node-finder.test.ts
+++ b/libs/platform/tests/node-finder.test.ts
@@ -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', () => {