From cfc90ea6d908713f3a6984219f0c5a039741fd57 Mon Sep 17 00:00:00 2001 From: REPPL Date: Wed, 12 Aug 2026 12:19:52 +0200 Subject: [PATCH 1/2] feat: abcd installs where the user can write, and the detector finds it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PATH detector recognised exactly one blessed target, /usr/local/bin/abcd, so a working ~/.local/bin/abcd — the single-user location the uv/pipx/rustup class of tools uses — reported symlink.missing while the detector was itself running as that very binary. Letting install "fix" it would have written a symlink to /abcd without checking the target exists: a dangling link early on PATH shadows every working abcd behind it. Detection now scans PATH in order, resolves each hit through the same seam as iss-170 (resolvePath -> EvalSymlinks), and classifies it dev-shim / owned / foreign. An abcd-owned entry anywhere on PATH is the install, adopted where it stands. Two states that were invisible get named: an owned entry whose binary has gone is symlink.dangling, and an install directory absent from PATH is path.bin_dir_not_on_path — required but NOT resolvable, carrying the one-line export fix. abcd prints that line and never edits a shell profile. Install defaults to ~/.local/bin, created when absent. A system-wide directory is reachable only through an explicit --bin-dir, whose writability is probed before any write and whose failure is an error rather than a silent skip: abcd escalates nothing, so there is no privileged fallback to hide a refusal behind. Creating a symlink whose target does not exist is refused outright, with the reason carried on a new `notes` channel — a refusal that shows up only as a still-open gap reads as a silent failure. The two print sites outside the install receipt's own note seam render user-scope paths in tilde form, so the doctor gap text and the uninstall receipt carry the location without carrying the username. Both can be unified onto the receipt scrub's primitive once that lands. The README one-liner drops sudo and installs to ~/.local/bin, which is the breaking change: the documented install location moves. An existing /usr/local/bin/abcd keeps working — the detector adopts it. Resolves iss-171. Assisted-by: Claude:claude-opus-5[1m] --- .../brief/02-constraints/04-naming.md | 2 +- .../development/brief/04-surfaces/01-ahoy.md | 30 +- .abcd/development/release/surface.json | 7 + ...ctor-and-installer-assume-one-blessed-l.md | 2 + CHANGELOG.md | 22 ++ README.md | 32 +- commands/ahoy.md | 23 +- docs/reference/cli/commands.md | 1 + internal/core/ahoy/ahoy.go | 10 + internal/core/ahoy/apply.go | 161 +++++++- internal/core/ahoy/bin_install_test.go | 372 ++++++++++++++++++ internal/core/ahoy/detect.go | 113 ++++-- internal/core/ahoy/fsutil.go | 29 ++ internal/core/ahoy/store.go | 152 ++++++- internal/surface/cli/ahoy_bin_install_test.go | 87 ++++ internal/surface/cli/cli.go | 14 +- 16 files changed, 988 insertions(+), 69 deletions(-) rename .abcd/work/issues/{open => resolved}/iss-171-the-path-install-detector-and-installer-assume-one-blessed-l.md (73%) create mode 100644 internal/core/ahoy/bin_install_test.go create mode 100644 internal/surface/cli/ahoy_bin_install_test.go diff --git a/.abcd/development/brief/02-constraints/04-naming.md b/.abcd/development/brief/02-constraints/04-naming.md index be59cba0..51a93147 100644 --- a/.abcd/development/brief/02-constraints/04-naming.md +++ b/.abcd/development/brief/02-constraints/04-naming.md @@ -6,7 +6,7 @@ Commands and abcd-owned directories use ship/voyage metaphors where natural. |---|---| | `/abcd:ahoy` | bare invocation — status + help: shows folder kind, install state, detected gaps, last install date. ZERO writes. | | `/abcd:ahoy install` | mutating sub-verb — applies detected gaps (skeleton, config-change, history-store, marker-block, PATH symlink, version stamp). Centralised per-category approval. | -| `/abcd:ahoy uninstall` | reversible removal — strips the marker block from `CLAUDE.md` / `AGENTS.md` and the `/usr/local/bin/abcd` symlink if owned. Preserves `.abcd/`, `~/.abcd/`, and `hooks/hooks.json`. | +| `/abcd:ahoy uninstall` | reversible removal — strips the marker block from `CLAUDE.md` / `AGENTS.md` and abcd's own `PATH` entry (`~/.local/bin/abcd` by default) if owned. Preserves `.abcd/`, `~/.abcd/`, and `hooks/hooks.json`. | | `/abcd:ahoy dry-run` | read-only emit of the `DetectionResult` envelope as JSON. ZERO writes. Drives the Claude Code skill's two-pass approval protocol. | | `/abcd:ahoy doctor` | read-only audit — detection envelope + cross-machine `history_audit.audit_repos()` gaps. ZERO writes. | | `/abcd:disembark` | leave the ship → pack a lifeboat for the journey | diff --git a/.abcd/development/brief/04-surfaces/01-ahoy.md b/.abcd/development/brief/04-surfaces/01-ahoy.md index 289587f5..4362c40c 100644 --- a/.abcd/development/brief/04-surfaces/01-ahoy.md +++ b/.abcd/development/brief/04-surfaces/01-ahoy.md @@ -17,7 +17,7 @@ the CLI for anything that writes; the sub-verbs below ship on the CLI as (idempotent; covers first-install and upgrade). Runs the detection pass, then the apply pass over the resulting gaps. - **`/abcd:ahoy uninstall`** — reversible marker-only removal: removes the - marker block and the `/usr/local/bin/abcd` symlink (if owned by this plugin). + marker block and abcd's own `PATH` entry (if owned by this plugin). NEVER mutates `hooks/hooks.json` (plugin-static per spc-14 T7 + spc-16 T1). Leaves `.abcd/` intact. Re-running `install` re-installs cleanly. - **`/abcd:ahoy dry-run`** — run the detection pass and render the @@ -158,8 +158,14 @@ Steps, run in parallel where independent: against the current template; classify `current` / `outdated` / `missing`. The marker block stands alone — there is no parent-`CLAUDE.md` reference to verify. -8. **PATH symlink** — does `/usr/local/bin/abcd` exist, and does it point at - this plugin, a different binary, or nothing? +8. **PATH entry** — scan `PATH` for `abcd`, resolving symlinks, and classify each + hit as this plugin's own entry, our dev shim, or a foreign binary. An + abcd-owned entry anywhere on `PATH` is the install. With none, the default + location `~/.local/bin/abcd` answers the same question — present, pointing at + this plugin, at a different binary, or at nothing. An owned entry whose target + has gone is `symlink.dangling`; an install directory absent from `PATH` is + `path.bin_dir_not_on_path`, required but not resolvable — abcd prints the + one-line `export` fix and never edits a shell profile (iss-171). 9. **Hook manifest verification** (verify-only per spc-16 T1) — VERIFY that `hooks/hooks.json` is present in the plugin install AND contains the three required event entries (`UserPromptSubmit`, `SessionStart`, `PreCompact`) @@ -297,11 +303,16 @@ interactive confirmation. users edit outside the markers.** Content comes from `internal/core/ahoy/defaults/claude-md-marker-block.md`. Write the minimal `.abcd/rules.json` skeleton if missing. -9. **PATH symlink** (`config-change`) — transparent prompt: "Install `abcd` - symlink to `/usr/local/bin/abcd`? Default: yes for private repos, no for - public." If accepted AND the target is absent or already points at this - plugin → write it. If a different `abcd` binary exists → refuse, show what - it points to, suggest manual resolution. +9. **PATH entry** (`config-change`) — transparent prompt: "Install `abcd` + symlink to `~/.local/bin/abcd`? Default: yes for private repos, no for + public." An abcd-owned entry already on `PATH` is adopted where it stands + rather than duplicated; `--bin-dir ` names a different directory (the + only route to a system-wide one) and fails loudly when it is not writable. + abcd NEVER escalates privileges. If accepted AND the target is absent or + already points at this plugin → write it, provided the binary it would point + at exists; a link to a missing target is refused, because a dangling `abcd` + early on `PATH` shadows every working one behind it. If a different `abcd` + binary exists → refuse, show what it points to, suggest manual resolution. 10. **Hook registration** (`plugin-owned`, VERIFY-ONLY per spc-16 T1) — install verifies that `hooks/hooks.json` is present (the manifest is plugin-static per spc-14 T7). Install NEVER writes `hooks.json`; uninstall NEVER mutates @@ -342,7 +353,8 @@ notes the orphaned-predecessor possibility in the summary. ## Sub-verb semantics **Uninstall (`/abcd:ahoy uninstall`):** removes the BEGIN/END marker block from -CLAUDE.md/AGENTS.md and the `/usr/local/bin/abcd` symlink **if it points at this +CLAUDE.md/AGENTS.md and abcd's own `PATH` entry (`~/.local/bin/abcd`, or wherever +on `PATH` it sits) **if it points at this plugin** (otherwise leave it alone). `hooks/hooks.json` is plugin-static per spc-14 T7 — uninstall NEVER mutates it (per spc-16 T1 brief amendment). **Leaves the entire `.abcd/` namespace intact** (`config/`, `config.json`, diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json index 1711af4c..2db4575f 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -45,6 +45,13 @@ "required": false, "hidden": false }, + { + "name": "bin-dir", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, { "name": "dev", "shorthand": "", diff --git a/.abcd/work/issues/open/iss-171-the-path-install-detector-and-installer-assume-one-blessed-l.md b/.abcd/work/issues/resolved/iss-171-the-path-install-detector-and-installer-assume-one-blessed-l.md similarity index 73% rename from .abcd/work/issues/open/iss-171-the-path-install-detector-and-installer-assume-one-blessed-l.md rename to .abcd/work/issues/resolved/iss-171-the-path-install-detector-and-installer-assume-one-blessed-l.md index e26c23b2..9d1900ea 100644 --- a/.abcd/work/issues/open/iss-171-the-path-install-detector-and-installer-assume-one-blessed-l.md +++ b/.abcd/work/issues/resolved/iss-171-the-path-install-detector-and-installer-assume-one-blessed-l.md @@ -7,6 +7,8 @@ category: "observation" source: "user-observation" found_during: "second-repo install session" found_at: "internal/core/ahoy/store.go" +resolution: "Detection scans PATH and adopts an abcd-owned entry wherever it sits; install defaults to ~/.local/bin (created when absent), adopts an existing owned install in place, reaches a system-wide dir only via an explicit --bin-dir that fails loudly when unwritable, never escalates privileges, and refuses a symlink whose target does not exist. An install dir absent from PATH is its own required gap carrying the one-line export fix; abcd never patches a shell profile. The README one-liner drops sudo." +impact: breaking --- The PATH-install detector and installer assume one blessed layout: binTarget defaults to /usr/local/bin/abcd (sudo territory abcd cannot assume) and symlink detection only recognises that exact target, so a working ~/.local/bin/abcd symlink install — the field-standard single-user location used by uv/pipx/rustup-class tools — reports symlink.missing, a false gap; the detector is itself running as abcd from PATH while reporting abcd not installed. Letting ahoy install 'fix' it would write a symlink to /abcd without validating the target exists — a dangling symlink shadowing the working install. Redesign per fix-the-detector and no-sudo: detection scans PATH for abcd, resolves symlinks (EvalSymlinks, same seam as iss-170), classifies dev-shim/owned/foreign; install defaults to ~/.local/bin (create if needed), adopts an existing owned install in place, does system-wide dirs only behind an explicit --bin-dir flag failing loudly when unwritable, never escalates privileges, refuses to create a symlink whose target does not exist; '~/.local/bin not on PATH' becomes its own loud gap with the printed one-line fix (script-first: print the export line, do not auto-patch shell profiles as the first rung). \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c0c4be4..46b011f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,28 @@ called out in a **Breaking** section. ## [Unreleased] +### Breaking + +- **The documented install location is `~/.local/bin`, and nothing abcd runs asks + for administrator rights** (iss-171). The README one-liner drops `sudo` and + copies the verified binary into `~/.local/bin`; `abcd ahoy install` writes its + `PATH` entry there too, creating the directory when absent. Anyone whose binary + sits in `/usr/local/bin` keeps a working `abcd` — the detector finds an + abcd-owned entry anywhere on `PATH` and adopts it in place — but the documented + path, the one a fresh install follows, moves. A system-wide directory is + reachable only through an explicit `--bin-dir`, which fails loudly when it is + not writable rather than re-running itself with privilege: abcd escalates + nothing, so there is no fallback to hide the refusal behind. Two matching gaps + arrive with it: `~/.local/bin` not on `PATH` is its own named gap carrying the + one-line `export PATH="$HOME/.local/bin:$PATH"` fix (abcd prints it and never + edits a shell profile), and an abcd-owned `PATH` entry whose binary has gone + is reported as dangling rather than silently trusted. Install refuses to create + a link whose target does not exist, because a dangling `abcd` early on `PATH` + shadows every working one behind it. The old detector recognised exactly one + blessed target, so a working `~/.local/bin/abcd` reported `symlink.missing` + while the detector was itself running as that very binary, and "fixing" it + would have written the shadowing link. + ### Added - **The attribution gate reads the git identity, not only the message.** A commit diff --git a/README.md b/README.md index a6df01a9..cd8adce2 100644 --- a/README.md +++ b/README.md @@ -177,22 +177,34 @@ That "someone" is your technical facilitator, who triages those captures later: ## Install -One line, checksum-verified. It detects your OS/architecture, downloads the -binary and the `checksums.txt` manifest from the latest release, verifies the -binary's SHA-256 against the manifest (and refuses to install on any -mismatch — or if the manifest doesn't list the binary at all), then installs -to `/usr/local/bin`: +One line, checksum-verified, no administrator rights. It detects your +OS/architecture, downloads the binary and the `checksums.txt` manifest from the +latest release, verifies the binary's SHA-256 against the manifest (and refuses +to install on any mismatch — or if the manifest doesn't list the binary at all), +then installs to `~/.local/bin`, the single-user location: ```sh -sh -c 'set -eu; cd "$(mktemp -d)"; os=$(uname -s | tr "[:upper:]" "[:lower:]"); arch=$(uname -m); case "$arch" in x86_64) arch=amd64;; aarch64) arch=arm64;; esac; b="abcd-$os-$arch"; curl -fsSLO "https://github.com/REPPL/abcd-cli/releases/latest/download/$b"; curl -fsSLO "https://github.com/REPPL/abcd-cli/releases/latest/download/checksums.txt"; grep " $b$" checksums.txt | if command -v sha256sum >/dev/null; then sha256sum -c -; else shasum -a 256 -c -; fi; sudo install -m 0755 "$b" /usr/local/bin/abcd; abcd version' +sh -c 'set -eu; cd "$(mktemp -d)"; os=$(uname -s | tr "[:upper:]" "[:lower:]"); arch=$(uname -m); case "$arch" in x86_64) arch=amd64;; aarch64) arch=arm64;; esac; b="abcd-$os-$arch"; curl -fsSLO "https://github.com/REPPL/abcd-cli/releases/latest/download/$b"; curl -fsSLO "https://github.com/REPPL/abcd-cli/releases/latest/download/checksums.txt"; grep " $b$" checksums.txt | if command -v sha256sum >/dev/null; then sha256sum -c -; else shasum -a 256 -c -; fi; mkdir -p "$HOME/.local/bin"; install -m 0755 "$b" "$HOME/.local/bin/abcd"; "$HOME/.local/bin/abcd" version' ``` +If `abcd` isn't found by name afterwards, `~/.local/bin` isn't on your `PATH`. +Add this line to your shell profile: + +```sh +export PATH="$HOME/.local/bin:$PATH" +``` + +`abcd ahoy` reports the same thing as a named gap with the same one-line fix, +and `abcd ahoy install` writes its own `PATH` entry to `~/.local/bin` unless you +point it elsewhere with `--bin-dir`. abcd never escalates privileges: a +directory it can't write to is an error, not a prompt for your password. + Prefer to inspect before running? The command is exactly what it says: two downloads from [the latest release](https://github.com/REPPL/abcd-cli/releases/latest), -a checksum verification, and a `sudo install`. You can do the same by hand — -grab the binary for your platform plus `checksums.txt` from the releases -page, run `shasum -a 256 -c` (or `sha256sum -c`) against the matching line, -and copy the binary anywhere on your `PATH`. Every release is built and +a checksum verification, and a copy into a directory you own. You can do the +same by hand — grab the binary for your platform plus `checksums.txt` from the +releases page, run `shasum -a 256 -c` (or `sha256sum -c`) against the matching +line, and copy the binary anywhere on your `PATH`. Every release is built and published by CI from the exact tagged commit, with the checksums generated over the same bytes that are uploaded. diff --git a/commands/ahoy.md b/commands/ahoy.md index c40c101e..5684ab1a 100644 --- a/commands/ahoy.md +++ b/commands/ahoy.md @@ -52,9 +52,20 @@ them. If `folder_kind` is `unmanaged-folder`, note there is nothing to act on ``` **This writes.** It applies the actionable gaps the detection pass found — the -marker block, the `.abcd/` scaffolding, the owned `PATH` symlink. Report the -returned `status` and what changed; the engine prompts before an ambiguous -adoption, so surface any prompt to the user rather than answering it for them. +marker block, the `.abcd/` scaffolding, the owned `PATH` entry. Report the +returned `status`, what changed, and any `notes` — a note is a refusal, stating +something abcd deliberately did not do and why. The engine prompts before an +ambiguous adoption, so surface any prompt to the user rather than answering it +for them. + +The `PATH` entry goes to `~/.local/bin` (created when absent), or to an +abcd-owned entry already on `PATH`, which is adopted exactly where it stands. +`--bin-dir ` names a different directory — the only way to reach a +system-wide one — and fails loudly when it is not writable. abcd never escalates +privileges, so never suggest re-running any of this under `sudo`; report the +failure and let the user pick a directory they own. If the report carries a +`path.bin_dir_not_on_path` gap, relay its one-line `export` fix verbatim and +leave the user's shell profile alone. For dogfooding abcd itself, `abcd ahoy install --dev` installs a track-latest shim instead of the pinned-binary symlink: the `PATH` entry rebuilds abcd from @@ -68,8 +79,10 @@ the source tip on every call and fails loudly on a broken build. Re-running ``` **This writes.** It removes the BEGIN/END marker block and abcd's own `PATH` -symlink and leaves `.abcd/` intact, so the repo's record survives. Report -`marker.removed` and the symlink note. It never touches `hooks.json`. +entry — found wherever it sits on `PATH` — and leaves `.abcd/` intact, so the +repo's record survives. Report `marker.removed` and the symlink note; the +receipt's `symlink.target` is already rendered in tilde form, so relay it as +given rather than expanding it. It never touches `hooks.json`. ## `doctor` — the full read-only report diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 3b1065f4..44e5556a 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -55,6 +55,7 @@ Install or update abcd in this repo (idempotent) ``` --adopt adopt an unmanaged repo without prompting + --bin-dir string directory for the PATH entry (default ~/.local/bin, or an existing abcd install adopted in place); fails when it is not writable — abcd never escalates privileges --dev track-latest dogfood mode: the PATH entry rebuilds from the source tip on every call instead of pinning the built binary --docs-target string marker target: claude_md | agents_md | both | skip --oracle-backend string oracle backend: host-delegated | native | cli | api | mcp diff --git a/internal/core/ahoy/ahoy.go b/internal/core/ahoy/ahoy.go index cfae0272..12b1118d 100644 --- a/internal/core/ahoy/ahoy.go +++ b/internal/core/ahoy/ahoy.go @@ -108,6 +108,12 @@ type InstallOptions struct { // binary (failing loudly on a broken build), instead of a symlink to the // pinned built binary. --dev. Dev bool + // BinDir is an explicit directory for the PATH entry (--bin-dir), the only + // way to reach a system-wide location. Empty means the default single-user + // location (~/.local/bin), or an existing owned install adopted in place. + // abcd NEVER escalates privileges: a BinDir it cannot write to is a loud + // error, never a silent skip and never a re-run under sudo. + BinDir string } // InstallResult is the outcome of Install. @@ -117,6 +123,10 @@ type InstallResult struct { Changes []string `json:"changes,omitempty"` // value overwrites an explicit override forced ("visibility: private -> public") Remaining []string `json:"remaining"` // required+resolvable gap ids left DeclinedCategories []string `json:"declined_categories"` // sorted category wire values + // Notes carries an apply step's loud refusal — a thing abcd deliberately did + // not do, and why. A refusal that only shows up as a still-open gap reads as a + // silent failure, so the reason travels with the result. + Notes []string `json:"notes,omitempty"` } // ApplyResult is the outcome of one apply step. diff --git a/internal/core/ahoy/apply.go b/internal/core/ahoy/apply.go index 9a6caf4e..623689ad 100644 --- a/internal/core/ahoy/apply.go +++ b/internal/core/ahoy/apply.go @@ -35,6 +35,14 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) return InstallResult{Status: "aborted"}, nil } + // Where the PATH entry goes, decided BEFORE any write: an explicit --bin-dir + // abcd cannot write to fails the whole install loudly rather than being + // discovered halfway through, and abcd never re-runs itself with privilege. + binTargetPath, err := resolveInstallTarget(opts, det.pluginRoot) + if err != nil { + return InstallResult{}, err + } + // Adoption gate for an unmanaged repo. adopted := false if det.FolderKind == UnmanagedRepo { @@ -61,7 +69,7 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) // apply-as-update on an otherwise-clean repo, the same way an explicit value // override does (iss-107): the requested install mode differs from what is on // disk, so there is work to do even with zero gaps. - modeForced := modeWouldChange(opts, det) + modeForced := modeWouldChange(opts, det, binTargetPath) if len(actionable(det.Gaps)) == 0 && !(!opts.Yes && pinAdoptable(det.Gaps)) && @@ -82,6 +90,7 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) autoYes: opts.Yes, devMode: opts.Dev, modeForced: modeForced, + binTarget: binTargetPath, } // Ordered apply steps. @@ -116,9 +125,81 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) Changes: ac.changes, Remaining: remaining, DeclinedCategories: declined, + Notes: ac.notes, }, nil } +// resolveInstallTarget decides where the PATH entry goes, before any write. +// +// The default is the field-standard single-user location (~/.local/bin), created +// when absent; an abcd-owned entry already on PATH is adopted exactly where it +// stands, so a second install is never planted beside a working one. A +// system-wide directory is reachable only through an explicit --bin-dir, and an +// unwritable one is an error: abcd never escalates privileges, so there is +// nothing to fall back to and pretending otherwise would install nothing while +// reporting success. +func resolveInstallTarget(opts InstallOptions, pluginRoot string) (string, error) { + if opts.BinDir == "" { + return adoptedBinTarget(pluginRoot), nil + } + dir, err := filepath.Abs(opts.BinDir) + if err != nil { + return "", fmt.Errorf("abcd ahoy install: --bin-dir %s is not a usable path: %w", opts.BinDir, err) + } + // Probe writability without creating anything: the directory is created by + // the apply step, under the same approval as every other write. + probe, existing := dir, false + if fi, serr := os.Stat(dir); serr == nil { + if !fi.IsDir() { + return "", fmt.Errorf("abcd ahoy install: --bin-dir %s is not a directory", displayPath(dir)) + } + existing = true + } else { + probe = nearestExistingDir(dir) + } + if !dirWritable(probe) { + detail := "it could not be created — " + displayPath(probe) + " is not writable" + if existing { + detail = "no file can be created in it" + } + return "", fmt.Errorf("abcd ahoy install: --bin-dir %s is not writable (%s). abcd never escalates privileges — name a directory you own, or omit --bin-dir to install into ~/.local/bin", displayPath(dir), detail) + } + return filepath.Join(dir, binName), nil +} + +// nearestExistingDir walks up from dir to the first directory that exists — the +// one whose writability decides whether dir can be created at all. +func nearestExistingDir(dir string) string { + for { + parent := filepath.Dir(dir) + if parent == dir { + return dir + } + if fi, err := os.Stat(parent); err == nil && fi.IsDir() { + return parent + } + dir = parent + } +} + +// adoptedBinTarget is the PATH entry abcd owns and acts on when no --bin-dir is +// given: an existing owned entry adopted exactly where it stands, else a +// dangling one of ours repaired in place (installing elsewhere would leave it +// shadowing the new entry from earlier in PATH), else the default location. +// Empty when the home directory cannot be resolved — there is no user-scope +// location to write, and inventing a privileged one is what iss-171 removes. +func adoptedBinTarget(pluginRoot string) string { + if pluginRoot != "" { + if e, ok := ownedPathEntry(pluginRoot); ok { + return e.path + } + if e, ok := danglingPathEntry(pluginRoot); ok { + return e.path + } + } + return binTarget() +} + // applyCtx threads the approved-category set and accumulated writes through the // ordered apply steps. type applyCtx struct { @@ -130,9 +211,11 @@ type applyCtx struct { gapPresent map[string]bool writes []string changes []string // human-readable value changes an explicit override forced + notes []string // loud refusals: what abcd deliberately did not do, and why autoYes bool // --yes: every category auto-approved without interaction devMode bool // --dev: install the track-latest shim instead of the symlink modeForced bool // the requested install mode differs from the on-disk state + binTarget string // the resolved PATH entry this run installs (never re-derived) visibilityForced bool // an explicit --visibility override overwrote a valid value docsTargetForced bool // an explicit --docs-target override overwrote a valid value @@ -141,6 +224,12 @@ type applyCtx struct { func (a *applyCtx) note(path string) { a.writes = append(a.writes, path) } +// refuse records a thing abcd deliberately did not do, and why. A refusal that +// shows up only as a still-open gap reads as a silent failure, so the reason +// travels with the install result. Callers pass text already rendered for a +// human — user-scope paths in tilde form. +func (a *applyCtx) refuse(reason string) { a.notes = append(a.notes, reason) } + // stepIdentityPin adopts the iss-62 identity gate for an un-pinned repo: it // writes .abcd/config/identity.json from the current git author identity (the // proposal), gated on ConfigChange approval (the confirmation). A mismatch is @@ -609,17 +698,22 @@ func markerFilesDropped(from, to string) []string { // stepSymlink installs the PATH entry: an owned symlink to the pinned binary // (default), or the track-latest dev shim under --dev. It runs on a fresh install -// (the symlink.missing gap) or when a mode switch was forced on an already-present -// owned entry (apply-as-update, iss-107). It refuses to clobber a foreign binary. +// (the symlink.missing / symlink.dangling gaps) or when a mode switch was forced +// on an already-present owned entry (apply-as-update, iss-107). It refuses to +// clobber a foreign binary. func (a *applyCtx) stepSymlink() { - if a.det.pluginRoot == "" { + if a.det.pluginRoot == "" || a.binTarget == "" { return } - gapDriven := a.approved[ConfigChange] && a.has("symlink.missing") + gapDriven := a.approved[ConfigChange] && (a.has("symlink.missing") || a.has("symlink.dangling")) if !gapDriven && !a.modeForced { return } - target := binTarget() + target := a.binTarget + // A dangling entry of ours is cleared first: it resolves to nothing, so + // removing it destroys nothing, while leaving it in place would keep a link + // that shadows every later PATH entry — including the one being installed. + a.clearDanglingEntry(target) kind := classifyBinTarget(target, a.det.pluginRoot) if kind == binTargetForeign { return // never clobber something we do not own @@ -656,12 +750,42 @@ func (a *applyCtx) installDevShim(target string, kind binTargetKind) { a.note(target) } +// clearDanglingEntry removes an abcd-owned symlink at target whose destination +// no longer exists. It is deliberately narrow: only a SYMLINK, only one that +// resolves to nothing, and only when the binary it would be repointed at exists. +// Nothing is destroyed (the link already answered nothing) and the alternative is +// worse — a dangling `abcd` earlier on PATH shadows the working install. +func (a *applyCtx) clearDanglingEntry(target string) { + fi, err := os.Lstat(target) + if err != nil || fi.Mode()&os.ModeSymlink == 0 { + return + } + if present, serr := fsutil.Exists(target); serr != nil || present { + return + } + if !fileExists(pluginBinaryPath(a.det.pluginRoot)) { + return // nothing to repoint it at; the refusal below states why + } + if err := os.Remove(target); err != nil { + a.refuse("could not remove the dangling PATH entry " + displayPath(target) + ": " + err.Error()) + } +} + // installPinnedSymlink writes the owned symlink to the pinned binary, replacing a // dev shim if one is there. An existing owned symlink is left as-is (idempotent). +// It REFUSES to create a link whose target does not exist: a dangling `abcd` on +// PATH shadows whatever else would have answered, so a broken plugin install must +// not be converted into a broken PATH (iss-171). func (a *applyCtx) installPinnedSymlink(target string, kind binTargetKind) { if kind == binTargetOwnedSymlink { return } + source := pluginBinaryPath(a.det.pluginRoot) + if !fileExists(source) { + a.refuse("refused to create the PATH entry " + displayPath(target) + + ": its target " + displayPath(source) + " does not exist — a dangling link would shadow any working abcd on PATH. Reinstall the plugin, then re-run `abcd ahoy install`.") + return + } if kind == binTargetDevShim { if err := os.Remove(target); err != nil { return @@ -669,22 +793,24 @@ func (a *applyCtx) installPinnedSymlink(target string, kind binTargetKind) { a.echoChange("install_mode", "dev", "pinned") } if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + a.refuse("could not create the install directory " + displayPath(filepath.Dir(target)) + ": " + err.Error()) return } - source := pluginBinaryPath(a.det.pluginRoot) if err := os.Symlink(source, target); err == nil { a.note(target) + } else { + a.refuse("could not write the PATH entry " + displayPath(target) + ": " + err.Error()) } } // modeWouldChange reports whether the requested install mode differs from the // on-disk PATH target, so an otherwise up-to-date repo still has work to do. A // foreign occupant is never touched, so it never counts as a change. -func modeWouldChange(opts InstallOptions, det DetectionResult) bool { - if det.pluginRoot == "" { +func modeWouldChange(opts InstallOptions, det DetectionResult, target string) bool { + if det.pluginRoot == "" || target == "" { return false } - kind := classifyBinTarget(binTarget(), det.pluginRoot) + kind := classifyBinTarget(target, det.pluginRoot) if kind == binTargetForeign { return false } @@ -759,10 +885,19 @@ func Uninstall(cwd string) (UninstallReceipt, error) { } } - // Symlink: remove only if it points at this plugin's binary. - target := binTarget() - receipt.Symlink.Target = target + // Symlink: remove only if it points at this plugin's binary. The entry is + // found the same way detection finds it — an owned entry anywhere on PATH, + // else the default location — so uninstall reaches the install that exists + // rather than one blessed path. pluginRoot, ok := resolvePluginRoot() + target := adoptedBinTarget(pluginRoot) + if target == "" { + receipt.Symlink.Note = "no user-scope install location; left untouched" + return receipt, nil + } + // The receipt is written to be pasted into an issue, so the location is + // rendered in tilde form and never carries the username (iss-177). + receipt.Symlink.Target = displayPath(target) fi, lerr := os.Lstat(target) switch { case lerr != nil: diff --git a/internal/core/ahoy/bin_install_test.go b/internal/core/ahoy/bin_install_test.go new file mode 100644 index 00000000..364d12fe --- /dev/null +++ b/internal/core/ahoy/bin_install_test.go @@ -0,0 +1,372 @@ +package ahoy + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// setupUserScope drives the DEFAULT (no-sudo) install location: HOME is +// redirected, the ABCD_BIN_TARGET test override is cleared so binTarget() +// resolves to ~/.local/bin/abcd, and PATH is exactly the dirs given — so a test +// can state, rather than inherit, what the machine has on PATH. +func setupUserScope(t *testing.T, pathDirs ...string) (home, pluginRoot string) { + t.Helper() + home, pluginRoot = setupHermetic(t) + t.Setenv("ABCD_BIN_TARGET", "") + t.Setenv("PATH", strings.Join(pathDirs, string(os.PathListSeparator))) + return home, pluginRoot +} + +// managedRepo returns a temp dir that classifies as a managed repo (a marker +// block fired), so the deeper gap checks run. +func managedRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + body := "# Project\n\n\nx\n\n" + if err := os.WriteFile(filepath.Join(dir, "CLAUDE.md"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +// linkOwned plants an owned install (a symlink to the plugin binary) at path. +func linkOwned(t *testing.T, path, pluginRoot string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(pluginBinaryPath(pluginRoot), path); err != nil { + t.Fatal(err) + } +} + +func gapByID(gaps []Gap, id string) *Gap { + for i := range gaps { + if gaps[i].ID == id { + return &gaps[i] + } + } + return nil +} + +// TestDetectAdoptsOwnedInstallOnPath is iss-171's headline defect: a working +// ~/.local/bin/abcd — the field-standard single-user location — must read as +// installed, not as symlink.missing, while the detector is itself running as +// abcd from PATH. +func TestDetectAdoptsOwnedInstallOnPath(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + linkOwned(t, filepath.Join(binDir, "abcd"), pluginRoot) + + det, err := Detect(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + if hasGap(det.Gaps, "symlink.missing") { + t.Errorf("a working %s reported symlink.missing: %+v", filepath.Join(binDir, "abcd"), det.Gaps) + } + if m, _ := det.Signals["install_mode"].(string); m != "pinned" { + t.Errorf("install_mode = %q, want pinned", m) + } +} + +// TestDetectAdoptsOwnedInstallAnywhereOnPath proves the detector scans PATH +// rather than recognising one blessed target: an owned install in any PATH +// directory counts. +func TestDetectAdoptsOwnedInstallAnywhereOnPath(t *testing.T) { + _, pluginRoot := setupUserScope(t) + other := filepath.Join(t.TempDir(), "opt", "bin") + t.Setenv("PATH", other) + linkOwned(t, filepath.Join(other, "abcd"), pluginRoot) + + det, err := Detect(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + if hasGap(det.Gaps, "symlink.missing") { + t.Errorf("owned install at %s reported symlink.missing: %+v", other, det.Gaps) + } +} + +// TestDetectDanglingOwnedSymlinkIsItsOwnGap pins the shadowing failure mode: an +// owned symlink whose target has gone is NOT a healthy install, and it is not +// silence either — it is its own gap. +func TestDetectDanglingOwnedSymlinkIsItsOwnGap(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + linkOwned(t, filepath.Join(binDir, "abcd"), pluginRoot) + if err := os.Remove(pluginBinaryPath(pluginRoot)); err != nil { + t.Fatal(err) + } + + det, err := Detect(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + g := gapByID(det.Gaps, "symlink.dangling") + if g == nil { + t.Fatalf("dangling owned symlink produced no symlink.dangling gap: %+v", det.Gaps) + } + if !g.Required { + t.Errorf("symlink.dangling must be required: %+v", g) + } + if m, _ := det.Signals["install_mode"].(string); m == "pinned" { + t.Errorf("a dangling symlink reported install_mode=pinned") + } +} + +// TestDetectBinDirNotOnPathIsALoudGap pins the script-first remedy: an install +// directory that is not on PATH is its own loud gap carrying the one-line export +// fix, and abcd never offers to patch a shell profile (the gap is not resolvable +// by an apply step). +func TestDetectBinDirNotOnPathIsALoudGap(t *testing.T) { + home, pluginRoot := setupUserScope(t, filepath.Join(t.TempDir(), "elsewhere")) + linkOwned(t, filepath.Join(home, ".local", "bin", "abcd"), pluginRoot) + + det, err := Detect(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + g := gapByID(det.Gaps, "path.bin_dir_not_on_path") + if g == nil { + t.Fatalf("no path.bin_dir_not_on_path gap: %+v", det.Gaps) + } + if !g.Required { + t.Errorf("the PATH gap must be required (loud): %+v", g) + } + if g.Resolvable { + t.Errorf("the PATH gap must NOT be resolvable — abcd prints the fix, it never patches a shell profile: %+v", g) + } + if want := `export PATH="$HOME/.local/bin:$PATH"`; !strings.Contains(g.FixHint, want) { + t.Errorf("fix hint = %q, want it to carry %q", g.FixHint, want) + } +} + +// TestGapTextCarriesNoAbsoluteHomePath pins the receipt-hygiene half of the +// hand-off from iss-177: the moment the default install location moves under +// $HOME, any gap line embedding it carries the developer's username. Gap text +// renders user-scope paths in tilde form. +func TestGapTextCarriesNoAbsoluteHomePath(t *testing.T) { + home, _ := setupUserScope(t, filepath.Join(t.TempDir(), "elsewhere")) + + det, err := Detect(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + sawTilde := false + for _, g := range det.Gaps { + for _, field := range []string{g.Title, g.Detail, g.FixHint} { + if strings.Contains(field, home) { + t.Errorf("gap %s leaks the absolute home path %q: %q", g.ID, home, field) + } + } + if strings.Contains(g.Detail, "~/.local/bin/abcd") || strings.Contains(g.Title, "~/.local/bin") { + sawTilde = true + } + } + if !sawTilde { + t.Errorf("no gap named the default install location in tilde form: %+v", det.Gaps) + } +} + +// TestInstallDefaultsToUserLocalBin is decision 5 of the install-experience +// plan: install writes ~/.local/bin/abcd, creating the directory, with no +// privilege escalation anywhere. +func TestInstallDefaultsToUserLocalBin(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + + res, err := Install(repo, installOpts(), RefusingPrompter{}) + if err != nil { + t.Fatal(err) + } + target := filepath.Join(binDir, "abcd") + fi, err := os.Lstat(target) + if err != nil { + t.Fatalf("install did not create %s: %v", target, err) + } + if fi.Mode()&os.ModeSymlink == 0 { + t.Fatalf("install wrote a regular file, want a symlink") + } + dest, _ := os.Readlink(target) + if resolveSymlinkDest(target, dest) != resolvePath(pluginBinaryPath(pluginRoot)) { + t.Errorf("symlink dest = %q, want %q", dest, pluginBinaryPath(pluginRoot)) + } + // The write is recorded on the receipt through the same note seam as every + // other apply step, so the receipt scrub owned by iss-177 covers it. Scrubbing + // the receipt itself belongs to that change, not this one — what this change + // owns is the two print sites OUTSIDE the seam (the gap text and the uninstall + // receipt), asserted by their own tests. + if !containsPath(res.Writes, target) { + t.Errorf("the PATH entry was not recorded on the receipt: %v", res.Writes) + } + if m, _ := detectSignal(t, repo, "install_mode").(string); m != "pinned" { + t.Errorf("install_mode = %q, want pinned", m) + } + _ = home +} + +func containsPath(paths []string, want string) bool { + for _, p := range paths { + if p == want { + return true + } + } + return false +} + +// detectSignal returns one detection signal for repo. +func detectSignal(t *testing.T, repo, key string) any { + t.Helper() + det, err := Detect(repo) + if err != nil { + t.Fatal(err) + } + return det.Signals[key] +} + +// TestInstallAdoptsOwnedInstallInPlace proves install never plants a second +// install beside a working one: an owned entry already on PATH is adopted where +// it stands. +func TestInstallAdoptsOwnedInstallInPlace(t *testing.T) { + home, pluginRoot := setupUserScope(t) + other := filepath.Join(t.TempDir(), "opt", "bin") + t.Setenv("PATH", other) + linkOwned(t, filepath.Join(other, "abcd"), pluginRoot) + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + + if _, err := Install(repo, installOpts(), RefusingPrompter{}); err != nil { + t.Fatal(err) + } + if _, err := os.Lstat(filepath.Join(home, ".local", "bin", "abcd")); !os.IsNotExist(err) { + t.Errorf("install planted a second entry at ~/.local/bin beside the adopted one: %v", err) + } + if _, err := os.Lstat(filepath.Join(other, "abcd")); err != nil { + t.Errorf("install disturbed the adopted install: %v", err) + } +} + +// TestInstallRefusesToCreateDanglingSymlink is the shadowing refusal: with no +// binary at /abcd, install must NOT write a symlink at all — a +// dangling link on PATH shadows whatever else would have answered. +func TestInstallRefusesToCreateDanglingSymlink(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + if err := os.Remove(pluginBinaryPath(pluginRoot)); err != nil { + t.Fatal(err) + } + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + + res, err := Install(repo, installOpts(), RefusingPrompter{}) + if err != nil { + t.Fatal(err) + } + if _, err := os.Lstat(filepath.Join(binDir, "abcd")); !os.IsNotExist(err) { + t.Fatalf("install created a symlink to a non-existent target: %v", err) + } + joined := strings.Join(res.Notes, "\n") + if !strings.Contains(joined, "does not exist") { + t.Errorf("the refusal was silent; notes = %v", res.Notes) + } +} + +// TestInstallBinDirUnwritableFailsLoudly pins the system-wide path: an explicit +// --bin-dir abcd cannot write to is an error, never a silent skip and never a +// privilege escalation. +func TestInstallBinDirUnwritableFailsLoudly(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root: every directory is writable, so the refusal cannot be observed") + } + setupUserScope(t) + ro := filepath.Join(t.TempDir(), "ro") + if err := os.Mkdir(ro, 0o555); err != nil { + t.Fatal(err) + } + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + opts := installOpts() + opts.BinDir = ro + + _, err := Install(repo, opts, RefusingPrompter{}) + if err == nil { + t.Fatalf("an unwritable --bin-dir was accepted silently") + } + msg := err.Error() + for _, want := range []string{"not writable", "privilege"} { + if !strings.Contains(msg, want) { + t.Errorf("error = %q, want it to carry %q", msg, want) + } + } + if _, err := os.Lstat(filepath.Join(ro, "abcd")); !os.IsNotExist(err) { + t.Errorf("something was written into the unwritable dir: %v", err) + } +} + +// TestInstallBinDirWritableInstallsThere pins the opt-in: an explicit, writable +// --bin-dir is where the entry lands. +func TestInstallBinDirWritableInstallsThere(t *testing.T) { + _, pluginRoot := setupUserScope(t) + dir := filepath.Join(t.TempDir(), "opt", "bin") + t.Setenv("PATH", dir) + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + opts := installOpts() + opts.BinDir = dir + + if _, err := Install(repo, opts, RefusingPrompter{}); err != nil { + t.Fatal(err) + } + target := filepath.Join(dir, "abcd") + dest, err := os.Readlink(target) + if err != nil { + t.Fatalf("--bin-dir install did not create %s: %v", target, err) + } + if resolveSymlinkDest(target, dest) != resolvePath(pluginBinaryPath(pluginRoot)) { + t.Errorf("symlink dest = %q, want %q", dest, pluginBinaryPath(pluginRoot)) + } +} + +// TestUninstallReceiptCarriesNoAbsoluteHomePath is the second half of the +// iss-177 hand-off: `ahoy uninstall` prints the receipt's symlink target +// verbatim, so the target must be rendered in tilde form once the default +// location lives under $HOME. +func TestUninstallReceiptCarriesNoAbsoluteHomePath(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + linkOwned(t, filepath.Join(binDir, "abcd"), pluginRoot) + + receipt, err := Uninstall(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + if !receipt.Symlink.Removed { + t.Fatalf("uninstall did not remove the owned install: %+v", receipt.Symlink) + } + if strings.Contains(receipt.Symlink.Target, home) { + t.Errorf("receipt leaks the absolute home path: %q", receipt.Symlink.Target) + } + if want := "~/.local/bin/abcd"; receipt.Symlink.Target != want { + t.Errorf("receipt target = %q, want %q", receipt.Symlink.Target, want) + } +} diff --git a/internal/core/ahoy/detect.go b/internal/core/ahoy/detect.go index 69827c0e..62c68c44 100644 --- a/internal/core/ahoy/detect.go +++ b/internal/core/ahoy/detect.go @@ -381,46 +381,99 @@ func detectMarkerDrift(cwd string) []Gap { return gaps } +// detectPathSymlink reports the state of abcd's own entry on PATH. It scans +// PATH rather than inspecting one blessed location (iss-171): a working +// ~/.local/bin/abcd is an install, wherever the default happens to point, so the +// detector can no longer report "not installed" while running as that very +// binary. Every path it renders goes through displayPath, so a gap pasted into +// an issue carries no username. func detectPathSymlink(pluginRoot string, pluginOK bool) []Gap { if !pluginOK { return nil } - target := binTarget() - expected := pluginBinaryPath(pluginRoot) - fi, err := lstat(target) - if err != nil { - if isNotExist(err) { - return []Gap{{ - ID: "symlink.missing", Category: ConfigChange, Scope: "machine", - Title: "PATH symlink not installed", Detail: target + " does not exist.", - FixHint: "ahoy install creates the symlink (refuses to clobber).", Required: true, Resolvable: true, - }} - } - return nil + var gaps []Gap + + // A link of ours whose binary has gone shadows whatever else on PATH would + // have answered. It is neither "installed" nor "missing" — it is its own gap. + if e, ok := danglingPathEntry(pluginRoot); ok { + gaps = append(gaps, Gap{ + ID: "symlink.dangling", Category: ConfigChange, Scope: "machine", + Title: "PATH entry points at a binary that is gone", + Detail: displayPath(e.path) + " is an abcd-owned entry whose target no longer exists, so it shadows every later PATH entry.", + FixHint: "ahoy install repoints it once the plugin binary is present; remove it with `ahoy uninstall` if abcd is gone.", + Required: true, Resolvable: true, + }) } - if fi.Mode()&modeSymlink == 0 { + + target := effectiveBinTarget(pluginRoot) + if target == "" { + return gaps // no home directory: there is no user-scope target to report on + } + installed := false + + fi, err := lstat(target) + switch { + case err != nil && isNotExist(err): + gaps = append(gaps, Gap{ + ID: "symlink.missing", Category: ConfigChange, Scope: "machine", + Title: "abcd is not on PATH", Detail: "No abcd-owned entry was found on PATH, and " + displayPath(target) + " does not exist.", + FixHint: "ahoy install writes the entry (refuses to clobber, and never escalates privileges).", Required: true, Resolvable: true, + }) + case err != nil: + // Present but unstattable: never claim anything about it. + case fi.Mode()&modeSymlink == 0: if isDevShimFile(target) { // Our own track-latest dev shim (abcd ahoy install --dev) — a valid // install, not a foreign occupant. Surfaced via the install_mode signal. - return nil + installed = true + } else { + gaps = append(gaps, Gap{ + ID: "symlink.foreign", Category: ConfigChange, Scope: "machine", + Title: "non-symlink at " + displayPath(target), Detail: "A regular file occupies the PATH entry abcd would write.", + FixHint: "Resolve manually; ahoy refuses to clobber.", Required: false, Resolvable: false, + }) + } + default: + dest, rerr := readlink(target) + switch { + case rerr != nil: + // Unreadable link: say nothing rather than guess. + case resolveSymlinkDest(target, dest) == resolvePath(pluginBinaryPath(pluginRoot)): + installed = true + default: + gaps = append(gaps, Gap{ + ID: "symlink.foreign", Category: ConfigChange, Scope: "machine", + Title: "foreign symlink at " + displayPath(target), + Detail: displayPath(target) + " -> " + displayPath(dest) + " (expected " + displayPath(pluginBinaryPath(pluginRoot)) + ").", + FixHint: "Resolve manually; ahoy refuses to clobber.", Required: false, Resolvable: false, + }) } - return []Gap{{ - ID: "symlink.foreign", Category: ConfigChange, Scope: "machine", - Title: "non-symlink at " + target, Detail: "A regular file occupies the PATH symlink target.", - FixHint: "Resolve manually; ahoy refuses to clobber.", Required: false, Resolvable: false, - }} } - dest, err := readlink(target) - if err != nil { + + gaps = append(gaps, detectBinDirOnPath(filepath.Dir(target), installed)...) + return gaps +} + +// detectBinDirOnPath reports an install directory that is not on PATH. It is a +// gap in its own right: the entry exists and abcd still cannot be run by name, +// which otherwise reads as a broken install. The remedy is printed, never +// applied — abcd states the one-line export and leaves the user's shell profile +// alone (script-first), so the gap is required but NOT resolvable. +func detectBinDirOnPath(dir string, installed bool) []Gap { + if dir == "" || dirOnPath(dir) { return nil } - if resolveSymlinkDest(target, dest) == resolvePath(expected) { + // Silent while there is nothing there yet AND no install: the missing-entry + // gap already says what to do, and install will surface this straight after. + if !installed { return nil } return []Gap{{ - ID: "symlink.foreign", Category: ConfigChange, Scope: "machine", - Title: "foreign symlink at " + target, Detail: target + " -> " + dest + " (expected " + expected + ").", - FixHint: "Resolve manually; ahoy refuses to clobber.", Required: false, Resolvable: false, + ID: "path.bin_dir_not_on_path", Category: ConfigChange, Scope: "machine", + Title: displayPath(dir) + " is not on PATH", + Detail: "abcd is installed at " + displayPath(filepath.Join(dir, binName)) + ", but " + displayPath(dir) + " is not in PATH, so `abcd` cannot be run by name.", + FixHint: "Add it to your shell profile: " + exportPathLine(dir), + Required: true, Resolvable: false, }} } @@ -432,7 +485,15 @@ func detectInstallMode(pluginRoot string, pluginOK bool) string { if !pluginOK { return "" } - switch classifyBinTarget(binTarget(), pluginRoot) { + target := effectiveBinTarget(pluginRoot) + if target == "" { + return "" + } + // A link whose binary is gone is not an install mode; it is the dangling gap. + if present, err := fsutil.Exists(target); err == nil && !present { + return "" + } + switch classifyBinTarget(target, pluginRoot) { case binTargetDevShim: return "dev (tip build)" case binTargetOwnedSymlink: diff --git a/internal/core/ahoy/fsutil.go b/internal/core/ahoy/fsutil.go index 23598915..f0a7c48f 100644 --- a/internal/core/ahoy/fsutil.go +++ b/internal/core/ahoy/fsutil.go @@ -3,8 +3,37 @@ package ahoy import ( "os" "path/filepath" + "strings" ) +// displayPath renders p for any surface a human reads or pastes: a path inside +// the user's home directory becomes its tilde form (~/.local/bin/abcd), so a gap +// line or an uninstall receipt carries the location without carrying the +// username. Everything outside home is unchanged — a system path is not +// sensitive and shortening it would lose information. +// +// This is the same hygiene iss-177 applies inside the install receipt's note +// seam; it lives here because the two print sites outside that seam (the doctor +// gap text and the uninstall receipt) render paths of their own. Both can be +// unified onto one primitive once the receipt scrub lands. +func displayPath(p string) string { + if p == "" { + return p + } + home, err := os.UserHomeDir() + if err != nil || home == "" || home == string(os.PathSeparator) { + return p + } + home = filepath.Clean(home) + if p == home { + return "~" + } + if strings.HasPrefix(p, home+string(os.PathSeparator)) { + return "~" + string(os.PathSeparator) + p[len(home)+1:] + } + return p +} + // modeSymlink aliases os.ModeSymlink so detection reads naturally. const modeSymlink = os.ModeSymlink diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index cdfafaa6..0a9176cd 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -125,12 +125,160 @@ func pluginBinaryPath(pluginRoot string) string { return filepath.Join(pluginRoot, "abcd") } -// binTarget is the PATH symlink target, overridable for tests. +// binName is the name abcd occupies on PATH. +const binName = "abcd" + +// userBinDir is the single-user install directory, ~/.local/bin — the location +// uv/pipx/rustup-class tools use, writable without privilege escalation. Empty +// when the home directory cannot be resolved. +func userBinDir() string { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return "" + } + return filepath.Join(home, ".local", "bin") +} + +// binTarget is the DEFAULT PATH entry: ~/.local/bin/abcd (iss-171). abcd never +// escalates privileges, so a system-wide directory is reachable only through an +// explicit --bin-dir. Overridable for tests via ABCD_BIN_TARGET; empty when the +// home directory cannot be resolved, which every caller reads as "no target to +// act on" rather than falling back to a privileged path. func binTarget() string { if v := os.Getenv("ABCD_BIN_TARGET"); v != "" { return v } - return "/usr/local/bin/abcd" + dir := userBinDir() + if dir == "" { + return "" + } + return filepath.Join(dir, binName) +} + +// pathDirs returns the PATH entries in order, dropping empties. +func pathDirs() []string { + var dirs []string + for _, d := range filepath.SplitList(os.Getenv("PATH")) { + if d != "" { + dirs = append(dirs, d) + } + } + return dirs +} + +// dirOnPath reports whether dir is one of the PATH entries, comparing canonical +// forms so /home/x/.local/bin and a symlinked route to it are the same entry. +func dirOnPath(dir string) bool { + if dir == "" { + return false + } + want := resolvePath(dir) + for _, d := range pathDirs() { + if resolvePath(d) == want { + return true + } + } + return false +} + +// pathEntry is one `abcd` found on PATH, classified. +type pathEntry struct { + path string // the entry as it sits on PATH (unresolved) + kind binTargetKind // dev-shim / owned / foreign + dangling bool // ours, but the binary it points at is gone +} + +// owned reports whether the entry is one abcd installed (a pinned symlink or the +// dev shim) rather than a foreign occupant. +func (e pathEntry) owned() bool { + return e.kind == binTargetOwnedSymlink || e.kind == binTargetDevShim +} + +// scanPathEntries walks PATH in order and classifies every `abcd` it finds. It +// is the fix-the-detector half of iss-171: abcd is installed if abcd is on PATH, +// wherever it sits, rather than at one blessed location. Symlinks are resolved +// through the same seam as iss-170 (resolvePath -> filepath.EvalSymlinks), so a +// relative or indirect link classifies the same as a direct one. +func scanPathEntries(pluginRoot string) []pathEntry { + var entries []pathEntry + seen := map[string]bool{} + for _, dir := range pathDirs() { + candidate := filepath.Join(dir, binName) + if seen[candidate] { + continue + } + seen[candidate] = true + if _, err := os.Lstat(candidate); err != nil { + continue + } + e := pathEntry{path: candidate, kind: classifyBinTarget(candidate, pluginRoot)} + if e.owned() { + // Stat FOLLOWS the link: a dangling entry is one whose target is gone. + // A stat error other than not-exist is not proof of a dangling link, so + // it reads as healthy rather than manufacturing a gap. + if present, err := fsutil.Exists(candidate); err == nil && !present { + e.dangling = true + } + } + entries = append(entries, e) + } + return entries +} + +// ownedPathEntry returns the first healthy abcd-owned entry on PATH. That entry +// IS the install: `ahoy install` adopts it in place rather than planting a second +// one, and detection reports it rather than a false symlink.missing. +func ownedPathEntry(pluginRoot string) (pathEntry, bool) { + for _, e := range scanPathEntries(pluginRoot) { + if e.owned() && !e.dangling { + return e, true + } + } + return pathEntry{}, false +} + +// danglingPathEntry returns the first abcd-owned entry on PATH whose target has +// gone — a link that shadows whatever else on PATH would have answered. +func danglingPathEntry(pluginRoot string) (pathEntry, bool) { + for _, e := range scanPathEntries(pluginRoot) { + if e.owned() && e.dangling { + return e, true + } + } + return pathEntry{}, false +} + +// effectiveBinTarget is the PATH entry every verb acts on: an existing owned +// install (adopted where it stands), else the default target. +func effectiveBinTarget(pluginRoot string) string { + if e, ok := ownedPathEntry(pluginRoot); ok { + return e.path + } + return binTarget() +} + +// dirWritable reports whether a file can actually be created in dir. It probes +// rather than reading the mode bits, so an ACL or a read-only mount answers +// honestly. A directory abcd cannot write to is refused, never escalated. +func dirWritable(dir string) bool { + f, err := os.CreateTemp(dir, ".abcd-write-probe-*") + if err != nil { + return false + } + name := f.Name() + f.Close() + os.Remove(name) + return true +} + +// exportPathLine renders the one-line PATH fix for dir, in the form a user can +// paste. abcd prints it and never patches a shell profile (script-first). +func exportPathLine(dir string) string { + d := displayPath(dir) + if strings.HasPrefix(d, "~/") { + d = "$HOME/" + strings.TrimPrefix(d, "~/") + } + return `export PATH="` + d + `:$PATH"` } // --------------------------------------------------------------------------- diff --git a/internal/surface/cli/ahoy_bin_install_test.go b/internal/surface/cli/ahoy_bin_install_test.go new file mode 100644 index 00000000..80b147a7 --- /dev/null +++ b/internal/surface/cli/ahoy_bin_install_test.go @@ -0,0 +1,87 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// userScopeEnv is hermeticEnv with the ABCD_BIN_TARGET override cleared and PATH +// stated, so the CLI exercises the real default install location +// (~/.local/bin/abcd) rather than a temp target that hides a home-path leak. +func userScopeEnv(t *testing.T, pathDirs ...string) (home, pluginRoot string) { + t.Helper() + home = t.TempDir() + pluginRoot = t.TempDir() + if err := os.MkdirAll(filepath.Join(pluginRoot, "hooks"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pluginRoot, "hooks", "hooks.json"), []byte(validHooksJSON), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pluginRoot, "abcd"), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("HOME", home) + t.Setenv("ABCD_PLUGIN_ROOT", pluginRoot) + t.Setenv("CLAUDE_PLUGIN_ROOT", "") + t.Setenv("ABCD_BIN_TARGET", "") + t.Setenv("PATH", strings.Join(pathDirs, string(os.PathListSeparator))) + return home, pluginRoot +} + +// TestAhoyUninstallPrintsNoAbsoluteHomePath pins the iss-177 hand-off at the one +// print site outside the receipt's own note seam: `ahoy uninstall` renders the +// symlink target verbatim, and the default install location now lives under +// $HOME, so the line would carry the username unless the target is rendered in +// tilde form. +func TestAhoyUninstallPrintsNoAbsoluteHomePath(t *testing.T) { + home, pluginRoot := userScopeEnv(t) + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + if err := os.MkdirAll(binDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(pluginRoot, "abcd"), filepath.Join(binDir, "abcd")); err != nil { + t.Fatal(err) + } + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + t.Chdir(repo) + + out := string(runCLI(t, "ahoy", "uninstall")) + if strings.Contains(out, home) { + t.Fatalf("uninstall printed the absolute home path %q:\n%s", home, out) + } + if !strings.Contains(out, "~/.local/bin/abcd") { + t.Fatalf("uninstall did not name the removed entry in tilde form:\n%s", out) + } +} + +// TestAhoyInstallBinDirFlagIsWired proves --bin-dir reaches the core engine from +// the CLI front door: the entry lands in the named directory, never in the +// default one. +func TestAhoyInstallBinDirFlagIsWired(t *testing.T) { + home, _ := userScopeEnv(t) + dir := filepath.Join(t.TempDir(), "opt", "bin") + t.Setenv("PATH", dir) + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + t.Chdir(repo) + + runCLI(t, "ahoy", "install", "--yes", "--adopt", "--bin-dir", dir, + "--visibility", "private", "--docs-target", "both", + "--oracle-backend", "host-delegated", "--scan-deep", "false", "--json") + + if _, err := os.Lstat(filepath.Join(dir, "abcd")); err != nil { + t.Fatalf("--bin-dir did not reach the core engine: %v", err) + } + if _, err := os.Lstat(filepath.Join(home, ".local", "bin", "abcd")); !os.IsNotExist(err) { + t.Errorf("--bin-dir install also wrote the default location: %v", err) + } +} diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index f72dd2b9..9fa7dcb0 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -1525,6 +1525,7 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { adopt bool refuseAdopt bool dev bool + binDir string visibility string docsTarget string oracleBackend string @@ -1539,7 +1540,7 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { if err != nil { return err } - opts, err := installOptionsFromFlags(cmd, yes, adopt, refuseAdopt, dev, visibility, docsTarget, oracleBackend, scanDeep) + opts, err := installOptionsFromFlags(cmd, yes, adopt, refuseAdopt, dev, binDir, visibility, docsTarget, oracleBackend, scanDeep) if err != nil { return err } @@ -1555,6 +1556,12 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { for _, p := range res.Writes { fmt.Fprintf(w, " wrote: %s\n", p) } + // A refusal is louder than an unexplained missing write: it says + // what abcd did not do and why (a dangling PATH entry it declined + // to create, a directory it could not write). + for _, n := range res.Notes { + fmt.Fprintf(w, " note: %s\n", n) + } if len(res.DeclinedCategories) > 0 { fmt.Fprintf(w, " declined: %s\n", strings.Join(res.DeclinedCategories, ", ")) } @@ -1568,6 +1575,7 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { installCmd.Flags().BoolVar(&adopt, "adopt", false, "adopt an unmanaged repo without prompting") installCmd.Flags().BoolVar(&refuseAdopt, "refuse-adopt", false, "decline to adopt an unmanaged repo") installCmd.Flags().BoolVar(&dev, "dev", false, "track-latest dogfood mode: the PATH entry rebuilds from the source tip on every call instead of pinning the built binary") + installCmd.Flags().StringVar(&binDir, "bin-dir", "", "directory for the PATH entry (default ~/.local/bin, or an existing abcd install adopted in place); fails when it is not writable — abcd never escalates privileges") installCmd.Flags().StringVar(&visibility, "visibility", "", "repo visibility: private | public") installCmd.Flags().StringVar(&docsTarget, "docs-target", "", "marker target: claude_md | agents_md | both | skip") installCmd.Flags().StringVar(&oracleBackend, "oracle-backend", "", "oracle backend: host-delegated | native | cli | api | mcp") @@ -1671,8 +1679,8 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { // installOptionsFromFlags validates the install flags and builds InstallOptions. // Only explicitly-set value flags become overrides; unset values fall through to // the prompter (interactive) or its default (non-interactive). -func installOptionsFromFlags(cmd *cobra.Command, yes, adopt, refuseAdopt, dev bool, visibility, docsTarget, oracleBackend, scanDeep string) (ahoy.InstallOptions, error) { - opts := ahoy.InstallOptions{Yes: yes, Dev: dev} +func installOptionsFromFlags(cmd *cobra.Command, yes, adopt, refuseAdopt, dev bool, binDir, visibility, docsTarget, oracleBackend, scanDeep string) (ahoy.InstallOptions, error) { + opts := ahoy.InstallOptions{Yes: yes, Dev: dev, BinDir: binDir} if adopt && refuseAdopt { return opts, fmt.Errorf("abcd ahoy install: --adopt and --refuse-adopt are mutually exclusive") } From b4f420798b296621dab927efafaaec862a72aaa5 Mon Sep 17 00:00:00 2001 From: REPPL Date: Wed, 12 Aug 2026 13:32:31 +0200 Subject: [PATCH 2/2] fix: an entry that is never reached is not an install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round on iss-171. Six correctness findings and three security ones, all about the same blind spot: the scan classified every `abcd` on PATH and then discarded everything it did not own, so the states it could see it did not say. The population the OLD one-liner created is the sharp case. It COPIED the binary to /usr/local/bin, and a copy is a regular file, so it classifies foreign, is never adopted, and the new entry lands behind it — status clean, install_mode pinned, and the stale copy still answering `abcd` forever. Any entry preceding abcd's own on PATH is now `symlink.shadowed`: required, diagnostic, naming the occupant and both remedies. abcd will not remove it, so it does not pretend it can. install_mode reports "pinned (shadowed on PATH)" rather than health, and dangling is computed for every entry, not only ours, so a foreign link pointing at nothing is described instead of invisible. The two required-but-unresolvable gaps (shadowed, bin-dir-not-on-PATH) now travel out of Install on the Notes slice. They were printed by nothing on a fresh install: excluded from Remaining, absent from InstallResult, and reachable only through `doctor` — which cannot be run by name on the machine where abcd is not yet on PATH. Install computes both against the target it actually wrote, which is the only place an explicit --bin-dir is known. The remaining refusals are made audible. A foreign occupant at the target bailed out recording nothing, so a --bin-dir run reported success having written nothing; it now names what is in the way. installPinnedSymlink's source check moves ahead of its owned-entry early return, so a dangling entry whose binary is gone states the reason instead of returning silently. Every note renders OS error text through the same home-path scrub as the rest of the seam. `ahoy uninstall` takes --bin-dir: an entry placed outside PATH by --bin-dir is invisible to a PATH scan, so uninstall would otherwise orphan it. The --bin-dir writability probe moves after the adoption gate, so a declined run no longer creates and removes a file in a directory the user never adopted. The CHANGELOG's mitigation sentence was false for exactly the population that needs it — it promised adoption to users whose binary is a copy — and now tells them what they will see and what to do. README and the command surface say the same. Watched red before, green after: seven new tests, one per reviewer scenario — shadowing foreign copy (detect and install), dangling foreign entry, foreign occupant at --bin-dir, fresh-install PATH note, dangling entry with a missing binary, and an off-PATH --bin-dir install described and then removed. Assisted-by: Claude:claude-opus-5[1m] --- .../development/brief/04-surfaces/01-ahoy.md | 6 +- .abcd/development/release/surface.json | 10 +- CHANGELOG.md | 40 ++-- README.md | 6 + commands/ahoy.md | 12 +- docs/reference/cli/commands.md | 8 +- internal/core/ahoy/apply.go | 92 ++++++-- internal/core/ahoy/bin_install_test.go | 212 +++++++++++++++++- internal/core/ahoy/detect.go | 41 +++- internal/core/ahoy/dev_install_test.go | 2 +- internal/core/ahoy/fsutil.go | 26 +++ internal/core/ahoy/idempotency_test.go | 2 +- internal/core/ahoy/store.go | 88 +++++++- internal/surface/cli/cli.go | 9 +- 14 files changed, 498 insertions(+), 56 deletions(-) diff --git a/.abcd/development/brief/04-surfaces/01-ahoy.md b/.abcd/development/brief/04-surfaces/01-ahoy.md index 4362c40c..c4cc4d77 100644 --- a/.abcd/development/brief/04-surfaces/01-ahoy.md +++ b/.abcd/development/brief/04-surfaces/01-ahoy.md @@ -165,7 +165,11 @@ Steps, run in parallel where independent: this plugin, at a different binary, or at nothing. An owned entry whose target has gone is `symlink.dangling`; an install directory absent from `PATH` is `path.bin_dir_not_on_path`, required but not resolvable — abcd prints the - one-line `export` fix and never edits a shell profile (iss-171). + one-line `export` fix and never edits a shell profile; and any `abcd` that + comes BEFORE abcd's own entry on `PATH` is `symlink.shadowed`, because an + entry that is correct and never reached is not an install (iss-171). Install + carries the two non-resolvable ones on its own result as notes: a fresh user + cannot run `doctor` by name on a machine where abcd is not yet on `PATH`. 9. **Hook manifest verification** (verify-only per spc-16 T1) — VERIFY that `hooks/hooks.json` is present in the plugin install AND contains the three required event entries (`UserPromptSubmit`, `SessionStart`, `PreCompact`) diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json index 2db4575f..d805d66d 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -106,7 +106,15 @@ { "path": "abcd ahoy uninstall", "hidden": false, - "flags": [] + "flags": [ + { + "name": "bin-dir", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] }, { "path": "abcd audit", diff --git a/CHANGELOG.md b/CHANGELOG.md index 46b011f3..efed0909 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,22 +15,30 @@ called out in a **Breaking** section. - **The documented install location is `~/.local/bin`, and nothing abcd runs asks for administrator rights** (iss-171). The README one-liner drops `sudo` and copies the verified binary into `~/.local/bin`; `abcd ahoy install` writes its - `PATH` entry there too, creating the directory when absent. Anyone whose binary - sits in `/usr/local/bin` keeps a working `abcd` — the detector finds an - abcd-owned entry anywhere on `PATH` and adopts it in place — but the documented - path, the one a fresh install follows, moves. A system-wide directory is - reachable only through an explicit `--bin-dir`, which fails loudly when it is - not writable rather than re-running itself with privilege: abcd escalates - nothing, so there is no fallback to hide the refusal behind. Two matching gaps - arrive with it: `~/.local/bin` not on `PATH` is its own named gap carrying the - one-line `export PATH="$HOME/.local/bin:$PATH"` fix (abcd prints it and never - edits a shell profile), and an abcd-owned `PATH` entry whose binary has gone - is reported as dangling rather than silently trusted. Install refuses to create - a link whose target does not exist, because a dangling `abcd` early on `PATH` - shadows every working one behind it. The old detector recognised exactly one - blessed target, so a working `~/.local/bin/abcd` reported `symlink.missing` - while the detector was itself running as that very binary, and "fixing" it - would have written the shadowing link. + `PATH` entry there too, creating the directory when absent. **What existing + users see depends on how their binary got there.** An abcd-owned symlink — the + thing `ahoy install` writes — is found anywhere on `PATH` and adopted in place, + so nothing changes. A binary the old one-liner *copied* into `/usr/local/bin` + is a plain file abcd does not own: it is never adopted, and a new entry in + `~/.local/bin` lands behind it on `PATH`, so the copy is still what runs. That + case now reports a `symlink.shadowed` gap naming the copy, both from `ahoy` + and on the install run itself; the remedy is to delete the stale copy + (`rm /usr/local/bin/abcd`, which needs the rights that put it there) or to + install ahead of it with `--bin-dir`. abcd will not remove it — it never + touches a binary it does not own. A system-wide directory is reachable only + through an explicit `--bin-dir`, which fails loudly when it is not writable + rather than re-running itself with privilege: abcd escalates nothing, so there + is no fallback to hide the refusal behind. Two more gaps arrive with it: + `~/.local/bin` not on `PATH` is its own named gap carrying the one-line + `export PATH="$HOME/.local/bin:$PATH"` fix (abcd prints it and never edits a + shell profile), and an abcd-owned `PATH` entry whose binary has gone is + reported as dangling rather than silently trusted. Install refuses to create a + link whose target does not exist, because a dangling `abcd` early on `PATH` + shadows every working one behind it, and it reports every such refusal on the + result rather than leaving a gap to speak for it. The old detector recognised + exactly one blessed target, so a working `~/.local/bin/abcd` reported + `symlink.missing` while the detector was itself running as that very binary, + and "fixing" it would have written the shadowing link. ### Added diff --git a/README.md b/README.md index cd8adce2..3f349ef2 100644 --- a/README.md +++ b/README.md @@ -199,6 +199,12 @@ and `abcd ahoy install` writes its own `PATH` entry to `~/.local/bin` unless you point it elsewhere with `--bin-dir`. abcd never escalates privileges: a directory it can't write to is an error, not a prompt for your password. +Already have an `abcd` in a system directory from an earlier install? Delete it +(`rm /usr/local/bin/abcd`, with whatever rights put it there) — otherwise it +comes first on `PATH` and keeps answering instead of the new one. `abcd ahoy` +names it in a gap rather than removing it: abcd does not touch a binary it does +not own. + Prefer to inspect before running? The command is exactly what it says: two downloads from [the latest release](https://github.com/REPPL/abcd-cli/releases/latest), a checksum verification, and a copy into a directory you own. You can do the diff --git a/commands/ahoy.md b/commands/ahoy.md index 5684ab1a..86248298 100644 --- a/commands/ahoy.md +++ b/commands/ahoy.md @@ -67,6 +67,14 @@ failure and let the user pick a directory they own. If the report carries a `path.bin_dir_not_on_path` gap, relay its one-line `export` fix verbatim and leave the user's shell profile alone. +A `symlink.shadowed` gap (or a note saying the same) means another `abcd` comes +first on `PATH`, so the entry abcd just wrote is NOT what runs — typically a +binary an older install copied into a system directory. Relay it prominently: +the install is not finished from the user's point of view. abcd will not remove +that binary, and neither should you offer to; state the two remedies it gives +(delete the stale one, or install ahead of it with `--bin-dir`) and let the user +choose. + For dogfooding abcd itself, `abcd ahoy install --dev` installs a track-latest shim instead of the pinned-binary symlink: the `PATH` entry rebuilds abcd from the source tip on every call and fails loudly on a broken build. Re-running @@ -82,7 +90,9 @@ the source tip on every call and fails loudly on a broken build. Re-running entry — found wherever it sits on `PATH` — and leaves `.abcd/` intact, so the repo's record survives. Report `marker.removed` and the symlink note; the receipt's `symlink.target` is already rendered in tilde form, so relay it as -given rather than expanding it. It never touches `hooks.json`. +given rather than expanding it. It never touches `hooks.json`. An entry that was +installed with `--bin-dir` into a directory outside `PATH` cannot be found by a +`PATH` scan — pass the same `--bin-dir ` to `uninstall` to remove it. ## `doctor` — the full read-only report diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 44e5556a..6399571c 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -69,7 +69,13 @@ Install or update abcd in this repo (idempotent) Remove the marker block and owned PATH symlink (leaves .abcd/ intact) -**Usage:** `abcd ahoy uninstall` +**Usage:** `abcd ahoy uninstall [flags]` + +**Flags:** + +``` + --bin-dir string directory holding the PATH entry to remove; needed only when it was installed with --bin-dir into a directory that is not on PATH +``` ### `abcd audit` diff --git a/internal/core/ahoy/apply.go b/internal/core/ahoy/apply.go index 623689ad..85991ce6 100644 --- a/internal/core/ahoy/apply.go +++ b/internal/core/ahoy/apply.go @@ -35,14 +35,6 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) return InstallResult{Status: "aborted"}, nil } - // Where the PATH entry goes, decided BEFORE any write: an explicit --bin-dir - // abcd cannot write to fails the whole install loudly rather than being - // discovered halfway through, and abcd never re-runs itself with privilege. - binTargetPath, err := resolveInstallTarget(opts, det.pluginRoot) - if err != nil { - return InstallResult{}, err - } - // Adoption gate for an unmanaged repo. adopted := false if det.FolderKind == UnmanagedRepo { @@ -60,6 +52,17 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) } _ = adopted + // Where the PATH entry goes, decided BEFORE any write but AFTER the adoption + // gate: an explicit --bin-dir abcd cannot write to fails the whole install + // loudly rather than being discovered halfway through, and abcd never re-runs + // itself with privilege. Deciding it after adoption keeps the writability + // probe — which creates and removes a temp file — out of a run the user + // declines. + binTargetPath, err := resolveInstallTarget(opts, det.pluginRoot) + if err != nil { + return InstallResult{}, err + } + // Idempotency: zero required+resolvable gaps => exact no-op. Two exceptions // fall through: the advisory git-identity pin, which install adopts through an // interactive confirmation (never under --yes), as the gap's fix hint @@ -107,6 +110,8 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) ac.stepRules() ac.stepVersionStamp() ac.stepIdentityPin() + // Last, because it describes the entry the steps above actually wrote. + ac.noteReachability() // Re-detect to compute what remains. final, err := Detect(abs) @@ -716,7 +721,14 @@ func (a *applyCtx) stepSymlink() { a.clearDanglingEntry(target) kind := classifyBinTarget(target, a.det.pluginRoot) if kind == binTargetForeign { - return // never clobber something we do not own + // Never clobber something we do not own — and never in silence. The + // symlink.foreign gap is advisory, so it is filtered out of Remaining, and + // with no note the run reports nothing written and no reason why; under an + // explicit --bin-dir the detection gap does not even describe this location. + a.refuse("refused to write the PATH entry " + displayPath(target) + + ": it is occupied by " + describeEntry(pathEntry{path: target, kind: kind}) + + ". abcd never clobbers a binary it does not own — remove it, or choose another directory with `--bin-dir`.") + return } if a.devMode { a.installDevShim(target, kind) @@ -764,10 +776,14 @@ func (a *applyCtx) clearDanglingEntry(target string) { return } if !fileExists(pluginBinaryPath(a.det.pluginRoot)) { - return // nothing to repoint it at; the refusal below states why + // Nothing to repoint it at. Leaving the link is the lesser evil (removing + // it would take abcd off PATH entirely for no gain), and installPinnedSymlink + // states the reason — its source check runs before the owned-entry early + // return precisely so this case is never silent. + return } if err := os.Remove(target); err != nil { - a.refuse("could not remove the dangling PATH entry " + displayPath(target) + ": " + err.Error()) + a.refuse("could not remove the dangling PATH entry " + displayPath(target) + ": " + errText(err)) } } @@ -777,15 +793,19 @@ func (a *applyCtx) clearDanglingEntry(target string) { // PATH shadows whatever else would have answered, so a broken plugin install must // not be converted into a broken PATH (iss-171). func (a *applyCtx) installPinnedSymlink(target string, kind binTargetKind) { - if kind == binTargetOwnedSymlink { - return - } + // The source check comes FIRST, before the idempotent early return: an owned + // entry whose binary is gone classifies as owned, so checking the kind first + // would return silently and leave a dangling link reported as a healthy + // install with no reason recorded anywhere. source := pluginBinaryPath(a.det.pluginRoot) if !fileExists(source) { - a.refuse("refused to create the PATH entry " + displayPath(target) + + a.refuse("refused to write the PATH entry " + displayPath(target) + ": its target " + displayPath(source) + " does not exist — a dangling link would shadow any working abcd on PATH. Reinstall the plugin, then re-run `abcd ahoy install`.") return } + if kind == binTargetOwnedSymlink { + return + } if kind == binTargetDevShim { if err := os.Remove(target); err != nil { return @@ -793,13 +813,39 @@ func (a *applyCtx) installPinnedSymlink(target string, kind binTargetKind) { a.echoChange("install_mode", "dev", "pinned") } if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { - a.refuse("could not create the install directory " + displayPath(filepath.Dir(target)) + ": " + err.Error()) + a.refuse("could not create the install directory " + displayPath(filepath.Dir(target)) + ": " + errText(err)) return } if err := os.Symlink(source, target); err == nil { a.note(target) } else { - a.refuse("could not write the PATH entry " + displayPath(target) + ": " + err.Error()) + a.refuse("could not write the PATH entry " + displayPath(target) + ": " + errText(err)) + } +} + +// noteReachability describes, on the install result itself, whether the entry +// that was just written can actually be RUN. +// +// Both conditions are required gaps no apply step can close, and neither reaches +// the user any other way: a required+non-resolvable gap is excluded from +// Remaining, InstallResult carries no gaps, and `abcd ahoy doctor` — where the +// gap text lives — cannot be invoked by name on a machine where abcd is not yet +// on PATH. Install is the one place a fresh user sees output, so it says both +// things here. It also computes them against the target actually written, which +// is the only place an explicit --bin-dir is known. +func (a *applyCtx) noteReachability() { + if a.binTarget == "" { + return + } + present, err := fsutil.ExistsNoFollow(a.binTarget) + if err != nil || !present { + return // nothing was installed here; a refusal already said why + } + if dir := filepath.Dir(a.binTarget); !dirOnPath(dir) { + a.refuse(pathReachMessage(dir)) + } + if e, ok := shadowingEntry(a.det.pluginRoot, a.binTarget); ok { + a.refuse(shadowMessage(e, a.binTarget)) } } @@ -868,7 +914,12 @@ func (a *applyCtx) stepVersionStamp() { // Uninstall removes the marker block and the owned PATH symlink only. It never // mutates hooks.json or the .abcd/ namespace. -func Uninstall(cwd string) (UninstallReceipt, error) { +// +// binDir names the directory to look in, for the one case detection cannot +// derive: an install placed by `--bin-dir` in a directory that is not on PATH is +// invisible to a PATH scan, so uninstall would otherwise orphan it. Empty means +// the derived location — an abcd-owned entry anywhere on PATH, else the default. +func Uninstall(cwd, binDir string) (UninstallReceipt, error) { abs, err := filepath.Abs(cwd) if err != nil { return UninstallReceipt{}, err @@ -891,6 +942,11 @@ func Uninstall(cwd string) (UninstallReceipt, error) { // rather than one blessed path. pluginRoot, ok := resolvePluginRoot() target := adoptedBinTarget(pluginRoot) + if binDir != "" { + if abs, aerr := filepath.Abs(binDir); aerr == nil { + target = filepath.Join(abs, binName) + } + } if target == "" { receipt.Symlink.Note = "no user-scope install location; left untouched" return receipt, nil diff --git a/internal/core/ahoy/bin_install_test.go b/internal/core/ahoy/bin_install_test.go index 364d12fe..5178b3b8 100644 --- a/internal/core/ahoy/bin_install_test.go +++ b/internal/core/ahoy/bin_install_test.go @@ -356,7 +356,7 @@ func TestUninstallReceiptCarriesNoAbsoluteHomePath(t *testing.T) { t.Setenv("PATH", binDir) linkOwned(t, filepath.Join(binDir, "abcd"), pluginRoot) - receipt, err := Uninstall(managedRepo(t)) + receipt, err := Uninstall(managedRepo(t), "") if err != nil { t.Fatal(err) } @@ -370,3 +370,213 @@ func TestUninstallReceiptCarriesNoAbsoluteHomePath(t *testing.T) { t.Errorf("receipt target = %q, want %q", receipt.Symlink.Target, want) } } + +// --------------------------------------------------------------------------- +// review round: what shadows what, and what says so +// --------------------------------------------------------------------------- + +// writeForeign plants a binary abcd does not own at path. +func writeForeign(t *testing.T, path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("#!/bin/sh\necho stale\n"), 0o755); err != nil { + t.Fatal(err) + } +} + +func notesJoined(notes []string) string { return strings.Join(notes, "\n") } + +// TestDetectShadowedByForeignCopyOnPath is the population the old README +// one-liner created: it COPIED the binary to /usr/local/bin, and a copy is a +// regular file, so it classifies foreign, is never adopted, and a correct entry +// installed behind it is never what runs. Reporting that as a clean pinned +// install is the lie this gap ends. +func TestDetectShadowedByForeignCopyOnPath(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + stale := filepath.Join(t.TempDir(), "usr-local-bin") + t.Setenv("PATH", stale+string(os.PathListSeparator)+binDir) + writeForeign(t, filepath.Join(stale, "abcd")) + linkOwned(t, filepath.Join(binDir, "abcd"), pluginRoot) + + det, err := Detect(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + g := gapByID(det.Gaps, "symlink.shadowed") + if g == nil { + t.Fatalf("a stale copy earlier on PATH produced no symlink.shadowed gap: %+v", det.Gaps) + } + if !g.Required { + t.Errorf("symlink.shadowed must be required: %+v", g) + } + if !strings.Contains(g.Detail, filepath.Join(stale, "abcd")) { + t.Errorf("the gap does not name the occupant: %q", g.Detail) + } + if m, _ := det.Signals["install_mode"].(string); m == "pinned" { + t.Errorf("install_mode = %q — a shadowed entry must not report as a healthy pinned install", m) + } +} + +// TestInstallBehindForeignCopyReportsShadowing is the same defect at the moment +// it is created: install writes a correct entry BEHIND the stale copy, and must +// say so on the result. A clean status with no note is how a machine ends up +// running the old binary forever. +func TestInstallBehindForeignCopyReportsShadowing(t *testing.T) { + home, _ := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + stale := filepath.Join(t.TempDir(), "usr-local-bin") + t.Setenv("PATH", stale+string(os.PathListSeparator)+binDir) + writeForeign(t, filepath.Join(stale, "abcd")) + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + + res, err := Install(repo, installOpts(), RefusingPrompter{}) + if err != nil { + t.Fatal(err) + } + if _, err := os.Lstat(filepath.Join(binDir, "abcd")); err != nil { + t.Fatalf("install wrote no entry at all: %v", err) + } + if !strings.Contains(notesJoined(res.Notes), filepath.Join(stale, "abcd")) { + t.Errorf("install did not report the shadowing binary; notes = %v", res.Notes) + } +} + +// TestDetectDanglingForeignEntryIsDescribed pins the half the first cut missed: +// dangling was computed only for entries abcd owns, so a foreign `abcd` pointing +// at nothing was reported as nothing at all — while still occupying the name. +func TestDetectDanglingForeignEntryIsDescribed(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + stale := filepath.Join(t.TempDir(), "stale") + t.Setenv("PATH", stale+string(os.PathListSeparator)+binDir) + if err := os.MkdirAll(stale, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(t.TempDir(), "gone", "abcd"), filepath.Join(stale, "abcd")); err != nil { + t.Fatal(err) + } + linkOwned(t, filepath.Join(binDir, "abcd"), pluginRoot) + + det, err := Detect(managedRepo(t)) + if err != nil { + t.Fatal(err) + } + g := gapByID(det.Gaps, "symlink.shadowed") + if g == nil { + t.Fatalf("a dangling foreign entry produced no gap: %+v", det.Gaps) + } + if !strings.Contains(g.Detail, "target is gone") { + t.Errorf("the gap does not describe the occupant as dangling: %q", g.Detail) + } +} + +// TestInstallForeignOccupantAtBinDirIsRefusedLoudly: the foreign bail-out wrote +// nothing and recorded nothing, and symlink.foreign is advisory so it is +// filtered out of Remaining — under an explicit --bin-dir the run reported a +// clean status while the remedy text pointed at a location the user overrode. +func TestInstallForeignOccupantAtBinDirIsRefusedLoudly(t *testing.T) { + setupUserScope(t) + dir := filepath.Join(t.TempDir(), "opt", "bin") + t.Setenv("PATH", dir) + writeForeign(t, filepath.Join(dir, "abcd")) + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + opts := installOpts() + opts.BinDir = dir + + res, err := Install(repo, opts, RefusingPrompter{}) + if err != nil { + t.Fatal(err) + } + joined := notesJoined(res.Notes) + if !strings.Contains(joined, dir) || !strings.Contains(joined, "does not own") { + t.Errorf("the foreign occupant was refused in silence; notes = %v", res.Notes) + } +} + +// TestInstallFreshPathGapIsCarriedOnNotes: "~/.local/bin is not on PATH" is a +// required, non-resolvable gap, so it is excluded from Remaining, absent from +// InstallResult, and printed only by `doctor` — which cannot be run by name on +// the very machine where abcd is not on PATH. Install carries it. +func TestInstallFreshPathGapIsCarriedOnNotes(t *testing.T) { + home, _ := setupUserScope(t, filepath.Join(t.TempDir(), "elsewhere")) + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + + res, err := Install(repo, installOpts(), RefusingPrompter{}) + if err != nil { + t.Fatal(err) + } + joined := notesJoined(res.Notes) + if !strings.Contains(joined, `export PATH="$HOME/.local/bin:$PATH"`) { + t.Errorf("install did not carry the PATH fix; notes = %v", res.Notes) + } + if strings.Contains(joined, home) { + t.Errorf("a note leaks the absolute home path: %v", res.Notes) + } +} + +// TestInstallDanglingEntryWithMissingBinaryRefusesLoudly: the owned-symlink +// early return preceded the source check, so a dangling entry plus a missing +// plugin binary produced status=partial with no note and no reason anywhere. +func TestInstallDanglingEntryWithMissingBinaryRefusesLoudly(t *testing.T) { + home, pluginRoot := setupUserScope(t) + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + linkOwned(t, filepath.Join(binDir, "abcd"), pluginRoot) + if err := os.Remove(pluginBinaryPath(pluginRoot)); err != nil { + t.Fatal(err) + } + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + + res, err := Install(repo, installOpts(), RefusingPrompter{}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(notesJoined(res.Notes), "does not exist") { + t.Errorf("a dangling entry with no binary to repoint at was silent; notes = %v", res.Notes) + } +} + +// TestInstallBinDirOffPathIsDescribedAndRemovable: an entry written outside PATH +// by an explicit --bin-dir is invisible to a PATH scan, so it was written and +// never described, and uninstall could not reach it. Install describes the +// directory it actually wrote, and uninstall takes the same flag. +func TestInstallBinDirOffPathIsDescribedAndRemovable(t *testing.T) { + setupUserScope(t, filepath.Join(t.TempDir(), "elsewhere")) + dir := filepath.Join(t.TempDir(), "opt", "bin") + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatal(err) + } + opts := installOpts() + opts.BinDir = dir + + res, err := Install(repo, opts, RefusingPrompter{}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(notesJoined(res.Notes), `export PATH="`+dir+`:$PATH"`) { + t.Errorf("the --bin-dir entry was written without describing its reachability; notes = %v", res.Notes) + } + receipt, err := Uninstall(repo, dir) + if err != nil { + t.Fatal(err) + } + if !receipt.Symlink.Removed { + t.Errorf("uninstall could not reach the --bin-dir entry: %+v", receipt.Symlink) + } +} diff --git a/internal/core/ahoy/detect.go b/internal/core/ahoy/detect.go index 62c68c44..fcc5daf6 100644 --- a/internal/core/ahoy/detect.go +++ b/internal/core/ahoy/detect.go @@ -451,9 +451,31 @@ func detectPathSymlink(pluginRoot string, pluginOK bool) []Gap { } gaps = append(gaps, detectBinDirOnPath(filepath.Dir(target), installed)...) + gaps = append(gaps, detectShadowedEntry(pluginRoot, target)...) return gaps } +// detectShadowedEntry reports an `abcd` that precedes abcd's own entry on PATH. +// Without it the most common pre-iss-171 machine reports a clean, pinned install +// forever while a stale copy keeps executing: the old one-liner COPIED the binary +// to /usr/local/bin, and a copy is a regular file, so it classifies foreign, is +// never adopted, and the new entry lands behind it. abcd cannot resolve this — +// removing a binary it does not own is exactly what it refuses to do — so the gap +// is required and diagnostic, naming the occupant and both remedies. +func detectShadowedEntry(pluginRoot, target string) []Gap { + e, ok := shadowingEntry(pluginRoot, target) + if !ok { + return nil + } + return []Gap{{ + ID: "symlink.shadowed", Category: ConfigChange, Scope: "machine", + Title: "another abcd on PATH answers first", + Detail: shadowMessage(e, target), + FixHint: "Remove or rename " + displayPath(e.path) + ", or install ahead of it with `abcd ahoy install --bin-dir `.", + Required: true, Resolvable: false, + }} +} + // detectBinDirOnPath reports an install directory that is not on PATH. It is a // gap in its own right: the entry exists and abcd still cannot be run by name, // which otherwise reads as a broken install. The remedy is printed, never @@ -464,14 +486,16 @@ func detectBinDirOnPath(dir string, installed bool) []Gap { return nil } // Silent while there is nothing there yet AND no install: the missing-entry - // gap already says what to do, and install will surface this straight after. + // gap already says what to do, and the install run itself emits the same + // wording as a note for the directory it actually writes (which is the only + // place that knows about an explicit --bin-dir). if !installed { return nil } return []Gap{{ ID: "path.bin_dir_not_on_path", Category: ConfigChange, Scope: "machine", Title: displayPath(dir) + " is not on PATH", - Detail: "abcd is installed at " + displayPath(filepath.Join(dir, binName)) + ", but " + displayPath(dir) + " is not in PATH, so `abcd` cannot be run by name.", + Detail: "abcd is installed at " + displayPath(filepath.Join(dir, binName)) + ". " + pathReachMessage(dir), FixHint: "Add it to your shell profile: " + exportPathLine(dir), Required: true, Resolvable: false, }} @@ -480,7 +504,9 @@ func detectBinDirOnPath(dir string, installed bool) []Gap { // detectInstallMode reports the current PATH-target install mode: "dev (tip // build)" when the track-latest shim occupies it, "pinned" when our owned symlink // does, and "" when the target is absent, foreign, or the plugin root is -// unresolved (nothing to attribute a mode to). +// unresolved (nothing to attribute a mode to). A mode that another `abcd` earlier +// on PATH shadows is reported as shadowed rather than as a healthy install: the +// entry is correct and it is still not what runs. func detectInstallMode(pluginRoot string, pluginOK bool) string { if !pluginOK { return "" @@ -493,14 +519,19 @@ func detectInstallMode(pluginRoot string, pluginOK bool) string { if present, err := fsutil.Exists(target); err == nil && !present { return "" } + mode := "" switch classifyBinTarget(target, pluginRoot) { case binTargetDevShim: - return "dev (tip build)" + mode = "dev (tip build)" case binTargetOwnedSymlink: - return "pinned" + mode = "pinned" default: return "" } + if _, shadowed := shadowingEntry(pluginRoot, target); shadowed { + return mode + " (shadowed on PATH)" + } + return mode } func detectHookManifest(pluginRoot string, pluginOK bool) []Gap { diff --git a/internal/core/ahoy/dev_install_test.go b/internal/core/ahoy/dev_install_test.go index e5fd48a8..ad9433d7 100644 --- a/internal/core/ahoy/dev_install_test.go +++ b/internal/core/ahoy/dev_install_test.go @@ -235,7 +235,7 @@ func TestUninstallRemovesDevShim(t *testing.T) { if _, err := Install(repo, devInstallOpts(), RefusingPrompter{}); err != nil { t.Fatal(err) } - receipt, err := Uninstall(repo) + receipt, err := Uninstall(repo, "") if err != nil { t.Fatal(err) } diff --git a/internal/core/ahoy/fsutil.go b/internal/core/ahoy/fsutil.go index f0a7c48f..6dde1b3f 100644 --- a/internal/core/ahoy/fsutil.go +++ b/internal/core/ahoy/fsutil.go @@ -34,6 +34,32 @@ func displayPath(p string) string { return p } +// displayText renders arbitrary text — an OS error string, which embeds whatever +// absolute path the syscall was given — for the same surfaces displayPath serves. +// The home prefix is replaced WHEREVER it appears, not only at the start, because +// an error reads "symlink /a/b /home/u/.local/bin/abcd: permission denied" and the +// username sits in the middle. A path that merely starts with the home string but +// continues into another name (/home/user2 against /home/user) is left alone. +func displayText(s string) string { + home, err := os.UserHomeDir() + if err != nil || home == "" || home == string(os.PathSeparator) { + return s + } + home = filepath.Clean(home) + sep := string(os.PathSeparator) + out := strings.ReplaceAll(s, home+sep, "~"+sep) + // A bare mention of the home directory itself, with no trailing component. + return strings.ReplaceAll(out, home, "~") +} + +// errText renders an error for a human-facing note with the same hygiene. +func errText(err error) string { + if err == nil { + return "" + } + return displayText(err.Error()) +} + // modeSymlink aliases os.ModeSymlink so detection reads naturally. const modeSymlink = os.ModeSymlink diff --git a/internal/core/ahoy/idempotency_test.go b/internal/core/ahoy/idempotency_test.go index cd85b171..d5cdfcfa 100644 --- a/internal/core/ahoy/idempotency_test.go +++ b/internal/core/ahoy/idempotency_test.go @@ -172,7 +172,7 @@ func TestUninstallInstallRoundTrip(t *testing.T) { if _, err := Install(repo, installOpts(), RefusingPrompter{}); err != nil { t.Fatal(err) } - if _, err := Uninstall(repo); err != nil { + if _, err := Uninstall(repo, ""); err != nil { t.Fatal(err) } // After uninstall the marker block is gone but .abcd/ survives. diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 0a9176cd..6e9979aa 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -212,13 +212,14 @@ func scanPathEntries(pluginRoot string) []pathEntry { continue } e := pathEntry{path: candidate, kind: classifyBinTarget(candidate, pluginRoot)} - if e.owned() { - // Stat FOLLOWS the link: a dangling entry is one whose target is gone. - // A stat error other than not-exist is not proof of a dangling link, so - // it reads as healthy rather than manufacturing a gap. - if present, err := fsutil.Exists(candidate); err == nil && !present { - e.dangling = true - } + // Stat FOLLOWS the link: a dangling entry is one whose target is gone. It + // is computed for EVERY entry, ours or not — a foreign dangling `abcd` + // still occupies the name and still shadows the entries behind it, and a + // scan that only looked at our own would report it as nothing at all. A + // stat error other than not-exist is not proof of a dangling link, so it + // reads as healthy rather than manufacturing a gap. + if present, err := fsutil.Exists(candidate); err == nil && !present { + e.dangling = true } entries = append(entries, e) } @@ -257,6 +258,79 @@ func effectiveBinTarget(pluginRoot string) string { return binTarget() } +// sameEntry reports whether two PATH entries name the same file. The DIRECTORY +// is canonicalised and the leaf name compared verbatim: resolving the leaf would +// follow the symlink and compare an entry against the binary it points at, which +// is a different question (and would make an owned entry equal to every other +// link into the same plugin root). +func sameEntry(a, b string) bool { + if a == "" || b == "" { + return false + } + if filepath.Base(a) != filepath.Base(b) { + return false + } + return resolvePath(filepath.Dir(a)) == resolvePath(filepath.Dir(b)) +} + +// shadowingEntry returns the first `abcd` on PATH that precedes target — the +// binary that actually answers when the user types `abcd`, whatever abcd itself +// installed. Writing a correct entry BEHIND an existing one leaves the machine +// running the old binary while every report says the install is clean, which is +// exactly the state a copied pre-iss-171 install (a regular file at +// /usr/local/bin/abcd, foreign because it is not a symlink of ours) produces. +func shadowingEntry(pluginRoot, target string) (pathEntry, bool) { + if target == "" { + return pathEntry{}, false + } + for _, e := range scanPathEntries(pluginRoot) { + if sameEntry(e.path, target) { + return pathEntry{}, false // target is reached first: nothing shadows it + } + return e, true + } + return pathEntry{}, false +} + +// describeEntry names what occupies a PATH entry, in the words a human needs to +// decide what to do about it. It never guesses: an entry abcd does not own is +// described by its shape, not by an assumption about its provenance. +func describeEntry(e pathEntry) string { + switch { + case e.kind == binTargetDevShim: + return "abcd's own dev-mode shim" + case e.kind == binTargetOwnedSymlink && e.dangling: + return "an abcd symlink whose target is gone" + case e.kind == binTargetOwnedSymlink: + return "an abcd symlink" + case e.dangling: + return "a symlink whose target is gone" + } + if fi, err := os.Lstat(e.path); err == nil && fi.Mode()&os.ModeSymlink != 0 { + if dest, rerr := os.Readlink(e.path); rerr == nil { + return "a symlink to " + displayPath(dest) + ", which abcd does not own" + } + return "a symlink abcd does not own" + } + return "a file abcd does not own" +} + +// shadowMessage is the single wording for a shadowed install, shared by the +// detection gap and the install-time note so the two can never drift. +func shadowMessage(e pathEntry, target string) string { + return displayPath(e.path) + " (" + describeEntry(e) + ") comes before " + + displayPath(target) + " on PATH, so it is what runs when you type `abcd`. " + + "Remove or rename it, or install ahead of it with `abcd ahoy install --bin-dir `. " + + "abcd never clobbers a binary it does not own." +} + +// pathReachMessage is the single wording for an install directory that is not on +// PATH, shared by the detection gap and the install-time note. +func pathReachMessage(dir string) string { + return displayPath(dir) + " is not on PATH, so `abcd` cannot be run by name. " + + "Add it to your shell profile: " + exportPathLine(dir) +} + // dirWritable reports whether a file can actually be created in dir. It probes // rather than reading the mode bits, so an ACL or a read-only mount answers // honestly. A directory abcd cannot write to is refused, never escalated. diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 9fa7dcb0..79d35c80 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -1583,7 +1583,8 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { ahoyCmd.AddCommand(installCmd) // uninstall - ahoyCmd.AddCommand(&cobra.Command{ + var uninstallBinDir string + uninstallCmd := &cobra.Command{ Use: "uninstall", Short: "Remove the marker block and owned PATH symlink (leaves .abcd/ intact)", Args: cobra.NoArgs, @@ -1592,7 +1593,7 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { if err != nil { return err } - receipt, err := ahoy.Uninstall(cwd) + receipt, err := ahoy.Uninstall(cwd, uninstallBinDir) if err != nil { return err } @@ -1602,7 +1603,9 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { fmt.Fprintf(w, " symlink: %s\n", symlinkNote(receipt)) }) }, - }) + } + uninstallCmd.Flags().StringVar(&uninstallBinDir, "bin-dir", "", "directory holding the PATH entry to remove; needed only when it was installed with --bin-dir into a directory that is not on PATH") + ahoyCmd.AddCommand(uninstallCmd) // doctor ahoyCmd.AddCommand(&cobra.Command{