From 95ace765d246786192f8671e409ae94bb9f78627 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Sat, 4 Jul 2026 01:13:58 +0600 Subject: [PATCH 1/2] fix(detector): resolve symlinks when classifying the system node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The system-node classifier matched exec.LookPath's result against manager roots and path patterns without ever resolving symlinks. fnm is the canonical casualty: its shell integration puts a per-shell multishell shim dir on PATH ($FNM_MULTISHELL_PATH/bin), so the node nodeup finds symlink-resolves into $FNM_DIR but shares no path prefix with it — and the user's own manager-owned node reported as "unrecognized layout", a kind the cleanup logic treats as safe to overwrite. ResolveSystemNode now resolves the PATH entry once (new evalSymlinks seam) and consults both locations: manager roots match either path, while path-pattern classification still prefers the unresolved path and only consults the resolved one when the original is unrecognized — resolution can upgrade a classification, never downgrade one (Debian's /usr/bin/node -> /etc/alternatives symlink stays os-package). managerManagedRoots additionally recognizes the fnm multishell farm dirs ($FNM_MULTISHELL_PATH and its parent, $XDG_STATE_HOME / ~/.local/state / legacy $TMPDIR fnm_multishells) so the farm classifies as fnm-owned even when resolution fails. Verified live: nodeup check now reports the fnm multishell node as "managed by fnm". Refs #111 Co-Authored-By: Claude Fable 5 --- internal/detector/system_node.go | 59 +++++++- internal/detector/system_node_helpers.go | 14 +- internal/detector/system_node_test.go | 165 ++++++++++++++++++++++- 3 files changed, 229 insertions(+), 9 deletions(-) diff --git a/internal/detector/system_node.go b/internal/detector/system_node.go index fe1c6c7..7efd19a 100644 --- a/internal/detector/system_node.go +++ b/internal/detector/system_node.go @@ -151,6 +151,18 @@ func ResolveSystemNode(ctx context.Context, m Manager) (SystemNodeInfo, error) { return SystemNodeInfo{}, ErrNoNodeOnPATH } + // The PATH entry is often a symlink into somewhere more telling — + // fnm's per-shell multishell shims are the canonical case (#111): + // ~/.local/state/fnm_multishells//bin/node resolves into + // $FNM_DIR/node-versions//installation/bin/node, squarely under + // a manager root. Resolve once and consult both locations below. + // A failed resolution (broken link, permission) just leaves the + // original path as the only candidate. + resolved := "" + if r, err := evalSymlinks(p); err == nil && r != p { + resolved = r + } + // If the binary is inside the manager's data dir, it's clearly // managed. We try this first because the path-based classifier // can false-positive on a manager that happens to live under @@ -158,7 +170,7 @@ func ResolveSystemNode(ctx context.Context, m Manager) (SystemNodeInfo, error) { if m != nil { if managedRoots, ok := managerManagedRoots(m); ok { for _, root := range managedRoots { - if isInside(p, root) { + if isInside(p, root) || (resolved != "" && isInside(resolved, root)) { return SystemNodeInfo{ Path: p, Kind: SystemNodeManaged, @@ -169,9 +181,20 @@ func ResolveSystemNode(ctx context.Context, m Manager) (SystemNodeInfo, error) { } } + // Classification deliberately prefers the unresolved path: for + // several recognized layouts the symlink itself is the signal + // (Debian's /usr/bin/node → /etc/alternatives/node → /usr/lib/... + // must stay os-package, not become unknown). The resolved path is + // only consulted when the original is unrecognized, so resolution + // can upgrade a classification but never downgrade one. + kind := classifySystemNodePath(p) + if kind == SystemNodeUnknown && resolved != "" { + kind = classifySystemNodePath(resolved) + } + return SystemNodeInfo{ Path: p, - Kind: classifySystemNodePath(p), + Kind: kind, }, nil } @@ -379,11 +402,10 @@ func managerManagedRoots(m Manager) (roots []string, ok bool) { // every plausible root so the path classifier still // recognizes an fnm-managed install when FNM_DIR is // unset. The env override is checked first. - if d := strings.TrimSpace(getenv("FNM_DIR")); d != "" { - return []string{d}, true - } roots := []string{} - if h, err := userHomeDir(); err == nil { + if d := strings.TrimSpace(getenv("FNM_DIR")); d != "" { + roots = append(roots, d) + } else if h, err := userHomeDir(); err == nil { roots = append(roots, filepath.Join(h, ".fnm"), filepath.Join(h, "Library", "Application Support", "fnm"), @@ -394,6 +416,31 @@ func managerManagedRoots(m Manager) (roots []string, ok bool) { roots = append(roots, filepath.Join(h, ".local", "share", "fnm")) } } + + // Multishell farm roots, always appended: fnm's shell + // integration puts a per-shell symlink dir on PATH + // ($FNM_MULTISHELL_PATH/bin), not FNM_DIR itself, so the + // node binary found via PATH lives under the farm and only + // symlink resolution leads back to FNM_DIR. Recognizing the + // farm directly keeps the classifier working even when + // resolution fails. See issue #111. + if ms := strings.TrimSpace(getenv("FNM_MULTISHELL_PATH")); ms != "" { + // The var names this shell's own instance dir; its parent + // is the farm, which also covers instances belonging to + // other still-open shells found via a stale PATH entry. + roots = append(roots, ms, filepath.Dir(ms)) + } + if xs := strings.TrimSpace(getenv("XDG_STATE_HOME")); xs != "" { + roots = append(roots, filepath.Join(xs, "fnm_multishells")) + } + if h, err := userHomeDir(); err == nil { + // fnm's default state dir on macOS + Linux. + roots = append(roots, filepath.Join(h, ".local", "state", "fnm_multishells")) + } + if tmp := strings.TrimSpace(getenv("TMPDIR")); tmp != "" { + // Legacy fnm versions kept the farm under the OS temp dir. + roots = append(roots, filepath.Join(tmp, "fnm_multishells")) + } return roots, true case "nvm": if d := strings.TrimSpace(getenv("NVM_DIR")); d != "" { diff --git a/internal/detector/system_node_helpers.go b/internal/detector/system_node_helpers.go index 17a3193..2211632 100644 --- a/internal/detector/system_node_helpers.go +++ b/internal/detector/system_node_helpers.go @@ -8,7 +8,10 @@ // and runScript in nvm.go. package detector -import "os" +import ( + "os" + "path/filepath" +) // getenv is a package-level seam around os.Getenv used by // managerManagedRoots. Tests overwrite it to inject canned @@ -25,3 +28,12 @@ var getenv = os.Getenv // // Signature matches os.UserHomeDir so a direct assignment works. var userHomeDir = os.UserHomeDir + +// evalSymlinks is a package-level seam around filepath.EvalSymlinks +// used by ResolveSystemNode. Tests overwrite it to map canned PATH +// entries to canned resolution targets without creating real symlinks +// (which need elevated rights on Windows runners). Production code +// never reassigns it. +// +// Signature matches filepath.EvalSymlinks so a direct assignment works. +var evalSymlinks = filepath.EvalSymlinks diff --git a/internal/detector/system_node_test.go b/internal/detector/system_node_test.go index fcd9d25..92592b3 100644 --- a/internal/detector/system_node_test.go +++ b/internal/detector/system_node_test.go @@ -179,6 +179,16 @@ func stubWhichNode(t *testing.T, fn func(ctx context.Context) (string, error)) { whichNode = fn } +// stubEvalSymlinks replaces the package-level evalSymlinks seam for +// the duration of the test. Canned resolutions avoid creating real +// symlinks, which need elevated rights on Windows runners. +func stubEvalSymlinks(t *testing.T, fn func(string) (string, error)) { + t.Helper() + orig := evalSymlinks + t.Cleanup(func() { evalSymlinks = orig }) + evalSymlinks = fn +} + func TestResolveSystemNode_NoNodeOnPATH(t *testing.T) { stubWhichNode(t, func(ctx context.Context) (string, error) { return "", nil // which silently returned empty @@ -289,6 +299,109 @@ func TestResolveSystemNode_NilManagerSafe(t *testing.T) { } } +func TestResolveSystemNode_SymlinkResolvesIntoManagerRoot(t *testing.T) { + // The issue #111 shape: the PATH entry is an fnm multishell shim + // that lives nowhere near FNM_DIR, but symlink-resolves into it. + // The manager-root match must fire on the resolved location. + t.Setenv("FNM_DIR", "/custom/fnm") + + const shim = "/weird/shells/81335_1783094161422/bin/node" + stubWhichNode(t, func(ctx context.Context) (string, error) { + return shim, nil + }) + stubEvalSymlinks(t, func(p string) (string, error) { + if p == shim { + return "/custom/fnm/node-versions/v24.18.0/installation/bin/node", nil + } + return p, nil + }) + + info, err := ResolveSystemNode(context.Background(), fakeManager{name: "fnm"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if info.Kind != SystemNodeManaged { + t.Errorf("Kind = %s, want %s (resolved path is under FNM_DIR)", info.Kind, SystemNodeManaged) + } + if info.Manager != "fnm" { + t.Errorf("Manager = %q, want %q", info.Manager, "fnm") + } + if info.Path != shim { + t.Errorf("Path = %q, want the original PATH entry %q", info.Path, shim) + } +} + +func TestResolveSystemNode_MultishellFarmRootWithoutResolution(t *testing.T) { + // Even when symlink resolution fails outright, a node under the + // fnm multishell farm must classify as fnm-managed: the farm + // parent of $FNM_MULTISHELL_PATH is a recognized root, covering + // sibling instances left on PATH by other shells. + t.Setenv("FNM_MULTISHELL_PATH", "/state/fnm_multishells/999_1") + + stubWhichNode(t, func(ctx context.Context) (string, error) { + return "/state/fnm_multishells/123_9/bin/node", nil + }) + stubEvalSymlinks(t, func(p string) (string, error) { + return "", errors.New("broken link") + }) + + info, err := ResolveSystemNode(context.Background(), fakeManager{name: "fnm"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if info.Kind != SystemNodeManaged { + t.Errorf("Kind = %s, want %s (farm root should match without resolution)", info.Kind, SystemNodeManaged) + } + if info.Manager != "fnm" { + t.Errorf("Manager = %q, want %q", info.Manager, "fnm") + } +} + +func TestResolveSystemNode_ResolvedPathUpgradesUnknownClassification(t *testing.T) { + // No manager attribution (nil manager). A path the classifier + // doesn't recognize, resolving into a recognized layout, should + // take the resolved classification — while Path keeps the + // original PATH entry for display. + stubWhichNode(t, func(ctx context.Context) (string, error) { + return "/weird/place/node", nil + }) + stubEvalSymlinks(t, func(p string) (string, error) { + return "/opt/homebrew/Cellar/node/26.4.0/bin/node", nil + }) + + info, err := ResolveSystemNode(context.Background(), nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if info.Kind != SystemNodeHomebrewCore { + t.Errorf("Kind = %s, want %s (resolved into the Cellar)", info.Kind, SystemNodeHomebrewCore) + } + if info.Path != "/weird/place/node" { + t.Errorf("Path = %q, want the original PATH entry", info.Path) + } +} + +func TestResolveSystemNode_OriginalClassificationWinsOverResolved(t *testing.T) { + // Debian-alternatives shape: /usr/bin/node is a symlink into + // /etc/alternatives, which the classifier does NOT recognize. + // The original path's os-package classification must win — + // resolution may only upgrade Unknown, never replace a match. + stubWhichNode(t, func(ctx context.Context) (string, error) { + return "/usr/bin/node", nil + }) + stubEvalSymlinks(t, func(p string) (string, error) { + return "/etc/alternatives/node", nil + }) + + info, err := ResolveSystemNode(context.Background(), nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if info.Kind != SystemNodeOSPackage { + t.Errorf("Kind = %s, want %s (original classification must win)", info.Kind, SystemNodeOSPackage) + } +} + // --- managerManagedRoots --------------------------------------------------- func TestManagerManagedRoots(t *testing.T) { @@ -308,10 +421,16 @@ func TestManagerManagedRoots(t *testing.T) { "FNM_DIR", "NVM_DIR", "VOLTA_HOME", "ASDF_DIR", "ASDF_DATA_DIR", "MISE_DATA_DIR", "N_PREFIX", "NODENV_ROOT", "XDG_DATA_HOME", + "FNM_MULTISHELL_PATH", "XDG_STATE_HOME", "TMPDIR", } { t.Setenv(v, "") } + // The fnm multishell state-dir root is derived from userHomeDir + // and appended unconditionally (issue #111), so every fnm case + // expects it as a suffix. + fnmStateRoot := filepath.Join("/home/tester", ".local", "state", "fnm_multishells") + cases := []struct { name string mgr string @@ -324,9 +443,9 @@ func TestManagerManagedRoots(t *testing.T) { { name: "fnm env wins over home fallback", mgr: "fnm", envKey: "FNM_DIR", envVal: "/custom/fnm", - wantRoots: []string{"/custom/fnm"}, + wantRoots: []string{"/custom/fnm", fnmStateRoot}, wantOK: true, - description: "env override takes precedence", + description: "env override replaces the data-dir candidates; multishell state root always present", }, { name: "fnm falls back to home (no env)", @@ -335,6 +454,7 @@ func TestManagerManagedRoots(t *testing.T) { filepath.Join("/home/tester", ".fnm"), filepath.Join("/home/tester", "Library", "Application Support", "fnm"), filepath.Join("/home/tester", ".local", "share", "fnm"), + fnmStateRoot, }, wantOK: true, description: "no env → enumerate plausible XDG + legacy roots", @@ -346,10 +466,51 @@ func TestManagerManagedRoots(t *testing.T) { filepath.Join("/home/tester", ".fnm"), filepath.Join("/home/tester", "Library", "Application Support", "fnm"), filepath.Join("/srv/data", "fnm"), + fnmStateRoot, }, wantOK: true, description: "XDG_DATA_HOME wins over ~/.local/share", }, + { + name: "fnm multishell env adds instance + farm", + mgr: "fnm", envKey: "FNM_MULTISHELL_PATH", envVal: "/state/fnm_multishells/999_1", + wantRoots: []string{ + filepath.Join("/home/tester", ".fnm"), + filepath.Join("/home/tester", "Library", "Application Support", "fnm"), + filepath.Join("/home/tester", ".local", "share", "fnm"), + "/state/fnm_multishells/999_1", + "/state/fnm_multishells", + fnmStateRoot, + }, + wantOK: true, + description: "FNM_MULTISHELL_PATH contributes the instance dir and its farm parent (#111)", + }, + { + name: "fnm XDG_STATE_HOME multishell root", + mgr: "fnm", envKey: "XDG_STATE_HOME", envVal: "/srv/state", + wantRoots: []string{ + filepath.Join("/home/tester", ".fnm"), + filepath.Join("/home/tester", "Library", "Application Support", "fnm"), + filepath.Join("/home/tester", ".local", "share", "fnm"), + filepath.Join("/srv/state", "fnm_multishells"), + fnmStateRoot, + }, + wantOK: true, + description: "XDG_STATE_HOME contributes its fnm_multishells farm (#111)", + }, + { + name: "fnm TMPDIR legacy multishell root", + mgr: "fnm", envKey: "TMPDIR", envVal: "/tmpx", + wantRoots: []string{ + filepath.Join("/home/tester", ".fnm"), + filepath.Join("/home/tester", "Library", "Application Support", "fnm"), + filepath.Join("/home/tester", ".local", "share", "fnm"), + fnmStateRoot, + filepath.Join("/tmpx", "fnm_multishells"), + }, + wantOK: true, + description: "legacy fnm kept the farm under the OS temp dir (#111)", + }, { name: "nvm env wins", mgr: "nvm", envKey: "NVM_DIR", envVal: "/usr/local/nvm", From 6ce416b33a43ffa13f8abb2bc9f7b391a614b616 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Sat, 4 Jul 2026 01:16:24 +0600 Subject: [PATCH 2/2] test(detector): normalize expected farm-parent path for Windows filepath.Dir rewrites separators, so the Windows runner got \state\fnm_multishells where the table expected the forward-slash literal. Mirror production by computing the expectation with filepath.Dir. Refs #111 Co-Authored-By: Claude Fable 5 --- internal/detector/system_node_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/detector/system_node_test.go b/internal/detector/system_node_test.go index 92592b3..51134bb 100644 --- a/internal/detector/system_node_test.go +++ b/internal/detector/system_node_test.go @@ -479,7 +479,9 @@ func TestManagerManagedRoots(t *testing.T) { filepath.Join("/home/tester", "Library", "Application Support", "fnm"), filepath.Join("/home/tester", ".local", "share", "fnm"), "/state/fnm_multishells/999_1", - "/state/fnm_multishells", + // filepath.Dir normalizes separators (backslashes on + // Windows), so mirror production instead of a literal. + filepath.Dir("/state/fnm_multishells/999_1"), fnmStateRoot, }, wantOK: true,