diff --git a/go.mod b/go.mod index 30c7b8d..8ddbf38 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 github.com/schollz/progressbar/v3 v3.19.1 github.com/spf13/cobra v1.10.2 + golang.org/x/sys v0.47.0 golang.org/x/term v0.45.0 golang.org/x/text v0.40.0 gopkg.in/yaml.v3 v3.0.1 @@ -58,7 +59,6 @@ require ( go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/net v0.57.0 // indirect golang.org/x/oauth2 v0.34.0 // indirect - golang.org/x/sys v0.47.0 // indirect golang.org/x/time v0.14.0 // indirect google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af // indirect gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect diff --git a/internal/cli/console_input_other.go b/internal/cli/console_input_other.go new file mode 100644 index 0000000..9444861 --- /dev/null +++ b/internal/cli/console_input_other.go @@ -0,0 +1,14 @@ +//go:build !windows + +package cli + +// enableKeyEventInput is a no-op off Windows. +// +// The problem it solves is specific to the Windows console: survey navigates from +// VK_UP / VK_DOWN virtual-key records and breaks when ENABLE_VIRTUAL_TERMINAL_INPUT +// makes the console deliver arrows as ANSI escape sequences instead. On POSIX the +// terminal is put into raw mode and survey parses those escape sequences itself, +// which is the normal, working path — there is nothing to adjust. +// +// Returns a non-nil func so callers can `defer restore()` unconditionally. +func enableKeyEventInput() (restore func()) { return func() {} } diff --git a/internal/cli/console_input_test.go b/internal/cli/console_input_test.go new file mode 100644 index 0000000..d0d84ec --- /dev/null +++ b/internal/cli/console_input_test.go @@ -0,0 +1,110 @@ +package cli + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "regexp" + "strings" + "testing" +) + +// enableKeyEventInput sits in front of every interactive prompt, so its failure +// modes matter more than its happy path: if it can ever panic or return nil, a +// console quirk turns into a crashed command. Under `go test` stdin is not a +// console, so on Windows this exercises the GetConsoleMode-failed branch and off +// Windows the no-op — both of which must be silently harmless. +func TestEnableKeyEventInputIsAlwaysSafe(t *testing.T) { + restore := enableKeyEventInput() + if restore == nil { + t.Fatal("enableKeyEventInput returned a nil restore func; callers `defer restore()` unconditionally and would panic") + } + restore() + + // Prompts run in sequence in the guided flow, so this is called and restored + // many times per command. Nested/repeated use must not blow up either. + for i := 0; i < 3; i++ { + r := enableKeyEventInput() + if r == nil { + t.Fatalf("call %d returned a nil restore func", i) + } + defer r() + } +} + +// The bug this guards (#475): pressing the down arrow in the ingest task-type +// Select typed "[B" into the filter and never moved the selection, because the +// console was delivering arrows as ANSI escapes instead of VK_DOWN key events. +// The fix only holds while every prompt is wrapped, and the failure is invisible +// off Windows — nothing in a macOS/Linux test run or in CI would notice a fourth +// prompt added without the wrapper. So assert it structurally, at the seam where +// survey is actually called. +func TestEverySurveyPromptClearsVirtualTerminalInput(t *testing.T) { + const file = "interactive.go" + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, file, nil, parser.ParseComments) + if err != nil { + t.Fatalf("parsing %s: %v", file, err) + } + + // Walk each function that calls survey.AskOne and require the same function + // body to also call enableKeyEventInput. + ast.Inspect(f, func(n ast.Node) bool { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Body == nil { + return true + } + + var callsAskOne, callsEnable bool + ast.Inspect(fn.Body, func(m ast.Node) bool { + call, ok := m.(*ast.CallExpr) + if !ok { + return true + } + switch f := call.Fun.(type) { + case *ast.SelectorExpr: // survey.AskOne + if pkg, ok := f.X.(*ast.Ident); ok && pkg.Name == "survey" && f.Sel.Name == "AskOne" { + callsAskOne = true + } + case *ast.Ident: // enableKeyEventInput() + if f.Name == "enableKeyEventInput" { + callsEnable = true + } + } + return true + }) + + if callsAskOne && !callsEnable { + t.Errorf("%s: %s calls survey.AskOne without `defer enableKeyEventInput()()`.\n"+ + "On Windows the console can deliver arrow keys as ANSI escape sequences "+ + "instead of VK_UP/VK_DOWN key events; survey only understands the key events, "+ + "so arrows stop navigating and leak \"[B\" into the prompt (#475). "+ + "Every prompt needs the wrapper — it is a no-op everywhere else.", + file, fn.Name.Name) + } + return true + }) +} + +// Both build-tagged halves must expose the identical signature, or one platform +// fails to compile — and CI only builds some of them. Cheap to assert here. +func TestConsoleInputBuildTagsCoverEveryPlatform(t *testing.T) { + for _, tc := range []struct{ file, wantTag string }{ + {"console_input_windows.go", "//go:build windows"}, + {"console_input_other.go", "//go:build !windows"}, + } { + src, err := os.ReadFile(tc.file) + if err != nil { + t.Fatalf("reading %s: %v", tc.file, err) + } + body := string(src) + if !strings.HasPrefix(body, tc.wantTag+"\n") { + t.Errorf("%s must start with %q so the two halves are mutually exclusive and exhaustive", tc.file, tc.wantTag) + } + if !regexp.MustCompile(`func enableKeyEventInput\(\) \(restore func\(\)\) \{`).MatchString(body) { + t.Errorf("%s: enableKeyEventInput signature drifted; both halves must match or one platform will not build", tc.file) + } + } +} diff --git a/internal/cli/console_input_windows.go b/internal/cli/console_input_windows.go new file mode 100644 index 0000000..67cf385 --- /dev/null +++ b/internal/cli/console_input_windows.go @@ -0,0 +1,60 @@ +//go:build windows + +package cli + +import ( + "os" + + "golang.org/x/sys/windows" +) + +// enableKeyEventInput makes arrow keys work in the guided prompts on Windows. +// +// The bug it fixes: pressing ↓ in a Select typed "[B" into the filter box and never +// moved the selection. +// +// Why that happens. survey's Windows rune reader (terminal/runereader_windows.go) +// reads console records with ReadConsoleInputW and recognises navigation from the +// VIRTUAL KEY codes VK_UP / VK_DOWN. Before reading it clears ENABLE_ECHO_INPUT, +// ENABLE_LINE_INPUT and ENABLE_PROCESSED_INPUT — but NOT +// ENABLE_VIRTUAL_TERMINAL_INPUT. With that flag set the console stops delivering +// arrows as VK_* events and delivers them as ANSI escape sequences (ESC '[' 'B') +// instead, so survey sees three ordinary runes: the ESC is swallowed and "[B" lands +// in the filter. Navigation can never fire because no VK_DOWN ever arrives. +// +// Why it can appear "suddenly": console input mode is state on the console handle, +// not something the CLI chooses. A terminal that opts into VT input, or any program +// in the session that sets the flag and does not restore it, changes this with no +// change on our side. So clearing the flag for the duration of a prompt is the fix +// regardless of who turned it on. +// +// Contract: returns a restore func that is ALWAYS safe to call (never nil). If the +// mode cannot be read or set — stdin redirected to a pipe or file, no console +// attached, a hardened environment that refuses the call — everything is a no-op and +// prompting proceeds exactly as before. Interactive niceness must never be able to +// break a run. +func enableKeyEventInput() (restore func()) { + noop := func() {} + + h := windows.Handle(os.Stdin.Fd()) + + var mode uint32 + if err := windows.GetConsoleMode(h, &mode); err != nil { + // Not a console (piped/redirected stdin, or no console at all). + return noop + } + if mode&windows.ENABLE_VIRTUAL_TERMINAL_INPUT == 0 { + // Already delivering VK_* key events — survey's path works, leave it alone. + return noop + } + + want := mode &^ windows.ENABLE_VIRTUAL_TERMINAL_INPUT + if err := windows.SetConsoleMode(h, want); err != nil { + return noop + } + + // Restore the caller's mode. The user's shell owns this state, so we put back + // exactly what we found rather than a value we think is right — leaving VT input + // off would change how the terminal behaves after the CLI exits. + return func() { _ = windows.SetConsoleMode(h, mode) } +} diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index aff4fe8..d28a097 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -66,6 +66,7 @@ func (s surveyPrompter) Input(label, help, def string, validate func(string) err return validate(s) })) } + defer enableKeyEventInput()() if err := survey.AskOne(q, &ans, opts...); err != nil { return "", mapErr(err) } @@ -75,6 +76,10 @@ func (s surveyPrompter) Input(label, help, def string, validate func(string) err func (s surveyPrompter) Select(label, help string, options []string, def string) (string, error) { var ans string q := &survey.Select{Message: s.message(label), Help: help, Options: options, Default: def} + // Arrow keys only work while the console delivers VK_* key events; see + // enableKeyEventInput. Without this, ↓ typed "[B" into the filter on Windows and + // the selection never moved (#475). + defer enableKeyEventInput()() if err := survey.AskOne(q, &ans); err != nil { return "", mapErr(err) } @@ -87,6 +92,7 @@ func (s surveyPrompter) Confirm(label string, def bool) (bool, error) { // the cluster phase, with nothing printed before it — a bare "? (y/N)" // there would be a label-less destructive prompt. ans := def + defer enableKeyEventInput()() if err := survey.AskOne(&survey.Confirm{Message: label, Default: def}, &ans); err != nil { return false, mapErr(err) }