fix(install): clean up stale symlinks and detect manifest spec drift

cmd/install.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"runtime"
+	"sort"
 
 	"github.com/spf13/cobra"
 
@@ -190,8 +191,12 @@ func runInstallReconcile(mgr *tool.Manager, cfg *config.Config) error {
 	return nil
 }
 
-// isUpToDate checks whether a tool is already installed at the target version.
-// Returns true (and prints a warning) if the installed version matches, false otherwise.
+// isUpToDate reports whether the installed copy already matches the
+// target version AND the manifest's asset spec. The spec check covers
+// the case where the user added or renamed a bin/man/completion entry
+// after the tool was already installed at the current release tag —
+// without it, `gh tool install` would print "up to date" and never
+// create the new symlinks.
 func isUpToDate(mgr *tool.Manager, t config.Tool) bool {
 	name := t.Name()
 	state := mgr.ReadState(name)
@@ -208,10 +213,38 @@ func isUpToDate(mgr *tool.Manager, t config.Tool) bool {
 		targetTag = latest
 	}
 
-	if state.Tag == targetTag {
-		fmt.Printf("%s %s up to date (%s)\n", ui.IconBullet, name, targetTag)
-		return true
+	if state.Tag != targetTag {
+		return false
+	}
+	if !stringSlicesEqual(state.Bin, t.Bin) ||
+		!stringSlicesEqual(state.Man, t.Man) ||
+		!stringSlicesEqual(state.Completions, t.Completions) {
+		return false
 	}
 
-	return false
+	fmt.Printf("%s %s up to date (%s)\n", ui.IconBullet, name, targetTag)
+	return true
+}
+
+// stringSlicesEqual compares two slices as unordered sets, treating
+// nil and empty as equivalent. Used by isUpToDate to detect manifest
+// asset-spec changes that require a reinstall regardless of whether
+// the release tag moved.
+func stringSlicesEqual(a, b []string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	if len(a) == 0 {
+		return true
+	}
+	ac := append([]string(nil), a...)
+	bc := append([]string(nil), b...)
+	sort.Strings(ac)
+	sort.Strings(bc)
+	for i := range ac {
+		if ac[i] != bc[i] {
+			return false
+		}
+	}
+	return true
 }

cmd/install_test.go

@@ -0,0 +1,107 @@
+package cmd
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/BurntSushi/toml"
+
+	"github.com/ascarter/gh-tool/internal/config"
+	"github.com/ascarter/gh-tool/internal/paths"
+	"github.com/ascarter/gh-tool/internal/tool"
+)
+
+func TestStringSlicesEqual(t *testing.T) {
+	cases := []struct {
+		a, b []string
+		want bool
+	}{
+		{nil, nil, true},
+		{nil, []string{}, true},
+		{[]string{"x"}, []string{"x"}, true},
+		{[]string{"a", "b"}, []string{"b", "a"}, true}, // unordered
+		{[]string{"x"}, []string{"y"}, false},
+		{[]string{"x", "y"}, []string{"x"}, false},
+	}
+	for _, c := range cases {
+		if got := stringSlicesEqual(c.a, c.b); got != c.want {
+			t.Errorf("stringSlicesEqual(%v, %v) = %v, want %v", c.a, c.b, got, c.want)
+		}
+	}
+}
+
+// Regression: when a tool is already installed at the manifest's target
+// tag but the manifest's bin/man/completions spec has changed (entries
+// added or removed), isUpToDate must return false so install runs and
+// actualizes the new spec. The previous implementation only compared
+// the tag and would report "up to date" forever.
+func TestIsUpToDateDetectsManifestSpecChange(t *testing.T) {
+	root := t.TempDir()
+	dirs := paths.Dirs{
+		Config: filepath.Join(root, "config"),
+		Data:   filepath.Join(root, "data"),
+		State:  filepath.Join(root, "state"),
+		Cache:  filepath.Join(root, "cache"),
+	}
+	if err := dirs.EnsureDirs(); err != nil {
+		t.Fatalf("EnsureDirs: %v", err)
+	}
+	mgr := tool.NewManager(dirs)
+
+	// Persist a state file as if the tool had been installed at tag v1
+	// with bin=[fzf] only.
+	state := tool.InstalledState{
+		Repo:        "junegunn/fzf",
+		Tag:         "v1",
+		Pattern:     "fzf-v1.tar.gz",
+		Bin:         []string{"fzf"},
+		InstalledAt: time.Now().UTC().Format(time.RFC3339),
+	}
+	if err := writeStateForTest(dirs, "fzf", state); err != nil {
+		t.Fatalf("write state: %v", err)
+	}
+
+	// Same tag, bin spec unchanged → up to date.
+	tmatch := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf"}}
+	if !isUpToDate(mgr, tmatch) {
+		t.Errorf("isUpToDate(matching spec)=false, want true")
+	}
+
+	// Same tag, manifest gained completions entry → must NOT be up to
+	// date so install runs and creates the new completion symlinks.
+	tadded := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf"}, Completions: []string{"shell/completion.bash"}}
+	if isUpToDate(mgr, tadded) {
+		t.Errorf("isUpToDate(spec gained completions)=true, want false")
+	}
+
+	// Same tag, bin renamed → must NOT be up to date.
+	trenamed := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf-bin:fzf"}}
+	if isUpToDate(mgr, trenamed) {
+		t.Errorf("isUpToDate(renamed bin)=true, want false")
+	}
+
+	// Different tag → false regardless of spec. We pin Tag to bypass
+	// the LatestTag network call.
+	tnewtag := config.Tool{Repo: "junegunn/fzf", Tag: "v2", Bin: []string{"fzf"}}
+	if isUpToDate(mgr, tnewtag) {
+		t.Errorf("isUpToDate(different tag)=true, want false")
+	}
+}
+
+// writeStateForTest seeds an InstalledState toml file at the path
+// Manager.ReadState looks at. Avoids exporting Manager.writeState.
+func writeStateForTest(dirs paths.Dirs, name string, state tool.InstalledState) error {
+	path := dirs.StateFile(name)
+	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+		return err
+	}
+	f, err := os.Create(path)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+	return toml.NewEncoder(f).Encode(state)
+}
+

