diff --git a/CLAUDE.md b/CLAUDE.md index e6e44c6d..3ebfc08b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -203,6 +203,9 @@ The release job (`.github/workflows/ci.yml`) builds the npm packages with `gorel # Code Style - Don't add comments for self-explanatory code. Only comment when the "why" isn't obvious from the code itself. +- Keep comments short: 1–3 lines is the norm, ~6 is a ceiling that needs a real reason (a subtle invariant, upstream behaviour that will surprise the reader, a rejected alternative someone will otherwise re-attempt). Long comments are harder to read and go stale — they are a maintenance burden, not extra rigor. +- State each reason once, in the fewest words that carry it. Don't restate the code, the declaration's own name, or what the adjacent test already asserts, and don't narrate why the obvious path is safe. Where the design took some working out, record the constraint that came out of it rather than the path that led there — the constraint is what a future reader needs in order not to break it. +- If a declaration seems to need several unrelated paragraphs, that's a signal to split the code — not to grow the comment. - Do not remove comments added by someone else than yourself. - Errors returned by functions should always be checked unless in test files. - Terminology: in user-facing CLI/help/docs, prefer `emulator` over `container`/`runtime`; use `container`/`runtime` only for internal implementation details. @@ -220,6 +223,8 @@ The release job (`.github/workflows/ci.yml`) builds the npm packages with `gorel 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. +Only Homebrew installs wire up completion automatically (`homebrew_casks.completions` in `.goreleaser.yaml`), so the first successful *interactive* start emits a one-line `> Tip:` pointing at `lstk completion [bash|zsh|fish|powershell]` and the docs. It is only a pointer — lstk never writes to the user's shell config, and there is no install flag (rationale on the `completionTip` const in `internal/ui/run.go`). A second such nudge must be gated the same way: `firstRun` only, interactive only, after the emulator is up. + `lstk aws ` completes AWS services/operations/parameters by delegating to the AWS CLI's own `aws_completer` from a `ValidArgsFunction` on the `aws` command (DEVX-846) — `awscli.Complete` in `internal/awscli/complete.go`, wired in `cmd/aws.go`. Going through Cobra rather than registering `complete -C aws_completer lstk` is what makes it work in every shell `lstk completion` supports (the native registration is bash/zsh-only) and keeps the bash fallback above unchanged. `aws_completer` speaks bash's `complete -C` protocol: `COMP_LINE`/`COMP_POINT` in, candidates one-per-line out. Two constraints it imposes: the line must start with the literal word `aws` (the completer drops the first word before matching, so `lstk aws s3 l` returns nothing), and `COMP_POINT` is a **character** offset, not a byte one. Cobra never runs `PreRunE` on the `__complete` path, so completion stays offline — no config load, no Docker health check, no endpoint resolution — which matches the completer, which never contacts an endpoint. A missing or failing completer must return `ShellCompDirectiveDefault` and print nothing: any output on this path is read by the shell as a candidate. The Tab-press timeout is set by the caller in `cmd/aws.go` (`awsCompletionTimeout`), not inside `awscli.Complete` — an internal deadline made the unit test flaky on cold CI runs. That deadline only actually bounds a Tab press because `Complete` also sets `cmd.WaitDelay`: `exec.CommandContext` kills the completer on expiry but does not close a pipe the completer's own children still hold, so a completer leaving a grandchild on stdout (a wrapper script that forks instead of exec'ing — what `/bin/sh` does on Linux, where it is dash) made `cmd.Output()` block for the grandchild's full lifetime regardless of the context. Keep `WaitDelay` set on any similar short captured-output exec. `lstk az` needs the same treatment but a different protocol (argcomplete: `_ARGCOMPLETE=1`, output on fd 8). # CLI Help Text @@ -346,7 +351,7 @@ When making significant changes to the codebase (new commands, architectural cha **This is the only agent-instruction file in the repo.** Where each kind of detail should go instead: -- **Anything about a single declaration** (mechanism, rationale, invariants, why an obvious alternative was rejected, upstream/external behaviour it depends on) → a doc comment on that function, type, method, field, or constant. Negative statements work fine there too: anchor "there is deliberately no X" to the function where X would have gone. +- **Details about one function, type, method, field, or constant** → a doc comment on it, not here: how it works, why it's built that way, what has to stay true, why the obvious alternative doesn't work, and any outside behaviour it depends on. Same for what deliberately isn't there — put "there is no X, on purpose" on the function where X would have gone. Keep it concise (few lines). - **User-facing config reference** → `internal/config/default_config.toml`, which ships as the user's own commented config. - **User-facing command reference** → the command's Cobra `Short`/`Long` in `cmd/`, which is also what `lstk docs` renders. - **Design rationale and non-goals for in-flight work** → `openspec/changes//design.md`. diff --git a/cmd/root.go b/cmd/root.go index 5917e166..fa4064c7 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -351,6 +351,11 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t logger.Info("could not resolve friendly config path: %v", err) } + // Captured before ApplyEmulatorType clears firstRun to skip the picker and + // default-emulator notice. `lstk start --type aws` on a fresh install is + // still a first run, so the completion tip must fire there too. + wasFirstRun := firstRun + // Apply the --type flag before resolving snapshot and start options so // everything downstream reflects the selected emulator. Uses the caller's // sink even in interactive mode, since the config mutation has to happen @@ -394,6 +399,7 @@ func startEmulator(ctx context.Context, rt runtime.Runtime, cfg *env.Env, tel *t ConfigPath: configPath, EmulatorLabel: config.CachedPlanLabel(), NeedsEmulatorSelection: firstRun, + CompletionTip: wasFirstRun, PostStart: autoLoad, }) } diff --git a/internal/ui/run.go b/internal/ui/run.go index 06958d7d..8cdd763d 100644 --- a/internal/ui/run.go +++ b/internal/ui/run.go @@ -41,8 +41,25 @@ type RunOptions struct { // auto-load a configured snapshot). It is skipped when the emulator was // already running. PostStart func(ctx context.Context, sink output.Sink) error + // CompletionTip shows completionTip once the emulator is up. The caller + // decides when it applies (first run), so this package holds no policy. + CompletionTip bool } +// completionTip points at the completion scripts lstk ships. Only Homebrew +// installs wire them up automatically (homebrew_casks.completions in +// .goreleaser.yaml), so npm and binary users never find them. First run is the +// trigger because no install path offers a usable hook (generated npm +// package.json, no hook at all for binaries) and it needs no new persisted +// state: config.toml was absent, and that same run creates it. +// +// Must stay a plain MessageEvent, not a DeferredEvent — Run does not render +// DeferredOutput (only runWithTUI does), so a deferred event would be dropped. +// The "> Tip: " prefix, SeveritySecondary, and verb-colon-command wording match +// tipsForType (internal/container/start.go), whose tip renders right above it. +const completionTip = "> Tip: Enable tab completion for your shell: lstk completion [bash|zsh|fish|powershell] " + + "See https://docs.localstack.cloud/aws/developer-tools/running-localstack/lstk/#shell-completions" + func Run(parentCtx context.Context, runOpts RunOptions) error { ctx, cancel := context.WithCancel(parentCtx) defer cancel() @@ -138,6 +155,9 @@ func Run(parentCtx context.Context, runOpts RunOptions) error { } else { go container.ResolveAndCacheLabel(ctx, runOpts.StartOptions, result.Version, labelCh) } + if runOpts.CompletionTip { + sink.Emit(output.MessageEvent{Severity: output.SeveritySecondary, Text: completionTip}) + } p.Send(runDoneMsg{}) }() diff --git a/test/integration/awsconfig_test.go b/test/integration/awsconfig_test.go index ee0d1a46..fb509005 100644 --- a/test/integration/awsconfig_test.go +++ b/test/integration/awsconfig_test.go @@ -2,7 +2,6 @@ package integration_test import ( "os" - "os/exec" "path/filepath" "runtime" "strings" @@ -27,16 +26,7 @@ import ( func awsConfigEnv(t *testing.T) (env.Environ, string) { t.Helper() tmpHome := t.TempDir() - // Runs before t.TempDir() cleanup (LIFO order). The emulator runs as root - // inside the container, so files it writes into the volume are root-owned on - // Linux. Go's TempDir cleanup can't delete them, so we use a Docker container - // to remove them first. - t.Cleanup(func() { - volumeDir := filepath.Join(tmpHome, ".cache", "lstk", "volume") - if _, err := os.Stat(volumeDir); err == nil { - _ = exec.Command("docker", "run", "--rm", "-v", volumeDir+":/d", "alpine", "sh", "-c", "rm -rf /d/*").Run() - } - }) + scheduleVolumeCleanup(t, tmpHome) writeConfigFile(t, filepath.Join(tmpHome, ".config", "lstk", "config.toml")) e := env.With(env.AuthToken, env.Get(env.AuthToken)).WithHome(tmpHome) return e, tmpHome diff --git a/test/integration/completion_tip_test.go b/test/integration/completion_tip_test.go new file mode 100644 index 00000000..efca0c76 --- /dev/null +++ b/test/integration/completion_tip_test.go @@ -0,0 +1,165 @@ +package integration_test + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + "time" + + "github.com/localstack/lstk/test/integration/env" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// completionTipText is asserted verbatim, "> Tip: " prefix included: that +// prefix is the convention the neighbouring post-start tips use (tipsForType in +// internal/container/start.go), so it is observable behavior, not styling. +const completionTipText = "> Tip: Enable tab completion for your shell: lstk completion [bash|zsh|fish|powershell] " + + "See https://docs.localstack.cloud/aws/developer-tools/running-localstack/lstk/#shell-completions" + +// firstRunHome returns an isolated home with no lstk config, so the run under +// test is a first run (config.toml absent is what firstRun means). +func firstRunHome(t *testing.T) (env.Environ, string) { + t.Helper() + + tmpHome := t.TempDir() + scheduleVolumeCleanup(t, tmpHome) + require.NoError(t, os.MkdirAll(filepath.Join(tmpHome, ".config"), 0755)) + e := env.Environ(testEnvWithHome(tmpHome, tmpHome)).With(env.DisableEvents, "1") + + configPath, _, err := runLstk(t, testContext(t), "", e, "config", "path") + require.NoError(t, err) + require.NoFileExists(t, configPath, "test setup: config must be absent for this to be a first run") + + return e, configPath +} + +func TestFirstRunShowsCompletionTip(t *testing.T) { + requireDocker(t) + _ = env.Require(t, env.AuthToken) + + cleanup() + t.Cleanup(cleanup) + + mockServer := createMockLicenseServer(true) + defer mockServer.Close() + + e, _ := firstRunHome(t) + + p := startLstkInPTY(t, testContext(t), e.With(env.APIEndpoint, mockServer.URL), "start") + + // First run shows the emulator picker; accept the default (AWS). + p.waitForOutput("Which emulator would you like to use?", "emulator selection prompt should appear on first run") + p.write("\r") + + // Post-start setup asks about the AWS CLI profile; decline it. + p.waitForOutputTimeout(awsSetupPrompt, 2*time.Minute, "container should become ready") + p.write("n") + + out, err := p.wait() + require.NoError(t, err, "lstk start should exit successfully") + + assert.Contains(t, out, completionTipText, "first successful interactive start should point at shell completion setup") +} + +// --type answers the first-run picker, so it suppresses it — but the run is +// still a first run and the tip must survive that. +func TestFirstRunWithEmulatorTypeFlagShowsCompletionTip(t *testing.T) { + requireDocker(t) + _ = env.Require(t, env.AuthToken) + + cleanup() + t.Cleanup(cleanup) + + mockServer := createMockLicenseServer(true) + defer mockServer.Close() + + e, _ := firstRunHome(t) + + p := startLstkInPTY(t, testContext(t), e.With(env.APIEndpoint, mockServer.URL), "start", "--type", "aws") + + assert.NotContains(t, p.output(), "Which emulator would you like to use?", + "--type should answer the picker rather than showing it") + + p.waitForOutputTimeout(awsSetupPrompt, 2*time.Minute, "container should become ready") + p.write("n") + + out, err := p.wait() + require.NoError(t, err, "lstk start --type aws should exit successfully") + + assert.Contains(t, out, completionTipText, "--type must not suppress the first-run tip along with the picker") +} + +func TestSubsequentRunDoesNotShowCompletionTip(t *testing.T) { + requireDocker(t) + _ = env.Require(t, env.AuthToken) + + cleanup() + t.Cleanup(cleanup) + + mockServer := createMockLicenseServer(true) + defer mockServer.Close() + + e, configPath := firstRunHome(t) + + // Pre-create the config so this is no longer a first run. + require.NoError(t, os.MkdirAll(filepath.Dir(configPath), 0755)) + require.NoError(t, os.WriteFile(configPath, + []byte("[[containers]]\ntype = \"aws\"\ntag = \"latest\"\nport = \"4566\"\n"), 0644)) + + p := startLstkInPTY(t, testContext(t), e.With(env.APIEndpoint, mockServer.URL), "start") + + p.waitForOutputTimeout(awsSetupPrompt, 2*time.Minute, "container should become ready") + p.write("n") + + out, err := p.wait() + require.NoError(t, err, "lstk start should exit successfully") + + assert.NotContains(t, out, completionTipText, "the tip must not repeat once lstk has been configured") +} + +func TestFirstRunNonInteractiveShowsNoCompletionTip(t *testing.T) { + requireDocker(t) + _ = env.Require(t, env.AuthToken) + + cleanup() + t.Cleanup(cleanup) + + mockServer := createMockLicenseServer(true) + defer mockServer.Close() + + e, _ := firstRunHome(t) + + stdout, stderr, err := runLstk(t, testContext(t), "", e.With(env.APIEndpoint, mockServer.URL), "start") + require.NoError(t, err, "lstk start failed: %s", stderr) + + assert.Contains(t, stdout, "Configured with default emulator", "test setup: this should be a first run") + assert.NotContains(t, stdout, completionTipText, "a tip is noise in non-interactive output") +} + +func TestFirstRunJSONEnvelopeHasNoCompletionTip(t *testing.T) { + requireDocker(t) + _ = env.Require(t, env.AuthToken) + + cleanup() + t.Cleanup(cleanup) + + mockServer := createMockLicenseServer(true) + defer mockServer.Close() + + e, _ := firstRunHome(t) + + stdout, stderr, err := runLstk(t, testContext(t), "", e.With(env.APIEndpoint, mockServer.URL), "start", "--json") + require.NoError(t, err, "lstk start --json failed: %s", stderr) + + // A distinctive substring, not completionTipText: inside a JSON string, + // escaping could break an exact match and hide the leak. + assert.NotContains(t, stdout, "tab completion", "the tip must never reach machine-readable output") + + envelope := decodeEnvelope(t, stdout) + assert.Equal(t, "ok", envelope.Status) + var data startJSONData + require.NoError(t, json.Unmarshal(envelope.Data, &data)) + assert.Equal(t, "aws", data.Emulator, "test setup: the first run should have started the default emulator") +} diff --git a/test/integration/main_test.go b/test/integration/main_test.go index f8b41d36..ba92d5ce 100644 --- a/test/integration/main_test.go +++ b/test/integration/main_test.go @@ -176,6 +176,24 @@ const ( testImage = "alpine:latest" ) +// scheduleVolumeCleanup removes the emulator volume under an isolated tmpHome. +// Call it right after t.TempDir(): t.Cleanup runs LIFO, so registering it later +// is what makes it run before TempDir's own cleanup. The emulator runs as root +// in the container, so on Linux its volume files are root-owned and TempDir +// cleanup cannot unlink them (Docker Desktop on macOS maps them to the calling +// user); deleting them from inside a container sidesteps that. Needed by any +// test that isolates HOME under t.TempDir() and starts a real emulator. +func scheduleVolumeCleanup(t *testing.T, tmpHome string) { + t.Helper() + t.Cleanup(func() { + volumeDir := filepath.Join(tmpHome, ".cache", "lstk", "volume") + if _, err := os.Stat(volumeDir); err != nil { + return + } + _ = exec.Command("docker", "run", "--rm", "-v", volumeDir+":/d", "alpine", "sh", "-c", "rm -rf /d/*").Run() + }) +} + // startTestContainer starts the test container with no port bindings by default. // Pass hostPort to bind 4566/tcp to a specific host port (e.g. to test that lstk status // uses the actual bound port rather than the port from config).