From 1820bd51082390ac5f8378657b1a2959016a99ae Mon Sep 17 00:00:00 2001 From: Andrew Carter Date: Sat, 6 Jun 2026 13:27:39 -0700 Subject: [PATCH] fix(install): clean up stale symlinks and detect manifest spec drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related correctness fixes for the bin/man/completions install flow. 1. Reap stale symlinks before re-extracting. installFromAsset wiped the tool's ToolDir and recreated symlinks for everything in the current manifest entry — but never removed the previous install's symlinks first. If the user renamed a bin spec (e.g. bin = ["old"] → bin = ["new"]) or removed a man/ completion entry, the old symlinks in BinDir/ManDir/completion dirs remained, dangling into the now-deleted ToolDir. Fix: call m.removeToolSymlinks(name) at the top of installFromAsset, before os.RemoveAll(toolDir). The reaper only removes symlinks whose targets resolve into the tool's ToolDir, so it's safe to call when no prior install exists. 2. Detect manifest asset-spec drift in isUpToDate. isUpToDate compared only the release tag, so adding a new man/completion entry to an already-installed tool (or renaming a bin) at the same release tag was reported as "up to date" and the new symlinks were never created — the user had to know to pass --force. Fix: also compare state.Bin/Man/Completions against the manifest entry as unordered sets, and return false when any of them differ. Adds a small stringSlicesEqual helper. Both fixes are independent of any particular asset class — they apply equally to bin, man, and completions. Regression tests added for both behaviors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/install.go | 45 +++++++++++++--- cmd/install_test.go | 107 +++++++++++++++++++++++++++++++++++++ internal/tool/tool.go | 10 +++- internal/tool/tool_test.go | 58 ++++++++++++++++++++ 4 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 cmd/install_test.go diff --git a/cmd/install.go b/cmd/install.go index fb84f24..3aada30 100644 --- a/cmd/install.go +++ b/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 } diff --git a/cmd/install_test.go b/cmd/install_test.go new file mode 100644 index 0000000..40422d9 --- /dev/null +++ b/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) +} + diff --git a/internal/tool/tool.go b/internal/tool/tool.go index 68a4c46..e86eb2f 100644 --- a/internal/tool/tool.go +++ b/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) diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index 862f756..492a793 100644 --- a/internal/tool/tool_test.go +++ b/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{