From 790c75aec7d4cd120bbef2195da765e0d7f500e6 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Mon, 27 Jul 2026 23:39:55 +0300 Subject: [PATCH 1/3] docs: add uv/bun/pnpm update-detection implementation plan Co-Authored-By: Claude Fable 5 --- docs/plans/20260727-uv-bun-pnpm-updaters.md | 277 ++++++++++++++++++++ 1 file changed, 277 insertions(+) create mode 100644 docs/plans/20260727-uv-bun-pnpm-updaters.md diff --git a/docs/plans/20260727-uv-bun-pnpm-updaters.md b/docs/plans/20260727-uv-bun-pnpm-updaters.md new file mode 100644 index 0000000..b20c274 --- /dev/null +++ b/docs/plans/20260727-uv-bun-pnpm-updaters.md @@ -0,0 +1,277 @@ +# uv / bun / pnpm update detection + +## Overview + +Add three package managers — **uv**, **bun**, **pnpm** — to the update-detection +chain in `internal/updater`, so `[u]` can update tools installed through them +(requested by users after the reddit announcement). Along the way this fixes a +real misdetection, empirically verified during plan review: + +- **bun** globals resolve through symlinks into + `$BUN_INSTALL/install/global/node_modules//…`, so today the npm step + claims them and offers `npm install -g ` — the wrong manager, which + would install a duplicate copy under npm's prefix (verified with bun 1.3.14). +- **pnpm** globals are **not** symlinks at all: `$PNPM_HOME/bin/` is a + cmd-shim **shell script** `EvalSymlinks` cannot resolve (verified with pnpm + 11.17.0), so today they land in `ErrUnknownManager`. The shim's last line is + machine-readable — `# cmd-shim-target=/cli.js>` + — which is what the pnpm branch parses. + +Scope is `internal/updater` only. Installed-version fallbacks in +`internal/version` (the brew-dir / cargo-list idea) are deliberately **not** +added for these managers: their tools are ordinary CLIs that answer +`--version`, and pipx lives without such a fallback too (YAGNI). + +## Context (from discovery) + +- Files involved: `internal/updater/updater.go`, `internal/updater/updater_test.go`; + docs: `CLAUDE.md` (chain at lines ~39 and ~95), `README.md` (manager list at + line ~43, detection bullets at ~140–144, "Updating tools" caveats at + ~146–161), `ARCHITECTURE.md` (chain verbatim at line ~199). +- Patterns to follow: pure detection core `detectFromPath` (table-tested, no + subprocesses) + OS-facing `Detect` wrapper that gathers signals + (`goBuildinfo`, and now the pnpm shim target); `segmentUnder` / `underDir` + path helpers; `autoPlan` constructor; the `cargoCrateFromList`/`cargoCrate` + split (pure parser + bounded OS read) for the shim parser; the + `launcher.planFor` idiom (injected `getenv`) for env-dependent pure cores. +- One production `detectFromPath` call site (updater.go:143) and one test call + site (the table runner at `updater_test.go:84`). +- Design agreed in a brainstorm session, then revised after a plan-review agent + empirically validated all three manager layouts (uv 0.11.32, bun 1.3.14, + pnpm 11.17.0) — the pnpm branch was redesigned around the cmd-shim target + and the misdetection premise reattributed from pnpm to bun. + +## Development Approach + +- **testing approach**: Regular (code + table tests in the same task, package convention) +- complete each task fully before moving to the next +- make small, focused changes +- **CRITICAL: every task MUST include new/updated tests** for code changes in that task + - tests are not optional - they are a required part of the checklist + - tests cover both success and error scenarios +- **CRITICAL: all tests must pass before starting next task** - no exceptions +- **CRITICAL: update this plan file when scope changes during implementation** +- run `go test -race ./internal/updater/` after each change; full `go test -race ./...` at the end +- backward compatibility: zero-value `managerDirs{}` + empty shim target + disable the three new checks; the only intentional behavior changes reachable + with zero-value inputs are the `npmPackage` service-segment fix and the npm + step's `.pnpm` gate (both Task 2, both deliberate) + +## Testing Strategy + +- **unit tests**: table-driven, in `internal/updater/updater_test.go`, no real + package managers required. Env resolution is tested through the injected + `getenv`, never by mutating the process environment. Exception: **one** + unix-gated `Detect`-level fixture test (the `TestDetectResolvesSymlink` + style, `updater_test.go:277`) pins the `Detect` → `resolveManagerDirs()` → + `detectFromPath` wiring via `t.Setenv` — that is wiring verification, not + env-resolution testing, and the package already does this for `PATH`. +- **e2e tests**: none in this project; a live smoke check with the locally + installed uv is part of acceptance (Task 6). + +## Progress Tracking + +- mark completed items with `[x]` immediately when done +- add newly discovered tasks with ➕ prefix +- document issues/blockers with ⚠️ prefix +- keep plan in sync with actual work done + +## Solution Overview + +Extend the pure chain with three checks. New order: + +**brew → go buildinfo → cargo → pipx → uv → pnpm → bun → npm** + +pnpm/bun sit **before** npm because their paths can contain `node_modules` — +same load-bearing-order principle as the documented "brew before go". uv sits +next to pipx (identical technique). Soft degradation everywhere: an +unresolvable manager dir or unreadable shim just disables its check, and an +exhausted chain still ends in `ErrUnknownManager` with the existing +`update_cmd` hint. + +Key design decisions: + +- **`managerDirs` struct, not new package seams**: `detectFromPath` gains + parameters for the new signals. The OS-facing `Detect` fills them via + `resolveManagerDirs()` = thin wrapper over the pure + `managerDirsFrom(getenv, home, goos)` (the `launcher.planFor` idiom). + Empty field = check off, **by explicit `!= ""` guards** — `underDir(p, "")` + is treacherously true for relative paths, so emptiness must be checked + before the path helpers, mirroring the existing `if home != ""` guards + (updater.go:268/277). +- **pnpm via the cmd-shim target**: the pure parser `pnpmShimTarget(contents)` + extracts the `# cmd-shim-target=` line; `Detect` does a bounded best-effort + read of the found binary **only when it sits under `pnpmHome`** and passes + the target into the core as a signal (exactly how `goBuildinfo` rides in). + The target path contains `node_modules/`, so the existing `npmPackage` + extracts the name. +- **`pnpm add -g` / `bun add -g`, not `update -g`**: `update` respects the + saved semver range and can silently refuse a major bump, while keepkit + promises to install the version shown as `latest:`; `add -g` mirrors the + npm branch's `npm install -g` semantics. +- **`uv tool upgrade `**: the canonical uv command. +- **The npm step refuses `.pnpm` store paths**: a path through + `/node_modules/.pnpm/` outside the detected pnpm dir means "pnpm layout we + failed to attribute" — offering `npm install -g ` there would silently + install a working duplicate under npm's prefix, so the chain falls through + to `ErrUnknownManager` + the `update_cmd` hint instead (honest degradation). +- **Manager self-updates are out of scope** (`bun upgrade`, + `pnpm self-update`): the managers' own binaries (no `node_modules` segment, + no shim target) fall through the chain. +- **Non-goal**: cargo/pipx keep resolving home inside the core via `homeDir()` + as today; `managerDirs` carries only the three new dirs. No refactor of the + existing steps. + +## Technical Details + +Directory resolution (`managerDirsFrom(getenv, home, goos)`): + +| Field | Env override | Default | +|---|---|---| +| `uvTools` | `$UV_TOOL_DIR` | `$XDG_DATA_HOME/uv/tools`, else `/.local/share/uv/tools` (macOS and Linux — uv uses XDG paths on macOS too; verified). **Windows: no default**, env-only (uv's Windows layout differs and is unverified — honest degradation) | +| `pnpmHome` | `$PNPM_HOME` | darwin: `/Library/pnpm`; linux: `$XDG_DATA_HOME/pnpm`, else `/.local/share/pnpm`; windows: `%LOCALAPPDATA%\pnpm` | +| `bunInstall` | `$BUN_INSTALL` | `/.bun` (all platforms) | + +Empty `home` disables home-based defaults (field stays empty). + +Signature: `detectFromPath(realPath, buildinfo, shimTarget string, dirs managerDirs)` +— `shimTarget` is the pnpm cmd-shim target (empty when none), gathered by +`Detect` alongside `buildinfo`. One signature change, done in Task 1; +`shimTarget` is consumed from Task 4. + +Detection steps (inside `detectFromPath`, after pipx, before npm; each behind +its explicit `dirs. != ""` guard): + +- **uv**: `segmentUnder(realPath, dirs.uvTools)` → `autoPlan("uv", ["uv", "tool", "upgrade", pkg])`. + Verified layout: `//bin/` behind a symlink in uv's bin dir. +- **pnpm**: `underDir(realPath, dirs.pnpmHome)` and a package name from + `npmPackage(shimTarget)` (shim layout, pnpm ≥ 9) **or** `npmPackage(realPath)` + (legacy symlink layouts) → `autoPlan("pnpm", ["pnpm", "add", "-g", pkg])`; + no name → fall through, no error (that's the manager's own binary). + Verified target shape: `/global/v11//node_modules//cli.js`. +- **bun**: `underDir(realPath, dirs.bunInstall)` and `npmPackage(realPath) != ""` + → `autoPlan("bun", ["bun", "add", "-g", pkg])`; bun's own binary + (`/bin/bun`) has no `node_modules` segment and falls through. + Verified layout: `$BUN_INSTALL/bin/` symlink → `../install/global/node_modules//cli.js`. + +Shim reading (`Detect` side): only when the found path sits under a non-empty +`pnpmHome`; read capped (e.g. 8 KiB — real shims are ~1.5 KiB); any +read/parse failure yields an empty signal, never an error. Pure parser +`pnpmShimTarget(contents string) string` returns the path after +`# cmd-shim-target=` (last matching line wins), `""` otherwise. + +`npmPackage` fix: when the segment after `node_modules` starts with `.` +(`.pnpm`, `.bin` — service dirs, never package names), skip it and keep +scanning for the next `node_modules` occurrence. Scoped packages +(`@scope/name`) keep working. Independently, the **npm step** refuses any +path containing `/node_modules/.pnpm/` (see Solution Overview). + +Known limitations (accepted, documented in README by Task 7): + +- Windows pnpm/bun/npm bins are `.cmd` shims — pre-existing npm limitation, + inherited unchanged (the unix shim parser reads `#!/bin/sh` scripts, which + is what pnpm writes on macOS/Linux). +- Detection is path-convention-based: a future manager layout change makes its + check miss silently and the chain degrades to `ErrUnknownManager` + hint. + +## What Goes Where + +- **Implementation Steps** (`[ ]` checkboxes): code, tests, docs — all in this repo +- **Post-Completion** (no checkboxes): reddit follow-up, out-of-scope notes + +## Implementation Steps + +### Task 1: managerDirs resolution and detectFromPath signature + +**Files:** +- Modify: `internal/updater/updater.go` +- Modify: `internal/updater/updater_test.go` + +- [ ] add `managerDirs` struct + pure `managerDirsFrom(getenv func(string) string, home, goos string) managerDirs` implementing the resolution table above +- [ ] add thin OS wrapper `resolveManagerDirs()` (`os.Getenv`, `homeDir()`, `runtime.GOOS`) and pass its result from `Detect` into `detectFromPath` +- [ ] change the signature to `detectFromPath(realPath, buildinfo, shimTarget string, dirs managerDirs)` (`shimTarget` documented, consumed from Task 4; `Detect` passes `""` until then); add `shimTarget string` + `dirs managerDirs` fields to the existing `TestDetectFromPath` table struct so Tasks 3–5 add **rows**, not new runners +- [ ] write table tests for `managerDirsFrom`: `UV_TOOL_DIR` set / only `XDG_DATA_HOME` / bare home; `PNPM_HOME` set/unset × darwin/linux/windows (incl. `LOCALAPPDATA`); `BUN_INSTALL` set/unset; empty home disables defaults; windows uv default absent — expectations built with `filepath.Join`, never literal `\` (the `configdir.baseFor` table-test precedent; the core runs on the host OS) +- [ ] run `go test -race ./internal/updater/` - must pass before task 2 + +### Task 2: npmPackage service segments and the npm-step .pnpm gate + +**Files:** +- Modify: `internal/updater/updater.go` +- Modify: `internal/updater/updater_test.go` + +- [ ] teach `npmPackage` to skip a post-`node_modules` segment starting with `.` and continue scanning for the next `node_modules` +- [ ] gate the npm step: a path containing `/node_modules/.pnpm/` is never claimed as npm — it falls through (ends in `ErrUnknownManager` + `update_cmd` hint unless an earlier step claimed it) +- [ ] write `npmPackage` tests: `.pnpm` virtual-store path yields the real package (not `.pnpm`); scoped package under the `.pnpm` store; `.bin`-only path yields `""`; plain and scoped npm paths unchanged +- [ ] write chain tests: `.pnpm` store path **outside** all manager dirs → `ErrUnknownManager` (not a silent `npm install -g` duplicate); plain `node_modules` path still yields npm +- [ ] run `go test -race ./internal/updater/` - must pass before task 3 + +### Task 3: uv detection step + +**Files:** +- Modify: `internal/updater/updater.go` +- Modify: `internal/updater/updater_test.go` + +- [ ] add the uv check after pipx, behind `dirs.uvTools != ""`: `segmentUnder(realPath, dirs.uvTools)` → `autoPlan("uv", []string{"uv", "tool", "upgrade", pkg})` +- [ ] extend the `Plan.Manager` doc comment with `"uv"`, listing managers in chain order: `"brew" | "go" | "cargo" | "pipx" | "uv" | "pnpm" | "bun" | "npm" | "custom"` (pnpm/bun land in Tasks 4–5) +- [ ] write test rows: uv tool path detected (argv `uv tool upgrade `); empty `uvTools` with a **relative** path stays undetected (pins the explicit guard — `segmentUnder(p, "")` matches relative paths without it) +- [ ] run `go test -race ./internal/updater/` - must pass before task 4 + +### Task 4: pnpm detection via the cmd-shim target + +**Files:** +- Modify: `internal/updater/updater.go` +- Modify: `internal/updater/updater_test.go` + +- [ ] add pure parser `pnpmShimTarget(contents string) string` (`# cmd-shim-target=` line → target path, `""` otherwise) +- [ ] add the bounded best-effort shim read in `Detect`, fired only when the found path sits under a non-empty `pnpmHome`; result rides into `detectFromPath` as `shimTarget` +- [ ] add the pnpm check before bun/npm, behind `dirs.pnpmHome != ""`: `underDir(realPath, dirs.pnpmHome)` + name from `npmPackage(shimTarget)` or `npmPackage(realPath)` → `autoPlan("pnpm", []string{"pnpm", "add", "-g", pkg})`; no name → fall through without error +- [ ] write parser tests: real shim contents (the verified pnpm 11 shape); contents without the marker; empty string; marker with scoped-package target +- [ ] write chain rows: shim-target path (`…/global/v11//node_modules//cli.js`) → `pnpm add -g ` (explicitly not npm); legacy symlink layout via `npmPackage(realPath)`; pnpm's own binary (no target, no `node_modules`) falls through to `ErrUnknownManager`; empty `pnpmHome` guard with a relative path +- [ ] run `go test -race ./internal/updater/` - must pass before task 5 + +### Task 5: bun detection step and the Detect wiring test + +**Files:** +- Modify: `internal/updater/updater.go` +- Modify: `internal/updater/updater_test.go` + +- [ ] add the bun check between pnpm and npm, behind `dirs.bunInstall != ""`: `underDir(realPath, dirs.bunInstall)` + `npmPackage(realPath) != ""` → `autoPlan("bun", []string{"bun", "add", "-g", pkg})` +- [ ] write chain rows: bun global under `/install/global/node_modules/` → `bun add -g `; scoped package under bun; ordering row (path under `bunInstall` containing `node_modules` resolves to bun, not npm); bun's own binary (`/bin/bun`) falls through to `ErrUnknownManager` +- [ ] write the unix-gated `Detect`-level wiring fixture test (`TestDetectResolvesSymlink` style): tmp `BUN_INSTALL` with `bin/` → `../install/global/node_modules//cli.js` symlink, `t.Setenv("BUN_INSTALL", …)` + `t.Setenv("PATH", …)`, assert `bun add -g ` — pins `Detect` → `resolveManagerDirs()` → `detectFromPath(…, dirs)` threading that pure-core rows cannot catch +- [ ] run `go test -race ./internal/updater/` - must pass before task 6 + +### Task 6: Verify acceptance criteria + +- [ ] verify all requirements from Overview are implemented (three managers detected, bun-as-npm misdetection fixed, pnpm shim path working, zero-value compatibility) +- [ ] run full test suite: `go test -race ./...` +- [ ] run the preflight skill (build / vet / race tests / golangci-lint — the CI matrix) +- [ ] live smoke check with the locally installed uv, no home pollution: `UV_TOOL_DIR=/tools UV_TOOL_BIN_DIR=/bin uv tool install cowsay`, then a **temporary env-gated test inside the package** (a scratchpad `go run` main cannot import an internal package) asserting `Detect` yields `uv tool upgrade cowsay`; remove the temporary test and the tmp dirs afterwards + +### Task 7: [Final] Update documentation + +**Files:** +- Modify: `CLAUDE.md` +- Modify: `README.md` +- Modify: `ARCHITECTURE.md` + +- [ ] update both CLAUDE.md chain mentions (package-table updater row ~line 39 and the `[u]` section ~line 95): full chain `brew → go → cargo → pipx → uv → pnpm → bun → npm`, the pnpm/bun-before-npm ordering rationale, the shim-target signal, the npm `.pnpm` gate +- [ ] update README.md: manager list (~line 43), detection bullets (~140–144) with uv/pnpm/bun entries and their commands, and the "Updating tools" caveats (~146–161) with the accepted limitations (Windows `.cmd` shims inherited from npm; manager self-updates out of scope) +- [ ] update ARCHITECTURE.md's verbatim chain (~line 199) +- [ ] run the docs-sync skill to catch remaining drift +- [ ] move this plan to `docs/plans/completed/` + +## Post-Completion + +*Items requiring manual intervention or external systems - no checkboxes, informational only* + +**Manual verification:** +- optionally verify pnpm/bun detection on a machine where they are installed + as daily drivers; the review agent validated the layouts empirically + (scratchpad fixtures, pnpm 11.17.0 / bun 1.3.14 / uv 0.11.32), but a live + confirmation from a pnpm/bun user (e.g. the reddit requesters) closes the loop +- pnpm versions older than the verified 11.x may use different store layouts; + the legacy `npmPackage(realPath)` branch and the `.pnpm` skip cover the known + ones, and anything else degrades to `ErrUnknownManager` + hint + +**External follow-up:** +- reply in the reddit thread once the release ships From 4243c2bd439cd850cd04e042310a04bf6122cce6 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Tue, 28 Jul 2026 00:36:48 +0300 Subject: [PATCH 2/3] feat: detect uv, pnpm and bun tool updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chain is now brew → go → cargo → pipx → uv → pnpm → bun → npm. pnpm and bun sit before npm because both keep their globals behind node_modules paths: a bun global resolved to `npm install -g `, which installs a duplicate under npm's prefix while the bun copy keeps shadowing it on PATH. uv goes next to pipx — the same technique one ecosystem over. uv reads //bin/ and yields `uv tool upgrade `; bun reads /install/global/node_modules/ and yields `bun add -g `. pnpm needs a second signal: its globals are not symlinks at all but cmd-shim shell scripts, so the package name comes from the `# cmd-shim-target=` line Detect reads off the binary, with the resolved path as the legacy-layout fallback. `add -g` rather than `update -g` for both, since update honours the range saved at install time and can refuse the major bump the card is offering. Two npm-side consequences of the pnpm layout: npmPackage skips a post-node_modules segment starting with a dot (.pnpm is the virtual store, .bin the shim dir) and keeps scanning, and the npm step refuses any path through /node_modules/.pnpm/, where offering `npm install -g` would be the duplicate install again — the chain degrades to ErrUnknownManager and the update_cmd hint instead. managerDirs carries all five path-convention roots, cargo and pipx included, and resolveManagerDirs expands symlinks in each. Detect matches these roots against a symlink-resolved binary path, so a relocated ~/.bun, a home on a secondary volume or /home under autofs missed silently, and bun and legacy pnpm globals then fell through to npm — the misdetection above, back again. Carrying cargo/pipx there also takes homeDir() out of detectFromPath, so its "no I/O, no environment" contract holds literally. The empty-root check moved into underDir/segmentUnder, where it belongs: filepath.Rel("", "bin/exa") succeeds, so a relative path reads as living under every disabled root, and the guard was six hand-copied copies of one convention. Shim reads reject a file over the 8 KiB cap instead of parsing it truncated. A cut inside the marker line shortens the path, and .../node_modules/typescript shortened to .../node_modules/types yields types, a real package on npm — a confidently wrong `pnpm add -g` where the rest of the chain degrades honestly. Co-Authored-By: Claude Opus 5 --- ARCHITECTURE.md | 39 +- CLAUDE.md | 4 +- README.md | 23 +- .../20260727-uv-bun-pnpm-updaters.md | 113 ++- internal/updater/updater.go | 299 ++++++- internal/updater/updater_test.go | 736 +++++++++++++++++- 6 files changed, 1146 insertions(+), 68 deletions(-) rename docs/plans/{ => completed}/20260727-uv-bun-pnpm-updaters.md (76%) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index fba8f6c..62a6908 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -196,11 +196,44 @@ Key invariants: ## Updating a tool (`u`) `updater.Detect` identifies the manager from the installed binary — the chain is -brew → go → cargo → pipx → npm (order matters: brew before go, so a brew-installed -Go binary is not misrouted to `go install`). `update_cmd` from `meta.yaml` always -wins and runs via `sh -c`. Detection spawns subprocesses, so it runs as a `tea.Cmd`, +brew → go → cargo → pipx → uv → pnpm → bun → npm. Order matters twice: brew before +go, so a brew-installed Go binary is not misrouted to `go install`, and pnpm/bun +before npm, because both layouts contain `node_modules` segments the npm step would +otherwise claim (a bun global really did resolve to `npm install -g `, which +installs a duplicate under npm's prefix). `update_cmd` from `meta.yaml` always wins +and runs via `sh -c`. Detection spawns subprocesses, so it runs as a `tea.Cmd`, never inside `Update()`. +Five steps are path-convention based (cargo, pipx, uv, pnpm, bun) and take their +roots from `managerDirsFrom(getenv, home, goos)` (pure core, `resolveManagerDirs()` +wrapper — the `launcher.planFor` idiom): `$UV_TOOL_DIR`, `$PNPM_HOME` and +`$BUN_INSTALL` with per-platform defaults, plus home-derived `~/.cargo/bin` and +`~/.local/pipx/venvs` (no `$CARGO_HOME`/`$PIPX_HOME` — unchanged behaviour, and a +separate question from path resolution). Carrying all five is what lets +`detectFromPath` stop calling `homeDir()`, so its "no I/O, no environment" contract +holds literally. An empty field switches that step off, which is what makes the zero +value backwards compatible. That check lives inside `underDir`/`segmentUnder`, which +answer "no match" for an empty dir, rather than in a `!= ""` guard repeated at every +step: `filepath.Rel("", "bin/exa")` succeeds, so a relative path would otherwise read +as living under every disabled root. + +The wrapper then expands symlinks in every root (`resolveDir`, keeping the raw path +when it does not resolve). This is load-bearing: `Detect` matches these roots against +a symlink-*resolved* binary path, so a root reached through a symlink — a relocated +`~/.bun`, a home on a secondary volume, `/home` under autofs — never matches. uv and +pnpm's shim/store layouts then merely lose the update offer, but bun and legacy pnpm +globals keep a plain `node_modules/` segment, so npm claims them and offers the +duplicate install this chain exists to prevent. + +pnpm needs a second signal: its +global bins are cmd-shim shell scripts, not symlinks, so `Detect` does a bounded +best-effort read of the binary (only under `$PNPM_HOME`) and passes the +`# cmd-shim-target=` path into the core exactly as `go version -m` output rides in. +A file over the 8 KiB cap is rejected whole rather than parsed truncated — a cut +inside the marker line shortens the path, and a shortened package name is a +confidently wrong update command rather than the honest degradation everything +else in the chain falls back to. + When the tool has no binary of its own on PATH, one fallback runs before `ErrUnknownManager`: a `Cellar/`/`Caskroom/` directory means brew owns the name, so the plan is `brew upgrade `. That is the `rust` case — the diff --git a/CLAUDE.md b/CLAUDE.md index b2054ce..9dc589e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -36,7 +36,7 @@ CI (`.github/workflows/ci.yml`) runs build / vet / `test -race` / golangci-lint | `internal/model` | Entire Bubble Tea model — all TUI state, key handling, and rendering | | `internal/proc` | `DetachTTY` — runs tool probe subprocesses without a controlling terminal (`Setsid` on unix, `DETACHED_PROCESS` on Windows); `KillGroup` — process-group SIGKILL (negative pid; plain `Process.Kill` on Windows) for the update streamer's timeout path | | `internal/ui` | Lip Gloss styles and `PlaceOverlay` helper | -| `internal/updater` | Detect the package manager that owns an installed binary and produce an update `Plan{Manager, Argv, Display}` (brew → go → cargo → pipx → npm chain; `update_cmd` override always wins; on a `LookPath` miss **or an exhausted chain**, a brew-by-name fallback before giving up — see the `[u]` section). Bottom of the import graph like `version`: no TUI knowledge, depends only on `loader` for `Tool`. Pure `detectFromPath`/`brewNamePlanAt` cores + OS-facing `Detect`/`brewNamePlan` wrappers; `testHomeDir` and `testBrewPrefix` seams (the latter a deliberate duplicate of `version`'s — two bottom leaves that may not import each other) | +| `internal/updater` | Detect the package manager that owns an installed binary and produce an update `Plan{Manager, Argv, Display}` (brew → go → cargo → pipx → uv → pnpm → bun → npm chain; `update_cmd` override always wins; on a `LookPath` miss **or an exhausted chain**, a brew-by-name fallback before giving up — see the `[u]` section). Bottom of the import graph like `version`: no TUI knowledge, depends only on `loader` for `Tool`. Pure `detectFromPath`/`brewNamePlanAt`/`managerDirsFrom`/`pnpmShimTarget` cores + OS-facing `Detect`/`brewNamePlan`/`resolveManagerDirs`/`readPnpmShim` wrappers; `testHomeDir` and `testBrewPrefix` seams (the latter a deliberate duplicate of `version`'s — two bottom leaves that may not import each other) | | `internal/version` | Detect installed version locally — `InstalledVersion(t) (ver string, present bool)`, the two results independent so the card can tell "installed but won't say its version" from "not installed". Sources in order: `--version`/`-V`, then `brewDirVersion` in `brew.go` (reads the version from the `Caskroom/`/`Cellar/` directory names — no brew subprocess — so casks with no version CLI still resolve), then `cargoListVersion`/`cargoVersionFromList` (same idea one ecosystem over: `cargo install --list` names every cargo-installed crate's version without running its binary; gated on the binary existing, and `LookPath("cargo")` short-circuits before any subprocess). A fallback hit suppresses the anomaly log; `testBrewPrefix` seam. Also: fetch latest release, repo card, changelog and README from the GitHub API with a 24h cache; semver comparison (`IsNewer`); keepkit's own self-check (`selfcheck.go`: `SelfRepo`, `SelfLatest`) | The `model` package is split by responsibility (one package, several files): @@ -92,7 +92,7 @@ The model is a three-panel layout with focus cycling via `→/←` between `focu - **Card versions**: `renderCard`'s `[info]` section shows `installed:` from `m.versions[name].Installed` — all four states (`installed: \uf412 `, `installed: detecting…`, `installed: ✓ no version`, `installed: ✕ not installed`) in the section's muted `InfoStyle`. The two version-less states split what `InstalledKnown` alone used to collapse, on `InstalledPresent`: a tool that is installed but won't name its version (a ratatui app that ignores `--version`) is a working install and reads `✓` (U+2713) in `ui.OkStyle` — green, deliberately not `UpdateAvailableStyle`, whose orange means "act on this" — while a genuine absence keeps the `✕` (U+2715) in `DangerStyle`, the same red glyph/style pair the `[L]` overlay uses for exhaustion. The section's gate includes `installed != ""`, so a locally installed tool with no `GitHub` ref still renders it. When `hasUpdate(name)` (installed older than latest per `version.IsNewer`) the `latest:` value renders in `UpdateAvailableStyle` with a ` ↑` suffix — the same glyph the tools list appends to the row. The `latest:` line is gated on `card.Latest != ""` (a repo with no release shows no line) and appends the release date as a ` (YYYY-MM-DD)` suffix derived from `card.PublishedAt`. **Both version lines carry the `\uf412` glyph** (nf-oct-tag) before the version — its meaning is "a version", not specifically "a release tag", which is why it widened from `latest:` to `installed:`; the three states with no version to tag (`detecting…`, `✓ no version`, `✕ not installed`) stay bare. Written as a `\u` escape in source and prose alike: the raw glyph is invisible in most editors and diffs and gets silently lost. Versions live in the card only; the 30-column list never shows version strings. - **Panel titles**: all three panels inset a title into their top border (`┌─ [1] Tools ─…─┐`) via the shared `insetPanelTitle` (render.go) — an ANSI-safe splice over the already-rendered frame (`ui`'s `truncateVisible` is unexported; the helper remeasures with `stripANSI` and rebuilds the top line), colored like the border (focus-aware) and dropped whole when the panel is too narrow. The titles are `[1] Tools`, `[2] Brief` and `[3] Readme` / `[3] Help` / `[3] Man` (a `switch m.helpMode`, overridden by `[3] Update` while a live log shows) — they double as the documentation for the digit focus hotkeys, so the status bar carries no digit hints. All title characters are single-width and non-East-Asian-Ambiguous, keeping the border width math stable. - **Refresh (`r` in `focusBrief`)**: `refreshSelectedCmd(t)` force-refreshes the selected tool bypassing the 24h cache TTL — the repo pass (`refreshRemoteCmd` → `version.RefreshRepoData`) + changelog (`refreshChangelogCmd` → `version.RefreshChangelog`) + README (`refreshReadmeCmd` → `version.RefreshReadme`, preceded by a `delete(m.readmeData, name)` so a session-cached 404/rate-limit negative can recover, then a `markReadmeLoading(name)` — the deletion makes `needsReadme` true again for the whole in-flight window, so without the marker leaving and re-entering the tool would spend a second request; `refreshingFor` does *not* cover it, since `remoteMsg` clears that flag as soon as the repo pass lands, which can be well before the README does) + a local installed re-detect (`fetchInstalledCmd`). It emits the same `remoteMsg`/`changelogMsg` as the startup path, so the merge/re-render logic is reused. While the repo pass is in flight `m.refreshingFor` (the tool name) turns the card title into a status line — `refreshing data ` (`bubbles/spinner`, `MiniDot`; the about is hidden) — with no status-bar takeover; the `remoteMsg` handler clears `refreshingFor` on completion, which reverts the title to name+about and halts the `spinner.TickMsg` loop. `refreshingFor` doubles as the double-press guard; a tool with no `GitHub` only re-detects the installed version (`m.statusMsg = "no repo to refresh"`, no spinner). Note the same `case "r"` branches on focus three ways: rename in `focusTools`, refresh in `focusBrief`, README mode in `focusHelp`. -- **Update (`u` in `focusBrief`)**: installs a newer release from inside the TUI. `[u]` fires only in `focusBrief` (in `focusTools` `u` stays untrack — same `case "u"`, branched on focus); it requires `hasUpdate(name)` (else `statusMsg`) and reports the shared `updateBusyStatus` (`another update is running`) while `updatingFor != ""` instead of starting a second one (one update at a time, no queue; the same wording `[U]` uses, since the running update's only other sign is a card spinner invisible unless that tool is selected). The key fires `detectUpdateCmd(t, false)` — detection spawns subprocesses (`go version -m`, `cargo install --list`) so it must never run inside `Update()`, same as every other probe. `updater.Detect` runs the brew → go → cargo → pipx → npm chain (order matters: brew before go, so a brew-installed Go binary with buildinfo isn't misrouted); a `update_cmd` in `meta.yaml` always wins and runs via `sh -c` (skips detection entirely). The chain starts from `exec.LookPath(t.Name)`, and a **miss there is not the end**: the tracked name can still be a brew formula whose binaries are named differently — `rust` ships `rustc`/`cargo`, so `LookPath("rust")` misses while `brew upgrade rust` is exactly right — so `brewNamePlan` checks for a `Cellar/`/`Caskroom/` directory before `ErrUnknownManager` is returned. **An exhausted chain takes the same fallback**: a binary can be found and still belong to no known manager — a Homebrew cask app keeps its launcher on `PATH` while the executable lives inside the `.app` bundle (`agterm`), matching neither the Cellar regex nor any later step — so `Detect` retries `brewNamePlan` on `ErrUnknownManager` from `detectFromPath` too (`TestDetectBrewByNameChainExhausted`). Both sites are self-validating (they fire only when such a keg actually exists) and share `version.brewDirVersion`'s traversal guard: a name carrying `/` or `\` can't be a formula name and must not turn the `Join` into a traversal. The `updateDetectedMsg` handler drops a stale result through the shared predicate **`acceptsUpdateDetect(msg)`**: both paths refuse while `updatingFor != ""` and while `m.mode != modeNormal` (detection spawns subprocesses and can answer seconds later — a confirm dialog opening under an editor, a search or an overlay steals the keystroke aimed at it, the mirror of `launchDoneMsg`'s mode gate), and beyond that a *tool* result must still match the selection while keepkit's own has no selection to match. It maps `ErrUnknownManager` to a `statusMsg` hint (`no known updater for — set update_cmd or [o] releases` — no dead-end dialog; the wording branches on **`isSelfUpdate(msg.tool)`**, not on `msg.self`, so the identical failure of the identical binary reads the same from `[u]` on a tracked `keepkit` row as from `[U]` — `msg.self` keeps the one meaning only it has, "no selection to match", inside `acceptsUpdateDetect`), and on success stores `m.updatePlan` plus **`m.updateTarget = msg.tool`** and enters `modeConfirmUpdate`. The target is resolved *there*, not when enter is pressed: a selection that moved while detection ran can no longer retarget the dialog, and keepkit's own update — which has no row, and no selection at all with an empty tracker — needs no second identity. The confirm status bar shows `update : [enter] run [esc] cancel`; `enter` sets `m.updatingFor`/`m.updateLogFor` to that target, resets the log, and fires `startUpdateCmd` (any other key cancels, not just `esc`, and clears `updateTarget` with the plan it named). The `[u] update` hint is prepended to the `focusBrief` bar only when `hasUpdate(selected)`. +- **Update (`u` in `focusBrief`)**: installs a newer release from inside the TUI. `[u]` fires only in `focusBrief` (in `focusTools` `u` stays untrack — same `case "u"`, branched on focus); it requires `hasUpdate(name)` (else `statusMsg`) and reports the shared `updateBusyStatus` (`another update is running`) while `updatingFor != ""` instead of starting a second one (one update at a time, no queue; the same wording `[U]` uses, since the running update's only other sign is a card spinner invisible unless that tool is selected). The key fires `detectUpdateCmd(t, false)` — detection spawns subprocesses (`go version -m`, `cargo install --list`) so it must never run inside `Update()`, same as every other probe. `updater.Detect` runs the brew → go → cargo → pipx → uv → pnpm → bun → npm chain. **Order is load-bearing twice**: brew before go, so a brew-installed Go binary with buildinfo isn't misrouted, and pnpm/bun before npm, because both layouts carry `node_modules` segments the npm step claims on sight — a bun global (`$BUN_INSTALL/bin/` → `install/global/node_modules//…`) really did resolve to `npm install -g `, the wrong manager, installing a duplicate under npm's prefix that the bun copy keeps shadowing on `PATH`. Five steps are **path-convention** based (cargo, pipx, uv, pnpm, bun), and an empty root switches its own step off — enforced **inside `underDir`/`segmentUnder`**, which answer "no match" for an empty dir, deliberately *not* by a `!= ""` guard repeated at each step. The hazard is real: `filepath.Rel("", "bin/exa")` succeeds and yields `bin/exa`, so a **relative** path reads as living under every disabled root, and one forgotten copy of the guard silently claims it. It lived as six hand-copied copies of that one convention until the review that turned it into one definition — the same lesson as `version.applyReleaseOutcome` (`TestPathHelpersRejectEmptyDir`, plus a per-step `empty leaves a relative path undetected` row): `uv` = `segmentUnder(realPath, uvTools)` → `uv tool upgrade `; `pnpm` = `underDir(realPath, pnpmHome)` plus a name from the shim target or the resolved path → `pnpm add -g `; `bun` = `underDir(realPath, bunInstall)` + `npmPackage(realPath)` → `bun add -g `. **`add -g`, not `update -g`**: `update` honours the semver range saved at install time and can silently refuse a major bump, while keepkit promises the version the card shows as `latest:`. A manager's own binary carries no `node_modules` segment and no shim target, so it falls through — `bun upgrade`/`pnpm self-update` are deliberately out of scope. **All five roots** come from **`managerDirsFrom(getenv, home, goos)`** (pure core, `resolveManagerDirs()` wrapper — the `launcher.planFor` idiom), and carrying cargo/pipx there too is what lets `detectFromPath` stop calling `homeDir()`, making its "no I/O, no environment" contract literally true instead of nearly true. cargo and pipx stay home-derived with **no env var of their own** — `$CARGO_HOME`/`$PIPX_HOME` are not consulted, exactly as before the struct existed, since honouring them changes behaviour for anyone who sets them and belongs in its own commit. `/.cargo/bin` and `/.local/pipx/venvs`; `$UV_TOOL_DIR` else `$XDG_DATA_HOME/uv/tools` else `~/.local/share/uv/tools` (macOS included — uv is XDG there too; **Windows env-only**, its layout is unverified and a wrong guess beats no guess only in the wrong direction), `$PNPM_HOME` else `~/Library/pnpm` (darwin) / `$XDG_DATA_HOME/pnpm` else `~/.local/share/pnpm` (linux) / `%LOCALAPPDATA%\pnpm` (windows), `$BUN_INSTALL` else `~/.bun`. An empty field is a *disabled* check, which is what keeps the zero-value `managerDirs{}` backwards compatible. The wrapper then **expands symlinks in every one of the five roots** (`resolveDir` → `filepath.EvalSymlinks`, falling back to the raw path when it does not resolve — a root that does not exist yet is the normal state for an uninstalled manager and must stay a harmless *non-match* rather than becoming an empty, i.e. disabled, field). That expansion is load-bearing, not hygiene: `Detect` compares these roots against an `EvalSymlinks`-**resolved** binary path, so a root carrying any symlink component — a relocated `~/.bun` → `/mnt/big/bun`, a home on a secondary volume, `/home` under autofs, and on macOS `/var` → `/private/var`, which is what made every fixture test here need `EvalSymlinks(t.TempDir())` — silently fails to match. For uv and pnpm's shim/store layouts that only costs the update offer, but a bun global and a legacy pnpm one still carry a plain `node_modules/` segment, so the npm step claims them and offers `npm install -g ` — the exact duplicate-install misdetection this chain exists to prevent, measured on all five layouts and pinned by `TestDetectSymlinkedManagerRoot`. The expansion is a **loop over pointers to all five fields**, so a sixth field added to `managerDirs` but forgotten there would resolve to a root nothing can ever match; `TestResolveManagerDirsExpandsSymlinks` asserts all five at once as the guard. **pnpm needs a second signal** because its globals are not symlinks at all: `$PNPM_HOME/bin/` is a cmd-shim `/bin/sh` script `EvalSymlinks` resolves to itself, whose last line — `# cmd-shim-target=` — is the only machine-readable link to the owning package. `Detect` reads it (best-effort, capped at `pnpmShimMaxBytes` = 8 KiB against real shims of ~1.5 KiB, and **only** when the found path sits under a non-empty `pnpmHome` — reading every binary on `PATH` to look for a comment would be an open per detection) through `readPnpmShim` → the pure `pnpmShimTarget` (last marker wins: cmd-shim writes it as the final line). A file **over** the cap is rejected whole (`LimitReader(cap+1)` + a length check, `getReadme`'s idiom) rather than parsed truncated, and that is a correctness guard, not tidiness: a cut landing inside the marker line hands the parser a *shortened* path, and `…/node_modules/typescript/…` shortened to `…/node_modules/types` yields `types` — a real package on npm, so the chain would offer a confidently wrong `pnpm add -g types` where everything else here degrades honestly (`TestReadPnpmShim`'s oversized row, whose fixture asserts the truncated parse *would* have said `types`), and passes it into the core as `shimTarget` exactly the way `goBuildinfo` rides in; an unreadable file yields `""`, i.e. the check simply has no signal. Two npm-side consequences of the pnpm layout: `npmPackage` **skips a post-`node_modules` segment starting with `.`** (`.pnpm` is the virtual store, `.bin` the shim dir — never package names) and keeps scanning, and the **npm step refuses any path through `/node_modules/.pnpm/`** — such a path reaching npm means a pnpm layout the pnpm step failed to attribute, and `npm install -g ` there is the duplicate-install failure again, so the chain falls through to `ErrUnknownManager` + the `update_cmd` hint (honest degradation). Detection is convention-based end to end: a future layout change makes its check miss *silently* and degrade to the hint, never to a wrong command. A `update_cmd` in `meta.yaml` always wins and runs via `sh -c` (skips detection entirely). The chain starts from `exec.LookPath(t.Name)`, and a **miss there is not the end**: the tracked name can still be a brew formula whose binaries are named differently — `rust` ships `rustc`/`cargo`, so `LookPath("rust")` misses while `brew upgrade rust` is exactly right — so `brewNamePlan` checks for a `Cellar/`/`Caskroom/` directory before `ErrUnknownManager` is returned. **An exhausted chain takes the same fallback**: a binary can be found and still belong to no known manager — a Homebrew cask app keeps its launcher on `PATH` while the executable lives inside the `.app` bundle (`agterm`), matching neither the Cellar regex nor any later step — so `Detect` retries `brewNamePlan` on `ErrUnknownManager` from `detectFromPath` too (`TestDetectBrewByNameChainExhausted`). Both sites are self-validating (they fire only when such a keg actually exists) and share `version.brewDirVersion`'s traversal guard: a name carrying `/` or `\` can't be a formula name and must not turn the `Join` into a traversal. The `updateDetectedMsg` handler drops a stale result through the shared predicate **`acceptsUpdateDetect(msg)`**: both paths refuse while `updatingFor != ""` and while `m.mode != modeNormal` (detection spawns subprocesses and can answer seconds later — a confirm dialog opening under an editor, a search or an overlay steals the keystroke aimed at it, the mirror of `launchDoneMsg`'s mode gate), and beyond that a *tool* result must still match the selection while keepkit's own has no selection to match. It maps `ErrUnknownManager` to a `statusMsg` hint (`no known updater for — set update_cmd or [o] releases` — no dead-end dialog; the wording branches on **`isSelfUpdate(msg.tool)`**, not on `msg.self`, so the identical failure of the identical binary reads the same from `[u]` on a tracked `keepkit` row as from `[U]` — `msg.self` keeps the one meaning only it has, "no selection to match", inside `acceptsUpdateDetect`), and on success stores `m.updatePlan` plus **`m.updateTarget = msg.tool`** and enters `modeConfirmUpdate`. The target is resolved *there*, not when enter is pressed: a selection that moved while detection ran can no longer retarget the dialog, and keepkit's own update — which has no row, and no selection at all with an empty tracker — needs no second identity. The confirm status bar shows `update : [enter] run [esc] cancel`; `enter` sets `m.updatingFor`/`m.updateLogFor` to that target, resets the log, and fires `startUpdateCmd` (any other key cancels, not just `esc`, and clears `updateTarget` with the plan it named). The `[u] update` hint is prepended to the `focusBrief` bar only when `hasUpdate(selected)`. - **Streaming** (channel + re-subscribe idiom, no `*tea.Program`): `startUpdateCmd` runs the plan via `exec.Command` + `proc.DetachTTY` (10-min deadline; a sudo prompt fails fast instead of hanging — deliberate), with stdout+stderr merged into one pipe. **Reader ordering is load-bearing** (os/exec forbids `Wait` before pipe reads finish): the goroutine scans the pipe to EOF via `streamLines` → then `cmd.Wait()` → sends the exit error as a final `updateLine{done:true, err}` → then `close(ch)`. `waitForChunkCmd` does one receive → `updateChunkMsg`; a done item or closed channel → `updateDoneMsg`. The channel carries a typed `updateLine{text, replace, done, err}` (not `chan string`) so the `replace` flag and completion error ride the same channel — no second error channel threaded through every re-subscribe. Each segment is sanitized through `cleanTerminalOutput` (which already strips ANSI) at the boundary; `streamLines` splits on `\n` **and** `\r`, and a `\r` segment sets `replace` so brew/npm progress bars collapse to one updating line. `m.updateLog` is capped at ~500 lines (tail matters). On deadline, `proc.KillGroup` SIGKILLs the process group (negative pid — `DetachTTY`'s `Setsid` makes the child a session leader, so a plain kill would orphan `sh -c` grandchildren). - **Live log in `[3]`**: `m.updateLog []string` is a single active-session buffer (not a map); `m.updateLogFor` names its tool. Every site that asks "does the log own panel `[3]`?" asks the **single predicate `showsUpdateLog()`** (see **Self-update**), never `updateLogFor` directly: `renderHelpContent()` returns the log **ahead of** the `helpLoadingFor`/cache branches, and `autoFetchCmdsForSelected` skips the help fetch (and `helpLoadingFor` set) — otherwise re-selecting the tool paints `Loading...` or a late `helpOutputMsg` clobbers the live log. The panel title reads `[3] Update` while the log shows (same `insetPanelTitle` path as `[3] Help`/`[3] Man`), autoscrolls to bottom on each chunk, and the buffer persists after completion until the next update. For a *tool* update the claim is per tool: navigating away shows the other tool's normal help; back shows the live log. - **Spinner + completion**: `m.updatingFor` twins `refreshingFor` — card title `updating `; the `spinner.TickMsg` gate is `refreshingFor != "" || updatingFor != ""` (or the spinner freezes after one frame). The `updateDoneMsg` handler clears `updatingFor`; success → `statusMsg "updated "` + `fetchInstalledCmd(t)` (the version merge extinguishes `↑` and the existing by-name cursor remap moves the tool out of the update group); failure → `statusMsg "update failed — see [3]"` + **`recordUpdateFailure(msg)`**, the helper shared with the self path that seeds the log with the exit error when the command produced no output at all (otherwise `[3]` would still read `starting update…` while the status bar points there) and writes the `logx.Errorf` line (manager, exit code, last log lines — never the token). One definition, so the two paths cannot drift in log format. A tool untracked mid-update just clears `updatingFor` (no re-fetch, no crash). diff --git a/README.md b/README.md index 207d754..09bf64c 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,8 @@ Pure TUI, no subcommands; the only flags are `--version` and `--help`. and cargo fallbacks for tools that won't answer `--version`), the latest release comes from GitHub; outdated tools are marked `↑` and gathered at the top of the list - **Update from inside the TUI** — one key detects the package manager - (brew / go / cargo / pipx / npm, or `update_cmd` from `meta.yaml`), asks for + (brew / go / cargo / pipx / uv / pnpm / bun / npm, or `update_cmd` from + `meta.yaml`), asks for confirmation and streams the command output into panel `[3]` in real time - **Self-update** — when a newer keepkit release exists, the status bar offers it; after the update one more key restarts keepkit in place, in the same terminal tab @@ -141,8 +142,20 @@ on the tool card. keepkit detects the package manager the binary was installed w - `go` — buildinfo (`go version -m`) with a `path` field → `go install @latest`; - `cargo` — a binary in `~/.cargo/bin` → `cargo install `; - `pipx` — a venv in `~/.local/pipx/venvs//` → `pipx upgrade `; +- `uv` — a tool under `$UV_TOOL_DIR` (default `~/.local/share/uv/tools//`) → + `uv tool upgrade `; +- `pnpm` — a global under `$PNPM_HOME` (default `~/Library/pnpm` on macOS, + `~/.local/share/pnpm` on Linux) → `pnpm add -g `; +- `bun` — a global under `$BUN_INSTALL` (default `~/.bun`) → `bun add -g `; - `npm` — a global `node_modules/` → `npm install -g `. +pnpm and bun are checked before npm on purpose: both keep their globals behind +`node_modules` paths, so npm would otherwise claim them and offer +`npm install -g ` — a duplicate copy under npm's prefix while the original keeps +shadowing it on `PATH`. `add -g` rather than `update -g` for both, because `update` +respects the range recorded at install time and can quietly refuse the major bump the +card is offering. + If the binary cannot be attributed to any of those — or the tool has no binary of its own on `PATH` at all — one more check runs before giving up: a Homebrew keg or cask named after the tool → `brew upgrade `. That covers formulae whose @@ -150,6 +163,14 @@ binaries are named differently (`rust` installs `rustc` and `cargo`, so there is `rust` binary to detect from) and cask apps whose executable lives inside the `.app` bundle, where the path itself names no manager. +Two limits are worth knowing. Detection for uv, pnpm, bun and npm reads path +conventions, so on **Windows** their bins are `.cmd` shims keepkit does not parse — +a pre-existing npm limitation the three new managers inherit; on macOS and Linux +pnpm's shims are `#!/bin/sh` scripts, which keepkit does read. And a manager's **own** +binary is out of scope: `bun upgrade`, `pnpm self-update` and friends are not offered, +those tools update themselves. Anything a check misses degrades the same way as an +unknown manager — the `update_cmd` hint below, never a wrong command. + The command is shown in the status bar for confirmation; its output streams into panel `[3] Update` in real time and the TUI stays responsive. After a successful update the version is re-detected and the `↑` marker disappears. One update runs at diff --git a/docs/plans/20260727-uv-bun-pnpm-updaters.md b/docs/plans/completed/20260727-uv-bun-pnpm-updaters.md similarity index 76% rename from docs/plans/20260727-uv-bun-pnpm-updaters.md rename to docs/plans/completed/20260727-uv-bun-pnpm-updaters.md index b20c274..7fad0e1 100644 --- a/docs/plans/20260727-uv-bun-pnpm-updaters.md +++ b/docs/plans/completed/20260727-uv-bun-pnpm-updaters.md @@ -187,11 +187,11 @@ Known limitations (accepted, documented in README by Task 7): - Modify: `internal/updater/updater.go` - Modify: `internal/updater/updater_test.go` -- [ ] add `managerDirs` struct + pure `managerDirsFrom(getenv func(string) string, home, goos string) managerDirs` implementing the resolution table above -- [ ] add thin OS wrapper `resolveManagerDirs()` (`os.Getenv`, `homeDir()`, `runtime.GOOS`) and pass its result from `Detect` into `detectFromPath` -- [ ] change the signature to `detectFromPath(realPath, buildinfo, shimTarget string, dirs managerDirs)` (`shimTarget` documented, consumed from Task 4; `Detect` passes `""` until then); add `shimTarget string` + `dirs managerDirs` fields to the existing `TestDetectFromPath` table struct so Tasks 3–5 add **rows**, not new runners -- [ ] write table tests for `managerDirsFrom`: `UV_TOOL_DIR` set / only `XDG_DATA_HOME` / bare home; `PNPM_HOME` set/unset × darwin/linux/windows (incl. `LOCALAPPDATA`); `BUN_INSTALL` set/unset; empty home disables defaults; windows uv default absent — expectations built with `filepath.Join`, never literal `\` (the `configdir.baseFor` table-test precedent; the core runs on the host OS) -- [ ] run `go test -race ./internal/updater/` - must pass before task 2 +- [x] add `managerDirs` struct + pure `managerDirsFrom(getenv func(string) string, home, goos string) managerDirs` implementing the resolution table above +- [x] add thin OS wrapper `resolveManagerDirs()` (`os.Getenv`, `homeDir()`, `runtime.GOOS`) and pass its result from `Detect` into `detectFromPath` +- [x] change the signature to `detectFromPath(realPath, buildinfo, shimTarget string, dirs managerDirs)` (`shimTarget` documented, consumed from Task 4; `Detect` passes `""` until then); add `shimTarget string` + `dirs managerDirs` fields to the existing `TestDetectFromPath` table struct so Tasks 3–5 add **rows**, not new runners +- [x] write table tests for `managerDirsFrom`: `UV_TOOL_DIR` set / only `XDG_DATA_HOME` / bare home; `PNPM_HOME` set/unset × darwin/linux/windows (incl. `LOCALAPPDATA`); `BUN_INSTALL` set/unset; empty home disables defaults; windows uv default absent — expectations built with `filepath.Join`, never literal `\` (the `configdir.baseFor` table-test precedent; the core runs on the host OS) +- [x] run `go test -race ./internal/updater/` - must pass before task 2 ### Task 2: npmPackage service segments and the npm-step .pnpm gate @@ -199,11 +199,11 @@ Known limitations (accepted, documented in README by Task 7): - Modify: `internal/updater/updater.go` - Modify: `internal/updater/updater_test.go` -- [ ] teach `npmPackage` to skip a post-`node_modules` segment starting with `.` and continue scanning for the next `node_modules` -- [ ] gate the npm step: a path containing `/node_modules/.pnpm/` is never claimed as npm — it falls through (ends in `ErrUnknownManager` + `update_cmd` hint unless an earlier step claimed it) -- [ ] write `npmPackage` tests: `.pnpm` virtual-store path yields the real package (not `.pnpm`); scoped package under the `.pnpm` store; `.bin`-only path yields `""`; plain and scoped npm paths unchanged -- [ ] write chain tests: `.pnpm` store path **outside** all manager dirs → `ErrUnknownManager` (not a silent `npm install -g` duplicate); plain `node_modules` path still yields npm -- [ ] run `go test -race ./internal/updater/` - must pass before task 3 +- [x] teach `npmPackage` to skip a post-`node_modules` segment starting with `.` and continue scanning for the next `node_modules` +- [x] gate the npm step: a path containing `/node_modules/.pnpm/` is never claimed as npm — it falls through (ends in `ErrUnknownManager` + `update_cmd` hint unless an earlier step claimed it) +- [x] write `npmPackage` tests: `.pnpm` virtual-store path yields the real package (not `.pnpm`); scoped package under the `.pnpm` store; `.bin`-only path yields `""`; plain and scoped npm paths unchanged +- [x] write chain tests: `.pnpm` store path **outside** all manager dirs → `ErrUnknownManager` (not a silent `npm install -g` duplicate); plain `node_modules` path still yields npm +- [x] run `go test -race ./internal/updater/` - must pass before task 3 ### Task 3: uv detection step @@ -211,10 +211,10 @@ Known limitations (accepted, documented in README by Task 7): - Modify: `internal/updater/updater.go` - Modify: `internal/updater/updater_test.go` -- [ ] add the uv check after pipx, behind `dirs.uvTools != ""`: `segmentUnder(realPath, dirs.uvTools)` → `autoPlan("uv", []string{"uv", "tool", "upgrade", pkg})` -- [ ] extend the `Plan.Manager` doc comment with `"uv"`, listing managers in chain order: `"brew" | "go" | "cargo" | "pipx" | "uv" | "pnpm" | "bun" | "npm" | "custom"` (pnpm/bun land in Tasks 4–5) -- [ ] write test rows: uv tool path detected (argv `uv tool upgrade `); empty `uvTools` with a **relative** path stays undetected (pins the explicit guard — `segmentUnder(p, "")` matches relative paths without it) -- [ ] run `go test -race ./internal/updater/` - must pass before task 4 +- [x] add the uv check after pipx, behind `dirs.uvTools != ""`: `segmentUnder(realPath, dirs.uvTools)` → `autoPlan("uv", []string{"uv", "tool", "upgrade", pkg})` +- [x] extend the `Plan.Manager` doc comment with `"uv"`, listing managers in chain order: `"brew" | "go" | "cargo" | "pipx" | "uv" | "pnpm" | "bun" | "npm" | "custom"` (pnpm/bun land in Tasks 4–5) +- [x] write test rows: uv tool path detected (argv `uv tool upgrade `); empty `uvTools` with a **relative** path stays undetected (pins the explicit guard — `segmentUnder(p, "")` matches relative paths without it) +- [x] run `go test -race ./internal/updater/` - must pass before task 4 ### Task 4: pnpm detection via the cmd-shim target @@ -222,12 +222,12 @@ Known limitations (accepted, documented in README by Task 7): - Modify: `internal/updater/updater.go` - Modify: `internal/updater/updater_test.go` -- [ ] add pure parser `pnpmShimTarget(contents string) string` (`# cmd-shim-target=` line → target path, `""` otherwise) -- [ ] add the bounded best-effort shim read in `Detect`, fired only when the found path sits under a non-empty `pnpmHome`; result rides into `detectFromPath` as `shimTarget` -- [ ] add the pnpm check before bun/npm, behind `dirs.pnpmHome != ""`: `underDir(realPath, dirs.pnpmHome)` + name from `npmPackage(shimTarget)` or `npmPackage(realPath)` → `autoPlan("pnpm", []string{"pnpm", "add", "-g", pkg})`; no name → fall through without error -- [ ] write parser tests: real shim contents (the verified pnpm 11 shape); contents without the marker; empty string; marker with scoped-package target -- [ ] write chain rows: shim-target path (`…/global/v11//node_modules//cli.js`) → `pnpm add -g ` (explicitly not npm); legacy symlink layout via `npmPackage(realPath)`; pnpm's own binary (no target, no `node_modules`) falls through to `ErrUnknownManager`; empty `pnpmHome` guard with a relative path -- [ ] run `go test -race ./internal/updater/` - must pass before task 5 +- [x] add pure parser `pnpmShimTarget(contents string) string` (`# cmd-shim-target=` line → target path, `""` otherwise) +- [x] add the bounded best-effort shim read in `Detect`, fired only when the found path sits under a non-empty `pnpmHome`; result rides into `detectFromPath` as `shimTarget` +- [x] add the pnpm check before bun/npm, behind `dirs.pnpmHome != ""`: `underDir(realPath, dirs.pnpmHome)` + name from `npmPackage(shimTarget)` or `npmPackage(realPath)` → `autoPlan("pnpm", []string{"pnpm", "add", "-g", pkg})`; no name → fall through without error +- [x] write parser tests: real shim contents (the verified pnpm 11 shape); contents without the marker; empty string; marker with scoped-package target +- [x] write chain rows: shim-target path (`…/global/v11//node_modules//cli.js`) → `pnpm add -g ` (explicitly not npm); legacy symlink layout via `npmPackage(realPath)`; pnpm's own binary (no target, no `node_modules`) falls through to `ErrUnknownManager`; empty `pnpmHome` guard with a relative path +- [x] run `go test -race ./internal/updater/` - must pass before task 5 ### Task 5: bun detection step and the Detect wiring test @@ -235,17 +235,17 @@ Known limitations (accepted, documented in README by Task 7): - Modify: `internal/updater/updater.go` - Modify: `internal/updater/updater_test.go` -- [ ] add the bun check between pnpm and npm, behind `dirs.bunInstall != ""`: `underDir(realPath, dirs.bunInstall)` + `npmPackage(realPath) != ""` → `autoPlan("bun", []string{"bun", "add", "-g", pkg})` -- [ ] write chain rows: bun global under `/install/global/node_modules/` → `bun add -g `; scoped package under bun; ordering row (path under `bunInstall` containing `node_modules` resolves to bun, not npm); bun's own binary (`/bin/bun`) falls through to `ErrUnknownManager` -- [ ] write the unix-gated `Detect`-level wiring fixture test (`TestDetectResolvesSymlink` style): tmp `BUN_INSTALL` with `bin/` → `../install/global/node_modules//cli.js` symlink, `t.Setenv("BUN_INSTALL", …)` + `t.Setenv("PATH", …)`, assert `bun add -g ` — pins `Detect` → `resolveManagerDirs()` → `detectFromPath(…, dirs)` threading that pure-core rows cannot catch -- [ ] run `go test -race ./internal/updater/` - must pass before task 6 +- [x] add the bun check between pnpm and npm, behind `dirs.bunInstall != ""`: `underDir(realPath, dirs.bunInstall)` + `npmPackage(realPath) != ""` → `autoPlan("bun", []string{"bun", "add", "-g", pkg})` +- [x] write chain rows: bun global under `/install/global/node_modules/` → `bun add -g `; scoped package under bun; ordering row (path under `bunInstall` containing `node_modules` resolves to bun, not npm); bun's own binary (`/bin/bun`) falls through to `ErrUnknownManager` +- [x] write the unix-gated `Detect`-level wiring fixture test (`TestDetectResolvesSymlink` style): tmp `BUN_INSTALL` with `bin/` → `../install/global/node_modules//cli.js` symlink, `t.Setenv("BUN_INSTALL", …)` + `t.Setenv("PATH", …)`, assert `bun add -g ` — pins `Detect` → `resolveManagerDirs()` → `detectFromPath(…, dirs)` threading that pure-core rows cannot catch +- [x] run `go test -race ./internal/updater/` - must pass before task 6 ### Task 6: Verify acceptance criteria -- [ ] verify all requirements from Overview are implemented (three managers detected, bun-as-npm misdetection fixed, pnpm shim path working, zero-value compatibility) -- [ ] run full test suite: `go test -race ./...` -- [ ] run the preflight skill (build / vet / race tests / golangci-lint — the CI matrix) -- [ ] live smoke check with the locally installed uv, no home pollution: `UV_TOOL_DIR=/tools UV_TOOL_BIN_DIR=/bin uv tool install cowsay`, then a **temporary env-gated test inside the package** (a scratchpad `go run` main cannot import an internal package) asserting `Detect` yields `uv tool upgrade cowsay`; remove the temporary test and the tmp dirs afterwards +- [x] verify all requirements from Overview are implemented (three managers detected, bun-as-npm misdetection fixed, pnpm shim path working, zero-value compatibility) +- [x] run full test suite: `go test -race ./...` +- [x] run the preflight skill (build / vet / race tests / golangci-lint — the CI matrix) +- [x] live smoke check with the locally installed uv, no home pollution: `UV_TOOL_DIR=/tools UV_TOOL_BIN_DIR=/bin uv tool install cowsay`, then a **temporary env-gated test inside the package** (a scratchpad `go run` main cannot import an internal package) asserting `Detect` yields `uv tool upgrade cowsay`; remove the temporary test and the tmp dirs afterwards ### Task 7: [Final] Update documentation @@ -254,11 +254,58 @@ Known limitations (accepted, documented in README by Task 7): - Modify: `README.md` - Modify: `ARCHITECTURE.md` -- [ ] update both CLAUDE.md chain mentions (package-table updater row ~line 39 and the `[u]` section ~line 95): full chain `brew → go → cargo → pipx → uv → pnpm → bun → npm`, the pnpm/bun-before-npm ordering rationale, the shim-target signal, the npm `.pnpm` gate -- [ ] update README.md: manager list (~line 43), detection bullets (~140–144) with uv/pnpm/bun entries and their commands, and the "Updating tools" caveats (~146–161) with the accepted limitations (Windows `.cmd` shims inherited from npm; manager self-updates out of scope) -- [ ] update ARCHITECTURE.md's verbatim chain (~line 199) -- [ ] run the docs-sync skill to catch remaining drift -- [ ] move this plan to `docs/plans/completed/` +- [x] update both CLAUDE.md chain mentions (package-table updater row ~line 39 and the `[u]` section ~line 95): full chain `brew → go → cargo → pipx → uv → pnpm → bun → npm`, the pnpm/bun-before-npm ordering rationale, the shim-target signal, the npm `.pnpm` gate +- [x] update README.md: manager list (~line 43), detection bullets (~140–144) with uv/pnpm/bun entries and their commands, and the "Updating tools" caveats (~146–161) with the accepted limitations (Windows `.cmd` shims inherited from npm; manager self-updates out of scope) +- [x] update ARCHITECTURE.md's verbatim chain (~line 199) +- [x] run the docs-sync skill to catch remaining drift +- [x] move this plan to `docs/plans/completed/` + +### ➕ Task 8: post-review fixes (scope change, agreed after Task 7) + +A diff review found two defects and one coverage hole; the second fix overrides +this plan's "no refactor of the existing steps" non-goal by explicit decision. + +- [x] **oversized shim was parsed truncated** — a cut inside the + `# cmd-shim-target=` line shortened the path, and + `…/node_modules/typescript` shortened to `…/node_modules/types` yields + `types`, a real npm package: the chain would have offered a confidently wrong + `pnpm add -g types`. `readPnpmShim` now reads `cap+1` and rejects an + overrunning file whole (the `getReadme` idiom), so the parser never sees a + truncated tail (`TestReadPnpmShim`) +- [x] **manager roots were compared unexpanded against a resolved binary path** + — measured on all five layouts: uv and pnpm shim/store degraded honestly, but + bun and legacy pnpm globals fell through to `npm install -g `, the very + misdetection this plan set out to fix. `resolveManagerDirs` now expands + symlinks in every root; **`managerDirs` absorbed `cargoBin`/`pipxVenvs`** so + the fix is symmetrical across all five path-convention managers, which also + lets `detectFromPath` stop calling `homeDir()` and makes its "no I/O" + contract literal (`TestDetectSymlinkedManagerRoot`, + `TestResolveManagerDirsExpandsSymlinks`, `TestResolveDirKeepsUnresolvable`) +- [x] **`resolveManagerDirs` arg order was untestable** — both `Detect` + fixtures set the manager's own env var, so `home`/`goos` went unexercised and + swapping them (both `string`) left the suite green. + `TestDetectResolvesUvTool` reaches a root through its *default* and kills it +- [x] docs updated for the widened `managerDirs` and the symlink expansion + (CLAUDE.md, ARCHITECTURE.md); preflight + cross-compile green + +Second review pass over the same diff: + +- [x] **the `!= ""` guard was six hand-copied copies of one convention** — the + very footgun the docs warned about, one omission away from claiming any + relative path. Moved into `underDir`/`segmentUnder`, which now answer "no + match" for an empty dir; all six call sites collapse to a plain helper call + (`TestPathHelpersRejectEmptyDir`) +- [x] **vestigial test setup** — `TestDetectFromPath` still overrode + `testHomeDir` although the core no longer reads home (proved dead by removing + it: suite stayed green). Dropped, with a comment stating the new contract +- [x] **coverage regression caused by that fix** — the empty-dir guard + short-circuited the only input that reached `filepath.Rel`'s error branch; + re-covered with a real shape instead (a relative root such as + `BUN_INSTALL=./bun` against an absolute binary path) + +**Deliberately not done:** `$CARGO_HOME`/`$PIPX_HOME` are still not consulted. +Honouring them changes behaviour for anyone who sets them and is a separate +commit, not a rider on a path-resolution fix. ## Post-Completion diff --git a/internal/updater/updater.go b/internal/updater/updater.go index c87a56b..7d54b5b 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -8,10 +8,12 @@ import ( "context" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" "regexp" + "runtime" "strings" "time" @@ -27,7 +29,9 @@ var ErrUnknownManager = errors.New("no known package manager for tool") // Plan describes how to update a tool. Argv is executed directly (not through a // shell) except for the "custom" manager, where it is ["sh", "-c", ]. type Plan struct { - Manager string // "brew" | "go" | "cargo" | "npm" | "pipx" | "custom" + // Manager names the detected package manager, in chain order: + // "brew" | "go" | "cargo" | "pipx" | "uv" | "pnpm" | "bun" | "npm" | "custom". + Manager string Argv []string // e.g. ["brew", "upgrade", "ripgrep"] Display string // human-facing command shown in the confirm dialog } @@ -47,6 +51,124 @@ func homeDir() string { return h } +// managerDirs carries the per-user root directories of every manager whose +// detection is path-convention based (cargo, pipx, uv, pnpm, bun). Detect fills +// it from the environment; the pure core only reads it — which is what lets +// detectFromPath make good on "no I/O", since it no longer calls homeDir() +// itself. +// +// An empty field switches that manager's check off, which is what makes the +// zero value backwards compatible. That rule is enforced inside underDir and +// segmentUnder, which answer "no match" for an empty dir — deliberately not by +// a `!= ""` guard at each step, which is what it was until six copies of one +// convention proved the point this codebase already made with +// version.applyReleaseOutcome. +type managerDirs struct { + cargoBin string // cargo bin dir: / + pipxVenvs string // pipx venv root: //bin/ + uvTools string // uv tool root: //bin/ + pnpmHome string // pnpm global home: the bin shims plus the global store + bunInstall string // bun install root: /bin/ +} + +// resolveManagerDirs is the thin OS-facing wrapper over managerDirsFrom +// (the launcher.planFor idiom: pure core over an injected env lookup), plus the +// one thing the pure core cannot do — expanding symlinks in the resolved roots. +// +// That expansion is load-bearing, not hygiene. Detect compares these roots +// against an EvalSymlinks-*resolved* binary path, so a root carrying any +// symlink component (a relocated `~/.bun` → `/mnt/big/bun`, a home on a +// secondary volume, `/home` under autofs) simply fails to match, and the miss +// is silent. For uv and pnpm's shim/store layouts that only costs the update +// offer, but a bun global — and a legacy pnpm one — still carries a plain +// `node_modules/` segment, so the npm step claims it and offers +// `npm install -g `: the exact duplicate-install misdetection this chain +// exists to prevent (`TestDetectSymlinkedManagerRoot`). +func resolveManagerDirs() managerDirs { + d := managerDirsFrom(os.Getenv, homeDir(), runtime.GOOS) + // Every root, not just home: the symlink can sit anywhere on the path, and + // an env override brings its own. A new field must be added here too — the + // all-five assertion in TestResolveManagerDirsExpandsSymlinks is the guard. + for _, dir := range []*string{&d.cargoBin, &d.pipxVenvs, &d.uvTools, &d.pnpmHome, &d.bunInstall} { + *dir = resolveDir(*dir) + } + return d +} + +// resolveDir expands every symlink component of dir, falling back to dir itself +// when that fails — a root that does not exist yet is the normal state for a +// manager the user has not installed, and it must stay a harmless non-match +// rather than becoming an empty (disabled) field. +func resolveDir(dir string) string { + if dir == "" { + return "" + } + if real, err := filepath.EvalSymlinks(dir); err == nil { + return real + } + return dir +} + +// managerDirsFrom is the pure resolution core. Each manager's own env var wins; +// otherwise the platform default is used, and an empty home disables every +// home-based default (the field stays empty, i.e. the check stays off). +// +// cargo and pipx deliberately read no env var of their own here — $CARGO_HOME +// and $PIPX_HOME are not consulted, exactly as before this struct existed. +// Honouring them is a behaviour change for anyone who sets them and belongs in +// its own commit, not smuggled in with a path-resolution fix. +func managerDirsFrom(getenv func(string) string, home, goos string) managerDirs { + var d managerDirs + + // cargo and pipx: home-derived on every platform, no override. + if home != "" { + d.cargoBin = filepath.Join(home, ".cargo", "bin") + d.pipxVenvs = filepath.Join(home, ".local", "pipx", "venvs") + } + + // uv: $UV_TOOL_DIR wins. uv follows the XDG layout on macOS too (it does not + // use ~/Library/Application Support), so darwin and linux share one default. + // Windows has no default at all — uv's layout there is unverified and a + // wrong guess would be worse than the check being off (honest degradation). + d.uvTools = getenv("UV_TOOL_DIR") + if d.uvTools == "" && goos != "windows" { + if xdg := getenv("XDG_DATA_HOME"); xdg != "" { + d.uvTools = filepath.Join(xdg, "uv", "tools") + } else if home != "" { + d.uvTools = filepath.Join(home, ".local", "share", "uv", "tools") + } + } + + // pnpm: $PNPM_HOME wins; the default differs per platform. + d.pnpmHome = getenv("PNPM_HOME") + if d.pnpmHome == "" { + switch goos { + case "darwin": + if home != "" { + d.pnpmHome = filepath.Join(home, "Library", "pnpm") + } + case "windows": + if local := getenv("LOCALAPPDATA"); local != "" { + d.pnpmHome = filepath.Join(local, "pnpm") + } + default: + if xdg := getenv("XDG_DATA_HOME"); xdg != "" { + d.pnpmHome = filepath.Join(xdg, "pnpm") + } else if home != "" { + d.pnpmHome = filepath.Join(home, ".local", "share", "pnpm") + } + } + } + + // bun: $BUN_INSTALL wins, else ~/.bun on every platform. + d.bunInstall = getenv("BUN_INSTALL") + if d.bunInstall == "" && home != "" { + d.bunInstall = filepath.Join(home, ".bun") + } + + return d +} + // testBrewPrefix overrides the Homebrew prefix in tests. var testBrewPrefix string @@ -139,8 +261,17 @@ func Detect(t loader.Tool) (Plan, error) { } buildinfo := goBuildinfo(realPath) + dirs := resolveManagerDirs() + + // The pnpm signal, gathered exactly like buildinfo and only where it can + // possibly exist: a bin shim under PNPM_HOME. Reading every binary on PATH + // to look for a comment would be a needless open per detection. + shimTarget := "" + if underDir(realPath, dirs.pnpmHome) { + shimTarget = readPnpmShim(realPath) + } - plan, err := detectFromPath(realPath, buildinfo) + plan, err := detectFromPath(realPath, buildinfo, shimTarget, dirs) if err != nil { // The chain can exhaust with the binary found: a Homebrew cask app keeps // its launcher on PATH while the executable lives inside the .app bundle @@ -234,6 +365,61 @@ func runProbe(name string, args ...string) (string, error) { return string(out), nil } +// pnpmShimMarker is the trailing comment pnpm's cmd-shim writes into every +// global bin script, naming the file the shim actually executes. +const pnpmShimMarker = "# cmd-shim-target=" + +// pnpmShimMaxBytes caps the shim read. Real shims are ~1.5 KiB; the cap is what +// keeps the read bounded when the path turns out to be an ordinary binary that +// merely happens to sit under PNPM_HOME. +// +// Anything larger is rejected outright rather than parsed truncated, and that +// is a correctness guard, not tidiness: a cut landing inside the marker line +// hands the parser a *shortened* path, and shortening +// ".../node_modules/typescript/..." to ".../node_modules/types" yields "types" +// — a real package on npm. The chain would then offer `pnpm add -g types`, a +// confidently wrong command where the whole design promises honest degradation. +const pnpmShimMaxBytes = 8 << 10 + +// pnpmShimTarget is the pure parser for a pnpm global bin shim: the path the +// shim executes, or "" when the contents carry no marker. +// +// pnpm's globals are not symlinks — $PNPM_HOME/bin/ is a /bin/sh script +// EvalSymlinks resolves to itself — so this comment is the only machine +// readable link from the binary on PATH to the package that owns it. The last +// marker wins: cmd-shim writes it as the final line, so anything earlier came +// from the package's own text. +func pnpmShimTarget(contents string) string { + target := "" + for line := range strings.SplitSeq(contents, "\n") { + if after, ok := strings.CutPrefix(strings.TrimSpace(line), pnpmShimMarker); ok { + target = strings.TrimSpace(after) + } + } + return target +} + +// readPnpmShim is the OS-facing half of the pnpm signal: a bounded best-effort +// read of a pnpm global bin shim. Every failure yields "" rather than an error +// — an unreadable file just leaves the pnpm check without its signal, exactly +// like a missing `go` leaves goBuildinfo empty. +func readPnpmShim(path string) string { + f, err := os.Open(path) + if err != nil { + return "" + } + defer func() { _ = f.Close() }() + // Read one byte past the cap (the version.getReadme idiom) purely to tell + // "fits" from "does not fit": a file that overruns is not a cmd-shim, and + // the parser must never see a truncated tail. Rejecting is the whole point + // — see pnpmShimMaxBytes. + buf, err := io.ReadAll(io.LimitReader(f, pnpmShimMaxBytes+1)) + if err != nil || len(buf) > pnpmShimMaxBytes { + return "" + } + return pnpmShimTarget(string(buf)) +} + // cellarRe extracts the formula name from a Homebrew Cellar path segment: // .../Cellar///bin/. var cellarRe = regexp.MustCompile(`/Cellar/([^/]+)/`) @@ -242,13 +428,17 @@ var cellarRe = regexp.MustCompile(`/Cellar/([^/]+)/`) var goPathRe = regexp.MustCompile(`(?m)^\s*path\s+(\S+)\s*$`) // detectFromPath is the pure detection core: given a binary's real (symlink -// resolved) path and the output of `go version -m ` (may be empty), it -// returns the update Plan. It performs no I/O and spawns no subprocesses, so -// table tests need no real package managers installed. +// resolved) path, the output of `go version -m ` (may be empty), the pnpm +// cmd-shim target Detect read off that binary (empty when there is none) and +// the resolved manager directories, it returns the update Plan. It performs no +// I/O, reads no environment and spawns no subprocesses — every root arrives in +// dirs — so table tests need no real package managers installed. // -// Order matters: brew is checked before go because a brew-installed Go binary -// carries buildinfo and would otherwise be misrouted to `go install`. -func detectFromPath(realPath, buildinfo string) (Plan, error) { +// Order matters twice over: brew is checked before go because a brew-installed +// Go binary carries buildinfo and would otherwise be misrouted to `go install`, +// and pnpm/bun are checked before npm because their layouts contain +// node_modules segments the npm step would otherwise claim. +func detectFromPath(realPath, buildinfo, shimTarget string, dirs managerDirs) (Plan, error) { // 1. Homebrew Cellar. if m := cellarRe.FindStringSubmatch(realPath); m != nil { formula := m[1] @@ -261,29 +451,61 @@ func detectFromPath(realPath, buildinfo string) (Plan, error) { return autoPlan("go", []string{"go", "install", module + "@latest"}), nil } - home := homeDir() - // 3. Cargo (~/.cargo/bin). Crate name defaults to the binary name; the OS // wrapper refines it via `cargo install --list`. - if home != "" { - cargoBin := filepath.Join(home, ".cargo", "bin") - if underDir(realPath, cargoBin) { - crate := binaryName(realPath) - return autoPlan("cargo", []string{"cargo", "install", crate}), nil - } + if underDir(realPath, dirs.cargoBin) { + crate := binaryName(realPath) + return autoPlan("cargo", []string{"cargo", "install", crate}), nil } // 4. pipx (~/.local/pipx/venvs//...). - if home != "" { - pipxVenvs := filepath.Join(home, ".local", "pipx", "venvs") - if pkg := segmentUnder(realPath, pipxVenvs); pkg != "" { - return autoPlan("pipx", []string{"pipx", "upgrade", pkg}), nil + if pkg := segmentUnder(realPath, dirs.pipxVenvs); pkg != "" { + return autoPlan("pipx", []string{"pipx", "upgrade", pkg}), nil + } + + // 5. uv (//bin/, reached through a symlink in uv's own + // bin dir) — the same technique as pipx, one ecosystem tool over. + if pkg := segmentUnder(realPath, dirs.uvTools); pkg != "" { + return autoPlan("uv", []string{"uv", "tool", "upgrade", pkg}), nil + } + + // 6. pnpm globals, before npm because the store path carries node_modules + // segments the npm step would otherwise claim. The package name comes from + // the shim target Detect read for us (pnpm >= 9) or, for the legacy symlink + // layouts, from the resolved path itself. Neither yielding a name means this + // is not a package binary at all — pnpm's own executable — so it falls + // through without an error (`pnpm self-update` is out of scope). + if underDir(realPath, dirs.pnpmHome) { + if pkg := npmPackage(shimTarget); pkg != "" { + return autoPlan("pnpm", []string{"pnpm", "add", "-g", pkg}), nil + } + if pkg := npmPackage(realPath); pkg != "" { + return autoPlan("pnpm", []string{"pnpm", "add", "-g", pkg}), nil } } - // 5. npm global (.../node_modules//...). - if pkg := npmPackage(realPath); pkg != "" { - return autoPlan("npm", []string{"npm", "install", "-g", pkg}), nil + // 7. bun globals ($BUN_INSTALL/bin/ symlinked into + // install/global/node_modules//...). Before npm for the same reason as + // pnpm, and not merely for tidiness: the npm step used to claim these and + // offer `npm install -g `, installing a duplicate under npm's prefix. + // bun's own binary has no node_modules segment and falls through — `bun + // upgrade` is out of scope. + if underDir(realPath, dirs.bunInstall) { + if pkg := npmPackage(realPath); pkg != "" { + return autoPlan("bun", []string{"bun", "add", "-g", pkg}), nil + } + } + + // 8. npm global (.../node_modules//...). A path through pnpm's virtual + // store that got this far is a pnpm layout the pnpm step failed to + // attribute — claiming it for npm would offer `npm install -g `, which + // silently installs a second working copy under npm's prefix while the pnpm + // one keeps shadowing it on PATH. Falling through to ErrUnknownManager plus + // the update_cmd hint is the honest degradation. + if !strings.Contains(filepath.ToSlash(realPath), pnpmStoreSegment) { + if pkg := npmPackage(realPath); pkg != "" { + return autoPlan("npm", []string{"npm", "install", "-g", pkg}), nil + } } return Plan{}, ErrUnknownManager @@ -300,7 +522,17 @@ func binaryName(p string) string { } // underDir reports whether path p lives under directory dir. +// +// An empty dir is **no match**, and that guard belongs here rather than at the +// call sites: filepath.Rel("", "bin/exa") succeeds and yields "bin/exa", so a +// *relative* p would otherwise read as living under every root. Every caller +// passes a managerDirs field, where empty means "this check is off", and that +// used to be a `!= ""` convention hand-copied into six call sites — the shape +// this codebase already learned to replace with one definition. func underDir(p, dir string) bool { + if dir == "" { + return false + } rel, err := filepath.Rel(dir, p) if err != nil { return false @@ -310,8 +542,11 @@ func underDir(p, dir string) bool { // segmentUnder returns the first path segment of p immediately below dir, or "" // if p is not under dir. E.g. segmentUnder(".../venvs/black/bin/black", -// ".../venvs") == "black". +// ".../venvs") == "black". An empty dir yields "" for underDir's reason. func segmentUnder(p, dir string) string { + if dir == "" { + return "" + } rel, err := filepath.Rel(dir, p) if err != nil { return "" @@ -329,7 +564,13 @@ func segmentUnder(p, dir string) string { // npmPackage returns the npm package name for a binary whose realpath goes // through a node_modules directory (.../node_modules//...), handling // scoped packages (@scope/name). Returns "" when there is no node_modules -// segment. +// segment carrying a package name. +// +// A segment starting with "." right after node_modules is a service directory, +// never a package: ".pnpm" is pnpm's virtual store and ".bin" holds the shims. +// Such a segment is skipped and the scan continues — pnpm's store nests the +// real package one level deeper +// (.../node_modules/.pnpm/@/node_modules//...). func npmPackage(p string) string { parts := strings.Split(filepath.ToSlash(p), "/") for i, seg := range parts { @@ -340,6 +581,9 @@ func npmPackage(p string) string { return "" } pkg := parts[i+1] + if strings.HasPrefix(pkg, ".") { + continue // service dir (.pnpm, .bin) — keep scanning + } if strings.HasPrefix(pkg, "@") && i+2 < len(parts) { return pkg + "/" + parts[i+2] } @@ -347,3 +591,8 @@ func npmPackage(p string) string { } return "" } + +// pnpmStoreSegment is pnpm's virtual store directory inside node_modules. A +// path through it that no earlier chain step claimed is a pnpm layout we failed +// to attribute — see the npm step's gate in detectFromPath. +const pnpmStoreSegment = "/node_modules/.pnpm/" diff --git a/internal/updater/updater_test.go b/internal/updater/updater_test.go index aeb6664..520aec2 100644 --- a/internal/updater/updater_test.go +++ b/internal/updater/updater_test.go @@ -12,17 +12,22 @@ import ( ) func TestDetectFromPath(t *testing.T) { + // Just a prefix for building fixture paths — no testHomeDir override, and + // that is the point: every root now arrives in dirs, so the core reads no + // environment at all. An override here would suggest otherwise. home := "/home/tester" - origHome := testHomeDir - testHomeDir = home - defer func() { testHomeDir = origHome }() goBuildinfo := "\tpath\tgithub.com/junegunn/fzf\n\tmod\tgithub.com/junegunn/fzf\tv0.55.0\n" + pnpmHome := filepath.Join(home, "Library", "pnpm") + bunInstall := filepath.Join(home, ".bun") + bunGlobal := filepath.Join(bunInstall, "install", "global", "node_modules") tests := []struct { name string realPath string buildinfo string + shimTarget string + dirs managerDirs wantManager string wantArgv []string wantErr error @@ -43,15 +48,24 @@ func TestDetectFromPath(t *testing.T) { { name: "cargo bin dir", realPath: filepath.Join(home, ".cargo", "bin", "exa"), + dirs: managerDirs{cargoBin: filepath.Join(home, ".cargo", "bin")}, wantManager: "cargo", wantArgv: []string{"cargo", "install", "exa"}, }, { name: "pipx venv package", realPath: filepath.Join(home, ".local", "pipx", "venvs", "black", "bin", "black"), + dirs: managerDirs{pipxVenvs: filepath.Join(home, ".local", "pipx", "venvs")}, wantManager: "pipx", wantArgv: []string{"pipx", "upgrade", "black"}, }, + { + // The same `!= ""` guard the three newer steps carry: with an empty + // cargoBin, underDir(p, "") would claim any relative path. + name: "empty cargoBin leaves a relative path undetected", + realPath: "bin/exa", + wantErr: ErrUnknownManager, + }, { name: "npm global node_modules", realPath: "/usr/local/lib/node_modules/typescript/bin/tsc", @@ -70,6 +84,112 @@ func TestDetectFromPath(t *testing.T) { buildinfo: "", wantErr: ErrUnknownManager, }, + { + name: "uv tool", + realPath: filepath.Join(home, ".local", "share", "uv", "tools", "cowsay", "bin", "cowsay"), + dirs: managerDirs{uvTools: filepath.Join(home, ".local", "share", "uv", "tools")}, + wantManager: "uv", + wantArgv: []string{"uv", "tool", "upgrade", "cowsay"}, + }, + { + // The explicit `dirs.uvTools != ""` guard: segmentUnder(p, "") matches + // any relative path, so without it an empty dir would claim this one. + name: "empty uvTools leaves a relative path undetected", + realPath: "somewhere/cowsay/bin/cowsay", + wantErr: ErrUnknownManager, + }, + { + // pnpm >= 9: the bin entry is a shell script, so the package name can + // only come from the shim target Detect read off it. + name: "pnpm shim target", + realPath: filepath.Join(pnpmHome, "bin", "tsc"), + shimTarget: filepath.Join(pnpmHome, "global", "v11", "8f3a2c1", "node_modules", "typescript", "cli.js"), + dirs: managerDirs{pnpmHome: pnpmHome}, + wantManager: "pnpm", + wantArgv: []string{"pnpm", "add", "-g", "typescript"}, + }, + { + name: "pnpm shim target with a scoped package", + realPath: filepath.Join(pnpmHome, "bin", "ng"), + shimTarget: filepath.Join(pnpmHome, "global", "v11", "1d0e9b4", "node_modules", "@angular", "cli", "bin", "ng.js"), + dirs: managerDirs{pnpmHome: pnpmHome}, + wantManager: "pnpm", + wantArgv: []string{"pnpm", "add", "-g", "@angular/cli"}, + }, + { + // Legacy layouts symlink into the store, so the resolved path itself + // names the package and no shim target is involved. + name: "pnpm legacy symlink layout", + realPath: filepath.Join(pnpmHome, "global", "5", "node_modules", "typescript", "bin", "tsc"), + dirs: managerDirs{pnpmHome: pnpmHome}, + wantManager: "pnpm", + wantArgv: []string{"pnpm", "add", "-g", "typescript"}, + }, + { + // The virtual store under PNPM_HOME: pnpm claims it, and the ".pnpm" + // service segment must not become the package name. + name: "pnpm virtual store layout", + realPath: filepath.Join(pnpmHome, "global", "5", "node_modules", ".pnpm", "typescript@5.6.2", "node_modules", "typescript", "bin", "tsc"), + dirs: managerDirs{pnpmHome: pnpmHome}, + wantManager: "pnpm", + wantArgv: []string{"pnpm", "add", "-g", "typescript"}, + }, + { + // pnpm's own binary: no shim target, no node_modules segment. Manager + // self-updates are out of scope, so it falls through the whole chain. + name: "pnpm own binary falls through", + realPath: filepath.Join(pnpmHome, "bin", "pnpm"), + dirs: managerDirs{pnpmHome: pnpmHome}, + wantErr: ErrUnknownManager, + }, + { + // The explicit `dirs.pnpmHome != ""` guard: underDir(p, "") is true for + // any relative path, so without it this shim target would claim pnpm. + name: "empty pnpmHome leaves a relative path undetected", + realPath: "bin/tsc", + shimTarget: "/somewhere/node_modules/typescript/cli.js", + wantErr: ErrUnknownManager, + }, + { + name: "bun global package", + realPath: filepath.Join(bunGlobal, "prettier", "bin", "prettier.cjs"), + dirs: managerDirs{bunInstall: bunInstall}, + wantManager: "bun", + wantArgv: []string{"bun", "add", "-g", "prettier"}, + }, + { + name: "bun scoped package", + realPath: filepath.Join(bunGlobal, "@angular", "cli", "bin", "ng"), + dirs: managerDirs{bunInstall: bunInstall}, + wantManager: "bun", + wantArgv: []string{"bun", "add", "-g", "@angular/cli"}, + }, + { + // Ordering: the path carries node_modules, so with bun checked after + // npm this resolved to `npm install -g prettier` — the wrong manager, + // installing a duplicate under npm's prefix. + name: "order: bun path with node_modules yields bun not npm", + realPath: filepath.Join(bunGlobal, "prettier", "bin", "prettier.cjs"), + dirs: managerDirs{bunInstall: bunInstall, uvTools: filepath.Join(home, "uv")}, + wantManager: "bun", + wantArgv: []string{"bun", "add", "-g", "prettier"}, + }, + { + // bun's own binary: under bunInstall but with no node_modules segment. + // `bun upgrade` is out of scope, so it falls through the chain. + name: "bun own binary falls through", + realPath: filepath.Join(bunInstall, "bin", "bun"), + dirs: managerDirs{bunInstall: bunInstall}, + wantErr: ErrUnknownManager, + }, + { + // A pnpm store path no manager dir covers must NOT be claimed by npm: + // `npm install -g typescript` would install a duplicate under npm's + // prefix while the pnpm copy keeps shadowing it on PATH. + name: "pnpm store path outside every manager dir is not npm", + realPath: "/elsewhere/node_modules/.pnpm/typescript@5.6.2/node_modules/typescript/bin/tsc", + wantErr: ErrUnknownManager, + }, { name: "order: cellar with buildinfo yields brew not go", realPath: "/opt/homebrew/Cellar/gopls/0.16.1/bin/gopls", @@ -81,7 +201,7 @@ func TestDetectFromPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - plan, err := detectFromPath(tt.realPath, tt.buildinfo) + plan, err := detectFromPath(tt.realPath, tt.buildinfo, tt.shimTarget, tt.dirs) if tt.wantErr != nil { if !errors.Is(err, tt.wantErr) { t.Fatalf("err = %v, want %v", err, tt.wantErr) @@ -105,6 +225,478 @@ func TestDetectFromPath(t *testing.T) { } } +// TestManagerDirsFrom pins the pure env resolution of the three +// path-convention manager roots. Expectations are built with filepath.Join, not +// literal separators: the core runs on the host OS (the configdir.baseFor +// table-test precedent), so a hardcoded "\" would fail everywhere but Windows. +func TestManagerDirsFrom(t *testing.T) { + const home = "/home/tester" + joinPath := filepath.Join + + tests := []struct { + name string + env map[string]string + home string + goos string + want managerDirs + }{ + { + name: "linux bare home", + home: home, + goos: "linux", + want: managerDirs{ + uvTools: joinPath(home, ".local", "share", "uv", "tools"), + pnpmHome: joinPath(home, ".local", "share", "pnpm"), + bunInstall: joinPath(home, ".bun"), + }, + }, + { + name: "linux XDG_DATA_HOME", + env: map[string]string{"XDG_DATA_HOME": "/xdg/data"}, + home: home, + goos: "linux", + want: managerDirs{ + uvTools: joinPath("/xdg/data", "uv", "tools"), + pnpmHome: joinPath("/xdg/data", "pnpm"), + bunInstall: joinPath(home, ".bun"), + }, + }, + { + // uv uses the XDG layout on macOS too, so only pnpm differs here. + name: "darwin bare home", + home: home, + goos: "darwin", + want: managerDirs{ + uvTools: joinPath(home, ".local", "share", "uv", "tools"), + pnpmHome: joinPath(home, "Library", "pnpm"), + bunInstall: joinPath(home, ".bun"), + }, + }, + { + name: "darwin ignores XDG for pnpm", + env: map[string]string{"XDG_DATA_HOME": "/xdg/data"}, + home: home, + goos: "darwin", + want: managerDirs{ + uvTools: joinPath("/xdg/data", "uv", "tools"), + pnpmHome: joinPath(home, "Library", "pnpm"), + bunInstall: joinPath(home, ".bun"), + }, + }, + { + // Windows: uv has no default at all, pnpm rides LOCALAPPDATA. + name: "windows LOCALAPPDATA", + env: map[string]string{"LOCALAPPDATA": `C:\Users\t\AppData\Local`}, + home: home, + goos: "windows", + want: managerDirs{ + uvTools: "", + pnpmHome: joinPath(`C:\Users\t\AppData\Local`, "pnpm"), + bunInstall: joinPath(home, ".bun"), + }, + }, + { + name: "windows without LOCALAPPDATA", + home: home, + goos: "windows", + want: managerDirs{ + uvTools: "", + pnpmHome: "", + bunInstall: joinPath(home, ".bun"), + }, + }, + { + name: "windows uv is env-only", + env: map[string]string{"UV_TOOL_DIR": "/uv/tools", "XDG_DATA_HOME": "/xdg/data"}, + home: home, + goos: "windows", + want: managerDirs{ + uvTools: "/uv/tools", + pnpmHome: "", + bunInstall: joinPath(home, ".bun"), + }, + }, + { + name: "env overrides win over every default", + env: map[string]string{ + "UV_TOOL_DIR": "/custom/uv", + "PNPM_HOME": "/custom/pnpm", + "BUN_INSTALL": "/custom/bun", + "XDG_DATA_HOME": "/xdg/data", + }, + home: home, + goos: "linux", + want: managerDirs{uvTools: "/custom/uv", pnpmHome: "/custom/pnpm", bunInstall: "/custom/bun"}, + }, + { + // No home, no env: every check stays off. + name: "empty home disables defaults", + goos: "linux", + want: managerDirs{}, + }, + { + // An env override still works without a home. + name: "empty home keeps env overrides", + env: map[string]string{"PNPM_HOME": "/custom/pnpm"}, + goos: "darwin", + want: managerDirs{pnpmHome: "/custom/pnpm"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // cargo and pipx depend on home alone — no env var, no per-platform + // branch — so they are asserted once here instead of being repeated + // identically in every row, where they would bury the differences + // the table exists to show. + want := tt.want + if tt.home != "" { + want.cargoBin = joinPath(tt.home, ".cargo", "bin") + want.pipxVenvs = joinPath(tt.home, ".local", "pipx", "venvs") + } + getenv := func(k string) string { return tt.env[k] } + if got := managerDirsFrom(getenv, tt.home, tt.goos); got != want { + t.Errorf("managerDirsFrom() = %+v, want %+v", got, want) + } + }) + } +} + +// TestNpmPackage pins the package-name extraction, including the service +// segments (".pnpm", ".bin") that are never package names — before the skip, +// a pnpm store path resolved to the literal package ".pnpm". +func TestNpmPackage(t *testing.T) { + tests := []struct { + name string + path string + want string + }{ + {"plain global package", "/usr/local/lib/node_modules/typescript/bin/tsc", "typescript"}, + {"scoped global package", "/usr/local/lib/node_modules/@angular/cli/bin/ng", "@angular/cli"}, + { + "pnpm virtual store skips .pnpm", + "/home/t/.local/share/pnpm/global/5/node_modules/.pnpm/typescript@5.6.2/node_modules/typescript/bin/tsc", + "typescript", + }, + { + "pnpm virtual store with a scoped package", + "/home/t/.local/share/pnpm/global/5/node_modules/.pnpm/@angular+cli@18.0.0/node_modules/@angular/cli/bin/ng", + "@angular/cli", + }, + {"only a .bin shim dir", "/usr/local/lib/node_modules/.bin/tsc", ""}, + {"no node_modules segment", "/usr/local/bin/tsc", ""}, + {"node_modules is the last segment", "/usr/local/lib/node_modules", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := npmPackage(tt.path); got != tt.want { + t.Errorf("npmPackage(%q) = %q, want %q", tt.path, got, tt.want) + } + }) + } +} + +// TestPnpmShimTarget pins the pure shim parser against the real pnpm cmd-shim +// shape: the marker is the last line, and the path it names is the only machine +// readable link from a pnpm global bin script to the package that owns it. +func TestPnpmShimTarget(t *testing.T) { + const target = "/home/tester/Library/pnpm/global/v11/8f3a2c1/node_modules/typescript/bin/tsc" + shim := "#!/bin/sh\n" + + `basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")` + "\n" + + "\n" + + "case `uname` in\n" + + " *CYGWIN*|*MINGW*|*MSYS*)\n" + + " if command -v cygpath > /dev/null 2>&1; then\n" + + ` basedir=$(cygpath -w "$basedir")` + "\n" + + " fi\n" + + " ;;\n" + + "esac\n" + + "\n" + + `if [ -x "$basedir/node" ]; then` + "\n" + + ` exec "$basedir/node" "` + target + `" "$@"` + "\n" + + "else\n" + + ` exec node "` + target + `" "$@"` + "\n" + + "fi\n" + + "# cmd-shim-target=" + target + "\n" + + tests := []struct { + name string + contents string + want string + }{ + {"real shim", shim, target}, + { + "scoped package target", + "#!/bin/sh\n# cmd-shim-target=/home/t/Library/pnpm/global/v11/1d0/node_modules/@angular/cli/bin/ng.js\n", + "/home/t/Library/pnpm/global/v11/1d0/node_modules/@angular/cli/bin/ng.js", + }, + {"no marker", "#!/bin/sh\nexec node foo.js\n", ""}, + {"empty contents", "", ""}, + { + // pnpm writes \r\n shims on some setups; the \r must not ride along + // into the path and turn node_modules lookups into misses. + "crlf line endings", + "#!/bin/sh\r\n# cmd-shim-target=/home/t/pnpm/global/v11/1d0/node_modules/typescript/bin/tsc\r\n", + "/home/t/pnpm/global/v11/1d0/node_modules/typescript/bin/tsc", + }, + { + // The marker is written last, so a copy of it inside the package's own + // text (an npm bin shim the package itself ships) must not win. + "last marker wins", + "# cmd-shim-target=/old/node_modules/a/cli.js\n# cmd-shim-target=/new/node_modules/b/cli.js\n", + "/new/node_modules/b/cli.js", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := pnpmShimTarget(tt.contents); got != tt.want { + t.Errorf("pnpmShimTarget() = %q, want %q", got, tt.want) + } + }) + } +} + +// TestReadPnpmShim covers the OS-facing half of the pnpm signal, whose whole +// contract is "every failure is an empty signal, never an error" — plus the one +// case where an empty signal is a correctness guard rather than degradation: +// an oversized file, whose truncated tail would otherwise be parsed. +func TestReadPnpmShim(t *testing.T) { + dir := t.TempDir() + const target = "/home/t/Library/pnpm/global/v11/abc/node_modules/typescript/bin/tsc" + markerLine := pnpmShimMarker + target + "\n" + + write := func(t *testing.T, name, contents string) string { + t.Helper() + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(contents), 0o755); err != nil { + t.Fatal(err) + } + return p + } + + // Exactly at the cap: still read, still parsed. + atCap := "#!/bin/sh\n" + strings.Repeat("#", pnpmShimMaxBytes-len(markerLine)-11) + "\n" + markerLine + if len(atCap) != pnpmShimMaxBytes { + t.Fatalf("fixture is %d bytes, want exactly the cap %d", len(atCap), pnpmShimMaxBytes) + } + + // One byte over, with the cut landing inside the target's package name: + // parsed truncated this yields "types", a real npm package, and the chain + // would offer `pnpm add -g types`. + straddling := "#!/bin/sh\n" + strings.Repeat("#", pnpmShimMaxBytes-len(markerLine)+3) + "\n" + markerLine + + tests := []struct { + name string + path string + want string + }{ + {"real shim", write(t, "tsc", "#!/bin/sh\nexec node x\n"+markerLine), target}, + {"no marker", write(t, "plain", "#!/bin/sh\nexec node x\n"), ""}, + {"empty file", write(t, "empty", ""), ""}, + {"exactly at the cap", write(t, "atcap", atCap), target}, + {"oversized shim is rejected whole", write(t, "big", straddling), ""}, + {"missing file", filepath.Join(dir, "nope"), ""}, + {"path is a directory", dir, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := readPnpmShim(tt.path); got != tt.want { + t.Errorf("readPnpmShim() = %q, want %q", got, tt.want) + } + }) + } + + // The guard is what makes the rejection meaningful: spell out what parsing + // the truncated tail would have produced. + if got := pnpmShimTarget(straddling[:pnpmShimMaxBytes]); npmPackage(got) != "types" { + t.Fatalf("fixture no longer straddles the package name: target %q → pkg %q", got, npmPackage(got)) + } +} + +// TestPathHelpersRejectEmptyDir pins the safe-by-construction half of +// managerDirs: an empty root means "this check is off", and both helpers answer +// that themselves. It used to be a `!= ""` guard hand-copied into six call +// sites, where one omission silently claimed any *relative* path — a real +// hazard, since filepath.Rel("", "bin/exa") succeeds and yields "bin/exa". +func TestPathHelpersRejectEmptyDir(t *testing.T) { + // Relative paths are the dangerous ones; absolutes already failed Rel. + for _, p := range []string{"bin/exa", "cowsay/bin/cowsay", "exa", "", "/abs/bin/exa"} { + if underDir(p, "") { + t.Errorf("underDir(%q, \"\") = true, want false", p) + } + if got := segmentUnder(p, ""); got != "" { + t.Errorf("segmentUnder(%q, \"\") = %q, want \"\"", p, got) + } + } + // A relative root against an absolute path (a user writing + // `export BUN_INSTALL=./bun`): filepath.Rel cannot relate the two and + // errors, which must read as "no match" rather than propagate. + if underDir("/abs/bin/exa", filepath.Join("rel", "root")) { + t.Error("underDir claimed an absolute path for a relative root") + } + if got := segmentUnder("/abs/bin/exa", filepath.Join("rel", "root")); got != "" { + t.Errorf("segmentUnder = %q, want \"\" for a relative root", got) + } + + // A non-empty dir keeps working, so the guard is not a blanket off switch. + if !underDir(filepath.Join("root", "bin", "exa"), "root") { + t.Error("underDir lost a genuine match") + } + if got := segmentUnder(filepath.Join("root", "black", "bin"), "root"); got != "black" { + t.Errorf("segmentUnder = %q, want %q", got, "black") + } +} + +// TestResolveManagerDirsExpandsSymlinks asserts that **every** field comes back +// symlink-expanded. It is the guard on the loop in resolveManagerDirs: a field +// added to managerDirs but forgotten there would resolve to a root Detect can +// never match against a real (resolved) binary path, and the miss is silent. +// +// Both mechanisms are covered at once — cargo/pipx/uv inherit their symlink +// from a relocated home, pnpm/bun bring their own through an env override. +func TestResolveManagerDirsExpandsSymlinks(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink fixture is unix-only, and uv has no Windows default") + } + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + + // A home reached through a symlink: /home under autofs, a home on a + // secondary volume, /var → /private/var on macOS. + realHome := filepath.Join(base, "realhome") + homeLink := filepath.Join(base, "home") + mustMkdirAll(t, realHome) + if err := os.Symlink(realHome, homeLink); err != nil { + t.Fatal(err) + } + setTestHomeDir(t, homeLink) + t.Setenv("UV_TOOL_DIR", "") + t.Setenv("XDG_DATA_HOME", "") + + // A relocated tool root: `ln -s /mnt/big/bun ~/.bun`, the disk-space case. + realStore := filepath.Join(base, "store") + storeLink := filepath.Join(base, "storelink") + mustMkdirAll(t, realStore) + if err := os.Symlink(realStore, storeLink); err != nil { + t.Fatal(err) + } + t.Setenv("PNPM_HOME", filepath.Join(storeLink, "pnpm")) + t.Setenv("BUN_INSTALL", filepath.Join(storeLink, "bun")) + + // EvalSymlinks resolves only paths that exist, so every root must. + uvDefault := filepath.Join(realHome, ".local", "share", "uv", "tools") + for _, d := range []string{ + filepath.Join(realHome, ".cargo", "bin"), + filepath.Join(realHome, ".local", "pipx", "venvs"), + uvDefault, + filepath.Join(realStore, "pnpm"), + filepath.Join(realStore, "bun"), + } { + mustMkdirAll(t, d) + } + + got := resolveManagerDirs() + want := managerDirs{ + cargoBin: filepath.Join(realHome, ".cargo", "bin"), + pipxVenvs: filepath.Join(realHome, ".local", "pipx", "venvs"), + uvTools: uvDefault, + pnpmHome: filepath.Join(realStore, "pnpm"), + bunInstall: filepath.Join(realStore, "bun"), + } + if got != want { + t.Errorf("resolveManagerDirs() = %+v,\nwant %+v", got, want) + } +} + +// TestResolveDirKeepsUnresolvable pins the fallback: a root that does not exist +// (the normal state for a manager the user has not installed) must stay a +// harmless non-match, never become an empty — i.e. disabled — field, which +// would be indistinguishable from "this platform has no default". +func TestResolveDirKeepsUnresolvable(t *testing.T) { + missing := filepath.Join(t.TempDir(), "nope", "bin") + if got := resolveDir(missing); got != missing { + t.Errorf("resolveDir(missing) = %q, want it unchanged", got) + } + if got := resolveDir(""); got != "" { + t.Errorf("resolveDir(\"\") = %q, want \"\"", got) + } +} + +// TestDetectSymlinkedManagerRoot is the end-to-end regression for the two +// layouts where an unexpanded root did not merely lose the update offer but +// produced a *wrong command*: bun globals and legacy pnpm globals both keep a +// plain node_modules/ segment, so the npm step claimed them and offered +// `npm install -g ` — a duplicate under npm's prefix, shadowed on PATH by +// the original. Both returned exactly that before resolveManagerDirs expanded +// the roots. +func TestDetectSymlinkedManagerRoot(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink/PATH fixture is unix-only") + } + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + realRoot := filepath.Join(base, "real") + rootLink := filepath.Join(base, "link") + mustMkdirAll(t, realRoot) + if err := os.Symlink(realRoot, rootLink); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + env string // env var naming the root, set to the symlinked path + tool string + pkgPath []string // package binary, relative to the real root + wantArgv []string + }{ + { + name: "bun global", + env: "BUN_INSTALL", + tool: "prettier", + pkgPath: []string{"install", "global", "node_modules", "prettier", "bin", "prettier.cjs"}, + wantArgv: []string{"bun", "add", "-g", "prettier"}, + }, + { + name: "pnpm legacy global", + env: "PNPM_HOME", + tool: "tsc", + pkgPath: []string{"global", "5", "node_modules", "typescript", "bin", "tsc"}, + wantArgv: []string{"pnpm", "add", "-g", "typescript"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := filepath.Join(realRoot, tt.name) + pkgBin := filepath.Join(append([]string{root}, tt.pkgPath...)...) + mustMkdirAll(t, filepath.Dir(pkgBin)) + if err := os.WriteFile(pkgBin, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + binDir := filepath.Join(root, "bin") + mustMkdirAll(t, binDir) + if err := os.Symlink(pkgBin, filepath.Join(binDir, tt.tool)); err != nil { + t.Fatal(err) + } + // The root as a shell rc would export it: through the symlink. + t.Setenv(tt.env, filepath.Join(rootLink, tt.name)) + t.Setenv("PATH", binDir) + + plan, err := Detect(loader.Tool{Name: tt.tool}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !equalStrings(plan.Argv, tt.wantArgv) { + t.Errorf("Argv = %v, want %v", plan.Argv, tt.wantArgv) + } + }) + } +} + func TestDetectUpdateCmdOverride(t *testing.T) { // A tool with UpdateCmd set returns a custom plan even when the binary is // not on PATH — proving detection (LookPath) is skipped entirely. @@ -314,6 +906,142 @@ func TestDetectResolvesSymlink(t *testing.T) { } } +// TestDetectResolvesBunGlobal pins the Detect → resolveManagerDirs() → +// detectFromPath threading, which no pure-core row can reach: with the dirs +// argument dropped on the floor every table row above would still pass while +// the shipped binary fed a zero-value managerDirs into the core and never +// claimed a uv, pnpm or bun path again. +func TestDetectResolvesBunGlobal(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink/PATH fixture is unix-only") + } + // EvalSymlinks up front: on macOS t.TempDir() hands back a /var path that is + // itself a symlink to /private/var, and Detect compares the *resolved* + // binary path against BUN_INSTALL. + tmp, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + + // The real file: $BUN_INSTALL/install/global/node_modules//. + pkgBin := filepath.Join(tmp, "install", "global", "node_modules", "prettier", "bin", "prettier.cjs") + mustMkdirAll(t, filepath.Dir(pkgBin)) + if err := os.WriteFile(pkgBin, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + + // $BUN_INSTALL/bin/ symlinks to it, and that bin dir is the PATH. + binDir := filepath.Join(tmp, "bin") + mustMkdirAll(t, binDir) + if err := os.Symlink(pkgBin, filepath.Join(binDir, "prettier")); err != nil { + t.Fatal(err) + } + t.Setenv("BUN_INSTALL", tmp) + t.Setenv("PATH", binDir) + + plan, err := Detect(loader.Tool{Name: "prettier"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if plan.Manager != "bun" { + t.Errorf("Manager = %q, want %q", plan.Manager, "bun") + } + wantArgv := []string{"bun", "add", "-g", "prettier"} + if !equalStrings(plan.Argv, wantArgv) { + t.Errorf("Argv = %v, want %v", plan.Argv, wantArgv) + } +} + +// setTestHomeDir points homeDir at dir for the duration of the test, restoring +// the previous override afterwards (the setTestBrewPrefix shape). +func setTestHomeDir(t *testing.T, dir string) { + t.Helper() + orig := testHomeDir + testHomeDir = dir + t.Cleanup(func() { testHomeDir = orig }) +} + +// TestDetectResolvesUvTool is the only Detect-level test that reaches a manager +// root through its *default* rather than its env var, and that is the point: +// the bun and pnpm fixtures below set BUN_INSTALL/PNPM_HOME, so the `home` and +// `goos` arguments of resolveManagerDirs go unexercised there — swapping the two +// (both are strings, so it compiles) left the whole suite green until this test +// existed. +func TestDetectResolvesUvTool(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uv has no Windows default and the fixture needs symlinks") + } + home, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + // Both env overrides off, so resolution has to go through home + goos. + t.Setenv("UV_TOOL_DIR", "") + t.Setenv("XDG_DATA_HOME", "") + setTestHomeDir(t, home) + + // The verified uv layout: //bin/, behind a symlink in + // uv's own bin dir (measured against uv 0.11.32). + pkgBin := filepath.Join(home, ".local", "share", "uv", "tools", "cowsay", "bin", "cowsay") + mustMkdirAll(t, filepath.Dir(pkgBin)) + if err := os.WriteFile(pkgBin, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + binDir := filepath.Join(home, ".local", "bin") + mustMkdirAll(t, binDir) + if err := os.Symlink(pkgBin, filepath.Join(binDir, "cowsay")); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", binDir) + + plan, err := Detect(loader.Tool{Name: "cowsay"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if plan.Manager != "uv" { + t.Errorf("Manager = %q, want %q", plan.Manager, "uv") + } + wantArgv := []string{"uv", "tool", "upgrade", "cowsay"} + if !equalStrings(plan.Argv, wantArgv) { + t.Errorf("Argv = %v, want %v", plan.Argv, wantArgv) + } +} + +// TestDetectReadsPnpmShim pins the other half of the Detect wiring: the shim +// read. pnpm's bin entries are scripts, not symlinks, so without this read the +// core gets an empty shimTarget and a real pnpm global is undetectable. +func TestDetectReadsPnpmShim(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("sh shim/PATH fixture is unix-only") + } + tmp, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + + target := filepath.Join(tmp, "global", "v11", "8f3a2c1", "node_modules", "typescript", "bin", "tsc") + binDir := filepath.Join(tmp, "bin") + mustMkdirAll(t, binDir) + shim := "#!/bin/sh\nexec node \"" + target + "\" \"$@\"\n# cmd-shim-target=" + target + "\n" + if err := os.WriteFile(filepath.Join(binDir, "tsc"), []byte(shim), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PNPM_HOME", tmp) + t.Setenv("PATH", binDir) + + plan, err := Detect(loader.Tool{Name: "tsc"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if plan.Manager != "pnpm" { + t.Errorf("Manager = %q, want %q", plan.Manager, "pnpm") + } + wantArgv := []string{"pnpm", "add", "-g", "typescript"} + if !equalStrings(plan.Argv, wantArgv) { + t.Errorf("Argv = %v, want %v", plan.Argv, wantArgv) + } +} + func TestCargoCrateFromList(t *testing.T) { list := "" + "exa v0.10.1:\n" + From 095336aec5108e1a57472083f6baa9afbe919bb7 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Tue, 28 Jul 2026 01:07:49 +0300 Subject: [PATCH 3/3] fix(updater): refuse a bare scope as a package name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit npmPackage returned the scope itself when nothing followed it, so .../node_modules/@angular yielded `pnpm add -g @angular` — a wrong command where the chain owes an honest miss. Reachable in practice: the pnpm step feeds npmPackage a cmd-shim target, which is file content nothing validated. Also correct the README. It claimed a layout no check recognises always degrades to the update_cmd hint, "never a wrong command". That is not true and this branch is where it stops being true: pnpm and bun globals sit behind node_modules paths, so an unresolved root still reads as a plain npm install and the offer becomes `npm install -g `. Say so, and say which env vars keep it from happening. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 2 +- README.md | 14 ++++++++++++-- internal/updater/updater.go | 9 ++++++++- internal/updater/updater_test.go | 19 +++++++++++++++++++ 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9dc589e..5e65b97 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -92,7 +92,7 @@ The model is a three-panel layout with focus cycling via `→/←` between `focu - **Card versions**: `renderCard`'s `[info]` section shows `installed:` from `m.versions[name].Installed` — all four states (`installed: \uf412 `, `installed: detecting…`, `installed: ✓ no version`, `installed: ✕ not installed`) in the section's muted `InfoStyle`. The two version-less states split what `InstalledKnown` alone used to collapse, on `InstalledPresent`: a tool that is installed but won't name its version (a ratatui app that ignores `--version`) is a working install and reads `✓` (U+2713) in `ui.OkStyle` — green, deliberately not `UpdateAvailableStyle`, whose orange means "act on this" — while a genuine absence keeps the `✕` (U+2715) in `DangerStyle`, the same red glyph/style pair the `[L]` overlay uses for exhaustion. The section's gate includes `installed != ""`, so a locally installed tool with no `GitHub` ref still renders it. When `hasUpdate(name)` (installed older than latest per `version.IsNewer`) the `latest:` value renders in `UpdateAvailableStyle` with a ` ↑` suffix — the same glyph the tools list appends to the row. The `latest:` line is gated on `card.Latest != ""` (a repo with no release shows no line) and appends the release date as a ` (YYYY-MM-DD)` suffix derived from `card.PublishedAt`. **Both version lines carry the `\uf412` glyph** (nf-oct-tag) before the version — its meaning is "a version", not specifically "a release tag", which is why it widened from `latest:` to `installed:`; the three states with no version to tag (`detecting…`, `✓ no version`, `✕ not installed`) stay bare. Written as a `\u` escape in source and prose alike: the raw glyph is invisible in most editors and diffs and gets silently lost. Versions live in the card only; the 30-column list never shows version strings. - **Panel titles**: all three panels inset a title into their top border (`┌─ [1] Tools ─…─┐`) via the shared `insetPanelTitle` (render.go) — an ANSI-safe splice over the already-rendered frame (`ui`'s `truncateVisible` is unexported; the helper remeasures with `stripANSI` and rebuilds the top line), colored like the border (focus-aware) and dropped whole when the panel is too narrow. The titles are `[1] Tools`, `[2] Brief` and `[3] Readme` / `[3] Help` / `[3] Man` (a `switch m.helpMode`, overridden by `[3] Update` while a live log shows) — they double as the documentation for the digit focus hotkeys, so the status bar carries no digit hints. All title characters are single-width and non-East-Asian-Ambiguous, keeping the border width math stable. - **Refresh (`r` in `focusBrief`)**: `refreshSelectedCmd(t)` force-refreshes the selected tool bypassing the 24h cache TTL — the repo pass (`refreshRemoteCmd` → `version.RefreshRepoData`) + changelog (`refreshChangelogCmd` → `version.RefreshChangelog`) + README (`refreshReadmeCmd` → `version.RefreshReadme`, preceded by a `delete(m.readmeData, name)` so a session-cached 404/rate-limit negative can recover, then a `markReadmeLoading(name)` — the deletion makes `needsReadme` true again for the whole in-flight window, so without the marker leaving and re-entering the tool would spend a second request; `refreshingFor` does *not* cover it, since `remoteMsg` clears that flag as soon as the repo pass lands, which can be well before the README does) + a local installed re-detect (`fetchInstalledCmd`). It emits the same `remoteMsg`/`changelogMsg` as the startup path, so the merge/re-render logic is reused. While the repo pass is in flight `m.refreshingFor` (the tool name) turns the card title into a status line — `refreshing data ` (`bubbles/spinner`, `MiniDot`; the about is hidden) — with no status-bar takeover; the `remoteMsg` handler clears `refreshingFor` on completion, which reverts the title to name+about and halts the `spinner.TickMsg` loop. `refreshingFor` doubles as the double-press guard; a tool with no `GitHub` only re-detects the installed version (`m.statusMsg = "no repo to refresh"`, no spinner). Note the same `case "r"` branches on focus three ways: rename in `focusTools`, refresh in `focusBrief`, README mode in `focusHelp`. -- **Update (`u` in `focusBrief`)**: installs a newer release from inside the TUI. `[u]` fires only in `focusBrief` (in `focusTools` `u` stays untrack — same `case "u"`, branched on focus); it requires `hasUpdate(name)` (else `statusMsg`) and reports the shared `updateBusyStatus` (`another update is running`) while `updatingFor != ""` instead of starting a second one (one update at a time, no queue; the same wording `[U]` uses, since the running update's only other sign is a card spinner invisible unless that tool is selected). The key fires `detectUpdateCmd(t, false)` — detection spawns subprocesses (`go version -m`, `cargo install --list`) so it must never run inside `Update()`, same as every other probe. `updater.Detect` runs the brew → go → cargo → pipx → uv → pnpm → bun → npm chain. **Order is load-bearing twice**: brew before go, so a brew-installed Go binary with buildinfo isn't misrouted, and pnpm/bun before npm, because both layouts carry `node_modules` segments the npm step claims on sight — a bun global (`$BUN_INSTALL/bin/` → `install/global/node_modules//…`) really did resolve to `npm install -g `, the wrong manager, installing a duplicate under npm's prefix that the bun copy keeps shadowing on `PATH`. Five steps are **path-convention** based (cargo, pipx, uv, pnpm, bun), and an empty root switches its own step off — enforced **inside `underDir`/`segmentUnder`**, which answer "no match" for an empty dir, deliberately *not* by a `!= ""` guard repeated at each step. The hazard is real: `filepath.Rel("", "bin/exa")` succeeds and yields `bin/exa`, so a **relative** path reads as living under every disabled root, and one forgotten copy of the guard silently claims it. It lived as six hand-copied copies of that one convention until the review that turned it into one definition — the same lesson as `version.applyReleaseOutcome` (`TestPathHelpersRejectEmptyDir`, plus a per-step `empty leaves a relative path undetected` row): `uv` = `segmentUnder(realPath, uvTools)` → `uv tool upgrade `; `pnpm` = `underDir(realPath, pnpmHome)` plus a name from the shim target or the resolved path → `pnpm add -g `; `bun` = `underDir(realPath, bunInstall)` + `npmPackage(realPath)` → `bun add -g `. **`add -g`, not `update -g`**: `update` honours the semver range saved at install time and can silently refuse a major bump, while keepkit promises the version the card shows as `latest:`. A manager's own binary carries no `node_modules` segment and no shim target, so it falls through — `bun upgrade`/`pnpm self-update` are deliberately out of scope. **All five roots** come from **`managerDirsFrom(getenv, home, goos)`** (pure core, `resolveManagerDirs()` wrapper — the `launcher.planFor` idiom), and carrying cargo/pipx there too is what lets `detectFromPath` stop calling `homeDir()`, making its "no I/O, no environment" contract literally true instead of nearly true. cargo and pipx stay home-derived with **no env var of their own** — `$CARGO_HOME`/`$PIPX_HOME` are not consulted, exactly as before the struct existed, since honouring them changes behaviour for anyone who sets them and belongs in its own commit. `/.cargo/bin` and `/.local/pipx/venvs`; `$UV_TOOL_DIR` else `$XDG_DATA_HOME/uv/tools` else `~/.local/share/uv/tools` (macOS included — uv is XDG there too; **Windows env-only**, its layout is unverified and a wrong guess beats no guess only in the wrong direction), `$PNPM_HOME` else `~/Library/pnpm` (darwin) / `$XDG_DATA_HOME/pnpm` else `~/.local/share/pnpm` (linux) / `%LOCALAPPDATA%\pnpm` (windows), `$BUN_INSTALL` else `~/.bun`. An empty field is a *disabled* check, which is what keeps the zero-value `managerDirs{}` backwards compatible. The wrapper then **expands symlinks in every one of the five roots** (`resolveDir` → `filepath.EvalSymlinks`, falling back to the raw path when it does not resolve — a root that does not exist yet is the normal state for an uninstalled manager and must stay a harmless *non-match* rather than becoming an empty, i.e. disabled, field). That expansion is load-bearing, not hygiene: `Detect` compares these roots against an `EvalSymlinks`-**resolved** binary path, so a root carrying any symlink component — a relocated `~/.bun` → `/mnt/big/bun`, a home on a secondary volume, `/home` under autofs, and on macOS `/var` → `/private/var`, which is what made every fixture test here need `EvalSymlinks(t.TempDir())` — silently fails to match. For uv and pnpm's shim/store layouts that only costs the update offer, but a bun global and a legacy pnpm one still carry a plain `node_modules/` segment, so the npm step claims them and offers `npm install -g ` — the exact duplicate-install misdetection this chain exists to prevent, measured on all five layouts and pinned by `TestDetectSymlinkedManagerRoot`. The expansion is a **loop over pointers to all five fields**, so a sixth field added to `managerDirs` but forgotten there would resolve to a root nothing can ever match; `TestResolveManagerDirsExpandsSymlinks` asserts all five at once as the guard. **pnpm needs a second signal** because its globals are not symlinks at all: `$PNPM_HOME/bin/` is a cmd-shim `/bin/sh` script `EvalSymlinks` resolves to itself, whose last line — `# cmd-shim-target=` — is the only machine-readable link to the owning package. `Detect` reads it (best-effort, capped at `pnpmShimMaxBytes` = 8 KiB against real shims of ~1.5 KiB, and **only** when the found path sits under a non-empty `pnpmHome` — reading every binary on `PATH` to look for a comment would be an open per detection) through `readPnpmShim` → the pure `pnpmShimTarget` (last marker wins: cmd-shim writes it as the final line). A file **over** the cap is rejected whole (`LimitReader(cap+1)` + a length check, `getReadme`'s idiom) rather than parsed truncated, and that is a correctness guard, not tidiness: a cut landing inside the marker line hands the parser a *shortened* path, and `…/node_modules/typescript/…` shortened to `…/node_modules/types` yields `types` — a real package on npm, so the chain would offer a confidently wrong `pnpm add -g types` where everything else here degrades honestly (`TestReadPnpmShim`'s oversized row, whose fixture asserts the truncated parse *would* have said `types`), and passes it into the core as `shimTarget` exactly the way `goBuildinfo` rides in; an unreadable file yields `""`, i.e. the check simply has no signal. Two npm-side consequences of the pnpm layout: `npmPackage` **skips a post-`node_modules` segment starting with `.`** (`.pnpm` is the virtual store, `.bin` the shim dir — never package names) and keeps scanning, and the **npm step refuses any path through `/node_modules/.pnpm/`** — such a path reaching npm means a pnpm layout the pnpm step failed to attribute, and `npm install -g ` there is the duplicate-install failure again, so the chain falls through to `ErrUnknownManager` + the `update_cmd` hint (honest degradation). Detection is convention-based end to end: a future layout change makes its check miss *silently* and degrade to the hint, never to a wrong command. A `update_cmd` in `meta.yaml` always wins and runs via `sh -c` (skips detection entirely). The chain starts from `exec.LookPath(t.Name)`, and a **miss there is not the end**: the tracked name can still be a brew formula whose binaries are named differently — `rust` ships `rustc`/`cargo`, so `LookPath("rust")` misses while `brew upgrade rust` is exactly right — so `brewNamePlan` checks for a `Cellar/`/`Caskroom/` directory before `ErrUnknownManager` is returned. **An exhausted chain takes the same fallback**: a binary can be found and still belong to no known manager — a Homebrew cask app keeps its launcher on `PATH` while the executable lives inside the `.app` bundle (`agterm`), matching neither the Cellar regex nor any later step — so `Detect` retries `brewNamePlan` on `ErrUnknownManager` from `detectFromPath` too (`TestDetectBrewByNameChainExhausted`). Both sites are self-validating (they fire only when such a keg actually exists) and share `version.brewDirVersion`'s traversal guard: a name carrying `/` or `\` can't be a formula name and must not turn the `Join` into a traversal. The `updateDetectedMsg` handler drops a stale result through the shared predicate **`acceptsUpdateDetect(msg)`**: both paths refuse while `updatingFor != ""` and while `m.mode != modeNormal` (detection spawns subprocesses and can answer seconds later — a confirm dialog opening under an editor, a search or an overlay steals the keystroke aimed at it, the mirror of `launchDoneMsg`'s mode gate), and beyond that a *tool* result must still match the selection while keepkit's own has no selection to match. It maps `ErrUnknownManager` to a `statusMsg` hint (`no known updater for — set update_cmd or [o] releases` — no dead-end dialog; the wording branches on **`isSelfUpdate(msg.tool)`**, not on `msg.self`, so the identical failure of the identical binary reads the same from `[u]` on a tracked `keepkit` row as from `[U]` — `msg.self` keeps the one meaning only it has, "no selection to match", inside `acceptsUpdateDetect`), and on success stores `m.updatePlan` plus **`m.updateTarget = msg.tool`** and enters `modeConfirmUpdate`. The target is resolved *there*, not when enter is pressed: a selection that moved while detection ran can no longer retarget the dialog, and keepkit's own update — which has no row, and no selection at all with an empty tracker — needs no second identity. The confirm status bar shows `update : [enter] run [esc] cancel`; `enter` sets `m.updatingFor`/`m.updateLogFor` to that target, resets the log, and fires `startUpdateCmd` (any other key cancels, not just `esc`, and clears `updateTarget` with the plan it named). The `[u] update` hint is prepended to the `focusBrief` bar only when `hasUpdate(selected)`. +- **Update (`u` in `focusBrief`)**: installs a newer release from inside the TUI. `[u]` fires only in `focusBrief` (in `focusTools` `u` stays untrack — same `case "u"`, branched on focus); it requires `hasUpdate(name)` (else `statusMsg`) and reports the shared `updateBusyStatus` (`another update is running`) while `updatingFor != ""` instead of starting a second one (one update at a time, no queue; the same wording `[U]` uses, since the running update's only other sign is a card spinner invisible unless that tool is selected). The key fires `detectUpdateCmd(t, false)` — detection spawns subprocesses (`go version -m`, `cargo install --list`) so it must never run inside `Update()`, same as every other probe. `updater.Detect` runs the brew → go → cargo → pipx → uv → pnpm → bun → npm chain. **Order is load-bearing twice**: brew before go, so a brew-installed Go binary with buildinfo isn't misrouted, and pnpm/bun before npm, because both layouts carry `node_modules` segments the npm step claims on sight — a bun global (`$BUN_INSTALL/bin/` → `install/global/node_modules//…`) really did resolve to `npm install -g `, the wrong manager, installing a duplicate under npm's prefix that the bun copy keeps shadowing on `PATH`. Five steps are **path-convention** based (cargo, pipx, uv, pnpm, bun), and an empty root switches its own step off — enforced **inside `underDir`/`segmentUnder`**, which answer "no match" for an empty dir, deliberately *not* by a `!= ""` guard repeated at each step. The hazard is real: `filepath.Rel("", "bin/exa")` succeeds and yields `bin/exa`, so a **relative** path reads as living under every disabled root, and one forgotten copy of the guard silently claims it. It lived as six hand-copied copies of that one convention until the review that turned it into one definition — the same lesson as `version.applyReleaseOutcome` (`TestPathHelpersRejectEmptyDir`, plus a per-step `empty leaves a relative path undetected` row): `uv` = `segmentUnder(realPath, uvTools)` → `uv tool upgrade `; `pnpm` = `underDir(realPath, pnpmHome)` plus a name from the shim target or the resolved path → `pnpm add -g `; `bun` = `underDir(realPath, bunInstall)` + `npmPackage(realPath)` → `bun add -g `. **`add -g`, not `update -g`**: `update` honours the semver range saved at install time and can silently refuse a major bump, while keepkit promises the version the card shows as `latest:`. A manager's own binary carries no `node_modules` segment and no shim target, so it falls through — `bun upgrade`/`pnpm self-update` are deliberately out of scope. **All five roots** come from **`managerDirsFrom(getenv, home, goos)`** (pure core, `resolveManagerDirs()` wrapper — the `launcher.planFor` idiom), and carrying cargo/pipx there too is what lets `detectFromPath` stop calling `homeDir()`, making its "no I/O, no environment" contract literally true instead of nearly true. cargo and pipx stay home-derived with **no env var of their own** — `$CARGO_HOME`/`$PIPX_HOME` are not consulted, exactly as before the struct existed, since honouring them changes behaviour for anyone who sets them and belongs in its own commit. `/.cargo/bin` and `/.local/pipx/venvs`; `$UV_TOOL_DIR` else `$XDG_DATA_HOME/uv/tools` else `~/.local/share/uv/tools` (macOS included — uv is XDG there too; **Windows env-only**, its layout is unverified and a wrong guess beats no guess only in the wrong direction), `$PNPM_HOME` else `~/Library/pnpm` (darwin) / `$XDG_DATA_HOME/pnpm` else `~/.local/share/pnpm` (linux) / `%LOCALAPPDATA%\pnpm` (windows), `$BUN_INSTALL` else `~/.bun`. An empty field is a *disabled* check, which is what keeps the zero-value `managerDirs{}` backwards compatible. The wrapper then **expands symlinks in every one of the five roots** (`resolveDir` → `filepath.EvalSymlinks`, falling back to the raw path when it does not resolve — a root that does not exist yet is the normal state for an uninstalled manager and must stay a harmless *non-match* rather than becoming an empty, i.e. disabled, field). That expansion is load-bearing, not hygiene: `Detect` compares these roots against an `EvalSymlinks`-**resolved** binary path, so a root carrying any symlink component — a relocated `~/.bun` → `/mnt/big/bun`, a home on a secondary volume, `/home` under autofs, and on macOS `/var` → `/private/var`, which is what made every fixture test here need `EvalSymlinks(t.TempDir())` — silently fails to match. For uv and pnpm's shim/store layouts that only costs the update offer, but a bun global and a legacy pnpm one still carry a plain `node_modules/` segment, so the npm step claims them and offers `npm install -g ` — the exact duplicate-install misdetection this chain exists to prevent, measured on all five layouts and pinned by `TestDetectSymlinkedManagerRoot`. The expansion is a **loop over pointers to all five fields**, so a sixth field added to `managerDirs` but forgotten there would resolve to a root nothing can ever match; `TestResolveManagerDirsExpandsSymlinks` asserts all five at once as the guard. **pnpm needs a second signal** because its globals are not symlinks at all: `$PNPM_HOME/bin/` is a cmd-shim `/bin/sh` script `EvalSymlinks` resolves to itself, whose last line — `# cmd-shim-target=` — is the only machine-readable link to the owning package. `Detect` reads it (best-effort, capped at `pnpmShimMaxBytes` = 8 KiB against real shims of ~1.5 KiB, and **only** when the found path sits under a non-empty `pnpmHome` — reading every binary on `PATH` to look for a comment would be an open per detection) through `readPnpmShim` → the pure `pnpmShimTarget` (last marker wins: cmd-shim writes it as the final line). A file **over** the cap is rejected whole (`LimitReader(cap+1)` + a length check, `getReadme`'s idiom) rather than parsed truncated, and that is a correctness guard, not tidiness: a cut landing inside the marker line hands the parser a *shortened* path, and `…/node_modules/typescript/…` shortened to `…/node_modules/types` yields `types` — a real package on npm, so the chain would offer a confidently wrong `pnpm add -g types` where everything else here degrades honestly (`TestReadPnpmShim`'s oversized row, whose fixture asserts the truncated parse *would* have said `types`), and passes it into the core as `shimTarget` exactly the way `goBuildinfo` rides in; an unreadable file yields `""`, i.e. the check simply has no signal. Two npm-side consequences of the pnpm layout: `npmPackage` **refuses a bare scope** (`…/node_modules/@angular` with nothing after it is a directory holding packages, not one — returning it offered `pnpm add -g @angular`, and the pnpm step feeds this function a *shim target*, i.e. file content nothing validated) and **skips a post-`node_modules` segment starting with `.`** (`.pnpm` is the virtual store, `.bin` the shim dir — never package names) and keeps scanning, and the **npm step refuses any path through `/node_modules/.pnpm/`** — such a path reaching npm means a pnpm layout the pnpm step failed to attribute, and `npm install -g ` there is the duplicate-install failure again, so the chain falls through to `ErrUnknownManager` + the `update_cmd` hint (honest degradation). Detection is convention-based end to end: a future layout change makes its check miss *silently* and degrade to the hint, never to a wrong command. A `update_cmd` in `meta.yaml` always wins and runs via `sh -c` (skips detection entirely). The chain starts from `exec.LookPath(t.Name)`, and a **miss there is not the end**: the tracked name can still be a brew formula whose binaries are named differently — `rust` ships `rustc`/`cargo`, so `LookPath("rust")` misses while `brew upgrade rust` is exactly right — so `brewNamePlan` checks for a `Cellar/`/`Caskroom/` directory before `ErrUnknownManager` is returned. **An exhausted chain takes the same fallback**: a binary can be found and still belong to no known manager — a Homebrew cask app keeps its launcher on `PATH` while the executable lives inside the `.app` bundle (`agterm`), matching neither the Cellar regex nor any later step — so `Detect` retries `brewNamePlan` on `ErrUnknownManager` from `detectFromPath` too (`TestDetectBrewByNameChainExhausted`). Both sites are self-validating (they fire only when such a keg actually exists) and share `version.brewDirVersion`'s traversal guard: a name carrying `/` or `\` can't be a formula name and must not turn the `Join` into a traversal. The `updateDetectedMsg` handler drops a stale result through the shared predicate **`acceptsUpdateDetect(msg)`**: both paths refuse while `updatingFor != ""` and while `m.mode != modeNormal` (detection spawns subprocesses and can answer seconds later — a confirm dialog opening under an editor, a search or an overlay steals the keystroke aimed at it, the mirror of `launchDoneMsg`'s mode gate), and beyond that a *tool* result must still match the selection while keepkit's own has no selection to match. It maps `ErrUnknownManager` to a `statusMsg` hint (`no known updater for — set update_cmd or [o] releases` — no dead-end dialog; the wording branches on **`isSelfUpdate(msg.tool)`**, not on `msg.self`, so the identical failure of the identical binary reads the same from `[u]` on a tracked `keepkit` row as from `[U]` — `msg.self` keeps the one meaning only it has, "no selection to match", inside `acceptsUpdateDetect`), and on success stores `m.updatePlan` plus **`m.updateTarget = msg.tool`** and enters `modeConfirmUpdate`. The target is resolved *there*, not when enter is pressed: a selection that moved while detection ran can no longer retarget the dialog, and keepkit's own update — which has no row, and no selection at all with an empty tracker — needs no second identity. The confirm status bar shows `update : [enter] run [esc] cancel`; `enter` sets `m.updatingFor`/`m.updateLogFor` to that target, resets the log, and fires `startUpdateCmd` (any other key cancels, not just `esc`, and clears `updateTarget` with the plan it named). The `[u] update` hint is prepended to the `focusBrief` bar only when `hasUpdate(selected)`. - **Streaming** (channel + re-subscribe idiom, no `*tea.Program`): `startUpdateCmd` runs the plan via `exec.Command` + `proc.DetachTTY` (10-min deadline; a sudo prompt fails fast instead of hanging — deliberate), with stdout+stderr merged into one pipe. **Reader ordering is load-bearing** (os/exec forbids `Wait` before pipe reads finish): the goroutine scans the pipe to EOF via `streamLines` → then `cmd.Wait()` → sends the exit error as a final `updateLine{done:true, err}` → then `close(ch)`. `waitForChunkCmd` does one receive → `updateChunkMsg`; a done item or closed channel → `updateDoneMsg`. The channel carries a typed `updateLine{text, replace, done, err}` (not `chan string`) so the `replace` flag and completion error ride the same channel — no second error channel threaded through every re-subscribe. Each segment is sanitized through `cleanTerminalOutput` (which already strips ANSI) at the boundary; `streamLines` splits on `\n` **and** `\r`, and a `\r` segment sets `replace` so brew/npm progress bars collapse to one updating line. `m.updateLog` is capped at ~500 lines (tail matters). On deadline, `proc.KillGroup` SIGKILLs the process group (negative pid — `DetachTTY`'s `Setsid` makes the child a session leader, so a plain kill would orphan `sh -c` grandchildren). - **Live log in `[3]`**: `m.updateLog []string` is a single active-session buffer (not a map); `m.updateLogFor` names its tool. Every site that asks "does the log own panel `[3]`?" asks the **single predicate `showsUpdateLog()`** (see **Self-update**), never `updateLogFor` directly: `renderHelpContent()` returns the log **ahead of** the `helpLoadingFor`/cache branches, and `autoFetchCmdsForSelected` skips the help fetch (and `helpLoadingFor` set) — otherwise re-selecting the tool paints `Loading...` or a late `helpOutputMsg` clobbers the live log. The panel title reads `[3] Update` while the log shows (same `insetPanelTitle` path as `[3] Help`/`[3] Man`), autoscrolls to bottom on each chunk, and the buffer persists after completion until the next update. For a *tool* update the claim is per tool: navigating away shows the other tool's normal help; back shows the live log. - **Spinner + completion**: `m.updatingFor` twins `refreshingFor` — card title `updating `; the `spinner.TickMsg` gate is `refreshingFor != "" || updatingFor != ""` (or the spinner freezes after one frame). The `updateDoneMsg` handler clears `updatingFor`; success → `statusMsg "updated "` + `fetchInstalledCmd(t)` (the version merge extinguishes `↑` and the existing by-name cursor remap moves the tool out of the update group); failure → `statusMsg "update failed — see [3]"` + **`recordUpdateFailure(msg)`**, the helper shared with the self path that seeds the log with the exit error when the command produced no output at all (otherwise `[3]` would still read `starting update…` while the status bar points there) and writes the `logx.Errorf` line (manager, exit code, last log lines — never the token). One definition, so the two paths cannot drift in log format. A tool untracked mid-update just clears `updatingFor` (no re-fetch, no crash). diff --git a/README.md b/README.md index 09bf64c..e2d5f3d 100644 --- a/README.md +++ b/README.md @@ -168,8 +168,18 @@ conventions, so on **Windows** their bins are `.cmd` shims keepkit does not pars a pre-existing npm limitation the three new managers inherit; on macOS and Linux pnpm's shims are `#!/bin/sh` scripts, which keepkit does read. And a manager's **own** binary is out of scope: `bun upgrade`, `pnpm self-update` and friends are not offered, -those tools update themselves. Anything a check misses degrades the same way as an -unknown manager — the `update_cmd` hint below, never a wrong command. +those tools update themselves. A layout no check recognises normally degrades to the +same `update_cmd` hint as an unknown manager. + +One case does not, and it is worth knowing: pnpm and bun globals sit behind +`node_modules` paths, so if keepkit cannot work out where their root is, the path +still looks like an ordinary npm install and the offer becomes +`npm install -g `. Running it would install a second copy under npm's +prefix while the original keeps shadowing it on `PATH`. keepkit reads `$PNPM_HOME` +and `$BUN_INSTALL` (and the platform defaults) and expands symlinks in them, so this +only bites when the root is somewhere the environment does not name — a store moved +by a config file, or a session started without your shell profile. Export the +variable, or set `update_cmd` on the tool, and the right manager is picked. The command is shown in the status bar for confirmation; its output streams into panel `[3] Update` in real time and the TUI stays responsive. After a successful diff --git a/internal/updater/updater.go b/internal/updater/updater.go index 7d54b5b..e58b88a 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -584,7 +584,14 @@ func npmPackage(p string) string { if strings.HasPrefix(pkg, ".") { continue // service dir (.pnpm, .bin) — keep scanning } - if strings.HasPrefix(pkg, "@") && i+2 < len(parts) { + if strings.HasPrefix(pkg, "@") { + // A scope alone names no package: "@angular" is a directory holding + // them. Returning it produced `pnpm add -g @scope`, a wrong command + // where the chain owes an honest miss — and it is reachable, because + // one caller passes a shim target, i.e. unvalidated file content. + if i+2 >= len(parts) || parts[i+2] == "" { + return "" + } return pkg + "/" + parts[i+2] } return pkg diff --git a/internal/updater/updater_test.go b/internal/updater/updater_test.go index 520aec2..29c04bf 100644 --- a/internal/updater/updater_test.go +++ b/internal/updater/updater_test.go @@ -116,6 +116,16 @@ func TestDetectFromPath(t *testing.T) { wantManager: "pnpm", wantArgv: []string{"pnpm", "add", "-g", "@angular/cli"}, }, + { + // The reachable half of the bare-scope hole: a shim target is file + // content nothing validated, and a target ending at the scope used to + // come back as `pnpm add -g @angular`. + name: "pnpm shim target naming a bare scope", + realPath: filepath.Join(pnpmHome, "bin", "ng"), + shimTarget: filepath.Join(pnpmHome, "global", "v11", "1d0e9b4", "node_modules", "@angular"), + dirs: managerDirs{pnpmHome: pnpmHome}, + wantErr: ErrUnknownManager, + }, { // Legacy layouts symlink into the store, so the resolved path itself // names the package and no shim target is involved. @@ -384,6 +394,15 @@ func TestNpmPackage(t *testing.T) { "@angular/cli", }, {"only a .bin shim dir", "/usr/local/lib/node_modules/.bin/tsc", ""}, + // A scope with nothing after it is a directory, not a package. It used to + // come back as "@angular", i.e. `npm install -g @angular`. + {"bare scope", "/usr/local/lib/node_modules/@angular", ""}, + {"bare scope with a trailing slash", "/usr/local/lib/node_modules/@angular/", ""}, + { + "bare scope inside the pnpm store", + "/home/t/.local/share/pnpm/global/5/node_modules/.pnpm/@scope+cli@1.0.0/node_modules/@scope", + "", + }, {"no node_modules segment", "/usr/local/bin/tsc", ""}, {"node_modules is the last segment", "/usr/local/lib/node_modules", ""}, }