From a8b3ad1ed56d77b0f4ec497819497deebe77ca27 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Sun, 14 Jun 2026 23:28:03 +0100 Subject: [PATCH 1/7] feat(update): detect install method to tailor or suppress update notices Detect how the CLI was installed and adjust the "new release available" notice accordingly: - System package managers (apt, yum/dnf, apk) are detected via a marker file installed by the nfpm packages. The notice is suppressed and the network check is skipped, since the OS handles updates. - Homebrew, Scoop, npm, and the bash installer get the exact upgrade command, built from config fields so vendor builds stay correct. - Unknown installs keep the generic GitHub link. Detection precedence: an explicit override (INSTALL_METHOD env or wrapper.install_method config), then the package marker, then the resolved executable path (npm/scoop/homebrew), then standard bin dirs (script). The notice is now shown before the command runs, using a latest-version cache in state.json refreshed by the background check, and throttled to once a week (LastNotified). This removes the per-run nagging. Adds Wrapper.NpmPackage, Wrapper.InstallerURL, and Wrapper.InstallMethod config fields, and the packaging/install-source marker to both nfpm entries. Documents the behavior in README and the design doc; Phase 2 (opt-in self-update) is planned in docs/design. Co-Authored-By: Claude Opus 4.8 (1M context) --- .goreleaser.yaml | 16 + CLAUDE.md | 10 +- README.md | 18 + commands/root.go | 66 +-- commands/root_test.go | 42 ++ .../update-message-install-detection.md | 422 ++++++++++++++++++ internal/config/platformsh-cli.yaml | 1 + internal/config/schema.go | 7 +- internal/config/upsun-cli.yaml | 2 + internal/install.go | 211 +++++++++ internal/install_test.go | 164 +++++++ internal/state/state.go | 4 +- internal/update.go | 57 ++- internal/update_test.go | 47 ++ packaging/install-source | 1 + 15 files changed, 1033 insertions(+), 35 deletions(-) create mode 100644 commands/root_test.go create mode 100644 docs/design/update-message-install-detection.md create mode 100644 internal/install.go create mode 100644 internal/install_test.go create mode 100644 internal/update_test.go create mode 100644 packaging/install-source diff --git a/.goreleaser.yaml b/.goreleaser.yaml index d5cc87637..a5cd59716 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -282,6 +282,14 @@ nfpms: dst: /etc/bash_completion.d/platform - src: completion/zsh/_platform dst: /usr/local/share/zsh/site-functions/_platform + # Marks the install as managed by a system package manager, so the CLI + # suppresses its update message. See internal/install.go. The dst must stay + # at /share//install-source, where is the parent of + # the binary's dir (default bindir /usr/bin -> prefix /usr); changing the + # nfpm bindir would silently break suppression. + - src: packaging/install-source + dst: /usr/share/platformsh-cli/install-source + file_info: { mode: 0644 } apk: signature: key_file: "{{ with .Env.RSA_SIGNING_KEY_FILE }}{{ . }}{{ end }}" @@ -309,6 +317,14 @@ nfpms: dst: /etc/bash_completion.d/upsun - src: completion/zsh/_upsun dst: /usr/local/share/zsh/site-functions/_upsun + # Marks the install as managed by a system package manager, so the CLI + # suppresses its update message. See internal/install.go. The dst must stay + # at /share//install-source, where is the parent of + # the binary's dir (default bindir /usr/bin -> prefix /usr); changing the + # nfpm bindir would silently break suppression. + - src: packaging/install-source + dst: /usr/share/upsun/install-source + file_info: { mode: 0644 } apk: signature: key_file: "{{ with .Env.RSA_SIGNING_KEY_FILE }}{{ . }}{{ end }}" diff --git a/CLAUDE.md b/CLAUDE.md index c9cea0d88..dea872ac3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -163,4 +163,12 @@ PHP version is still injected via ldflags: ### Update Checks -The CLI checks for updates from GitHub releases (when Wrapper.GitHubRepo is set in config). This runs in a background goroutine and prints a message after command execution. +The CLI checks for updates from GitHub releases (when Wrapper.GitHubRepo is set in config). The network check runs in a background goroutine and caches the latest known version in `state.json`; the notice is shown before the command on a later run (see `internal/update.go`). + +Install-method detection (`internal/install.go`) tailors or suppresses the notice: +- System package managers (apt, yum/dnf, apk) are detected via a marker file installed by the nfpm packages (`packaging/install-source` → `/usr/share//install-source`). The notice is suppressed because the OS handles updates. +- Homebrew, Scoop, npm, and the bash installer get a tailored upgrade command, built from config fields (`Wrapper.HomebrewTap`, `Wrapper.NpmPackage`, `Wrapper.InstallerURL`, `Application.Executable`). +- The notice is throttled to once a week (`LastNotified` in state) and only shown in an interactive terminal. +- `INSTALL_METHOD` forces the method; `UPDATES_CHECK=0` disables checks entirely. + +See `docs/design/update-message-install-detection.md` for the full design, including the planned Phase 2 (opt-in self-update). diff --git a/README.md b/README.md index e3024a9bf..a567858ea 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,24 @@ sudo apt-get update && sudo apt-get upgrade upsun-cli sudo dnf upgrade -y upsun-cli ``` +### Update notifications + +When a newer release is available, the CLI prints a short notice with the +upgrade command for the tool you installed it with. It detects the install +method automatically and, for system package managers (apt, yum/dnf, apk), stays +silent because those update the CLI through the OS. + +The notice is shown at most once a week and only in an interactive terminal. To +control it: + +- `UPSUN_CLI_UPDATES_CHECK=0` disables update checks and notices entirely. +- `UPSUN_CLI_INSTALL_METHOD=` forces the detected install method, where + `` is one of `homebrew`, `scoop`, `npm`, `package`, or `script`. Use + `package` to silence the notice when the OS manages updates. + +(For the `platform` command, the prefix is `PLATFORMSH_CLI_` instead of +`UPSUN_CLI_`.) + ## Building Build a single binary: diff --git a/commands/root.go b/commands/root.go index 31c3c7cfa..cd1ba1c0c 100644 --- a/commands/root.go +++ b/commands/root.go @@ -40,10 +40,7 @@ func Execute(cnf *config.Config) error { } func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cobra.Command { - var ( - updateMessageChan = make(chan *internal.ReleaseInfo, 1) - versionCommand = newVersionCommand(cnf) - ) + versionCommand := newVersionCommand(cnf) cmd := &cobra.Command{ Use: cnf.Application.Executable, Short: cnf.Application.Name, @@ -77,9 +74,16 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob os.Exit(0) } if cnf.Wrapper.GitHubRepo != "" { + // Show any update found by a previous run, before the command's + // output. The check itself runs in the background (below) and + // caches its result for the next invocation. + if rel := internal.PendingNotification(cnf, config.Version); rel != nil { + printUpdateMessage(cmd.ErrOrStderr(), rel, cnf) + internal.MarkNotified(cnf) + } go func() { - rel, _ := internal.CheckForUpdate(cnf, config.Version) - updateMessageChan <- rel + //nolint:errcheck // a failed update check should not affect the command + internal.CheckForUpdate(cnf, config.Version) }() } if alt.ShouldUpdate(cnf) { @@ -101,11 +105,6 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob return } checkShellConfigLeftovers(cmd.ErrOrStderr(), cnf) - select { - case rel := <-updateMessageChan: - printUpdateMessage(cmd.ErrOrStderr(), rel, cnf) - default: - } }, } @@ -212,19 +211,14 @@ func printUpdateMessage(w io.Writer, newRelease *internal.ReleaseInfo, cnf *conf return } - fmt.Fprintf(w, "\n\n%s %s → %s\n", + fmt.Fprintf(w, "\n%s %s → %s\n", color.YellowString(fmt.Sprintf("A new release of the %s is available:", cnf.Application.Name)), color.CyanString(config.Version), color.CyanString(newRelease.Version), ) - executable, err := os.Executable() - if err == nil && cnf.Wrapper.HomebrewTap != "" && isUnderHomebrew(executable) { - fmt.Fprintf( - w, - "To upgrade, run: brew update && brew upgrade %s\n", - color.YellowString(cnf.Wrapper.HomebrewTap), - ) + if cmd := upgradeCommand(cnf); cmd != "" { + fmt.Fprintf(w, "To upgrade, run: %s\n", color.YellowString(cmd)) } else if cnf.Wrapper.GitHubRepo != "" { fmt.Fprintf( w, @@ -236,19 +230,31 @@ func printUpdateMessage(w io.Writer, newRelease *internal.ReleaseInfo, cnf *conf fmt.Fprintf(w, "%s\n\n", color.YellowString(newRelease.URL)) } -func isUnderHomebrew(binary string) bool { - brewExe, err := exec.LookPath("brew") - if err != nil { - return false - } +// upgradeCommand returns the upgrade command for the detected install method, or +// an empty string when there is no tailored command (the caller then falls back +// to a generic link). +func upgradeCommand(cnf *config.Config) string { + return upgradeCommandFor(cnf, internal.DetectInstallMethod(cnf)) +} - brewPrefixBytes, err := exec.Command(brewExe, "--prefix").Output() - if err != nil { - return false +func upgradeCommandFor(cnf *config.Config, method internal.InstallMethod) string { + switch method { + case internal.InstallHomebrew: + if cnf.Wrapper.HomebrewTap != "" { + return "brew update && brew upgrade " + cnf.Wrapper.HomebrewTap + } + case internal.InstallScoop: + return "scoop update " + cnf.Application.Executable + case internal.InstallNpm: + if cnf.Wrapper.NpmPackage != "" { + return "npm install -g " + cnf.Wrapper.NpmPackage + "@latest" + } + case internal.InstallScript: + if cnf.Wrapper.InstallerURL != "" { + return "curl -fsSL " + cnf.Wrapper.InstallerURL + " | bash" + } } - - brewBinPrefix := filepath.Join(strings.TrimSpace(string(brewPrefixBytes)), "bin") + string(filepath.Separator) - return strings.HasPrefix(binary, brewBinPrefix) + return "" } func debugLogf(format string, v ...any) { diff --git a/commands/root_test.go b/commands/root_test.go new file mode 100644 index 000000000..29ba46e19 --- /dev/null +++ b/commands/root_test.go @@ -0,0 +1,42 @@ +package commands + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/upsun/cli/internal" +) + +func TestUpgradeCommandFor(t *testing.T) { + cnf := testConfig() + cnf.Wrapper.HomebrewTap = "upsun/tap/upsun-cli" + cnf.Wrapper.NpmPackage = "upsun" + cnf.Wrapper.InstallerURL = "https://example.com/installer.sh" + cnf.Application.Executable = "upsun" + + cases := []struct { + method internal.InstallMethod + want string + }{ + {internal.InstallHomebrew, "brew update && brew upgrade upsun/tap/upsun-cli"}, + {internal.InstallScoop, "scoop update upsun"}, + {internal.InstallNpm, "npm install -g upsun@latest"}, + {internal.InstallScript, "curl -fsSL https://example.com/installer.sh | bash"}, + {internal.InstallPackage, ""}, // suppressed; no tailored command + {internal.InstallUnknown, ""}, // falls back to the generic link + } + for _, c := range cases { + t.Run(string(c.method), func(t *testing.T) { + assert.Equal(t, c.want, upgradeCommandFor(cnf, c.method)) + }) + } +} + +func TestUpgradeCommandForMissingConfigFallsBack(t *testing.T) { + cnf := testConfig() // no Wrapper.* fields set + // Homebrew/npm/script require their config field; without it, fall back (empty). + assert.Empty(t, upgradeCommandFor(cnf, internal.InstallHomebrew)) + assert.Empty(t, upgradeCommandFor(cnf, internal.InstallNpm)) + assert.Empty(t, upgradeCommandFor(cnf, internal.InstallScript)) +} diff --git a/docs/design/update-message-install-detection.md b/docs/design/update-message-install-detection.md new file mode 100644 index 000000000..721ad1e7b --- /dev/null +++ b/docs/design/update-message-install-detection.md @@ -0,0 +1,422 @@ +# Design: install-method detection for update messages + +Status: draft +Author: Claude Code (for Patrick Dawkins) +Date: 2026-06-14 + +## Background + +The CLI checks GitHub for a newer release and, if one exists, prints a message +after the command runs. The check runs in a background goroutine started in +`PersistentPreRun` (`commands/root.go:75`), and the message is printed in +`PersistentPostRun` via a non-blocking channel read (`commands/root.go:95`). + +The check itself lives in `internal/update.go`: + +- `CheckForUpdate` gates on `shouldCheckForUpdate`, throttles to + `Updates.CheckInterval` (default 3600s) using `state.json`, fetches + `releases/latest` from GitHub, and compares versions. +- `shouldCheckForUpdate` (`internal/update.go:61`) skips the check for dev + builds, when `Updates.Check` is false, when `UPDATES_CHECK=0`, in CI, + and when stdout/stderr are not terminals. + +The message is formatted by `printUpdateMessage` (`commands/root.go:202`). Today +it tailors the upgrade instruction in exactly one way: if the binary sits under +the Homebrew prefix (`isUnderHomebrew`, which shells out to `brew --prefix`), it +prints `brew update && brew upgrade `. Otherwise it prints a generic +`https://github.com/#upgrade` link. + +## Problem + +The message is shown identically regardless of how the CLI was installed, and it +is shown even when the user cannot act on it usefully: + +1. For users who installed via a system package manager (`apt`, `yum`/`dnf`, + `apk`), the OS updates the CLI on its normal schedule. Telling them to visit a + GitHub page is noise, and following it would fight the package manager. +2. For Scoop, npm, and the bash installer, the generic GitHub link is less + helpful than the one command that actually upgrades their installation. + +## Goals + +- Suppress the update message entirely when the CLI is managed by an + auto-updating system package manager (`apt`, `yum`/`dnf`, `apk`). +- For manually-updated channels, print the exact upgrade command for that + channel (Homebrew, Scoop, npm, bash installer). +- Fall back to today's generic GitHub link when the method is unknown. +- Make the notice substantially less naggy: interactive-only, throttled to once + a week, shown before the command rather than buried after it. +- Offer one-tap, opt-in self-update for channels where the upgrade command is + local and unprivileged, then re-run the original command on the new version. +- Keep detection off the synchronous command path (the network check already + runs in the background goroutine) and avoid new latency on every invocation. +- Keep the design vendorizable — no hard-coded `upsun`-specific strings in Go. +- Provide an escape hatch to force or override detection. + +## Non-goals + +- Silent or unattended self-update. Updates are only ever performed after an + explicit interactive prompt. `self:update` (the PHP command) stays disabled. +- Auto-running privileged or remote-code upgrades (`sudo`, `curl … | sh`). For + those channels we print the command rather than executing it (see Phase 2). +- Changing the network-check throttle, the CI gate, or the TTY gate. +- Detecting manual `dpkg -i` / `rpm -i` installs that bypass a repo. These are + treated as "package" (auto-updating); see Edge cases. + +## Phasing + +This grew from "tailor the message" into "opt-in self-update", so it ships in +phases: + +- **Phase 1** — install-method detection, suppress for packages, tailor wording, + weekly display throttle, show-before-via-cache. No process re-exec; low risk. +- **Phase 2** — interactive prompt + auto-update + re-exec for the unprivileged + channels (Homebrew, Scoop, npm). +- **Phase 3 (optional)** — move the bash installer's `raw` method to a + user-local path, which removes `sudo` and lets `script` installs join the + one-tap auto-update set. + +## Distribution channel inventory + +Verified from `.goreleaser.yaml`, `installer.sh`, and `npm/wrapper/`. + +| Channel | Where the running binary lives | Auto-updates? | Desired behavior | +|---|---|---|---| +| Homebrew (`brews:`) | `$(brew --prefix)/bin/` (symlink into `…/Cellar/`) | No | `brew update && brew upgrade ` | +| Scoop (`scoops:`, Windows) | `…\scoop\apps\\current\.exe` (shim in `…\scoop\shims\`) | No | `scoop update ` | +| npm (`npm/wrapper/`) | `…/node_modules/@upsun/cli-/bin/` | No | `npm install -g @latest` | +| apt / `.deb` (`nfpms:`) | `/usr/bin/` | Yes | suppress | +| yum·dnf / `.rpm` (`nfpms:`) | `/usr/bin/` | Yes | suppress | +| apk / `.apk` (`nfpms:`) | `/usr/bin/` | Yes | suppress | +| bash installer, raw method | Linux: `/usr/bin/`; macOS: `/usr/local/bin/`; or `$INSTALL_DIR` | No | re-run installer command | +| Unknown / built from source | anywhere | — | generic GitHub `#upgrade` link | + +### The core ambiguity + +On Linux the bash installer's `raw` method installs to `/usr/bin` (`installer.sh:37`), +which is the same location nfpm packages use. **Path alone cannot distinguish a +distro package (auto-updating, suppress) from a raw script install (manual, +show).** This is exactly the distinction the feature needs, so a path heuristic +is insufficient on Linux; we need a positive signal that a package manager owns +the binary. + +The bash installer's `apt`/`yum`/`apk` methods install the distro package from +`repositories.upsun.com`, so those collapse into the package case and carry +whatever signal the package carries. + +## Detection design + +### A positive package marker (resolves the Linux ambiguity) + +Have the nfpm packages install a small marker file that the binary can read at +runtime. nfpm `contents` can place a file at any absolute path: + +```yaml +# .goreleaser.yaml, in each nfpms entry +contents: + - src: packaging/install-source + dst: /usr/share//install-source + file_info: { mode: 0644 } +``` + +`packaging/install-source` contains the single word `package`. We do not need to +distinguish apt/yum/apk for suppression — all three auto-update — so one marker +value is enough, and we avoid splitting the single nfpm entry into three. + +At runtime, presence of the marker (found relative to the resolved executable, or +at the known `/usr/share//install-source` path) means "managed by a system +package manager → auto-updating". Its absence at `/usr/bin/` means the +binary got there some other way (raw installer) → not suppressed. + +This keeps all packaging changes inside this repo's `.goreleaser.yaml`; the +Homebrew tap and Scoop manifests (in `upsun/homebrew-tap`) need no changes +because those channels are detected by path. + +### Detection precedence + +A new function `DetectInstallMethod(cnf) InstallMethod` resolves the method once +(memoized with `sync.Once`). Order, first match wins: + +1. **Override.** `INSTALL_METHOD` env var (e.g. `UPSUN_CLI_INSTALL_METHOD`), + or an optional `wrapper.install_method` config key. Parsed into the enum; + unknown values are ignored with a debug log. Lets CI, distro maintainers, and + power users force behavior. +2. **Package marker.** Marker file present → `package`. +3. **npm.** Resolved executable path contains `node_modules` and the package + scope (`@/cli-` derived from config, or simply `node_modules`). → `npm`. +4. **Scoop** (Windows only). Path contains a `scoop{sep}` segment. → `scoop`. +5. **Homebrew.** Existing `isUnderHomebrew` logic (path under `brew --prefix`), + plus a cheap pre-check for `/Cellar/` in the path to avoid the subprocess when + obviously not Homebrew. → `homebrew`. +6. **Script.** Otherwise, if the binary is in a normal bin dir + (`/usr/bin`, `/usr/local/bin`, `$INSTALL_DIR`-style) → `script`. +7. **Unknown.** Anything else (e.g. `go run`, source builds). → `unknown`. + +Resolve symlinks with `filepath.EvalSymlinks(os.Executable())` before matching so +that shimmed/symlinked installs report their real location. + +Because detection runs inside the background goroutine (`CheckForUpdate`) and at +message time — never on the synchronous hot path — shelling out to `brew --prefix` +remains acceptable. Memoization means it happens at most once per process. + +### Enum + +```go +type InstallMethod string + +const ( + InstallUnknown InstallMethod = "" + InstallHomebrew InstallMethod = "homebrew" + InstallScoop InstallMethod = "scoop" + InstallNpm InstallMethod = "npm" + InstallPackage InstallMethod = "package" // apt / yum / dnf / apk + InstallScript InstallMethod = "script" // bash installer, raw method +) + +// AutoUpdating reports whether the host system updates the CLI on its own. +func (m InstallMethod) AutoUpdating() bool { return m == InstallPackage } +``` + +### Wiring into the check + +In `shouldCheckForUpdate`, short-circuit when auto-updating so we skip both the +message and the network request: + +```go +if DetectInstallMethod(cnf).AutoUpdating() { + return false +} +``` + +`printUpdateMessage` switches on the method to choose the upgrade line. Since +`AutoUpdating` methods never reach the message, the switch only handles the +manual channels plus the unknown fallback (current behavior preserved). + +## Behavior matrix + +"Auto-updatable" means the upgrade command is local and unprivileged, so we may +offer to run it for the user (Phase 2). `script`/`unknown` only ever print a +hint. + +| Method | Notice shown? | Upgrade command | Auto-updatable? | +|---|---|---|---| +| `package` | No (suppressed) | — | — (OS handles it) | +| `homebrew` | Yes | `brew update && brew upgrade ` | Yes | +| `scoop` | Yes | `scoop update ` | Yes | +| `npm` | Yes | `npm install -g @latest` | Yes | +| `script` | Yes | `curl -fsSL \| sh` | No (remote + maybe sudo)¹ | +| `unknown` | Yes | `follow the instructions at https://github.com/#upgrade` (today's text) | No | + +¹ Becomes auto-updatable in Phase 3 if raw installs move to a user-local, +sudo-free path. + +All upgrade strings are built from config fields, not literals, so vendor builds +get correct instructions. + +## Reducing nagginess + +The point of the notice is to be useful, not to interrupt. Four changes make it +quiet enough to keep on by default, plus an opt-in updater. + +### Interactive-only + +The passive one-line notice shows on any TTY (today's gate). The *prompt* and any +auto-update only happen when fully interactive: TTY **and** not `--yes` / +`--no-interaction`. Also skip plumbing commands (`completion`, `--version`) so we +never corrupt machine-readable output. + +### Weekly display throttle + +Decouple "how often we check the network" (hourly, `LastChecked`) from "how often +we tell the user" (weekly). Add a separate timestamp; set it whenever we show the +notice or prompt, regardless of whether the user accepts or declines, so we stay +quiet for a week either way. + +### Show before the command, via a cache + +Today the notice prints in `PersistentPostRun`, after the output, because the +background check may not have finished. Instead, cache the latest known version +in `state.json`. `PersistentPreRun` reads the cache and shows/prompts +immediately — no blocking — while the background goroutine refreshes the cache +for the *next* invocation. The notice therefore lags real availability by one +command, which is acceptable, and it appears before output (and is a prerequisite +for prompting, which cannot happen after the command has run). + +State (`internal/state/state.go`) gains: + +```go +Updates struct { + LastChecked int64 `json:"last_checked"` + LastNotified int64 `json:"last_notified,omitempty"` + KnownLatestVersion string `json:"known_latest_version,omitempty"` +} +``` + +### Opt-in one-tap update + re-exec (Phase 2) + +When fully interactive, an update is known, the weekly throttle allows it, and +the method is auto-updatable (Homebrew/Scoop/npm), prompt: + +``` +A new release of the Upsun CLI is available: v2.3.1 → v2.4.0 +Update now? [Y/n] +``` + +- **Default.** Enter = yes (your "just hit enter" flow) — but *only* for the + unprivileged channels. We never default-run a `sudo` or `curl … | sh` upgrade; + for `script`/`unknown` we print the command and do not prompt to run it. +- **On accept.** Run the channel's upgrade command, streaming its output. On + success, re-resolve the (possibly relocated) executable and re-run the original + command on the new version: + - Unix: `syscall.Exec(newBinary, os.Args, os.Environ())` — replaces the process + image. Safe because we prompt in `PersistentPreRun`, before any command logic + has run. + - Windows: spawn the new binary with the original args, forward stdio, exit with + its status (the pattern `npm/wrapper/bin/upsun.js` already uses). + - On upgrade failure: print a warning and continue with the current version — + never block the user's command. +- **On decline.** Set `LastNotified` and run the command normally; stay quiet for + a week. + +Because the prompt and update run in `PersistentPreRun`, and the legacy PHP layer +is already invoked with `UPDATES_CHECK=0` (`internal/legacy/legacy.go:139`), +there is no double-notification when delegating to PHP commands. + +### The bash installer's destination (Phase 3, optional) + +The `raw` method installs to `/usr/bin` on Linux (`installer.sh:37`), which needs +`sudo` and collides with the package path (the source of the detection +ambiguity). Targeting a user-local dir instead (`$XDG_BIN_HOME`, else +`~/.local/bin`) would remove `sudo`, make `script` unambiguous from `package` +without relying on the marker, and let `script` installs join the one-tap update +set. Back-compat: existing `/usr/bin` installs stay where they are; PATH ordering +must be considered. Tracked separately from Phases 1–2. + +## Config schema changes + +Add to the `Wrapper` struct (`internal/config/schema.go:22`): + +```go +Wrapper struct { + HomebrewTap string `yaml:"homebrew_tap,omitempty"` + GitHubRepo string `yaml:"github_repo,omitempty"` + NpmPackage string `yaml:"npm_package,omitempty"` // e.g. "upsun" + InstallerURL string `yaml:"installer_url,omitempty"` // e.g. "https://raw.githubusercontent.com/upsun/cli/main/installer.sh" + InstallMethod string `yaml:"install_method,omitempty"` // optional forced override +} +``` + +Populate `npm_package` and `installer_url` in `internal/config/upsun-cli.yaml` and +`internal/config/platformsh-cli.yaml`. A channel with an empty config field +simply falls back to the generic GitHub link for that branch (graceful for vendor +builds that don't ship that channel). + +## Code changes + +Phase 1: + +- `internal/update.go` + - New `InstallMethod` type + `AutoUpdating`. + - New `DetectInstallMethod(cnf)` (memoized) with the precedence above. + - `shouldCheckForUpdate`: short-circuit on `AutoUpdating()`. + - Cache the latest known version into state during the background check. +- `internal/state/state.go` + - Add `LastNotified` and `KnownLatestVersion`. +- `commands/root.go` + - Read the cached version in `PersistentPreRun`; show the tailored notice there + (throttled weekly, interactive-only) instead of in `PersistentPostRun`. + - `printUpdateMessage`: replace the Homebrew-only `if/else` with a `switch` on + `DetectInstallMethod(cnf)`. Move/keep `isUnderHomebrew` as a detector helper. + +Phase 2: + +- New `internal/update` updater: `Upgrade(ctx, method, cnf)` runs the channel's + command; `reexec(args)` does `syscall.Exec` (Unix) / spawn-and-forward (Windows). +- `commands/root.go` `PersistentPreRun`: when interactive + auto-updatable + + update known + weekly-allowed, prompt; on accept, `Upgrade` then `reexec`. + +## Packaging changes + +- `.goreleaser.yaml`: add the `install-source` marker to both `nfpms` entries' + `contents`. +- `packaging/install-source`: new one-line file containing `package`. +- `installer.sh`: no change required for suppression (its `apt`/`yum`/`apk` + methods install the marked package). Optional follow-up: have the `raw` method + export `UPSUN_CLI_INSTALL_METHOD=script` guidance in its success output, or + write a `script` marker, so `script` detection is explicit rather than inferred. + +## Edge cases + +- **Manual `dpkg -i` / `rpm -i` of a downloaded package** still carries the + marker, so it is treated as `package` and suppressed even though it will not + auto-update from a repo. Acceptable: the user chose a package format and can + re-run the same install, and the GitHub releases page is still discoverable. +- **`brew --prefix` unavailable / slow.** The `/Cellar/` pre-check avoids the + subprocess in the common non-Homebrew case; if `brew` is missing we fall + through to `script`/`unknown`, matching today's behavior. +- **npm global vs local, pnpm, nested installs.** Matching on `node_modules` in + the resolved executable path covers flat, nested, and pnpm layouts (the same + resolution `npm/wrapper/bin/upsun.js` relies on). +- **`$INSTALL_DIR` to a non-standard dir.** Reports `script` (no marker, not a + known package path) — correct. +- **Windows non-Scoop (raw zip on PATH).** No marker, not under scoop → `unknown` + → generic link. Acceptable until a Windows installer exists. + +## Known limitations + +- `state.json` has no file locking (`state.Save` is a bare write). Two CLI + invocations running concurrently can lose each other's writes: the background + `CheckForUpdate` of one process can overwrite the `LastNotified` just written + by another process's `MarkNotified`, so the weekly throttle may occasionally + let a notice through more than once. This races on a cosmetic field only, and + the file was already written without locking before this change. Fixing it + properly (advisory lock around load-modify-save) is out of scope for Phase 1. + +## Testing plan + +Table-driven tests in `internal/update_test.go` for `DetectInstallMethod`, +injecting the executable path, GOOS, env, and a marker-file probe via small +interfaces/fakes (avoid real `brew`/filesystem): + +- marker present → `package`; `AutoUpdating()` true. +- `…/node_modules/@upsun/cli-linux-x64/bin/upsun` → `npm`. +- `…\scoop\apps\upsun\current\upsun.exe` (GOOS=windows) → `scoop`. +- brew-prefix path → `homebrew`. +- `/usr/bin/upsun` no marker → `script`. +- `INSTALL_METHOD=homebrew` overrides a `/usr/bin` path. +- `go run` temp path → `unknown`. + +For `shouldCheckForUpdate`: assert it returns false when the method is a package, +true otherwise (given the other gates pass). For `printUpdateMessage`: assert the +upgrade line per method, including empty-config fallbacks. + +## Docs to update + +- `README.md` `#upgrade` section — note that package-manager installs update via + the OS and that the message is suppressed there. +- Upsun docs CLI admin page + (`sites/upsun/src/administration/cli`) — document `INSTALL_METHOD` and + reconfirm `UPDATES_CHECK=0`. +- `CLAUDE.md` "Update Checks" section — mention install-method detection. + +## Resolved decisions + +- `package` installs: total silence, no hint. The OS handles updates. +- Manual `dpkg -i`/`rpm -i`: treated as `package` (suppressed). Acceptable. +- npm upgrade command: global `npm install -g @latest`. +- Ambiguous / no marker: fall back to `script`/`unknown`, advise re-running the + installer (or the GitHub link). +- Prompt default: Enter = yes, but only for unprivileged channels + (Homebrew/Scoop/npm). Never default-run `sudo`/`curl | sh`. + +## Open questions + +1. Phase 3: do we move the bash installer's `raw` destination to a user-local + path? It simplifies detection and unlocks `script` auto-update, but changes + long-standing behavior and PATH expectations. +2. Windows re-exec after update: spawn-and-forward is more complex than Unix + `syscall.Exec`. Acceptable to ship Phase 2 for macOS/Linux first and print the + command on Windows until the spawn path is built? +3. Should there be a config/env switch to disable just the *prompt* (keep the + passive notice) for users who want to be told but never asked to act? Or is + `UPDATES_CHECK=0` (disables everything) enough? diff --git a/internal/config/platformsh-cli.yaml b/internal/config/platformsh-cli.yaml index e3860e3ab..f7b596427 100644 --- a/internal/config/platformsh-cli.yaml +++ b/internal/config/platformsh-cli.yaml @@ -6,6 +6,7 @@ wrapper: homebrew_tap: upsun/tap/platformsh-cli github_repo: upsun/cli + installer_url: https://raw.githubusercontent.com/upsun/cli/main/installer.sh application: name: "Upsun CLI (Platform.sh compatibility)" diff --git a/internal/config/schema.go b/internal/config/schema.go index e995c8026..476f5f6a3 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -20,8 +20,11 @@ import ( type Config struct { // Fields only used by to the Go wrapper. Wrapper struct { - HomebrewTap string `yaml:"homebrew_tap,omitempty"` // e.g. "upsun/tap/platformsh-cli" - GitHubRepo string `yaml:"github_repo,omitempty"` // e.g. "upsun/cli" + HomebrewTap string `yaml:"homebrew_tap,omitempty"` // e.g. "upsun/tap/platformsh-cli" + GitHubRepo string `yaml:"github_repo,omitempty"` // e.g. "upsun/cli" + NpmPackage string `yaml:"npm_package,omitempty"` // e.g. "upsun" + InstallerURL string `yaml:"installer_url,omitempty"` // e.g. "https://raw.githubusercontent.com/upsun/cli/main/installer.sh" + InstallMethod string `yaml:"install_method,omitempty"` // forces the detected install method (homebrew, scoop, npm, package, script) } `yaml:"wrapper,omitempty"` Application struct { diff --git a/internal/config/upsun-cli.yaml b/internal/config/upsun-cli.yaml index 39d96adf1..19051f8aa 100644 --- a/internal/config/upsun-cli.yaml +++ b/internal/config/upsun-cli.yaml @@ -4,6 +4,8 @@ wrapper: homebrew_tap: upsun/tap/upsun-cli github_repo: upsun/cli + npm_package: upsun + installer_url: https://raw.githubusercontent.com/upsun/cli/main/installer.sh application: name: "Upsun CLI" diff --git a/internal/install.go b/internal/install.go new file mode 100644 index 000000000..9c60dc5cc --- /dev/null +++ b/internal/install.go @@ -0,0 +1,211 @@ +package internal + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync" + + "github.com/upsun/cli/internal/config" +) + +// InstallMethod identifies how the CLI binary was installed, so update messages +// can be tailored or suppressed. +type InstallMethod string + +const ( + InstallUnknown InstallMethod = "" + InstallHomebrew InstallMethod = "homebrew" + InstallScoop InstallMethod = "scoop" + InstallNpm InstallMethod = "npm" + InstallPackage InstallMethod = "package" // a system package manager: apt, yum/dnf, apk + InstallScript InstallMethod = "script" // the bash installer's raw method +) + +// AutoUpdating reports whether the host system updates the CLI on its own, in +// which case the CLI should stay quiet about new releases. +func (m InstallMethod) AutoUpdating() bool { + return m == InstallPackage +} + +// installProbe holds the (injectable) inputs to install-method detection. +type installProbe struct { + goos string + exe string // the resolved (symlinks followed) executable path + envPrefix string + slug string + configMethod string + getenv func(string) string + fileExists func(string) bool + brewPrefix func() (string, bool) +} + +var ( + detectOnce sync.Once + detectedMethod InstallMethod +) + +// DetectInstallMethod resolves the install method once per process. It may shell +// out to "brew", so it is only called when about to print an update message, +// never on the hot path of every command. +func DetectInstallMethod(cnf *config.Config) InstallMethod { + detectOnce.Do(func() { + detectedMethod = detectInstallMethod(&installProbe{ + goos: runtime.GOOS, + exe: resolvedExecutable(), + envPrefix: cnf.Application.EnvPrefix, + slug: cnf.Application.Slug, + configMethod: cnf.Wrapper.InstallMethod, + getenv: getenvFunc, + fileExists: fileExists, + brewPrefix: brewPrefix, + }) + }) + return detectedMethod +} + +// IsAutoUpdating is a cheap check (no subprocess) suitable for the hot path. It +// only considers an explicit override and the package marker file. +func IsAutoUpdating(cnf *config.Config) bool { + if m, ok := overrideMethod(cnf.Application.EnvPrefix, cnf.Wrapper.InstallMethod, getenvFunc); ok { + return m.AutoUpdating() + } + return packageMarkerExists(resolvedExecutable(), cnf.Application.Slug, fileExists) +} + +func detectInstallMethod(p *installProbe) InstallMethod { + if m, ok := overrideMethod(p.envPrefix, p.configMethod, p.getenv); ok { + return m + } + if packageMarkerExists(p.exe, p.slug, p.fileExists) { + return InstallPackage + } + n := normPath(p.exe) + if strings.Contains(n, "/node_modules/") { + return InstallNpm + } + if p.goos == "windows" && strings.Contains(n, "/scoop/") { + return InstallScoop + } + if isHomebrew(p.exe, p.brewPrefix) { + return InstallHomebrew + } + if inStandardBinDir(p.exe) { + return InstallScript + } + return InstallUnknown +} + +// overrideMethod reads a forced install method from the environment or config. +func overrideMethod(envPrefix, configMethod string, getenv func(string) string) (InstallMethod, bool) { + if v := getenv(envPrefix + "INSTALL_METHOD"); v != "" { + return parseMethod(v) + } + if configMethod != "" { + return parseMethod(configMethod) + } + return InstallUnknown, false +} + +func parseMethod(v string) (InstallMethod, bool) { + switch strings.ToLower(strings.TrimSpace(v)) { + case string(InstallHomebrew), "brew": + return InstallHomebrew, true + case string(InstallScoop): + return InstallScoop, true + case string(InstallNpm): + return InstallNpm, true + case string(InstallScript): + return InstallScript, true + case string(InstallPackage), "apt", "yum", "dnf", "apk", "deb", "rpm": + return InstallPackage, true + } + return InstallUnknown, false +} + +// packageMarkerExists reports whether the system package installed a marker file. +// Packages install the binary at /bin/; the marker sits alongside at +// /share//install-source. +func packageMarkerExists(exe, slug string, fileExists func(string) bool) bool { + if exe == "" || slug == "" { + return false + } + prefix := filepath.Dir(filepath.Dir(exe)) + return fileExists(filepath.Join(prefix, "share", slug, "install-source")) +} + +func isHomebrew(exe string, brewPrefix func() (string, bool)) bool { + n := normPath(exe) + // Homebrew bins symlink into the Cellar; the resolved path reveals it cheaply. + if strings.Contains(n, "/cellar/") { + return true + } + if prefix, ok := brewPrefix(); ok && prefix != "" { + if strings.HasPrefix(n, normPath(prefix)+"/bin/") { + return true + } + } + return false +} + +func inStandardBinDir(exe string) bool { + if exe == "" { + return false + } + dir := strings.TrimSuffix(normPath(filepath.Dir(exe)), "/") + switch dir { + case "/usr/bin", "/usr/local/bin", "/bin", "/usr/sbin", "/sbin", "/opt/bin": + return true + } + return strings.HasSuffix(dir, "/.local/bin") +} + +// normPath lowercases and normalizes separators so matching works regardless of +// the host OS (useful for cross-platform tests and Windows paths). +func normPath(p string) string { + return strings.ToLower(strings.ReplaceAll(p, "\\", "/")) +} + +var ( + resolvedExeOnce sync.Once + resolvedExe string +) + +// resolvedExecutable returns the current executable with symlinks followed, so +// Homebrew/Scoop shims resolve to their real location. Memoized. +func resolvedExecutable() string { + resolvedExeOnce.Do(func() { + exe, err := os.Executable() + if err != nil { + return + } + if r, err := filepath.EvalSymlinks(exe); err == nil { + exe = r + } + resolvedExe = exe + }) + return resolvedExe +} + +func getenvFunc(k string) string { + return os.Getenv(k) +} + +func fileExists(p string) bool { + _, err := os.Stat(p) + return err == nil +} + +func brewPrefix() (string, bool) { + exe, err := exec.LookPath("brew") + if err != nil { + return "", false + } + out, err := exec.Command(exe, "--prefix").Output() + if err != nil { + return "", false + } + return strings.TrimSpace(string(out)), true +} diff --git a/internal/install_test.go b/internal/install_test.go new file mode 100644 index 000000000..5094c9ec7 --- /dev/null +++ b/internal/install_test.go @@ -0,0 +1,164 @@ +package internal + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDetectInstallMethod(t *testing.T) { + noBrew := func() (string, bool) { return "", false } + brewAt := func(prefix string) func() (string, bool) { + return func() (string, bool) { return prefix, true } + } + noFile := func(string) bool { return false } + fileAt := func(want string) func(string) bool { + return func(p string) bool { return p == want } + } + + cases := []struct { + name string + probe installProbe + want InstallMethod + }{ + { + name: "env override wins over path", + probe: installProbe{ + goos: "linux", exe: "/usr/bin/upsun", envPrefix: "UPSUN_CLI_", slug: "upsun", + getenv: func(k string) string { return map[string]string{"UPSUN_CLI_INSTALL_METHOD": "homebrew"}[k] }, + fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallHomebrew, + }, + { + name: "config override", + probe: installProbe{ + goos: "linux", exe: "/usr/bin/upsun", slug: "upsun", configMethod: "npm", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallNpm, + }, + { + name: "override alias apt maps to package", + probe: installProbe{ + goos: "linux", exe: "/somewhere/upsun", envPrefix: "UPSUN_CLI_", slug: "upsun", + getenv: func(k string) string { return map[string]string{"UPSUN_CLI_INSTALL_METHOD": "apt"}[k] }, + fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallPackage, + }, + { + name: "package marker present", + probe: installProbe{ + goos: "linux", exe: "/usr/bin/upsun", slug: "upsun", + getenv: func(string) string { return "" }, brewPrefix: noBrew, + fileExists: fileAt("/usr/share/upsun/install-source"), + }, + want: InstallPackage, + }, + { + name: "usr-bin without marker is script", + probe: installProbe{ + goos: "linux", exe: "/usr/bin/upsun", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallScript, + }, + { + name: "npm node_modules path", + probe: installProbe{ + goos: "linux", exe: "/home/u/project/node_modules/@upsun/cli-linux-x64/bin/upsun", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallNpm, + }, + { + name: "scoop path on windows", + probe: installProbe{ + goos: "windows", exe: `C:\Users\u\scoop\apps\upsun\current\upsun.exe`, slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallScoop, + }, + { + name: "scoop path on linux is not detected as scoop", + probe: installProbe{ + goos: "linux", exe: "/home/u/scoop/apps/upsun/upsun", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallUnknown, + }, + { + name: "user-local bin is script", + probe: installProbe{ + goos: "linux", exe: "/home/u/.local/bin/upsun", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallScript, + }, + { + name: "homebrew via cellar symlink target", + probe: installProbe{ + goos: "darwin", exe: "/opt/homebrew/Cellar/upsun-cli/2.0.0/bin/upsun", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallHomebrew, + }, + { + name: "homebrew via brew prefix", + probe: installProbe{ + goos: "darwin", exe: "/usr/local/bin/upsun", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: brewAt("/usr/local"), + }, + want: InstallHomebrew, + }, + { + name: "unknown for go-build temp path", + probe: installProbe{ + goos: "linux", exe: "/tmp/go-build123/b001/exe/platform", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallUnknown, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, detectInstallMethod(&c.probe)) + }) + } +} + +func TestInstallMethodAutoUpdating(t *testing.T) { + assert.True(t, InstallPackage.AutoUpdating()) + assert.False(t, InstallHomebrew.AutoUpdating()) + assert.False(t, InstallScript.AutoUpdating()) + assert.False(t, InstallUnknown.AutoUpdating()) +} + +func TestParseMethod(t *testing.T) { + cases := []struct { + in string + want InstallMethod + ok bool + }{ + {"homebrew", InstallHomebrew, true}, + {"brew", InstallHomebrew, true}, + {"APT", InstallPackage, true}, + {"dnf", InstallPackage, true}, + {"apk", InstallPackage, true}, + {"npm", InstallNpm, true}, + {" scoop ", InstallScoop, true}, + {"nonsense", InstallUnknown, false}, + {"", InstallUnknown, false}, + } + for _, c := range cases { + t.Run(c.in, func(t *testing.T) { + got, ok := parseMethod(c.in) + assert.Equal(t, c.ok, ok) + if c.ok { + assert.Equal(t, c.want, got) + } + }) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index 1a0e65a19..cef446d57 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -10,7 +10,9 @@ import ( type State struct { Updates struct { - LastChecked int64 `json:"last_checked"` + LastChecked int64 `json:"last_checked"` + LastNotified int64 `json:"last_notified,omitempty"` + KnownLatestVersion string `json:"known_latest_version,omitempty"` } `json:"updates,omitempty"` ConfigUpdates struct { diff --git a/internal/update.go b/internal/update.go index e7dee1680..86196d52b 100644 --- a/internal/update.go +++ b/internal/update.go @@ -46,6 +46,10 @@ func CheckForUpdate(cnf *config.Config, currentVersion string) (*ReleaseInfo, er return nil, fmt.Errorf("could not determine latest release: %w", err) } + // Cache the latest known version so the next invocation can show a message + // before its command runs, without blocking on the network. + s.Updates.KnownLatestVersion = releaseInfo.Version + cmp, err := version.Compare(releaseInfo.Version, currentVersion) if err != nil { return nil, fmt.Errorf("could not compare versions: %w", err) @@ -57,13 +61,64 @@ func CheckForUpdate(cnf *config.Config, currentVersion string) (*ReleaseInfo, er return nil, nil } +// notifyInterval is the minimum time between showing update messages. +const notifyInterval = 7 * 24 * 60 * 60 // one week, in seconds + +// PendingNotification returns a release to tell the user about, based on the +// version cached by a previous background check. It returns nil when there is +// nothing to show, when the weekly throttle has not elapsed, or when the +// environment is not eligible. The caller should print the result and then call +// MarkNotified. +func PendingNotification(cnf *config.Config, currentVersion string) *ReleaseInfo { + if !shouldCheckForUpdate(cnf) { + return nil + } + s, err := state.Load(cnf) + if err != nil { + return nil + } + return notificationFromState(s, cnf.Wrapper.GitHubRepo, currentVersion, time.Now().Unix()) +} + +// notificationFromState decides whether to notify about the cached latest +// version, given the current time. It is the pure core of PendingNotification. +func notificationFromState(s state.State, repo, currentVersion string, now int64) *ReleaseInfo { + if s.Updates.KnownLatestVersion == "" { + return nil + } + if s.Updates.LastNotified != 0 && now-s.Updates.LastNotified < notifyInterval { + return nil + } + cmp, err := version.Compare(s.Updates.KnownLatestVersion, currentVersion) + if err != nil || cmp <= 0 { + return nil + } + return &ReleaseInfo{ + Version: s.Updates.KnownLatestVersion, + URL: fmt.Sprintf("https://github.com/%s/releases/tag/%s", repo, s.Updates.KnownLatestVersion), + } +} + +// MarkNotified records that an update message was just shown, so it is not +// repeated until the next notify interval. +func MarkNotified(cnf *config.Config) { + s, err := state.Load(cnf) + if err != nil { + return + } + s.Updates.LastNotified = time.Now().Unix() + //nolint:errcheck // failing to save state should not affect the rest of the program + state.Save(s, cnf) +} + // shouldCheckForUpdate checks updates are not disabled and the environment is a terminal func shouldCheckForUpdate(cnf *config.Config) bool { return config.Version != "0.0.0" && cnf.Wrapper.GitHubRepo != "" && cnf.Updates.Check && os.Getenv(cnf.Application.EnvPrefix+"UPDATES_CHECK") != "0" && - !isCI() && terminal.IsTerminal(os.Stdout) && terminal.IsTerminal(os.Stderr) + !isCI() && !IsAutoUpdating(cnf) && + terminal.IsTerminal(os.Stdout) && terminal.IsTerminal(os.Stderr) } func isCI() bool { diff --git a/internal/update_test.go b/internal/update_test.go new file mode 100644 index 000000000..e2639acac --- /dev/null +++ b/internal/update_test.go @@ -0,0 +1,47 @@ +package internal + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/upsun/cli/internal/state" +) + +func TestNotificationFromState(t *testing.T) { + const now = 1_700_000_000 + + newState := func(latest string, lastNotified int64) state.State { + var s state.State + s.Updates.KnownLatestVersion = latest + s.Updates.LastNotified = lastNotified + return s + } + + t.Run("newer version not yet notified", func(t *testing.T) { + rel := notificationFromState(newState("v2.0.0", 0), "upsun/cli", "v1.0.0", now) + if assert.NotNil(t, rel) { + assert.Equal(t, "v2.0.0", rel.Version) + assert.Equal(t, "https://github.com/upsun/cli/releases/tag/v2.0.0", rel.URL) + } + }) + + t.Run("no cached version", func(t *testing.T) { + assert.Nil(t, notificationFromState(newState("", 0), "upsun/cli", "v1.0.0", now)) + }) + + t.Run("cached version not newer", func(t *testing.T) { + assert.Nil(t, notificationFromState(newState("v1.0.0", 0), "upsun/cli", "v1.0.0", now)) + assert.Nil(t, notificationFromState(newState("v0.9.0", 0), "upsun/cli", "v1.0.0", now)) + }) + + t.Run("notified within the past week is throttled", func(t *testing.T) { + lastNotified := int64(now - 3*24*60*60) // 3 days ago + assert.Nil(t, notificationFromState(newState("v2.0.0", lastNotified), "upsun/cli", "v1.0.0", now)) + }) + + t.Run("notified over a week ago shows again", func(t *testing.T) { + lastNotified := int64(now - 8*24*60*60) // 8 days ago + assert.NotNil(t, notificationFromState(newState("v2.0.0", lastNotified), "upsun/cli", "v1.0.0", now)) + }) +} diff --git a/packaging/install-source b/packaging/install-source new file mode 100644 index 000000000..ba3bd7873 --- /dev/null +++ b/packaging/install-source @@ -0,0 +1 @@ +package From 2ea9f82a81100d4d3bdd89b1d7ebf7b0583e91ab Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 24 Sep 2026 22:18:29 +0100 Subject: [PATCH 2/7] fix(update): address review of install-method update notices - Upgrade command for script installs pins INSTALL_DIR to the current binary's directory. This replaces the binary in place and forces the installer's raw method, which it would not otherwise pick on hosts with apt/yum/apk, and pipes to sh (the installer is POSIX sh). - Drop installer_url from the platform flavor: installer.sh only installs the upsun binary. - Remove the leading blank line from the notice, now that it is printed before the command's output. - Clarify in the design doc which flavors set npm_package/installer_url. Co-Authored-By: Claude Opus 5.5 --- commands/root.go | 13 ++++++++----- commands/root_test.go | 10 +++++----- docs/design/update-message-install-detection.md | 7 ++++--- internal/config/platformsh-cli.yaml | 1 - 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/commands/root.go b/commands/root.go index cd1ba1c0c..ea39e0325 100644 --- a/commands/root.go +++ b/commands/root.go @@ -211,7 +211,7 @@ func printUpdateMessage(w io.Writer, newRelease *internal.ReleaseInfo, cnf *conf return } - fmt.Fprintf(w, "\n%s %s → %s\n", + fmt.Fprintf(w, "%s %s → %s\n", color.YellowString(fmt.Sprintf("A new release of the %s is available:", cnf.Application.Name)), color.CyanString(config.Version), color.CyanString(newRelease.Version), @@ -234,10 +234,11 @@ func printUpdateMessage(w io.Writer, newRelease *internal.ReleaseInfo, cnf *conf // an empty string when there is no tailored command (the caller then falls back // to a generic link). func upgradeCommand(cnf *config.Config) string { - return upgradeCommandFor(cnf, internal.DetectInstallMethod(cnf)) + exe, _ := os.Executable() + return upgradeCommandFor(cnf, internal.DetectInstallMethod(cnf), exe) } -func upgradeCommandFor(cnf *config.Config, method internal.InstallMethod) string { +func upgradeCommandFor(cnf *config.Config, method internal.InstallMethod, exe string) string { switch method { case internal.InstallHomebrew: if cnf.Wrapper.HomebrewTap != "" { @@ -250,8 +251,10 @@ func upgradeCommandFor(cnf *config.Config, method internal.InstallMethod) string return "npm install -g " + cnf.Wrapper.NpmPackage + "@latest" } case internal.InstallScript: - if cnf.Wrapper.InstallerURL != "" { - return "curl -fsSL " + cnf.Wrapper.InstallerURL + " | bash" + // INSTALL_DIR replaces the binary in place and forces the installer's raw + // method, which it might not otherwise choose (e.g. apt on Debian). + if cnf.Wrapper.InstallerURL != "" && exe != "" { + return "curl -fsSL " + cnf.Wrapper.InstallerURL + " | INSTALL_DIR=" + filepath.Dir(exe) + " sh" } } return "" diff --git a/commands/root_test.go b/commands/root_test.go index 29ba46e19..93f09a0c6 100644 --- a/commands/root_test.go +++ b/commands/root_test.go @@ -22,13 +22,13 @@ func TestUpgradeCommandFor(t *testing.T) { {internal.InstallHomebrew, "brew update && brew upgrade upsun/tap/upsun-cli"}, {internal.InstallScoop, "scoop update upsun"}, {internal.InstallNpm, "npm install -g upsun@latest"}, - {internal.InstallScript, "curl -fsSL https://example.com/installer.sh | bash"}, + {internal.InstallScript, "curl -fsSL https://example.com/installer.sh | INSTALL_DIR=/usr/local/bin sh"}, {internal.InstallPackage, ""}, // suppressed; no tailored command {internal.InstallUnknown, ""}, // falls back to the generic link } for _, c := range cases { t.Run(string(c.method), func(t *testing.T) { - assert.Equal(t, c.want, upgradeCommandFor(cnf, c.method)) + assert.Equal(t, c.want, upgradeCommandFor(cnf, c.method, "/usr/local/bin/upsun")) }) } } @@ -36,7 +36,7 @@ func TestUpgradeCommandFor(t *testing.T) { func TestUpgradeCommandForMissingConfigFallsBack(t *testing.T) { cnf := testConfig() // no Wrapper.* fields set // Homebrew/npm/script require their config field; without it, fall back (empty). - assert.Empty(t, upgradeCommandFor(cnf, internal.InstallHomebrew)) - assert.Empty(t, upgradeCommandFor(cnf, internal.InstallNpm)) - assert.Empty(t, upgradeCommandFor(cnf, internal.InstallScript)) + assert.Empty(t, upgradeCommandFor(cnf, internal.InstallHomebrew, "/usr/local/bin/upsun")) + assert.Empty(t, upgradeCommandFor(cnf, internal.InstallNpm, "/usr/local/bin/upsun")) + assert.Empty(t, upgradeCommandFor(cnf, internal.InstallScript, "/usr/local/bin/upsun")) } diff --git a/docs/design/update-message-install-detection.md b/docs/design/update-message-install-detection.md index 721ad1e7b..b4a5ebd9a 100644 --- a/docs/design/update-message-install-detection.md +++ b/docs/design/update-message-install-detection.md @@ -204,7 +204,7 @@ hint. | `homebrew` | Yes | `brew update && brew upgrade ` | Yes | | `scoop` | Yes | `scoop update ` | Yes | | `npm` | Yes | `npm install -g @latest` | Yes | -| `script` | Yes | `curl -fsSL \| sh` | No (remote + maybe sudo)¹ | +| `script` | Yes | `curl -fsSL \| INSTALL_DIR= sh` | No (remote + maybe sudo)¹ | | `unknown` | Yes | `follow the instructions at https://github.com/#upgrade` (today's text) | No | ¹ Becomes auto-updatable in Phase 3 if raw installs move to a user-local, @@ -306,8 +306,9 @@ Wrapper struct { } ``` -Populate `npm_package` and `installer_url` in `internal/config/upsun-cli.yaml` and -`internal/config/platformsh-cli.yaml`. A channel with an empty config field +Populate `npm_package` and `installer_url` in `internal/config/upsun-cli.yaml` +only: npm and `installer.sh` ship only the `upsun` binary, so +`internal/config/platformsh-cli.yaml` leaves them empty. A channel with an empty config field simply falls back to the generic GitHub link for that branch (graceful for vendor builds that don't ship that channel). diff --git a/internal/config/platformsh-cli.yaml b/internal/config/platformsh-cli.yaml index f7b596427..e3860e3ab 100644 --- a/internal/config/platformsh-cli.yaml +++ b/internal/config/platformsh-cli.yaml @@ -6,7 +6,6 @@ wrapper: homebrew_tap: upsun/tap/platformsh-cli github_repo: upsun/cli - installer_url: https://raw.githubusercontent.com/upsun/cli/main/installer.sh application: name: "Upsun CLI (Platform.sh compatibility)" From 61e1b6631a7105fb651335dd8bb40340cc77511c Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 24 Sep 2026 22:20:14 +0100 Subject: [PATCH 3/7] fix(update): keep notices for quiet runs and quote the installer dir A --quiet run discards stderr, so showing the notice there would consume the weekly throttle without the user seeing it. Leave it for a later run. Shell-quote INSTALL_DIR in the suggested installer command, so paths with spaces or metacharacters are copied safely. Co-Authored-By: Claude Opus 5.5 --- commands/root.go | 20 ++++++++++++++++---- commands/root_test.go | 12 ++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/commands/root.go b/commands/root.go index ea39e0325..c8f1c7e7e 100644 --- a/commands/root.go +++ b/commands/root.go @@ -54,7 +54,8 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob // Completions must be fast and quiet. return } - if viper.GetBool("quiet") && !viper.GetBool("debug") && !viper.GetBool("verbose") { + quiet := viper.GetBool("quiet") && !viper.GetBool("debug") && !viper.GetBool("verbose") + if quiet { viper.Set("no-interaction", true) cmd.SetErr(io.Discard) } else { @@ -76,8 +77,9 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob if cnf.Wrapper.GitHubRepo != "" { // Show any update found by a previous run, before the command's // output. The check itself runs in the background (below) and - // caches its result for the next invocation. - if rel := internal.PendingNotification(cnf, config.Version); rel != nil { + // caches its result for the next invocation. In quiet mode the + // notice would be discarded, so it is left for a later run. + if rel := internal.PendingNotification(cnf, config.Version); rel != nil && !quiet { printUpdateMessage(cmd.ErrOrStderr(), rel, cnf) internal.MarkNotified(cnf) } @@ -254,12 +256,22 @@ func upgradeCommandFor(cnf *config.Config, method internal.InstallMethod, exe st // INSTALL_DIR replaces the binary in place and forces the installer's raw // method, which it might not otherwise choose (e.g. apt on Debian). if cnf.Wrapper.InstallerURL != "" && exe != "" { - return "curl -fsSL " + cnf.Wrapper.InstallerURL + " | INSTALL_DIR=" + filepath.Dir(exe) + " sh" + return "curl -fsSL " + cnf.Wrapper.InstallerURL + " | INSTALL_DIR=" + shellQuote(filepath.Dir(exe)) + " sh" } } return "" } +var shellSafe = regexp.MustCompile(`^[A-Za-z0-9_./-]+$`) + +// shellQuote quotes s for a POSIX shell, if needed. +func shellQuote(s string) string { + if shellSafe.MatchString(s) { + return s + } + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + func debugLogf(format string, v ...any) { if !viper.GetBool("debug") { return diff --git a/commands/root_test.go b/commands/root_test.go index 93f09a0c6..772158a0e 100644 --- a/commands/root_test.go +++ b/commands/root_test.go @@ -40,3 +40,15 @@ func TestUpgradeCommandForMissingConfigFallsBack(t *testing.T) { assert.Empty(t, upgradeCommandFor(cnf, internal.InstallNpm, "/usr/local/bin/upsun")) assert.Empty(t, upgradeCommandFor(cnf, internal.InstallScript, "/usr/local/bin/upsun")) } + +func TestShellQuote(t *testing.T) { + cases := []struct{ in, want string }{ + {"/usr/local/bin", "/usr/local/bin"}, + {"/home/Jane Doe/.local/bin", "'/home/Jane Doe/.local/bin'"}, + {"/tmp/it's", `'/tmp/it'\''s'`}, + {"/tmp/$(id)", "'/tmp/$(id)'"}, + } + for _, c := range cases { + assert.Equal(t, c.want, shellQuote(c.in)) + } +} From e1abdbfec007d17baad306295c5281968a6cbf6b Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 24 Sep 2026 22:29:20 +0100 Subject: [PATCH 4/7] fix(update): force raw installer method and harden detection - Add INSTALL_METHOD=raw to the script upgrade command. installer.sh only reads INSTALL_DIR after choosing the raw method, so without it the command would install via apt/yum/apk or Homebrew instead. - Before printing a notice, detect package installs that predate the marker file by asking dpkg/rpm/apk whether they own the binary, and stay silent for them. - Only treat a Cellar path as Homebrew when it contains the tap's formula (/Cellar//). - Treat a LastNotified in the future (clock skew) as stale. - Add state.Update, which reloads and saves state under a mutex, so the update check and config update goroutines no longer drop each other's fields. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- commands/root.go | 12 +++++--- commands/root_test.go | 4 +-- .../update-message-install-detection.md | 14 +++++---- internal/config/alt/update.go | 5 ++-- internal/install.go | 30 +++++++++++++++++-- internal/install_test.go | 23 +++++++++++++- internal/state/state.go | 16 ++++++++++ internal/update.go | 23 +++++++------- internal/update_test.go | 5 ++++ 10 files changed, 106 insertions(+), 28 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index dea872ac3..8ba3afb20 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -166,7 +166,7 @@ PHP version is still injected via ldflags: The CLI checks for updates from GitHub releases (when Wrapper.GitHubRepo is set in config). The network check runs in a background goroutine and caches the latest known version in `state.json`; the notice is shown before the command on a later run (see `internal/update.go`). Install-method detection (`internal/install.go`) tailors or suppresses the notice: -- System package managers (apt, yum/dnf, apk) are detected via a marker file installed by the nfpm packages (`packaging/install-source` → `/usr/share//install-source`). The notice is suppressed because the OS handles updates. +- System package managers (apt, yum/dnf, apk) are detected via a marker file installed by the nfpm packages (`packaging/install-source` → `/usr/share//install-source`). Before printing a notice, older package installs without the marker are also detected by querying dpkg/rpm/apk. The notice is suppressed because the OS handles updates. - Homebrew, Scoop, npm, and the bash installer get a tailored upgrade command, built from config fields (`Wrapper.HomebrewTap`, `Wrapper.NpmPackage`, `Wrapper.InstallerURL`, `Application.Executable`). - The notice is throttled to once a week (`LastNotified` in state) and only shown in an interactive terminal. - `INSTALL_METHOD` forces the method; `UPDATES_CHECK=0` disables checks entirely. diff --git a/commands/root.go b/commands/root.go index c8f1c7e7e..2c7b060b4 100644 --- a/commands/root.go +++ b/commands/root.go @@ -80,7 +80,10 @@ func newRootCommand(cnf *config.Config, assets *vendorization.VendorAssets) *cob // caches its result for the next invocation. In quiet mode the // notice would be discarded, so it is left for a later run. if rel := internal.PendingNotification(cnf, config.Version); rel != nil && !quiet { - printUpdateMessage(cmd.ErrOrStderr(), rel, cnf) + // Full detection can find package installs that lack the marker. + if !internal.DetectInstallMethod(cnf).AutoUpdating() { + printUpdateMessage(cmd.ErrOrStderr(), rel, cnf) + } internal.MarkNotified(cnf) } go func() { @@ -253,10 +256,11 @@ func upgradeCommandFor(cnf *config.Config, method internal.InstallMethod, exe st return "npm install -g " + cnf.Wrapper.NpmPackage + "@latest" } case internal.InstallScript: - // INSTALL_DIR replaces the binary in place and forces the installer's raw - // method, which it might not otherwise choose (e.g. apt on Debian). + // The raw method replaces the binary in INSTALL_DIR; without it, the + // installer may choose a package manager instead (e.g. apt on Debian). if cnf.Wrapper.InstallerURL != "" && exe != "" { - return "curl -fsSL " + cnf.Wrapper.InstallerURL + " | INSTALL_DIR=" + shellQuote(filepath.Dir(exe)) + " sh" + return "curl -fsSL " + cnf.Wrapper.InstallerURL + + " | INSTALL_METHOD=raw INSTALL_DIR=" + shellQuote(filepath.Dir(exe)) + " sh" } } return "" diff --git a/commands/root_test.go b/commands/root_test.go index 772158a0e..fddd8066a 100644 --- a/commands/root_test.go +++ b/commands/root_test.go @@ -12,7 +12,7 @@ func TestUpgradeCommandFor(t *testing.T) { cnf := testConfig() cnf.Wrapper.HomebrewTap = "upsun/tap/upsun-cli" cnf.Wrapper.NpmPackage = "upsun" - cnf.Wrapper.InstallerURL = "https://example.com/installer.sh" + cnf.Wrapper.InstallerURL = "https://x.test/i.sh" cnf.Application.Executable = "upsun" cases := []struct { @@ -22,7 +22,7 @@ func TestUpgradeCommandFor(t *testing.T) { {internal.InstallHomebrew, "brew update && brew upgrade upsun/tap/upsun-cli"}, {internal.InstallScoop, "scoop update upsun"}, {internal.InstallNpm, "npm install -g upsun@latest"}, - {internal.InstallScript, "curl -fsSL https://example.com/installer.sh | INSTALL_DIR=/usr/local/bin sh"}, + {internal.InstallScript, "curl -fsSL https://x.test/i.sh | INSTALL_METHOD=raw INSTALL_DIR=/usr/local/bin sh"}, {internal.InstallPackage, ""}, // suppressed; no tailored command {internal.InstallUnknown, ""}, // falls back to the generic link } diff --git a/docs/design/update-message-install-detection.md b/docs/design/update-message-install-detection.md index b4a5ebd9a..d5166f922 100644 --- a/docs/design/update-message-install-detection.md +++ b/docs/design/update-message-install-detection.md @@ -146,10 +146,13 @@ A new function `DetectInstallMethod(cnf) InstallMethod` resolves the method once scope (`@/cli-` derived from config, or simply `node_modules`). → `npm`. 4. **Scoop** (Windows only). Path contains a `scoop{sep}` segment. → `scoop`. 5. **Homebrew.** Existing `isUnderHomebrew` logic (path under `brew --prefix`), - plus a cheap pre-check for `/Cellar/` in the path to avoid the subprocess when - obviously not Homebrew. → `homebrew`. + plus a cheap pre-check for `/Cellar//` in the path to avoid the + subprocess when obviously Homebrew. → `homebrew`. 6. **Script.** Otherwise, if the binary is in a normal bin dir - (`/usr/bin`, `/usr/local/bin`, `$INSTALL_DIR`-style) → `script`. + (`/usr/bin`, `/usr/local/bin`, `$INSTALL_DIR`-style) → `script`, unless on + Linux `dpkg-query -S`, `rpm -qf` or `apk info --who-owns` says a package owns + it → `package`. This covers packages installed before the marker existed. + It is too slow for the hot path, so it only runs before printing a notice. 7. **Unknown.** Anything else (e.g. `go run`, source builds). → `unknown`. Resolve symlinks with `filepath.EvalSymlinks(os.Executable())` before matching so @@ -204,7 +207,7 @@ hint. | `homebrew` | Yes | `brew update && brew upgrade ` | Yes | | `scoop` | Yes | `scoop update ` | Yes | | `npm` | Yes | `npm install -g @latest` | Yes | -| `script` | Yes | `curl -fsSL \| INSTALL_DIR= sh` | No (remote + maybe sudo)¹ | +| `script` | Yes | `curl -fsSL \| INSTALL_METHOD=raw INSTALL_DIR= sh` | No (remote + maybe sudo)¹ | | `unknown` | Yes | `follow the instructions at https://github.com/#upgrade` (today's text) | No | ¹ Becomes auto-updatable in Phase 3 if raw installs move to a user-local, @@ -383,7 +386,8 @@ interfaces/fakes (avoid real `brew`/filesystem): - `…/node_modules/@upsun/cli-linux-x64/bin/upsun` → `npm`. - `…\scoop\apps\upsun\current\upsun.exe` (GOOS=windows) → `scoop`. - brew-prefix path → `homebrew`. -- `/usr/bin/upsun` no marker → `script`. +- `/usr/bin/upsun` no marker, not package-owned → `script`. +- `/usr/bin/upsun` no marker, package-owned → `package`. - `INSTALL_METHOD=homebrew` overrides a `/usr/bin` path. - `go run` temp path → `unknown`. diff --git a/internal/config/alt/update.go b/internal/config/alt/update.go index 6f38fd3f9..317aded28 100644 --- a/internal/config/alt/update.go +++ b/internal/config/alt/update.go @@ -52,8 +52,9 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i } defer func() { - s.ConfigUpdates.LastChecked = time.Now().Unix() - if err := state.Save(s, cnf); err != nil { + if err := state.Update(cnf, func(s *state.State) { + s.ConfigUpdates.LastChecked = time.Now().Unix() + }); err != nil { debugLog("Error saving state: %s", err) } }() diff --git a/internal/install.go b/internal/install.go index 9c60dc5cc..0eae6b35c 100644 --- a/internal/install.go +++ b/internal/install.go @@ -3,6 +3,7 @@ package internal import ( "os" "os/exec" + "path" "path/filepath" "runtime" "strings" @@ -37,9 +38,11 @@ type installProbe struct { envPrefix string slug string configMethod string + brewFormula string // the last segment of the Homebrew tap, e.g. "upsun-cli" getenv func(string) string fileExists func(string) bool brewPrefix func() (string, bool) + pkgOwned func(exe string) bool } var ( @@ -58,9 +61,11 @@ func DetectInstallMethod(cnf *config.Config) InstallMethod { envPrefix: cnf.Application.EnvPrefix, slug: cnf.Application.Slug, configMethod: cnf.Wrapper.InstallMethod, + brewFormula: path.Base(cnf.Wrapper.HomebrewTap), getenv: getenvFunc, fileExists: fileExists, brewPrefix: brewPrefix, + pkgOwned: ownedBySystemPackage, }) }) return detectedMethod @@ -89,10 +94,14 @@ func detectInstallMethod(p *installProbe) InstallMethod { if p.goos == "windows" && strings.Contains(n, "/scoop/") { return InstallScoop } - if isHomebrew(p.exe, p.brewPrefix) { + if isHomebrew(p.exe, p.brewFormula, p.brewPrefix) { return InstallHomebrew } if inStandardBinDir(p.exe) { + // Packages installed before the marker file existed lack it. + if p.goos == "linux" && p.pkgOwned(p.exe) { + return InstallPackage + } return InstallScript } return InstallUnknown @@ -136,10 +145,10 @@ func packageMarkerExists(exe, slug string, fileExists func(string) bool) bool { return fileExists(filepath.Join(prefix, "share", slug, "install-source")) } -func isHomebrew(exe string, brewPrefix func() (string, bool)) bool { +func isHomebrew(exe, formula string, brewPrefix func() (string, bool)) bool { n := normPath(exe) // Homebrew bins symlink into the Cellar; the resolved path reveals it cheaply. - if strings.Contains(n, "/cellar/") { + if formula != "" && formula != "." && strings.Contains(n, "/cellar/"+strings.ToLower(formula)+"/") { return true } if prefix, ok := brewPrefix(); ok && prefix != "" { @@ -198,6 +207,21 @@ func fileExists(p string) bool { return err == nil } +// ownedBySystemPackage asks the system package database whether it owns exe. +func ownedBySystemPackage(exe string) bool { + queries := [][]string{ + {"dpkg-query", "-S", exe}, + {"rpm", "-qf", exe}, + {"apk", "info", "--who-owns", exe}, + } + for _, q := range queries { + if p, err := exec.LookPath(q[0]); err == nil && exec.Command(p, q[1:]...).Run() == nil { + return true + } + } + return false +} + func brewPrefix() (string, bool) { exe, err := exec.LookPath("brew") if err != nil { diff --git a/internal/install_test.go b/internal/install_test.go index 5094c9ec7..f7cfab067 100644 --- a/internal/install_test.go +++ b/internal/install_test.go @@ -100,10 +100,28 @@ func TestDetectInstallMethod(t *testing.T) { name: "homebrew via cellar symlink target", probe: installProbe{ goos: "darwin", exe: "/opt/homebrew/Cellar/upsun-cli/2.0.0/bin/upsun", slug: "upsun", - getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + brewFormula: "upsun-cli", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, }, want: InstallHomebrew, }, + { + name: "cellar directory not belonging to homebrew", + probe: installProbe{ + goos: "linux", exe: "/srv/cellar/bin/upsun", slug: "upsun", brewFormula: "upsun-cli", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + }, + want: InstallUnknown, + }, + { + name: "usr-bin owned by a package without marker", + probe: installProbe{ + goos: "linux", exe: "/usr/bin/upsun", slug: "upsun", + getenv: func(string) string { return "" }, fileExists: noFile, brewPrefix: noBrew, + pkgOwned: func(exe string) bool { return exe == "/usr/bin/upsun" }, + }, + want: InstallPackage, + }, { name: "homebrew via brew prefix", probe: installProbe{ @@ -124,6 +142,9 @@ func TestDetectInstallMethod(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { + if c.probe.pkgOwned == nil { + c.probe.pkgOwned = func(string) bool { return false } + } assert.Equal(t, c.want, detectInstallMethod(&c.probe)) }) } diff --git a/internal/state/state.go b/internal/state/state.go index cef446d57..7f5304e21 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "sync" "github.com/upsun/cli/internal/config" ) @@ -52,6 +53,21 @@ func Save(state State, cnf *config.Config) error { return os.WriteFile(statePath, data, 0o600) } +var mu sync.Mutex + +// Update loads the state, applies fn and saves it. Reloading under a lock stops +// concurrent updates in one process from dropping each other's fields. +func Update(cnf *config.Config, fn func(*State)) error { + mu.Lock() + defer mu.Unlock() + s, err := Load(cnf) + if err != nil { + return err + } + fn(&s) + return Save(s, cnf) +} + // getPath determines the path to the state JSON file depending on config. func getPath(cnf *config.Config) (string, error) { writableDir, err := cnf.WritableUserDir() //nolint:staticcheck // backwards compatibility is needed for state files diff --git a/internal/update.go b/internal/update.go index 86196d52b..387532475 100644 --- a/internal/update.go +++ b/internal/update.go @@ -34,11 +34,16 @@ func CheckForUpdate(cnf *config.Config, currentVersion string) (*ReleaseInfo, er return nil, nil } + var latest string defer func() { // After checking, save the last check time. - s.Updates.LastChecked = time.Now().Unix() //nolint:errcheck // not being able to set the state should have no impact on the rest of the program - state.Save(s, cnf) + state.Update(cnf, func(s *state.State) { + s.Updates.LastChecked = time.Now().Unix() + if latest != "" { + s.Updates.KnownLatestVersion = latest + } + }) }() releaseInfo, err := getLatestReleaseInfo(cnf.Wrapper.GitHubRepo) @@ -48,7 +53,7 @@ func CheckForUpdate(cnf *config.Config, currentVersion string) (*ReleaseInfo, er // Cache the latest known version so the next invocation can show a message // before its command runs, without blocking on the network. - s.Updates.KnownLatestVersion = releaseInfo.Version + latest = releaseInfo.Version cmp, err := version.Compare(releaseInfo.Version, currentVersion) if err != nil { @@ -86,7 +91,8 @@ func notificationFromState(s state.State, repo, currentVersion string, now int64 if s.Updates.KnownLatestVersion == "" { return nil } - if s.Updates.LastNotified != 0 && now-s.Updates.LastNotified < notifyInterval { + // A LastNotified in the future (clock skew) is treated as stale. + if s.Updates.LastNotified != 0 && s.Updates.LastNotified <= now && now-s.Updates.LastNotified < notifyInterval { return nil } cmp, err := version.Compare(s.Updates.KnownLatestVersion, currentVersion) @@ -102,13 +108,10 @@ func notificationFromState(s state.State, repo, currentVersion string, now int64 // MarkNotified records that an update message was just shown, so it is not // repeated until the next notify interval. func MarkNotified(cnf *config.Config) { - s, err := state.Load(cnf) - if err != nil { - return - } - s.Updates.LastNotified = time.Now().Unix() //nolint:errcheck // failing to save state should not affect the rest of the program - state.Save(s, cnf) + state.Update(cnf, func(s *state.State) { + s.Updates.LastNotified = time.Now().Unix() + }) } // shouldCheckForUpdate checks updates are not disabled and the environment is a terminal diff --git a/internal/update_test.go b/internal/update_test.go index e2639acac..0ad5e3f6b 100644 --- a/internal/update_test.go +++ b/internal/update_test.go @@ -44,4 +44,9 @@ func TestNotificationFromState(t *testing.T) { lastNotified := int64(now - 8*24*60*60) // 8 days ago assert.NotNil(t, notificationFromState(newState("v2.0.0", lastNotified), "upsun/cli", "v1.0.0", now)) }) + + t.Run("notifies when last notified is in the future", func(t *testing.T) { + lastNotified := int64(now + 30*24*60*60) // clock skew + assert.NotNil(t, notificationFromState(newState("v2.0.0", lastNotified), "upsun/cli", "v1.0.0", now)) + }) } From e8bf4a21490e77eafaf712c47adcc4d60e5e716f Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 24 Sep 2026 22:37:36 +0100 Subject: [PATCH 5/7] fix(update): repair corrupt state and bound package queries - state.Update starts from empty state when state.json cannot be read, so a corrupt file is rewritten instead of blocking every later save (which would stop update throttling and notices). - Give the dpkg/rpm/apk ownership query a one-second time limit, since it runs before the user's command. Co-Authored-By: Claude Opus 5.5 --- internal/install.go | 7 ++++++- internal/state/state.go | 5 +++-- internal/state/state_test.go | 31 +++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 internal/state/state_test.go diff --git a/internal/install.go b/internal/install.go index 0eae6b35c..df7d41e37 100644 --- a/internal/install.go +++ b/internal/install.go @@ -1,6 +1,7 @@ package internal import ( + "context" "os" "os/exec" "path" @@ -8,6 +9,7 @@ import ( "runtime" "strings" "sync" + "time" "github.com/upsun/cli/internal/config" ) @@ -208,14 +210,17 @@ func fileExists(p string) bool { } // ownedBySystemPackage asks the system package database whether it owns exe. +// A time limit stops a slow database from delaying the command. func ownedBySystemPackage(exe string) bool { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() queries := [][]string{ {"dpkg-query", "-S", exe}, {"rpm", "-qf", exe}, {"apk", "info", "--who-owns", exe}, } for _, q := range queries { - if p, err := exec.LookPath(q[0]); err == nil && exec.Command(p, q[1:]...).Run() == nil { + if p, err := exec.LookPath(q[0]); err == nil && exec.CommandContext(ctx, p, q[1:]...).Run() == nil { return true } } diff --git a/internal/state/state.go b/internal/state/state.go index 7f5304e21..dfa4d6b11 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -56,13 +56,14 @@ func Save(state State, cnf *config.Config) error { var mu sync.Mutex // Update loads the state, applies fn and saves it. Reloading under a lock stops -// concurrent updates in one process from dropping each other's fields. +// concurrent updates in one process from dropping each other's fields. An +// unreadable state file is replaced, so a corrupt file does not persist. func Update(cnf *config.Config, fn func(*State)) error { mu.Lock() defer mu.Unlock() s, err := Load(cnf) if err != nil { - return err + s = State{} } fn(&s) return Save(s, cnf) diff --git a/internal/state/state_test.go b/internal/state/state_test.go new file mode 100644 index 000000000..17a4014cb --- /dev/null +++ b/internal/state/state_test.go @@ -0,0 +1,31 @@ +package state + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/upsun/cli/internal/config" +) + +func TestUpdateRepairsCorruptFile(t *testing.T) { + home := t.TempDir() + cnf := &config.Config{} + cnf.Application.EnvPrefix = "TEST_STATE_" + cnf.Application.WritableUserDir = ".test" + cnf.Application.UserStateFile = "state.json" + t.Setenv("TEST_STATE_HOME", home) + + statePath := filepath.Join(home, ".test", "state.json") + require.NoError(t, os.MkdirAll(filepath.Dir(statePath), 0o700)) + require.NoError(t, os.WriteFile(statePath, []byte(`{"updates":{}}garbage`), 0o600)) + + require.NoError(t, Update(cnf, func(s *State) { s.Updates.LastChecked = 42 })) + + s, err := Load(cnf) + require.NoError(t, err) + assert.EqualValues(t, 42, s.Updates.LastChecked) +} From 9097dd35f9e31691a017157e2fd4a3101885df94 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 24 Sep 2026 22:42:32 +0100 Subject: [PATCH 6/7] fix(update): write state atomically and bound each package query - state.Save writes to a temporary file and renames it, so a concurrent process never reads a partial file and resets the state. - Give each dpkg/rpm/apk query its own time limit, and treat a timeout as package-owned so a slow database never leads to suggesting the installer over a package-owned binary. Co-Authored-By: Claude Opus 5.5 --- internal/install.go | 15 +++++++++++---- internal/state/state.go | 16 +++++++++++++++- internal/state/state_test.go | 8 ++++++++ 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/internal/install.go b/internal/install.go index df7d41e37..c810847f1 100644 --- a/internal/install.go +++ b/internal/install.go @@ -210,17 +210,24 @@ func fileExists(p string) bool { } // ownedBySystemPackage asks the system package database whether it owns exe. -// A time limit stops a slow database from delaying the command. +// A time limit stops a slow database from delaying the command; a timeout +// counts as owned, to err on the side of silence. func ownedBySystemPackage(exe string) bool { - ctx, cancel := context.WithTimeout(context.Background(), time.Second) - defer cancel() queries := [][]string{ {"dpkg-query", "-S", exe}, {"rpm", "-qf", exe}, {"apk", "info", "--who-owns", exe}, } for _, q := range queries { - if p, err := exec.LookPath(q[0]); err == nil && exec.CommandContext(ctx, p, q[1:]...).Run() == nil { + p, err := exec.LookPath(q[0]) + if err != nil { + continue + } + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + err = exec.CommandContext(ctx, p, q[1:]...).Run() + timedOut := ctx.Err() != nil + cancel() + if err == nil || timedOut { return true } } diff --git a/internal/state/state.go b/internal/state/state.go index dfa4d6b11..ec068aa8d 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -50,7 +50,21 @@ func Save(state State, cnf *config.Config) error { return err } - return os.WriteFile(statePath, data, 0o600) + // Write to a temporary file and rename it, so that other processes never + // read a partially written file. + f, err := os.CreateTemp(filepath.Dir(statePath), ".state-*.tmp") + if err != nil { + return err + } + defer os.Remove(f.Name()) + if _, err := f.Write(data); err != nil { + f.Close() + return err + } + if err := f.Close(); err != nil { + return err + } + return os.Rename(f.Name(), statePath) } var mu sync.Mutex diff --git a/internal/state/state_test.go b/internal/state/state_test.go index 17a4014cb..5c3da57a4 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -28,4 +28,12 @@ func TestUpdateRepairsCorruptFile(t *testing.T) { s, err := Load(cnf) require.NoError(t, err) assert.EqualValues(t, 42, s.Updates.LastChecked) + + // The save leaves no temporary files and keeps the file private. + entries, err := os.ReadDir(filepath.Dir(statePath)) + require.NoError(t, err) + require.Len(t, entries, 1) + info, err := os.Stat(statePath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) } From f8c20abe0cc92dbf197b4ab8bbbedf8c40b09b4f Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 24 Sep 2026 22:47:34 +0100 Subject: [PATCH 7/7] fix(update): fall back to a direct state write and share the query deadline - If renaming the temporary state file fails (on Windows, while another process has state.json open), write the file directly instead of dropping the update. - Share one one-second deadline across the dpkg/rpm/apk queries, so detection delays a command by at most a second. Co-Authored-By: Claude Opus 5.5 --- internal/install.go | 8 +++----- internal/state/state.go | 14 +++++++++++--- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/install.go b/internal/install.go index c810847f1..c6f6b3e3b 100644 --- a/internal/install.go +++ b/internal/install.go @@ -213,6 +213,8 @@ func fileExists(p string) bool { // A time limit stops a slow database from delaying the command; a timeout // counts as owned, to err on the side of silence. func ownedBySystemPackage(exe string) bool { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() queries := [][]string{ {"dpkg-query", "-S", exe}, {"rpm", "-qf", exe}, @@ -223,11 +225,7 @@ func ownedBySystemPackage(exe string) bool { if err != nil { continue } - ctx, cancel := context.WithTimeout(context.Background(), time.Second) - err = exec.CommandContext(ctx, p, q[1:]...).Run() - timedOut := ctx.Err() != nil - cancel() - if err == nil || timedOut { + if err := exec.CommandContext(ctx, p, q[1:]...).Run(); err == nil || ctx.Err() != nil { return true } } diff --git a/internal/state/state.go b/internal/state/state.go index ec068aa8d..92e1b4b0b 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -51,8 +51,16 @@ func Save(state State, cnf *config.Config) error { } // Write to a temporary file and rename it, so that other processes never - // read a partially written file. - f, err := os.CreateTemp(filepath.Dir(statePath), ".state-*.tmp") + // read a partially written file. The rename can fail on Windows while + // another process has the file open, so fall back to a direct write. + if err := writeAndRename(statePath, data); err != nil { + return os.WriteFile(statePath, data, 0o600) + } + return nil +} + +func writeAndRename(path string, data []byte) error { + f, err := os.CreateTemp(filepath.Dir(path), ".state-*.tmp") if err != nil { return err } @@ -64,7 +72,7 @@ func Save(state State, cnf *config.Config) error { if err := f.Close(); err != nil { return err } - return os.Rename(f.Name(), statePath) + return os.Rename(f.Name(), path) } var mu sync.Mutex