From 5fe2a9a584a56312f60809866ffcdf3f78def188 Mon Sep 17 00:00:00 2001 From: Joel Scheuner Date: Mon, 7 Sep 2026 13:52:18 +0200 Subject: [PATCH 1/6] Allow permanently disabling lstk update checks Co-Authored-By: Claude --- CLAUDE.md | 7 + cmd/root.go | 57 ++- cmd/update.go | 17 +- cmd/update_check_test.go | 67 ++++ docs/structured-output.md | 7 +- internal/config/config.go | 24 +- internal/config/default_config.toml | 4 + internal/config/update_check.go | 49 +++ internal/config/update_check_test.go | 140 +++++++ internal/env/env.go | 9 + internal/env/env_test.go | 25 ++ internal/output/error_code.go | 142 +++---- internal/output/error_code_test.go | 4 +- internal/ui/run_update.go | 4 +- internal/update/external_install.go | 124 ++++++ internal/update/external_install_test.go | 277 ++++++++++++++ internal/update/install_method.go | 85 ++++- internal/update/install_method_test.go | 2 +- internal/update/notify.go | 118 ++++-- internal/update/notify_guard_test.go | 240 ++++++++++++ internal/update/notify_mode_test.go | 262 +++++++++++++ internal/update/notify_test.go | 56 +-- internal/update/update.go | 81 +++- .../changes/add-update-check-config/design.md | 126 +++++++ .../add-update-check-config/proposal.md | 36 ++ .../specs/external-install-detection/spec.md | 84 +++++ .../specs/update-check-config/spec.md | 77 ++++ .../changes/add-update-check-config/tasks.md | 89 +++++ .../specs/error-codes/spec.md | 1 + test/integration/update_check_test.go | 357 ++++++++++++++++++ test/integration/update_test.go | 9 +- 31 files changed, 2399 insertions(+), 181 deletions(-) create mode 100644 cmd/update_check_test.go create mode 100644 internal/config/update_check.go create mode 100644 internal/config/update_check_test.go create mode 100644 internal/env/env_test.go create mode 100644 internal/update/external_install.go create mode 100644 internal/update/external_install_test.go create mode 100644 internal/update/notify_guard_test.go create mode 100644 internal/update/notify_mode_test.go create mode 100644 openspec/changes/add-update-check-config/design.md create mode 100644 openspec/changes/add-update-check-config/proposal.md create mode 100644 openspec/changes/add-update-check-config/specs/external-install-detection/spec.md create mode 100644 openspec/changes/add-update-check-config/specs/update-check-config/spec.md create mode 100644 openspec/changes/add-update-check-config/tasks.md create mode 100644 test/integration/update_check_test.go diff --git a/CLAUDE.md b/CLAUDE.md index b5e23402..1c98d28f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,6 +156,7 @@ Environment variables: - `LOCALSTACK_AUTH_TOKEN` - Auth token (skips browser login if set). It takes precedence over credentials stored in the keyring, so a per-invocation token overrides a previous `lstk login` without a `lstk logout` first; resolution order is env var → keyring → browser login (`auth.GetToken`, mirrored in `cmd/root.go`'s telemetry token resolution). - `LSTK_STARTUP_TIMEOUT` - Startup readiness deadline for `lstk start` (Go duration). Zero/unset uses the per-mode default resolved in `resolveStartupTimeout` (`internal/container/start.go`): 20s interactive (deadline only shows a recoverable keep-waiting/stop prompt, re-armed by "keep waiting"), 60s non-interactive (fatal; the container is left running for inspection). Container exits are detected separately — and instantly, with the exit code — via the exit wait `runtime.Runtime.Start` registers between create and start. `lstk start --timeout ` (also on the bare root) overrides this for a single run; the flag wins over the env var when explicitly set, and `--timeout 0` falls back to the per-mode default (`addTimeoutFlag`/`applyTimeoutFlag` in `cmd/root.go`). `restart` and the snapshot auto-start path do not expose the flag. - `LSTK_OTEL=1` - Enables OpenTelemetry trace export (disabled by default); when enabled, standard `OTEL_EXPORTER_OTLP_*` env vars are respected by the SDK. Requires an OTLP-compatible backend to receive and visualize telemetry — for local development, `make otel` starts one (UI at http://localhost:16686). +- `LSTK_UPDATE_CHECK` - Overrides `[cli] update_check` for one run (`prompt`/`notify`/`off`); see Update Checks below. - `LSTK_MERGE_STRATEGY` - Default merge strategy for `snapshot load` / `load` (`account-region-merge`, `overwrite`, or `service-merge`) when `--merge` is not passed; an explicit `--merge` always wins. Resolved in `resolveMergeStrategy` (`cmd/snapshot.go`). # Infrastructure as Code Commands @@ -221,6 +222,12 @@ The release job (`.github/workflows/ci.yml`) builds the npm packages with `gorel - Do not call `config.Get()` from domain/business-logic packages. Instead, extract the values you need at the command boundary (`cmd/`) and pass them as explicit function arguments. This keeps domain functions testable without requiring Viper/config initialization. - Validate user/agent-supplied values where they are first accepted (the command boundary, or the domain parser that owns the format) via `internal/validate` — never an inline one-off regexp. Route by value class: pod names → `validate.PodName`; opaque secrets → only loose malformed-ness checks (`validate.AuthToken` style — no charset restriction); paths and URLs → their existing parsers (`filepath`, `net/url`). For other identifiers, follow the owning API's documented contract and add a dedicated validator if needed. If no validator fits, add one to `internal/validate` with rule-code tests instead of forking rules locally — parallel validators for the same value class drift (the pod-name rules forked exactly this way before #293 re-unified them). +# Update Checks + +`lstk start` (and the bare root) checks for a newer lstk release and, interactively, prompts to install it. Two things gate that: `[cli] update_check` in config.toml (`prompt` default / `notify` / `off`) with `LSTK_UPDATE_CHECK` overriding it for one run, and — when neither is set — whether the install looks externally managed. `lstk update` itself is never gated by the setting; it is a direct request, not a background nag (DEVX-1029). + +Resolution happens at the command boundary (`resolveUpdateCheckMode` in `cmd/root.go`), never inside `internal/update`. Externally-managed installs (recognized from path-segment markers in `classifyPath`) default to `notify` and are never updated in place — every path that replaces the binary goes through `applyUpdate`, which is the single choke point for that guard. Mechanism and rationale for each piece live on the declarations: `config.UpdateCheckMode`, `env.Env.UpdateCheck`, `NotifyOptions.CanPrompt`, `externalMarkers`, `classifyPath`, `blockSelfUpdate`, and the `--force` flag. + # Shell Completion Cobra's generated bash completion script requires `_get_comp_words_by_ref` from the bash-completion package on both of its init paths, and stock macOS (bash 3.2) ships without that package — so completion failed with "command not found" on every Tab (DEVX-950). `selfContainBashCompletion` in `cmd/completion.go` wraps the autogenerated `completion bash` command to prepend a guarded pure-bash fallback (defined only when the package is absent, the git-completion.bash approach) and replaces the help text. The fallback body must stay bash 3.2 compatible (no `declare -A`, namerefs, `mapfile`, case-conversion expansions). It covers only `_get_comp_words_by_ref`; Cobra's script still calls bash-completion's `_filedir` for `ShellCompDirectiveFilterFileExt`/`ShellCompDirectiveFilterDirs` (`MarkFlagFilename`/`MarkFlagDirname`) and the ActiveHelp second-Tab path — lstk uses none of these today, so adopting one means growing the fallback. In docs/help, never recommend `source <(lstk completion bash)` — it is a silent no-op on bash 3.2; recommend `eval "$(lstk completion bash)"` instead. Zsh/fish/powershell scripts are self-contained upstream and untouched. diff --git a/cmd/root.go b/cmd/root.go index fa4064c7..809996ca 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -324,6 +324,22 @@ func configureCommandExecution(root *cobra.Command, cfg *env.Env, tel *telemetry wrapPreRunEForJSON(root, cfg, stdout) } +// resolveUpdateCheckMode applies the update-check resolution order: +// LSTK_UPDATE_CHECK wins over the [cli] update_check config key. An unset value +// stays UpdateCheckUnset rather than defaulting to prompt here, so the domain +// layer can still fall through to install detection — which is what keeps that +// detection off the path when the user did express a preference. +func resolveUpdateCheckMode(cfg *env.Env, appConfig *config.Config) (config.UpdateCheckMode, error) { + if cfg.UpdateCheck != "" { + mode, err := config.ParseUpdateCheckMode(cfg.UpdateCheck) + if err != nil { + return "", fmt.Errorf("invalid %s: %w", env.UpdateCheckVar, err) + } + return mode, nil + } + return config.ParseUpdateCheckMode(appConfig.CLI.UpdateCheck) +} + func buildStartOptions(cfg *env.Env, appConfig *config.Config, logger log.Logger, tel *telemetry.Client, persist bool) container.StartOptions { return container.StartOptions{ PlatformClient: api.NewPlatformClient(cfg.APIEndpoint, logger), @@ -343,7 +359,23 @@ func buildStartOptions(cfg *env.Env, appConfig *config.Config, logger log.Logger func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *telemetry.Client, logger log.Logger, sink output.Sink, persist bool, firstRun bool, snapshotFlag string, noSnapshot bool, emulatorType config.EmulatorType) error { appConfig, err := config.Get() if err != nil { - return fmt.Errorf("failed to get config: %w", err) + return failGetConfig(sink, cfg, err) + } + + // Resolved before anything is written or started, so a bad + // LSTK_UPDATE_CHECK fails as early as a bad config key already does + // (config.Get validates the [cli] section above). + updateCheckMode, err := resolveUpdateCheckMode(cfg, appConfig) + if err != nil { + sink.Emit(output.ErrorEvent{ + Title: err.Error(), + Actions: []output.ErrorAction{{ + Label: "Accepted values are prompt, notify and off. Unset it with:", + Value: "unset " + env.UpdateCheckVar, + }}, + Code: output.ErrConfigInvalid, + }) + return output.NewSilentError(err) } configPath, err := config.FriendlyConfigPath() @@ -384,10 +416,17 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t opts := buildStartOptions(cfg, appConfig, logger, tel, persist) notifyOpts := update.NotifyOptions{ - GitHubToken: cfg.GitHubToken, - UpdatePrompt: true, - SkippedVersion: appConfig.CLI.UpdateSkippedVersion, - PersistSkipVersion: config.SetUpdateSkippedVersion, + GitHubToken: cfg.GitHubToken, + CanPrompt: true, + Mode: updateCheckMode, + DetectInstall: update.DetectInstallMethod, + } + // Only offer to persist a preference when there is a file to persist it to. + // On a genuine first run config.toml does not exist yet — it is created + // later, by the emulator picker — so the update prompt must not offer an + // option whose effect would be silently dropped. + if config.HasFile() { + notifyOpts.PersistUpdateCheck = config.SetUpdateCheck } if isInteractiveMode(cfg) { @@ -411,7 +450,13 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t Text: fmt.Sprintf("Configured with default emulator %s.", emName), }) } - update.NotifyUpdate(ctx, sink, update.NotifyOptions{GitHubToken: cfg.GitHubToken}) + // Same options as the interactive path, minus the ability to prompt: the + // mode and the detection hook still apply, so `update_check = "off"` + // silences a non-interactive start too, and a note there still names an + // external manager rather than advising a command that would refuse. + nonInteractiveNotify := notifyOpts + nonInteractiveNotify.CanPrompt = false + update.NotifyUpdate(ctx, sink, nonInteractiveNotify) result, err := container.Start(ctx, rt, sink, opts, false) if err != nil { return err diff --git a/cmd/update.go b/cmd/update.go index 2ecfa878..155e6dde 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -11,24 +11,31 @@ import ( func newUpdateCmd(cfg *env.Env) *cobra.Command { var checkOnly bool + var force bool cmd := &cobra.Command{ - Use: "update", - Short: "Update lstk to the latest version", - Long: "Check for and apply updates to the lstk CLI. Respects the original installation method (Homebrew, npm, or direct binary).", + Use: "update", + Short: "Update lstk to the latest version", + Long: "Check for and apply updates to the lstk CLI. Respects the original installation method (Homebrew, npm, or direct binary).\n\n" + + "An install managed by an external tool (mise, nix, guix, asdf, scoop, chocolatey), or one in a directory lstk cannot write to, is not updated in place — update it through that tool instead, or pass --force to replace the binary anyway. --check always reports whether a newer version exists, whatever the install.\n\n" + + "This command always checks for updates. The [cli] update_check config key and LSTK_UPDATE_CHECK only govern the automatic check on 'lstk start'.", PreRunE: initConfigDeferCreate(nil), Annotations: map[string]string{jsonSupportedAnnotation: "true"}, RunE: func(cmd *cobra.Command, args []string) error { sink := jsonAwareSink(cmd, cfg, os.Stdout) if isInteractiveMode(cfg) { - return ui.RunUpdate(cmd.Context(), checkOnly, cfg.GitHubToken) + return ui.RunUpdate(cmd.Context(), checkOnly, cfg.GitHubToken, force) } - return update.Update(cmd.Context(), sink, checkOnly, cfg.GitHubToken) + return update.Update(cmd.Context(), sink, checkOnly, cfg.GitHubToken, force) }, } cmd.Flags().BoolVar(&checkOnly, "check", false, "Only check for updates without applying them") + // Exists because externally-managed-install detection is a path-marker + // heuristic: it cannot recognize every packaging layout, so a user whose + // install it misreads must still have a way through. + cmd.Flags().BoolVar(&force, "force", false, "Replace the binary even when the install is externally managed or its directory looks unwritable") return cmd } diff --git a/cmd/update_check_test.go b/cmd/update_check_test.go new file mode 100644 index 00000000..8a9a23df --- /dev/null +++ b/cmd/update_check_test.go @@ -0,0 +1,67 @@ +package cmd + +import ( + "testing" + + "github.com/localstack/lstk/internal/config" + "github.com/localstack/lstk/internal/env" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveUpdateCheckMode(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + envValue string + confValue string + want config.UpdateCheckMode + }{ + {"neither set stays unset so detection can decide", "", "", config.UpdateCheckUnset}, + {"config only", "", "notify", config.UpdateCheckNotify}, + {"env only", "off", "", config.UpdateCheckOff}, + {"env wins over config", "prompt", "off", config.UpdateCheckPrompt}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := resolveUpdateCheckMode( + &env.Env{UpdateCheck: tt.envValue}, + &config.Config{CLI: config.CLIConfig{UpdateCheck: tt.confValue}}, + ) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// "quiet" is not a mode — it stands in for a plausible-sounding typo, to prove +// a bad value is reported rather than silently coerced into some default. +func TestResolveUpdateCheckModeRejectsInvalidEnvValue(t *testing.T) { + t.Parallel() + + _, err := resolveUpdateCheckMode( + &env.Env{UpdateCheck: "quiet"}, + &config.Config{}, + ) + + require.Error(t, err) + assert.Contains(t, err.Error(), env.UpdateCheckVar) + assert.Contains(t, err.Error(), "quiet") +} + +// An invalid env value must be rejected even when the config file holds a +// perfectly good one: silently falling back would hide the user's typo and +// apply a mode they did not ask for. +func TestResolveUpdateCheckModeRejectsInvalidEnvValueOverValidConfig(t *testing.T) { + t.Parallel() + + _, err := resolveUpdateCheckMode( + &env.Env{UpdateCheck: "quiet"}, + &config.Config{CLI: config.CLIConfig{UpdateCheck: "notify"}}, + ) + + require.Error(t, err) +} diff --git a/docs/structured-output.md b/docs/structured-output.md index 45609971..4b8ae659 100644 --- a/docs/structured-output.md +++ b/docs/structured-output.md @@ -121,6 +121,7 @@ Every `error.code` is one of the following fixed constants. A failure that doesn | `USAGE_ERROR` | Cobra-level flag or argument parsing failed | No | `USAGE` | | `NOT_JSON_CAPABLE` | The requested command has not been annotated as JSON-capable yet | No | `USAGE` | | `NETWORK_ERROR` | An unclassified network/transport failure occurred | Yes | `RUNTIME` | +| `UPDATE_EXTERNALLY_MANAGED` | `lstk update` refused to replace a binary it does not own: the install is managed by an external tool (mise, nix, guix, asdf, scoop, chocolatey) or its directory is not writable. `--force` overrides | No | `RUNTIME` | | `CANCELLED` | The operation was interrupted (e.g. context cancellation via Ctrl+C) | Yes | `INTERNAL` | | `INTERNAL_ERROR` | Unclassified or unexpected failure; the universal fallback | No | `INTERNAL` | | `IAC_FILE_NOT_FOUND` | A required infrastructure-as-code file or directory does not exist or cannot be read (e.g. the workspace `lstk deploy detect --dir` was pointed at) | No | `IAC` | @@ -130,11 +131,11 @@ Every `error.code` is one of the following fixed constants. A failure that doesn ### Error categories -`error.category` groups the 29 codes above into 8 buckets, additive alongside `code` — it exists purely so a caller that only wants coarse handling doesn't have to build and maintain its own mapping from all 29 codes. `code` is unaffected and remains the primary, stable identifier for anything more specific. +`error.category` groups the 35 codes above into 8 buckets, additive alongside `code` — it exists purely so a caller that only wants coarse handling doesn't have to build and maintain its own mapping from all 35 codes. `code` is unaffected and remains the primary, stable identifier for anything more specific. ``` RUNTIME RUNTIME_UNAVAILABLE, IMAGE_PULL_FAILED, DEPENDENCY_MISSING, - DNS_RESOLUTION_REQUIRED, NETWORK_ERROR + DNS_RESOLUTION_REQUIRED, NETWORK_ERROR, UPDATE_EXTERNALLY_MANAGED → something outside lstk's control (Docker, network, a missing binary) EMULATOR EMULATOR_NOT_RUNNING, EMULATOR_ALREADY_RUNNING, EMULATOR_WRONG_TYPE, @@ -269,7 +270,7 @@ Codes: `EMULATOR_NOT_CONFIGURED` (no AWS container configured), `EMULATOR_NOT_RU "error": null } ``` -Codes: `NETWORK_ERROR` (GitHub API unreachable), `INTERNAL_ERROR` (archive download verification, extraction, or replacement failure), `CONFIG_INVALID`, `CONFIG_NOT_FOUND` (bad or missing `--config` path). +Codes: `NETWORK_ERROR` (GitHub API unreachable), `UPDATE_EXTERNALLY_MANAGED` (the install is managed by an external tool or sits in a directory lstk cannot write to; emitted before any version check, and suppressed by `--check` and `--force`), `INTERNAL_ERROR` (archive download verification, extraction, or replacement failure), `CONFIG_INVALID`, `CONFIG_NOT_FOUND` (bad or missing `--config` path). **`lstk start`** — a flat object, not an `emulators: [...]` list: only one `[[containers]]` block can be enabled at a time (`container.Start`'s `checkSingleContainer` guard), so `start` only ever acts on one emulator, unlike `stop`/`status` which genuinely enumerate multiple *configured* emulators. A version/config mismatch on an already-running instance surfaces as a `warnings[]` entry rather than a new field. The bare `lstk --json` invocation (no `start`) carries identical support: it runs through the same `startEmulator` path, so `--json` behaves the same way there, and the envelope's `command` field reads `"start"` either way. ```json diff --git a/internal/config/config.go b/internal/config/config.go index e95040b2..96c14f48 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -18,7 +18,7 @@ import ( var defaultConfigTemplate string type CLIConfig struct { - UpdateSkippedVersion string `mapstructure:"update_skipped_version"` + UpdateCheck string `mapstructure:"update_check"` } type Config struct { @@ -171,8 +171,23 @@ func setInFile(path, key string, value any) error { return os.WriteFile(path, []byte(content), 0644) } -func SetUpdateSkippedVersion(version string) error { - return Set("cli.update_skipped_version", version) +// SetUpdateCheck persists the update-check mode. Unlike Set, it fails rather +// than succeeding in memory only when there is no config file to write to: +// this backs the "Never ask again" prompt option, and reporting success for a +// write that was silently dropped would tell the user their choice was saved +// when the next run would prompt them again. +func SetUpdateCheck(mode UpdateCheckMode) error { + if resolvedConfigPath() == "" { + return errors.New("no config file to write to yet") + } + return Set("cli.update_check", string(mode)) +} + +// HasFile reports whether a config file has been resolved, i.e. whether +// settings can be persisted. The command boundary uses it to decide whether to +// offer options that write config. +func HasFile() bool { + return resolvedConfigPath() != "" } func Get() (*Config, error) { @@ -188,6 +203,9 @@ func Get() (*Config, error) { if err := validateNamedEnvs(cfg.Env); err != nil { return nil, err } + if _, err := ParseUpdateCheckMode(cfg.CLI.UpdateCheck); err != nil { + return nil, fmt.Errorf("invalid [cli] config: %w", err) + } return &cfg, nil } diff --git a/internal/config/default_config.toml b/internal/config/default_config.toml index 4c07d723..48cd6c2b 100644 --- a/internal/config/default_config.toml +++ b/internal/config/default_config.toml @@ -61,3 +61,7 @@ port = "4566" # Host port the emulator will be accessible on # [env.ci] # SERVICES = "s3,sqs" # EAGER_SERVICE_LOADING = "1" + +# CLI behavior +[cli] +# update_check = "notify" # Update check on start: "prompt" (default), "notify", "off" diff --git a/internal/config/update_check.go b/internal/config/update_check.go new file mode 100644 index 00000000..1841b022 --- /dev/null +++ b/internal/config/update_check.go @@ -0,0 +1,49 @@ +package config + +import ( + "fmt" + "strings" +) + +// UpdateCheckMode is the value of the `[cli] update_check` config key (and of +// the LSTK_UPDATE_CHECK environment variable), governing the automatic update +// check on the start path only — an explicit `lstk update` always runs. +// +// UpdateCheckUnset is the zero value and means "no preference expressed", which +// is what lets a caller distinguish an unset key from an explicit "prompt" and +// fall through to the next source in the resolution order. +type UpdateCheckMode string + +const ( + UpdateCheckUnset UpdateCheckMode = "" + // UpdateCheckPrompt checks and, on an interactive start, blocks on a choice. + UpdateCheckPrompt UpdateCheckMode = "prompt" + // UpdateCheckNotify checks and emits a single non-blocking note. + UpdateCheckNotify UpdateCheckMode = "notify" + // UpdateCheckOff performs no check at all: no request, no output. + UpdateCheckOff UpdateCheckMode = "off" +) + +// updateCheckModes is the accepted set, in the order used to build error text. +var updateCheckModes = []UpdateCheckMode{UpdateCheckPrompt, UpdateCheckNotify, UpdateCheckOff} + +// ParseUpdateCheckMode validates a raw update_check value. An empty string is +// valid and yields UpdateCheckUnset; anything else must match a mode exactly. +// Matching is deliberately strict — no trimming or case folding — so a typo +// surfaces as an error the user can see rather than being silently coerced into +// a mode they did not ask for. +func ParseUpdateCheckMode(s string) (UpdateCheckMode, error) { + if s == "" { + return UpdateCheckUnset, nil + } + for _, m := range updateCheckModes { + if string(m) == s { + return m, nil + } + } + valid := make([]string, len(updateCheckModes)) + for i, m := range updateCheckModes { + valid[i] = string(m) + } + return "", fmt.Errorf("invalid update_check value %q (must be one of: %s)", s, strings.Join(valid, ", ")) +} diff --git a/internal/config/update_check_test.go b/internal/config/update_check_test.go new file mode 100644 index 00000000..accd9105 --- /dev/null +++ b/internal/config/update_check_test.go @@ -0,0 +1,140 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseUpdateCheckMode(t *testing.T) { + t.Parallel() + tests := []struct { + name string + in string + want UpdateCheckMode + wantErr bool + }{ + {"prompt", "prompt", UpdateCheckPrompt, false}, + {"notify", "notify", UpdateCheckNotify, false}, + {"off", "off", UpdateCheckOff, false}, + {"empty means unset", "", UpdateCheckUnset, false}, + // A plausible-sounding non-mode: it must be reported, not coerced. + {"unknown value", "quiet", "", true}, + {"case sensitive", "Off", "", true}, + {"whitespace is not trimmed away silently", " off", "", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := ParseUpdateCheckMode(tt.in) + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), "update_check") + assert.Contains(t, err.Error(), "prompt") + assert.Contains(t, err.Error(), "notify") + assert.Contains(t, err.Error(), "off") + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// writeTestConfig writes a config.toml with the given [cli] body and loads it. +// Not parallel-safe: viper state is global. +func loadConfigWithCLISection(t *testing.T, cliBody string) { + t.Helper() + path := filepath.Join(t.TempDir(), "config.toml") + content := "[[containers]]\ntype = \"aws\"\ntag = \"latest\"\nport = \"4566\"\n" + if cliBody != "" { + content += "\n[cli]\n" + cliBody + "\n" + } + require.NoError(t, os.WriteFile(path, []byte(content), 0644)) + require.NoError(t, InitFromPath(path)) +} + +func TestGetRejectsInvalidUpdateCheck(t *testing.T) { + loadConfigWithCLISection(t, `update_check = "quiet"`) + + _, err := Get() + require.Error(t, err) + assert.Contains(t, err.Error(), "update_check") + assert.Contains(t, err.Error(), "quiet") +} + +func TestGetAcceptsValidUpdateCheck(t *testing.T) { + loadConfigWithCLISection(t, `update_check = "notify"`) + + cfg, err := Get() + require.NoError(t, err) + assert.Equal(t, "notify", cfg.CLI.UpdateCheck) +} + +func TestGetTreatsMissingUpdateCheckAsUnset(t *testing.T) { + loadConfigWithCLISection(t, "") + + cfg, err := Get() + require.NoError(t, err) + assert.Empty(t, cfg.CLI.UpdateCheck) +} + +func TestSetUpdateCheckPersistsToFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.toml") + content := "# my config\n[[containers]]\ntype = \"aws\"\n\n[cli]\nupdate_skipped_version = \"v1.2.3\"\n" + require.NoError(t, os.WriteFile(path, []byte(content), 0644)) + require.NoError(t, InitFromPath(path)) + + require.NoError(t, SetUpdateCheck(UpdateCheckNotify)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + // setInFile encodes strings as TOML literal strings (single quotes); see + // TestSetInFileAppendsWhenKeyAbsent. + assert.Contains(t, string(data), `update_check = 'notify'`) + assert.Contains(t, string(data), "# my config", "existing comments must survive") + assert.Contains(t, string(data), `update_skipped_version = "v1.2.3"`, "sibling keys must survive") +} + +// SetUpdateCheck must fail rather than succeed in memory only when there is no +// config file: it backs the "Never ask again" prompt option, and a silent no-op +// would tell the user their choice was saved when the next run would ask again. +func TestSetUpdateCheckFailsWithoutAConfigFile(t *testing.T) { + viper.Reset() + + assert.False(t, HasFile()) + require.Error(t, SetUpdateCheck(UpdateCheckNotify)) +} + +func TestHasFileReportsAResolvedConfig(t *testing.T) { + loadConfigWithCLISection(t, "") + + assert.True(t, HasFile()) +} + +// The shipped template documents update_check as a commented line inside +// [cli], while setInFile inserts a written key directly below the header — so +// the live value lands above the comment describing it. That is accepted +// (the alternative was a comment block detached from its own table), but the +// result must still be valid TOML that reads back correctly. +func TestSetUpdateCheckOnTheShippedTemplate(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.toml") + require.NoError(t, os.WriteFile(path, []byte(defaultConfigTemplate), 0644)) + require.NoError(t, InitFromPath(path)) + + require.NoError(t, SetUpdateCheck(UpdateCheckNotify)) + + require.NoError(t, InitFromPath(path)) + cfg, err := Get() + require.NoError(t, err) + assert.Equal(t, "notify", cfg.CLI.UpdateCheck) + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Contains(t, string(data), "# CLI behavior") +} diff --git a/internal/env/env.go b/internal/env/env.go index 71534ea1..99a459c0 100644 --- a/internal/env/env.go +++ b/internal/env/env.go @@ -8,6 +8,11 @@ import ( "github.com/spf13/viper" ) +// UpdateCheckVar is the environment variable that overrides the [cli] +// update_check config key. Named here so error messages can quote the exact +// variable a user set rather than a hardcoded string. +const UpdateCheckVar = "LSTK_UPDATE_CHECK" + type Env struct { AuthToken string LocalStackHost string @@ -25,6 +30,7 @@ type Env struct { JSON bool GitHubToken string MergeStrategy string + UpdateCheck string } // Init initializes environment variable configuration and returns the result. @@ -51,6 +57,9 @@ func Init() *Env { AnalyticsEndpoint: viper.GetString("analytics_endpoint"), GitHubToken: viper.GetString("github_token"), MergeStrategy: viper.GetString("merge_strategy"), + // Captured here rather than read from viper later: config.loadConfig + // calls viper.Reset(), which drops the env-var binding this relies on. + UpdateCheck: viper.GetString("update_check"), } } diff --git a/internal/env/env_test.go b/internal/env/env_test.go new file mode 100644 index 00000000..e9ac943f --- /dev/null +++ b/internal/env/env_test.go @@ -0,0 +1,25 @@ +package env + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestInitReadsUpdateCheck(t *testing.T) { + t.Setenv("LSTK_UPDATE_CHECK", "off") + + cfg := Init() + + assert.Equal(t, "off", cfg.UpdateCheck) +} + +func TestInitLeavesUpdateCheckEmptyWhenUnset(t *testing.T) { + // Explicitly cleared: without this the test reads the developer's own + // environment and fails for anyone who exports the variable. + t.Setenv("LSTK_UPDATE_CHECK", "") + + cfg := Init() + + assert.Empty(t, cfg.UpdateCheck) +} diff --git a/internal/output/error_code.go b/internal/output/error_code.go index 2c081028..45435b5f 100644 --- a/internal/output/error_code.go +++ b/internal/output/error_code.go @@ -6,40 +6,41 @@ package output type ErrorCode string const ( - ErrRuntimeUnavailable ErrorCode = "RUNTIME_UNAVAILABLE" - ErrImagePullFailed ErrorCode = "IMAGE_PULL_FAILED" - ErrEmulatorNotRunning ErrorCode = "EMULATOR_NOT_RUNNING" - ErrEmulatorAlreadyRunning ErrorCode = "EMULATOR_ALREADY_RUNNING" - ErrEmulatorWrongType ErrorCode = "EMULATOR_WRONG_TYPE" - ErrEmulatorNotConfigured ErrorCode = "EMULATOR_NOT_CONFIGURED" - ErrEmulatorStartFailed ErrorCode = "EMULATOR_START_FAILED" - ErrAuthRequired ErrorCode = "AUTH_REQUIRED" - ErrAuthLoginFailed ErrorCode = "AUTH_LOGIN_FAILED" - ErrCredentialsMissing ErrorCode = "CREDENTIALS_MISSING" - ErrLicenseInvalid ErrorCode = "LICENSE_INVALID" - ErrLicenseUnsupportedTag ErrorCode = "LICENSE_UNSUPPORTED_TAG" - ErrLicenseNotCovered ErrorCode = "LICENSE_NOT_COVERED" - ErrSnapshotNotFound ErrorCode = "SNAPSHOT_NOT_FOUND" - ErrSnapshotInvalidRef ErrorCode = "SNAPSHOT_INVALID_REF" - ErrSnapshotRemoteError ErrorCode = "SNAPSHOT_REMOTE_ERROR" - ErrSnapshotBucketNotFound ErrorCode = "SNAPSHOT_BUCKET_NOT_FOUND" - ErrConfigInvalid ErrorCode = "CONFIG_INVALID" - ErrConfigNotFound ErrorCode = "CONFIG_NOT_FOUND" - ErrIntegrationNotSetUp ErrorCode = "INTEGRATION_NOT_SET_UP" - ErrDependencyMissing ErrorCode = "DEPENDENCY_MISSING" - ErrDNSResolutionRequired ErrorCode = "DNS_RESOLUTION_REQUIRED" - ErrPortConflict ErrorCode = "PORT_CONFLICT" - ErrConfirmationRequired ErrorCode = "CONFIRMATION_REQUIRED" - ErrValidationError ErrorCode = "VALIDATION_ERROR" - ErrUsageError ErrorCode = "USAGE_ERROR" - ErrNotJSONCapable ErrorCode = "NOT_JSON_CAPABLE" - ErrNetworkError ErrorCode = "NETWORK_ERROR" - ErrCancelled ErrorCode = "CANCELLED" - ErrInternal ErrorCode = "INTERNAL_ERROR" - ErrIACFileNotFound ErrorCode = "IAC_FILE_NOT_FOUND" - ErrIACNoToolDetected ErrorCode = "IAC_NO_TOOL_DETECTED" - ErrIACToolAmbiguous ErrorCode = "IAC_TOOL_AMBIGUOUS" - ErrIACDeployFailed ErrorCode = "IAC_DEPLOY_FAILED" + ErrRuntimeUnavailable ErrorCode = "RUNTIME_UNAVAILABLE" + ErrImagePullFailed ErrorCode = "IMAGE_PULL_FAILED" + ErrEmulatorNotRunning ErrorCode = "EMULATOR_NOT_RUNNING" + ErrEmulatorAlreadyRunning ErrorCode = "EMULATOR_ALREADY_RUNNING" + ErrEmulatorWrongType ErrorCode = "EMULATOR_WRONG_TYPE" + ErrEmulatorNotConfigured ErrorCode = "EMULATOR_NOT_CONFIGURED" + ErrEmulatorStartFailed ErrorCode = "EMULATOR_START_FAILED" + ErrAuthRequired ErrorCode = "AUTH_REQUIRED" + ErrAuthLoginFailed ErrorCode = "AUTH_LOGIN_FAILED" + ErrCredentialsMissing ErrorCode = "CREDENTIALS_MISSING" + ErrLicenseInvalid ErrorCode = "LICENSE_INVALID" + ErrLicenseUnsupportedTag ErrorCode = "LICENSE_UNSUPPORTED_TAG" + ErrLicenseNotCovered ErrorCode = "LICENSE_NOT_COVERED" + ErrSnapshotNotFound ErrorCode = "SNAPSHOT_NOT_FOUND" + ErrSnapshotInvalidRef ErrorCode = "SNAPSHOT_INVALID_REF" + ErrSnapshotRemoteError ErrorCode = "SNAPSHOT_REMOTE_ERROR" + ErrSnapshotBucketNotFound ErrorCode = "SNAPSHOT_BUCKET_NOT_FOUND" + ErrConfigInvalid ErrorCode = "CONFIG_INVALID" + ErrConfigNotFound ErrorCode = "CONFIG_NOT_FOUND" + ErrIntegrationNotSetUp ErrorCode = "INTEGRATION_NOT_SET_UP" + ErrDependencyMissing ErrorCode = "DEPENDENCY_MISSING" + ErrDNSResolutionRequired ErrorCode = "DNS_RESOLUTION_REQUIRED" + ErrPortConflict ErrorCode = "PORT_CONFLICT" + ErrConfirmationRequired ErrorCode = "CONFIRMATION_REQUIRED" + ErrValidationError ErrorCode = "VALIDATION_ERROR" + ErrUsageError ErrorCode = "USAGE_ERROR" + ErrNotJSONCapable ErrorCode = "NOT_JSON_CAPABLE" + ErrNetworkError ErrorCode = "NETWORK_ERROR" + ErrUpdateExternallyManaged ErrorCode = "UPDATE_EXTERNALLY_MANAGED" + ErrCancelled ErrorCode = "CANCELLED" + ErrInternal ErrorCode = "INTERNAL_ERROR" + ErrIACFileNotFound ErrorCode = "IAC_FILE_NOT_FOUND" + ErrIACNoToolDetected ErrorCode = "IAC_NO_TOOL_DETECTED" + ErrIACToolAmbiguous ErrorCode = "IAC_TOOL_AMBIGUOUS" + ErrIACDeployFailed ErrorCode = "IAC_DEPLOY_FAILED" ) // retryableCodes is the single source of truth for whether a given ErrorCode @@ -127,6 +128,7 @@ var allErrorCodes = []ErrorCode{ ErrUsageError, ErrNotJSONCapable, ErrNetworkError, + ErrUpdateExternallyManaged, ErrCancelled, ErrInternal, ErrIACFileNotFound, @@ -139,40 +141,44 @@ var allErrorCodes = []ErrorCode{ // static ErrorCategory, mirroring retryableCodes above. Every code in // allErrorCodes SHALL have an entry here. var categoryByCode = map[ErrorCode]ErrorCategory{ - ErrRuntimeUnavailable: CategoryRuntime, - ErrImagePullFailed: CategoryRuntime, - ErrDependencyMissing: CategoryRuntime, - ErrDNSResolutionRequired: CategoryRuntime, - ErrNetworkError: CategoryRuntime, - ErrPortConflict: CategoryRuntime, - ErrEmulatorNotRunning: CategoryEmulator, - ErrEmulatorAlreadyRunning: CategoryEmulator, - ErrEmulatorWrongType: CategoryEmulator, - ErrEmulatorNotConfigured: CategoryEmulator, - ErrEmulatorStartFailed: CategoryEmulator, - ErrAuthRequired: CategoryAuth, - ErrAuthLoginFailed: CategoryAuth, - ErrCredentialsMissing: CategoryAuth, - ErrLicenseInvalid: CategoryAuth, - ErrLicenseUnsupportedTag: CategoryAuth, - ErrLicenseNotCovered: CategoryAuth, - ErrSnapshotNotFound: CategoryResource, - ErrSnapshotInvalidRef: CategoryResource, - ErrSnapshotRemoteError: CategoryResource, - ErrSnapshotBucketNotFound: CategoryResource, - ErrConfigInvalid: CategoryConfig, - ErrConfigNotFound: CategoryConfig, - ErrIntegrationNotSetUp: CategoryConfig, - ErrConfirmationRequired: CategoryUsage, - ErrValidationError: CategoryUsage, - ErrUsageError: CategoryUsage, - ErrNotJSONCapable: CategoryUsage, - ErrCancelled: CategoryInternal, - ErrInternal: CategoryInternal, - ErrIACFileNotFound: CategoryIAC, - ErrIACNoToolDetected: CategoryIAC, - ErrIACToolAmbiguous: CategoryIAC, - ErrIACDeployFailed: CategoryIAC, + ErrRuntimeUnavailable: CategoryRuntime, + ErrImagePullFailed: CategoryRuntime, + ErrDependencyMissing: CategoryRuntime, + ErrDNSResolutionRequired: CategoryRuntime, + ErrNetworkError: CategoryRuntime, + // Runtime rather than Usage: how lstk was installed is a fact about the + // machine, not something the invocation can fix. --force exists, but the + // correct response is to update through the manager that owns the install. + ErrUpdateExternallyManaged: CategoryRuntime, + ErrPortConflict: CategoryRuntime, + ErrEmulatorNotRunning: CategoryEmulator, + ErrEmulatorAlreadyRunning: CategoryEmulator, + ErrEmulatorWrongType: CategoryEmulator, + ErrEmulatorNotConfigured: CategoryEmulator, + ErrEmulatorStartFailed: CategoryEmulator, + ErrAuthRequired: CategoryAuth, + ErrAuthLoginFailed: CategoryAuth, + ErrCredentialsMissing: CategoryAuth, + ErrLicenseInvalid: CategoryAuth, + ErrLicenseUnsupportedTag: CategoryAuth, + ErrLicenseNotCovered: CategoryAuth, + ErrSnapshotNotFound: CategoryResource, + ErrSnapshotInvalidRef: CategoryResource, + ErrSnapshotRemoteError: CategoryResource, + ErrSnapshotBucketNotFound: CategoryResource, + ErrConfigInvalid: CategoryConfig, + ErrConfigNotFound: CategoryConfig, + ErrIntegrationNotSetUp: CategoryConfig, + ErrConfirmationRequired: CategoryUsage, + ErrValidationError: CategoryUsage, + ErrUsageError: CategoryUsage, + ErrNotJSONCapable: CategoryUsage, + ErrCancelled: CategoryInternal, + ErrInternal: CategoryInternal, + ErrIACFileNotFound: CategoryIAC, + ErrIACNoToolDetected: CategoryIAC, + ErrIACToolAmbiguous: CategoryIAC, + ErrIACDeployFailed: CategoryIAC, } // Category reports the code's static, coarse grouping. Every ErrorCode in diff --git a/internal/output/error_code_test.go b/internal/output/error_code_test.go index aaf53e3b..5e65e6ce 100644 --- a/internal/output/error_code_test.go +++ b/internal/output/error_code_test.go @@ -31,8 +31,8 @@ func TestErrorCode_AllErrorCodesIsComplete(t *testing.T) { t.Errorf("ErrorCode %q appears %d times in allErrorCodes, want exactly once", code, count) } } - if len(allErrorCodes) != 34 { - t.Errorf("expected 34 documented error codes, got %d — update this test's expectation alongside error-codes/spec.md if a code was intentionally added or removed", len(allErrorCodes)) + if len(allErrorCodes) != 35 { + t.Errorf("expected 35 documented error codes, got %d — update this test's expectation alongside error-codes/spec.md if a code was intentionally added or removed", len(allErrorCodes)) } } diff --git a/internal/ui/run_update.go b/internal/ui/run_update.go index ebafe295..d8f786b3 100644 --- a/internal/ui/run_update.go +++ b/internal/ui/run_update.go @@ -10,7 +10,7 @@ import ( "github.com/localstack/lstk/internal/update" ) -func RunUpdate(parentCtx context.Context, checkOnly bool, githubToken string) error { +func RunUpdate(parentCtx context.Context, checkOnly bool, githubToken string, force bool) error { ctx, cancel := context.WithCancel(parentCtx) defer cancel() @@ -19,7 +19,7 @@ func RunUpdate(parentCtx context.Context, checkOnly bool, githubToken string) er runErrCh := make(chan error, 1) go func() { - err := update.Update(ctx, output.NewTUISink(programSender{p: p}), checkOnly, githubToken) + err := update.Update(ctx, output.NewTUISink(programSender{p: p}), checkOnly, githubToken, force) runErrCh <- err if err != nil && !errors.Is(err, context.Canceled) { p.Send(runErrMsg{err: err}) diff --git a/internal/update/external_install.go b/internal/update/external_install.go new file mode 100644 index 00000000..2646b84e --- /dev/null +++ b/internal/update/external_install.go @@ -0,0 +1,124 @@ +package update + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "syscall" + + "github.com/localstack/lstk/internal/output" +) + +// installDirWritable reports whether the directory holding the given +// executable path can be written to, which is what an in-place binary update +// requires. It is the backstop for install methods no path marker in +// externalMarkers recognizes — a root-owned /usr/bin install run as a normal +// user, a read-only container layer, an immutable store lstk has not been +// taught about. +// +// It probes by creating and removing a file rather than calling access(2), +// which can report success for root or under an ACL that the subsequent rename +// would still fail. The probe costs ~50µs against access(2)'s ~5µs, which is +// why it is confined to the explicit `lstk update` path — where it precedes a +// multi-megabyte download and the difference is noise. It must never be put on +// the automatic start-path check, which runs on every `lstk start`. +// +// A permission error means "not writable" rather than a failure; any other +// error is returned, so a caller never reads an unrelated I/O fault as a +// read-only install. +func installDirWritable(exePath string) (bool, error) { + dir := filepath.Dir(exePath) + f, err := os.CreateTemp(dir, ".lstk-update-probe-*") + if err != nil { + if errors.Is(err, fs.ErrPermission) || errors.Is(err, os.ErrPermission) { + return false, nil + } + // A read-only filesystem surfaces as EROFS, which is not ErrPermission. + if isReadOnlyFSError(err) { + return false, nil + } + return false, fmt.Errorf("cannot determine whether %s is writable: %w", dir, err) + } + name := f.Name() + if err := f.Close(); err != nil { + _ = os.Remove(name) + return false, fmt.Errorf("cannot close write probe in %s: %w", dir, err) + } + if err := os.Remove(name); err != nil { + return false, fmt.Errorf("cannot remove write probe %s: %w", name, err) + } + return true, nil +} + +// isReadOnlyFSError reports whether err is a read-only filesystem error, which +// is how an immutable store (nix, a read-only container layer) refuses a write +// rather than with a permission error. syscall.EROFS is defined on Windows too, +// so this needs no per-platform variant. +func isReadOnlyFSError(err error) bool { + return errors.Is(err, syscall.EROFS) +} + +// selfUpdateBlocker explains why lstk must not replace its own binary in place. +// Manager names the external tool that owns the install; it is empty when the +// only problem is that the install directory cannot be written to. +type selfUpdateBlocker struct { + Manager string + Path string +} + +func (b selfUpdateBlocker) title() string { + if b.Manager != "" { + return fmt.Sprintf("lstk is managed by %s and will not update itself", b.Manager) + } + return "lstk cannot update itself: its install directory is not writable" +} + +func (b selfUpdateBlocker) summary() string { + return fmt.Sprintf("Installed at %s", b.Path) +} + +func (b selfUpdateBlocker) action() output.ErrorAction { + if b.Manager != "" { + return output.ErrorAction{ + Label: fmt.Sprintf("Update it through %s, or force an in-place replacement:", b.Manager), + Value: "lstk update --force", + } + } + return output.ErrorAction{ + Label: fmt.Sprintf("Update lstk the way it was installed, grant write access to %s, or force it:", filepath.Dir(b.Path)), + Value: "lstk update --force", + } +} + +// blockSelfUpdate reports why an in-place binary replacement must not be +// attempted, or nil when it may proceed. +// +// Homebrew and npm installs are never blocked: they delegate to `brew upgrade` +// and `npm install -g`, which own the install directory themselves and work +// even where lstk cannot write to it directly. +// +// An indeterminate writability probe deliberately does not block. Guessing +// "read-only" from an unrelated I/O error would refuse an update that would +// have worked; falling through instead leaves the pre-existing behavior, where +// the rename reports the real failure. +func blockSelfUpdate(info InstallInfo) *selfUpdateBlocker { + if info.Method == InstallExternal { + return &selfUpdateBlocker{Manager: info.Manager, Path: info.ResolvedPath} + } + if info.Method != InstallBinary { + return nil + } + // os.Executable() failed, so there is no install directory to probe. + // filepath.Dir("") is ".", which would write the probe into the user's + // working directory and report an unrelated path in the refusal. + if info.ResolvedPath == "" { + return nil + } + writable, err := installDirWritable(info.ResolvedPath) + if err != nil || writable { + return nil + } + return &selfUpdateBlocker{Path: info.ResolvedPath} +} diff --git a/internal/update/external_install_test.go b/internal/update/external_install_test.go new file mode 100644 index 00000000..94d4c837 --- /dev/null +++ b/internal/update/external_install_test.go @@ -0,0 +1,277 @@ +package update + +import ( + "os" + "path/filepath" + goruntime "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestClassifyPathExternalManagers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + path string + wantMethod InstallMethod + wantManager string + }{ + { + name: "nix store", + path: "/nix/store/9zk1abcd-lstk-0.5.0/bin/lstk", + wantMethod: InstallExternal, + wantManager: "nix", + }, + { + name: "guix store", + path: "/gnu/store/abcd1234-lstk-0.5.0/bin/lstk", + wantMethod: InstallExternal, + wantManager: "guix", + }, + { + name: "mise install", + path: "/Users/joe/.local/share/mise/installs/github-localstack-lstk/latest/lstk", + wantMethod: InstallExternal, + wantManager: "mise", + }, + { + name: "mise shim", + path: "/Users/joe/.local/share/mise/shims/lstk", + wantMethod: InstallExternal, + wantManager: "mise", + }, + { + name: "legacy rtx install reports as mise", + path: "/Users/joe/.local/share/rtx/installs/lstk/0.5.0/lstk", + wantMethod: InstallExternal, + wantManager: "mise", + }, + { + name: "asdf via ASDF_DATA_DIR (no leading dot)", + path: "/home/user/.local/share/asdf/installs/lstk/0.5.0/bin/lstk", + wantMethod: InstallExternal, + wantManager: "asdf", + }, + { + name: "scoop shim (the path actually on PATH)", + path: "C:/Users/joe/scoop/shims/lstk.exe", + wantMethod: InstallExternal, + wantManager: "scoop", + }, + { + name: "chocolatey bin (the path actually on PATH)", + path: "C:/ProgramData/chocolatey/bin/lstk.exe", + wantMethod: InstallExternal, + wantManager: "chocolatey", + }, + { + name: "asdf install", + path: "/home/user/.asdf/installs/lstk/0.5.0/bin/lstk", + wantMethod: InstallExternal, + wantManager: "asdf", + }, + { + name: "asdf shim", + path: "/home/user/.asdf/shims/lstk", + wantMethod: InstallExternal, + wantManager: "asdf", + }, + { + name: "scoop", + path: "C:/Users/joe/scoop/apps/lstk/current/lstk.exe", + wantMethod: InstallExternal, + wantManager: "scoop", + }, + { + name: "chocolatey", + path: "C:/ProgramData/chocolatey/lib/lstk/tools/lstk.exe", + wantMethod: InstallExternal, + wantManager: "chocolatey", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + method, manager := classifyPath(tt.path) + assert.Equal(t, tt.wantMethod, method) + assert.Equal(t, tt.wantManager, manager) + }) + } +} + +// The npm and Homebrew install methods can update themselves correctly even +// when their interpreter or prefix was provisioned by a tool manager, so their +// markers must win over the tool-manager markers regardless of which appears +// first in the path. +func TestClassifyPathSelfUpdatableMethodsWinOverToolManagers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + path string + want InstallMethod + }{ + { + name: "npm under a mise-managed node", + path: "/Users/someone/.local/share/mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_darwin_arm64/lstk", + want: InstallNPM, + }, + { + name: "npm under an asdf-managed node", + path: "/Users/geo/.asdf/installs/nodejs/22.12.0/lib/node_modules/@localstack/lstk_darwin_arm64/lstk", + want: InstallNPM, + }, + { + name: "homebrew cask under a scoop-shaped path", + path: "/opt/homebrew/Caskroom/lstk/0.3.0/lstk", + want: InstallHomebrew, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + method, manager := classifyPath(tt.path) + assert.Equal(t, tt.want, method) + assert.Empty(t, manager, "only an externally-managed install names a manager") + }) + } +} + +func TestClassifyPathOrdinaryInstallsNameNoManager(t *testing.T) { + t.Parallel() + + for _, path := range []string{"/usr/local/bin/lstk", "/home/user/bin/lstk", "/home/user/Projects/lstk/bin/lstk"} { + method, manager := classifyPath(path) + assert.Equal(t, InstallBinary, method, path) + assert.Empty(t, manager, path) + } +} + +func TestInstallDirWritable(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + writable, err := installDirWritable(filepath.Join(dir, "lstk")) + require.NoError(t, err) + assert.True(t, writable) +} + +func TestInstallDirNotWritable(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("POSIX directory permissions do not port to Windows") + } + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions") + } + t.Parallel() + + dir := t.TempDir() + readOnly := filepath.Join(dir, "ro") + require.NoError(t, os.Mkdir(readOnly, 0500)) + + writable, err := installDirWritable(filepath.Join(readOnly, "lstk")) + require.NoError(t, err) + assert.False(t, writable) +} + +func TestInstallDirWritableLeavesNoProbeFileBehind(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + _, err := installDirWritable(filepath.Join(dir, "lstk")) + require.NoError(t, err) + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + assert.Empty(t, entries, "the probe must not leave a file in the install directory") +} + +// The JSON envelope's "method" field reports how the update was performed, and +// its documented values are homebrew/npm/binary. A --force update on an +// externally-managed install performs a binary replacement, so it must report +// "binary" rather than leaking the new install-method name into that enum. +func TestAppliedMethodName(t *testing.T) { + t.Parallel() + + assert.Equal(t, "homebrew", appliedMethodName(InstallHomebrew)) + assert.Equal(t, "npm", appliedMethodName(InstallNPM)) + assert.Equal(t, "binary", appliedMethodName(InstallBinary)) + assert.Equal(t, "binary", appliedMethodName(InstallExternal)) +} + +func TestBlockSelfUpdate(t *testing.T) { + t.Parallel() + + t.Run("external install is blocked and names the manager", func(t *testing.T) { + t.Parallel() + blocker := blockSelfUpdate(InstallInfo{ + Method: InstallExternal, + Manager: "mise", + ResolvedPath: "/home/u/.local/share/mise/installs/lstk/latest/lstk", + }) + require.NotNil(t, blocker) + assert.Equal(t, "mise", blocker.Manager) + assert.Contains(t, blocker.title(), "mise") + }) + + t.Run("homebrew is never blocked", func(t *testing.T) { + t.Parallel() + assert.Nil(t, blockSelfUpdate(InstallInfo{Method: InstallHomebrew, ResolvedPath: "/opt/homebrew/Caskroom/lstk/1/lstk"})) + }) + + t.Run("npm is never blocked", func(t *testing.T) { + t.Parallel() + assert.Nil(t, blockSelfUpdate(InstallInfo{Method: InstallNPM, ResolvedPath: "/usr/local/lib/node_modules/x/lstk"})) + }) + + t.Run("writable binary install proceeds", func(t *testing.T) { + t.Parallel() + assert.Nil(t, blockSelfUpdate(InstallInfo{Method: InstallBinary, ResolvedPath: filepath.Join(t.TempDir(), "lstk")})) + }) + + t.Run("read-only binary install is blocked and names the directory", func(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("POSIX directory permissions do not port to Windows") + } + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions") + } + t.Parallel() + dir := filepath.Join(t.TempDir(), "ro") + require.NoError(t, os.Mkdir(dir, 0500)) + t.Cleanup(func() { _ = os.Chmod(dir, 0700) }) + + blocker := blockSelfUpdate(InstallInfo{Method: InstallBinary, ResolvedPath: filepath.Join(dir, "lstk")}) + require.NotNil(t, blocker) + assert.Empty(t, blocker.Manager) + assert.Contains(t, blocker.action().Label, dir) + }) + +} + +// os.Executable() failed, so there is no install directory. filepath.Dir("") is +// ".", so an unguarded probe runs against the working directory: from a +// read-only cwd it reports the install as unwritable and refuses the update, +// naming a directory that has nothing to do with where lstk lives. +// +// Not parallel: t.Chdir cannot be used from a parallel test. +func TestBlockSelfUpdateUnknownPathIgnoresWorkingDirectory(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("POSIX directory permissions do not port to Windows") + } + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions") + } + + readOnly := filepath.Join(t.TempDir(), "ro") + require.NoError(t, os.Mkdir(readOnly, 0500)) + t.Cleanup(func() { _ = os.Chmod(readOnly, 0700) }) + t.Chdir(readOnly) + + assert.Nil(t, blockSelfUpdate(InstallInfo{Method: InstallBinary, ResolvedPath: ""})) +} diff --git a/internal/update/install_method.go b/internal/update/install_method.go index f882411e..7c4df67c 100644 --- a/internal/update/install_method.go +++ b/internal/update/install_method.go @@ -3,6 +3,7 @@ package update import ( "os" "path/filepath" + "slices" "strings" ) @@ -12,22 +13,13 @@ const ( InstallBinary InstallMethod = iota // standalone binary download InstallHomebrew // installed via Homebrew cask InstallNPM // installed via npm + InstallExternal // managed by an external tool (nix, mise, ...) ) -func (m InstallMethod) String() string { - switch m { - case InstallHomebrew: - return "homebrew" - case InstallNPM: - return "npm" - default: - return "binary" - } -} - // InstallInfo holds the detected install method and the resolved binary path. type InstallInfo struct { Method InstallMethod + Manager string ResolvedPath string } @@ -42,25 +34,86 @@ func DetectInstallMethod() InstallInfo { if err != nil { resolved = exe } + method, manager := classifyPath(resolved) return InstallInfo{ - Method: classifyPath(resolved), + Method: method, + Manager: manager, ResolvedPath: resolved, } } -func classifyPath(resolved string) InstallMethod { +// externalMarker identifies an externally-managed install by an adjacent pair +// of path segments: `first` immediately followed by any of `second`. Requiring +// two adjacent segments rather than one keeps an unrelated directory that +// happens to be called "mise" or "scoop" from being read as an install root. +type externalMarker struct { + first string + second []string + manager string +} + +// externalMarkers covers tool managers whose whole purpose is to own the +// version of the binary they installed, and immutable stores lstk cannot write +// to at all. `rtx` is mise's former directory name and reports as mise, since +// that is the tool the user would run. +// Each manager lists both its install root and the launcher directory that is +// actually on PATH — `shims`/`bin` entries are not symlinks into the install +// root on every platform (scoop's shims are launcher executables, asdf's are +// shell scripts), so EvalSymlinks does not rewrite them and the install-root +// marker alone would miss the common case. +var externalMarkers = []externalMarker{ + {first: "nix", second: []string{"store"}, manager: "nix"}, + {first: "gnu", second: []string{"store"}, manager: "guix"}, + {first: "mise", second: []string{"installs", "shims"}, manager: "mise"}, + {first: "rtx", second: []string{"installs", "shims"}, manager: "mise"}, + // Both layouts: ~/.asdf and the ASDF_DATA_DIR convention + // (~/.local/share/asdf) that Homebrew's asdf formula documents. + {first: ".asdf", second: []string{"installs", "shims"}, manager: "asdf"}, + {first: "asdf", second: []string{"installs", "shims"}, manager: "asdf"}, + {first: "scoop", second: []string{"apps", "shims"}, manager: "scoop"}, + {first: "chocolatey", second: []string{"lib", "bin"}, manager: "chocolatey"}, +} + +// classifyPath determines the install method from a resolved executable path, +// returning the recognized external manager's name for InstallExternal and an +// empty string for every other method. +// +// The npm and Homebrew markers are checked across the whole path *before* any +// external marker, and that order is load-bearing: an npm- or Homebrew-managed +// lstk may sit under a tool-manager-provisioned interpreter or prefix (e.g. +// .../mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_.../lstk), +// where the tool manager owns node but `npm install -g` still updates lstk +// correctly. A single in-order segment walk would see "mise" first and refuse +// to update a perfectly updatable install. +func classifyPath(resolved string) (InstallMethod, string) { cleaned := filepath.Clean(resolved) segments := strings.Split(cleaned, string(os.PathSeparator)) for _, seg := range segments { lower := strings.ToLower(seg) if lower == "caskroom" { - return InstallHomebrew + return InstallHomebrew, "" } if lower == "node_modules" { - return InstallNPM + return InstallNPM, "" + } + } + + for i, seg := range segments { + if i+1 >= len(segments) { + break + } + lower := strings.ToLower(seg) + next := strings.ToLower(segments[i+1]) + for _, m := range externalMarkers { + if lower != m.first { + continue + } + if slices.Contains(m.second, next) { + return InstallExternal, m.manager + } } } - return InstallBinary + return InstallBinary, "" } diff --git a/internal/update/install_method_test.go b/internal/update/install_method_test.go index ca2087eb..c261f33e 100644 --- a/internal/update/install_method_test.go +++ b/internal/update/install_method_test.go @@ -57,7 +57,7 @@ func TestClassifyPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := classifyPath(tt.path) + got, _ := classifyPath(tt.path) if got != tt.want { t.Fatalf("classifyPath(%q) = %v, want %v", tt.path, got, tt.want) } diff --git a/internal/update/notify.go b/internal/update/notify.go index 244420df..7eb3289e 100644 --- a/internal/update/notify.go +++ b/internal/update/notify.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/localstack/lstk/internal/config" "github.com/localstack/lstk/internal/output" "github.com/localstack/lstk/internal/version" ) @@ -12,10 +13,27 @@ import ( type versionFetcher func(ctx context.Context, token string) (string, error) type NotifyOptions struct { - GitHubToken string - UpdatePrompt bool - SkippedVersion string - PersistSkipVersion func(version string) error + GitHubToken string + // CanPrompt reports whether this call site is able to present a blocking + // prompt at all (an interactive TTY). It is independent of Mode, which is + // the user's preference: a non-interactive start can only ever emit a note, + // however Mode is set. + CanPrompt bool + Mode config.UpdateCheckMode + PersistUpdateCheck func(mode config.UpdateCheckMode) error + // DetectInstall resolves how lstk itself was installed. Injected rather + // than called directly so tests do not depend on where the test binary + // happens to live — which is also what makes the apply-time guard on the + // prompt path testable. Defaults to DetectInstallMethod when nil. + DetectInstall func() InstallInfo +} + +// installInfo resolves the install once per notification. +func (o NotifyOptions) installInfo() InstallInfo { + if o.DetectInstall == nil { + return DetectInstallMethod() + } + return o.DetectInstall() } const checkTimeout = 2 * time.Second @@ -51,35 +69,72 @@ func NotifyUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions) (ex } func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyOptions, currentVersion string, fetch versionFetcher) (exitAfter bool) { + if opts.Mode == config.UpdateCheckOff { + return false + } + current, latest, available := checkQuietlyWithVersion(ctx, opts.GitHubToken, currentVersion, fetch) if !available { return false } - if opts.SkippedVersion != "" && normalizeVersion(opts.SkippedVersion) == normalizeVersion(latest) { - return false + // Detection runs exactly once, and only now that an update is known to + // exist — which is what keeps it off every `lstk start`. It runs regardless + // of how the mode was set, because its answer feeds the note's wording as + // well as the prompt/note decision, and a note that says "run lstk update" + // on an install where that command refuses is wrong however the mode was + // reached. + info := opts.installInfo() + external := info.Method == InstallExternal + + mode := opts.Mode + if mode == config.UpdateCheckUnset { + mode = config.UpdateCheckPrompt } - if !opts.UpdatePrompt { - sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: fmt.Sprintf("Update available: %s → %s (run lstk update)", current, latest)}) + // An externally-managed install is never prompted, even when the user asked + // for prompt explicitly: "Update now" would replace a binary the external + // tool owns, and applyUpdate refuses it anyway. Offering an action that + // cannot be carried out is worse than not offering it. + if !opts.CanPrompt || external || mode == config.UpdateCheckNotify { + sink.Emit(updateNote(current, latest, info.Manager)) return false } - return promptAndUpdate(ctx, sink, opts, current, latest) + return promptAndUpdate(ctx, sink, opts, current, latest, info) +} + +// updateNote is the non-blocking "a newer version exists" line. When manager is +// set, it names that tool instead of pointing at `lstk update` — which refuses +// on an externally-managed install, so advising it there would send the user at +// a command that cannot work. +func updateNote(current, latest, manager string) output.MessageEvent { + text := fmt.Sprintf("Update available: %s → %s (run lstk update)", current, latest) + if manager != "" { + text = fmt.Sprintf("Update available: %s → %s (installed via %s — update it there)", current, latest, manager) + } + return output.MessageEvent{Severity: output.SeverityNote, Text: text} } -func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, current, latest string) (exitAfter bool) { +func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, current, latest string, info InstallInfo) (exitAfter bool) { releaseNotesURL := fmt.Sprintf("https://github.com/%s/releases/latest", githubRepo) sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: fmt.Sprintf("New lstk version available! %s → %s", current, latest)}) sink.Emit(output.MessageEvent{Severity: output.SeveritySecondary, Text: fmt.Sprintf("> Release notes: %s", releaseNotesURL)}) - responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.ActionChoice("Update lstk to latest version?", []output.InputOption{ + options := []output.InputOption{ {Key: "u", Label: "Update now"}, {Key: "r", Label: "Remind me next time"}, - {Key: "s", Label: "Skip this version"}, - }, responseCh)) + } + // Offered only when there is somewhere to write it. On a first run + // config.toml does not exist yet, and persisting would be silently dropped + // — telling the user their choice was saved when it was not. + if opts.PersistUpdateCheck != nil { + options = append(options, output.InputOption{Key: "n", Label: "Never ask again"}) + } + + responseCh := make(chan output.InputResponse, 1) + sink.Emit(output.ActionChoice("Update lstk to latest version?", options, responseCh)) var resp output.InputResponse select { @@ -94,7 +149,19 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, switch resp.SelectedKey { case "u": - if _, err := applyUpdate(ctx, sink, latest, opts.GitHubToken); err != nil { + // A refusal here is recoverable and the start continues, so it is + // surfaced as a single warning. Rendering it as an ErrorEvent (what the + // `lstk update` entry point does) would leave a persistent failure + // block on screen while the emulator comes up underneath it. + _, blocker, err := applyUpdate(ctx, sink, latest, opts.GitHubToken, false, info) + if blocker != nil { + sink.Emit(output.MessageEvent{ + Severity: output.SeverityWarning, + Text: fmt.Sprintf("%s (%s). %s", blocker.title(), blocker.summary(), blocker.action().Label), + }) + return false + } + if err != nil { sink.Emit(output.MessageEvent{Severity: output.SeverityWarning, Text: fmt.Sprintf("Update failed: %v", err)}) return false } @@ -102,13 +169,22 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, return true case "r": return false - case "s": - if opts.PersistSkipVersion != nil { - if err := opts.PersistSkipVersion(latest); err != nil { - sink.Emit(output.MessageEvent{Severity: output.SeverityWarning, Text: fmt.Sprintf("Failed to persist skipped version: %v", err)}) - } + case "n": + // Persists notify rather than off: the user asked to stop being + // interrupted, which is not the same as asking never to hear about a + // release again. Silencing entirely stays a deliberate config edit. + if opts.PersistUpdateCheck == nil { + // Unreachable while the option is only offered when the hook is set + // (see above), but a future edit that always appends it must warn + // rather than panic mid-start. + sink.Emit(output.MessageEvent{Severity: output.SeverityWarning, Text: "Cannot save update preference: no config file"}) + return false + } + if err := opts.PersistUpdateCheck(config.UpdateCheckNotify); err != nil { + sink.Emit(output.MessageEvent{Severity: output.SeverityWarning, Text: fmt.Sprintf("Failed to save update preference: %v", err)}) + return false } - sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Skipping version " + latest}) + sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Won't ask again — new versions will show as a note. Run lstk update to update."}) return false } diff --git a/internal/update/notify_guard_test.go b/internal/update/notify_guard_test.go new file mode 100644 index 00000000..4bbaa659 --- /dev/null +++ b/internal/update/notify_guard_test.go @@ -0,0 +1,240 @@ +package update + +import ( + "context" + "os" + "path/filepath" + goruntime "runtime" + "strings" + "testing" + + "github.com/localstack/lstk/internal/config" + "github.com/localstack/lstk/internal/output" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// An explicit update_check = "prompt" must not produce a prompt on an +// externally-managed install: pressing "Update now" would replace a binary the +// external tool owns, which is the bug this whole feature exists to prevent. +func TestNotifyUpdateExternalInstallNeverPromptsEvenWhenPromptIsExplicit(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + t.Error("an externally-managed install must never be prompted, even with Mode=prompt") + req.ResponseCh() <- output.InputResponse{Cancelled: true} + } + }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckPrompt, + CanPrompt: true, + DetectInstall: func() InstallInfo { + return InstallInfo{Method: InstallExternal, Manager: "mise"} + }, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, events, 1) + msg, ok := events[0].(output.MessageEvent) + require.True(t, ok) + assert.Contains(t, msg.Text, "mise") +} + +// A non-interactive start emits a note too, and it must name the manager for +// the same reason: `lstk update` refuses on such an install. +func TestNotifyUpdateNonInteractiveNoteNamesTheManager(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckUnset, + CanPrompt: false, + DetectInstall: func() InstallInfo { + return InstallInfo{Method: InstallExternal, Manager: "nix"} + }, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, events, 1) + msg, ok := events[0].(output.MessageEvent) + require.True(t, ok) + assert.Contains(t, msg.Text, "nix") + assert.NotContains(t, msg.Text, "lstk update") +} + +// An explicit notify on an externally-managed install must name the manager +// too — the advice is wrong there regardless of how notify was reached. +func TestNotifyUpdateExplicitNotifyNamesTheManager(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckNotify, + CanPrompt: true, + DetectInstall: func() InstallInfo { + return InstallInfo{Method: InstallExternal, Manager: "asdf"} + }, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, events, 1) + msg := events[0].(output.MessageEvent) + assert.Contains(t, msg.Text, "asdf") +} + +// Offering "Never remind me" when there is nowhere to persist it would tell the +// user their choice was saved when it was silently dropped (the first-run case, +// where config.toml does not exist yet). +func TestNotifyUpdateOmitsNeverRemindWhenItCannotBePersisted(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var options []output.InputOption + sink := output.SinkFunc(func(event output.Event) { + if req, ok := event.(output.UserInputRequestEvent); ok { + options = req.Options() + req.ResponseCh() <- output.InputResponse{SelectedKey: "r"} + } + }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckPrompt, + CanPrompt: true, + PersistUpdateCheck: nil, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, options, 2) + for _, o := range options { + assert.NotEqual(t, "n", o.Key, "the never-ask-again option must be absent when unpersistable") + } +} + +// The prompt path's "Update now" is guarded only by applyUpdate — the +// notify-level check lets it through, because a read-only install directory is +// not an externally-managed install. Deleting applyUpdate's guard must fail a +// test, or the round-1 blocker fix is unprotected. +func TestPromptUpdateNowIsRefusedWhenTheBinaryCannotBeReplaced(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("POSIX directory permissions do not port to Windows") + } + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions") + } + + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + readOnly := filepath.Join(t.TempDir(), "ro") + require.NoError(t, os.Mkdir(readOnly, 0500)) + t.Cleanup(func() { _ = os.Chmod(readOnly, 0700) }) + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + req.ResponseCh() <- output.InputResponse{SelectedKey: "u"} + } + }) + + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckPrompt, + CanPrompt: true, + DetectInstall: func() InstallInfo { + return InstallInfo{Method: InstallBinary, ResolvedPath: filepath.Join(readOnly, "lstk")} + }, + }, "1.0.0", testFetcher(server.URL)) + + assert.False(t, exit, "a refused update must not ask the user to re-run") + + var warned bool + for _, e := range events { + msg, ok := e.(output.MessageEvent) + if !ok { + continue + } + assert.NotContains(t, msg.Text, "Updated to", "no update may be reported as applied") + if msg.Severity == output.SeverityWarning && strings.Contains(msg.Text, readOnly) { + warned = true + } + } + assert.True(t, warned, "the refusal must be surfaced, naming the directory") +} + +// A refusal on the prompt path is recoverable, so it must not render as a +// persistent TUI error block (ErrorEvent sets hideHeader and stays on screen +// for the rest of the run) while the emulator start continues underneath it. +func TestPromptRefusalEmitsNoErrorEvent(t *testing.T) { + if goruntime.GOOS == "windows" || os.Geteuid() == 0 { + t.Skip("POSIX directory permissions required") + } + + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + readOnly := filepath.Join(t.TempDir(), "ro") + require.NoError(t, os.Mkdir(readOnly, 0500)) + t.Cleanup(func() { _ = os.Chmod(readOnly, 0700) }) + + var errorEvents int + var warnings int + sink := output.SinkFunc(func(event output.Event) { + switch e := event.(type) { + case output.ErrorEvent: + errorEvents++ + case output.MessageEvent: + if e.Severity == output.SeverityWarning { + warnings++ + } + case output.UserInputRequestEvent: + e.ResponseCh() <- output.InputResponse{SelectedKey: "u"} + } + }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckPrompt, + CanPrompt: true, + DetectInstall: func() InstallInfo { + return InstallInfo{Method: InstallBinary, ResolvedPath: filepath.Join(readOnly, "lstk")} + }, + }, "1.0.0", testFetcher(server.URL)) + + assert.Zero(t, errorEvents, "a recoverable refusal must not render as a failure block") + assert.Equal(t, 1, warnings, "exactly one warning, not a duplicate") +} + +// The prompt offers exactly three choices: apply, defer, or stop asking. +// "Skip this version" was removed — with a weekly release cadence it bought a +// few days of quiet, and the permanent opt-out covers the same need better. +func TestPromptOffersUpdateRemindAndNeverAskAgain(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var options []output.InputOption + sink := output.SinkFunc(func(event output.Event) { + if req, ok := event.(output.UserInputRequestEvent); ok { + options = req.Options() + req.ResponseCh() <- output.InputResponse{SelectedKey: "r"} + } + }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckPrompt, + CanPrompt: true, + PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, options, 3) + keys := []string{options[0].Key, options[1].Key, options[2].Key} + assert.Equal(t, []string{"u", "r", "n"}, keys) + for _, o := range options { + assert.NotEqual(t, "s", o.Key, "skip-this-version must be gone") + } +} diff --git a/internal/update/notify_mode_test.go b/internal/update/notify_mode_test.go new file mode 100644 index 00000000..956ff73a --- /dev/null +++ b/internal/update/notify_mode_test.go @@ -0,0 +1,262 @@ +package update + +import ( + "context" + "testing" + + "github.com/localstack/lstk/internal/config" + "github.com/localstack/lstk/internal/output" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// failingFetcher fails the test if the version check is performed at all. +func failingFetcher(t *testing.T) versionFetcher { + t.Helper() + return func(ctx context.Context, token string) (string, error) { + t.Error("no version check should be performed") + return "", nil + } +} + +func TestNotifyUpdateOffMakesNoRequestAndNoOutput(t *testing.T) { + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + req.ResponseCh() <- output.InputResponse{Cancelled: true} + } + }) + + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckOff, + CanPrompt: true, + }, "1.0.0", failingFetcher(t)) + + assert.False(t, exit) + assert.Empty(t, events) +} + +func TestNotifyUpdateNotifyModeEmitsNoteWithoutPrompting(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + t.Error("notify mode must never prompt") + req.ResponseCh() <- output.InputResponse{Cancelled: true} + } + }) + + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckNotify, + CanPrompt: true, + }, "1.0.0", testFetcher(server.URL)) + + assert.False(t, exit) + require.Len(t, events, 1) + msg, ok := events[0].(output.MessageEvent) + require.True(t, ok) + assert.Equal(t, output.SeverityNote, msg.Severity) + assert.Contains(t, msg.Text, "1.0.0") + assert.Contains(t, msg.Text, "v2.0.0") +} + +func TestNotifyUpdateExternalInstallDowngradesPromptToNote(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + t.Error("an externally-managed install must never be prompted") + req.ResponseCh() <- output.InputResponse{Cancelled: true} + } + }) + + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckUnset, + CanPrompt: true, + DetectInstall: func() InstallInfo { + return InstallInfo{Method: InstallExternal, Manager: "mise"} + }, + }, "1.0.0", testFetcher(server.URL)) + + assert.False(t, exit) + require.Len(t, events, 1) + msg, ok := events[0].(output.MessageEvent) + require.True(t, ok) + assert.Equal(t, output.SeverityNote, msg.Severity) +} + +// off must return before the version check, so detection cannot be reached. +func TestNotifyUpdateOffSkipsDetection(t *testing.T) { + sink := output.SinkFunc(func(event output.Event) { + if req, ok := event.(output.UserInputRequestEvent); ok { + req.ResponseCh() <- output.InputResponse{Cancelled: true} + } + }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckOff, + CanPrompt: true, + DetectInstall: func() InstallInfo { + t.Error("off must not consult install detection") + return InstallInfo{} + }, + }, "1.0.0", failingFetcher(t)) +} + +func TestNotifyUpdateSkipsDetectionWhenNoUpdateAvailable(t *testing.T) { + server := newTestGitHubServer(t, "v1.0.0") + defer server.Close() + + sink := output.SinkFunc(func(event output.Event) { + if req, ok := event.(output.UserInputRequestEvent); ok { + req.ResponseCh() <- output.InputResponse{Cancelled: true} + } + }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckUnset, + CanPrompt: true, + DetectInstall: func() InstallInfo { + t.Error("detection must not run before an update is known to exist") + return InstallInfo{} + }, + }, "v1.0.0", testFetcher(server.URL)) +} + +// A non-interactive call site still emits exactly one note. +func TestNotifyUpdateNonInteractiveEmitsExactlyOneNote(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckUnset, + CanPrompt: false, + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, events, 1) +} + +func TestNotifyUpdateNeverRemindPersistsNotifyAndAppliesNoUpdate(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var persisted config.UpdateCheckMode + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + req.ResponseCh() <- output.InputResponse{SelectedKey: "n"} + } + }) + + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckPrompt, + CanPrompt: true, + PersistUpdateCheck: func(mode config.UpdateCheckMode) error { + persisted = mode + return nil + }, + }, "1.0.0", testFetcher(server.URL)) + + assert.False(t, exit, "choosing never-remind must not restart the command") + assert.Equal(t, config.UpdateCheckNotify, persisted) + // "applies no update" is the other half of the behavior: exit == false + // alone would not notice an update actually being installed. + for _, e := range events { + if msg, ok := e.(output.MessageEvent); ok { + assert.NotContains(t, msg.Text, "Updated to", "no update may be applied") + assert.NotContains(t, msg.Text, "Downloading", "nothing may be downloaded") + } + } +} + +func TestNotifyUpdateNeverRemindWarnsWhenPersistFails(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + req.ResponseCh() <- output.InputResponse{SelectedKey: "n"} + } + }) + + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckPrompt, + CanPrompt: true, + PersistUpdateCheck: func(mode config.UpdateCheckMode) error { + return assert.AnError + }, + }, "1.0.0", testFetcher(server.URL)) + + assert.False(t, exit) + var warned bool + for _, e := range events { + if msg, ok := e.(output.MessageEvent); ok && msg.Severity == output.SeverityWarning { + warned = true + } + } + assert.True(t, warned, "a failure to persist must be surfaced as a warning") +} + +// The generic note tells the user to run `lstk update`, but on an externally +// managed install that command refuses. The note must not send them at a +// command that will not work. +func TestNotifyUpdateExternalNoteNamesTheManagerNotLstkUpdate(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { + events = append(events, event) + if req, ok := event.(output.UserInputRequestEvent); ok { + req.ResponseCh() <- output.InputResponse{Cancelled: true} + } + }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckUnset, + CanPrompt: true, + DetectInstall: func() InstallInfo { + return InstallInfo{Method: InstallExternal, Manager: "mise"} + }, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, events, 1) + msg, ok := events[0].(output.MessageEvent) + require.True(t, ok) + assert.Contains(t, msg.Text, "mise") + assert.NotContains(t, msg.Text, "lstk update") +} + +// A note that was *not* caused by detection keeps pointing at `lstk update`, +// which is the right advice for a self-managed install. +func TestNotifyUpdateOrdinaryNoteStillPointsAtLstkUpdate(t *testing.T) { + server := newTestGitHubServer(t, "v2.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) + + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + Mode: config.UpdateCheckNotify, + CanPrompt: true, + }, "1.0.0", testFetcher(server.URL)) + + require.Len(t, events, 1) + msg, ok := events[0].(output.MessageEvent) + require.True(t, ok) + assert.Contains(t, msg.Text, "lstk update") +} diff --git a/internal/update/notify_test.go b/internal/update/notify_test.go index 499b0916..298473a9 100644 --- a/internal/update/notify_test.go +++ b/internal/update/notify_test.go @@ -2,8 +2,10 @@ package update import ( "context" + "encoding/json" "fmt" + "github.com/localstack/lstk/internal/config" "net/http" "net/http/httptest" "testing" @@ -87,7 +89,7 @@ func TestNotifyUpdateNoUpdateAvailable(t *testing.T) { var events []output.Event sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "v1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{CanPrompt: true}, "v1.0.0", testFetcher(server.URL)) assert.False(t, exit) assert.Empty(t, events) } @@ -108,45 +110,6 @@ func TestNotifyUpdatePromptDisabled(t *testing.T) { assert.Contains(t, msg.Text, "Update available") } -func TestNotifyUpdatePromptSkip(t *testing.T) { - server := newTestGitHubServer(t, "v2.0.0") - defer server.Close() - - var skippedVersion string - var events []output.Event - sink := output.SinkFunc(func(event output.Event) { - events = append(events, event) - if req, ok := event.(output.UserInputRequestEvent); ok { - req.ResponseCh() <- output.InputResponse{SelectedKey: "s"} - } - }) - - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - UpdatePrompt: true, - PersistSkipVersion: func(v string) error { - skippedVersion = v - return nil - }, - }, "1.0.0", testFetcher(server.URL)) - assert.False(t, exit) - assert.Equal(t, "v2.0.0", skippedVersion) -} - -func TestNotifyUpdateSkippedVersionSuppressesPrompt(t *testing.T) { - server := newTestGitHubServer(t, "v2.0.0") - defer server.Close() - - var events []output.Event - sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) - - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - UpdatePrompt: true, - SkippedVersion: "v2.0.0", - }, "1.0.0", testFetcher(server.URL)) - assert.False(t, exit) - assert.Empty(t, events) -} - func TestNotifyUpdatePromptRemind(t *testing.T) { server := newTestGitHubServer(t, "v2.0.0") defer server.Close() @@ -159,7 +122,10 @@ func TestNotifyUpdatePromptRemind(t *testing.T) { } }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + CanPrompt: true, + PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, + }, "1.0.0", testFetcher(server.URL)) assert.False(t, exit) } @@ -175,12 +141,14 @@ func TestNotifyUpdatePromptCancelled(t *testing.T) { assert.Len(t, req.Options(), 3) assert.Equal(t, "u", req.Options()[0].Key) assert.Equal(t, "r", req.Options()[1].Key) - assert.Equal(t, "s", req.Options()[2].Key) + assert.Equal(t, "n", req.Options()[2].Key) req.ResponseCh() <- output.InputResponse{Cancelled: true} } }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + CanPrompt: true, + PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, + }, "1.0.0", testFetcher(server.URL)) assert.False(t, exit) } - diff --git a/internal/update/update.go b/internal/update/update.go index fc55138d..6e83fe5a 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -27,6 +27,7 @@ package update import ( "bytes" "context" + "errors" "fmt" "strings" "sync" @@ -62,7 +63,28 @@ func Check(ctx context.Context, sink output.Sink, githubToken string) (string, b } // Update checks for updates and applies the update if one is available. -func Update(ctx context.Context, sink output.Sink, checkOnly bool, githubToken string) error { +// +// When the update would be applied, it first refuses installs it must not +// replace in place (see blockSelfUpdate) — ahead of the version check and the +// download, so an install lstk cannot write to fails immediately instead of +// after fetching and verifying a release archive it can never install. A +// --check run is exempt: reporting whether a newer version exists is useful +// however lstk was installed, and writes nothing. +func Update(ctx context.Context, sink output.Sink, checkOnly bool, githubToken string, force bool) error { + info := DetectInstallMethod() + // This pre-check and applyUpdate's own guard both call blockSelfUpdate, so + // an unwritable-directory install is probed twice per `lstk update`. That + // is deliberate: applyUpdate must stay the choke point (every path that + // replaces the binary goes through it, and a caller-supplied verdict could + // be forgotten), while this check keeps the refusal ahead of the version + // check and the download. Two ~50µs probes on a command that would + // otherwise fetch megabytes is the cheaper half of that trade. + if !checkOnly && !force { + if blocker := blockSelfUpdate(info); blocker != nil { + return emitSelfUpdateBlocked(sink, blocker) + } + } + current := version.Version() latest, available, err := Check(ctx, sink, githubToken) if err != nil { @@ -76,7 +98,10 @@ func Update(ctx context.Context, sink output.Sink, checkOnly bool, githubToken s return nil } - method, err := applyUpdate(ctx, sink, latest, githubToken) + method, blocker, err := applyUpdate(ctx, sink, latest, githubToken, force, info) + if blocker != nil { + return emitSelfUpdateBlocked(sink, blocker) + } if err != nil { sink.Emit(output.ErrorEvent{Title: err.Error(), Code: output.ErrInternal}) return output.NewSilentError(err) @@ -99,10 +124,35 @@ func warnIfBundleMissing(sink output.Sink) { }) } -// applyUpdate detects the current install method and performs the update, +// emitSelfUpdateBlocked renders a refusal to replace lstk's own binary and +// returns the silent error the caller should propagate. +func emitSelfUpdateBlocked(sink output.Sink, blocker *selfUpdateBlocker) error { + sink.Emit(output.ErrorEvent{ + Title: blocker.title(), + Summary: blocker.summary(), + Actions: []output.ErrorAction{blocker.action()}, + Code: output.ErrUpdateExternallyManaged, + }) + return output.NewSilentError(errors.New(blocker.title())) +} + +// applyUpdate performs the update for an already-detected install method, // returning its canonical name ("homebrew"/"npm"/"binary") on success. -func applyUpdate(ctx context.Context, sink output.Sink, latest, githubToken string) (string, error) { - info := DetectInstallMethod() +// +// The blockSelfUpdate check here is the choke point: every path that actually +// replaces the binary goes through this function, including the start-path +// update prompt's "Update now". Guarding only the `lstk update` entry point +// left that prompt able to clobber an externally-managed install. +// It returns a non-nil blocker instead of performing the update when the +// binary must not be replaced, leaving the caller to choose how to render it: +// `lstk update` fails with an ErrorEvent, the start-path prompt warns and +// carries on. +func applyUpdate(ctx context.Context, sink output.Sink, latest, githubToken string, force bool, info InstallInfo) (string, *selfUpdateBlocker, error) { + if !force { + if blocker := blockSelfUpdate(info); blocker != nil { + return "", blocker, nil + } + } var err error switch info.Method { @@ -118,10 +168,27 @@ func applyUpdate(ctx context.Context, sink output.Sink, latest, githubToken stri sink.Emit(output.SpinnerStop()) } if err != nil { - return "", fmt.Errorf("update failed: %w", err) + return "", nil, fmt.Errorf("update failed: %w", err) } - return info.Method.String(), nil + return appliedMethodName(info.Method), nil, nil +} + +// appliedMethodName maps an install method to the name reported in the +// UpdateAppliedEvent (and so in the --json envelope's "method" field), whose +// documented values are homebrew/npm/binary. InstallExternal reports "binary" +// because the only way it reaches here is `lstk update --force`, which performs +// exactly the binary replacement — the field says how the update happened, not +// how lstk was originally installed. +func appliedMethodName(m InstallMethod) string { + switch m { + case InstallHomebrew: + return "homebrew" + case InstallNPM: + return "npm" + default: + return "binary" + } } // logLineWriter adapts an output.Sink into an io.Writer, emitting each diff --git a/openspec/changes/add-update-check-config/design.md b/openspec/changes/add-update-check-config/design.md new file mode 100644 index 00000000..3449c5ff --- /dev/null +++ b/openspec/changes/add-update-check-config/design.md @@ -0,0 +1,126 @@ +# Design + +## Is external management reliably detectable? + +Partially — reliably enough to be a heuristic, not reliably enough to be the only mechanism. This was the open question behind the ticket's own framing ("if not, the easiest way might be to offer an option to never prompt again"), so it is settled first. + +`DetectInstallMethod` already resolves symlinks (`filepath.EvalSymlinks`), which is what makes path markers viable — `~/.nix-profile/bin/lstk` and `/opt/homebrew/bin/lstk` both resolve into their real stores. + +| Manager | Resolved path marker | Confidence | +|---|---|---| +| nix | `/nix/store/…` prefix | unambiguous | +| guix | `/gnu/store/…` prefix | unambiguous | +| mise | `mise/installs/…`, `mise/shims/…` (also legacy `rtx/…`) | high | +| asdf | `.asdf/installs/…`, `.asdf/shims/…` | high | +| scoop | `scoop/apps/…` | high | +| chocolatey | `chocolatey/lib/…` | high | + +A real mise install looks like `/Users//.local/share/mise/installs/github-localstack-lstk/latest/lstk`. + +**What it cannot catch, ever.** Distro packages (`/usr/bin/lstk` from apt/dnf/AUR), a `COPY` in someone's Dockerfile, or any manager not in the table. False negatives are permanent, which is the whole reason detection cannot be the only mechanism: the authoritative control has to be an explicit, documented setting. Hence both, not either. + +**The ordering trap.** `classifyPath` walks path segments in order and returns on the first marker it recognizes. The existing test case `/Users/someone/.local/share/mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_darwin_arm64/lstk` is npm-installed lstk under a mise-managed *node* — `mise` appears before `node_modules`, so adding mise to the same in-order walk silently reclassifies a working npm install as unmanageable. `npm install -g` is perfectly fine there. So the npm/Homebrew markers are checked across the whole path first, and the tool-manager markers only if neither matched. + +**Writability as a backstop, not a primary signal.** Probing whether the resolved executable's directory is writable catches nix, read-only containers, and a root-owned `/usr/bin` run as non-root. It does *not* catch mise, whose install directory is writable — so it complements the path markers rather than replacing them. It is used only on the explicit `lstk update` path (see below), never to pick a default. + +## Overhead, and why detection is lazy + +Detection is cheap but not free, and the naive placement — resolving the mode at the `cmd/` boundary — would pay it on every `lstk` / `lstk start`. Measured on an M5, warm page cache, each operation in isolation: + +| Step | Cost | +|---|---| +| `os.Executable()` | 5.7 ns on macOS; a `readlink("/proc/self/exe")` on Linux, ~1–2 µs | +| `EvalSymlinks` on an 8-component mise path | 18 µs | +| `EvalSymlinks` on `/usr/local/bin` | 3.6 µs | +| `classifyPath` segment walk | 377 ns | +| `unix.Access(dir, W_OK)` | 5 µs | +| temp-file writability probe | 49 µs, and it writes into the executable's directory | +| full detection on a shallow path | 7.4 µs | + +`EvalSymlinks` dominates because it `lstat`s every path component, so cost grows with install depth. + +Worst realistic case is ~25 µs, which is negligible against an HTTPS round-trip to the GitHub API under a 2 s timeout — but only if it is actually on that path. Two facts keep it there: + +1. The update notification runs only in `startEmulator` (`cmd/root.go`, `internal/ui/run.go`). Bare `lstk` and `lstk start`, nothing else — `lstk aws …`, `status`, `logs`, `stop` never reach it. +2. Detection computes only a *default*, so it is skipped whenever `LSTK_UPDATE_CHECK` or `[cli] update_check` is set, returns before anything for `off`, and is otherwise consulted only once the version check has already reported an update available. + +Net: zero added cost on every command except `lstk`/`lstk start`, and on those, cost only in the branch where an update actually exists and the user has expressed no preference. + +The writability probe is deliberately excluded from that path. Path markers alone cover the managers in the table; the probe is only worth its cost on the explicit `lstk update` refusal, where nothing is time-sensitive. That also removes the objection that lstk would write a temp file into its own install directory on every start. + +**Correction made during implementation.** This section originally argued for `unix.Access` (5 µs) over the temp-file probe (49 µs) on the update path. That reasoning does not survive its own premise: the same paragraph establishes that cost is irrelevant there, and the temp file is both more accurate (`access()` can report success for root, or under an ACL that the subsequent rename still fails) and cross-platform without a build-tagged Windows variant. The implementation uses the temp file, removes it immediately, and treats an indeterminate probe as "do not block", so an unrelated I/O error can never refuse an update that would have worked. + +## Why three modes rather than a boolean + +The two stakeholders in the ticket want different things. The reporter asked to "completely disable the update check" — no notification at all. The ticket's own suggested resolution is the opposite: keep checking and show an info label, but never block. A boolean cannot express both, and collapsing them would ship the wrong one for somebody. + +- `prompt` — today's behavior. Default, because most installs are self-managed and the prompt is how updates actually reach users. +- `notify` — one line through the sink, no `ActionChoice`, never blocks. Also the automatic default for a detected externally-managed install. +- `off` — returns before the network request. Genuinely silent, not "silent but still phoning home", because a user disabling update checks on a locked-down or air-gapped machine means the request too. + +Two overlapping booleans (`disable_update_prompt` + `disable_update_check`) were rejected: the four combinations include one that means nothing, and the names invite reading them as independent when the second subsumes the first. + +## Why the setting does not gate `lstk update` + +`lstk update` is a direct request; `update_check` describes an unsolicited interruption. Gating the explicit command on it would mean a user who set `off` has no way to update at all short of editing config back, and would make the setting's meaning depend on how it was reached. So the setting scopes to the automatic check only, and `lstk update` always runs. + +Detection is the one exception, and for a different reason: there the refusal is not about noise but about the action being wrong. It still yields to `--force`, so detection never blocks a user who knows better than the heuristic. + +## Why the refusal comes before the version check + +`Update` checks for an externally-managed install first, ahead of the `version.Version() == "dev"` early return and the network call — but only when the update would actually be applied. `lstk update --check` is exempt: reporting whether a newer version exists is useful however lstk was installed and writes nothing, so refusing it would remove information for no gain. + +Two reasons for the ordering: + +- It avoids the current nix behavior of paying for a full download and checksum verification before discovering the target is read-only. +- It makes the refusal observable end to end. Integration tests run the freshly built `bin/lstk`, whose version is `dev`, and every existing update path returns early on that — so nothing downstream of the version check can be covered by an integration test today. With the refusal first, a test can copy `bin/lstk` into a mise-shaped temporary path, run `lstk update`, and assert the message and exit code through the CLI, which is what the repo's testing rules ask for. + +## Why "Never remind me" persists `notify` rather than `off` + +The prompt option is the fix for discoverability, and its job is to stop the interruption the user just experienced — not to decide that they never want to know about a release again. `notify` does exactly what was asked and leaves a one-line trail pointing at `lstk update`; `off` is a stronger, quieter choice that belongs in a config file the user wrote deliberately, not behind a single keystroke pressed to dismiss a prompt. + +## Deliberate non-goals + +- **No new `lstk config set` surface.** `SetUpdateCheck` follows the `SetUpdateSkippedVersion` pattern it replaces and is called from the prompt path only. A general config-writing command is a separate concern. +- **No attempt to run the external manager's own update.** `mise upgrade` exists but its semantics depend on the backend the user configured, and nix has no single equivalent. Naming the manager and the resolved path is honest and actionable; guessing a command is neither. +- **No rewriting of existing config files** to add the new commented block. `default_config.toml` is only ever written on first run, and CLAUDE.md is explicit that only a real emulator start may create it. + +## Added during implementation + +**The note names the manager.** The `notify`-mode line ends in "(run lstk update)", which is wrong advice on an externally-managed install — that command now refuses. When detection is what downgraded the mode, the note names the manager instead ("installed via mise — update it there"). A note reached any other way keeps pointing at `lstk update`. + +**`method` in the JSON envelope stays a closed enum.** `applyUpdate` reported `InstallMethod.String()`, so a `--force` update on an external install would have emitted `"method": "external"` — a new value in a field documented as homebrew/npm/binary. The field describes how the update was performed, and `--force` performs a binary replacement, so `appliedMethodName` maps it to `binary`. + +## Corrections after adversarial review + +Two defects in the first implementation defeated the feature's purpose; both are now covered by tests that were mutation-checked. + +**The prompt's "Update now" bypassed the guard.** `blockSelfUpdate` was wired only into `Update()`, while `promptAndUpdate` called `applyUpdate` directly — and `applyUpdate`'s switch falls through to the binary updater for `InstallExternal`. With an explicit `update_check = "prompt"`, detection was skipped, the prompt appeared on a mise install, and pressing `u` overwrote the mise-owned binary: verbatim the motivating bug. Fixed twice over: the guard moved into `applyUpdate` (the single choke point every replacing path goes through), and an externally-managed install is now never prompted at all, even under an explicit `prompt`. + +**"Never ask again" persisted nothing on a first run.** `config.Set` succeeds in memory only when no config file has been resolved, and the interactive path notifies before the emulator picker creates the file — so the user was told the preference was saved, nothing was written, and the next start prompted again. Rather than adding a fourth `EnsureCreated` caller (CLAUDE.md enumerates exactly three, and eager creation would rob the emulator picker of its first run), the option is now offered only when a config file exists, and `SetUpdateCheck` fails loudly instead of silently succeeding. + +**Detection is no longer skipped for explicitly-set modes.** The original "explicit mode skips detection" rule produced a note advising `lstk update` on installs where that command refuses — including on every non-interactive start, which short-circuited before detection entirely. Detection now runs once an update is known to exist, for every mode except `off`. The performance intent is unchanged: it stays off every start where no update exists, which is the overwhelming majority. + +**Marker coverage was wrong for the paths actually on `PATH`.** `.asdf` missed the `ASDF_DATA_DIR` layout (`~/.local/share/asdf`), and scoop and chocolatey were matched only at their install roots, not at the `shims`/`bin` launcher directories that are what `PATH` actually points at — and whose entries are not symlinks, so `EvalSymlinks` does not rewrite them. Each miss silently clobbered a managed install. + +## Why "Skip this version" is removed rather than kept + +The prompt would otherwise offer four options, three of which mean "no": remind me later, skip this one, never ask. They are genuinely distinct, but the middle one earns the least: + +- It is the option the ticket says does not work. Against a weekly cadence, "skip until the next version" buys days. +- It is the only per-version persisted state in the feature, and that state suppressed output in *every* mode — so a stale skip silenced the note `notify` mode promises, for exactly the release the user was most likely to see next. Closing that trap needed "Never ask again" to clear the skipped version, i.e. extra machinery whose only purpose was to undo the option being removed. +- "Remind me next time" already covers "not now", and the permanent opt-out covers "stop". + +Removing it drops `cli.update_skipped_version`, `config.SetUpdateSkippedVersion`, `NotifyOptions.SkippedVersion`, `NotifyOptions.PersistSkipVersion`, and the clearing logic. A leftover `update_skipped_version` key in an existing config becomes inert rather than an error — viper ignores unknown keys, so no migration is needed. Someone mid-skip gets prompted once more for the version they skipped, and can then say "never" instead, which is what they wanted. + +This also means nothing in automation is affected: `lstk start --non-interactive` never prompts — it emits a note and continues — so no scripted or toolkit flow depends on the prompt's option set. + +`internal/config/config_test.go` still uses `cli.update_skipped_version` as the fixture key for `setInFile`, which is a generic "write one section.field" helper. Those tests are unchanged deliberately: the key is arbitrary test data there, and rewriting a dozen pre-existing assertions would be churn unrelated to this change. + +## Open question for review: where the update prompt sits in the start flow + +`internal/ui/run.go` calls `NotifyUpdate` as the first action of the start goroutine — ahead of the Docker health check, the auth flow, and the emulator picker. One consequence is that "Never ask again" cannot be offered on a genuine first run: `config.toml` does not exist yet (the picker creates it), so the preference would have nowhere to go, and the option is therefore hidden there. + +Moving the notification after the picker would let the option appear on every run. **It is deliberately not moved**, because prompting early is worth more than that: a user on an old or broken CLI should be offered the update before the CLI attempts real work, which matters for both stability and usability. A late prompt would also be preempted by a Docker failure, i.e. exactly the situation where updating might be the fix. + +The residual gap is narrow — a first run means no config, which almost always means a fresh install already on the latest version, so there is usually nothing to prompt about. Raised here as an open question for the PR rather than settled unilaterally. diff --git a/openspec/changes/add-update-check-config/proposal.md b/openspec/changes/add-update-check-config/proposal.md new file mode 100644 index 00000000..ba8f8f4f --- /dev/null +++ b/openspec/changes/add-update-check-config/proposal.md @@ -0,0 +1,36 @@ +## Why + +`lstk` has no way to stop asking about updates. The only escape hatch today is "Skip this version" (`cli.update_skipped_version`), which the weekly release cadence invalidates within days — so a user on a fast-moving channel is prompted again almost every day (DEVX-1029, reported from Slack). That option is removed here rather than kept alongside the new one: it bought days, not quiet, and left the prompt with three ways to say "no". + +The friction is worst where lstk is not responsible for its own installation. When lstk is installed by `mise`, `nix`, or `asdf`, updates are already governed by that tool's policy — a lockfile, a flake, a deliberately pinned version — and self-updating is not just unwanted but actively wrong. Two concrete failures exist today, both reachable from the current prompt: + +- **nix:** the store path is read-only. `lstk update` downloads the release archive, verifies its SHA-256, and only then fails when `os.Rename` into `/nix/store` returns `EROFS` (`internal/update/extract.go:56-66`) and the `copyFile` fallback fails too. The user pays the full download for a guaranteed failure and a confusing error. +- **mise:** the install directory *is* writable, so the update **succeeds** — silently replacing the binary that mise has recorded a version for, desynchronizing lstk from the tool that manages it. This is exactly the "I don't want lstk to meddle with it" complaint in the report. + +So two things are missing: an explicit, permanent opt-out the user can set once, and enough awareness of externally-managed installs that lstk stops offering an action that cannot work. + +## What Changes + +- Add a `[cli] update_check` config setting with three values — `prompt` (today's blocking choice, the default), `notify` (a one-line note, never blocking), and `off` (no check at all: no network request, no output). The corresponding environment variable is `LSTK_UPDATE_CHECK`. +- Resolution order: `LSTK_UPDATE_CHECK` > `[cli] update_check` > a detected externally-managed install (defaults to `notify`) > `prompt`. +- The setting governs **only** the automatic check on the start path. An explicit `lstk update` / `lstk update --check` always checks and applies regardless of the setting — it is a direct request, not a background nag. +- Replace the prompt's "Skip this version" option with `n` — "Never ask again", which persists `cli.update_check = "notify"`. This is the discoverability half of the fix; the reported problem was not that the prompt exists but that there is no way to say "stop" from where it appears. The prompt keeps three choices rather than gaining a fourth, and `cli.update_skipped_version` and its setter are removed. +- Detect externally-managed installs (`nix`, `guix`, `mise`, `asdf`, `scoop`, `chocolatey`) from the resolved executable path, and use that both to pick the quieter default above and to make `lstk update` refuse rather than clobber — naming the manager and the resolved path instead of attempting an update. `lstk update --force` overrides the refusal, so a false positive is never a dead end. +- Detection is lazy and off the hot path: it only computes a *default*, so an explicit setting skips it entirely, `off` returns before it, and it is otherwise consulted only after the version check has already reported an update available. See design.md for the measured costs. + +## Capabilities + +### New Capabilities +- `update-check-config`: the `[cli] update_check` setting and `LSTK_UPDATE_CHECK` variable, the three modes and their exact output, the resolution order, the scope limit to the automatic check, and the "Never remind me" prompt option that persists the setting. +- `external-install-detection`: the path markers that identify an externally-managed install, their precedence relative to the existing Homebrew/npm classification, the `lstk update` refusal and its `--force` override, and the requirement that detection never runs on invocations that do not reach the update check. + +## Impact + +- `internal/update/notify.go`: `NotifyOptions` gains a resolved mode instead of the `UpdatePrompt bool`; `notifyUpdateWithVersion` returns before the network call for `off`, and consults detection only after `checkQuietlyWithVersion` reports an update. `promptAndUpdate` gains the `n` option. +- `internal/update/install_method.go`: `InstallMethod` gains `InstallExternal`; `InstallInfo` gains a `Manager` string. `classifyPath` is reordered so `node_modules`/`Caskroom` win over the tool-manager markers — the existing test case `…/mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_darwin_arm64/lstk` (npm-installed lstk under a mise-managed *node*) must stay `InstallNPM`, and the current in-order segment walk would misclassify it. +- `internal/update/update.go`: `Update` checks for an externally-managed install before the version check (which also makes the refusal observable end-to-end without a non-`dev` build) and refuses unless `--force`. A writability probe of the resolved executable's directory backs up the path markers on this path only. +- `internal/config/config.go`: `CLIConfig` gains `UpdateCheck` and loses `UpdateSkippedVersion`; a `SetUpdateCheck` setter replaces `SetUpdateSkippedVersion` (same surgical line rewrite); an invalid value is rejected in `Get()` alongside the container validation. +- `internal/env/env.go`: `Env` gains `UpdateCheck`, read in `Init()` — it must be captured there because `config.loadConfig` calls `viper.Reset()`. +- `cmd/root.go`: resolves the mode from env + `appConfig.CLI` at the command boundary and passes it into `NotifyOptions`; `cmd/update.go` gains `--force`. +- `internal/output/error_code.go`: a new `UPDATE_EXTERNALLY_MANAGED` code for the refusal, plus its `retryable`/`category` classification and a row in `docs/structured-output.md`. +- `internal/config/default_config.toml`: a commented `[cli] update_check` block. Note this only reaches users whose config is created after this change — existing files are never rewritten — so `lstk docs` and the prompt option are the discovery paths for everyone else. diff --git a/openspec/changes/add-update-check-config/specs/external-install-detection/spec.md b/openspec/changes/add-update-check-config/specs/external-install-detection/spec.md new file mode 100644 index 00000000..c9754daf --- /dev/null +++ b/openspec/changes/add-update-check-config/specs/external-install-detection/spec.md @@ -0,0 +1,84 @@ +## ADDED Requirements + +### Requirement: lstk recognizes an externally-managed install +lstk SHALL classify its own install as externally managed when the resolved path of the running executable (after symlink resolution) matches a known tool-manager or immutable-store marker, and SHALL record which manager was recognized. The recognized managers are nix, guix, mise, asdf, scoop, and chocolatey. + +The existing Homebrew and npm classifications SHALL take precedence: a path matching an npm or Homebrew marker anywhere SHALL classify as npm or Homebrew respectively, even when a tool-manager marker also appears in the path. This is required because a Homebrew- or npm-installed lstk may live under a tool-manager-provisioned interpreter, and both of those install methods can still update themselves correctly. + +#### Scenario: A mise install is recognized +- **WHEN** the resolved executable path is `/home/user/.local/share/mise/installs/github-localstack-lstk/latest/lstk` +- **THEN** the install is classified as externally managed by mise + +#### Scenario: A nix install is recognized +- **WHEN** the resolved executable path is under `/nix/store/` +- **THEN** the install is classified as externally managed by nix + +#### Scenario: npm under a tool-managed interpreter stays npm +- **WHEN** the resolved executable path is `/home/user/.local/share/mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_linux_amd64/lstk` +- **THEN** the install is classified as npm, not as externally managed + +#### Scenario: An ordinary install is not externally managed +- **WHEN** the resolved executable path is `/usr/local/bin/lstk` or `/home/user/bin/lstk` +- **THEN** the install is classified as a standalone binary + +### Requirement: Every path that replaces the binary is guarded +The refusal SHALL be enforced at the point the binary is actually replaced, not only at the `lstk update` entry point. In particular the start-path update prompt's "Update now" SHALL be subject to it, so that no combination of settings can reach an in-place replacement of an externally-managed install without `--force`. + +#### Scenario: The start-path prompt cannot clobber an externally-managed install +- **GIVEN** lstk is running from a path recognized as managed by mise +- **AND** `[cli] update_check = "prompt"` is set explicitly +- **WHEN** `lstk start` runs interactively with a newer version available +- **THEN** no update prompt is presented +- **AND** a note naming mise is emitted instead + +### Requirement: An externally-managed install is not updated in place +`lstk update` SHALL determine whether the install is externally managed before performing any version check or download, and SHALL refuse to update when it is — reporting the recognized manager and the resolved executable path, and stating that the update must go through that manager. The refusal SHALL exit non-zero and, under `--json`, SHALL carry the error code `UPDATE_EXTERNALLY_MANAGED`. + +`lstk update --check` SHALL be exempt from the refusal: it reports whether a newer version exists and writes nothing, which is useful however lstk was installed. + +lstk SHALL also refuse an in-place binary replacement when the resolved executable's directory cannot be written to, under the same error code, reporting the directory instead of a manager. Homebrew and npm installs SHALL never be refused on this basis, since they delegate to `brew upgrade` and `npm install -g` rather than writing the file themselves. A probe that cannot determine writability SHALL NOT refuse. + +`lstk update --force` SHALL bypass the refusal and update as if the install were a standalone binary, so that a misidentified install is never left without a path forward. + +#### Scenario: Update refuses on an externally-managed install +- **GIVEN** lstk is running from a path recognized as managed by mise +- **WHEN** `lstk update` is run +- **THEN** lstk exits non-zero naming mise and the resolved executable path +- **AND** no release archive is downloaded and the executable is not replaced + +#### Scenario: The refusal is machine-readable +- **GIVEN** lstk is running from a path recognized as managed by nix +- **WHEN** `lstk update --json` is run +- **THEN** the envelope reports `"status": "error"` with `"error": {"code": "UPDATE_EXTERNALLY_MANAGED", ...}` + +#### Scenario: --check is not refused +- **GIVEN** lstk is running from a path recognized as managed by mise +- **WHEN** `lstk update --check` is run +- **THEN** the version check is performed and its result reported, with no refusal + +#### Scenario: A read-only install directory is refused +- **GIVEN** lstk is running as a standalone binary from a directory it cannot write to +- **WHEN** `lstk update` is run +- **THEN** lstk exits non-zero naming that directory +- **AND** no release archive is downloaded + +#### Scenario: --force overrides the refusal +- **GIVEN** lstk is running from a path recognized as managed by mise +- **WHEN** `lstk update --force` is run +- **THEN** the version check and update proceed as they would for a standalone binary + +### Requirement: Detection does not run on invocations that do not check for updates +Install-method detection SHALL NOT be performed on any code path that does not otherwise reach the automatic update check or the update command. + +Within the **automatic start-path check** it SHALL NOT run when the resolved mode is `off`, or before the version check has reported that a newer version is available. This scoping is deliberate: `lstk update` detects unconditionally and up front, because its refusal must precede the version check and the download (see the requirement below), and that is a different code path with a different cost profile. + +It SHALL run once an update *is* known to exist, whatever the mode (other than `off`) and whether or not the call site can prompt: its answer determines the note's wording as well as the prompt/note decision, so skipping it for an explicitly-set mode produced a note advising `lstk update` on an install where that command refuses. + +#### Scenario: off never reaches detection +- **GIVEN** `[cli] update_check = "off"` +- **WHEN** `lstk start` runs +- **THEN** no version check is performed and install-method detection is not consulted + +#### Scenario: Other commands never detect +- **WHEN** any command other than `lstk`, `lstk start`, or `lstk update` is run +- **THEN** install-method detection is not performed diff --git a/openspec/changes/add-update-check-config/specs/update-check-config/spec.md b/openspec/changes/add-update-check-config/specs/update-check-config/spec.md new file mode 100644 index 00000000..e34a8ff0 --- /dev/null +++ b/openspec/changes/add-update-check-config/specs/update-check-config/spec.md @@ -0,0 +1,77 @@ +## ADDED Requirements + +### Requirement: A persistent setting controls the automatic update check +lstk SHALL read an update-check mode from the `[cli] update_check` key in `config.toml` and from the `LSTK_UPDATE_CHECK` environment variable, accepting exactly the values `prompt`, `notify`, and `off`. The mode SHALL govern the automatic update check performed on the start path, and SHALL NOT govern an explicit `lstk update` invocation. + +Resolution order, first match wins: `LSTK_UPDATE_CHECK`, then `[cli] update_check`, then `notify` when the install is detected as externally managed (see the `external-install-detection` capability), then `prompt`. + +Behavior per mode: + +- `prompt` — lstk checks for a newer version and, on an interactive start, presents the blocking update choice. +- `notify` — lstk checks for a newer version and emits a single non-blocking note naming the current and latest version and how to update. No prompt is presented and nothing waits for input. When the install is externally managed, the note SHALL name that manager rather than advising `lstk update`, which refuses on such an install — regardless of how the mode was reached, since the advice is equally wrong either way. + +An externally-managed install SHALL NOT be prompted even under `prompt`: applying the update is refused on such an install, and offering an action that cannot be carried out is worse than not offering it. `prompt` therefore behaves as `notify` there. + +- `off` — lstk performs no version check at all: no network request is made and no update-related output is emitted. + +An unrecognized value SHALL be rejected as a configuration error naming the key and the accepted values, rather than silently falling back to a default. + +#### Scenario: Default behavior is unchanged +- **WHEN** neither `LSTK_UPDATE_CHECK` nor `[cli] update_check` is set and the install is not detected as externally managed +- **THEN** the resolved mode is `prompt` +- **AND** an interactive `lstk start` with a newer version available presents the blocking update choice, as before this change + +#### Scenario: notify never blocks +- **GIVEN** `[cli] update_check = "notify"` +- **WHEN** `lstk start` runs interactively and a newer version is available +- **THEN** a single note naming both versions is emitted +- **AND** no prompt is presented and the start proceeds without waiting for input + +#### Scenario: off makes no network request +- **GIVEN** `[cli] update_check = "off"` +- **WHEN** `lstk start` runs +- **THEN** no request is made to the release API +- **AND** no update-related output is emitted in either interactive or non-interactive mode + +#### Scenario: Environment variable overrides config +- **GIVEN** `[cli] update_check = "off"` in `config.toml` +- **WHEN** `lstk start` runs with `LSTK_UPDATE_CHECK=prompt` +- **THEN** the resolved mode is `prompt` + +#### Scenario: Invalid value is rejected +- **WHEN** `lstk start` runs with `[cli] update_check = "quiet"` +- **THEN** lstk exits non-zero with a configuration error naming `update_check` and the values `prompt`, `notify`, `off` +- **AND** the emulator is not started + +#### Scenario: The setting does not disable the explicit update command +- **GIVEN** `[cli] update_check = "off"` +- **WHEN** `lstk update --check` is run +- **THEN** the version check is performed and its result reported as usual + +### Requirement: The update prompt offers exactly three choices +The blocking update prompt SHALL offer "Update now", "Remind me next time", and "Never ask again" — and no per-version "Skip this version" option. A skipped version bought a few days of quiet against a weekly release cadence (the complaint behind DEVX-1029) while adding a third flavour of "no" to the prompt and the only piece of per-version persisted state; the permanent opt-out serves the same need without either cost. `cli.update_skipped_version` is removed with it. + +"Never ask again" SHALL be offered **only when the setting can actually be persisted** — i.e. when a config file exists. On a first run config.toml has not been created yet (the emulator picker creates it later), and an option whose effect would be silently dropped SHALL NOT be offered. + +Selecting it SHALL persist `cli.update_check = "notify"` to the config file in use, preserving the file's existing comments and formatting, and SHALL NOT apply an update. + +A failure to persist SHALL be surfaced as a warning and SHALL NOT be reported as success. + +#### Scenario: The opt-out is not offered when it cannot be persisted +- **GIVEN** no config file exists yet (a first run) +- **WHEN** the update prompt is presented +- **THEN** it offers only "Update now" and "Remind me next time" +- **AND** no option claims a preference was saved + +#### Scenario: Never ask again persists the setting +- **GIVEN** an interactive `lstk start` with a newer version available and the mode resolved to `prompt` +- **WHEN** the user selects "Never ask again" +- **THEN** `cli.update_check` is written as `notify` to the config file reported by `lstk config path` +- **AND** no update is applied +- **AND** a subsequent `lstk start` with a newer version available emits a note instead of a prompt + +#### Scenario: Persisting the opt-out fails +- **GIVEN** the config file cannot be written +- **WHEN** the user selects "Never ask again" +- **THEN** a warning is emitted naming the failure +- **AND** the command continues and exits as it otherwise would diff --git a/openspec/changes/add-update-check-config/tasks.md b/openspec/changes/add-update-check-config/tasks.md new file mode 100644 index 00000000..d5034a8f --- /dev/null +++ b/openspec/changes/add-update-check-config/tasks.md @@ -0,0 +1,89 @@ +## 1. Config and environment plumbing + +- [x] 1.1 Add `UpdateCheck string` to `config.CLIConfig` (`mapstructure:"update_check"`), plus an `UpdateCheckMode` string type with the `prompt`/`notify`/`off` constants and a parser. +- [x] 1.2 Reject an unrecognized `update_check` value in `config.Get()`, alongside the existing container and named-env validation, with an error naming the key and the accepted values. +- [x] 1.3 Add `config.SetUpdateCheck(mode string) error` in place of `SetUpdateSkippedVersion` (delegating to `Set("cli.update_check", …)`, same surgical line rewrite). +- [x] 1.4 Add `UpdateCheck string` to `env.Env`, read in `env.Init()` via `viper.GetString("update_check")` — it must be captured there, since `config.loadConfig` calls `viper.Reset()`. +- [x] 1.5 Document the setting as a commented `[cli] update_check` block in `internal/config/default_config.toml`. +- [x] 1.6 Unit tests: value parsing (each valid value, an invalid value, empty), and `SetUpdateCheck` preserving surrounding comments and formatting on a file with and without an existing `[cli]` section. + +## 2. External-install detection + +Write the tests in this section before the implementation — 2.1's reordering is a regression risk to an existing passing test case. + +- [x] 2.1 Unit tests for `classifyPath`: each manager in the design.md table resolves to `InstallExternal` with the right `Manager`; the existing `…/mise/installs/node/…/node_modules/@localstack/lstk_darwin_arm64/lstk` case still resolves to `InstallNPM`; `/usr/local/bin/lstk` and `/home/user/bin/lstk` still resolve to `InstallBinary`. +- [x] 2.2 Add `InstallExternal` to `InstallMethod` (with its `String()` case) and a `Manager string` field to `InstallInfo`. +- [x] 2.3 Reorder `classifyPath`: scan the whole path for the npm/Homebrew markers first, then for the tool-manager markers, so npm and Homebrew always win. Cover nix (`/nix/store/` prefix), guix (`/gnu/store/`), mise (`mise/installs`, `mise/shims`, legacy `rtx`), asdf (`.asdf/installs`, `.asdf/shims`), scoop (`scoop/apps`), chocolatey (`chocolatey/lib`). +- [x] 2.4 Add a writability probe for the resolved executable's directory, used only from the `lstk update` path — never from the mode-default path. Implemented as a create-and-remove temp file rather than `unix.Access`: see design.md's "Correction made during implementation" (more accurate, cross-platform without build tags, and cost is irrelevant on that path). An indeterminate probe does not block. + +## 3. Notification modes + +- [x] 3.1 Replace `NotifyOptions.UpdatePrompt bool` with the resolved mode. Return before `checkQuietlyWithVersion` for `off`; keep the existing note for `notify`; keep `promptAndUpdate` for `prompt`. +- [x] 3.2 Consult detection only after the version check reports an update available and only when no explicit mode was supplied, downgrading `prompt` to `notify` for an externally-managed install. Pass detection in as a function on `NotifyOptions` so the unit tests do not depend on the test binary's own path. +- [x] 3.3 Add the `n` — "Never remind me" option to the `ActionChoice` in `promptAndUpdate`, persisting via a `PersistUpdateCheck` callback on `NotifyOptions`; emit a warning (not an error) when persisting fails, matching the existing skip-version behavior. +- [x] 3.4 Resolve the mode at the command boundary in `cmd/root.go` from `cfg.UpdateCheck` and `appConfig.CLI.UpdateCheck`, and wire `config.SetUpdateCheck` as `PersistUpdateCheck`. Both the interactive (`ui.Run`) and non-interactive `NotifyUpdate` call sites must pass the resolved mode — today the non-interactive one passes only `GitHubToken`. +- [x] 3.5 Unit tests: each mode's outcome (no fetch for `off`; note only for `notify`; prompt for `prompt`); detection downgrading `prompt` to `notify`; an explicit mode skipping detection; the `n` option persisting `notify` and applying no update; a persist failure warning but not failing. + +## 4. Update command + +- [x] 4.1 Integration test (before implementation): copy the built `bin/lstk` into a mise-shaped temporary path, run `lstk update` with an isolated `HOME`, and assert the non-zero exit and the message naming mise and the path. This works despite the test binary's `dev` version only because the refusal precedes the version check. +- [x] 4.2 Add `UPDATE_EXTERNALLY_MANAGED` to `output.ErrorCode`, with its `retryable: false` classification and category, and a row in `docs/structured-output.md`'s error table. +- [x] 4.3 Add the refusal to `update.Update` ahead of the `dev`-version early return and the version check: emit an `ErrorEvent` with the new code, the manager, the resolved path, and an action pointing at the manager; return a silent error. +- [x] 4.4 Add `--force` to `cmd/update.go` and thread it through to bypass the refusal; document on the flag that it exists because detection is a heuristic. +- [x] 4.5 Integration tests: `lstk update --force` from the same mise-shaped path proceeds past the refusal; `lstk update --json` from a nix-shaped path emits the envelope with `UPDATE_EXTERNALLY_MANAGED`. + +## 5. Documentation + +- [x] 5.1 Update `cmd/update.go`'s `Long` to state that `update_check` does not gate the explicit command, and to describe `--force`. +- [x] 5.2 Add the `update_check` resolution order and the detection behavior to CLAUDE.md only if the guidance spans packages; otherwise put it in doc comments on `UpdateCheckMode`, `classifyPath`, and the `--force` flag, per CLAUDE.md's "Maintaining This File" rules. +- [x] 5.3 Run `make test`, `make test-integration`, and `make lint`. + +## 6. Discovered during implementation + +- [x] 6.1 The `notify` note ends in "(run lstk update)", which refuses on an externally-managed install. When detection forced the downgrade, name the manager instead ("installed via mise — update it there"); notes reached any other way keep pointing at `lstk update`. +- [x] 6.2 `applyUpdate` returned `InstallMethod.String()`, so `lstk update --force` on an external install would emit `"method": "external"` — a value outside the documented homebrew/npm/binary enum. Added `appliedMethodName`, mapping InstallExternal to `binary` (the field says how the update happened, and --force performs a binary replacement). +- [x] 6.3 `lstk update --check` exempted from the refusal: it writes nothing and its answer is useful however lstk was installed. +- [x] 6.4 `internal/output`'s `TestErrorCode_AllErrorCodesIsComplete` hardcodes the code count; bumped 34 → 35 alongside the new code. `docs/structured-output.md` still says "the 29 codes above" in its category section, which was already stale before this change and is left alone. + +## 7. Adversarial-review fixes + +- [x] 7.1 BLOCKER: move the `blockSelfUpdate` guard into `applyUpdate` so the start-path prompt's "Update now" cannot replace an externally-managed binary, and stop prompting on such installs entirely (even under an explicit `prompt`). +- [x] 7.2 BLOCKER: offer "Never ask again" only when a config file exists, and make `config.SetUpdateCheck` fail rather than succeed in memory only — it previously told the user the preference was saved on a first run and wrote nothing. +- [x] 7.3 Run detection for every mode except `off` once an update is known to exist, so the note names the manager on the non-interactive path and under an explicit `notify` too. +- [x] 7.4 Classify the new config failures: `failGetConfig` for `config.Get` in `startEmulator`, and an `ErrorEvent{Code: ErrConfigInvalid}` for a bad `LSTK_UPDATE_CHECK` (both previously surfaced as `INTERNAL_ERROR` under `--json`). +- [x] 7.5 Add the missing markers: bare `asdf` (ASDF_DATA_DIR layout), `scoop/shims`, `chocolatey/bin` — the launcher directories actually on PATH. +- [x] 7.6 Guard `blockSelfUpdate` against an empty `ResolvedPath` (`filepath.Dir("")` is `"."`, so it probed the working directory), and add the `blockSelfUpdate` unit tests that were missing entirely. +- [x] 7.7 ~~Clear `cli.update_skipped_version` when persisting the opt-out~~ — superseded by section 9, which removes the skipped-version mechanism entirely. +- [x] 7.8 Add the end-to-end coverage that was missing for the headline behavior — `off` making no request, `notify` emitting a note, the env var overriding config, the note naming the manager, and both invalid-value paths reporting `CONFIG_INVALID` — using the existing `LSTK_UPDATE_GITHUB_API_ENDPOINT` mock-server hook and the ldflags version stamp. My earlier claim that this was impractical because the test binary reports `dev` was wrong; `test/integration/update_test.go` already does exactly this. +- [x] 7.9 Test-quality fixes: drop the vacuous `NotContains("UPDATE_EXTERNALLY_MANAGED")` assertion (codes never render outside JSON); stop using the exempt `--check` in the test that claims to exercise the guard; document the no-ldflags dependency in the `--force` test; skip the two `0500`-directory tests as root; clear `LSTK_UPDATE_CHECK` in the env test instead of reading the developer's environment. +- [x] 7.10 Delete `InstallMethod.String()`, dead since `appliedMethodName` took its only caller, along with the `TestInstallMethodStringIncludesExternal` case added earlier in this same changeset (which asserted the switch's own literal). +- [x] 7.11 Docs: add `guix` to the help text and `docs/structured-output.md`; describe both refusal reasons on `--force` and the `--check` exemption; fix the stale "29 codes" count (now 35) in the block this change already edits; add the new code to `json-output-schema`'s `error-codes` spec table; drop the commented `update_check` example from the config template, which `setInFile` would otherwise contradict by inserting the live key above it; trim the two CLAUDE.md paragraphs that restated doc comments added in this same changeset; add `LSTK_UPDATE_CHECK` to CLAUDE.md's environment-variable list. + +### Deliberately not changed + +- The `n` key for "Never ask again" is kept, with the label sharpened from "Never remind me". A reflexive `n` (meaning "no") does write config, but the write is non-destructive, reversible, and close to what someone dismissing a repeated prompt wants; `r` remains the true decline. +- A skipped version still suppresses output in `notify` mode. Skipping a version is an explicit per-version silence request, so honoring it in every mode is consistent; the stale-state trap it created is fixed by 7.7 instead. + +## 8. Second-round adversarial review fixes + +- [x] 8.1 MAJOR: the round-1 blocker fix had no test — deleting `applyUpdate`'s `blockSelfUpdate` guard left the whole suite green, because `promptAndUpdate` called the un-injectable `DetectInstallMethod()`. Replaced `NotifyOptions.DetectExternal` with a single injectable `DetectInstall func() InstallInfo` (also removing the redundant second detection), and added `TestPromptUpdateNowIsRefusedWhenTheBinaryCannotBeReplaced`, mutation-verified to fail without the guard. +- [x] 8.2 `applyUpdate` now returns the blocker instead of emitting an `ErrorEvent` itself, so each caller chooses the severity: `lstk update` fails with the error event, the start-path prompt emits one warning. Previously pressing "Update now" on a read-only install dir left a permanent red failure block on screen (ErrorEvent sets `hideHeader` and persists) *plus* a duplicate "Update failed" warning, while the emulator started underneath it. Covered by `TestPromptRefusalEmitsNoErrorEvent`, also mutation-verified. +- [x] 8.3 Cover the two untested halves of 7.2: `config.SetUpdateCheck`'s no-file error (unit) and the `config.HasFile()` gate in `cmd/root.go` (a PTY integration test asserting the option is absent on a first run and present when config exists). Both mutation-verified. +- [x] 8.4 Nil-guard `case "n"`'s `PersistUpdateCheck` call rather than relying on an invariant established 40 lines earlier. +- [x] 8.5 A partial failure on "n" (mode saved, skipped version not cleared) no longer reports success — it warns and names the consequence and the key to clear. +- [x] 8.6 Scope the "detection does not run" spec requirement to the automatic start-path check; it contradicted the requirement that `lstk update` detect before the version check. +- [x] 8.7 Strengthen `TestNotifyUpdateNeverRemindPersistsNotifyAndAppliesNoUpdate` (the "applies no update" half rested only on `exit == false`); make the `off` tests answer prompts so a regression fails an assertion instead of deadlocking the suite. +- [x] 8.8 Nits: fix the import grouping in `notify_test.go`. The `[cli]` comment block was first moved above its header (lstk inserts written keys directly below the header, so the docs ended up beneath the value), then reduced on review to the file's own house style — a single commented `update_check` line with an inline comment, matching how `[[containers]]` documents its keys. The written value does land above that line; `TestSetUpdateCheckOnTheShippedTemplate` pins that the result is still valid TOML that reads back correctly, which is what actually matters. + +### Deliberately not changed (second round) + +- **`lstk update` probes writability twice** (the pre-check and `applyUpdate`'s guard). Keeping `applyUpdate` as the choke point is worth more than one saved ~50µs probe, and the pre-check is what keeps the refusal ahead of the download. Documented at the call site. +- **An invalid `LSTK_UPDATE_CHECK` is only rejected by `lstk start`.** Making it symmetric with the config key needs `env.Env` threaded through `initConfigDeferCreate`'s every call site, and full symmetry is unreachable anyway (`version` and `--help` never load config). The variable only affects the start path, and the start path validates it. +- **The first interactive run offers no opt-out** (no config file exists yet to write to). Correct per 7.2, and the run after it offers it. + +## 9. Drop "Skip this version" + +- [x] 9.1 Remove the `s` option and its handler from the prompt, leaving exactly three choices (`u`/`r`/`n`, with `n` still conditional on a writable config). +- [x] 9.2 Remove the mechanism behind it: `NotifyOptions.SkippedVersion`, `NotifyOptions.PersistSkipVersion`, the suppression check, `CLIConfig.UpdateSkippedVersion`, `config.SetUpdateSkippedVersion`, and the clearing logic added by 7.7. A leftover key in an existing config is inert (viper ignores unknown keys), so no migration is needed. +- [x] 9.3 Retarget the pre-existing `TestUpdateNotification` "skip" subtest at the surviving config-writing option and rename it — it tests that a prompt-driven write preserves the user's comments and formatting, not which preference is written. +- [x] 9.4 Update the specs, proposal, and design rationale; leave `internal/config/config_test.go`'s use of the key as generic `setInFile` fixture data alone. diff --git a/openspec/changes/json-output-schema/specs/error-codes/spec.md b/openspec/changes/json-output-schema/specs/error-codes/spec.md index e7551f8c..189a2efa 100644 --- a/openspec/changes/json-output-schema/specs/error-codes/spec.md +++ b/openspec/changes/json-output-schema/specs/error-codes/spec.md @@ -33,6 +33,7 @@ Every `error.code` value emitted in a JSON envelope SHALL be one of a fixed, doc | `USAGE_ERROR` | Cobra-level flag or argument parsing failed | No | `USAGE` | | `NOT_JSON_CAPABLE` | The requested command has not been annotated as JSON-capable | No | `USAGE` | | `NETWORK_ERROR` | An unclassified network/transport failure occurred | Yes | `RUNTIME` | +| `UPDATE_EXTERNALLY_MANAGED` | `lstk update` refused to replace a binary it does not own (added by the `add-update-check-config` change) | No | `RUNTIME` | | `CANCELLED` | The operation was interrupted (e.g. context cancellation via Ctrl+C) | Yes | `INTERNAL` | | `INTERNAL_ERROR` | Unclassified or unexpected failure; the universal fallback | No | `INTERNAL` | | `IAC_FILE_NOT_FOUND` | A required infrastructure-as-code file or directory does not exist or cannot be read (e.g. the workspace `lstk deploy detect --dir` was pointed at) | No | `IAC` | diff --git a/test/integration/update_check_test.go b/test/integration/update_check_test.go new file mode 100644 index 00000000..7fb4e146 --- /dev/null +++ b/test/integration/update_check_test.go @@ -0,0 +1,357 @@ +package integration_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "runtime" + "sync/atomic" + "testing" + + "github.com/localstack/lstk/test/integration/env" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// installLstkUnder copies the built lstk binary into a directory laid out the +// way the named tool manager installs it, and returns the copied binary's path. +// Running that copy is what makes the externally-managed refusal observable +// through the CLI, since detection reads the resolved path of the running +// executable. +func installLstkUnder(t *testing.T, layout string) string { + t.Helper() + + src, err := filepath.Abs(binaryPath()) + require.NoError(t, err) + data, err := os.ReadFile(src) + require.NoError(t, err, "run `make build` first") + + dir := filepath.Join(t.TempDir(), filepath.FromSlash(layout)) + require.NoError(t, os.MkdirAll(dir, 0755)) + + name := "lstk" + if runtime.GOOS == "windows" { + name = "lstk.exe" + } + dst := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(dst, data, 0755)) + return dst +} + +func TestUpdateRefusesOnMiseManagedInstall(t *testing.T) { + t.Parallel() + + bin := installLstkUnder(t, ".local/share/mise/installs/github-localstack-lstk/latest") + stdout, stderr, err := runBinary(t, t.TempDir(), testEnvWithHome(t.TempDir(), ""), bin, "update") + + requireExitCode(t, 1, err) + combined := stdout + stderr + assert.Contains(t, combined, "mise", "the refusal must name the manager") + assert.Contains(t, combined, filepath.Dir(bin), "the refusal must name the resolved install path") +} + +func TestUpdateForceBypassesExternalRefusal(t *testing.T) { + t.Parallel() + + bin := installLstkUnder(t, ".local/share/mise/installs/github-localstack-lstk/latest") + // Relies on `make build` stamping no version, so Check short-circuits on + // the "dev" build. If the integration build ever stamped one, --force here + // would perform a real download and self-replace; use a mock endpoint then. + stdout, stderr, err := runBinary(t, t.TempDir(), testEnvWithHome(t.TempDir(), ""), bin, "update", "--force") + + require.NoError(t, err, stderr) + combined := stdout + stderr + assert.NotContains(t, combined, "managed by mise", "--force must skip the refusal entirely") +} + +func TestUpdateJSONReportsExternallyManagedCode(t *testing.T) { + t.Parallel() + + bin := installLstkUnder(t, "nix/store/9zk1abcdlstk-lstk-0.5.0/bin") + stdout, stderr, err := runBinary(t, t.TempDir(), testEnvWithHome(t.TempDir(), ""), bin, "update", "--json") + + requireExitCode(t, 1, err) + + var envelope struct { + Status string `json:"status"` + Error struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), "stdout was: %s / stderr: %s", stdout, stderr) + assert.Equal(t, "error", envelope.Status) + assert.Equal(t, "UPDATE_EXTERNALLY_MANAGED", envelope.Error.Code) + assert.Contains(t, envelope.Error.Message, "nix") +} + +func TestUpdateProceedsOnOrdinaryInstall(t *testing.T) { + t.Parallel() + + bin := installLstkUnder(t, "opt/tools/bin") + // No --check: --check is exempt from the refusal by design, so it would not + // exercise the guard at all. Safe without a version stamp because `make + // build` sets none, so Check short-circuits on the "dev" build before any + // network call. + stdout, stderr, err := runBinary(t, t.TempDir(), testEnvWithHome(t.TempDir(), ""), bin, "update") + + require.NoError(t, err, stderr) + assert.NotContains(t, stdout+stderr, "will not update itself") + assert.NotContains(t, stdout+stderr, "not writable") +} + +func writeConfigWithCLI(t *testing.T, body string) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + content := "[[containers]]\ntype = \"aws\"\ntag = \"latest\"\nport = \"4566\"\n\n[cli]\n" + body + "\n" + require.NoError(t, os.WriteFile(path, []byte(content), 0644)) + return path +} + +func TestInvalidUpdateCheckInConfigIsRejected(t *testing.T) { + t.Parallel() + + // "quiet" is not a valid mode; it stands in for a plausible-sounding typo. + configFile := writeConfigWithCLI(t, `update_check = "quiet"`) + stdout, stderr, err := runLstk(t, testContext(t), t.TempDir(), testEnvWithHome(t.TempDir(), ""), + "--config", configFile, "volume", "path") + + requireExitCode(t, 1, err) + combined := stdout + stderr + assert.Contains(t, combined, "update_check") + assert.Contains(t, combined, "quiet") +} + +func TestValidUpdateCheckInConfigIsAccepted(t *testing.T) { + t.Parallel() + + configFile := writeConfigWithCLI(t, `update_check = "off"`) + _, stderr, err := runLstk(t, testContext(t), t.TempDir(), testEnvWithHome(t.TempDir(), ""), + "--config", configFile, "volume", "path") + + require.NoError(t, err, stderr) +} + +func TestUpdateRefusesWhenInstallDirIsNotWritable(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX directory permissions do not port to Windows") + } + t.Parallel() + + bin := installLstkUnder(t, "opt/tools/bin") + dir := filepath.Dir(bin) + require.NoError(t, os.Chmod(dir, 0500)) + // Restore write permission so t.TempDir cleanup can remove the binary. + t.Cleanup(func() { _ = os.Chmod(dir, 0700) }) + + stdout, stderr, err := runBinary(t, t.TempDir(), testEnvWithHome(t.TempDir(), ""), bin, "update") + + requireExitCode(t, 1, err) + assert.Contains(t, stdout+stderr, dir, "the refusal must name the directory it cannot write to") +} + +// buildStampedLstk builds lstk with a real version number into the given +// layout under a temp dir, and returns the binary path. A version is required +// because `make build` stamps none, and `checkQuietlyWithVersion` skips the +// update check entirely for a "dev" build — so an unstamped binary can never +// exercise the notify/off behavior. +func buildStampedLstk(t *testing.T, layout, version string) string { + t.Helper() + + repoRoot, err := filepath.Abs("../..") + require.NoError(t, err) + + dir := filepath.Join(t.TempDir(), filepath.FromSlash(layout)) + require.NoError(t, os.MkdirAll(dir, 0755)) + name := "lstk" + if runtime.GOOS == "windows" { + name = "lstk.exe" + } + bin := filepath.Join(dir, name) + + build := exec.CommandContext(testContext(t), "go", "build", + "-ldflags", "-X github.com/localstack/lstk/internal/version.version="+version, + "-o", bin, ".") + build.Dir = repoRoot + out, err := build.CombinedOutput() + require.NoError(t, err, "go build failed: %s", string(out)) + return bin +} + +// countingReleaseServer serves the release-metadata endpoint and records how +// many times it was asked, so a test can prove no request was made at all. +func countingReleaseServer(t *testing.T, tag string, hits *atomic.Int32) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"tag_name": tag}) + })) + t.Cleanup(srv.Close) + return srv +} + +// startEnv runs `lstk start` against an unreachable Docker daemon: the update +// notification is emitted before container.Start, so its output is observable +// without a real emulator. +func startEnv(t *testing.T, srv *httptest.Server, configFile string, extra ...string) []string { + t.Helper() + e := append(testEnvWithHome(t.TempDir(), ""), + string(env.UpdateGitHubAPIEndpoint)+"="+srv.URL, + string(env.UpdateGitHubDownloadEndpoint)+"="+srv.URL, + unreachableDockerHost, + ) + return append(e, extra...) +} + +func TestUpdateCheckOffMakesNoRequestAndSaysNothing(t *testing.T) { + t.Parallel() + + var hits atomic.Int32 + srv := countingReleaseServer(t, "v9.9.9", &hits) + bin := buildStampedLstk(t, "opt/bin", "0.0.1") + configFile := writeConfigWithCLI(t, `update_check = "off"`) + + stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv, configFile), bin, + "--config", configFile, "start", "--non-interactive") + + assert.Equal(t, int32(0), hits.Load(), "off must make no request to the release API") + assert.NotContains(t, stdout+stderr, "Update available") +} + +func TestUpdateCheckNotifyEmitsNoteWithoutBlocking(t *testing.T) { + t.Parallel() + + var hits atomic.Int32 + srv := countingReleaseServer(t, "v9.9.9", &hits) + bin := buildStampedLstk(t, "opt/bin", "0.0.1") + configFile := writeConfigWithCLI(t, `update_check = "notify"`) + + stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv, configFile), bin, + "--config", configFile, "start", "--non-interactive") + + assert.Equal(t, int32(1), hits.Load(), "notify must still check") + assert.Contains(t, stdout+stderr, "Update available: 0.0.1 → v9.9.9") +} + +func TestUpdateCheckEnvVarOverridesConfig(t *testing.T) { + t.Parallel() + + var hits atomic.Int32 + srv := countingReleaseServer(t, "v9.9.9", &hits) + bin := buildStampedLstk(t, "opt/bin", "0.0.1") + configFile := writeConfigWithCLI(t, `update_check = "notify"`) + + stdout, stderr, _ := runBinary(t, t.TempDir(), + startEnv(t, srv, configFile, "LSTK_UPDATE_CHECK=off"), bin, + "--config", configFile, "start", "--non-interactive") + + assert.Equal(t, int32(0), hits.Load(), "the env var must win over the config key") + assert.NotContains(t, stdout+stderr, "Update available") +} + +// The note must name the external manager rather than advising `lstk update`, +// which refuses on such an install. +func TestUpdateCheckNoteNamesExternalManager(t *testing.T) { + t.Parallel() + + var hits atomic.Int32 + srv := countingReleaseServer(t, "v9.9.9", &hits) + bin := buildStampedLstk(t, ".local/share/mise/installs/github-localstack-lstk/latest", "0.0.1") + configFile := writeConfigWithCLI(t, `update_check = "notify"`) + + stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv, configFile), bin, + "--config", configFile, "start", "--non-interactive") + + combined := stdout + stderr + assert.Contains(t, combined, "mise") + assert.NotContains(t, combined, "run lstk update") +} + +func TestInvalidUpdateCheckEnvVarIsRejectedAsConfigInvalid(t *testing.T) { + t.Parallel() + + var hits atomic.Int32 + srv := countingReleaseServer(t, "v9.9.9", &hits) + bin := buildStampedLstk(t, "opt/bin", "0.0.1") + configFile := writeConfigWithCLI(t, `update_check = "notify"`) + + stdout, _, err := runBinary(t, t.TempDir(), + startEnv(t, srv, configFile, "LSTK_UPDATE_CHECK=quiet"), bin, + "--config", configFile, "start", "--non-interactive", "--json") + + requireExitCode(t, 1, err) + var envelope struct { + Error struct { + Code string `json:"code"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), "stdout: %s", stdout) + assert.Equal(t, "CONFIG_INVALID", envelope.Error.Code) +} + +func TestInvalidUpdateCheckInConfigIsConfigInvalidUnderJSON(t *testing.T) { + t.Parallel() + + configFile := writeConfigWithCLI(t, `update_check = "quiet"`) + stdout, _, err := runLstk(t, testContext(t), t.TempDir(), + append(testEnvWithHome(t.TempDir(), ""), unreachableDockerHost), + "--config", configFile, "start", "--non-interactive", "--json") + + requireExitCode(t, 1, err) + var envelope struct { + Error struct { + Code string `json:"code"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), "stdout: %s", stdout) + assert.Equal(t, "CONFIG_INVALID", envelope.Error.Code) +} + +// The "Never ask again" option writes config, so it must only appear when +// there is a config file to write to. On a genuine first run config.toml does +// not exist yet — it is created later, by the emulator picker — so offering it +// there would tell the user a preference was saved when it was dropped. +func TestUpdatePromptOmitsNeverAskAgainOnFirstRun(t *testing.T) { + t.Parallel() + + var hits atomic.Int32 + srv := countingReleaseServer(t, "v9.9.9", &hits) + bin := buildStampedLstk(t, "opt/bin", "0.0.1") + + // No --config and a fresh HOME: config.toml does not exist. + cmd := exec.Command(bin, "start") + cmd.Env = startEnv(t, srv, "") + proc := startCmdInPTY(t, testContext(t), cmd) + t.Cleanup(proc.kill) + + // The prompt is the first thing ui.Run emits, ahead of the Docker health + // check, so no emulator is needed to observe it. + proc.waitForOutput("Update lstk to latest version?", "the update prompt should appear") + out := proc.output() + assert.Contains(t, out, "Update now") + assert.Contains(t, out, "Remind me next time") + assert.NotContains(t, out, "Never ask again", "the opt-out must be absent with no config file") +} + +func TestUpdatePromptOffersNeverAskAgainWhenConfigExists(t *testing.T) { + t.Parallel() + + var hits atomic.Int32 + srv := countingReleaseServer(t, "v9.9.9", &hits) + bin := buildStampedLstk(t, "opt/bin", "0.0.1") + configFile := writeConfigWithCLI(t, `update_check = "prompt"`) + + cmd := exec.Command(bin, "--config", configFile, "start") + cmd.Env = startEnv(t, srv, configFile) + proc := startCmdInPTY(t, testContext(t), cmd) + t.Cleanup(proc.kill) + + proc.waitForOutput("Update lstk to latest version?", "the update prompt should appear") + proc.waitForOutput("Never ask again", "the opt-out must be offered when config exists") +} diff --git a/test/integration/update_test.go b/test/integration/update_test.go index e3851ae1..8a3c0e42 100644 --- a/test/integration/update_test.go +++ b/test/integration/update_test.go @@ -365,7 +365,7 @@ func TestUpdateNotification(t *testing.T) { mockServer := createMockLicenseServer(false) t.Cleanup(mockServer.Close) - t.Run("skip", func(t *testing.T) { + t.Run("never ask again", func(t *testing.T) { t.Parallel() configFile := filepath.Join(t.TempDir(), "config.toml") originalConfig := `# User-maintained lstk config @@ -384,7 +384,10 @@ port = "4566" # Host port p := startCmdInPTY(t, ctx, cmd) p.waitForOutput("New lstk version available", "update notification prompt should appear") - p.write("s") + // "Never ask again" replaced "Skip this version" as the prompt's + // config-writing option; this test is about the write preserving the + // user's file, not about which preference is written. + p.write("n") out, _ := p.wait() assert.Contains(t, out, "New lstk version available") @@ -392,7 +395,7 @@ port = "4566" # Host port configData, err := os.ReadFile(configFile) require.NoError(t, err) configStr := string(configData) - assert.Contains(t, configStr, "update_skipped_version", "skipped version should be persisted") + assert.Contains(t, configStr, "update_check", "the chosen preference should be persisted") assert.Contains(t, configStr, "# User-maintained lstk config", "file header comment should be preserved") assert.Contains(t, configStr, "# Emulator type", "inline comments should be preserved") assert.Contains(t, configStr, `port = "4566"`, "existing config values should be preserved") From f247dee430afadb6c2b28552906dc6f5a33dee77 Mon Sep 17 00:00:00 2001 From: Joel Scheuner Date: Tue, 8 Sep 2026 11:59:08 +0200 Subject: [PATCH 2/6] Fix Windows path assertions and tighten update-check comments Co-Authored-By: Claude --- cmd/root.go | 26 +++++------ cmd/update.go | 5 +-- internal/config/config.go | 13 +++--- internal/config/update_check.go | 19 +++----- internal/env/env.go | 9 ++-- internal/update/external_install.go | 50 ++++++++------------- internal/update/install_method.go | 36 +++++++--------- internal/update/notify.go | 62 ++++++++++++--------------- internal/update/update.go | 38 +++++++--------- test/integration/update_check_test.go | 18 +++++++- 10 files changed, 124 insertions(+), 152 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 809996ca..265fd90a 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -324,11 +324,10 @@ func configureCommandExecution(root *cobra.Command, cfg *env.Env, tel *telemetry wrapPreRunEForJSON(root, cfg, stdout) } -// resolveUpdateCheckMode applies the update-check resolution order: -// LSTK_UPDATE_CHECK wins over the [cli] update_check config key. An unset value -// stays UpdateCheckUnset rather than defaulting to prompt here, so the domain -// layer can still fall through to install detection — which is what keeps that -// detection off the path when the user did express a preference. +// resolveUpdateCheckMode applies the resolution order: LSTK_UPDATE_CHECK wins +// over the [cli] update_check key. An unset value stays UpdateCheckUnset rather +// than defaulting to prompt, so the domain layer can tell "no preference" from +// an explicit choice. func resolveUpdateCheckMode(cfg *env.Env, appConfig *config.Config) (config.UpdateCheckMode, error) { if cfg.UpdateCheck != "" { mode, err := config.ParseUpdateCheckMode(cfg.UpdateCheck) @@ -363,8 +362,7 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t } // Resolved before anything is written or started, so a bad - // LSTK_UPDATE_CHECK fails as early as a bad config key already does - // (config.Get validates the [cli] section above). + // LSTK_UPDATE_CHECK fails as early as a bad config key already does. updateCheckMode, err := resolveUpdateCheckMode(cfg, appConfig) if err != nil { sink.Emit(output.ErrorEvent{ @@ -421,10 +419,9 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t Mode: updateCheckMode, DetectInstall: update.DetectInstallMethod, } - // Only offer to persist a preference when there is a file to persist it to. - // On a genuine first run config.toml does not exist yet — it is created - // later, by the emulator picker — so the update prompt must not offer an - // option whose effect would be silently dropped. + // Only offer to persist a preference when there is a file for it. On a + // genuine first run config.toml does not exist yet (the emulator picker + // creates it), so the option would be silently dropped. if config.HasFile() { notifyOpts.PersistUpdateCheck = config.SetUpdateCheck } @@ -450,10 +447,9 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t Text: fmt.Sprintf("Configured with default emulator %s.", emName), }) } - // Same options as the interactive path, minus the ability to prompt: the - // mode and the detection hook still apply, so `update_check = "off"` - // silences a non-interactive start too, and a note there still names an - // external manager rather than advising a command that would refuse. + // Same options as the interactive path, minus the ability to prompt, so + // `update_check = "off"` silences a non-interactive start too and its note + // still names an external manager. nonInteractiveNotify := notifyOpts nonInteractiveNotify.CanPrompt = false update.NotifyUpdate(ctx, sink, nonInteractiveNotify) diff --git a/cmd/update.go b/cmd/update.go index 155e6dde..4e46704b 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -32,9 +32,8 @@ func newUpdateCmd(cfg *env.Env) *cobra.Command { } cmd.Flags().BoolVar(&checkOnly, "check", false, "Only check for updates without applying them") - // Exists because externally-managed-install detection is a path-marker - // heuristic: it cannot recognize every packaging layout, so a user whose - // install it misreads must still have a way through. + // Detection is a path-marker heuristic that cannot know every packaging + // layout, so a user it misreads needs a way through. cmd.Flags().BoolVar(&force, "force", false, "Replace the binary even when the install is externally managed or its directory looks unwritable") return cmd diff --git a/internal/config/config.go b/internal/config/config.go index 96c14f48..787ec467 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -171,11 +171,10 @@ func setInFile(path, key string, value any) error { return os.WriteFile(path, []byte(content), 0644) } -// SetUpdateCheck persists the update-check mode. Unlike Set, it fails rather -// than succeeding in memory only when there is no config file to write to: -// this backs the "Never ask again" prompt option, and reporting success for a -// write that was silently dropped would tell the user their choice was saved -// when the next run would prompt them again. +// SetUpdateCheck persists the update-check mode. Unlike Set, it fails when +// there is no config file rather than succeeding in memory only: it backs the +// "Never ask again" option, where a dropped write would tell the user their +// choice was saved and then prompt them again next run. func SetUpdateCheck(mode UpdateCheckMode) error { if resolvedConfigPath() == "" { return errors.New("no config file to write to yet") @@ -184,8 +183,8 @@ func SetUpdateCheck(mode UpdateCheckMode) error { } // HasFile reports whether a config file has been resolved, i.e. whether -// settings can be persisted. The command boundary uses it to decide whether to -// offer options that write config. +// settings can be persisted at all. The command boundary uses it to decide +// whether to offer options that write config. func HasFile() bool { return resolvedConfigPath() != "" } diff --git a/internal/config/update_check.go b/internal/config/update_check.go index 1841b022..f420b2f7 100644 --- a/internal/config/update_check.go +++ b/internal/config/update_check.go @@ -5,13 +5,10 @@ import ( "strings" ) -// UpdateCheckMode is the value of the `[cli] update_check` config key (and of -// the LSTK_UPDATE_CHECK environment variable), governing the automatic update -// check on the start path only — an explicit `lstk update` always runs. -// -// UpdateCheckUnset is the zero value and means "no preference expressed", which -// is what lets a caller distinguish an unset key from an explicit "prompt" and -// fall through to the next source in the resolution order. +// UpdateCheckMode is the `[cli] update_check` / LSTK_UPDATE_CHECK value. It +// governs only the automatic check on start; an explicit `lstk update` always +// runs. The zero value means "no preference", which lets a caller tell an unset +// key from an explicit "prompt" and fall through to the next source. type UpdateCheckMode string const ( @@ -27,11 +24,9 @@ const ( // updateCheckModes is the accepted set, in the order used to build error text. var updateCheckModes = []UpdateCheckMode{UpdateCheckPrompt, UpdateCheckNotify, UpdateCheckOff} -// ParseUpdateCheckMode validates a raw update_check value. An empty string is -// valid and yields UpdateCheckUnset; anything else must match a mode exactly. -// Matching is deliberately strict — no trimming or case folding — so a typo -// surfaces as an error the user can see rather than being silently coerced into -// a mode they did not ask for. +// ParseUpdateCheckMode validates a raw update_check value; "" yields +// UpdateCheckUnset. Matching is exact — no trimming or case folding — so a typo +// is reported rather than coerced into a mode the user did not ask for. func ParseUpdateCheckMode(s string) (UpdateCheckMode, error) { if s == "" { return UpdateCheckUnset, nil diff --git a/internal/env/env.go b/internal/env/env.go index 99a459c0..467ec71f 100644 --- a/internal/env/env.go +++ b/internal/env/env.go @@ -8,9 +8,8 @@ import ( "github.com/spf13/viper" ) -// UpdateCheckVar is the environment variable that overrides the [cli] -// update_check config key. Named here so error messages can quote the exact -// variable a user set rather than a hardcoded string. +// UpdateCheckVar overrides the [cli] update_check config key. Named so error +// messages can quote the exact variable the user set. const UpdateCheckVar = "LSTK_UPDATE_CHECK" type Env struct { @@ -57,8 +56,8 @@ func Init() *Env { AnalyticsEndpoint: viper.GetString("analytics_endpoint"), GitHubToken: viper.GetString("github_token"), MergeStrategy: viper.GetString("merge_strategy"), - // Captured here rather than read from viper later: config.loadConfig - // calls viper.Reset(), which drops the env-var binding this relies on. + // Captured here, not read from viper later: config.loadConfig calls + // viper.Reset(), dropping the env-var binding this relies on. UpdateCheck: viper.GetString("update_check"), } diff --git a/internal/update/external_install.go b/internal/update/external_install.go index 2646b84e..e0efb06b 100644 --- a/internal/update/external_install.go +++ b/internal/update/external_install.go @@ -11,23 +11,17 @@ import ( "github.com/localstack/lstk/internal/output" ) -// installDirWritable reports whether the directory holding the given -// executable path can be written to, which is what an in-place binary update -// requires. It is the backstop for install methods no path marker in -// externalMarkers recognizes — a root-owned /usr/bin install run as a normal -// user, a read-only container layer, an immutable store lstk has not been -// taught about. +// installDirWritable reports whether the directory holding exePath can be +// written to, as an in-place binary update requires. It backstops the path +// markers for installs they do not recognize: a root-owned /usr/bin run as a +// normal user, a read-only container layer, an unknown immutable store. // -// It probes by creating and removing a file rather than calling access(2), -// which can report success for root or under an ACL that the subsequent rename -// would still fail. The probe costs ~50µs against access(2)'s ~5µs, which is -// why it is confined to the explicit `lstk update` path — where it precedes a -// multi-megabyte download and the difference is noise. It must never be put on -// the automatic start-path check, which runs on every `lstk start`. -// -// A permission error means "not writable" rather than a failure; any other -// error is returned, so a caller never reads an unrelated I/O fault as a -// read-only install. +// It probes with a temp file rather than access(2), which can report success +// for root or under an ACL the later rename still fails. That costs ~10x more, +// so it belongs only on the explicit `lstk update` path — never on the +// start-path check, which runs on every `lstk start`. A permission or EROFS +// error means "not writable"; any other error is returned, so an unrelated I/O +// fault is never read as a read-only install. func installDirWritable(exePath string) (bool, error) { dir := filepath.Dir(exePath) f, err := os.CreateTemp(dir, ".lstk-update-probe-*") @@ -52,10 +46,9 @@ func installDirWritable(exePath string) (bool, error) { return true, nil } -// isReadOnlyFSError reports whether err is a read-only filesystem error, which -// is how an immutable store (nix, a read-only container layer) refuses a write -// rather than with a permission error. syscall.EROFS is defined on Windows too, -// so this needs no per-platform variant. +// isReadOnlyFSError reports whether err is EROFS — how an immutable store +// refuses a write, rather than with a permission error. syscall.EROFS exists on +// Windows too, so this needs no per-platform variant. func isReadOnlyFSError(err error) bool { return errors.Is(err, syscall.EROFS) } @@ -95,14 +88,10 @@ func (b selfUpdateBlocker) action() output.ErrorAction { // blockSelfUpdate reports why an in-place binary replacement must not be // attempted, or nil when it may proceed. // -// Homebrew and npm installs are never blocked: they delegate to `brew upgrade` -// and `npm install -g`, which own the install directory themselves and work -// even where lstk cannot write to it directly. -// -// An indeterminate writability probe deliberately does not block. Guessing -// "read-only" from an unrelated I/O error would refuse an update that would -// have worked; falling through instead leaves the pre-existing behavior, where -// the rename reports the real failure. +// Homebrew and npm are never blocked: they delegate to `brew upgrade` and +// `npm install -g`, which own their install directory. An indeterminate probe +// does not block either — guessing "read-only" from an unrelated I/O error +// would refuse an update that would have worked. func blockSelfUpdate(info InstallInfo) *selfUpdateBlocker { if info.Method == InstallExternal { return &selfUpdateBlocker{Manager: info.Manager, Path: info.ResolvedPath} @@ -110,9 +99,8 @@ func blockSelfUpdate(info InstallInfo) *selfUpdateBlocker { if info.Method != InstallBinary { return nil } - // os.Executable() failed, so there is no install directory to probe. - // filepath.Dir("") is ".", which would write the probe into the user's - // working directory and report an unrelated path in the refusal. + // os.Executable() failed, so there is no install directory. filepath.Dir("") + // is ".", which would probe the working directory and name it in the refusal. if info.ResolvedPath == "" { return nil } diff --git a/internal/update/install_method.go b/internal/update/install_method.go index 7c4df67c..fd8a364e 100644 --- a/internal/update/install_method.go +++ b/internal/update/install_method.go @@ -44,23 +44,21 @@ func DetectInstallMethod() InstallInfo { // externalMarker identifies an externally-managed install by an adjacent pair // of path segments: `first` immediately followed by any of `second`. Requiring -// two adjacent segments rather than one keeps an unrelated directory that -// happens to be called "mise" or "scoop" from being read as an install root. +// two keeps an unrelated directory named "mise" or "scoop" from matching. type externalMarker struct { first string second []string manager string } -// externalMarkers covers tool managers whose whole purpose is to own the -// version of the binary they installed, and immutable stores lstk cannot write -// to at all. `rtx` is mise's former directory name and reports as mise, since -// that is the tool the user would run. -// Each manager lists both its install root and the launcher directory that is -// actually on PATH — `shims`/`bin` entries are not symlinks into the install -// root on every platform (scoop's shims are launcher executables, asdf's are -// shell scripts), so EvalSymlinks does not rewrite them and the install-root -// marker alone would miss the common case. +// externalMarkers covers tool managers that own the version of the binary they +// installed, plus immutable stores lstk cannot write to. `rtx` is mise's former +// directory name and reports as mise, the tool the user would run. +// +// Each entry lists the install root *and* the launcher directory on PATH: +// `shims`/`bin` entries are not symlinks into the install root everywhere +// (scoop's are launcher exes, asdf's are shell scripts), so EvalSymlinks leaves +// them alone and the install-root marker would miss the common case. var externalMarkers = []externalMarker{ {first: "nix", second: []string{"store"}, manager: "nix"}, {first: "gnu", second: []string{"store"}, manager: "guix"}, @@ -75,16 +73,14 @@ var externalMarkers = []externalMarker{ } // classifyPath determines the install method from a resolved executable path, -// returning the recognized external manager's name for InstallExternal and an -// empty string for every other method. +// naming the recognized external manager for InstallExternal and "" otherwise. // -// The npm and Homebrew markers are checked across the whole path *before* any -// external marker, and that order is load-bearing: an npm- or Homebrew-managed -// lstk may sit under a tool-manager-provisioned interpreter or prefix (e.g. -// .../mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_.../lstk), -// where the tool manager owns node but `npm install -g` still updates lstk -// correctly. A single in-order segment walk would see "mise" first and refuse -// to update a perfectly updatable install. +// npm and Homebrew markers are checked across the whole path before any +// external marker, and that order is load-bearing: an npm-installed lstk can +// sit under a tool-manager-provisioned interpreter +// (.../mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_.../lstk), +// where `npm install -g` still updates it correctly. A single in-order walk +// would see "mise" first and refuse an update that would have worked. func classifyPath(resolved string) (InstallMethod, string) { cleaned := filepath.Clean(resolved) segments := strings.Split(cleaned, string(os.PathSeparator)) diff --git a/internal/update/notify.go b/internal/update/notify.go index 7eb3289e..7e262d74 100644 --- a/internal/update/notify.go +++ b/internal/update/notify.go @@ -14,17 +14,16 @@ type versionFetcher func(ctx context.Context, token string) (string, error) type NotifyOptions struct { GitHubToken string - // CanPrompt reports whether this call site is able to present a blocking - // prompt at all (an interactive TTY). It is independent of Mode, which is - // the user's preference: a non-interactive start can only ever emit a note, - // however Mode is set. + // CanPrompt reports whether this call site can block at all (an interactive + // TTY). Independent of Mode, the user's preference: a non-interactive start + // only ever emits a note, however Mode is set. CanPrompt bool Mode config.UpdateCheckMode PersistUpdateCheck func(mode config.UpdateCheckMode) error - // DetectInstall resolves how lstk itself was installed. Injected rather - // than called directly so tests do not depend on where the test binary - // happens to live — which is also what makes the apply-time guard on the - // prompt path testable. Defaults to DetectInstallMethod when nil. + // DetectInstall resolves how lstk itself was installed. Injected so tests + // do not depend on where the test binary lives, which is also what makes + // the prompt path's apply-time guard testable. Defaults to + // DetectInstallMethod when nil. DetectInstall func() InstallInfo } @@ -78,12 +77,10 @@ func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyO return false } - // Detection runs exactly once, and only now that an update is known to - // exist — which is what keeps it off every `lstk start`. It runs regardless - // of how the mode was set, because its answer feeds the note's wording as - // well as the prompt/note decision, and a note that says "run lstk update" - // on an install where that command refuses is wrong however the mode was - // reached. + // Once, and only now that an update is known to exist — which keeps + // detection off every `lstk start`. It runs whatever the mode, because its + // answer also decides the note's wording: "run lstk update" is wrong advice + // on an install where that command refuses. info := opts.installInfo() external := info.Method == InstallExternal @@ -92,10 +89,9 @@ func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyO mode = config.UpdateCheckPrompt } - // An externally-managed install is never prompted, even when the user asked - // for prompt explicitly: "Update now" would replace a binary the external - // tool owns, and applyUpdate refuses it anyway. Offering an action that - // cannot be carried out is worse than not offering it. + // Never prompt an externally-managed install, even under an explicit + // prompt: "Update now" would replace a binary the external tool owns, and + // applyUpdate refuses it anyway. Better not to offer it at all. if !opts.CanPrompt || external || mode == config.UpdateCheckNotify { sink.Emit(updateNote(current, latest, info.Manager)) return false @@ -104,10 +100,9 @@ func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyO return promptAndUpdate(ctx, sink, opts, current, latest, info) } -// updateNote is the non-blocking "a newer version exists" line. When manager is -// set, it names that tool instead of pointing at `lstk update` — which refuses -// on an externally-managed install, so advising it there would send the user at -// a command that cannot work. +// updateNote is the non-blocking "a newer version exists" line. With a manager +// set it names that tool instead of `lstk update`, which refuses on such an +// install. func updateNote(current, latest, manager string) output.MessageEvent { text := fmt.Sprintf("Update available: %s → %s (run lstk update)", current, latest) if manager != "" { @@ -126,9 +121,9 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, {Key: "u", Label: "Update now"}, {Key: "r", Label: "Remind me next time"}, } - // Offered only when there is somewhere to write it. On a first run - // config.toml does not exist yet, and persisting would be silently dropped - // — telling the user their choice was saved when it was not. + // Only offered when there is somewhere to write it: on a first run + // config.toml does not exist yet, so the choice would be silently dropped + // after telling the user it was saved. if opts.PersistUpdateCheck != nil { options = append(options, output.InputOption{Key: "n", Label: "Never ask again"}) } @@ -149,10 +144,9 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, switch resp.SelectedKey { case "u": - // A refusal here is recoverable and the start continues, so it is - // surfaced as a single warning. Rendering it as an ErrorEvent (what the - // `lstk update` entry point does) would leave a persistent failure - // block on screen while the emulator comes up underneath it. + // A refusal here is recoverable and the start continues, so warn once. + // An ErrorEvent (what `lstk update` emits) would leave a persistent + // failure block on screen while the emulator comes up underneath it. _, blocker, err := applyUpdate(ctx, sink, latest, opts.GitHubToken, false, info) if blocker != nil { sink.Emit(output.MessageEvent{ @@ -170,13 +164,11 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, case "r": return false case "n": - // Persists notify rather than off: the user asked to stop being - // interrupted, which is not the same as asking never to hear about a - // release again. Silencing entirely stays a deliberate config edit. + // notify, not off: the user asked to stop being interrupted, not to + // never hear about a release. Full silence stays a deliberate edit. if opts.PersistUpdateCheck == nil { - // Unreachable while the option is only offered when the hook is set - // (see above), but a future edit that always appends it must warn - // rather than panic mid-start. + // Unreachable while the option is conditional (see above), but a + // future edit that always appends it must warn, not panic. sink.Emit(output.MessageEvent{Severity: output.SeverityWarning, Text: "Cannot save update preference: no config file"}) return false } diff --git a/internal/update/update.go b/internal/update/update.go index 6e83fe5a..99c6fde4 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -62,23 +62,19 @@ func Check(ctx context.Context, sink output.Sink, githubToken string) (string, b return latest, available, nil } -// Update checks for updates and applies the update if one is available. +// Update checks for updates and applies one if available. // -// When the update would be applied, it first refuses installs it must not -// replace in place (see blockSelfUpdate) — ahead of the version check and the -// download, so an install lstk cannot write to fails immediately instead of -// after fetching and verifying a release archive it can never install. A -// --check run is exempt: reporting whether a newer version exists is useful -// however lstk was installed, and writes nothing. +// When it would apply, it first refuses installs it must not replace (see +// blockSelfUpdate) — ahead of the version check and download, so an install +// lstk cannot write to fails immediately rather than after fetching and +// verifying an archive it can never install. --check is exempt: it writes +// nothing, and its answer is useful however lstk was installed. func Update(ctx context.Context, sink output.Sink, checkOnly bool, githubToken string, force bool) error { info := DetectInstallMethod() - // This pre-check and applyUpdate's own guard both call blockSelfUpdate, so - // an unwritable-directory install is probed twice per `lstk update`. That - // is deliberate: applyUpdate must stay the choke point (every path that - // replaces the binary goes through it, and a caller-supplied verdict could - // be forgotten), while this check keeps the refusal ahead of the version - // check and the download. Two ~50µs probes on a command that would - // otherwise fetch megabytes is the cheaper half of that trade. + // Probes twice per `lstk update` (here and in applyUpdate), deliberately: + // applyUpdate stays the choke point every replacing path goes through, + // while this check keeps the refusal ahead of the download. Two cheap + // probes on a command that would otherwise fetch megabytes. if !checkOnly && !force { if blocker := blockSelfUpdate(info); blocker != nil { return emitSelfUpdateBlocked(sink, blocker) @@ -139,14 +135,12 @@ func emitSelfUpdateBlocked(sink output.Sink, blocker *selfUpdateBlocker) error { // applyUpdate performs the update for an already-detected install method, // returning its canonical name ("homebrew"/"npm"/"binary") on success. // -// The blockSelfUpdate check here is the choke point: every path that actually -// replaces the binary goes through this function, including the start-path -// update prompt's "Update now". Guarding only the `lstk update` entry point -// left that prompt able to clobber an externally-managed install. -// It returns a non-nil blocker instead of performing the update when the -// binary must not be replaced, leaving the caller to choose how to render it: -// `lstk update` fails with an ErrorEvent, the start-path prompt warns and -// carries on. +// Its blockSelfUpdate check is the choke point: every path that replaces the +// binary comes through here, including the start-path prompt's "Update now" — +// guarding only the `lstk update` entry point left that prompt able to clobber +// an externally-managed install. It returns a non-nil blocker rather than +// updating, so each caller picks the severity: `lstk update` fails with an +// ErrorEvent, the prompt warns and carries on. func applyUpdate(ctx context.Context, sink output.Sink, latest, githubToken string, force bool, info InstallInfo) (string, *selfUpdateBlocker, error) { if !force { if blocker := blockSelfUpdate(info); blocker != nil { diff --git a/test/integration/update_check_test.go b/test/integration/update_check_test.go index 7fb4e146..40af5e58 100644 --- a/test/integration/update_check_test.go +++ b/test/integration/update_check_test.go @@ -42,6 +42,18 @@ func installLstkUnder(t *testing.T, layout string) string { return dst } +// resolvedDir reports the directory holding path after symlink evaluation — +// the same resolution DetectInstallMethod applies, and so what lstk prints. +// Asserting on t.TempDir() directly does not work everywhere: macOS returns +// /var/... where lstk prints /private/var/..., and Windows returns an 8.3 +// short name (RUNNER~1) where lstk prints the long one (runneradmin). +func resolvedDir(t *testing.T, path string) string { + t.Helper() + resolved, err := filepath.EvalSymlinks(filepath.Dir(path)) + require.NoError(t, err) + return resolved +} + func TestUpdateRefusesOnMiseManagedInstall(t *testing.T) { t.Parallel() @@ -51,7 +63,7 @@ func TestUpdateRefusesOnMiseManagedInstall(t *testing.T) { requireExitCode(t, 1, err) combined := stdout + stderr assert.Contains(t, combined, "mise", "the refusal must name the manager") - assert.Contains(t, combined, filepath.Dir(bin), "the refusal must name the resolved install path") + assert.Contains(t, combined, resolvedDir(t, bin), "the refusal must name the resolved install path") } func TestUpdateForceBypassesExternalRefusal(t *testing.T) { @@ -145,6 +157,8 @@ func TestUpdateRefusesWhenInstallDirIsNotWritable(t *testing.T) { bin := installLstkUnder(t, "opt/tools/bin") dir := filepath.Dir(bin) + // Resolve before chmod, while the directory is still fully traversable. + want := resolvedDir(t, bin) require.NoError(t, os.Chmod(dir, 0500)) // Restore write permission so t.TempDir cleanup can remove the binary. t.Cleanup(func() { _ = os.Chmod(dir, 0700) }) @@ -152,7 +166,7 @@ func TestUpdateRefusesWhenInstallDirIsNotWritable(t *testing.T) { stdout, stderr, err := runBinary(t, t.TempDir(), testEnvWithHome(t.TempDir(), ""), bin, "update") requireExitCode(t, 1, err) - assert.Contains(t, stdout+stderr, dir, "the refusal must name the directory it cannot write to") + assert.Contains(t, stdout+stderr, want, "the refusal must name the directory it cannot write to") } // buildStampedLstk builds lstk with a real version number into the given From 18c52677967899923059e65687aec7723325438b Mon Sep 17 00:00:00 2001 From: Joel Scheuner Date: Tue, 8 Sep 2026 12:20:56 +0200 Subject: [PATCH 3/6] Cover --force and --check guards, correct overclaiming comments Co-Authored-By: Claude --- CLAUDE.md | 4 +- cmd/root.go | 5 +-- internal/config/update_check.go | 6 ++- internal/update/external_install.go | 10 +++-- internal/update/external_install_test.go | 40 +++++++++++++++++++ internal/update/install_method.go | 7 ++-- internal/update/notify.go | 7 +--- internal/update/notify_guard_test.go | 6 ++- internal/update/notify_mode_test.go | 31 ++++++++------ internal/update/notify_test.go | 11 +++-- .../changes/add-update-check-config/tasks.md | 20 +++++++++- test/integration/update_check_test.go | 38 ++++++++++++------ 12 files changed, 133 insertions(+), 52 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1c98d28f..531af022 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -224,9 +224,9 @@ The release job (`.github/workflows/ci.yml`) builds the npm packages with `gorel # Update Checks -`lstk start` (and the bare root) checks for a newer lstk release and, interactively, prompts to install it. Two things gate that: `[cli] update_check` in config.toml (`prompt` default / `notify` / `off`) with `LSTK_UPDATE_CHECK` overriding it for one run, and — when neither is set — whether the install looks externally managed. `lstk update` itself is never gated by the setting; it is a direct request, not a background nag (DEVX-1029). +`lstk start` (and the bare root) checks for a newer lstk release and, interactively, prompts to install it. `[cli] update_check` in config.toml (`prompt` default / `notify` / `off`) gates that, with `LSTK_UPDATE_CHECK` overriding it for one run. `lstk update` itself is never gated by the setting; it is a direct request, not a background nag (DEVX-1029). -Resolution happens at the command boundary (`resolveUpdateCheckMode` in `cmd/root.go`), never inside `internal/update`. Externally-managed installs (recognized from path-segment markers in `classifyPath`) default to `notify` and are never updated in place — every path that replaces the binary goes through `applyUpdate`, which is the single choke point for that guard. Mechanism and rationale for each piece live on the declarations: `config.UpdateCheckMode`, `env.Env.UpdateCheck`, `NotifyOptions.CanPrompt`, `externalMarkers`, `classifyPath`, `blockSelfUpdate`, and the `--force` flag. +Resolution happens at the command boundary (`resolveUpdateCheckMode` in `cmd/root.go`), never inside `internal/update`. Externally-managed installs (recognized from path-segment markers in `classifyPath`) are never prompted — not even under an explicit `prompt`, since applying the update is refused there anyway — and are never updated in place without `--force`. Every path that replaces the binary goes through `applyUpdate`, the single choke point for that guard. Install detection runs for every mode except `off`, and only once an update is known to exist, so it stays off the common start path. Mechanism and rationale for each piece live on the declarations: `config.UpdateCheckMode`, `env.Env.UpdateCheck`, `NotifyOptions.CanPrompt`, `externalMarkers`, `classifyPath`, `blockSelfUpdate`, and the `--force` flag. # Shell Completion diff --git a/cmd/root.go b/cmd/root.go index 265fd90a..a7d39a1a 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -325,9 +325,8 @@ func configureCommandExecution(root *cobra.Command, cfg *env.Env, tel *telemetry } // resolveUpdateCheckMode applies the resolution order: LSTK_UPDATE_CHECK wins -// over the [cli] update_check key. An unset value stays UpdateCheckUnset rather -// than defaulting to prompt, so the domain layer can tell "no preference" from -// an explicit choice. +// over the [cli] update_check key. Neither set yields UpdateCheckUnset, which +// the domain layer treats as prompt. func resolveUpdateCheckMode(cfg *env.Env, appConfig *config.Config) (config.UpdateCheckMode, error) { if cfg.UpdateCheck != "" { mode, err := config.ParseUpdateCheckMode(cfg.UpdateCheck) diff --git a/internal/config/update_check.go b/internal/config/update_check.go index f420b2f7..40bd143f 100644 --- a/internal/config/update_check.go +++ b/internal/config/update_check.go @@ -7,8 +7,10 @@ import ( // UpdateCheckMode is the `[cli] update_check` / LSTK_UPDATE_CHECK value. It // governs only the automatic check on start; an explicit `lstk update` always -// runs. The zero value means "no preference", which lets a caller tell an unset -// key from an explicit "prompt" and fall through to the next source. +// runs. The zero value means "unset", which is what lets the command boundary +// fall through from the env var to the config key. The domain layer then treats +// unset and prompt identically, since install detection decides between +// prompting and a note either way. type UpdateCheckMode string const ( diff --git a/internal/update/external_install.go b/internal/update/external_install.go index e0efb06b..ff1ffd50 100644 --- a/internal/update/external_install.go +++ b/internal/update/external_install.go @@ -26,7 +26,7 @@ func installDirWritable(exePath string) (bool, error) { dir := filepath.Dir(exePath) f, err := os.CreateTemp(dir, ".lstk-update-probe-*") if err != nil { - if errors.Is(err, fs.ErrPermission) || errors.Is(err, os.ErrPermission) { + if errors.Is(err, fs.ErrPermission) { return false, nil } // A read-only filesystem surfaces as EROFS, which is not ErrPermission. @@ -46,9 +46,11 @@ func installDirWritable(exePath string) (bool, error) { return true, nil } -// isReadOnlyFSError reports whether err is EROFS — how an immutable store -// refuses a write, rather than with a permission error. syscall.EROFS exists on -// Windows too, so this needs no per-platform variant. +// isReadOnlyFSError reports whether err is EROFS — how a unix immutable store +// (a nix store, a read-only container layer) refuses a write, rather than with +// a permission error. syscall.EROFS compiles on Windows but is never produced +// there, so write-protected Windows media falls through as an indeterminate +// probe and is not refused; ACL denials still are, via ErrPermission. func isReadOnlyFSError(err error) bool { return errors.Is(err, syscall.EROFS) } diff --git a/internal/update/external_install_test.go b/internal/update/external_install_test.go index 94d4c837..ba61caae 100644 --- a/internal/update/external_install_test.go +++ b/internal/update/external_install_test.go @@ -1,11 +1,13 @@ package update import ( + "context" "os" "path/filepath" goruntime "runtime" "testing" + "github.com/localstack/lstk/internal/output" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -275,3 +277,41 @@ func TestBlockSelfUpdateUnknownPathIgnoresWorkingDirectory(t *testing.T) { assert.Nil(t, blockSelfUpdate(InstallInfo{Method: InstallBinary, ResolvedPath: ""})) } + +// --force is the documented escape hatch for an install the path markers +// misread, so its bypass inside applyUpdate must stay reachable. Nothing else +// covers it: the integration test for --force runs a `dev` build, which +// short-circuits in Check before applyUpdate is ever called. +// +// The download is pointed at a dead address, so this asserts only that no +// blocker was returned — i.e. that force got past the guard. +func TestApplyUpdateForceBypassesTheBlocker(t *testing.T) { + t.Setenv("LSTK_UPDATE_GITHUB_DOWNLOAD_ENDPOINT", "http://127.0.0.1:1") + + sink := output.SinkFunc(func(output.Event) {}) + info := InstallInfo{ + Method: InstallExternal, + Manager: "mise", + ResolvedPath: filepath.Join(t.TempDir(), "lstk"), + } + + _, blocker, err := applyUpdate(context.Background(), sink, "v9.9.9", "", true, info) + + assert.Nil(t, blocker, "--force must not be refused") + assert.Error(t, err, "the update itself still fails: the download endpoint is dead") +} + +func TestApplyUpdateWithoutForceIsRefused(t *testing.T) { + sink := output.SinkFunc(func(output.Event) {}) + info := InstallInfo{ + Method: InstallExternal, + Manager: "mise", + ResolvedPath: filepath.Join(t.TempDir(), "lstk"), + } + + _, blocker, err := applyUpdate(context.Background(), sink, "v9.9.9", "", false, info) + + require.NotNil(t, blocker) + assert.Equal(t, "mise", blocker.Manager) + assert.NoError(t, err, "a refusal is not an error at this layer") +} diff --git a/internal/update/install_method.go b/internal/update/install_method.go index fd8a364e..f586b54b 100644 --- a/internal/update/install_method.go +++ b/internal/update/install_method.go @@ -55,10 +55,11 @@ type externalMarker struct { // installed, plus immutable stores lstk cannot write to. `rtx` is mise's former // directory name and reports as mise, the tool the user would run. // -// Each entry lists the install root *and* the launcher directory on PATH: -// `shims`/`bin` entries are not symlinks into the install root everywhere +// Tool-manager entries list the install root *and* the launcher directory on +// PATH: `shims`/`bin` entries are not symlinks into the install root everywhere // (scoop's are launcher exes, asdf's are shell scripts), so EvalSymlinks leaves -// them alone and the install-root marker would miss the common case. +// them alone and the install-root marker would miss the common case. The store +// entries need only one segment pair — nothing is installed outside the store. var externalMarkers = []externalMarker{ {first: "nix", second: []string{"store"}, manager: "nix"}, {first: "gnu", second: []string{"store"}, manager: "guix"}, diff --git a/internal/update/notify.go b/internal/update/notify.go index 7e262d74..c645363f 100644 --- a/internal/update/notify.go +++ b/internal/update/notify.go @@ -84,15 +84,10 @@ func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyO info := opts.installInfo() external := info.Method == InstallExternal - mode := opts.Mode - if mode == config.UpdateCheckUnset { - mode = config.UpdateCheckPrompt - } - // Never prompt an externally-managed install, even under an explicit // prompt: "Update now" would replace a binary the external tool owns, and // applyUpdate refuses it anyway. Better not to offer it at all. - if !opts.CanPrompt || external || mode == config.UpdateCheckNotify { + if !opts.CanPrompt || external || opts.Mode == config.UpdateCheckNotify { sink.Emit(updateNote(current, latest, info.Manager)) return false } diff --git a/internal/update/notify_guard_test.go b/internal/update/notify_guard_test.go index 4bbaa659..830d7820 100644 --- a/internal/update/notify_guard_test.go +++ b/internal/update/notify_guard_test.go @@ -90,10 +90,10 @@ func TestNotifyUpdateExplicitNotifyNamesTheManager(t *testing.T) { assert.Contains(t, msg.Text, "asdf") } -// Offering "Never remind me" when there is nowhere to persist it would tell the +// Offering "Never ask again" when there is nowhere to persist it would tell the // user their choice was saved when it was silently dropped (the first-run case, // where config.toml does not exist yet). -func TestNotifyUpdateOmitsNeverRemindWhenItCannotBePersisted(t *testing.T) { +func TestNotifyUpdateOmitsNeverAskAgainWhenItCannotBePersisted(t *testing.T) { server := newTestGitHubServer(t, "v2.0.0") defer server.Close() @@ -106,6 +106,7 @@ func TestNotifyUpdateOmitsNeverRemindWhenItCannotBePersisted(t *testing.T) { }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, Mode: config.UpdateCheckPrompt, CanPrompt: true, PersistUpdateCheck: nil, @@ -226,6 +227,7 @@ func TestPromptOffersUpdateRemindAndNeverAskAgain(t *testing.T) { }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, Mode: config.UpdateCheckPrompt, CanPrompt: true, PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, diff --git a/internal/update/notify_mode_test.go b/internal/update/notify_mode_test.go index 956ff73a..aaf372dc 100644 --- a/internal/update/notify_mode_test.go +++ b/internal/update/notify_mode_test.go @@ -29,8 +29,9 @@ func TestNotifyUpdateOffMakesNoRequestAndNoOutput(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckOff, - CanPrompt: true, + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, + Mode: config.UpdateCheckOff, + CanPrompt: true, }, "1.0.0", failingFetcher(t)) assert.False(t, exit) @@ -51,8 +52,9 @@ func TestNotifyUpdateNotifyModeEmitsNoteWithoutPrompting(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckNotify, - CanPrompt: true, + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, + Mode: config.UpdateCheckNotify, + CanPrompt: true, }, "1.0.0", testFetcher(server.URL)) assert.False(t, exit) @@ -147,7 +149,7 @@ func TestNotifyUpdateNonInteractiveEmitsExactlyOneNote(t *testing.T) { require.Len(t, events, 1) } -func TestNotifyUpdateNeverRemindPersistsNotifyAndAppliesNoUpdate(t *testing.T) { +func TestNotifyUpdateNeverAskAgainPersistsNotifyAndAppliesNoUpdate(t *testing.T) { server := newTestGitHubServer(t, "v2.0.0") defer server.Close() @@ -161,15 +163,16 @@ func TestNotifyUpdateNeverRemindPersistsNotifyAndAppliesNoUpdate(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckPrompt, - CanPrompt: true, + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, + Mode: config.UpdateCheckPrompt, + CanPrompt: true, PersistUpdateCheck: func(mode config.UpdateCheckMode) error { persisted = mode return nil }, }, "1.0.0", testFetcher(server.URL)) - assert.False(t, exit, "choosing never-remind must not restart the command") + assert.False(t, exit, "choosing never-ask-again must not restart the command") assert.Equal(t, config.UpdateCheckNotify, persisted) // "applies no update" is the other half of the behavior: exit == false // alone would not notice an update actually being installed. @@ -181,7 +184,7 @@ func TestNotifyUpdateNeverRemindPersistsNotifyAndAppliesNoUpdate(t *testing.T) { } } -func TestNotifyUpdateNeverRemindWarnsWhenPersistFails(t *testing.T) { +func TestNotifyUpdateNeverAskAgainWarnsWhenPersistFails(t *testing.T) { server := newTestGitHubServer(t, "v2.0.0") defer server.Close() @@ -194,8 +197,9 @@ func TestNotifyUpdateNeverRemindWarnsWhenPersistFails(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckPrompt, - CanPrompt: true, + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, + Mode: config.UpdateCheckPrompt, + CanPrompt: true, PersistUpdateCheck: func(mode config.UpdateCheckMode) error { return assert.AnError }, @@ -251,8 +255,9 @@ func TestNotifyUpdateOrdinaryNoteStillPointsAtLstkUpdate(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckNotify, - CanPrompt: true, + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, + Mode: config.UpdateCheckNotify, + CanPrompt: true, }, "1.0.0", testFetcher(server.URL)) require.Len(t, events, 1) diff --git a/internal/update/notify_test.go b/internal/update/notify_test.go index 298473a9..0cb2ba3e 100644 --- a/internal/update/notify_test.go +++ b/internal/update/notify_test.go @@ -2,14 +2,13 @@ package update import ( "context" - "encoding/json" "fmt" - "github.com/localstack/lstk/internal/config" "net/http" "net/http/httptest" "testing" + "github.com/localstack/lstk/internal/config" "github.com/localstack/lstk/internal/output" "github.com/stretchr/testify/assert" ) @@ -89,7 +88,8 @@ func TestNotifyUpdateNoUpdateAvailable(t *testing.T) { var events []output.Event sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{CanPrompt: true}, "v1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, CanPrompt: true}, "v1.0.0", testFetcher(server.URL)) assert.False(t, exit) assert.Empty(t, events) } @@ -101,7 +101,8 @@ func TestNotifyUpdatePromptDisabled(t *testing.T) { var events []output.Event sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{}, "1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }}, "1.0.0", testFetcher(server.URL)) assert.False(t, exit) assert.Len(t, events, 1) msg, ok := events[0].(output.MessageEvent) @@ -123,6 +124,7 @@ func TestNotifyUpdatePromptRemind(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, CanPrompt: true, PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, }, "1.0.0", testFetcher(server.URL)) @@ -147,6 +149,7 @@ func TestNotifyUpdatePromptCancelled(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, CanPrompt: true, PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, }, "1.0.0", testFetcher(server.URL)) diff --git a/openspec/changes/add-update-check-config/tasks.md b/openspec/changes/add-update-check-config/tasks.md index d5034a8f..940116da 100644 --- a/openspec/changes/add-update-check-config/tasks.md +++ b/openspec/changes/add-update-check-config/tasks.md @@ -62,7 +62,7 @@ Write the tests in this section before the implementation — 2.1's reordering i ### Deliberately not changed - The `n` key for "Never ask again" is kept, with the label sharpened from "Never remind me". A reflexive `n` (meaning "no") does write config, but the write is non-destructive, reversible, and close to what someone dismissing a repeated prompt wants; `r` remains the true decline. -- A skipped version still suppresses output in `notify` mode. Skipping a version is an explicit per-version silence request, so honoring it in every mode is consistent; the stale-state trap it created is fixed by 7.7 instead. +- ~~A skipped version still suppresses output in `notify` mode.~~ Superseded by section 9: the skipped-version mechanism is removed entirely, so neither the suppression nor the trap remains. ## 8. Second-round adversarial review fixes @@ -87,3 +87,21 @@ Write the tests in this section before the implementation — 2.1's reordering i - [x] 9.2 Remove the mechanism behind it: `NotifyOptions.SkippedVersion`, `NotifyOptions.PersistSkipVersion`, the suppression check, `CLIConfig.UpdateSkippedVersion`, `config.SetUpdateSkippedVersion`, and the clearing logic added by 7.7. A leftover key in an existing config is inert (viper ignores unknown keys), so no migration is needed. - [x] 9.3 Retarget the pre-existing `TestUpdateNotification` "skip" subtest at the surviving config-writing option and rename it — it tests that a prompt-driven write preserves the user's comments and formatting, not which preference is written. - [x] 9.4 Update the specs, proposal, and design rationale; leave `internal/config/config_test.go`'s use of the key as generic `setInFile` fixture data alone. + +## 10. Final review fixes + +- [x] 10.1 MAJOR: cover `--force`'s bypass inside `applyUpdate` — mutation-proven unprotected, since the integration test for `--force` runs a `dev` build that short-circuits before `applyUpdate` is reached. Added a unit test pointing the download at a dead address and asserting no blocker is returned. +- [x] 10.2 MAJOR: cover the `--check` exemption (`update --check` from a mise-shaped path must not refuse); a regression there would have exited 1 on every externally-managed install with nothing failing. +- [x] 10.3 Correct CLAUDE.md, which still described detection as running only "when neither is set" and omitted the `--force` caveat — the opposite of the shipped rule after 7.1/7.3. +- [x] 10.4 Remove the dead `Unset` → `Prompt` normalization in `notifyUpdateWithVersion` (a semantic no-op: only `Notify` is ever tested), and fix the two doc comments claiming the domain layer distinguishes unset from prompt. It does not — detection decides either way. +- [x] 10.5 `isReadOnlyFSError`'s comment claimed Windows coverage. `syscall.EROFS` compiles there but is never produced, so write-protected Windows media falls through as indeterminate; ACL denials still refuse. Comment corrected rather than adding a Windows errno. +- [x] 10.6 `resolvedDir`'s comment implied macOS previously failed; it passed by substring accident. Reworded to say Windows requires the helper and macOS merely benefits from it. +- [x] 10.7 `assert.Contains(combined, "mise")` was satisfied by the install path itself; tightened to `"managed by mise"`. Dropped `startEnv`'s unused `configFile` parameter. +- [x] 10.8 Inject `DetectInstall` in the 11 unit-test option literals that fell back to the real detector, matching the field's documented rationale — mutation-verified that no unit test now depends on where the test binary lives. +- [x] 10.9 Nits: correct the `externalMarkers` comment (store entries have no launcher directory), drop the redundant `os.ErrPermission` check (same sentinel as `fs.ErrPermission`), fix `notify_test.go` import grouping (8.8 claimed this but missed it), and rename the "Never remind" test names to match the "Never ask again" label. + +### Not changed + +- `internal/ui/app_test.go`'s `{Key: "s", Label: "Skip this version"}` fixture is pre-existing arbitrary sample data for a component test, unrelated to this prompt. +- `cmd/root.go`'s invalid-value action always advises unsetting the env var, which would misdescribe a bad *config* key — unreachable, because `config.Get()` rejects that one statement earlier. The two validations are redundant on that path by design: `Get()` covers every command, the env branch covers only the start path. +- For an unwritable install directory the offered `--force` will itself fail at the rename. Spec-mandated: `--force` exists for the path-marker heuristic, and suppressing it per-reason would make the flag's contract conditional. diff --git a/test/integration/update_check_test.go b/test/integration/update_check_test.go index 40af5e58..c701266d 100644 --- a/test/integration/update_check_test.go +++ b/test/integration/update_check_test.go @@ -44,9 +44,10 @@ func installLstkUnder(t *testing.T, layout string) string { // resolvedDir reports the directory holding path after symlink evaluation — // the same resolution DetectInstallMethod applies, and so what lstk prints. -// Asserting on t.TempDir() directly does not work everywhere: macOS returns -// /var/... where lstk prints /private/var/..., and Windows returns an 8.3 -// short name (RUNNER~1) where lstk prints the long one (runneradmin). +// Windows requires it: t.TempDir() hands back an 8.3 short name (RUNNER~1) +// where lstk prints the long one (runneradmin), which no substring match can +// bridge. On macOS the unresolved form passed only by accident (/var/... is a +// substring of /private/var/...), so this makes that assertion meaningful too. func resolvedDir(t *testing.T, path string) string { t.Helper() resolved, err := filepath.EvalSymlinks(filepath.Dir(path)) @@ -62,7 +63,7 @@ func TestUpdateRefusesOnMiseManagedInstall(t *testing.T) { requireExitCode(t, 1, err) combined := stdout + stderr - assert.Contains(t, combined, "mise", "the refusal must name the manager") + assert.Contains(t, combined, "managed by mise", "the refusal must name the manager") assert.Contains(t, combined, resolvedDir(t, bin), "the refusal must name the resolved install path") } @@ -213,7 +214,7 @@ func countingReleaseServer(t *testing.T, tag string, hits *atomic.Int32) *httpte // startEnv runs `lstk start` against an unreachable Docker daemon: the update // notification is emitted before container.Start, so its output is observable // without a real emulator. -func startEnv(t *testing.T, srv *httptest.Server, configFile string, extra ...string) []string { +func startEnv(t *testing.T, srv *httptest.Server, extra ...string) []string { t.Helper() e := append(testEnvWithHome(t.TempDir(), ""), string(env.UpdateGitHubAPIEndpoint)+"="+srv.URL, @@ -231,7 +232,7 @@ func TestUpdateCheckOffMakesNoRequestAndSaysNothing(t *testing.T) { bin := buildStampedLstk(t, "opt/bin", "0.0.1") configFile := writeConfigWithCLI(t, `update_check = "off"`) - stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv, configFile), bin, + stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv), bin, "--config", configFile, "start", "--non-interactive") assert.Equal(t, int32(0), hits.Load(), "off must make no request to the release API") @@ -246,7 +247,7 @@ func TestUpdateCheckNotifyEmitsNoteWithoutBlocking(t *testing.T) { bin := buildStampedLstk(t, "opt/bin", "0.0.1") configFile := writeConfigWithCLI(t, `update_check = "notify"`) - stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv, configFile), bin, + stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv), bin, "--config", configFile, "start", "--non-interactive") assert.Equal(t, int32(1), hits.Load(), "notify must still check") @@ -262,7 +263,7 @@ func TestUpdateCheckEnvVarOverridesConfig(t *testing.T) { configFile := writeConfigWithCLI(t, `update_check = "notify"`) stdout, stderr, _ := runBinary(t, t.TempDir(), - startEnv(t, srv, configFile, "LSTK_UPDATE_CHECK=off"), bin, + startEnv(t, srv, "LSTK_UPDATE_CHECK=off"), bin, "--config", configFile, "start", "--non-interactive") assert.Equal(t, int32(0), hits.Load(), "the env var must win over the config key") @@ -279,7 +280,7 @@ func TestUpdateCheckNoteNamesExternalManager(t *testing.T) { bin := buildStampedLstk(t, ".local/share/mise/installs/github-localstack-lstk/latest", "0.0.1") configFile := writeConfigWithCLI(t, `update_check = "notify"`) - stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv, configFile), bin, + stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv), bin, "--config", configFile, "start", "--non-interactive") combined := stdout + stderr @@ -296,7 +297,7 @@ func TestInvalidUpdateCheckEnvVarIsRejectedAsConfigInvalid(t *testing.T) { configFile := writeConfigWithCLI(t, `update_check = "notify"`) stdout, _, err := runBinary(t, t.TempDir(), - startEnv(t, srv, configFile, "LSTK_UPDATE_CHECK=quiet"), bin, + startEnv(t, srv, "LSTK_UPDATE_CHECK=quiet"), bin, "--config", configFile, "start", "--non-interactive", "--json") requireExitCode(t, 1, err) @@ -340,7 +341,7 @@ func TestUpdatePromptOmitsNeverAskAgainOnFirstRun(t *testing.T) { // No --config and a fresh HOME: config.toml does not exist. cmd := exec.Command(bin, "start") - cmd.Env = startEnv(t, srv, "") + cmd.Env = startEnv(t, srv) proc := startCmdInPTY(t, testContext(t), cmd) t.Cleanup(proc.kill) @@ -362,10 +363,23 @@ func TestUpdatePromptOffersNeverAskAgainWhenConfigExists(t *testing.T) { configFile := writeConfigWithCLI(t, `update_check = "prompt"`) cmd := exec.Command(bin, "--config", configFile, "start") - cmd.Env = startEnv(t, srv, configFile) + cmd.Env = startEnv(t, srv) proc := startCmdInPTY(t, testContext(t), cmd) t.Cleanup(proc.kill) proc.waitForOutput("Update lstk to latest version?", "the update prompt should appear") proc.waitForOutput("Never ask again", "the opt-out must be offered when config exists") } + +// --check reports whether a newer version exists and writes nothing, so it is +// exempt from the refusal. Without the exemption this exits 1 on every +// externally-managed install and every unwritable install directory. +func TestUpdateCheckIsNotRefusedOnExternalInstall(t *testing.T) { + t.Parallel() + + bin := installLstkUnder(t, ".local/share/mise/installs/github-localstack-lstk/latest") + stdout, stderr, err := runBinary(t, t.TempDir(), testEnvWithHome(t.TempDir(), ""), bin, "update", "--check") + + require.NoError(t, err, stderr) + assert.NotContains(t, stdout+stderr, "will not update itself") +} From 4e0b0930712279254ec29840b6cee3d2ad3eb5cc Mon Sep 17 00:00:00 2001 From: Joel Scheuner Date: Fri, 11 Sep 2026 15:39:05 +0200 Subject: [PATCH 4/6] Restore InstallMethod.String for the bundled-extensions reinstall check Co-Authored-By: Claude Opus 5 --- internal/update/install_method.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/internal/update/install_method.go b/internal/update/install_method.go index f586b54b..664616dc 100644 --- a/internal/update/install_method.go +++ b/internal/update/install_method.go @@ -16,6 +16,23 @@ const ( InstallExternal // managed by an external tool (nix, mise, ...) ) +// String names the install method itself. Distinct from appliedMethodName, +// which names how an update was *performed* for the --json envelope: a forced +// update of an external install replaces the binary, so it reports "binary" +// while the method here is still "external". +func (m InstallMethod) String() string { + switch m { + case InstallHomebrew: + return "homebrew" + case InstallNPM: + return "npm" + case InstallExternal: + return "external" + default: + return "binary" + } +} + // InstallInfo holds the detected install method and the resolved binary path. type InstallInfo struct { Method InstallMethod From bbe43ed1acb3a95a607f3682882e94cbc8a8fdf7 Mon Sep 17 00:00:00 2001 From: Joel Scheuner Date: Fri, 11 Sep 2026 18:15:15 +0200 Subject: [PATCH 5/6] Point an externally-managed install at its own tool for a missing bundle Co-Authored-By: Claude Opus 5 --- internal/update/reinstall.go | 16 +++++++++++++--- internal/update/reinstall_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/internal/update/reinstall.go b/internal/update/reinstall.go index 4294cebd..0f112f25 100644 --- a/internal/update/reinstall.go +++ b/internal/update/reinstall.go @@ -9,7 +9,7 @@ import ( "github.com/localstack/lstk/internal/version" ) -// MissingBundle describes a binary install of a bundling release whose bundled +// MissingBundle describes an install of a bundling release whose bundled // extensions are not beside lstk: what the pre-bundling updater leaves behind // when it installs a bundling release. Homebrew and npm replace the whole // package, so they never end up here. @@ -52,14 +52,24 @@ func DetectMissingBundleFor(command string) (MissingBundle, bool) { } func detectMissingBundle(info InstallInfo, goos string) (MissingBundle, bool) { - if info.Method != InstallBinary { + if info.Method != InstallBinary && info.Method != InstallExternal { return MissingBundle{}, false } dir := filepath.Dir(info.ResolvedPath) if !bundleMissing(dir, goos) { return MissingBundle{}, false } - return MissingBundle{Dir: dir, Reinstall: reinstallInstruction}, true + return MissingBundle{Dir: dir, Reinstall: reinstallInstructionFor(info)}, true +} + +// reinstallInstructionFor names the tool that owns an externally-managed +// install. Sending those to a release download would install outside the +// manager, leaving it to overwrite the result on its next sync. +func reinstallInstructionFor(info InstallInfo) string { + if info.Method == InstallExternal && info.Manager != "" { + return "reinstall lstk through " + info.Manager + } + return reinstallInstruction } // bundleMissing is true only when neither set member exists. A binary without diff --git a/internal/update/reinstall_test.go b/internal/update/reinstall_test.go index dcc6a20a..4ffb473d 100644 --- a/internal/update/reinstall_test.go +++ b/internal/update/reinstall_test.go @@ -55,3 +55,32 @@ func TestDetectMissingBundleByInstallMethod(t *testing.T) { _, ok = detectMissingBundle(InstallInfo{Method: InstallBinary, ResolvedPath: exe}, "linux") assert.False(t, ok) } + +// An externally-managed install missing its bundle must be pointed at the tool +// that owns it: a GitHub download would install outside the manager. +func TestDetectMissingBundleNamesTheExternalManager(t *testing.T) { + dir := t.TempDir() + exe := filepath.Join(dir, "lstk") + + mb, ok := detectMissingBundle(InstallInfo{ + Method: InstallExternal, + Manager: "mise", + ResolvedPath: exe, + }, "linux") + + require.True(t, ok) + assert.Equal(t, dir, mb.Dir) + assert.Contains(t, mb.Reinstall, "mise") + assert.NotContains(t, mb.Reinstall, "github.com", "an external install must not be sent to a release download") +} + +// A recognized external install with no manager name falls back to the +// generic instruction rather than emitting a dangling sentence. +func TestDetectMissingBundleFallsBackWithoutAManagerName(t *testing.T) { + exe := filepath.Join(t.TempDir(), "lstk") + + mb, ok := detectMissingBundle(InstallInfo{Method: InstallExternal, ResolvedPath: exe}, "linux") + + require.True(t, ok) + assert.Contains(t, mb.Reinstall, "github.com") +} From 4330e4d37c4b89f5d602f9ce0bc097eb040fb75d Mon Sep 17 00:00:00 2001 From: Joel Scheuner Date: Fri, 11 Sep 2026 18:26:56 +0200 Subject: [PATCH 6/6] Simplify the update-check setting to a boolean Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 6 +- cmd/root.go | 35 ++++---- cmd/update.go | 2 +- cmd/update_check_test.go | 49 ++++++----- internal/config/config.go | 27 ++++-- internal/config/default_config.toml | 2 +- internal/config/update_check.go | 51 ++++------- internal/config/update_check_test.go | 85 +++++++++---------- internal/env/env.go | 21 ++--- internal/env/env_test.go | 16 ++-- internal/update/notify.go | 26 +++--- internal/update/notify_guard_test.go | 34 ++++---- internal/update/notify_mode_test.go | 68 +++++++-------- internal/update/notify_test.go | 9 +- .../changes/add-update-check-config/design.md | 15 ++++ .../add-update-check-config/proposal.md | 24 ++++-- .../specs/external-install-detection/spec.md | 12 +-- .../specs/update-check-config/spec.md | 75 ++++++++-------- .../changes/add-update-check-config/tasks.md | 14 +++ test/integration/update_check_test.go | 38 ++++----- test/integration/update_test.go | 6 +- 21 files changed, 317 insertions(+), 298 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 531af022..9ae27bc5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,7 +156,7 @@ Environment variables: - `LOCALSTACK_AUTH_TOKEN` - Auth token (skips browser login if set). It takes precedence over credentials stored in the keyring, so a per-invocation token overrides a previous `lstk login` without a `lstk logout` first; resolution order is env var → keyring → browser login (`auth.GetToken`, mirrored in `cmd/root.go`'s telemetry token resolution). - `LSTK_STARTUP_TIMEOUT` - Startup readiness deadline for `lstk start` (Go duration). Zero/unset uses the per-mode default resolved in `resolveStartupTimeout` (`internal/container/start.go`): 20s interactive (deadline only shows a recoverable keep-waiting/stop prompt, re-armed by "keep waiting"), 60s non-interactive (fatal; the container is left running for inspection). Container exits are detected separately — and instantly, with the exit code — via the exit wait `runtime.Runtime.Start` registers between create and start. `lstk start --timeout ` (also on the bare root) overrides this for a single run; the flag wins over the env var when explicitly set, and `--timeout 0` falls back to the per-mode default (`addTimeoutFlag`/`applyTimeoutFlag` in `cmd/root.go`). `restart` and the snapshot auto-start path do not expose the flag. - `LSTK_OTEL=1` - Enables OpenTelemetry trace export (disabled by default); when enabled, standard `OTEL_EXPORTER_OTLP_*` env vars are respected by the SDK. Requires an OTLP-compatible backend to receive and visualize telemetry — for local development, `make otel` starts one (UI at http://localhost:16686). -- `LSTK_UPDATE_CHECK` - Overrides `[cli] update_check` for one run (`prompt`/`notify`/`off`); see Update Checks below. +- `LSTK_CHECK_FOR_UPDATE_ON_STARTUP` - Overrides `[cli] check_for_update_on_startup` for one run (`true`/`false`); see Update Checks below. - `LSTK_MERGE_STRATEGY` - Default merge strategy for `snapshot load` / `load` (`account-region-merge`, `overwrite`, or `service-merge`) when `--merge` is not passed; an explicit `--merge` always wins. Resolved in `resolveMergeStrategy` (`cmd/snapshot.go`). # Infrastructure as Code Commands @@ -224,9 +224,9 @@ The release job (`.github/workflows/ci.yml`) builds the npm packages with `gorel # Update Checks -`lstk start` (and the bare root) checks for a newer lstk release and, interactively, prompts to install it. `[cli] update_check` in config.toml (`prompt` default / `notify` / `off`) gates that, with `LSTK_UPDATE_CHECK` overriding it for one run. `lstk update` itself is never gated by the setting; it is a direct request, not a background nag (DEVX-1029). +`lstk start` (and the bare root) checks for a newer lstk release and, interactively, prompts to install it. `[cli] check_for_update_on_startup` in config.toml (default true) gates that, with `LSTK_CHECK_FOR_UPDATE_ON_STARTUP` overriding it for one run. It is a boolean rather than a prompt/notify/off enum: install detection already decides between a prompt and a non-blocking notice, so the only choice left to the user is whether to check at all. `lstk update` itself is never gated by the setting; it is a direct request, not a background nag (DEVX-1029). -Resolution happens at the command boundary (`resolveUpdateCheckMode` in `cmd/root.go`), never inside `internal/update`. Externally-managed installs (recognized from path-segment markers in `classifyPath`) are never prompted — not even under an explicit `prompt`, since applying the update is refused there anyway — and are never updated in place without `--force`. Every path that replaces the binary goes through `applyUpdate`, the single choke point for that guard. Install detection runs for every mode except `off`, and only once an update is known to exist, so it stays off the common start path. Mechanism and rationale for each piece live on the declarations: `config.UpdateCheckMode`, `env.Env.UpdateCheck`, `NotifyOptions.CanPrompt`, `externalMarkers`, `classifyPath`, `blockSelfUpdate`, and the `--force` flag. +Resolution happens at the command boundary (`resolveUpdateCheckEnabled` in `cmd/root.go`), never inside `internal/update` — which imports no config package at all. Externally-managed installs (recognized from path-segment markers in `classifyPath`) get the note rather than the prompt, since applying the update is refused there anyway, and are never updated in place without `--force`. Every path that replaces the binary goes through `applyUpdate`, the single choke point for that guard. Install detection runs only when the check is enabled, and only once an update is known to exist, so it stays off the common start path. Mechanism and rationale for each piece live on the declarations: `config.ParseCheckForUpdateOnStartup`, `env.Env.CheckForUpdateOnStartup`, `NotifyOptions.CanPrompt`, `externalMarkers`, `classifyPath`, `blockSelfUpdate`, and the `--force` flag. # Shell Completion diff --git a/cmd/root.go b/cmd/root.go index a7d39a1a..c1b621c2 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -324,18 +324,21 @@ func configureCommandExecution(root *cobra.Command, cfg *env.Env, tel *telemetry wrapPreRunEForJSON(root, cfg, stdout) } -// resolveUpdateCheckMode applies the resolution order: LSTK_UPDATE_CHECK wins -// over the [cli] update_check key. Neither set yields UpdateCheckUnset, which -// the domain layer treats as prompt. -func resolveUpdateCheckMode(cfg *env.Env, appConfig *config.Config) (config.UpdateCheckMode, error) { - if cfg.UpdateCheck != "" { - mode, err := config.ParseUpdateCheckMode(cfg.UpdateCheck) +// resolveUpdateCheckEnabled applies the resolution order: +// LSTK_CHECK_FOR_UPDATE_ON_STARTUP wins over the [cli] +// check_for_update_on_startup key, which wins over the default. +func resolveUpdateCheckEnabled(cfg *env.Env, appConfig *config.Config) (bool, error) { + if cfg.CheckForUpdateOnStartup != "" { + enabled, err := config.ParseCheckForUpdateOnStartup(cfg.CheckForUpdateOnStartup) if err != nil { - return "", fmt.Errorf("invalid %s: %w", env.UpdateCheckVar, err) + return false, fmt.Errorf("invalid %s: %w", env.CheckForUpdateOnStartupVar, err) } - return mode, nil + return enabled, nil } - return config.ParseUpdateCheckMode(appConfig.CLI.UpdateCheck) + if appConfig.CLI.CheckForUpdateOnStartup != nil { + return *appConfig.CLI.CheckForUpdateOnStartup, nil + } + return config.CheckForUpdateOnStartupDefault, nil } func buildStartOptions(cfg *env.Env, appConfig *config.Config, logger log.Logger, tel *telemetry.Client, persist bool) container.StartOptions { @@ -361,14 +364,14 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t } // Resolved before anything is written or started, so a bad - // LSTK_UPDATE_CHECK fails as early as a bad config key already does. - updateCheckMode, err := resolveUpdateCheckMode(cfg, appConfig) + // LSTK_CHECK_FOR_UPDATE_ON_STARTUP fails as early as a bad config key does. + updateCheckEnabled, err := resolveUpdateCheckEnabled(cfg, appConfig) if err != nil { sink.Emit(output.ErrorEvent{ Title: err.Error(), Actions: []output.ErrorAction{{ - Label: "Accepted values are prompt, notify and off. Unset it with:", - Value: "unset " + env.UpdateCheckVar, + Label: "Accepted values are true and false. Unset it with:", + Value: "unset " + env.CheckForUpdateOnStartupVar, }}, Code: output.ErrConfigInvalid, }) @@ -415,14 +418,14 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t notifyOpts := update.NotifyOptions{ GitHubToken: cfg.GitHubToken, CanPrompt: true, - Mode: updateCheckMode, + CheckEnabled: updateCheckEnabled, DetectInstall: update.DetectInstallMethod, } // Only offer to persist a preference when there is a file for it. On a // genuine first run config.toml does not exist yet (the emulator picker // creates it), so the option would be silently dropped. if config.HasFile() { - notifyOpts.PersistUpdateCheck = config.SetUpdateCheck + notifyOpts.PersistUpdateCheck = config.SetCheckForUpdateOnStartup } if isInteractiveMode(cfg) { @@ -447,7 +450,7 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t }) } // Same options as the interactive path, minus the ability to prompt, so - // `update_check = "off"` silences a non-interactive start too and its note + // a disabled check silences a non-interactive start too, and its note // still names an external manager. nonInteractiveNotify := notifyOpts nonInteractiveNotify.CanPrompt = false diff --git a/cmd/update.go b/cmd/update.go index 4e46704b..0dfddf62 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -18,7 +18,7 @@ func newUpdateCmd(cfg *env.Env) *cobra.Command { Short: "Update lstk to the latest version", Long: "Check for and apply updates to the lstk CLI. Respects the original installation method (Homebrew, npm, or direct binary).\n\n" + "An install managed by an external tool (mise, nix, guix, asdf, scoop, chocolatey), or one in a directory lstk cannot write to, is not updated in place — update it through that tool instead, or pass --force to replace the binary anyway. --check always reports whether a newer version exists, whatever the install.\n\n" + - "This command always checks for updates. The [cli] update_check config key and LSTK_UPDATE_CHECK only govern the automatic check on 'lstk start'.", + "This command always checks for updates. The [cli] check_for_update_on_startup config key and LSTK_CHECK_FOR_UPDATE_ON_STARTUP only govern the automatic check on 'lstk start'.", PreRunE: initConfigDeferCreate(nil), Annotations: map[string]string{jsonSupportedAnnotation: "true"}, RunE: func(cmd *cobra.Command, args []string) error { diff --git a/cmd/update_check_test.go b/cmd/update_check_test.go index 8a9a23df..fe0920fa 100644 --- a/cmd/update_check_test.go +++ b/cmd/update_check_test.go @@ -9,27 +9,31 @@ import ( "github.com/stretchr/testify/require" ) -func TestResolveUpdateCheckMode(t *testing.T) { +func boolPtr(b bool) *bool { return &b } + +func TestResolveUpdateCheckEnabled(t *testing.T) { t.Parallel() tests := []struct { name string envValue string - confValue string - want config.UpdateCheckMode + confValue *bool + want bool }{ - {"neither set stays unset so detection can decide", "", "", config.UpdateCheckUnset}, - {"config only", "", "notify", config.UpdateCheckNotify}, - {"env only", "off", "", config.UpdateCheckOff}, - {"env wins over config", "prompt", "off", config.UpdateCheckPrompt}, + {"neither set defaults to enabled", "", nil, true}, + {"config false", "", boolPtr(false), false}, + {"config true", "", boolPtr(true), true}, + {"env false", "false", nil, false}, + {"env wins over config", "true", boolPtr(false), true}, + {"env false wins over config true", "false", boolPtr(true), false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, err := resolveUpdateCheckMode( - &env.Env{UpdateCheck: tt.envValue}, - &config.Config{CLI: config.CLIConfig{UpdateCheck: tt.confValue}}, + got, err := resolveUpdateCheckEnabled( + &env.Env{CheckForUpdateOnStartup: tt.envValue}, + &config.Config{CLI: config.CLIConfig{CheckForUpdateOnStartup: tt.confValue}}, ) require.NoError(t, err) assert.Equal(t, tt.want, got) @@ -37,30 +41,29 @@ func TestResolveUpdateCheckMode(t *testing.T) { } } -// "quiet" is not a mode — it stands in for a plausible-sounding typo, to prove -// a bad value is reported rather than silently coerced into some default. -func TestResolveUpdateCheckModeRejectsInvalidEnvValue(t *testing.T) { +// "quiet" is not a boolean — it stands in for a plausible-sounding typo, to +// prove a bad value is reported rather than silently disabling the check. +func TestResolveUpdateCheckEnabledRejectsInvalidEnvValue(t *testing.T) { t.Parallel() - _, err := resolveUpdateCheckMode( - &env.Env{UpdateCheck: "quiet"}, + _, err := resolveUpdateCheckEnabled( + &env.Env{CheckForUpdateOnStartup: "quiet"}, &config.Config{}, ) require.Error(t, err) - assert.Contains(t, err.Error(), env.UpdateCheckVar) + assert.Contains(t, err.Error(), env.CheckForUpdateOnStartupVar) assert.Contains(t, err.Error(), "quiet") } -// An invalid env value must be rejected even when the config file holds a -// perfectly good one: silently falling back would hide the user's typo and -// apply a mode they did not ask for. -func TestResolveUpdateCheckModeRejectsInvalidEnvValueOverValidConfig(t *testing.T) { +// An invalid env value must be rejected even when the config file holds a good +// one: falling back would hide the typo and apply a setting not asked for. +func TestResolveUpdateCheckEnabledRejectsInvalidEnvValueOverValidConfig(t *testing.T) { t.Parallel() - _, err := resolveUpdateCheckMode( - &env.Env{UpdateCheck: "quiet"}, - &config.Config{CLI: config.CLIConfig{UpdateCheck: "notify"}}, + _, err := resolveUpdateCheckEnabled( + &env.Env{CheckForUpdateOnStartup: "quiet"}, + &config.Config{CLI: config.CLIConfig{CheckForUpdateOnStartup: boolPtr(true)}}, ) require.Error(t, err) diff --git a/internal/config/config.go b/internal/config/config.go index 787ec467..d05ea270 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -18,7 +18,8 @@ import ( var defaultConfigTemplate string type CLIConfig struct { - UpdateCheck string `mapstructure:"update_check"` + // Pointer so an unset key is distinguishable from an explicit false. + CheckForUpdateOnStartup *bool `mapstructure:"check_for_update_on_startup"` } type Config struct { @@ -171,15 +172,15 @@ func setInFile(path, key string, value any) error { return os.WriteFile(path, []byte(content), 0644) } -// SetUpdateCheck persists the update-check mode. Unlike Set, it fails when -// there is no config file rather than succeeding in memory only: it backs the -// "Never ask again" option, where a dropped write would tell the user their +// SetCheckForUpdateOnStartup persists the update-check setting. Unlike Set, it +// fails when there is no config file rather than succeeding in memory only: it +// backs the prompt's opt-out, where a dropped write would tell the user their // choice was saved and then prompt them again next run. -func SetUpdateCheck(mode UpdateCheckMode) error { +func SetCheckForUpdateOnStartup(enabled bool) error { if resolvedConfigPath() == "" { return errors.New("no config file to write to yet") } - return Set("cli.update_check", string(mode)) + return Set("cli."+checkForUpdateOnStartupKey, enabled) } // HasFile reports whether a config file has been resolved, i.e. whether @@ -190,6 +191,17 @@ func HasFile() bool { } func Get() (*Config, error) { + // Checked before unmarshal: mapstructure's own bool failure names the key + // but neither the offending value nor the accepted ones, and this is the + // same message the environment variable produces. + if raw := viper.Get("cli." + checkForUpdateOnStartupKey); raw != nil { + if _, ok := raw.(bool); !ok { + if _, err := ParseCheckForUpdateOnStartup(fmt.Sprint(raw)); err != nil { + return nil, fmt.Errorf("invalid [cli] config: %w", err) + } + } + } + var cfg Config if err := viper.Unmarshal(&cfg); err != nil { return nil, fmt.Errorf("failed to unmarshal config: %w", err) @@ -202,9 +214,6 @@ func Get() (*Config, error) { if err := validateNamedEnvs(cfg.Env); err != nil { return nil, err } - if _, err := ParseUpdateCheckMode(cfg.CLI.UpdateCheck); err != nil { - return nil, fmt.Errorf("invalid [cli] config: %w", err) - } return &cfg, nil } diff --git a/internal/config/default_config.toml b/internal/config/default_config.toml index 48cd6c2b..6376345e 100644 --- a/internal/config/default_config.toml +++ b/internal/config/default_config.toml @@ -64,4 +64,4 @@ port = "4566" # Host port the emulator will be accessible on # CLI behavior [cli] -# update_check = "notify" # Update check on start: "prompt" (default), "notify", "off" +# check_for_update_on_startup = false # Skip the update check on start (default: true) diff --git a/internal/config/update_check.go b/internal/config/update_check.go index 40bd143f..92b763bc 100644 --- a/internal/config/update_check.go +++ b/internal/config/update_check.go @@ -2,45 +2,24 @@ package config import ( "fmt" - "strings" + "strconv" ) -// UpdateCheckMode is the `[cli] update_check` / LSTK_UPDATE_CHECK value. It -// governs only the automatic check on start; an explicit `lstk update` always -// runs. The zero value means "unset", which is what lets the command boundary -// fall through from the env var to the config key. The domain layer then treats -// unset and prompt identically, since install detection decides between -// prompting and a note either way. -type UpdateCheckMode string +// CheckForUpdateOnStartupDefault applies when neither the `[cli] +// check_for_update_on_startup` key nor LSTK_CHECK_FOR_UPDATE_ON_STARTUP is set. +const CheckForUpdateOnStartupDefault = true -const ( - UpdateCheckUnset UpdateCheckMode = "" - // UpdateCheckPrompt checks and, on an interactive start, blocks on a choice. - UpdateCheckPrompt UpdateCheckMode = "prompt" - // UpdateCheckNotify checks and emits a single non-blocking note. - UpdateCheckNotify UpdateCheckMode = "notify" - // UpdateCheckOff performs no check at all: no request, no output. - UpdateCheckOff UpdateCheckMode = "off" -) - -// updateCheckModes is the accepted set, in the order used to build error text. -var updateCheckModes = []UpdateCheckMode{UpdateCheckPrompt, UpdateCheckNotify, UpdateCheckOff} +// checkForUpdateOnStartupKey is the key's name within the [cli] table, shared +// by the reader, the writer and the pre-unmarshal validation. +const checkForUpdateOnStartupKey = "check_for_update_on_startup" -// ParseUpdateCheckMode validates a raw update_check value; "" yields -// UpdateCheckUnset. Matching is exact — no trimming or case folding — so a typo -// is reported rather than coerced into a mode the user did not ask for. -func ParseUpdateCheckMode(s string) (UpdateCheckMode, error) { - if s == "" { - return UpdateCheckUnset, nil - } - for _, m := range updateCheckModes { - if string(m) == s { - return m, nil - } - } - valid := make([]string, len(updateCheckModes)) - for i, m := range updateCheckModes { - valid[i] = string(m) +// ParseCheckForUpdateOnStartup parses the environment variable's raw value. +// Anything strconv.ParseBool rejects is reported rather than coerced, so a typo +// cannot silently disable update checks. +func ParseCheckForUpdateOnStartup(s string) (bool, error) { + enabled, err := strconv.ParseBool(s) + if err != nil { + return false, fmt.Errorf("invalid check_for_update_on_startup value %q (must be true or false)", s) } - return "", fmt.Errorf("invalid update_check value %q (must be one of: %s)", s, strings.Join(valid, ", ")) + return enabled, nil } diff --git a/internal/config/update_check_test.go b/internal/config/update_check_test.go index accd9105..c53d865a 100644 --- a/internal/config/update_check_test.go +++ b/internal/config/update_check_test.go @@ -10,34 +10,31 @@ import ( "github.com/stretchr/testify/require" ) -func TestParseUpdateCheckMode(t *testing.T) { +func TestParseCheckForUpdateOnStartup(t *testing.T) { t.Parallel() tests := []struct { name string in string - want UpdateCheckMode + want bool wantErr bool }{ - {"prompt", "prompt", UpdateCheckPrompt, false}, - {"notify", "notify", UpdateCheckNotify, false}, - {"off", "off", UpdateCheckOff, false}, - {"empty means unset", "", UpdateCheckUnset, false}, - // A plausible-sounding non-mode: it must be reported, not coerced. - {"unknown value", "quiet", "", true}, - {"case sensitive", "Off", "", true}, - {"whitespace is not trimmed away silently", " off", "", true}, + {"true", "true", true, false}, + {"false", "false", false, false}, + {"1", "1", true, false}, + {"0", "0", false, false}, + {"TRUE", "TRUE", true, false}, + // A plausible-sounding non-value: it must be reported, not coerced. + {"unknown value", "quiet", false, true}, + {"empty", "", false, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, err := ParseUpdateCheckMode(tt.in) + got, err := ParseCheckForUpdateOnStartup(tt.in) if tt.wantErr { require.Error(t, err) - assert.Contains(t, err.Error(), "update_check") - assert.Contains(t, err.Error(), "prompt") - assert.Contains(t, err.Error(), "notify") - assert.Contains(t, err.Error(), "off") + assert.Contains(t, err.Error(), "check_for_update_on_startup") return } require.NoError(t, err) @@ -46,8 +43,8 @@ func TestParseUpdateCheckMode(t *testing.T) { } } -// writeTestConfig writes a config.toml with the given [cli] body and loads it. -// Not parallel-safe: viper state is global. +// loadConfigWithCLISection writes a config.toml with the given [cli] body and +// loads it. Not parallel-safe: viper state is global. func loadConfigWithCLISection(t *testing.T, cliBody string) { t.Helper() path := filepath.Join(t.TempDir(), "config.toml") @@ -59,56 +56,54 @@ func loadConfigWithCLISection(t *testing.T, cliBody string) { require.NoError(t, InitFromPath(path)) } -func TestGetRejectsInvalidUpdateCheck(t *testing.T) { - loadConfigWithCLISection(t, `update_check = "quiet"`) +func TestGetRejectsNonBooleanCheckForUpdateOnStartup(t *testing.T) { + loadConfigWithCLISection(t, `check_for_update_on_startup = "quiet"`) _, err := Get() require.Error(t, err) - assert.Contains(t, err.Error(), "update_check") - assert.Contains(t, err.Error(), "quiet") + assert.Contains(t, err.Error(), "check_for_update_on_startup") } -func TestGetAcceptsValidUpdateCheck(t *testing.T) { - loadConfigWithCLISection(t, `update_check = "notify"`) +func TestGetReadsCheckForUpdateOnStartup(t *testing.T) { + loadConfigWithCLISection(t, `check_for_update_on_startup = false`) cfg, err := Get() require.NoError(t, err) - assert.Equal(t, "notify", cfg.CLI.UpdateCheck) + require.NotNil(t, cfg.CLI.CheckForUpdateOnStartup) + assert.False(t, *cfg.CLI.CheckForUpdateOnStartup) } -func TestGetTreatsMissingUpdateCheckAsUnset(t *testing.T) { +// An unset key must stay distinguishable from an explicit false, so the +// default can apply. +func TestGetLeavesCheckForUpdateOnStartupNilWhenUnset(t *testing.T) { loadConfigWithCLISection(t, "") cfg, err := Get() require.NoError(t, err) - assert.Empty(t, cfg.CLI.UpdateCheck) + assert.Nil(t, cfg.CLI.CheckForUpdateOnStartup) } -func TestSetUpdateCheckPersistsToFile(t *testing.T) { +func TestSetCheckForUpdateOnStartupPersistsToFile(t *testing.T) { path := filepath.Join(t.TempDir(), "config.toml") - content := "# my config\n[[containers]]\ntype = \"aws\"\n\n[cli]\nupdate_skipped_version = \"v1.2.3\"\n" + content := "# my config\n[[containers]]\ntype = \"aws\"\n\n[cli]\n" require.NoError(t, os.WriteFile(path, []byte(content), 0644)) require.NoError(t, InitFromPath(path)) - require.NoError(t, SetUpdateCheck(UpdateCheckNotify)) + require.NoError(t, SetCheckForUpdateOnStartup(false)) data, err := os.ReadFile(path) require.NoError(t, err) - // setInFile encodes strings as TOML literal strings (single quotes); see - // TestSetInFileAppendsWhenKeyAbsent. - assert.Contains(t, string(data), `update_check = 'notify'`) + assert.Contains(t, string(data), "check_for_update_on_startup = false") assert.Contains(t, string(data), "# my config", "existing comments must survive") - assert.Contains(t, string(data), `update_skipped_version = "v1.2.3"`, "sibling keys must survive") } -// SetUpdateCheck must fail rather than succeed in memory only when there is no -// config file: it backs the "Never ask again" prompt option, and a silent no-op -// would tell the user their choice was saved when the next run would ask again. -func TestSetUpdateCheckFailsWithoutAConfigFile(t *testing.T) { +// Backs the prompt's opt-out: a dropped write would tell the user their choice +// was saved when the next run would prompt again. +func TestSetCheckForUpdateOnStartupFailsWithoutAConfigFile(t *testing.T) { viper.Reset() assert.False(t, HasFile()) - require.Error(t, SetUpdateCheck(UpdateCheckNotify)) + require.Error(t, SetCheckForUpdateOnStartup(false)) } func TestHasFileReportsAResolvedConfig(t *testing.T) { @@ -117,22 +112,20 @@ func TestHasFileReportsAResolvedConfig(t *testing.T) { assert.True(t, HasFile()) } -// The shipped template documents update_check as a commented line inside -// [cli], while setInFile inserts a written key directly below the header — so -// the live value lands above the comment describing it. That is accepted -// (the alternative was a comment block detached from its own table), but the -// result must still be valid TOML that reads back correctly. -func TestSetUpdateCheckOnTheShippedTemplate(t *testing.T) { +// The shipped template must stay writable: lstk inserts the key directly below +// the [cli] header, and the result has to read back correctly. +func TestSetCheckForUpdateOnStartupOnTheShippedTemplate(t *testing.T) { path := filepath.Join(t.TempDir(), "config.toml") require.NoError(t, os.WriteFile(path, []byte(defaultConfigTemplate), 0644)) require.NoError(t, InitFromPath(path)) - require.NoError(t, SetUpdateCheck(UpdateCheckNotify)) + require.NoError(t, SetCheckForUpdateOnStartup(false)) require.NoError(t, InitFromPath(path)) cfg, err := Get() require.NoError(t, err) - assert.Equal(t, "notify", cfg.CLI.UpdateCheck) + require.NotNil(t, cfg.CLI.CheckForUpdateOnStartup) + assert.False(t, *cfg.CLI.CheckForUpdateOnStartup) data, err := os.ReadFile(path) require.NoError(t, err) diff --git a/internal/env/env.go b/internal/env/env.go index 467ec71f..96c7e8e7 100644 --- a/internal/env/env.go +++ b/internal/env/env.go @@ -8,9 +8,9 @@ import ( "github.com/spf13/viper" ) -// UpdateCheckVar overrides the [cli] update_check config key. Named so error -// messages can quote the exact variable the user set. -const UpdateCheckVar = "LSTK_UPDATE_CHECK" +// CheckForUpdateOnStartupVar overrides the [cli] check_for_update_on_startup +// config key. Named so error messages can quote the exact variable the user set. +const CheckForUpdateOnStartupVar = "LSTK_CHECK_FOR_UPDATE_ON_STARTUP" type Env struct { AuthToken string @@ -25,11 +25,11 @@ type Env struct { ForceFileKeyring bool AnalyticsEndpoint string - NonInteractive bool - JSON bool - GitHubToken string - MergeStrategy string - UpdateCheck string + NonInteractive bool + JSON bool + GitHubToken string + MergeStrategy string + CheckForUpdateOnStartup string } // Init initializes environment variable configuration and returns the result. @@ -57,8 +57,9 @@ func Init() *Env { GitHubToken: viper.GetString("github_token"), MergeStrategy: viper.GetString("merge_strategy"), // Captured here, not read from viper later: config.loadConfig calls - // viper.Reset(), dropping the env-var binding this relies on. - UpdateCheck: viper.GetString("update_check"), + // viper.Reset(), dropping the env-var binding this relies on. Kept raw + // so an unset variable stays distinguishable from an explicit false. + CheckForUpdateOnStartup: viper.GetString("check_for_update_on_startup"), } } diff --git a/internal/env/env_test.go b/internal/env/env_test.go index e9ac943f..f772c35f 100644 --- a/internal/env/env_test.go +++ b/internal/env/env_test.go @@ -6,20 +6,20 @@ import ( "github.com/stretchr/testify/assert" ) -func TestInitReadsUpdateCheck(t *testing.T) { - t.Setenv("LSTK_UPDATE_CHECK", "off") +func TestInitReadsCheckForUpdateOnStartup(t *testing.T) { + t.Setenv("LSTK_CHECK_FOR_UPDATE_ON_STARTUP", "false") cfg := Init() - assert.Equal(t, "off", cfg.UpdateCheck) + assert.Equal(t, "false", cfg.CheckForUpdateOnStartup) } -func TestInitLeavesUpdateCheckEmptyWhenUnset(t *testing.T) { - // Explicitly cleared: without this the test reads the developer's own - // environment and fails for anyone who exports the variable. - t.Setenv("LSTK_UPDATE_CHECK", "") +// Kept raw and empty when unset, so the command boundary can tell an unset +// variable from an explicit false. +func TestInitLeavesCheckForUpdateOnStartupEmptyWhenUnset(t *testing.T) { + t.Setenv("LSTK_CHECK_FOR_UPDATE_ON_STARTUP", "") cfg := Init() - assert.Empty(t, cfg.UpdateCheck) + assert.Empty(t, cfg.CheckForUpdateOnStartup) } diff --git a/internal/update/notify.go b/internal/update/notify.go index c645363f..a85c58a5 100644 --- a/internal/update/notify.go +++ b/internal/update/notify.go @@ -5,7 +5,6 @@ import ( "fmt" "time" - "github.com/localstack/lstk/internal/config" "github.com/localstack/lstk/internal/output" "github.com/localstack/lstk/internal/version" ) @@ -17,9 +16,10 @@ type NotifyOptions struct { // CanPrompt reports whether this call site can block at all (an interactive // TTY). Independent of Mode, the user's preference: a non-interactive start // only ever emits a note, however Mode is set. - CanPrompt bool - Mode config.UpdateCheckMode - PersistUpdateCheck func(mode config.UpdateCheckMode) error + CanPrompt bool + // CheckEnabled is the resolved `[cli] check_for_update_on_startup`. + CheckEnabled bool + PersistUpdateCheck func(enabled bool) error // DetectInstall resolves how lstk itself was installed. Injected so tests // do not depend on where the test binary lives, which is also what makes // the prompt path's apply-time guard testable. Defaults to @@ -68,7 +68,7 @@ func NotifyUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions) (ex } func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyOptions, currentVersion string, fetch versionFetcher) (exitAfter bool) { - if opts.Mode == config.UpdateCheckOff { + if !opts.CheckEnabled { return false } @@ -84,10 +84,10 @@ func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyO info := opts.installInfo() external := info.Method == InstallExternal - // Never prompt an externally-managed install, even under an explicit - // prompt: "Update now" would replace a binary the external tool owns, and - // applyUpdate refuses it anyway. Better not to offer it at all. - if !opts.CanPrompt || external || opts.Mode == config.UpdateCheckNotify { + // Never prompt an externally-managed install: "Update now" would replace a + // binary the external tool owns, and applyUpdate refuses it anyway. Better + // not to offer it at all. + if !opts.CanPrompt || external { sink.Emit(updateNote(current, latest, info.Manager)) return false } @@ -120,7 +120,7 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, // config.toml does not exist yet, so the choice would be silently dropped // after telling the user it was saved. if opts.PersistUpdateCheck != nil { - options = append(options, output.InputOption{Key: "n", Label: "Never ask again"}) + options = append(options, output.InputOption{Key: "n", Label: "Never check again"}) } responseCh := make(chan output.InputResponse, 1) @@ -159,19 +159,17 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, case "r": return false case "n": - // notify, not off: the user asked to stop being interrupted, not to - // never hear about a release. Full silence stays a deliberate edit. if opts.PersistUpdateCheck == nil { // Unreachable while the option is conditional (see above), but a // future edit that always appends it must warn, not panic. sink.Emit(output.MessageEvent{Severity: output.SeverityWarning, Text: "Cannot save update preference: no config file"}) return false } - if err := opts.PersistUpdateCheck(config.UpdateCheckNotify); err != nil { + if err := opts.PersistUpdateCheck(false); err != nil { sink.Emit(output.MessageEvent{Severity: output.SeverityWarning, Text: fmt.Sprintf("Failed to save update preference: %v", err)}) return false } - sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Won't ask again — new versions will show as a note. Run lstk update to update."}) + sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Update checks disabled. Run lstk update to check and update."}) return false } diff --git a/internal/update/notify_guard_test.go b/internal/update/notify_guard_test.go index 830d7820..0f77bcb5 100644 --- a/internal/update/notify_guard_test.go +++ b/internal/update/notify_guard_test.go @@ -8,16 +8,14 @@ import ( "strings" "testing" - "github.com/localstack/lstk/internal/config" "github.com/localstack/lstk/internal/output" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// An explicit update_check = "prompt" must not produce a prompt on an -// externally-managed install: pressing "Update now" would replace a binary the -// external tool owns, which is the bug this whole feature exists to prevent. -func TestNotifyUpdateExternalInstallNeverPromptsEvenWhenPromptIsExplicit(t *testing.T) { +// An externally-managed install must never be prompted: pressing "Update now" +// would replace a binary the external tool owns, the bug this feature prevents. +func TestNotifyUpdateExternalInstallNeverPrompts(t *testing.T) { server := newTestGitHubServer(t, "v2.0.0") defer server.Close() @@ -31,8 +29,8 @@ func TestNotifyUpdateExternalInstallNeverPromptsEvenWhenPromptIsExplicit(t *test }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckPrompt, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: true, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallExternal, Manager: "mise"} }, @@ -54,8 +52,8 @@ func TestNotifyUpdateNonInteractiveNoteNamesTheManager(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckUnset, - CanPrompt: false, + CheckEnabled: true, + CanPrompt: false, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallExternal, Manager: "nix"} }, @@ -78,8 +76,8 @@ func TestNotifyUpdateExplicitNotifyNamesTheManager(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckNotify, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: true, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallExternal, Manager: "asdf"} }, @@ -107,7 +105,7 @@ func TestNotifyUpdateOmitsNeverAskAgainWhenItCannotBePersisted(t *testing.T) { notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, - Mode: config.UpdateCheckPrompt, + CheckEnabled: true, CanPrompt: true, PersistUpdateCheck: nil, }, "1.0.0", testFetcher(server.URL)) @@ -146,8 +144,8 @@ func TestPromptUpdateNowIsRefusedWhenTheBinaryCannotBeReplaced(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckPrompt, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: true, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary, ResolvedPath: filepath.Join(readOnly, "lstk")} }, @@ -200,8 +198,8 @@ func TestPromptRefusalEmitsNoErrorEvent(t *testing.T) { }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckPrompt, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: true, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary, ResolvedPath: filepath.Join(readOnly, "lstk")} }, @@ -228,9 +226,9 @@ func TestPromptOffersUpdateRemindAndNeverAskAgain(t *testing.T) { notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, - Mode: config.UpdateCheckPrompt, + CheckEnabled: true, CanPrompt: true, - PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, + PersistUpdateCheck: func(bool) error { return nil }, }, "1.0.0", testFetcher(server.URL)) require.Len(t, options, 3) diff --git a/internal/update/notify_mode_test.go b/internal/update/notify_mode_test.go index aaf372dc..7fdc1312 100644 --- a/internal/update/notify_mode_test.go +++ b/internal/update/notify_mode_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/localstack/lstk/internal/config" "github.com/localstack/lstk/internal/output" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -30,7 +29,7 @@ func TestNotifyUpdateOffMakesNoRequestAndNoOutput(t *testing.T) { exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, - Mode: config.UpdateCheckOff, + CheckEnabled: false, CanPrompt: true, }, "1.0.0", failingFetcher(t)) @@ -38,32 +37,28 @@ func TestNotifyUpdateOffMakesNoRequestAndNoOutput(t *testing.T) { assert.Empty(t, events) } -func TestNotifyUpdateNotifyModeEmitsNoteWithoutPrompting(t *testing.T) { +// A self-managed interactive install is prompted whenever the check is +// enabled. There is no user-selectable "notify" state: the non-blocking note +// is reserved for externally-managed installs and non-interactive runs. +func TestNotifyUpdateEnabledPromptsASelfManagedInstall(t *testing.T) { server := newTestGitHubServer(t, "v2.0.0") defer server.Close() - var events []output.Event + var prompted bool sink := output.SinkFunc(func(event output.Event) { - events = append(events, event) if req, ok := event.(output.UserInputRequestEvent); ok { - t.Error("notify mode must never prompt") - req.ResponseCh() <- output.InputResponse{Cancelled: true} + prompted = true + req.ResponseCh() <- output.InputResponse{SelectedKey: "r"} } }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ + notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, - Mode: config.UpdateCheckNotify, + CheckEnabled: true, CanPrompt: true, }, "1.0.0", testFetcher(server.URL)) - assert.False(t, exit) - require.Len(t, events, 1) - msg, ok := events[0].(output.MessageEvent) - require.True(t, ok) - assert.Equal(t, output.SeverityNote, msg.Severity) - assert.Contains(t, msg.Text, "1.0.0") - assert.Contains(t, msg.Text, "v2.0.0") + assert.True(t, prompted) } func TestNotifyUpdateExternalInstallDowngradesPromptToNote(t *testing.T) { @@ -80,8 +75,8 @@ func TestNotifyUpdateExternalInstallDowngradesPromptToNote(t *testing.T) { }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckUnset, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: true, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallExternal, Manager: "mise"} }, @@ -103,8 +98,8 @@ func TestNotifyUpdateOffSkipsDetection(t *testing.T) { }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckOff, - CanPrompt: true, + CheckEnabled: false, + CanPrompt: true, DetectInstall: func() InstallInfo { t.Error("off must not consult install detection") return InstallInfo{} @@ -123,8 +118,8 @@ func TestNotifyUpdateSkipsDetectionWhenNoUpdateAvailable(t *testing.T) { }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckUnset, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: true, DetectInstall: func() InstallInfo { t.Error("detection must not run before an update is known to exist") return InstallInfo{} @@ -141,7 +136,7 @@ func TestNotifyUpdateNonInteractiveEmitsExactlyOneNote(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckUnset, + CheckEnabled: true, CanPrompt: false, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, }, "1.0.0", testFetcher(server.URL)) @@ -153,7 +148,7 @@ func TestNotifyUpdateNeverAskAgainPersistsNotifyAndAppliesNoUpdate(t *testing.T) server := newTestGitHubServer(t, "v2.0.0") defer server.Close() - var persisted config.UpdateCheckMode + var persisted *bool var events []output.Event sink := output.SinkFunc(func(event output.Event) { events = append(events, event) @@ -164,16 +159,17 @@ func TestNotifyUpdateNeverAskAgainPersistsNotifyAndAppliesNoUpdate(t *testing.T) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, - Mode: config.UpdateCheckPrompt, + CheckEnabled: true, CanPrompt: true, - PersistUpdateCheck: func(mode config.UpdateCheckMode) error { - persisted = mode + PersistUpdateCheck: func(enabled bool) error { + persisted = &enabled return nil }, }, "1.0.0", testFetcher(server.URL)) assert.False(t, exit, "choosing never-ask-again must not restart the command") - assert.Equal(t, config.UpdateCheckNotify, persisted) + require.NotNil(t, persisted) + assert.False(t, *persisted, "the opt-out disables the check") // "applies no update" is the other half of the behavior: exit == false // alone would not notice an update actually being installed. for _, e := range events { @@ -198,9 +194,9 @@ func TestNotifyUpdateNeverAskAgainWarnsWhenPersistFails(t *testing.T) { exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, - Mode: config.UpdateCheckPrompt, + CheckEnabled: true, CanPrompt: true, - PersistUpdateCheck: func(mode config.UpdateCheckMode) error { + PersistUpdateCheck: func(bool) error { return assert.AnError }, }, "1.0.0", testFetcher(server.URL)) @@ -231,8 +227,8 @@ func TestNotifyUpdateExternalNoteNamesTheManagerNotLstkUpdate(t *testing.T) { }) notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - Mode: config.UpdateCheckUnset, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: true, DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallExternal, Manager: "mise"} }, @@ -247,7 +243,9 @@ func TestNotifyUpdateExternalNoteNamesTheManagerNotLstkUpdate(t *testing.T) { // A note that was *not* caused by detection keeps pointing at `lstk update`, // which is the right advice for a self-managed install. -func TestNotifyUpdateOrdinaryNoteStillPointsAtLstkUpdate(t *testing.T) { +// A self-managed install's note still points at `lstk update`, which works +// there. Reachable only when the call site cannot prompt. +func TestNotifyUpdateSelfManagedNotePointsAtLstkUpdate(t *testing.T) { server := newTestGitHubServer(t, "v2.0.0") defer server.Close() @@ -256,8 +254,8 @@ func TestNotifyUpdateOrdinaryNoteStillPointsAtLstkUpdate(t *testing.T) { notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, - Mode: config.UpdateCheckNotify, - CanPrompt: true, + CheckEnabled: true, + CanPrompt: false, }, "1.0.0", testFetcher(server.URL)) require.Len(t, events, 1) diff --git a/internal/update/notify_test.go b/internal/update/notify_test.go index 0cb2ba3e..a71093fc 100644 --- a/internal/update/notify_test.go +++ b/internal/update/notify_test.go @@ -8,7 +8,6 @@ import ( "net/http/httptest" "testing" - "github.com/localstack/lstk/internal/config" "github.com/localstack/lstk/internal/output" "github.com/stretchr/testify/assert" ) @@ -102,7 +101,9 @@ func TestNotifyUpdatePromptDisabled(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ - DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }}, "1.0.0", testFetcher(server.URL)) + DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, + CheckEnabled: true, + }, "1.0.0", testFetcher(server.URL)) assert.False(t, exit) assert.Len(t, events, 1) msg, ok := events[0].(output.MessageEvent) @@ -126,7 +127,7 @@ func TestNotifyUpdatePromptRemind(t *testing.T) { exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, CanPrompt: true, - PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, + PersistUpdateCheck: func(bool) error { return nil }, }, "1.0.0", testFetcher(server.URL)) assert.False(t, exit) } @@ -151,7 +152,7 @@ func TestNotifyUpdatePromptCancelled(t *testing.T) { exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ DetectInstall: func() InstallInfo { return InstallInfo{Method: InstallBinary} }, CanPrompt: true, - PersistUpdateCheck: func(config.UpdateCheckMode) error { return nil }, + PersistUpdateCheck: func(bool) error { return nil }, }, "1.0.0", testFetcher(server.URL)) assert.False(t, exit) } diff --git a/openspec/changes/add-update-check-config/design.md b/openspec/changes/add-update-check-config/design.md index 3449c5ff..490e47a0 100644 --- a/openspec/changes/add-update-check-config/design.md +++ b/openspec/changes/add-update-check-config/design.md @@ -124,3 +124,18 @@ This also means nothing in automation is affected: `lstk start --non-interactive Moving the notification after the picker would let the option appear on every run. **It is deliberately not moved**, because prompting early is worth more than that: a user on an old or broken CLI should be offered the update before the CLI attempts real work, which matters for both stability and usability. A late prompt would also be preempted by a Docker failure, i.e. exactly the situation where updating might be the fix. The residual gap is narrow — a first run means no config, which almost always means a fresh install already on the latest version, so there is usually nothing to prompt about. Raised here as an open question for the PR rather than settled unilaterally. + +## Simplified to a boolean after review + +Review asked whether three modes were needed, proposing `check_for_update_on_startup = false` with detection covering the rest ([#491 review](https://github.com/localstack/lstk/pull/491#discussion_r3989880826)). They were not, and the enum was over-built: + +| setting | self-managed, interactive | externally managed or non-interactive | +| --- | --- | --- | +| `true` (default) | prompt | non-blocking note | +| `false` | no check | no check | + +`notify` existed so a self-managed user could ask for "tell me, but do not block". Detection already produces exactly that for the case it was designed for — an externally-managed install — and a non-interactive start produced it regardless of mode. What remained was a rarely-wanted third state every user had to read past to reach the two that matter. + +Two consequences worth stating. A self-managed user who wants "note but no prompt" can no longer express it; they disable the check and run `lstk update` when they choose. And "Never check again" now persists `false` — no check at all — where it previously persisted `notify`, so the prompt's opt-out is a stronger commitment than before. Both follow from the reviewer's table, and match what the reporter actually asked for ("I'd like to disable it permanently"). + +The simplification also dissolved the layering question this design spent a paragraph on: `internal/update` no longer imports `internal/config` at all, since a bool needs no shared enum type. diff --git a/openspec/changes/add-update-check-config/proposal.md b/openspec/changes/add-update-check-config/proposal.md index ba8f8f4f..a82d9fc3 100644 --- a/openspec/changes/add-update-check-config/proposal.md +++ b/openspec/changes/add-update-check-config/proposal.md @@ -11,17 +11,23 @@ So two things are missing: an explicit, permanent opt-out the user can set once, ## What Changes -- Add a `[cli] update_check` config setting with three values — `prompt` (today's blocking choice, the default), `notify` (a one-line note, never blocking), and `off` (no check at all: no network request, no output). The corresponding environment variable is `LSTK_UPDATE_CHECK`. -- Resolution order: `LSTK_UPDATE_CHECK` > `[cli] update_check` > a detected externally-managed install (defaults to `notify`) > `prompt`. +- Add a `[cli] check_for_update_on_startup` boolean (default true), with `LSTK_CHECK_FOR_UPDATE_ON_STARTUP` as the environment variable. A boolean rather than a three-mode enum because install detection already decides between a prompt and a non-blocking note; the only choice left to the user is whether to check at all. + + | setting | self-managed, interactive | externally managed or non-interactive | + | --- | --- | --- | + | `true` (default) | today's blocking prompt | one non-blocking note | + | `false` | no check, no output, no request | no check | + +- Resolution order: `LSTK_CHECK_FOR_UPDATE_ON_STARTUP` > `[cli] check_for_update_on_startup` > default `true`. - The setting governs **only** the automatic check on the start path. An explicit `lstk update` / `lstk update --check` always checks and applies regardless of the setting — it is a direct request, not a background nag. -- Replace the prompt's "Skip this version" option with `n` — "Never ask again", which persists `cli.update_check = "notify"`. This is the discoverability half of the fix; the reported problem was not that the prompt exists but that there is no way to say "stop" from where it appears. The prompt keeps three choices rather than gaining a fourth, and `cli.update_skipped_version` and its setter are removed. -- Detect externally-managed installs (`nix`, `guix`, `mise`, `asdf`, `scoop`, `chocolatey`) from the resolved executable path, and use that both to pick the quieter default above and to make `lstk update` refuse rather than clobber — naming the manager and the resolved path instead of attempting an update. `lstk update --force` overrides the refusal, so a false positive is never a dead end. -- Detection is lazy and off the hot path: it only computes a *default*, so an explicit setting skips it entirely, `off` returns before it, and it is otherwise consulted only after the version check has already reported an update available. See design.md for the measured costs. +- Replace the prompt's "Skip this version" option with `n` — "Never check again", which persists `cli.check_for_update_on_startup = false`. This is the discoverability half of the fix; the reported problem was not that the prompt exists but that there is no way to say "stop" from where it appears. The prompt keeps three choices rather than gaining a fourth, and `cli.update_skipped_version` and its setter are removed. +- Detect externally-managed installs (`nix`, `guix`, `mise`, `asdf`, `scoop`, `chocolatey`) from the resolved executable path, and use that both to emit a note instead of a prompt and to make `lstk update` refuse rather than clobber — naming the manager and the resolved path instead of attempting an update. `lstk update --force` overrides the refusal, so a false positive is never a dead end. +- Detection is lazy and off the hot path: a disabled check returns before it, and it is otherwise consulted only after the version check has already reported an update available. See design.md for the measured costs. ## Capabilities ### New Capabilities -- `update-check-config`: the `[cli] update_check` setting and `LSTK_UPDATE_CHECK` variable, the three modes and their exact output, the resolution order, the scope limit to the automatic check, and the "Never remind me" prompt option that persists the setting. +- `update-check-config`: the `[cli] check_for_update_on_startup` setting and `LSTK_CHECK_FOR_UPDATE_ON_STARTUP` variable, the enabled/disabled behavior and its exact output, the resolution order, the scope limit to the automatic check, and the "Never check again" prompt option that persists it. - `external-install-detection`: the path markers that identify an externally-managed install, their precedence relative to the existing Homebrew/npm classification, the `lstk update` refusal and its `--force` override, and the requirement that detection never runs on invocations that do not reach the update check. ## Impact @@ -29,8 +35,8 @@ So two things are missing: an explicit, permanent opt-out the user can set once, - `internal/update/notify.go`: `NotifyOptions` gains a resolved mode instead of the `UpdatePrompt bool`; `notifyUpdateWithVersion` returns before the network call for `off`, and consults detection only after `checkQuietlyWithVersion` reports an update. `promptAndUpdate` gains the `n` option. - `internal/update/install_method.go`: `InstallMethod` gains `InstallExternal`; `InstallInfo` gains a `Manager` string. `classifyPath` is reordered so `node_modules`/`Caskroom` win over the tool-manager markers — the existing test case `…/mise/installs/node/24.8.0/lib/node_modules/@localstack/lstk_darwin_arm64/lstk` (npm-installed lstk under a mise-managed *node*) must stay `InstallNPM`, and the current in-order segment walk would misclassify it. - `internal/update/update.go`: `Update` checks for an externally-managed install before the version check (which also makes the refusal observable end-to-end without a non-`dev` build) and refuses unless `--force`. A writability probe of the resolved executable's directory backs up the path markers on this path only. -- `internal/config/config.go`: `CLIConfig` gains `UpdateCheck` and loses `UpdateSkippedVersion`; a `SetUpdateCheck` setter replaces `SetUpdateSkippedVersion` (same surgical line rewrite); an invalid value is rejected in `Get()` alongside the container validation. -- `internal/env/env.go`: `Env` gains `UpdateCheck`, read in `Init()` — it must be captured there because `config.loadConfig` calls `viper.Reset()`. +- `internal/config/config.go`: `CLIConfig` gains `CheckForUpdateOnStartup *bool` (a pointer so an unset key stays distinguishable from an explicit false) and loses `UpdateSkippedVersion`; a `SetCheckForUpdateOnStartup` setter replaces `SetUpdateSkippedVersion` (same surgical line rewrite); an invalid value is rejected in `Get()` alongside the container validation. +- `internal/env/env.go`: `Env` gains `CheckForUpdateOnStartup`, read in `Init()` — it must be captured there because `config.loadConfig` calls `viper.Reset()`. - `cmd/root.go`: resolves the mode from env + `appConfig.CLI` at the command boundary and passes it into `NotifyOptions`; `cmd/update.go` gains `--force`. - `internal/output/error_code.go`: a new `UPDATE_EXTERNALLY_MANAGED` code for the refusal, plus its `retryable`/`category` classification and a row in `docs/structured-output.md`. -- `internal/config/default_config.toml`: a commented `[cli] update_check` block. Note this only reaches users whose config is created after this change — existing files are never rewritten — so `lstk docs` and the prompt option are the discovery paths for everyone else. +- `internal/config/default_config.toml`: a commented `[cli] check_for_update_on_startup` line. Note this only reaches users whose config is created after this change — existing files are never rewritten — so `lstk docs` and the prompt option are the discovery paths for everyone else. diff --git a/openspec/changes/add-update-check-config/specs/external-install-detection/spec.md b/openspec/changes/add-update-check-config/specs/external-install-detection/spec.md index c9754daf..9e39b2db 100644 --- a/openspec/changes/add-update-check-config/specs/external-install-detection/spec.md +++ b/openspec/changes/add-update-check-config/specs/external-install-detection/spec.md @@ -26,7 +26,7 @@ The refusal SHALL be enforced at the point the binary is actually replaced, not #### Scenario: The start-path prompt cannot clobber an externally-managed install - **GIVEN** lstk is running from a path recognized as managed by mise -- **AND** `[cli] update_check = "prompt"` is set explicitly +- **AND** the check is enabled - **WHEN** `lstk start` runs interactively with a newer version available - **THEN** no update prompt is presented - **AND** a note naming mise is emitted instead @@ -36,7 +36,7 @@ The refusal SHALL be enforced at the point the binary is actually replaced, not `lstk update --check` SHALL be exempt from the refusal: it reports whether a newer version exists and writes nothing, which is useful however lstk was installed. -lstk SHALL also refuse an in-place binary replacement when the resolved executable's directory cannot be written to, under the same error code, reporting the directory instead of a manager. Homebrew and npm installs SHALL never be refused on this basis, since they delegate to `brew upgrade` and `npm install -g` rather than writing the file themselves. A probe that cannot determine writability SHALL NOT refuse. +lstk SHALL also refuse an in-place binary replacement when the resolved executable's directory cannot be written to, under the same error code, reporting the directory instead of a manager. When a release build finds no bundled extensions beside it, the reinstall hint SHALL name the managing tool for an externally-managed install rather than pointing at a release download, which would install outside that tool. Homebrew and npm installs SHALL never be refused on this basis, since they delegate to `brew upgrade` and `npm install -g` rather than writing the file themselves. A probe that cannot determine writability SHALL NOT refuse. `lstk update --force` SHALL bypass the refusal and update as if the install were a standalone binary, so that a misidentified install is never left without a path forward. @@ -70,12 +70,12 @@ lstk SHALL also refuse an in-place binary replacement when the resolved executab ### Requirement: Detection does not run on invocations that do not check for updates Install-method detection SHALL NOT be performed on any code path that does not otherwise reach the automatic update check or the update command. -Within the **automatic start-path check** it SHALL NOT run when the resolved mode is `off`, or before the version check has reported that a newer version is available. This scoping is deliberate: `lstk update` detects unconditionally and up front, because its refusal must precede the version check and the download (see the requirement below), and that is a different code path with a different cost profile. +Within the **automatic start-path check** it SHALL NOT run when the check is disabled, or before the version check has reported that a newer version is available. This scoping is deliberate: `lstk update` detects unconditionally and up front, because its refusal must precede the version check and the download (see the requirement below), and that is a different code path with a different cost profile. -It SHALL run once an update *is* known to exist, whatever the mode (other than `off`) and whether or not the call site can prompt: its answer determines the note's wording as well as the prompt/note decision, so skipping it for an explicitly-set mode produced a note advising `lstk update` on an install where that command refuses. +It SHALL run once an update *is* known to exist, whether or not the call site can prompt: its answer determines the note's wording as well as the prompt/note decision. -#### Scenario: off never reaches detection -- **GIVEN** `[cli] update_check = "off"` +#### Scenario: A disabled check never reaches detection +- **GIVEN** `[cli] check_for_update_on_startup = false` - **WHEN** `lstk start` runs - **THEN** no version check is performed and install-method detection is not consulted diff --git a/openspec/changes/add-update-check-config/specs/update-check-config/spec.md b/openspec/changes/add-update-check-config/specs/update-check-config/spec.md index e34a8ff0..7881ad3e 100644 --- a/openspec/changes/add-update-check-config/specs/update-check-config/spec.md +++ b/openspec/changes/add-update-check-config/specs/update-check-config/spec.md @@ -1,61 +1,62 @@ ## ADDED Requirements ### Requirement: A persistent setting controls the automatic update check -lstk SHALL read an update-check mode from the `[cli] update_check` key in `config.toml` and from the `LSTK_UPDATE_CHECK` environment variable, accepting exactly the values `prompt`, `notify`, and `off`. The mode SHALL govern the automatic update check performed on the start path, and SHALL NOT govern an explicit `lstk update` invocation. +lstk SHALL read a boolean from the `[cli] check_for_update_on_startup` key in `config.toml` and from the `LSTK_CHECK_FOR_UPDATE_ON_STARTUP` environment variable. It SHALL govern the automatic update check performed on the start path, and SHALL NOT govern an explicit `lstk update` invocation. -Resolution order, first match wins: `LSTK_UPDATE_CHECK`, then `[cli] update_check`, then `notify` when the install is detected as externally managed (see the `external-install-detection` capability), then `prompt`. +Resolution order, first match wins: the environment variable, then the config key, then a default of `true`. -Behavior per mode: +Behavior: -- `prompt` — lstk checks for a newer version and, on an interactive start, presents the blocking update choice. -- `notify` — lstk checks for a newer version and emits a single non-blocking note naming the current and latest version and how to update. No prompt is presented and nothing waits for input. When the install is externally managed, the note SHALL name that manager rather than advising `lstk update`, which refuses on such an install — regardless of how the mode was reached, since the advice is equally wrong either way. +- **enabled** (default) — lstk checks for a newer version. On an interactive start of a self-managed install it presents the blocking update choice; on an externally-managed install, or when the call site cannot prompt, it emits a single non-blocking note instead. +- **disabled** — lstk performs no version check at all: no network request is made and no update-related output is emitted. -An externally-managed install SHALL NOT be prompted even under `prompt`: applying the update is refused on such an install, and offering an action that cannot be carried out is worse than not offering it. `prompt` therefore behaves as `notify` there. +It is a boolean rather than a three-mode enum because install detection already decides between prompting and a note; the only choice left to the user is whether to check at all. -- `off` — lstk performs no version check at all: no network request is made and no update-related output is emitted. - -An unrecognized value SHALL be rejected as a configuration error naming the key and the accepted values, rather than silently falling back to a default. +A value that is not a boolean SHALL be rejected as a configuration error naming the key and the accepted values, rather than silently falling back to a default. An unset key SHALL stay distinguishable from an explicit `false`, so the default can apply. #### Scenario: Default behavior is unchanged -- **WHEN** neither `LSTK_UPDATE_CHECK` nor `[cli] update_check` is set and the install is not detected as externally managed -- **THEN** the resolved mode is `prompt` -- **AND** an interactive `lstk start` with a newer version available presents the blocking update choice, as before this change - -#### Scenario: notify never blocks -- **GIVEN** `[cli] update_check = "notify"` -- **WHEN** `lstk start` runs interactively and a newer version is available -- **THEN** a single note naming both versions is emitted +- **WHEN** neither the environment variable nor the config key is set +- **THEN** the check is enabled +- **AND** an interactive `lstk start` on a self-managed install with a newer version available presents the blocking update choice, as before this change + +#### Scenario: An externally-managed install is never prompted +- **GIVEN** the check is enabled +- **WHEN** `lstk start` runs interactively on an install recognized as externally managed and a newer version is available +- **THEN** a single note naming the managing tool is emitted - **AND** no prompt is presented and the start proceeds without waiting for input -#### Scenario: off makes no network request -- **GIVEN** `[cli] update_check = "off"` +#### Scenario: A non-interactive start emits a note +- **GIVEN** the check is enabled +- **WHEN** `lstk start --non-interactive` runs and a newer version is available +- **THEN** a single note naming both versions is emitted and nothing waits for input + +#### Scenario: Disabled makes no network request +- **GIVEN** `[cli] check_for_update_on_startup = false` - **WHEN** `lstk start` runs - **THEN** no request is made to the release API - **AND** no update-related output is emitted in either interactive or non-interactive mode #### Scenario: Environment variable overrides config -- **GIVEN** `[cli] update_check = "off"` in `config.toml` -- **WHEN** `lstk start` runs with `LSTK_UPDATE_CHECK=prompt` -- **THEN** the resolved mode is `prompt` +- **GIVEN** `[cli] check_for_update_on_startup = false` in `config.toml` +- **WHEN** `lstk start` runs with `LSTK_CHECK_FOR_UPDATE_ON_STARTUP=true` +- **THEN** the check is enabled -#### Scenario: Invalid value is rejected -- **WHEN** `lstk start` runs with `[cli] update_check = "quiet"` -- **THEN** lstk exits non-zero with a configuration error naming `update_check` and the values `prompt`, `notify`, `off` +#### Scenario: A non-boolean value is rejected +- **WHEN** `lstk start` runs with `[cli] check_for_update_on_startup = "quiet"` +- **THEN** lstk exits non-zero with a configuration error naming `check_for_update_on_startup` and the accepted values - **AND** the emulator is not started #### Scenario: The setting does not disable the explicit update command -- **GIVEN** `[cli] update_check = "off"` +- **GIVEN** `[cli] check_for_update_on_startup = false` - **WHEN** `lstk update --check` is run - **THEN** the version check is performed and its result reported as usual ### Requirement: The update prompt offers exactly three choices -The blocking update prompt SHALL offer "Update now", "Remind me next time", and "Never ask again" — and no per-version "Skip this version" option. A skipped version bought a few days of quiet against a weekly release cadence (the complaint behind DEVX-1029) while adding a third flavour of "no" to the prompt and the only piece of per-version persisted state; the permanent opt-out serves the same need without either cost. `cli.update_skipped_version` is removed with it. - -"Never ask again" SHALL be offered **only when the setting can actually be persisted** — i.e. when a config file exists. On a first run config.toml has not been created yet (the emulator picker creates it later), and an option whose effect would be silently dropped SHALL NOT be offered. +The blocking update prompt SHALL offer "Update now", "Remind me next time", and "Never check again" — and no per-version "Skip this version" option. A skipped version bought a few days of quiet against a weekly release cadence (the complaint behind DEVX-1029) while adding a third flavour of "no" to the prompt and the only piece of per-version persisted state; the permanent opt-out serves the same need without either cost. `cli.update_skipped_version` is removed with it. -Selecting it SHALL persist `cli.update_check = "notify"` to the config file in use, preserving the file's existing comments and formatting, and SHALL NOT apply an update. +"Never check again" SHALL be offered **only when the setting can actually be persisted** — i.e. when a config file exists. On a first run config.toml has not been created yet (the emulator picker creates it), and an option whose effect would be silently dropped SHALL NOT be offered. -A failure to persist SHALL be surfaced as a warning and SHALL NOT be reported as success. +Selecting it SHALL persist `cli.check_for_update_on_startup = false` to the config file in use, preserving the file's existing comments and formatting, and SHALL NOT apply an update. A failure to persist SHALL be surfaced as a warning and SHALL NOT be reported as success. #### Scenario: The opt-out is not offered when it cannot be persisted - **GIVEN** no config file exists yet (a first run) @@ -63,15 +64,15 @@ A failure to persist SHALL be surfaced as a warning and SHALL NOT be reported as - **THEN** it offers only "Update now" and "Remind me next time" - **AND** no option claims a preference was saved -#### Scenario: Never ask again persists the setting -- **GIVEN** an interactive `lstk start` with a newer version available and the mode resolved to `prompt` -- **WHEN** the user selects "Never ask again" -- **THEN** `cli.update_check` is written as `notify` to the config file reported by `lstk config path` +#### Scenario: Never check again persists the setting +- **GIVEN** an interactive `lstk start` with a newer version available and a config file present +- **WHEN** the user selects "Never check again" +- **THEN** `cli.check_for_update_on_startup` is written as `false` to the config file reported by `lstk config path` - **AND** no update is applied -- **AND** a subsequent `lstk start` with a newer version available emits a note instead of a prompt +- **AND** a subsequent `lstk start` performs no check at all #### Scenario: Persisting the opt-out fails - **GIVEN** the config file cannot be written -- **WHEN** the user selects "Never ask again" +- **WHEN** the user selects "Never check again" - **THEN** a warning is emitted naming the failure - **AND** the command continues and exits as it otherwise would diff --git a/openspec/changes/add-update-check-config/tasks.md b/openspec/changes/add-update-check-config/tasks.md index 940116da..4cb77c1b 100644 --- a/openspec/changes/add-update-check-config/tasks.md +++ b/openspec/changes/add-update-check-config/tasks.md @@ -105,3 +105,17 @@ Write the tests in this section before the implementation — 2.1's reordering i - `internal/ui/app_test.go`'s `{Key: "s", Label: "Skip this version"}` fixture is pre-existing arbitrary sample data for a component test, unrelated to this prompt. - `cmd/root.go`'s invalid-value action always advises unsetting the env var, which would misdescribe a bad *config* key — unreachable, because `config.Get()` rejects that one statement earlier. The two validations are redundant on that path by design: `Get()` covers every command, the env branch covers only the start path. - For an unwritable install directory the offered `--force` will itself fail at the rename. Spec-mandated: `--force` exists for the path-marker heuristic, and suppressing it per-reason would make the flag's contract conditional. + +## 11. Review: simplify the config option to a boolean + +- [x] 11.1 Replace `[cli] update_check` (`prompt`/`notify`/`off`) with `[cli] check_for_update_on_startup` (boolean, default true), and `LSTK_UPDATE_CHECK` with `LSTK_CHECK_FOR_UPDATE_ON_STARTUP`, per review. Detection alone now decides prompt vs note. +- [x] 11.2 `CLIConfig.CheckForUpdateOnStartup` is a `*bool` so an unset key stays distinguishable from an explicit false; the env var stays a raw string for the same reason. +- [x] 11.3 Validate a non-boolean config value before unmarshal: mapstructure's own failure names the key but neither the offending value nor the accepted ones, and the environment variable's message should match. +- [x] 11.4 `NotifyOptions.Mode` becomes `CheckEnabled bool`, deleting the `mode == notify` branch. `internal/update` no longer imports `internal/config`. +- [x] 11.5 The prompt's opt-out becomes "Never check again" and persists `false` — a stronger commitment than the previous "Never ask again" → notify; recorded in design.md. +- [x] 11.6 Retire the two unit tests for a state that no longer exists (explicit `notify` on a self-managed install): one becomes "an enabled check prompts a self-managed install", the other moves to the non-interactive path where the `lstk update` note still appears. + +## 12. Interaction with #482 (bundled extensions) + +- [x] 12.1 Restore `InstallMethod.String()`, deleted here as dead code while #482 landed a new caller in parallel. Documented alongside `appliedMethodName`, which names how an update was *performed* for the `--json` envelope rather than how lstk was installed. +- [x] 12.2 `detectMissingBundle` guarded on `InstallBinary`, so an externally-managed install silently lost the missing-bundle hint #482 introduced. It now covers `InstallExternal` and names the managing tool instead of pointing at a release download, which would install outside that tool. diff --git a/test/integration/update_check_test.go b/test/integration/update_check_test.go index c701266d..54295dc7 100644 --- a/test/integration/update_check_test.go +++ b/test/integration/update_check_test.go @@ -130,20 +130,20 @@ func TestInvalidUpdateCheckInConfigIsRejected(t *testing.T) { t.Parallel() // "quiet" is not a valid mode; it stands in for a plausible-sounding typo. - configFile := writeConfigWithCLI(t, `update_check = "quiet"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = "quiet"`) stdout, stderr, err := runLstk(t, testContext(t), t.TempDir(), testEnvWithHome(t.TempDir(), ""), "--config", configFile, "volume", "path") requireExitCode(t, 1, err) combined := stdout + stderr - assert.Contains(t, combined, "update_check") + assert.Contains(t, combined, "check_for_update_on_startup") assert.Contains(t, combined, "quiet") } func TestValidUpdateCheckInConfigIsAccepted(t *testing.T) { t.Parallel() - configFile := writeConfigWithCLI(t, `update_check = "off"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = false`) _, stderr, err := runLstk(t, testContext(t), t.TempDir(), testEnvWithHome(t.TempDir(), ""), "--config", configFile, "volume", "path") @@ -224,33 +224,33 @@ func startEnv(t *testing.T, srv *httptest.Server, extra ...string) []string { return append(e, extra...) } -func TestUpdateCheckOffMakesNoRequestAndSaysNothing(t *testing.T) { +func TestUpdateCheckDisabledMakesNoRequestAndSaysNothing(t *testing.T) { t.Parallel() var hits atomic.Int32 srv := countingReleaseServer(t, "v9.9.9", &hits) bin := buildStampedLstk(t, "opt/bin", "0.0.1") - configFile := writeConfigWithCLI(t, `update_check = "off"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = false`) stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv), bin, "--config", configFile, "start", "--non-interactive") - assert.Equal(t, int32(0), hits.Load(), "off must make no request to the release API") + assert.Equal(t, int32(0), hits.Load(), "a disabled check must make no request to the release API") assert.NotContains(t, stdout+stderr, "Update available") } -func TestUpdateCheckNotifyEmitsNoteWithoutBlocking(t *testing.T) { +func TestUpdateCheckEnabledEmitsNoteNonInteractively(t *testing.T) { t.Parallel() var hits atomic.Int32 srv := countingReleaseServer(t, "v9.9.9", &hits) bin := buildStampedLstk(t, "opt/bin", "0.0.1") - configFile := writeConfigWithCLI(t, `update_check = "notify"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = true`) stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv), bin, "--config", configFile, "start", "--non-interactive") - assert.Equal(t, int32(1), hits.Load(), "notify must still check") + assert.Equal(t, int32(1), hits.Load(), "an enabled check still reaches the release API") assert.Contains(t, stdout+stderr, "Update available: 0.0.1 → v9.9.9") } @@ -260,10 +260,10 @@ func TestUpdateCheckEnvVarOverridesConfig(t *testing.T) { var hits atomic.Int32 srv := countingReleaseServer(t, "v9.9.9", &hits) bin := buildStampedLstk(t, "opt/bin", "0.0.1") - configFile := writeConfigWithCLI(t, `update_check = "notify"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = true`) stdout, stderr, _ := runBinary(t, t.TempDir(), - startEnv(t, srv, "LSTK_UPDATE_CHECK=off"), bin, + startEnv(t, srv, "LSTK_CHECK_FOR_UPDATE_ON_STARTUP=false"), bin, "--config", configFile, "start", "--non-interactive") assert.Equal(t, int32(0), hits.Load(), "the env var must win over the config key") @@ -278,7 +278,7 @@ func TestUpdateCheckNoteNamesExternalManager(t *testing.T) { var hits atomic.Int32 srv := countingReleaseServer(t, "v9.9.9", &hits) bin := buildStampedLstk(t, ".local/share/mise/installs/github-localstack-lstk/latest", "0.0.1") - configFile := writeConfigWithCLI(t, `update_check = "notify"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = true`) stdout, stderr, _ := runBinary(t, t.TempDir(), startEnv(t, srv), bin, "--config", configFile, "start", "--non-interactive") @@ -294,10 +294,10 @@ func TestInvalidUpdateCheckEnvVarIsRejectedAsConfigInvalid(t *testing.T) { var hits atomic.Int32 srv := countingReleaseServer(t, "v9.9.9", &hits) bin := buildStampedLstk(t, "opt/bin", "0.0.1") - configFile := writeConfigWithCLI(t, `update_check = "notify"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = true`) stdout, _, err := runBinary(t, t.TempDir(), - startEnv(t, srv, "LSTK_UPDATE_CHECK=quiet"), bin, + startEnv(t, srv, "LSTK_CHECK_FOR_UPDATE_ON_STARTUP=quiet"), bin, "--config", configFile, "start", "--non-interactive", "--json") requireExitCode(t, 1, err) @@ -313,7 +313,7 @@ func TestInvalidUpdateCheckEnvVarIsRejectedAsConfigInvalid(t *testing.T) { func TestInvalidUpdateCheckInConfigIsConfigInvalidUnderJSON(t *testing.T) { t.Parallel() - configFile := writeConfigWithCLI(t, `update_check = "quiet"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = "quiet"`) stdout, _, err := runLstk(t, testContext(t), t.TempDir(), append(testEnvWithHome(t.TempDir(), ""), unreachableDockerHost), "--config", configFile, "start", "--non-interactive", "--json") @@ -328,7 +328,7 @@ func TestInvalidUpdateCheckInConfigIsConfigInvalidUnderJSON(t *testing.T) { assert.Equal(t, "CONFIG_INVALID", envelope.Error.Code) } -// The "Never ask again" option writes config, so it must only appear when +// The "Never check again" option writes config, so it must only appear when // there is a config file to write to. On a genuine first run config.toml does // not exist yet — it is created later, by the emulator picker — so offering it // there would tell the user a preference was saved when it was dropped. @@ -351,7 +351,7 @@ func TestUpdatePromptOmitsNeverAskAgainOnFirstRun(t *testing.T) { out := proc.output() assert.Contains(t, out, "Update now") assert.Contains(t, out, "Remind me next time") - assert.NotContains(t, out, "Never ask again", "the opt-out must be absent with no config file") + assert.NotContains(t, out, "Never check again", "the opt-out must be absent with no config file") } func TestUpdatePromptOffersNeverAskAgainWhenConfigExists(t *testing.T) { @@ -360,7 +360,7 @@ func TestUpdatePromptOffersNeverAskAgainWhenConfigExists(t *testing.T) { var hits atomic.Int32 srv := countingReleaseServer(t, "v9.9.9", &hits) bin := buildStampedLstk(t, "opt/bin", "0.0.1") - configFile := writeConfigWithCLI(t, `update_check = "prompt"`) + configFile := writeConfigWithCLI(t, `check_for_update_on_startup = true`) cmd := exec.Command(bin, "--config", configFile, "start") cmd.Env = startEnv(t, srv) @@ -368,7 +368,7 @@ func TestUpdatePromptOffersNeverAskAgainWhenConfigExists(t *testing.T) { t.Cleanup(proc.kill) proc.waitForOutput("Update lstk to latest version?", "the update prompt should appear") - proc.waitForOutput("Never ask again", "the opt-out must be offered when config exists") + proc.waitForOutput("Never check again", "the opt-out must be offered when config exists") } // --check reports whether a newer version exists and writes nothing, so it is diff --git a/test/integration/update_test.go b/test/integration/update_test.go index 8a3c0e42..484cd3fc 100644 --- a/test/integration/update_test.go +++ b/test/integration/update_test.go @@ -365,7 +365,7 @@ func TestUpdateNotification(t *testing.T) { mockServer := createMockLicenseServer(false) t.Cleanup(mockServer.Close) - t.Run("never ask again", func(t *testing.T) { + t.Run("never check again", func(t *testing.T) { t.Parallel() configFile := filepath.Join(t.TempDir(), "config.toml") originalConfig := `# User-maintained lstk config @@ -384,7 +384,7 @@ port = "4566" # Host port p := startCmdInPTY(t, ctx, cmd) p.waitForOutput("New lstk version available", "update notification prompt should appear") - // "Never ask again" replaced "Skip this version" as the prompt's + // "Never check again" replaced "Skip this version" as the prompt's // config-writing option; this test is about the write preserving the // user's file, not about which preference is written. p.write("n") @@ -395,7 +395,7 @@ port = "4566" # Host port configData, err := os.ReadFile(configFile) require.NoError(t, err) configStr := string(configData) - assert.Contains(t, configStr, "update_check", "the chosen preference should be persisted") + assert.Contains(t, configStr, "check_for_update_on_startup", "the chosen preference should be persisted") assert.Contains(t, configStr, "# User-maintained lstk config", "file header comment should be preserved") assert.Contains(t, configStr, "# Emulator type", "inline comments should be preserved") assert.Contains(t, configStr, `port = "4566"`, "existing config values should be preserved")