From 3888fb1e66c4ab113da19ce426d9be7c192c8e1d Mon Sep 17 00:00:00 2001 From: Misha Tiurin Date: Thu, 13 Aug 2026 18:16:37 +0200 Subject: [PATCH 1/6] Build prompts from intent constructors instead of a layout flag Co-Authored-By: Claude --- .claude/skills/review-pr/SKILL.md | 1 + CLAUDE.md | 16 ++- internal/auth/login.go | 6 +- internal/awsconfig/awsconfig.go | 8 +- internal/container/select.go | 7 +- internal/container/start.go | 39 +++--- internal/container/start_test.go | 19 ++- internal/output/events.go | 10 +- internal/output/plain_format.go | 34 ++++- internal/output/prompt.go | 131 ++++++++++++++++++++ internal/output/prompt_guard_test.go | 115 +++++++++++++++++ internal/output/prompt_test.go | 97 +++++++++++++++ internal/reset/reset.go | 11 +- internal/snapshot/remove.go | 15 +-- internal/ui/app.go | 10 +- internal/ui/app_test.go | 89 +++++++++---- internal/ui/components/input_prompt.go | 5 +- internal/ui/components/input_prompt_test.go | 7 +- internal/update/notify.go | 11 +- internal/volume/clear.go | 11 +- test/integration/emulator_select_test.go | 6 + test/integration/volume_test.go | 4 +- 22 files changed, 525 insertions(+), 127 deletions(-) create mode 100644 internal/output/prompt.go create mode 100644 internal/output/prompt_guard_test.go create mode 100644 internal/output/prompt_test.go diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index f6542dd0..b91e36c8 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -47,6 +47,7 @@ Go through each changed file and check for violations. Flag only actual problems - [ ] Domain code never reads from stdin directly - [ ] Interactive input uses `UserInputRequestEvent` + `ResponseCh` pattern +- [ ] Prompts are built with an intent constructor (`output.Confirm` / `ActionChoice` / `Acknowledge`), not a raw event literal; a choice between distinct actions is not shipped as an inline `[a/b]` hint, and an `ActionChoice` label does not spell out its own key - [ ] Non-TTY mode fails early with a helpful error if input would be required - [ ] New user-supplied inputs (args, flags, config values) are validated at the boundary via `internal/validate`; no new inline validation regexp duplicates an existing validator (pod names → `PodName`; opaque secrets → loose checks like `AuthToken`; paths/URLs → their existing parsers; other identifiers → the owning API's documented contract) and malformed-input cases are tested diff --git a/CLAUDE.md b/CLAUDE.md index e9f84f0e..97d02495 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -268,10 +268,12 @@ A JSON-capable command emits a single `output.Envelope` (schema version, `data`/ Domain code must never read from stdin or wait for user input directly. Instead: -1. Emit a `UserInputRequestEvent` via `sink.Emit(output.UserInputRequestEvent{...})` with: - - `Prompt`: message to display - - `Options`: available choices (e.g., `{Key: "enter", Label: "Press ENTER to continue"}`) - - `ResponseCh`: channel to receive the user's response +1. Emit a `UserInputRequestEvent` built with one of the three intent constructors in `internal/output/prompt.go` — never a raw struct literal, which a guard test rejects outside that package. Name what the prompt *is* and its layout follows: + - `output.Confirm(prompt, output.DefaultYes|DefaultNo, responseCh)` — y/n on an action the user already requested. Renders inline as `[y/N]`; the capitalized answer is what ENTER picks. `DefaultNo` for anything destructive. + - `output.ActionChoice(prompt, options, responseCh)` — a choice between distinct outcomes. Renders one selectable row per option, with the `[KEY]` shortcut derived from each option's `Key`, so labels stay plain prose. + - `output.Acknowledge(prompt, label, responseCh)` — a single keypress, no choice. + + If a new prompt is not clearly one of the three, ask the user which it should be rather than guessing. Vertical is not a global default: flattening distinct actions into a trailing hint reads as prose and wraps badly (DEVX-1045), but a confirmation is one line for good reason. 2. Wait on the `ResponseCh` for an `InputResponse` containing: - `SelectedKey`: which option was selected @@ -286,11 +288,7 @@ Domain code must never read from stdin or wait for user input directly. Instead: Example flow in auth login: ```go responseCh := make(chan output.InputResponse, 1) -sink.Emit(output.UserInputRequestEvent{ - Prompt: "Waiting for authentication...", - Options: []output.InputOption{{Key: "enter", Label: "Press ENTER when complete"}}, - ResponseCh: responseCh, -}) +sink.Emit(output.Acknowledge("Waiting for authentication...", "Press any key when complete", responseCh)) select { case resp := <-responseCh: diff --git a/internal/auth/login.go b/internal/auth/login.go index b6c8a02a..431a81af 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -74,11 +74,7 @@ func (l *loginProvider) Login(ctx context.Context) (string, error) { l.sink.Emit(output.SpinnerStart("Waiting for authorization...")) responseCh := make(chan output.InputResponse, 1) - l.sink.Emit(output.UserInputRequestEvent{ - Prompt: "Waiting for authorization...", - Options: []output.InputOption{{Key: "any", Label: "Press any key when complete"}}, - ResponseCh: responseCh, - }) + l.sink.Emit(output.Acknowledge("Waiting for authorization...", "Press any key when complete", responseCh)) select { case resp := <-responseCh: diff --git a/internal/awsconfig/awsconfig.go b/internal/awsconfig/awsconfig.go index 841aaa0d..343bfd97 100644 --- a/internal/awsconfig/awsconfig.go +++ b/internal/awsconfig/awsconfig.go @@ -306,18 +306,14 @@ func Setup(ctx context.Context, sink output.Sink, resolvedHost string, status pr if !skipConfirm { responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.UserInputRequestEvent{ - Prompt: "Set up a LocalStack profile for AWS CLI and SDKs in ~/.aws?", - Options: []output.InputOption{{Key: "y", Label: "Y"}, {Key: "n", Label: "n"}}, - ResponseCh: responseCh, - }) + sink.Emit(output.Confirm("Set up a LocalStack profile for AWS CLI and SDKs in ~/.aws?", output.DefaultYes, responseCh)) select { case resp := <-responseCh: if resp.Cancelled { return nil } - if resp.SelectedKey == "n" { + if resp.SelectedKey == output.KeyNo { sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Skipped adding LocalStack AWS profile."}) return nil } diff --git a/internal/container/select.go b/internal/container/select.go index 5d5b286a..b3d4de8f 100644 --- a/internal/container/select.go +++ b/internal/container/select.go @@ -19,12 +19,7 @@ func SelectEmulator( } responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.UserInputRequestEvent{ - Prompt: "Which emulator would you like to use?", - Options: options, - ResponseCh: responseCh, - Vertical: true, - }) + sink.Emit(output.ActionChoice("Which emulator would you like to use?", options, responseCh)) var resp output.InputResponse select { diff --git a/internal/container/start.go b/internal/container/start.go index df9e77a0..7fe13736 100644 --- a/internal/container/start.go +++ b/internal/container/start.go @@ -1335,22 +1335,20 @@ func isDefinitiveLicenseRejection(status int) bool { // ESC declines. Ctrl+C would do too, but it also cancels the root context, and // the ErrorEvent that the decline renders then races the TUI's own quit — so the // manual recovery steps sometimes never reach the terminal (DEVX-1045). An -// advertised decline key keeps that path deterministic. The choices render -// vertically so both keys read as selectable actions rather than a hint tacked -// onto the end of the sentence; the prompt therefore states the reason and -// leaves the two actions to the labels, which keeps it one wrapped statement -// instead of a statement whose trailing question dangles at the wrap point. +// advertised decline key keeps that path deterministic. The prompt states the +// reason and leaves the two actions to output.ActionChoice's labels, which keeps +// it one wrapped statement instead of a statement whose trailing question +// dangles at the wrap point. func promptRelogin(ctx context.Context, sink output.Sink, licErr *api.LicenseError) bool { responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.UserInputRequestEvent{ - Prompt: fmt.Sprintf("License validation failed: %s.", licErr.Message), - Options: []output.InputOption{ - {Key: "r", Label: "[R] Re-authenticate"}, - {Key: "esc", Label: "[ESC] Exit"}, + sink.Emit(output.ActionChoice( + fmt.Sprintf("License validation failed: %s.", licErr.Message), + []output.InputOption{ + {Key: "r", Label: "Re-authenticate"}, + {Key: "esc", Label: "Exit"}, }, - ResponseCh: responseCh, - Vertical: true, - }) + responseCh, + )) select { case resp := <-responseCh: return !resp.Cancelled && resp.SelectedKey != "esc" @@ -1629,15 +1627,14 @@ func (m *startupMonitor) await(ctx context.Context, containerID, healthURL strin m.sink.Emit(output.SpinnerStop()) responseCh = make(chan output.InputResponse, 1) - m.sink.Emit(output.UserInputRequestEvent{ - Prompt: "LocalStack is still starting. Check progress with 'lstk logs'.", - Options: []output.InputOption{ - {Key: "w", Label: "[W] Keep waiting"}, - {Key: "s", Label: "[S] Stop and exit"}, + m.sink.Emit(output.ActionChoice( + "LocalStack is still starting. Check progress with 'lstk logs'.", + []output.InputOption{ + {Key: "w", Label: "Keep waiting"}, + {Key: "s", Label: "Stop and exit"}, }, - ResponseCh: responseCh, - Vertical: true, - }) + responseCh, + )) case <-ticker.C: if ready, err := check(); err != nil || ready { return err diff --git a/internal/container/start_test.go b/internal/container/start_test.go index 73420031..8f38d405 100644 --- a/internal/container/start_test.go +++ b/internal/container/start_test.go @@ -853,9 +853,12 @@ func TestStartupMonitorAwait_InteractivePromptKeepWaitingThenStop(t *testing.T) assert.Equal(t, "LocalStack is still starting. Check progress with 'lstk logs'.", firstPrompt.Prompt) assert.True(t, firstPrompt.Vertical) assert.Equal(t, []output.InputOption{ - {Key: "w", Label: "[W] Keep waiting"}, - {Key: "s", Label: "[S] Stop and exit"}, + {Key: "w", Label: "Keep waiting"}, + {Key: "s", Label: "Stop and exit"}, }, firstPrompt.Options) + // Labels stay plain prose; the advertised keys come from output.OptionLabel. + assert.Equal(t, "[W] Keep waiting", output.OptionLabel(firstPrompt.Options[0])) + assert.Equal(t, "[S] Stop and exit", output.OptionLabel(firstPrompt.Options[1])) } func TestStartupMonitorAwait_DismissesPromptWhenEmulatorBecomesReady(t *testing.T) { @@ -1698,7 +1701,7 @@ func TestPromptRelogin_FoldsReasonIntoThePromptWithoutASeparateWarning(t *testin req, ok := events[0].(output.UserInputRequestEvent) require.True(t, ok, "the only event emitted must be the prompt itself") assert.Contains(t, req.Prompt, licErr.Message, "the prompt must explain why the user is being asked to log in again") - assert.Equal(t, "[R] Re-authenticate", req.Options[0].Label, "the recovery action belongs to the choice, not the prompt sentence") + assert.Equal(t, "Re-authenticate", req.Options[0].Label, "the recovery action belongs to the choice, not the prompt sentence") } // TestPromptRelogin_OffersAnAdvertisedDeclineKey covers DEVX-1045: Ctrl+C was the @@ -1730,9 +1733,13 @@ func TestPromptRelogin_OffersAnAdvertisedDeclineKey(t *testing.T) { assert.Equal(t, tc.accepted, accepted) assert.True(t, req.Vertical, "the choices must render as vertical, selectable actions") assert.Equal(t, []output.InputOption{ - {Key: "r", Label: "[R] Re-authenticate"}, - {Key: "esc", Label: "[ESC] Exit"}, - }, req.Options, "both the accept and the decline key must be advertised, shortcut first") + {Key: "r", Label: "Re-authenticate"}, + {Key: "esc", Label: "Exit"}, + }, req.Options) + // Labels stay plain prose; the advertised keys come from output.OptionLabel. + assert.Equal(t, "[R] Re-authenticate", output.OptionLabel(req.Options[0]), + "both the accept and the decline key must be advertised, shortcut first") + assert.Equal(t, "[ESC] Exit", output.OptionLabel(req.Options[1])) }) } } diff --git a/internal/output/events.go b/internal/output/events.go index af6cfa98..cacd334b 100644 --- a/internal/output/events.go +++ b/internal/output/events.go @@ -271,11 +271,19 @@ type InputResponse struct { Cancelled bool } +// UserInputRequestEvent asks the frontend to put a question to the user and +// send the answer back on ResponseCh. +// +// Build one with Confirm, ActionChoice, or Acknowledge (prompt.go) rather than +// by hand: naming what the prompt is settles how it renders, and a guard test +// fails the build on a raw literal outside this package. type UserInputRequestEvent struct { Prompt string Options []InputOption ResponseCh chan<- InputResponse - Vertical bool + // Vertical renders each option as its own selectable row instead of a + // trailing "[a/b]" hint. Set by ActionChoice; do not set it directly. + Vertical bool } // UserInputDismissEvent removes a pending prompt when the condition that diff --git a/internal/output/plain_format.go b/internal/output/plain_format.go index d153de90..567c2d9f 100644 --- a/internal/output/plain_format.go +++ b/internal/output/plain_format.go @@ -101,7 +101,31 @@ func formatStatusLine(e ContainerStatusEvent) (string, bool) { } func formatUserInputRequest(e UserInputRequestEvent) string { - return FormatPrompt(e.Prompt, e.Options) + return FormatPromptEvent(e) +} + +// FormatPromptEvent renders a prompt on a single line. A vertical prompt has no +// one-line form of its own, so its options are laid end to end — each keeping +// the shortcut OptionLabel derives, since a plain-prose label ("Log in again") +// would otherwise leave the user with no key to press. They carry their own +// brackets, so the surrounding "[a/b]" of an inline prompt is dropped rather +// than nested. Used wherever the full multi-line rendering does not fit: plain +// output, and the TUI's spinner text. +func FormatPromptEvent(e UserInputRequestEvent) string { + if !e.Vertical { + return FormatPrompt(e.Prompt, e.Options) + } + + labels := make([]string, 0, len(e.Options)) + for _, opt := range e.Options { + if label := OptionLabel(opt); label != "" { + labels = append(labels, label) + } + } + if len(labels) == 0 { + return appendPromptSuffix(e.Prompt, "") + } + return appendPromptSuffix(e.Prompt, " "+strings.Join(labels, " / ")) } // FormatPromptLabels formats option labels into a suffix string. @@ -125,8 +149,14 @@ func FormatPromptLabels(options []InputOption) string { // FormatPrompt formats a prompt string with its options into a display line. func FormatPrompt(prompt string, options []InputOption) string { + return appendPromptSuffix(prompt, FormatPromptLabels(options)) +} + +// appendPromptSuffix puts the option hints on the end of the prompt's first +// line, so any lines below it stay a block of their own. +func appendPromptSuffix(prompt, suffix string) string { lines := strings.Split(prompt, "\n") - firstLine := lines[0] + FormatPromptLabels(options) + firstLine := lines[0] + suffix rest := lines[1:] if len(rest) == 0 { return firstLine diff --git a/internal/output/prompt.go b/internal/output/prompt.go new file mode 100644 index 00000000..fe2c7db0 --- /dev/null +++ b/internal/output/prompt.go @@ -0,0 +1,131 @@ +package output + +import ( + "fmt" + "strings" +) + +// Keys carried by the options the constructors below build. Handlers should +// compare InputResponse.SelectedKey against these rather than string literals. +const ( + KeyYes = "y" + KeyNo = "n" + // KeyAny matches any keypress. resolveOption in internal/ui returns it + // before considering any other option, so it is only meaningful as the sole + // option of an Acknowledge prompt. + KeyAny = "any" +) + +// ConfirmDefault selects which answer ENTER picks in a Confirm prompt. It is +// conveyed to the user by capitalizing that answer's label, which is also how +// the TUI's key resolution finds it — so the displayed default and the honored +// default cannot drift apart. +type ConfirmDefault int + +const ( + DefaultYes ConfirmDefault = iota + DefaultNo +) + +// Confirm asks the user to approve an action they already requested, rendered +// inline as "Reset emulator state? [y/N]". +// +// Inline is deliberate here and should stay that way: the question has one +// answer the user is already leaning toward, the [y/N] idiom is universal, it +// costs one line, and it carries its default in the capitalization. Pass +// DefaultNo for anything destructive or irreversible. +// +// Use ActionChoice instead when the options are distinct outcomes rather than +// "do the thing I asked for, or don't". If a new prompt is not clearly one or +// the other, ask the user which it should be rather than guessing. +func Confirm(prompt string, def ConfirmDefault, responseCh chan<- InputResponse) UserInputRequestEvent { + yes, no := "Y", "n" + if def == DefaultNo { + yes, no = "y", "N" + } + return UserInputRequestEvent{ + Prompt: prompt, + Options: []InputOption{ + {Key: KeyYes, Label: yes}, + {Key: KeyNo, Label: no}, + }, + ResponseCh: responseCh, + } +} + +// ActionChoice offers a choice between distinct outcomes, rendered vertically +// as one selectable row per option: +// +// ? License validation failed: token expired. +// ● [ENTER] Log in again +// ○ [ESC] Exit +// +// Labels are plain prose — OptionLabel derives the bracketed shortcut from each +// option's Key, so a label must not spell the key out itself. +// +// Vertical is deliberate here and should stay that way: flattening several +// distinct actions into a trailing "[a/b]" hint reads as prose glued to the end +// of the question, wraps badly, and gives the user nothing to arrow through +// (DEVX-1045). Use Confirm for a yes/no on an action the user already +// requested, and Acknowledge when there is nothing to choose between. +func ActionChoice(prompt string, options []InputOption, responseCh chan<- InputResponse) UserInputRequestEvent { + return UserInputRequestEvent{ + Prompt: prompt, + Options: options, + ResponseCh: responseCh, + Vertical: true, + } +} + +// Acknowledge waits for any keypress, rendered inline as "Waiting for +// authorization... (Press any key when complete)". It is not a choice — there +// is one option and every key selects it — so it stays on one line. +func Acknowledge(prompt, label string, responseCh chan<- InputResponse) UserInputRequestEvent { + return UserInputRequestEvent{ + Prompt: prompt, + Options: []InputOption{{Key: KeyAny, Label: label}}, + ResponseCh: responseCh, + } +} + +// namedShortcuts spells out the keys whose names are not a single character. +// The values match what the terminal user is told to press, not tea.KeyType's +// own naming. +var namedShortcuts = map[string]string{ + "enter": "ENTER", + "esc": "ESC", + "space": "SPACE", + "tab": "TAB", +} + +// OptionLabel renders one option of a vertical prompt: its shortcut in +// brackets, then its label ("[ENTER] Log in again"). An option with no +// dedicated key — KeyAny, or an empty one — renders as the bare label, since +// there is no single key to advertise. +// +// Deriving the shortcut instead of leaving it to each label is what keeps the +// prompts consistent: hand-written labels had drifted into three styles +// ("[ENTER] Log in again", "Update now [U]", and a bare "AWS" that advertised +// no key at all) before this existed. +func OptionLabel(opt InputOption) string { + key := shortcut(opt.Key) + if key == "" { + return opt.Label + } + if opt.Label == "" { + return fmt.Sprintf("[%s]", key) + } + return fmt.Sprintf("[%s] %s", key, opt.Label) +} + +// shortcut returns the display form of an option key, or "" when the key names +// no single keypress the user can be told to hit. +func shortcut(key string) string { + if key == "" || key == KeyAny { + return "" + } + if named, ok := namedShortcuts[key]; ok { + return named + } + return strings.ToUpper(key) +} diff --git a/internal/output/prompt_guard_test.go b/internal/output/prompt_guard_test.go new file mode 100644 index 00000000..4909cf63 --- /dev/null +++ b/internal/output/prompt_guard_test.go @@ -0,0 +1,115 @@ +package output + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestPromptsUseIntentConstructors keeps every prompt in the CLI built through +// Confirm, ActionChoice, or Acknowledge. +// +// The point is not tidiness. A raw UserInputRequestEvent literal asks its author +// to pick a rendering (Vertical true or false) at a moment when the question +// they can actually answer is what the prompt IS — and the cheapest answer, +// leaving the field out, silently ships an inline prompt. That is how the +// license re-login prompt ended up with two advertised keys flattened into one +// dimmed hint (DEVX-1045). Naming the intent instead makes the layout a +// consequence rather than a decision. +// +// Test files are exempt: they construct events to drive the UI, not to ask a +// real user anything. test/integration is a separate module and out of scope. +func TestPromptsUseIntentConstructors(t *testing.T) { + t.Parallel() + + const guidance = "Use output.ActionChoice (a choice between distinct actions, rendered vertically), " + + "output.Confirm (y/n on an action the user already requested), or " + + "output.Acknowledge (a single key, no choice). If unsure which, ask the user." + + root := filepath.Join("..", "..") + // The constructors themselves live here, and they are the one place allowed + // to build the event by hand. + selfPkg := filepath.Join(root, "internal", "output") + + fset := token.NewFileSet() + // "." covers the root package (main.go) without descending into the module's + // other trees; WalkDir on it would pull in test/integration, a separate + // module, and every vendored or generated directory below. + for _, dir := range []string{".", "internal", "cmd"} { + walk := filepath.WalkDir + if dir == "." { + walk = walkDirTopLevel + } + err := walk(filepath.Join(root, dir), func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if path == selfPkg { + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + + file, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if err != nil { + return err + } + + ast.Inspect(file, func(n ast.Node) bool { + lit, ok := n.(*ast.CompositeLit) + if !ok || !isUserInputRequestEvent(lit.Type) { + return true + } + rel, relErr := filepath.Rel(root, path) + if relErr != nil { + rel = path + } + t.Errorf("%s:%d builds a raw output.UserInputRequestEvent literal.\n%s", + filepath.ToSlash(rel), fset.Position(lit.Pos()).Line, guidance) + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking %s: %v", dir, err) + } + } +} + +// walkDirTopLevel visits the files directly inside dir, never its subdirectories. +func walkDirTopLevel(dir string, fn fs.WalkDirFunc) error { + entries, err := os.ReadDir(dir) + if err != nil { + return err + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + if err := fn(filepath.Join(dir, entry.Name()), entry, nil); err != nil { + return err + } + } + return nil +} + +// isUserInputRequestEvent matches the type by name on either side of a possible +// package qualifier, so an import alias cannot slip a literal past the guard. +func isUserInputRequestEvent(expr ast.Expr) bool { + switch t := expr.(type) { + case *ast.Ident: + return t.Name == "UserInputRequestEvent" + case *ast.SelectorExpr: + return t.Sel.Name == "UserInputRequestEvent" + } + return false +} diff --git a/internal/output/prompt_test.go b/internal/output/prompt_test.go new file mode 100644 index 00000000..0f565d03 --- /dev/null +++ b/internal/output/prompt_test.go @@ -0,0 +1,97 @@ +package output + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConfirmRendersInlineWithTheDefaultCapitalized(t *testing.T) { + t.Parallel() + + // That the capitalized answer is the one ENTER selects is asserted end to end + // by TestAppEnterHonorsTheConfirmDefault in internal/ui, which exercises the + // real key resolution instead of a copy of its rule. + tests := []struct { + name string + def ConfirmDefault + labels []string + hint string + }{ + {name: "default yes", def: DefaultYes, labels: []string{"Y", "n"}, hint: " [Y/n]"}, + {name: "default no", def: DefaultNo, labels: []string{"y", "N"}, hint: " [y/N]"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ch := make(chan InputResponse, 1) + event := Confirm("Reset emulator state?", tt.def, ch) + + assert.False(t, event.Vertical, "a confirmation stays on one line") + require.Len(t, event.Options, 2) + assert.Equal(t, KeyYes, event.Options[0].Key) + assert.Equal(t, KeyNo, event.Options[1].Key) + assert.Equal(t, tt.labels, []string{event.Options[0].Label, event.Options[1].Label}) + assert.Equal(t, "Reset emulator state?"+tt.hint, FormatPromptEvent(event)) + }) + } +} + +func TestActionChoiceRendersVerticallyWithDerivedShortcuts(t *testing.T) { + t.Parallel() + + ch := make(chan InputResponse, 1) + event := ActionChoice("License validation failed: token expired.", []InputOption{ + {Key: "enter", Label: "Log in again"}, + {Key: "esc", Label: "Exit"}, + }, ch) + + assert.True(t, event.Vertical, "distinct actions render as selectable rows") + assert.Equal(t, "[ENTER] Log in again", OptionLabel(event.Options[0])) + assert.Equal(t, "[ESC] Exit", OptionLabel(event.Options[1])) + + // The one-line form keeps the shortcuts, so a prompt mirrored into spinner + // text never leaves the user without a key to press. The labels bring their + // own brackets, so it does not nest them inside an inline prompt's "[a/b]". + assert.Equal(t, + "License validation failed: token expired. [ENTER] Log in again / [ESC] Exit", + FormatPromptEvent(event)) +} + +func TestAcknowledgeRendersInlineWithASingleAnyKeyOption(t *testing.T) { + t.Parallel() + + ch := make(chan InputResponse, 1) + event := Acknowledge("Waiting for authorization...", "Press any key when complete", ch) + + assert.False(t, event.Vertical) + require.Len(t, event.Options, 1) + assert.Equal(t, KeyAny, event.Options[0].Key) + assert.Equal(t, "Waiting for authorization... (Press any key when complete)", FormatPromptEvent(event)) +} + +func TestOptionLabel(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + opt InputOption + want string + }{ + {name: "named key", opt: InputOption{Key: "enter", Label: "Log in again"}, want: "[ENTER] Log in again"}, + {name: "escape", opt: InputOption{Key: "esc", Label: "Exit"}, want: "[ESC] Exit"}, + {name: "single letter is uppercased", opt: InputOption{Key: "a", Label: "AWS"}, want: "[A] AWS"}, + {name: "any key advertises nothing", opt: InputOption{Key: KeyAny, Label: "Press any key"}, want: "Press any key"}, + {name: "empty key advertises nothing", opt: InputOption{Label: "Continue"}, want: "Continue"}, + {name: "no label", opt: InputOption{Key: "w"}, want: "[W]"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, OptionLabel(tt.opt)) + }) + } +} diff --git a/internal/reset/reset.go b/internal/reset/reset.go index 54b21e9d..7af1c190 100644 --- a/internal/reset/reset.go +++ b/internal/reset/reset.go @@ -41,18 +41,11 @@ func Reset(ctx context.Context, rt runtime.Runtime, containers []config.Containe if !force { responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.UserInputRequestEvent{ - Prompt: "Reset emulator state? All resources will be lost", - Options: []output.InputOption{ - {Key: "y", Label: "Yes"}, - {Key: "n", Label: "NO"}, - }, - ResponseCh: responseCh, - }) + sink.Emit(output.Confirm("Reset emulator state? All resources will be lost", output.DefaultNo, responseCh)) select { case resp := <-responseCh: - if resp.Cancelled || resp.SelectedKey != "y" { + if resp.Cancelled || resp.SelectedKey != output.KeyYes { sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Cancelled"}) return nil } diff --git a/internal/snapshot/remove.go b/internal/snapshot/remove.go index 5e994d3d..2ff65f62 100644 --- a/internal/snapshot/remove.go +++ b/internal/snapshot/remove.go @@ -49,18 +49,15 @@ func Remove(ctx context.Context, rt runtime.Runtime, containers []config.Contain if !force { responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.UserInputRequestEvent{ - Prompt: fmt.Sprintf("Delete cloud snapshot 'pod:%s'? This operation cannot be undone.", podName), - Options: []output.InputOption{ - {Key: "y", Label: "Y"}, - {Key: "n", Label: "n"}, - }, - ResponseCh: responseCh, - }) + sink.Emit(output.Confirm( + fmt.Sprintf("Delete cloud snapshot 'pod:%s'? This operation cannot be undone.", podName), + output.DefaultNo, + responseCh, + )) select { case resp := <-responseCh: - if resp.Cancelled || resp.SelectedKey != "y" { + if resp.Cancelled || resp.SelectedKey != output.KeyYes { sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Cancelled"}) return nil } diff --git a/internal/ui/app.go b/internal/ui/app.go index 0c9c5baa..8e24217e 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -195,7 +195,7 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // View renders one or the other. a.inputPrompt = a.inputPrompt.Show(msg.Prompt, msg.Options, msg.Vertical) if a.spinner.Visible() { - a.spinner = a.spinner.SetText(output.FormatPrompt(msg.Prompt, msg.Options)) + a.spinner = a.spinner.SetText(output.FormatPromptEvent(msg)) } case output.UserInputDismissEvent: if a.pendingInput == nil || a.pendingInput.ResponseCh != msg.ResponseCh { @@ -466,7 +466,7 @@ func formatResolvedInput(req output.UserInputRequestEvent, selectedKey string) s if req.Vertical { firstLine := strings.Split(req.Prompt, "\n")[0] - if selected == "" || !hasLabels || selectedKey == "any" { + if selected == "" || !hasLabels || selectedKey == output.KeyAny { return firstLine } return fmt.Sprintf("%s %s", firstLine, selected) @@ -475,14 +475,14 @@ func formatResolvedInput(req output.UserInputRequestEvent, selectedKey string) s formatted := output.FormatPrompt(req.Prompt, req.Options) firstLine := strings.Split(formatted, "\n")[0] - if selected == "" || !hasLabels || selectedKey == "any" { + if selected == "" || !hasLabels || selectedKey == output.KeyAny { return firstLine } return fmt.Sprintf("%s %s", firstLine, selected) } // resolveOption finds the best matching option for a key event, in priority order: -// 1. "any" — matches any keypress +// 1. output.KeyAny — matches any keypress // 2. "enter" — matches the Enter key explicitly // 3. uppercase label — matches Enter as the conventional default // 4. case-insensitive key match — matches any other key @@ -490,7 +490,7 @@ func resolveOption(options []output.InputOption, msg tea.KeyMsg) *output.InputOp var uppercaseDefault *output.InputOption for i, opt := range options { switch { - case opt.Key == "any": + case opt.Key == output.KeyAny: return &options[i] case msg.Type == tea.KeyEnter && opt.Key == "enter": return &options[i] diff --git a/internal/ui/app_test.go b/internal/ui/app_test.go index ae250477..15569425 100644 --- a/internal/ui/app_test.go +++ b/internal/ui/app_test.go @@ -795,12 +795,11 @@ func TestAppEnterSelectsHighlightedVerticalOption(t *testing.T) { app := NewApp("dev", "", "", nil) responseCh := make(chan output.InputResponse, 1) - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Update lstk to latest version?", - Options: []output.InputOption{{Key: "u", Label: "Update now [U]"}, {Key: "s", Label: "Skip this version [S]"}, {Key: "n", Label: "Never ask again [N]"}}, - ResponseCh: responseCh, - Vertical: true, - }) + model, _ := app.Update(output.ActionChoice("Update lstk to latest version?", []output.InputOption{ + {Key: "u", Label: "Update now"}, + {Key: "s", Label: "Skip this version"}, + {Key: "n", Label: "Never ask again"}, + }, responseCh)) app = model.(App) model, _ = app.Update(tea.KeyMsg{Type: tea.KeyDown}) @@ -837,15 +836,14 @@ func TestAppEscResolvesVerticalDeclineOption(t *testing.T) { app := NewApp("dev", "", "", nil) responseCh := make(chan output.InputResponse, 1) - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "License validation failed: invalid, inactive, or expired authentication token or subscription.", - Options: []output.InputOption{ - {Key: "r", Label: "[R] Re-authenticate"}, - {Key: "esc", Label: "[ESC] Exit"}, + model, _ := app.Update(output.ActionChoice( + "License validation failed: invalid, inactive, or expired authentication token or subscription.", + []output.InputOption{ + {Key: "r", Label: "Re-authenticate"}, + {Key: "esc", Label: "Exit"}, }, - ResponseCh: responseCh, - Vertical: true, - }) + responseCh, + )) app = model.(App) model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEscape}) @@ -878,15 +876,14 @@ func TestAppReloginShortcutIgnoresVerticalSelection(t *testing.T) { app := NewApp("dev", "", "", nil) responseCh := make(chan output.InputResponse, 1) - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "License validation failed: invalid, inactive, or expired authentication token or subscription.", - Options: []output.InputOption{ - {Key: "r", Label: "[R] Re-authenticate"}, - {Key: "esc", Label: "[ESC] Exit"}, + model, _ := app.Update(output.ActionChoice( + "License validation failed: invalid, inactive, or expired authentication token or subscription.", + []output.InputOption{ + {Key: "r", Label: "Re-authenticate"}, + {Key: "esc", Label: "Exit"}, }, - ResponseCh: responseCh, - Vertical: true, - }) + responseCh, + )) app = model.(App) model, _ = app.Update(tea.KeyMsg{Type: tea.KeyDown}) @@ -912,17 +909,55 @@ func TestAppReloginShortcutIgnoresVerticalSelection(t *testing.T) { } } +// TestAppEnterHonorsTheConfirmDefault pins the contract that lets output.Confirm +// advertise its default by capitalizing one label: ENTER must select whichever +// answer is capitalized, so a destructive prompt built with DefaultNo cannot be +// confirmed by a stray ENTER. +func TestAppEnterHonorsTheConfirmDefault(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + def output.ConfirmDefault + want string + }{ + {name: "default yes", def: output.DefaultYes, want: output.KeyYes}, + {name: "default no", def: output.DefaultNo, want: output.KeyNo}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + app := NewApp("dev", "", "", nil) + responseCh := make(chan output.InputResponse, 1) + + model, _ := app.Update(output.Confirm("Reset emulator state? All resources will be lost", tc.def, responseCh)) + app = model.(App) + + _, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd == nil { + t.Fatal("expected enter to resolve the confirmation") + } + cmd() + + select { + case resp := <-responseCh: + if resp.SelectedKey != tc.want { + t.Fatalf("expected enter to select %q, got %q", tc.want, resp.SelectedKey) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for response on channel") + } + }) + } +} + func TestAppAnyKeyOptionResolvesOnAnyKeypress(t *testing.T) { t.Parallel() app := NewApp("dev", "", "", nil) responseCh := make(chan output.InputResponse, 1) - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Waiting for authorization...", - Options: []output.InputOption{{Key: "any", Label: "Press any key when complete"}}, - ResponseCh: responseCh, - }) + model, _ := app.Update(output.Acknowledge("Waiting for authorization...", "Press any key when complete", responseCh)) app = model.(App) // Any key (e.g., spacebar) should resolve diff --git a/internal/ui/components/input_prompt.go b/internal/ui/components/input_prompt.go index 245cae55..f322ed21 100644 --- a/internal/ui/components/input_prompt.go +++ b/internal/ui/components/input_prompt.go @@ -123,10 +123,11 @@ func (p InputPrompt) viewVertical(width int) string { } for i, opt := range p.options { + label := output.OptionLabel(opt) if i == p.selectedIndex { - sb.WriteString(styles.NimboMid.Render("● " + opt.Label)) + sb.WriteString(styles.NimboMid.Render("● " + label)) } else { - sb.WriteString(styles.Secondary.Render("○ " + opt.Label)) + sb.WriteString(styles.Secondary.Render("○ " + label)) } sb.WriteString("\n") } diff --git a/internal/ui/components/input_prompt_test.go b/internal/ui/components/input_prompt_test.go index 7865efd6..69063e07 100644 --- a/internal/ui/components/input_prompt_test.go +++ b/internal/ui/components/input_prompt_test.go @@ -167,15 +167,16 @@ func TestInputPromptViewSlowStartChoicesAreScannable(t *testing.T) { // TestInputPromptViewReloginChoicesAreScannable covers the license re-login // prompt: its question is long enough to wrap, so flattening the two choices // into a trailing hint made them read as prose. They belong on their own lines -// below the wrapped question, shortcut first. +// below the wrapped question, shortcut first — and the shortcut is derived from +// each option's key, so a plain-prose label still advertises the key to press. func TestInputPromptViewReloginChoicesAreScannable(t *testing.T) { t.Parallel() const width = 40 question := "License validation failed: invalid, inactive, or expired authentication token or subscription." p := NewInputPrompt().Show(question, []output.InputOption{ - {Key: "r", Label: "[R] Re-authenticate"}, - {Key: "esc", Label: "[ESC] Exit"}, + {Key: "r", Label: "Re-authenticate"}, + {Key: "esc", Label: "Exit"}, }, true) view := p.View(width) diff --git a/internal/update/notify.go b/internal/update/notify.go index e7ffb031..244420df 100644 --- a/internal/update/notify.go +++ b/internal/update/notify.go @@ -75,12 +75,11 @@ func promptAndUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions, sink.Emit(output.MessageEvent{Severity: output.SeveritySecondary, Text: fmt.Sprintf("> Release notes: %s", releaseNotesURL)}) responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.UserInputRequestEvent{ - Prompt: "Update lstk to latest version?", - Options: []output.InputOption{{Key: "u", Label: "Update now [U]"}, {Key: "r", Label: "Remind me next time [R]"}, {Key: "s", Label: "Skip this version [S]"}}, - ResponseCh: responseCh, - Vertical: true, - }) + sink.Emit(output.ActionChoice("Update lstk to latest version?", []output.InputOption{ + {Key: "u", Label: "Update now"}, + {Key: "r", Label: "Remind me next time"}, + {Key: "s", Label: "Skip this version"}, + }, responseCh)) var resp output.InputResponse select { diff --git a/internal/volume/clear.go b/internal/volume/clear.go index 626f251a..550fb2c9 100644 --- a/internal/volume/clear.go +++ b/internal/volume/clear.go @@ -38,18 +38,11 @@ func Clear(ctx context.Context, sink output.Sink, containers []config.ContainerC if !force { responseCh := make(chan output.InputResponse, 1) - sink.Emit(output.UserInputRequestEvent{ - Prompt: "Clear volume data? This cannot be undone", - Options: []output.InputOption{ - {Key: "y", Label: "Yes"}, - {Key: "n", Label: "NO"}, - }, - ResponseCh: responseCh, - }) + sink.Emit(output.Confirm("Clear volume data? This cannot be undone", output.DefaultNo, responseCh)) select { case resp := <-responseCh: - if resp.Cancelled || resp.SelectedKey != "y" { + if resp.Cancelled || resp.SelectedKey != output.KeyYes { sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: "Cancelled"}) return nil } diff --git a/test/integration/emulator_select_test.go b/test/integration/emulator_select_test.go index 8dead502..2f023658 100644 --- a/test/integration/emulator_select_test.go +++ b/test/integration/emulator_select_test.go @@ -60,6 +60,12 @@ func TestFirstRunShowsEmulatorSelectionPrompt(t *testing.T) { p.waitForOutput("Which emulator would you like to use?", "emulator selection prompt should appear on first run") + // Each choice is a selectable row advertising the key that picks it directly. + // The shortcut is derived from the option's key by output.OptionLabel, so a + // picker whose labels are bare names still tells the user what to press. + p.waitForOutput("[A] AWS", "each emulator row should advertise its shortcut") + p.waitForOutput("[Z] Azure", "each emulator row should advertise its shortcut") + // Confirm the default-highlighted option (AWS) by pressing Enter. p.write("\r") diff --git a/test/integration/volume_test.go b/test/integration/volume_test.go index 99d5e485..7b6abd4d 100644 --- a/test/integration/volume_test.go +++ b/test/integration/volume_test.go @@ -292,7 +292,9 @@ volume = "` + escapeTomlPath(volumeDir) + `" startVolumeClear := func(t *testing.T, configFile string) *ptyProc { t.Helper() p := startLstkInPTY(t, testContext(t), testEnvWithHome(t.TempDir(), ""), "--config", configFile, "volume", "clear") - p.waitForOutput("Clear volume data?", "confirmation prompt should appear") + // An irreversible confirmation stays inline and capitalizes the answer + // ENTER picks, so a stray ENTER cannot wipe the volume. + p.waitForOutput("Clear volume data? This cannot be undone [y/N]", "confirmation prompt should appear") return p } From 67137736f447c4ee0100f6a788ea4853e9e4df1a Mon Sep 17 00:00:00 2001 From: Misha Tiurin Date: Thu, 13 Aug 2026 20:11:01 +0200 Subject: [PATCH 2/6] Seal prompt construction with unexported fields instead of a guard test Co-Authored-By: Claude --- .claude/skills/review-pr/SKILL.md | 2 +- CLAUDE.md | 2 +- internal/container/start_test.go | 34 ++--- internal/output/events.go | 40 +++-- internal/output/plain_format.go | 12 +- internal/output/prompt.go | 20 +-- internal/output/prompt_guard_test.go | 115 -------------- internal/output/prompt_test.go | 22 +-- internal/reset/reset_test.go | 4 +- internal/ui/app.go | 31 ++-- internal/ui/app_test.go | 214 +++++++-------------------- internal/update/notify_test.go | 16 +- 12 files changed, 152 insertions(+), 360 deletions(-) delete mode 100644 internal/output/prompt_guard_test.go diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index b91e36c8..39f44b24 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -47,7 +47,7 @@ Go through each changed file and check for violations. Flag only actual problems - [ ] Domain code never reads from stdin directly - [ ] Interactive input uses `UserInputRequestEvent` + `ResponseCh` pattern -- [ ] Prompts are built with an intent constructor (`output.Confirm` / `ActionChoice` / `Acknowledge`), not a raw event literal; a choice between distinct actions is not shipped as an inline `[a/b]` hint, and an `ActionChoice` label does not spell out its own key +- [ ] The intent constructor a prompt uses matches what it is (the compiler enforces that one is used, not that it is the right one): a choice between distinct actions is `ActionChoice`, not a `Confirm` with an inline `[a/b]` hint, and an `ActionChoice` label does not spell out its own key - [ ] Non-TTY mode fails early with a helpful error if input would be required - [ ] New user-supplied inputs (args, flags, config values) are validated at the boundary via `internal/validate`; no new inline validation regexp duplicates an existing validator (pod names → `PodName`; opaque secrets → loose checks like `AuthToken`; paths/URLs → their existing parsers; other identifiers → the owning API's documented contract) and malformed-input cases are tested diff --git a/CLAUDE.md b/CLAUDE.md index 97d02495..81a8ca53 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -268,7 +268,7 @@ A JSON-capable command emits a single `output.Envelope` (schema version, `data`/ Domain code must never read from stdin or wait for user input directly. Instead: -1. Emit a `UserInputRequestEvent` built with one of the three intent constructors in `internal/output/prompt.go` — never a raw struct literal, which a guard test rejects outside that package. Name what the prompt *is* and its layout follows: +1. Emit a `UserInputRequestEvent` built with one of the three intent constructors in `internal/output/prompt.go`. Its fields are unexported, so a struct literal built anywhere else does not compile — the constructors are the only way in. Name what the prompt *is* and its layout follows: - `output.Confirm(prompt, output.DefaultYes|DefaultNo, responseCh)` — y/n on an action the user already requested. Renders inline as `[y/N]`; the capitalized answer is what ENTER picks. `DefaultNo` for anything destructive. - `output.ActionChoice(prompt, options, responseCh)` — a choice between distinct outcomes. Renders one selectable row per option, with the `[KEY]` shortcut derived from each option's `Key`, so labels stay plain prose. - `output.Acknowledge(prompt, label, responseCh)` — a single keypress, no choice. diff --git a/internal/container/start_test.go b/internal/container/start_test.go index 8f38d405..9d91fd96 100644 --- a/internal/container/start_test.go +++ b/internal/container/start_test.go @@ -833,7 +833,7 @@ func TestStartupMonitorAwait_InteractivePromptKeepWaitingThenStop(t *testing.T) for i, key := range []string{"w", "s"} { select { case req := <-prompts: - req.ResponseCh <- output.InputResponse{SelectedKey: key} + req.ResponseCh() <- output.InputResponse{SelectedKey: key} case <-time.After(5 * time.Second): t.Errorf("prompt %d never appeared", i+1) return @@ -850,15 +850,15 @@ func TestStartupMonitorAwait_InteractivePromptKeepWaitingThenStop(t *testing.T) assert.True(t, timeoutErr.stopped, "choosing stop at the prompt must be recorded on the error") firstPrompt := <-seenPrompts - assert.Equal(t, "LocalStack is still starting. Check progress with 'lstk logs'.", firstPrompt.Prompt) - assert.True(t, firstPrompt.Vertical) + assert.Equal(t, "LocalStack is still starting. Check progress with 'lstk logs'.", firstPrompt.Prompt()) + assert.True(t, firstPrompt.Vertical()) assert.Equal(t, []output.InputOption{ {Key: "w", Label: "Keep waiting"}, {Key: "s", Label: "Stop and exit"}, - }, firstPrompt.Options) + }, firstPrompt.Options()) // Labels stay plain prose; the advertised keys come from output.OptionLabel. - assert.Equal(t, "[W] Keep waiting", output.OptionLabel(firstPrompt.Options[0])) - assert.Equal(t, "[S] Stop and exit", output.OptionLabel(firstPrompt.Options[1])) + assert.Equal(t, "[W] Keep waiting", output.OptionLabel(firstPrompt.Options()[0])) + assert.Equal(t, "[S] Stop and exit", output.OptionLabel(firstPrompt.Options()[1])) } func TestStartupMonitorAwait_DismissesPromptWhenEmulatorBecomesReady(t *testing.T) { @@ -896,7 +896,7 @@ func TestStartupMonitorAwait_DismissesPromptWhenEmulatorBecomesReady(t *testing. require.NoError(t, err) prompt := <-prompts dismissal := <-dismissals - assert.Equal(t, prompt.ResponseCh, dismissal.ResponseCh) + assert.Equal(t, prompt.ResponseCh(), dismissal.ResponseCh) } func TestStartupMonitorAwait_DoesNotStopEmulatorThatBecameReadyBeforeSelection(t *testing.T) { @@ -917,7 +917,7 @@ func TestStartupMonitorAwait_DoesNotStopEmulatorThatBecameReadyBeforeSelection(t sink := output.SinkFunc(func(event output.Event) { if prompt, ok := event.(output.UserInputRequestEvent); ok { ready.Store(true) - prompt.ResponseCh <- output.InputResponse{SelectedKey: "s"} + prompt.ResponseCh() <- output.InputResponse{SelectedKey: "s"} } }) @@ -1650,7 +1650,7 @@ func TestStart_SecondLicenseRejectionAfterReloginRendersErrorEvent(t *testing.T) // Auto-answer every prompt (the re-login confirmation, then the login // flow's "press any key" completion prompt) as if the user pressed enter. if req, ok := event.(output.UserInputRequestEvent); ok { - req.ResponseCh <- output.InputResponse{SelectedKey: "enter"} + req.ResponseCh() <- output.InputResponse{SelectedKey: "enter"} } }) @@ -1690,7 +1690,7 @@ func TestPromptRelogin_FoldsReasonIntoThePromptWithoutASeparateWarning(t *testin sink := output.SinkFunc(func(event output.Event) { events = append(events, event) if req, ok := event.(output.UserInputRequestEvent); ok { - req.ResponseCh <- output.InputResponse{Cancelled: true} + req.ResponseCh() <- output.InputResponse{Cancelled: true} } }) @@ -1700,8 +1700,8 @@ func TestPromptRelogin_FoldsReasonIntoThePromptWithoutASeparateWarning(t *testin require.Len(t, events, 1, "the rejection reason must be folded into the prompt, not emitted as a separate message first") req, ok := events[0].(output.UserInputRequestEvent) require.True(t, ok, "the only event emitted must be the prompt itself") - assert.Contains(t, req.Prompt, licErr.Message, "the prompt must explain why the user is being asked to log in again") - assert.Equal(t, "Re-authenticate", req.Options[0].Label, "the recovery action belongs to the choice, not the prompt sentence") + assert.Contains(t, req.Prompt(), licErr.Message, "the prompt must explain why the user is being asked to log in again") + assert.Equal(t, "Re-authenticate", req.Options()[0].Label, "the recovery action belongs to the choice, not the prompt sentence") } // TestPromptRelogin_OffersAnAdvertisedDeclineKey covers DEVX-1045: Ctrl+C was the @@ -1724,22 +1724,22 @@ func TestPromptRelogin_OffersAnAdvertisedDeclineKey(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { if r, ok := event.(output.UserInputRequestEvent); ok { req = r - r.ResponseCh <- tc.response + r.ResponseCh() <- tc.response } }) accepted := promptRelogin(context.Background(), sink, licErr) assert.Equal(t, tc.accepted, accepted) - assert.True(t, req.Vertical, "the choices must render as vertical, selectable actions") + assert.True(t, req.Vertical(), "the choices must render as vertical, selectable actions") assert.Equal(t, []output.InputOption{ {Key: "r", Label: "Re-authenticate"}, {Key: "esc", Label: "Exit"}, - }, req.Options) + }, req.Options()) // Labels stay plain prose; the advertised keys come from output.OptionLabel. - assert.Equal(t, "[R] Re-authenticate", output.OptionLabel(req.Options[0]), + assert.Equal(t, "[R] Re-authenticate", output.OptionLabel(req.Options()[0]), "both the accept and the decline key must be advertised, shortcut first") - assert.Equal(t, "[ESC] Exit", output.OptionLabel(req.Options[1])) + assert.Equal(t, "[ESC] Exit", output.OptionLabel(req.Options()[1])) }) } } diff --git a/internal/output/events.go b/internal/output/events.go index cacd334b..64ec1d92 100644 --- a/internal/output/events.go +++ b/internal/output/events.go @@ -272,20 +272,40 @@ type InputResponse struct { } // UserInputRequestEvent asks the frontend to put a question to the user and -// send the answer back on ResponseCh. +// send the answer back on the response channel. // -// Build one with Confirm, ActionChoice, or Acknowledge (prompt.go) rather than -// by hand: naming what the prompt is settles how it renders, and a guard test -// fails the build on a raw literal outside this package. +// Build one with Confirm, ActionChoice, or Acknowledge (prompt.go). The fields +// are unexported so that is the only way: a struct literal asks its author to +// pick a rendering at the moment the question they can actually answer is what +// the prompt IS, and the cheapest answer — leaving the layout out — silently +// ships an inline prompt. That is how the license re-login prompt ended up with +// two advertised keys flattened into one dimmed hint (DEVX-1045). Naming the +// intent instead makes the layout a consequence rather than a decision. type UserInputRequestEvent struct { - Prompt string - Options []InputOption - ResponseCh chan<- InputResponse - // Vertical renders each option as its own selectable row instead of a - // trailing "[a/b]" hint. Set by ActionChoice; do not set it directly. - Vertical bool + prompt string + options []InputOption + responseCh chan<- InputResponse + // vertical renders each option as its own selectable row instead of a + // trailing "[a/b]" hint. + vertical bool } +// Prompt is the question put to the user. It may span several lines; the +// options are appended to the first one. +func (e UserInputRequestEvent) Prompt() string { return e.prompt } + +// Options are the answers the user may choose between. The returned slice is +// not copied — treat it as read-only. +func (e UserInputRequestEvent) Options() []InputOption { return e.options } + +// ResponseCh receives the user's answer. It also identifies the request, so a +// UserInputDismissEvent can name the exact prompt it retracts. +func (e UserInputRequestEvent) ResponseCh() chan<- InputResponse { return e.responseCh } + +// Vertical reports whether each option should render as its own selectable row +// rather than as a trailing "[a/b]" hint. Set by ActionChoice. +func (e UserInputRequestEvent) Vertical() bool { return e.vertical } + // UserInputDismissEvent removes a pending prompt when the condition that // required input resolves on its own. ResponseCh identifies the exact request // so a late dismissal cannot hide a newer prompt. diff --git a/internal/output/plain_format.go b/internal/output/plain_format.go index 567c2d9f..1e28ac31 100644 --- a/internal/output/plain_format.go +++ b/internal/output/plain_format.go @@ -112,20 +112,20 @@ func formatUserInputRequest(e UserInputRequestEvent) string { // than nested. Used wherever the full multi-line rendering does not fit: plain // output, and the TUI's spinner text. func FormatPromptEvent(e UserInputRequestEvent) string { - if !e.Vertical { - return FormatPrompt(e.Prompt, e.Options) + if !e.vertical { + return FormatPrompt(e.prompt, e.options) } - labels := make([]string, 0, len(e.Options)) - for _, opt := range e.Options { + labels := make([]string, 0, len(e.options)) + for _, opt := range e.options { if label := OptionLabel(opt); label != "" { labels = append(labels, label) } } if len(labels) == 0 { - return appendPromptSuffix(e.Prompt, "") + return appendPromptSuffix(e.prompt, "") } - return appendPromptSuffix(e.Prompt, " "+strings.Join(labels, " / ")) + return appendPromptSuffix(e.prompt, " "+strings.Join(labels, " / ")) } // FormatPromptLabels formats option labels into a suffix string. diff --git a/internal/output/prompt.go b/internal/output/prompt.go index fe2c7db0..b6344061 100644 --- a/internal/output/prompt.go +++ b/internal/output/prompt.go @@ -44,12 +44,12 @@ func Confirm(prompt string, def ConfirmDefault, responseCh chan<- InputResponse) yes, no = "y", "N" } return UserInputRequestEvent{ - Prompt: prompt, - Options: []InputOption{ + prompt: prompt, + options: []InputOption{ {Key: KeyYes, Label: yes}, {Key: KeyNo, Label: no}, }, - ResponseCh: responseCh, + responseCh: responseCh, } } @@ -70,10 +70,10 @@ func Confirm(prompt string, def ConfirmDefault, responseCh chan<- InputResponse) // requested, and Acknowledge when there is nothing to choose between. func ActionChoice(prompt string, options []InputOption, responseCh chan<- InputResponse) UserInputRequestEvent { return UserInputRequestEvent{ - Prompt: prompt, - Options: options, - ResponseCh: responseCh, - Vertical: true, + prompt: prompt, + options: options, + responseCh: responseCh, + vertical: true, } } @@ -82,9 +82,9 @@ func ActionChoice(prompt string, options []InputOption, responseCh chan<- InputR // is one option and every key selects it — so it stays on one line. func Acknowledge(prompt, label string, responseCh chan<- InputResponse) UserInputRequestEvent { return UserInputRequestEvent{ - Prompt: prompt, - Options: []InputOption{{Key: KeyAny, Label: label}}, - ResponseCh: responseCh, + prompt: prompt, + options: []InputOption{{Key: KeyAny, Label: label}}, + responseCh: responseCh, } } diff --git a/internal/output/prompt_guard_test.go b/internal/output/prompt_guard_test.go deleted file mode 100644 index 4909cf63..00000000 --- a/internal/output/prompt_guard_test.go +++ /dev/null @@ -1,115 +0,0 @@ -package output - -import ( - "go/ast" - "go/parser" - "go/token" - "io/fs" - "os" - "path/filepath" - "strings" - "testing" -) - -// TestPromptsUseIntentConstructors keeps every prompt in the CLI built through -// Confirm, ActionChoice, or Acknowledge. -// -// The point is not tidiness. A raw UserInputRequestEvent literal asks its author -// to pick a rendering (Vertical true or false) at a moment when the question -// they can actually answer is what the prompt IS — and the cheapest answer, -// leaving the field out, silently ships an inline prompt. That is how the -// license re-login prompt ended up with two advertised keys flattened into one -// dimmed hint (DEVX-1045). Naming the intent instead makes the layout a -// consequence rather than a decision. -// -// Test files are exempt: they construct events to drive the UI, not to ask a -// real user anything. test/integration is a separate module and out of scope. -func TestPromptsUseIntentConstructors(t *testing.T) { - t.Parallel() - - const guidance = "Use output.ActionChoice (a choice between distinct actions, rendered vertically), " + - "output.Confirm (y/n on an action the user already requested), or " + - "output.Acknowledge (a single key, no choice). If unsure which, ask the user." - - root := filepath.Join("..", "..") - // The constructors themselves live here, and they are the one place allowed - // to build the event by hand. - selfPkg := filepath.Join(root, "internal", "output") - - fset := token.NewFileSet() - // "." covers the root package (main.go) without descending into the module's - // other trees; WalkDir on it would pull in test/integration, a separate - // module, and every vendored or generated directory below. - for _, dir := range []string{".", "internal", "cmd"} { - walk := filepath.WalkDir - if dir == "." { - walk = walkDirTopLevel - } - err := walk(filepath.Join(root, dir), func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() { - if path == selfPkg { - return fs.SkipDir - } - return nil - } - if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { - return nil - } - - file, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) - if err != nil { - return err - } - - ast.Inspect(file, func(n ast.Node) bool { - lit, ok := n.(*ast.CompositeLit) - if !ok || !isUserInputRequestEvent(lit.Type) { - return true - } - rel, relErr := filepath.Rel(root, path) - if relErr != nil { - rel = path - } - t.Errorf("%s:%d builds a raw output.UserInputRequestEvent literal.\n%s", - filepath.ToSlash(rel), fset.Position(lit.Pos()).Line, guidance) - return true - }) - return nil - }) - if err != nil { - t.Fatalf("walking %s: %v", dir, err) - } - } -} - -// walkDirTopLevel visits the files directly inside dir, never its subdirectories. -func walkDirTopLevel(dir string, fn fs.WalkDirFunc) error { - entries, err := os.ReadDir(dir) - if err != nil { - return err - } - for _, entry := range entries { - if entry.IsDir() { - continue - } - if err := fn(filepath.Join(dir, entry.Name()), entry, nil); err != nil { - return err - } - } - return nil -} - -// isUserInputRequestEvent matches the type by name on either side of a possible -// package qualifier, so an import alias cannot slip a literal past the guard. -func isUserInputRequestEvent(expr ast.Expr) bool { - switch t := expr.(type) { - case *ast.Ident: - return t.Name == "UserInputRequestEvent" - case *ast.SelectorExpr: - return t.Sel.Name == "UserInputRequestEvent" - } - return false -} diff --git a/internal/output/prompt_test.go b/internal/output/prompt_test.go index 0f565d03..218debe3 100644 --- a/internal/output/prompt_test.go +++ b/internal/output/prompt_test.go @@ -29,11 +29,11 @@ func TestConfirmRendersInlineWithTheDefaultCapitalized(t *testing.T) { ch := make(chan InputResponse, 1) event := Confirm("Reset emulator state?", tt.def, ch) - assert.False(t, event.Vertical, "a confirmation stays on one line") - require.Len(t, event.Options, 2) - assert.Equal(t, KeyYes, event.Options[0].Key) - assert.Equal(t, KeyNo, event.Options[1].Key) - assert.Equal(t, tt.labels, []string{event.Options[0].Label, event.Options[1].Label}) + assert.False(t, event.Vertical(), "a confirmation stays on one line") + require.Len(t, event.Options(), 2) + assert.Equal(t, KeyYes, event.Options()[0].Key) + assert.Equal(t, KeyNo, event.Options()[1].Key) + assert.Equal(t, tt.labels, []string{event.Options()[0].Label, event.Options()[1].Label}) assert.Equal(t, "Reset emulator state?"+tt.hint, FormatPromptEvent(event)) }) } @@ -48,9 +48,9 @@ func TestActionChoiceRendersVerticallyWithDerivedShortcuts(t *testing.T) { {Key: "esc", Label: "Exit"}, }, ch) - assert.True(t, event.Vertical, "distinct actions render as selectable rows") - assert.Equal(t, "[ENTER] Log in again", OptionLabel(event.Options[0])) - assert.Equal(t, "[ESC] Exit", OptionLabel(event.Options[1])) + assert.True(t, event.Vertical(), "distinct actions render as selectable rows") + assert.Equal(t, "[ENTER] Log in again", OptionLabel(event.Options()[0])) + assert.Equal(t, "[ESC] Exit", OptionLabel(event.Options()[1])) // The one-line form keeps the shortcuts, so a prompt mirrored into spinner // text never leaves the user without a key to press. The labels bring their @@ -66,9 +66,9 @@ func TestAcknowledgeRendersInlineWithASingleAnyKeyOption(t *testing.T) { ch := make(chan InputResponse, 1) event := Acknowledge("Waiting for authorization...", "Press any key when complete", ch) - assert.False(t, event.Vertical) - require.Len(t, event.Options, 1) - assert.Equal(t, KeyAny, event.Options[0].Key) + assert.False(t, event.Vertical()) + require.Len(t, event.Options(), 1) + assert.Equal(t, KeyAny, event.Options()[0].Key) assert.Equal(t, "Waiting for authorization... (Press any key when complete)", FormatPromptEvent(event)) } diff --git a/internal/reset/reset_test.go b/internal/reset/reset_test.go index 13521da7..c2c37591 100644 --- a/internal/reset/reset_test.go +++ b/internal/reset/reset_test.go @@ -147,7 +147,7 @@ func TestReset_ConfirmYes(t *testing.T) { go func() { req := <-prompts - req.ResponseCh <- output.InputResponse{SelectedKey: "y"} + req.ResponseCh() <- output.InputResponse{SelectedKey: "y"} }() err := reset.Reset(context.Background(), healthyRunningMock(t), awsContainers, resetter, "host:4566", false, sink) @@ -163,7 +163,7 @@ func TestReset_ConfirmNo(t *testing.T) { go func() { req := <-prompts - req.ResponseCh <- output.InputResponse{SelectedKey: "n"} + req.ResponseCh() <- output.InputResponse{SelectedKey: "n"} }() err := reset.Reset(context.Background(), healthyRunningMock(t), awsContainers, resetter, "host:4566", false, sink) diff --git a/internal/ui/app.go b/internal/ui/app.go index 8e24217e..fc5d20c6 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -123,7 +123,7 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.String() == "ctrl+c" || msg.String() == "q" { var responseCmd tea.Cmd if a.pendingInput != nil { - responseCmd = sendInputResponseCmd(a.pendingInput.ResponseCh, output.InputResponse{Cancelled: true}) + responseCmd = sendInputResponseCmd(a.pendingInput.ResponseCh(), output.InputResponse{Cancelled: true}) a.pendingInput = nil a.inputPrompt = a.inputPrompt.Hide() } @@ -146,11 +146,11 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return a, skipCmd } if a.pendingInput != nil { - if a.pendingInput.Vertical { + if a.pendingInput.Vertical() { return a.handleVerticalPromptKey(msg) } - if opt := resolveOption(a.pendingInput.Options, msg); opt != nil { - responseCmd := sendInputResponseCmd(a.pendingInput.ResponseCh, output.InputResponse{SelectedKey: opt.Key}) + if opt := resolveOption(a.pendingInput.Options(), msg); opt != nil { + responseCmd := sendInputResponseCmd(a.pendingInput.ResponseCh(), output.InputResponse{SelectedKey: opt.Key}) a.pendingInput = nil a.inputPrompt = components.NewInputPrompt() a.spinner = a.spinner.SetText("") @@ -193,12 +193,12 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // blank screen while the domain waits on ResponseCh (DEVX-1045). The // spinner text is a mirror for as long as the spinner is on screen, since // View renders one or the other. - a.inputPrompt = a.inputPrompt.Show(msg.Prompt, msg.Options, msg.Vertical) + a.inputPrompt = a.inputPrompt.Show(msg.Prompt(), msg.Options(), msg.Vertical()) if a.spinner.Visible() { a.spinner = a.spinner.SetText(output.FormatPromptEvent(msg)) } case output.UserInputDismissEvent: - if a.pendingInput == nil || a.pendingInput.ResponseCh != msg.ResponseCh { + if a.pendingInput == nil || a.pendingInput.ResponseCh() != msg.ResponseCh { return a, nil } a.pendingInput = nil @@ -432,19 +432,20 @@ func (a App) handleVerticalPromptKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { a.inputPrompt = a.inputPrompt.SetSelectedIndex(a.inputPrompt.SelectedIndex() + 1) return a, nil case tea.KeyEnter: + options := a.pendingInput.Options() idx := a.inputPrompt.SelectedIndex() - if idx >= 0 && idx < len(a.pendingInput.Options) { - opt := a.pendingInput.Options[idx] + if idx >= 0 && idx < len(options) { + opt := options[idx] a.lines = appendLine(a.lines, styledLine{text: formatResolvedInput(*a.pendingInput, opt.Key)}) - responseCmd := sendInputResponseCmd(a.pendingInput.ResponseCh, output.InputResponse{SelectedKey: opt.Key}) + responseCmd := sendInputResponseCmd(a.pendingInput.ResponseCh(), output.InputResponse{SelectedKey: opt.Key}) a.pendingInput = nil a.inputPrompt = a.inputPrompt.Hide() return a, responseCmd } } - if opt := resolveOption(a.pendingInput.Options, msg); opt != nil { + if opt := resolveOption(a.pendingInput.Options(), msg); opt != nil { a.lines = appendLine(a.lines, styledLine{text: formatResolvedInput(*a.pendingInput, opt.Key)}) - responseCmd := sendInputResponseCmd(a.pendingInput.ResponseCh, output.InputResponse{SelectedKey: opt.Key}) + responseCmd := sendInputResponseCmd(a.pendingInput.ResponseCh(), output.InputResponse{SelectedKey: opt.Key}) a.pendingInput = nil a.inputPrompt = a.inputPrompt.Hide() return a, responseCmd @@ -455,7 +456,7 @@ func (a App) handleVerticalPromptKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { func formatResolvedInput(req output.UserInputRequestEvent, selectedKey string) string { selected := selectedKey hasLabels := false - for _, opt := range req.Options { + for _, opt := range req.Options() { if opt.Label != "" { hasLabels = true } @@ -464,15 +465,15 @@ func formatResolvedInput(req output.UserInputRequestEvent, selectedKey string) s } } - if req.Vertical { - firstLine := strings.Split(req.Prompt, "\n")[0] + if req.Vertical() { + firstLine := strings.Split(req.Prompt(), "\n")[0] if selected == "" || !hasLabels || selectedKey == output.KeyAny { return firstLine } return fmt.Sprintf("%s %s", firstLine, selected) } - formatted := output.FormatPrompt(req.Prompt, req.Options) + formatted := output.FormatPrompt(req.Prompt(), req.Options()) firstLine := strings.Split(formatted, "\n")[0] if selected == "" || !hasLabels || selectedKey == output.KeyAny { diff --git a/internal/ui/app_test.go b/internal/ui/app_test.go index 15569425..4f5d35f5 100644 --- a/internal/ui/app_test.go +++ b/internal/ui/app_test.go @@ -169,11 +169,7 @@ func TestAppEnterRespondsToInputRequest(t *testing.T) { app := NewApp("dev", "", "", nil) responseCh := make(chan output.InputResponse, 1) - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Press enter", - Options: []output.InputOption{{Key: "enter", Label: "Continue"}}, - ResponseCh: responseCh, - }) + model, _ := app.Update(output.Acknowledge("Press enter", "Continue", responseCh)) app = model.(App) if !app.inputPrompt.Visible() { @@ -189,8 +185,8 @@ func TestAppEnterRespondsToInputRequest(t *testing.T) { select { case resp := <-responseCh: - if resp.SelectedKey != "enter" { - t.Fatalf("expected enter key, got %q", resp.SelectedKey) + if resp.SelectedKey != output.KeyAny { + t.Fatalf("expected the any-key option, got %q", resp.SelectedKey) } case <-time.After(time.Second): t.Fatal("timed out waiting for response on channel") @@ -210,11 +206,9 @@ func TestAppDismissesOnlyTheMatchingPendingInput(t *testing.T) { responseCh := make(chan output.InputResponse, 1) prompt := "LocalStack is still starting." - model, _ = app.Update(output.UserInputRequestEvent{ - Prompt: prompt, - Options: []output.InputOption{{Key: "w", Label: "[W] Keep waiting"}}, - ResponseCh: responseCh, - }) + model, _ = app.Update(output.ActionChoice(prompt, []output.InputOption{ + {Key: "w", Label: "Keep waiting"}, + }, responseCh)) app = model.(App) model, _ = app.Update(output.UserInputDismissEvent{ResponseCh: make(chan output.InputResponse, 1)}) @@ -255,13 +249,12 @@ func TestAppPendingInputSurvivesDeferredSpinnerStop(t *testing.T) { t.Fatal("expected the spinner stop to be deferred by the min duration") } - prompt := "License validation failed. Log in again to refresh your credentials?" + prompt := "License validation failed: token expired." responseCh := make(chan output.InputResponse, 1) - model, _ = app.Update(output.UserInputRequestEvent{ - Prompt: prompt, - Options: []output.InputOption{{Key: "enter", Label: "ENTER to log in again"}}, - ResponseCh: responseCh, - }) + model, _ = app.Update(output.ActionChoice(prompt, []output.InputOption{ + {Key: "r", Label: "Re-authenticate"}, + {Key: "esc", Label: "Exit"}, + }, responseCh)) app = model.(App) model, _ = app.Update(components.SpinnerMinDurationElapsedMsg{}) @@ -286,8 +279,8 @@ func TestAppPendingInputSurvivesDeferredSpinnerStop(t *testing.T) { select { case resp := <-responseCh: - if resp.SelectedKey != "enter" { - t.Fatalf("expected enter key, got %q", resp.SelectedKey) + if resp.SelectedKey != "r" { + t.Fatalf("expected enter to select the highlighted action, got %q", resp.SelectedKey) } case <-time.After(time.Second): t.Fatal("timed out waiting for response on channel") @@ -299,28 +292,23 @@ func TestAppPendingInputSurvivesDeferredSpinnerStop(t *testing.T) { // TestAppPromptWrapsAtTerminalWidth covers the other half of DEVX-1045: a long // prompt has to be wrapped to the terminal width, since Bubble Tea's renderer -// would otherwise truncate the key hints off the right edge. +// would otherwise truncate the key hints off the right edge. An inline +// confirmation is the case that can lose them — its hint sits at the very end +// of the question rather than on rows of its own. func TestAppPromptWrapsAtTerminalWidth(t *testing.T) { t.Parallel() const width = 40 - question := "License validation failed: invalid, inactive, or expired authentication token or subscription. Log in again to refresh your credentials?" + question := "Delete cloud snapshot 'pod:nightly-regression-baseline'? This operation cannot be undone." app := NewApp("dev", "", "", nil) model, _ := app.Update(tea.WindowSizeMsg{Width: width}) app = model.(App) - model, _ = app.Update(output.UserInputRequestEvent{ - Prompt: question, - Options: []output.InputOption{ - {Key: "enter", Label: "ENTER to log in again"}, - {Key: "esc", Label: "ESC to exit"}, - }, - ResponseCh: make(chan output.InputResponse, 1), - }) + model, _ = app.Update(output.Confirm(question, output.DefaultNo, make(chan output.InputResponse, 1))) app = model.(App) view := app.View() - if !strings.Contains(view, "[ENTER to log in again/ESC to exit]") { + if !strings.Contains(view, "[y/N]") { t.Errorf("expected the key hints to be rendered, got:\n%s", view) } if strings.Contains(view, question) { @@ -335,11 +323,7 @@ func TestAppCtrlCCancelsPendingInput(t *testing.T) { app := NewApp("dev", "", "", func() { cancelled = true }) responseCh := make(chan output.InputResponse, 1) - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Press enter", - Options: []output.InputOption{{Key: "enter", Label: "Continue"}}, - ResponseCh: responseCh, - }) + model, _ := app.Update(output.Acknowledge("Press enter", "Continue", responseCh)) app = model.(App) model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyCtrlC}) @@ -648,134 +632,29 @@ func TestAppNonSilentErrorShowsInErrorDisplay(t *testing.T) { } } -func TestAppEnterPrefersExplicitEnterOption(t *testing.T) { - t.Parallel() - - app := NewApp("dev", "", "", nil) - responseCh := make(chan output.InputResponse, 1) - - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Open browser now?", - Options: []output.InputOption{ - {Key: "y", Label: "Y"}, - {Key: "n", Label: "n"}, - {Key: "enter", Label: "Press ENTER when complete"}, - }, - ResponseCh: responseCh, - }) - app = model.(App) - - model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) - app = model.(App) - if cmd == nil { - t.Fatal("expected response command") - } - cmd() - - select { - case resp := <-responseCh: - if resp.SelectedKey != "enter" { - t.Fatalf("expected enter key, got %q", resp.SelectedKey) - } - case <-time.After(time.Second): - t.Fatal("timed out waiting for response on channel") - } - - if app.inputPrompt.Visible() { - t.Fatal("expected input prompt to be hidden after response") - } -} - -func TestAppEnterSelectsUppercaseLabelDefault(t *testing.T) { - t.Parallel() - - app := NewApp("dev", "", "", nil) - responseCh := make(chan output.InputResponse, 1) - - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Open browser now?", - Options: []output.InputOption{ - {Key: "y", Label: "Y"}, - {Key: "n", Label: "n"}, - }, - ResponseCh: responseCh, - }) - app = model.(App) - - model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) - app = model.(App) - if cmd == nil { - t.Fatal("expected response command when enter is pressed with uppercase default") - } - cmd() - - select { - case resp := <-responseCh: - if resp.SelectedKey != "y" { - t.Fatalf("expected y key, got %q", resp.SelectedKey) - } - case <-time.After(time.Second): - t.Fatal("timed out waiting for response on channel") - } - - if app.inputPrompt.Visible() { - t.Fatal("expected input prompt to be hidden after response") - } -} - -func TestAppEnterDoesNothingWithoutDefault(t *testing.T) { +// TestAppUnmatchedKeyLeavesPromptPending covers the app-level half of a +// resolveOption miss: no response command, and the prompt stays on screen still +// waiting for an answer. +// +// The matching rules themselves belong to TestResolveOption, which feeds them +// arbitrary option slices. That is now the only place they can be exercised at +// all: every prompt a user sees comes from a constructor, so the shapes this +// file used to build by hand to reach them one at a time — an explicit "enter" +// option outranking an uppercase default, all-lowercase labels, non-letter +// labels — can no longer be built outside internal/output. +func TestAppUnmatchedKeyLeavesPromptPending(t *testing.T) { t.Parallel() app := NewApp("dev", "", "", nil) responseCh := make(chan output.InputResponse, 1) - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Choose:", - Options: []output.InputOption{ - {Key: "y", Label: "y"}, - {Key: "n", Label: "n"}, - }, - ResponseCh: responseCh, - }) + model, _ := app.Update(output.Confirm("Open browser now?", output.DefaultYes, responseCh)) app = model.(App) - model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) + model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'x'}}) app = model.(App) if cmd != nil { - t.Fatal("expected no response command when no uppercase default option exists") - } - - select { - case resp := <-responseCh: - t.Fatalf("expected no response, got %+v", resp) - case <-time.After(200 * time.Millisecond): - } - - if !app.inputPrompt.Visible() { - t.Fatal("expected input prompt to remain visible") - } -} - -func TestAppEnterDoesNothingWithNonLetterLabel(t *testing.T) { - t.Parallel() - - app := NewApp("dev", "", "", nil) - responseCh := make(chan output.InputResponse, 1) - - model, _ := app.Update(output.UserInputRequestEvent{ - Prompt: "Choose:", - Options: []output.InputOption{ - {Key: "1", Label: "1"}, - {Key: "2", Label: "2"}, - }, - ResponseCh: responseCh, - }) - app = model.(App) - - model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) - app = model.(App) - if cmd != nil { - t.Fatal("expected no response command when label contains no letters") + t.Fatal("expected no response command for a key no option claims") } select { @@ -933,7 +812,8 @@ func TestAppEnterHonorsTheConfirmDefault(t *testing.T) { model, _ := app.Update(output.Confirm("Reset emulator state? All resources will be lost", tc.def, responseCh)) app = model.(App) - _, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) + model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) + app = model.(App) if cmd == nil { t.Fatal("expected enter to resolve the confirmation") } @@ -947,6 +827,10 @@ func TestAppEnterHonorsTheConfirmDefault(t *testing.T) { case <-time.After(time.Second): t.Fatal("timed out waiting for response on channel") } + + if app.inputPrompt.Visible() { + t.Fatal("expected input prompt to be hidden after response") + } }) } } @@ -1075,14 +959,10 @@ func TestAppPendingInputOptionCOverridesClipboardShortcut(t *testing.T) { model, _ := app.Update(output.AuthEvent{URL: "https://example.com"}) app = model.(App) - model, _ = app.Update(output.UserInputRequestEvent{ - Prompt: "Choose option", - Options: []output.InputOption{ - {Key: "c", Label: "Continue"}, - {Key: "x", Label: "Cancel"}, - }, - ResponseCh: responseCh, - }) + model, _ = app.Update(output.ActionChoice("Choose option", []output.InputOption{ + {Key: "c", Label: "Continue"}, + {Key: "x", Label: "Cancel"}, + }, responseCh)) app = model.(App) model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'c'}}) @@ -1179,6 +1059,12 @@ func TestResolveOption(t *testing.T) { press: enter, wantOptionKey: "", }, + { + name: "all-lowercase labels leave Enter unanswered", + options: []output.InputOption{{Key: "y", Label: "y"}, {Key: "n", Label: "n"}}, + press: enter, + wantOptionKey: "", + }, // case-insensitive key matching { diff --git a/internal/update/notify_test.go b/internal/update/notify_test.go index 05f13321..499b0916 100644 --- a/internal/update/notify_test.go +++ b/internal/update/notify_test.go @@ -117,7 +117,7 @@ func TestNotifyUpdatePromptSkip(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) if req, ok := event.(output.UserInputRequestEvent); ok { - req.ResponseCh <- output.InputResponse{SelectedKey: "s"} + req.ResponseCh() <- output.InputResponse{SelectedKey: "s"} } }) @@ -155,7 +155,7 @@ func TestNotifyUpdatePromptRemind(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) if req, ok := event.(output.UserInputRequestEvent); ok { - req.ResponseCh <- output.InputResponse{SelectedKey: "r"} + req.ResponseCh() <- output.InputResponse{SelectedKey: "r"} } }) @@ -171,12 +171,12 @@ func TestNotifyUpdatePromptCancelled(t *testing.T) { sink := output.SinkFunc(func(event output.Event) { events = append(events, event) if req, ok := event.(output.UserInputRequestEvent); ok { - assert.Equal(t, "Update lstk to latest version?", req.Prompt) - 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) - req.ResponseCh <- output.InputResponse{Cancelled: true} + assert.Equal(t, "Update lstk to latest version?", req.Prompt()) + 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) + req.ResponseCh() <- output.InputResponse{Cancelled: true} } }) From d06321056d5b0e1163155caedb2d11859b3ec8f6 Mon Sep 17 00:00:00 2001 From: Misha Tiurin Date: Mon, 17 Aug 2026 14:01:54 +0200 Subject: [PATCH 3/6] Pin the snapshot remove confirmation's no default in an integration test Co-Authored-By: Claude --- test/integration/snapshot_remove_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/integration/snapshot_remove_test.go b/test/integration/snapshot_remove_test.go index 5f050912..b150cf0e 100644 --- a/test/integration/snapshot_remove_test.go +++ b/test/integration/snapshot_remove_test.go @@ -206,7 +206,11 @@ func TestSnapshotRemoveInteractive(t *testing.T) { With(env.LocalStackHost, lsHost(srv)). With(env.AuthToken, "test-token"), "snapshot", "remove", "pod:my-baseline") - p.waitForOutput("Delete cloud snapshot", "confirmation prompt should appear") + p.waitForOutput("Delete cloud snapshot 'pod:my-baseline'?", "confirmation prompt should appear") + // An irreversible delete capitalizes the answer ENTER picks, so a stray + // ENTER cancels instead of deleting. Asserted apart from the question + // because the hint moves to its own line when the question wraps. + p.waitForOutput("[y/N]", "the confirmation should advertise 'no' as its default") return p } From d097b3a0353eac4f2530a2d185170e8512d349cb Mon Sep 17 00:00:00 2001 From: Misha Tiurin Date: Mon, 17 Aug 2026 16:57:26 +0200 Subject: [PATCH 4/6] Simplify comments --- internal/output/events.go | 13 +++------- internal/output/prompt.go | 52 +++++++++++---------------------------- 2 files changed, 17 insertions(+), 48 deletions(-) diff --git a/internal/output/events.go b/internal/output/events.go index 64ec1d92..eabf4d01 100644 --- a/internal/output/events.go +++ b/internal/output/events.go @@ -271,16 +271,9 @@ type InputResponse struct { Cancelled bool } -// UserInputRequestEvent asks the frontend to put a question to the user and -// send the answer back on the response channel. -// -// Build one with Confirm, ActionChoice, or Acknowledge (prompt.go). The fields -// are unexported so that is the only way: a struct literal asks its author to -// pick a rendering at the moment the question they can actually answer is what -// the prompt IS, and the cheapest answer — leaving the layout out — silently -// ships an inline prompt. That is how the license re-login prompt ended up with -// two advertised keys flattened into one dimmed hint (DEVX-1045). Naming the -// intent instead makes the layout a consequence rather than a decision. +// Base struct for all user input requests. Not to be instantiated directly. +// Build user input requests with typical constructors defined in prompt.go, +// e.g. Confirm, ActionChoice, or Acknowledge. type UserInputRequestEvent struct { prompt string options []InputOption diff --git a/internal/output/prompt.go b/internal/output/prompt.go index b6344061..25a917ce 100644 --- a/internal/output/prompt.go +++ b/internal/output/prompt.go @@ -29,15 +29,10 @@ const ( // Confirm asks the user to approve an action they already requested, rendered // inline as "Reset emulator state? [y/N]". -// -// Inline is deliberate here and should stay that way: the question has one -// answer the user is already leaning toward, the [y/N] idiom is universal, it -// costs one line, and it carries its default in the capitalization. Pass -// DefaultNo for anything destructive or irreversible. +// Pass DefaultNo for anything destructive or irreversible. // // Use ActionChoice instead when the options are distinct outcomes rather than -// "do the thing I asked for, or don't". If a new prompt is not clearly one or -// the other, ask the user which it should be rather than guessing. +// "do the thing I asked for, or don't". func Confirm(prompt string, def ConfirmDefault, responseCh chan<- InputResponse) UserInputRequestEvent { yes, no := "Y", "n" if def == DefaultNo { @@ -60,14 +55,8 @@ func Confirm(prompt string, def ConfirmDefault, responseCh chan<- InputResponse) // ● [ENTER] Log in again // ○ [ESC] Exit // -// Labels are plain prose — OptionLabel derives the bracketed shortcut from each +// OptionLabel derives the bracketed shortcut from each // option's Key, so a label must not spell the key out itself. -// -// Vertical is deliberate here and should stay that way: flattening several -// distinct actions into a trailing "[a/b]" hint reads as prose glued to the end -// of the question, wraps badly, and gives the user nothing to arrow through -// (DEVX-1045). Use Confirm for a yes/no on an action the user already -// requested, and Acknowledge when there is nothing to choose between. func ActionChoice(prompt string, options []InputOption, responseCh chan<- InputResponse) UserInputRequestEvent { return UserInputRequestEvent{ prompt: prompt, @@ -77,9 +66,7 @@ func ActionChoice(prompt string, options []InputOption, responseCh chan<- InputR } } -// Acknowledge waits for any keypress, rendered inline as "Waiting for -// authorization... (Press any key when complete)". It is not a choice — there -// is one option and every key selects it — so it stays on one line. +// Acknowledge waits for any keypress after printing the label. func Acknowledge(prompt, label string, responseCh chan<- InputResponse) UserInputRequestEvent { return UserInputRequestEvent{ prompt: prompt, @@ -88,25 +75,6 @@ func Acknowledge(prompt, label string, responseCh chan<- InputResponse) UserInpu } } -// namedShortcuts spells out the keys whose names are not a single character. -// The values match what the terminal user is told to press, not tea.KeyType's -// own naming. -var namedShortcuts = map[string]string{ - "enter": "ENTER", - "esc": "ESC", - "space": "SPACE", - "tab": "TAB", -} - -// OptionLabel renders one option of a vertical prompt: its shortcut in -// brackets, then its label ("[ENTER] Log in again"). An option with no -// dedicated key — KeyAny, or an empty one — renders as the bare label, since -// there is no single key to advertise. -// -// Deriving the shortcut instead of leaving it to each label is what keeps the -// prompts consistent: hand-written labels had drifted into three styles -// ("[ENTER] Log in again", "Update now [U]", and a bare "AWS" that advertised -// no key at all) before this existed. func OptionLabel(opt InputOption) string { key := shortcut(opt.Key) if key == "" { @@ -118,8 +86,6 @@ func OptionLabel(opt InputOption) string { return fmt.Sprintf("[%s] %s", key, opt.Label) } -// shortcut returns the display form of an option key, or "" when the key names -// no single keypress the user can be told to hit. func shortcut(key string) string { if key == "" || key == KeyAny { return "" @@ -129,3 +95,13 @@ func shortcut(key string) string { } return strings.ToUpper(key) } + +// namedShortcuts spells out the keys whose names are not a single character. +// The values match what the terminal user is told to press, not tea.KeyType's +// own naming. +var namedShortcuts = map[string]string{ + "enter": "ENTER", + "esc": "ESC", + "space": "SPACE", + "tab": "TAB", +} From 2714264f89a7241da54ca4860dc19bc8a2941b34 Mon Sep 17 00:00:00 2001 From: Misha Tiurin Date: Mon, 17 Aug 2026 18:04:09 +0200 Subject: [PATCH 5/6] Simplify comments --- internal/output/events.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/output/events.go b/internal/output/events.go index eabf4d01..65f91653 100644 --- a/internal/output/events.go +++ b/internal/output/events.go @@ -283,12 +283,8 @@ type UserInputRequestEvent struct { vertical bool } -// Prompt is the question put to the user. It may span several lines; the -// options are appended to the first one. func (e UserInputRequestEvent) Prompt() string { return e.prompt } -// Options are the answers the user may choose between. The returned slice is -// not copied — treat it as read-only. func (e UserInputRequestEvent) Options() []InputOption { return e.options } // ResponseCh receives the user's answer. It also identifies the request, so a @@ -296,7 +292,7 @@ func (e UserInputRequestEvent) Options() []InputOption { return e.options } func (e UserInputRequestEvent) ResponseCh() chan<- InputResponse { return e.responseCh } // Vertical reports whether each option should render as its own selectable row -// rather than as a trailing "[a/b]" hint. Set by ActionChoice. +// rather than as a trailing "[a/b]" hint. func (e UserInputRequestEvent) Vertical() bool { return e.vertical } // UserInputDismissEvent removes a pending prompt when the condition that From f07e12d11fc5d86fd41deac74ad9a701dac1355b Mon Sep 17 00:00:00 2001 From: Misha Tiurin Date: Mon, 17 Aug 2026 18:31:26 +0200 Subject: [PATCH 6/6] Clarify new point in review-pr skill --- .claude/skills/review-pr/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 39f44b24..07338601 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -47,7 +47,7 @@ Go through each changed file and check for violations. Flag only actual problems - [ ] Domain code never reads from stdin directly - [ ] Interactive input uses `UserInputRequestEvent` + `ResponseCh` pattern -- [ ] The intent constructor a prompt uses matches what it is (the compiler enforces that one is used, not that it is the right one): a choice between distinct actions is `ActionChoice`, not a `Confirm` with an inline `[a/b]` hint, and an `ActionChoice` label does not spell out its own key +- [ ] `UserInputRequestEvent` is built using pre-defined constructors from `internal/output/prompt.go` - [ ] Non-TTY mode fails early with a helpful error if input would be required - [ ] New user-supplied inputs (args, flags, config values) are validated at the boundary via `internal/validate`; no new inline validation regexp duplicates an existing validator (pod names → `PodName`; opaque secrets → loose checks like `AuthToken`; paths/URLs → their existing parsers; other identifiers → the owning API's documented contract) and malformed-input cases are tested