From 24537c58d9e428a9effec4fccf95eedfbdcf42ad Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Thu, 23 Apr 2026 12:42:56 -0500 Subject: [PATCH] fix(update): actionable EACCES + suppress stale nudge after self-update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit atomicReplace wrapped os.Rename with %w, so a permission-denied write to /usr/local/bin/ana surfaced as raw "permission denied" plus the tempdir path — users had to guess that sudo was the fix. formatReplaceErr now detects fs.ErrPermission and returns platform-aware guidance: "Re-run with sudo" on Unix, "Re-run from an elevated (Administrator) shell" on Windows. The Windows rollback path also skips the misleading "recover from <.old>" wording when the root cause is EACCES, since .old sits in the same admin-only dir. The passive update-check goroutine captures the build-time version at process start and emitted its "new version available" nudge even after `ana update` had just swapped the binary. Suppress at the drainNudge layer when verb=="update" and verbErr==nil. Failed updates still nudge so the user gets the retry hint on the next run. Closes #28 --- cmd/ana/main.go | 24 +++++- cmd/ana/main_test.go | 47 +++++++++-- internal/update/CLAUDE.md | 2 +- internal/update/selfupdate.go | 33 ++++++-- internal/update/selfupdate_test.go | 125 +++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 17 deletions(-) diff --git a/cmd/ana/main.go b/cmd/ana/main.go index 2062448..4ed6eed 100644 --- a/cmd/ana/main.go +++ b/cmd/ana/main.go @@ -67,7 +67,7 @@ func run(args []string, stdio cli.IO, env func(string) string) error { // can place `--profile`/`--endpoint`/`--token-file` after the verb and // still have config resolution honour them — ParseGlobal stops at the // first positional, which would leave those flags invisible here. - global, _, err := cli.StripGlobals(args) + global, verbArgs, err := cli.StripGlobals(args) if err != nil { // Don't return here — let Dispatch produce the canonical usage error // (it prints root help + err to stderr). Fall through with zero @@ -131,10 +131,19 @@ func run(args []string, stdio cli.IO, env func(string) string) error { // nil flows through drainNudge as a no-op. nudgeCh := startNudge(env, loaded, global) err = cli.Dispatch(ctx, verbs, args, stdio) - drainNudge(nudgeCh, 500*time.Millisecond, err, stdio.Stderr) + drainNudge(nudgeCh, 500*time.Millisecond, err, firstVerb(verbArgs), stdio.Stderr) return err } +// firstVerb returns the leading positional from StripGlobals' verb-args slice, +// or "" for an empty slice. Keeps the drainNudge call site readable. +func firstVerb(verbArgs []string) string { + if len(verbArgs) == 0 { + return "" + } + return verbArgs[0] +} + // startNudge launches the passive update-check goroutine when enabled and // returns a buffered channel that drainNudge reads. Returns nil whenever the // check is skipped so drainNudge can short-circuit without touching the @@ -180,14 +189,21 @@ func startNudge(env func(string) string, loaded config.Config, global cli.Global // it produces a non-empty message and the verb did not return a help/usage // error, the message is written to stderr — we intentionally suppress the // nudge on help/usage paths so help text doesn't get crowded by an upgrade -// prompt. A nil ch (check was skipped) or a timeout is a clean no-op. -func drainNudge(ch chan string, timeout time.Duration, verbErr error, stderr io.Writer) { +// prompt. A successful `ana update` is also suppressed: the goroutine captured +// the pre-swap version at process start and can't know the binary was just +// replaced, so its "new version available" line would contradict the verb's +// own success output. A failed `ana update` still nudges — the user needs the +// retry hint. A nil ch (check was skipped) or a timeout is a clean no-op. +func drainNudge(ch chan string, timeout time.Duration, verbErr error, verb string, stderr io.Writer) { if ch == nil { return } if errors.Is(verbErr, cli.ErrHelp) || errors.Is(verbErr, cli.ErrUsage) { return } + if verb == "update" && verbErr == nil { + return + } timer := time.NewTimer(timeout) defer timer.Stop() select { diff --git a/cmd/ana/main_test.go b/cmd/ana/main_test.go index f0aa622..65ff2ef 100644 --- a/cmd/ana/main_test.go +++ b/cmd/ana/main_test.go @@ -576,13 +576,14 @@ func TestStartNudge_SkipConditions(t *testing.T) { } } -// TestDrainNudge covers the four branches: nil channel, help-err suppression, -// non-empty message printed, and empty message (no print). +// TestDrainNudge covers every branch: nil channel, help-err suppression, +// successful `ana update` suppression, failed `ana update` still nudges, +// non-empty message printed, empty message (no print), and the timeout. func TestDrainNudge(t *testing.T) { t.Parallel() t.Run("nil channel is a no-op", func(t *testing.T) { var buf bytes.Buffer - drainNudge(nil, time.Millisecond, nil, &buf) + drainNudge(nil, time.Millisecond, nil, "", &buf) if buf.Len() != 0 { t.Fatalf("stderr: %q", buf.String()) } @@ -591,16 +592,34 @@ func TestDrainNudge(t *testing.T) { ch := make(chan string, 1) ch <- "should not print" var buf bytes.Buffer - drainNudge(ch, time.Millisecond, cli.ErrHelp, &buf) + drainNudge(ch, time.Millisecond, cli.ErrHelp, "", &buf) if buf.Len() != 0 { t.Fatalf("stderr: %q", buf.String()) } }) + t.Run("update success suppresses", func(t *testing.T) { + ch := make(chan string, 1) + ch <- "stale nudge from pre-swap version" + var buf bytes.Buffer + drainNudge(ch, time.Millisecond, nil, "update", &buf) + if buf.Len() != 0 { + t.Fatalf("stderr: %q", buf.String()) + } + }) + t.Run("update failure still nudges", func(t *testing.T) { + ch := make(chan string, 1) + ch <- "retry hint" + var buf bytes.Buffer + drainNudge(ch, time.Millisecond, errors.New("permission denied"), "update", &buf) + if !strings.Contains(buf.String(), "retry hint") { + t.Fatalf("stderr: %q", buf.String()) + } + }) t.Run("message printed", func(t *testing.T) { ch := make(chan string, 1) ch <- "hello" var buf bytes.Buffer - drainNudge(ch, time.Millisecond, nil, &buf) + drainNudge(ch, time.Millisecond, nil, "", &buf) if !strings.Contains(buf.String(), "hello") { t.Fatalf("stderr: %q", buf.String()) } @@ -609,7 +628,7 @@ func TestDrainNudge(t *testing.T) { ch := make(chan string, 1) ch <- "" var buf bytes.Buffer - drainNudge(ch, time.Millisecond, nil, &buf) + drainNudge(ch, time.Millisecond, nil, "", &buf) if buf.Len() != 0 { t.Fatalf("stderr: %q", buf.String()) } @@ -617,13 +636,27 @@ func TestDrainNudge(t *testing.T) { t.Run("timeout", func(t *testing.T) { ch := make(chan string) // no sender var buf bytes.Buffer - drainNudge(ch, 10*time.Millisecond, nil, &buf) + drainNudge(ch, 10*time.Millisecond, nil, "", &buf) if buf.Len() != 0 { t.Fatalf("stderr: %q", buf.String()) } }) } +// TestFirstVerb covers the empty-slice branch and the typical case. +func TestFirstVerb(t *testing.T) { + t.Parallel() + if got := firstVerb(nil); got != "" { + t.Errorf("nil slice: got %q, want empty", got) + } + if got := firstVerb([]string{}); got != "" { + t.Errorf("empty slice: got %q, want empty", got) + } + if got := firstVerb([]string{"update", "--json"}); got != "update" { + t.Errorf("got %q, want update", got) + } +} + // TestUpdateCmd_Help short-circuits on --help like every other leaf verb. func TestUpdateCmd_Help(t *testing.T) { t.Parallel() diff --git a/internal/update/CLAUDE.md b/internal/update/CLAUDE.md index 9e9401b..ca9726f 100644 --- a/internal/update/CLAUDE.md +++ b/internal/update/CLAUDE.md @@ -7,7 +7,7 @@ Powers the passive "new release available" nudge and the `ana update` self-updat - `update.go` — package doc, `HTTPDoer` interface, and the two package-level URL vars (`latestReleaseURL`, `releasesBaseURL`) that tests repoint at `httptest` servers. - `semver.go` — `CmpSemver` + `parseSemver`. Three-int semver with optional `v` prefix and `-prerelease` suffix; prerelease sorts below release at the same X.Y.Z so `prerelease: auto` beta→stable flows notify correctly. Malformed input returns 0 (never trigger a nudge on junk). - `check.go` — passive nudge surface: `LatestRelease`, `CacheDeps`, `CachePath` (XDG → HOME), `ParseInterval` (`nil`→4h, `"0"`/`"disable"`→off), `CachedCheck` (reads/writes `update-check.json` atomically), plus the unexported `cacheFile` / `readCache` / `writeCache` / `shouldNotify`. -- `selfupdate.go` — `Deps` + `DefaultDeps` + `resolveDeps`, the `SelfUpdate` orchestration (resolve → compare → download → verify → extract → atomic replace), the `updateStatus` JSON shape + `emitStatus` (routes JSON through `cli.WriteJSON` for byte-compat), and `atomicReplace` (Unix rename-over; Windows rename-aside + rollback). +- `selfupdate.go` — `Deps` + `DefaultDeps` + `resolveDeps`, the `SelfUpdate` orchestration (resolve → compare → download → verify → extract → atomic replace), the `updateStatus` JSON shape + `emitStatus` (routes JSON through `cli.WriteJSON` for byte-compat), `atomicReplace` (Unix rename-over; Windows rename-aside + rollback), and `formatReplaceErr` (maps `fs.ErrPermission` to platform-aware sudo/Administrator guidance so EACCES isn't a dead-end for users). - `download.go` — HTTP helpers (`httpGet`, `downloadFile` — streams body to disk and returns the sha256 via `TeeReader`, avoiding a re-read, `downloadBody` — 1 MiB-capped for `checksums.txt`) and `verifyChecksum` (parses goreleaser's ` ` or sha256sum's ` *` format and compares against the caller-supplied hex). - `extract.go` — `extractBinary` dispatches on `archiveExt`; `extractFromTarGz` / `extractFromZip` walk the archive for the matching member and hand the reader to `writeBinary` (0755). - `_test.go` — one test file per source; shared helpers (`fakeDoer`, `releaseServer`, `stageUpdate`, `wantErr`, `withURLs`, `fakeArchive`) live in `update_test.go` per the repo convention. 100% coverage gate. diff --git a/internal/update/selfupdate.go b/internal/update/selfupdate.go index 4185352..96f3acd 100644 --- a/internal/update/selfupdate.go +++ b/internal/update/selfupdate.go @@ -2,8 +2,10 @@ package update import ( "context" + "errors" "fmt" "io" + "io/fs" "net/http" "os" "path/filepath" @@ -156,21 +158,42 @@ func emitStatus(w io.Writer, jsonOut bool, st updateStatus, plain string) error func atomicReplace(deps Deps, exePath, newPath string) error { if deps.GOOS != "windows" { if err := deps.Rename(newPath, exePath); err != nil { - return fmt.Errorf("update: replace %s: %w", exePath, err) + return formatReplaceErr(deps.GOOS, exePath, err) } return nil } oldPath := exePath + ".old" if err := deps.Rename(exePath, oldPath); err != nil { - return fmt.Errorf("update: rename %s -> %s: %w", exePath, oldPath, err) + // No .old exists yet, so no rollback wording; route through the + // permission-aware formatter for the EACCES-equivalent case. + return formatReplaceErr(deps.GOOS, exePath, err) } if err := deps.Rename(newPath, exePath); err != nil { - // Roll the old binary back; if that also fails we can't fix it for - // the user so tell them where the last-known-good copy lives. + // Roll the old binary back; if that also fails and the root cause + // is a permission denial, the `.old` we'd name in the recovery + // message sits in the same admin-only directory — point the user + // at elevation instead of a path they also can't touch. if rbErr := deps.Rename(oldPath, exePath); rbErr != nil { + if errors.Is(err, fs.ErrPermission) { + return formatReplaceErr(deps.GOOS, exePath, err) + } return fmt.Errorf("update: replace %s failed (%w); rollback also failed (%w); recover from %s", exePath, err, rbErr, oldPath) } - return fmt.Errorf("update: replace %s: %w", exePath, err) + return formatReplaceErr(deps.GOOS, exePath, err) } return nil } + +// formatReplaceErr turns an os.Rename failure into an actionable message. +// On fs.ErrPermission we tell the user exactly how to retry (sudo on Unix, +// Administrator on Windows) instead of leaking the tempdir path the user +// never picked. Every other error falls through to the plain wrapped form. +func formatReplaceErr(goos, exePath string, err error) error { + if errors.Is(err, fs.ErrPermission) { + if goos == "windows" { + return fmt.Errorf("update: cannot write to %s: permission denied. Re-run from an elevated (Administrator) shell, or reinstall ana into a user-writable directory (e.g. %%LOCALAPPDATA%%)", exePath) + } + return fmt.Errorf("update: cannot write to %s: permission denied. Re-run with sudo, or reinstall ana into a user-writable directory", exePath) + } + return fmt.Errorf("update: replace %s: %w", exePath, err) +} diff --git a/internal/update/selfupdate_test.go b/internal/update/selfupdate_test.go index 999eec1..b99d259 100644 --- a/internal/update/selfupdate_test.go +++ b/internal/update/selfupdate_test.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/http" "os" "strings" @@ -286,6 +287,130 @@ func TestEmitStatus_WriteError(t *testing.T) { wantErr(t, err, "emit status") } +// TestFormatReplaceErr_PermissionGuidance pins the user-facing wording for the +// EACCES branch on Unix and Windows. The strings are what users will see on +// perm-denied paths (e.g. /usr/local/bin/ana, C:\Program Files\ana), and the +// plan commits to exactly this copy — a silent wording change would erase the +// actionable hint that motivated this fix. +func TestFormatReplaceErr_PermissionGuidance(t *testing.T) { + t.Parallel() + t.Run("unix permission denied", func(t *testing.T) { + err := formatReplaceErr("linux", "/usr/local/bin/ana", fs.ErrPermission) + got := err.Error() + if !strings.Contains(got, "cannot write to /usr/local/bin/ana") { + t.Errorf("missing path: %q", got) + } + if !strings.Contains(got, "sudo") { + t.Errorf("missing sudo hint: %q", got) + } + if strings.Contains(got, "/tmp") || strings.Contains(got, "ana-update-") { + t.Errorf("leaked tempdir path: %q", got) + } + }) + t.Run("windows permission denied", func(t *testing.T) { + err := formatReplaceErr("windows", `C:\Program Files\ana\ana.exe`, fs.ErrPermission) + got := err.Error() + if !strings.Contains(got, "Administrator") { + t.Errorf("missing Administrator hint: %q", got) + } + if !strings.Contains(got, "LOCALAPPDATA") { + t.Errorf("missing LOCALAPPDATA hint: %q", got) + } + }) + t.Run("non-permission error falls through", func(t *testing.T) { + err := formatReplaceErr("linux", "/x/ana", errors.New("disk full")) + got := err.Error() + if !strings.Contains(got, "disk full") { + t.Errorf("should preserve cause: %q", got) + } + if strings.Contains(got, "sudo") { + t.Errorf("should not suggest sudo for non-permission errors: %q", got) + } + }) +} + +// permDeniedErr wraps a *PathError around fs.ErrPermission so errors.Is +// matches both the stdlib rename error shape (PathError) and the sentinel. +func permDeniedErr() error { + return &fs.PathError{Op: "rename", Path: "x", Err: fs.ErrPermission} +} + +// TestSelfUpdate_UnixEACCES drives the unix rename-permission branch end to end +// and asserts the user-facing message includes the sudo hint rather than the +// raw tempdir path. +func TestSelfUpdate_UnixEACCES(t *testing.T) { + exePath, deps := stageUpdate(t, "linux", "amd64", "ana") + deps.Rename = func(string, string) error { return permDeniedErr() } + r := &releaseServer{tag: "v2.0.0", archiveName: "ana_2.0.0_linux_amd64.tar.gz"} + r.archiveBody = fakeArchive(t, "tar.gz", "ana", []byte("NEW")) + srv := r.serve(t) + defer srv.Close() + withURLs(t, srv) + + err := SelfUpdate(context.Background(), deps, "1.0.0", io.Discard, false) + wantErr(t, err, "cannot write to "+exePath) + if !strings.Contains(err.Error(), "sudo") { + t.Errorf("missing sudo hint: %v", err) + } + if got, _ := os.ReadFile(exePath); string(got) != "old" { + t.Errorf("exe must not change on EACCES: %q", got) + } +} + +// TestSelfUpdate_WindowsEACCES_AsideFails covers the first-rename permission +// denial on Windows (no .old yet, no rollback) and pins the Administrator hint. +func TestSelfUpdate_WindowsEACCES_AsideFails(t *testing.T) { + exePath, deps := stageUpdate(t, "windows", "amd64", "ana.exe") + deps.Rename = func(string, string) error { return permDeniedErr() } + r := &releaseServer{tag: "v2.0.0", archiveName: "ana_2.0.0_windows_amd64.zip"} + r.archiveBody = fakeArchive(t, "zip", "ana.exe", []byte("NEW")) + srv := r.serve(t) + defer srv.Close() + withURLs(t, srv) + + err := SelfUpdate(context.Background(), deps, "1.0.0", io.Discard, false) + wantErr(t, err, "cannot write to "+exePath) + if !strings.Contains(err.Error(), "Administrator") { + t.Errorf("missing Administrator hint: %v", err) + } + // No ".old" recovery wording — the aside rename never succeeded. + if strings.Contains(err.Error(), "recover from") { + t.Errorf("should not reference .old recovery when aside failed: %v", err) + } +} + +// TestSelfUpdate_WindowsEACCES_SecondFailsRollbackFails covers the pathological +// case: aside succeeded, rename-in failed with EACCES, rollback also failed. +// The old "recover from <.old>" wording is misleading there — .old sits in the +// same admin-only directory — so the message must point at elevation. +func TestSelfUpdate_WindowsEACCES_SecondFailsRollbackFails(t *testing.T) { + exePath, deps := stageUpdate(t, "windows", "amd64", "ana.exe") + var n int + deps.Rename = func(o, np string) error { + n++ + switch n { + case 1: + return os.Rename(o, np) // aside succeeds + default: + return permDeniedErr() // rename-in + rollback both fail EACCES + } + } + r := &releaseServer{tag: "v2.0.0", archiveName: "ana_2.0.0_windows_amd64.zip"} + r.archiveBody = fakeArchive(t, "zip", "ana.exe", []byte("NEW")) + srv := r.serve(t) + defer srv.Close() + withURLs(t, srv) + + err := SelfUpdate(context.Background(), deps, "1.0.0", io.Discard, false) + wantErr(t, err, "cannot write to "+exePath) + if !strings.Contains(err.Error(), "Administrator") { + t.Errorf("missing Administrator hint: %v", err) + } + if strings.Contains(err.Error(), "recover from") { + t.Errorf("should not point at .old when both sides are admin-only: %v", err) + } +} + func TestDeps(t *testing.T) { t.Parallel() t.Run("DefaultDeps populates everything", func(t *testing.T) {