From d311962928467e64cc663dc496caffad44d6329e Mon Sep 17 00:00:00 2001 From: gok03 Date: Thu, 18 Jun 2026 15:27:49 +0530 Subject: [PATCH 1/2] fix(shim): relink dangling shims instead of failing the install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `refuse install` probed each shim with os.Stat, which follows the symlink and errors on a broken one. So a dangling shim — left after the target binary moved or a Homebrew cask was removed — was never cleared, and os.Symlink then failed the whole install with: Error: create shim .../bin/npm: symlink ...: file exists Worse, dangling shims silently disable protection: the shell skips broken symlinks, so `npm` falls through PATH to the real npm and nothing is checked. - Install: detect existing entries with os.Lstat (does not follow the link), skip only when a valid symlink already resolves to us, and remove any foreign / stale / dangling entry before relinking. - Uninstall: likewise use Lstat so a dangling shim pointing into our own bin dir is removed too, instead of being treated as already gone. - Add a regression test covering the dangling-shim case for install + uninstall. --- internal/shim/install.go | 52 +++++++++++++++++++-------- internal/shim/install_test.go | 67 +++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 14 deletions(-) create mode 100644 internal/shim/install_test.go diff --git a/internal/shim/install.go b/internal/shim/install.go index e8460e0..d64b818 100644 --- a/internal/shim/install.go +++ b/internal/shim/install.go @@ -67,21 +67,29 @@ func Install(shims []string) (InstallResult, error) { } target := shimTargetPath(dir, name) - // Idempotency: if target already points at us, nothing to do. - if cur, err := os.Stat(target); err == nil { - if self, sErr := os.Stat(selfExe); sErr == nil && os.SameFile(cur, self) { - res.Skipped = append(res.Skipped, name+": already installed") - // Still try the legacy cleanup so a re-run after upgrade tidies stragglers. - if cleanupLegacyBareName(dir, name, selfExe) { - res.LegacyRemoved = append(res.LegacyRemoved, name) + // Detect anything already at target with Lstat, which (unlike os.Stat) + // does NOT follow the link — so a dangling/broken shim is seen too. + // os.Stat errors on a broken shim, which used to skip cleanup and make + // createShim fail with "file exists" on the next install. + if lfi, lerr := os.Lstat(target); lerr == nil { + // A symlink that still resolves to us? Idempotent — leave it. + if lfi.Mode()&os.ModeSymlink != 0 { + if cur, sErr := os.Stat(target); sErr == nil { + if self, e := os.Stat(selfExe); e == nil && os.SameFile(cur, self) { + res.Skipped = append(res.Skipped, name+": already installed") + // Still try the legacy cleanup so a re-run after upgrade tidies stragglers. + if cleanupLegacyBareName(dir, name, selfExe) { + res.LegacyRemoved = append(res.LegacyRemoved, name) + } + continue + } } - continue } - // Foreign file at our spot — remove it before relink. - _ = os.Remove(target) + // Foreign file, wrong target, or dangling shim — clear it before relinking. + if rmErr := os.Remove(target); rmErr != nil { + return res, fmt.Errorf("remove stale shim %s: %w", target, rmErr) + } } - // Stale entry that isn't a file but isn't a known link either (e.g., broken symlink). - // os.Stat above fails for broken symlinks; nothing to do then. if cleanupLegacyBareName(dir, name, selfExe) { res.LegacyRemoved = append(res.LegacyRemoved, name) @@ -123,7 +131,10 @@ func Uninstall() (InstallResult, error) { } for name := range KnownManagers { target := shimTargetPath(dir, name) - info, err := os.Stat(target) + + // Lstat so a dangling shim is detected too — os.Stat reports ENOENT for a + // broken link, which used to make us treat an existing shim as already gone. + lfi, err := os.Lstat(target) if errors.Is(err, os.ErrNotExist) { // Even if the canonical target is gone, a legacy bare-name shim may linger. if cleanupLegacyBareName(dir, name, selfExe) { @@ -135,7 +146,20 @@ func Uninstall() (InstallResult, error) { res.Skipped = append(res.Skipped, name+": "+err.Error()) continue } - if !os.SameFile(info, selfInfo) { + // Ours if it resolves to us, or if it's a symlink (valid OR dangling) + // pointing at the refuse binary in our own bin dir. Leave foreign files alone. + ours := false + if cur, sErr := os.Stat(target); sErr == nil { + ours = os.SameFile(cur, selfInfo) + } else if lfi.Mode()&os.ModeSymlink != 0 { + if dest, rlErr := os.Readlink(target); rlErr == nil { + if !filepath.IsAbs(dest) { + dest = filepath.Join(dir, dest) + } + ours = filepath.Dir(filepath.Clean(dest)) == dir + } + } + if !ours { res.Skipped = append(res.Skipped, name+": not our shim") continue } diff --git a/internal/shim/install_test.go b/internal/shim/install_test.go new file mode 100644 index 0000000..a624cf7 --- /dev/null +++ b/internal/shim/install_test.go @@ -0,0 +1,67 @@ +package shim + +import ( + "os" + "path/filepath" + "testing" +) + +// A dangling shim (a symlink whose target no longer exists — e.g. left over +// after the refuse binary moved) must NOT block a re-install, and must be +// cleaned up by uninstall. Regression for: +// +// Error: create shim .../bin/npm: symlink ...: file exists +// +// which happened because the old code probed the shim with os.Stat (which +// follows the link and errors on a broken one), so the stale link was never +// cleared before os.Symlink. +func TestInstallReplacesDanglingShim(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("REFUSE_HOME", "") // force the HOME-derived ~/.refuse path + + binPath := filepath.Join(home, ".refuse", "bin") + if err := os.MkdirAll(binPath, 0o755); err != nil { + t.Fatal(err) + } + + self, err := os.Executable() + if err != nil { + t.Fatal(err) + } + if r, e := filepath.EvalSymlinks(self); e == nil { + self = r + } + + npm := filepath.Join(binPath, "npm") + // Pre-seed a DANGLING npm shim pointing at a refuse that no longer exists. + if err := os.Symlink(filepath.Join(binPath, "refuse-gone"), npm); err != nil { + t.Fatal(err) + } + + if _, err := Install([]string{"npm"}); err != nil { + t.Fatalf("Install over a dangling shim failed: %v", err) + } + + resolved, err := filepath.EvalSymlinks(npm) + if err != nil { + t.Fatalf("npm shim still dangling after install: %v", err) + } + if resolved != self { + t.Errorf("npm shim resolves to %s, want %s", resolved, self) + } + + // Uninstall must remove a dangling shim too (one that points into our bin dir). + if err := os.Remove(npm); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(binPath, "refuse"), npm); err != nil { // dangling, in our dir + t.Fatal(err) + } + if _, err := Uninstall(); err != nil { + t.Fatalf("Uninstall failed: %v", err) + } + if _, err := os.Lstat(npm); !os.IsNotExist(err) { + t.Errorf("uninstall left the dangling shim behind (lstat err = %v)", err) + } +} From 9ec31af40e7e0ad0e1cadf9ca671779ea9187a5d Mon Sep 17 00:00:00 2001 From: gok03 Date: Thu, 18 Jun 2026 15:32:36 +0530 Subject: [PATCH 2/2] test(shim): skip dangling-shim relink test on windows (unix-only symlinks) --- internal/shim/install_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/shim/install_test.go b/internal/shim/install_test.go index a624cf7..39e7ce3 100644 --- a/internal/shim/install_test.go +++ b/internal/shim/install_test.go @@ -3,6 +3,7 @@ package shim import ( "os" "path/filepath" + "runtime" "testing" ) @@ -16,6 +17,13 @@ import ( // follows the link and errors on a broken one), so the stale link was never // cleared before os.Symlink. func TestInstallReplacesDanglingShim(t *testing.T) { + // Shims are symlinks only on unix; on Windows createShim uses a + // hardlink/copy (and an extension), so the dangling-symlink scenario and + // these EvalSymlinks assertions don't apply. + if runtime.GOOS == "windows" { + t.Skip("shim relink test is unix-specific (windows shims are not symlinks)") + } + home := t.TempDir() t.Setenv("HOME", home) t.Setenv("REFUSE_HOME", "") // force the HOME-derived ~/.refuse path