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 bdeb5120..a7f114f9 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,18 @@ 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; 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`) @@ -325,11 +335,16 @@ closes stdin and pre-answers: `abcd ahoy install --yes --refuse-adopt 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 @@ -370,7 +385,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..d805d66d 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": "", @@ -99,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/.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 6191dbce..c8465710 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,36 @@ 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. **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 - **The attribution gate reads the git identity, not only the message.** A commit diff --git a/README.md b/README.md index edf361e9..47ac0614 100644 --- a/README.md +++ b/README.md @@ -177,22 +177,40 @@ 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 the installer 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 `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 92ec1e2c..c770f049 100644 --- a/commands/ahoy.md +++ b/commands/ahoy.md @@ -52,9 +52,28 @@ 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. + +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. Prompts read stdin whether or not stdin is a terminal, so an answer can be relayed without one: @@ -104,8 +123,12 @@ 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`. 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 263e6491..8b806c00 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 @@ -68,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/ahoy.go b/internal/core/ahoy/ahoy.go index a5d7ba62..b01dd494 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"` // OptionalSkipped names the optional gaps a --yes run deliberately did not // apply — today the advisory git-identity pin alone. --yes approves every // resolvable CATEGORY, but it never writes the pin, because the pin captures diff --git a/internal/core/ahoy/apply.go b/internal/core/ahoy/apply.go index 5daff66b..32e487e8 100644 --- a/internal/core/ahoy/apply.go +++ b/internal/core/ahoy/apply.go @@ -52,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 against a // confirmed answer (never under --yes), as the gap's fix hint @@ -61,7 +72,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)) && @@ -85,6 +96,7 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) autoYes: opts.Yes, devMode: opts.Dev, modeForced: modeForced, + binTarget: binTargetPath, } // Ordered apply steps. @@ -101,6 +113,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) @@ -119,10 +133,82 @@ func Install(cwd string, opts InstallOptions, p Prompter) (InstallResult, error) Changes: ac.changes, Remaining: remaining, DeclinedCategories: declined, + Notes: ac.notes, OptionalSkipped: optionalSkipped(opts, final.Gaps), }, 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 { @@ -134,9 +220,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 @@ -146,6 +234,12 @@ type applyCtx struct { // note is the receipt seam; it lives in receipt.go with the path scrub it // applies, because every apply step below reports its writes through it. +// 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 @@ -616,20 +710,32 @@ 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 + // 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) @@ -663,9 +769,47 @@ 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)) { + // 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) + ": " + errText(err)) + } +} + // 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) { + // 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 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 } @@ -676,22 +820,50 @@ 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)) + ": " + errText(err)) 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) + ": " + 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)) } } // 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 } @@ -749,7 +921,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 @@ -766,10 +943,24 @@ 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 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 + } + // 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..849c0340 --- /dev/null +++ b/internal/core/ahoy/bin_install_test.go @@ -0,0 +1,580 @@ +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, and the receipt scrub owned by iss-177 renders a + // user-scope write home-relative — so the entry appears in tilde form. + if !containsPath(res.Writes, "~/.local/bin/abcd") { + 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) + } +} + +// --------------------------------------------------------------------------- +// 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 69827c0e..fcc5daf6 100644 --- a/internal/core/ahoy/detect.go +++ b/internal/core/ahoy/detect.go @@ -381,65 +381,157 @@ 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, + }) + } + + target := effectiveBinTarget(pluginRoot) + if target == "" { + return gaps // no home directory: there is no user-scope target to report on } - if fi.Mode()&modeSymlink == 0 { + 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)...) + 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 } - if resolveSymlinkDest(target, dest) == resolvePath(expected) { + 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 +// 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 + } + // Silent while there is nothing there yet AND no install: the missing-entry + // 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: "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)) + ". " + pathReachMessage(dir), + FixHint: "Add it to your shell profile: " + exportPathLine(dir), + Required: true, Resolvable: false, }} } // 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 "" } - 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 "" + } + 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 23598915..6dde1b3f 100644 --- a/internal/core/ahoy/fsutil.go +++ b/internal/core/ahoy/fsutil.go @@ -3,8 +3,63 @@ 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 +} + +// 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 cdfafaa6..6e9979aa 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -125,12 +125,234 @@ 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)} + // 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) + } + 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() +} + +// 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. +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 e1cfaac7..a278086e 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, ", ")) } @@ -1578,6 +1585,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") @@ -1585,7 +1593,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, @@ -1594,7 +1603,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 } @@ -1604,7 +1613,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{ @@ -1681,8 +1692,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") }