internal/tool/tool.go

@@ -164,7 +164,15 @@ func (m *Manager) installFromAsset(t config.Tool, assetPath, tag, resolvedPatter
 		m.verifyAttestation(name, t.Repo, assetPath)
 	}
 
-	// Clean previous install
+	// Reap any prior install's symlinks before wiping the tool dir. The
+	// new install's createSymlinks below will only place links the
+	// current manifest entry calls for, so without this reap a renamed
+	// or removed bin/man/completion entry would leave a dangling
+	// symlink behind. Safe to call when there is no prior install:
+	// removeToolSymlinks only removes links whose targets resolve into
+	// the tool's ToolDir.
+	m.removeToolSymlinks(name)
+
 	toolDir := m.Dirs.ToolDir(name)
 	_ = os.RemoveAll(toolDir)
 

internal/tool/tool_test.go

@@ -289,6 +289,64 @@ func TestRemoveToolSymlinksRelativeTarget(t *testing.T) {
 	}
 }
 
+// Regression: installFromAsset must reap the tool's existing symlinks
+// before re-extracting, otherwise links from a previous install whose
+// bin/man/completion spec changed (rename or removal) dangle pointing
+// into the now-deleted ToolDir.
+func TestInstallFromAssetReapsStaleSymlinks(t *testing.T) {
+	root := t.TempDir()
+	dirs := paths.Dirs{
+		Config: filepath.Join(root, "config"),
+		Data:   filepath.Join(root, "data"),
+		State:  filepath.Join(root, "state"),
+		Cache:  filepath.Join(root, "cache"),
+	}
+	if err := dirs.EnsureDirs(); err != nil {
+		t.Fatalf("EnsureDirs: %v", err)
+	}
+	mgr := NewManager(dirs)
+
+	name := "mytool"
+	toolDir := dirs.ToolDir(name)
+	if err := os.MkdirAll(toolDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	// Simulate a prior install: a symlink in BinDir pointing at a file
+	// inside the (about-to-be-replaced) ToolDir. The new install's
+	// manifest will not reference this file.
+	staleSrc := filepath.Join(toolDir, "old-name")
+	if err := os.WriteFile(staleSrc, []byte{}, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	staleLink := filepath.Join(dirs.BinDir(), "old-name")
+	if err := os.Symlink(staleSrc, staleLink); err != nil {
+		t.Fatalf("symlink: %v", err)
+	}
+
+	// Stage a fake "downloaded asset" — a bare binary at a known path —
+	// and use InstallFromAsset (the public wrapper) with a Tool whose
+	// only bin is the new name.
+	asset := filepath.Join(root, "new-name")
+	if err := os.WriteFile(asset, []byte{}, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	tool := config.Tool{Repo: "ex/mytool", Pattern: "new-name", Bin: []string{"new-name"}}
+	if err := mgr.InstallFromAsset(tool, asset, "v1", false); err != nil {
+		t.Fatalf("InstallFromAsset: %v", err)
+	}
+
+	// The stale link must be gone (the bug was that it dangled).
+	if _, err := os.Lstat(staleLink); !os.IsNotExist(err) {
+		t.Errorf("expected stale symlink %s to be removed by reinstall; err=%v", staleLink, err)
+	}
+	// The new link must exist and resolve to the new binary in ToolDir.
+	newLink := filepath.Join(dirs.BinDir(), "new-name")
+	if _, err := os.Lstat(newLink); err != nil {
+		t.Errorf("new bin symlink missing: %v", err)
+	}
+}
+
 func TestInstalledStateRoundTrip(t *testing.T) {
 	root := t.TempDir()
 	dirs := paths.Dirs{