Fix y/n questions being skipped after the first one
Every y/n prompt after the first in a session answered itself with its default and left the keypress queued for the next command line. MIN and TIME are not part of the canonical/non-canonical switch: they live in their own slots of the control-character array and survive `stty icanon`. drainTTY left them at "min 0 time 0" — return whatever is buffered, do not wait — and the restore named only icanon and echo, so the next read returned zero bytes without ever waiting for a key. Set MIN and TIME explicitly on the way in, and restore the terminal from the state captured with `stty -g` instead of naming the flags we changed; mgsh now hands the terminal back exactly as it found it, where before "min 0" outlived mgsh itself and broke the next program's single-key reads too. getkey also reports whether it got a key at all. When it did not, the answer is no whatever the default says: a question nobody saw must not be taken as consent. All five call sites default to no, so the bug never destroyed anything — it only made agreeing impossible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -1,5 +1,11 @@
|
||||
package main
|
||||
|
||||
// input.go — the y/n prompts that guard the destructive commands.
|
||||
//
|
||||
// These questions are the only thing standing between `init` and a wiped
|
||||
// server repository, so the terminal handling here has to be exactly right: a
|
||||
// question that cannot be answered is worse than no question at all.
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"os"
|
||||
@@ -13,50 +19,86 @@ import (
|
||||
// variable because it guards the destructive operations: tests replace it to
|
||||
// drive those paths without a terminal, and to assert that the question was
|
||||
// asked at all.
|
||||
//
|
||||
// When the keypress cannot be read at all the answer is no, whatever the
|
||||
// default says — a question nobody saw must never be taken as consent.
|
||||
var yesno = func(prompt string, def bool) bool {
|
||||
suffix := " y/N ? "
|
||||
if def {
|
||||
suffix = " Y/n ? "
|
||||
}
|
||||
ans := strings.ToLower(strings.TrimSpace(getkey(prompt + suffix)))
|
||||
key, ok := getkey(prompt + suffix)
|
||||
if !ok {
|
||||
errorln("could not read an answer from the terminal — assuming no")
|
||||
return false
|
||||
}
|
||||
ans := strings.ToLower(strings.TrimSpace(key))
|
||||
if ans == "" {
|
||||
return def
|
||||
}
|
||||
return ans == "y"
|
||||
}
|
||||
|
||||
// getkey reads a single keypress from the terminal without echo. It reads a
|
||||
// single byte directly from stdin; readline is not reading at this point (we
|
||||
// are inside command execution), so there is no reader to desync with.
|
||||
// keyModeArgs put the terminal into single-key input: one keypress is delivered
|
||||
// as it is typed, and it is not echoed.
|
||||
//
|
||||
// MIN and TIME are set explicitly, and that is not decoration. They are not
|
||||
// part of the canonical/non-canonical switch — they live in their own slots of
|
||||
// the control-character array and survive `stty icanon`. drainTTY leaves them
|
||||
// at "min 0 time 0" ("return what is buffered, do not wait"), so without this
|
||||
// the *second* question of a session read zero bytes, answered itself with its
|
||||
// default, and left the keypress queued for the next prompt line.
|
||||
var keyModeArgs = []string{"-icanon", "-echo", "min", "1", "time", "0"}
|
||||
|
||||
// getkey reads a single keypress from the terminal without echo, and reports
|
||||
// whether it got one. It reads a single byte directly from stdin; readline only
|
||||
// reads while it is inside Readline(), and we are inside command execution
|
||||
// here, so there is no reader to desync with.
|
||||
//
|
||||
// Anything else already typed on the same line is discarded: answering "yes"
|
||||
// to a y/n prompt must not leave "es\n" queued for the next readline call,
|
||||
// where it would come back as a bogus command.
|
||||
func getkey(prompt string) string {
|
||||
func getkey(prompt string) (string, bool) {
|
||||
fmt.Print(prompt)
|
||||
tty := readline.IsTerminal(int(os.Stdin.Fd()))
|
||||
if tty {
|
||||
stty("-icanon", "-echo")
|
||||
|
||||
restore := func() {}
|
||||
if stdinIsTTY() {
|
||||
restore = singleKeyMode()
|
||||
}
|
||||
var buf [1]byte
|
||||
n, err := os.Stdin.Read(buf[:])
|
||||
if tty {
|
||||
drainTTY()
|
||||
stty("icanon", "echo")
|
||||
}
|
||||
key := ""
|
||||
if err == nil && n > 0 {
|
||||
key = strings.Trim(string(buf[:n]), "\r\n\t")
|
||||
restore()
|
||||
|
||||
if err != nil || n == 0 {
|
||||
fmt.Println()
|
||||
return "", false
|
||||
}
|
||||
key := strings.Trim(string(buf[:n]), "\r\n\t")
|
||||
fmt.Println(key)
|
||||
return key
|
||||
return key, true
|
||||
}
|
||||
|
||||
// singleKeyMode switches the terminal to single-key input and returns the
|
||||
// function that puts it back. The previous settings are restored verbatim from
|
||||
// `stty -g` rather than by naming the flags we changed: naming them is how the
|
||||
// MIN/TIME above were left behind in the first place, and mgsh should hand the
|
||||
// terminal back exactly as it found it.
|
||||
func singleKeyMode() func() {
|
||||
saved, err := sttyRun("-g")
|
||||
saved = strings.TrimSpace(saved)
|
||||
sttyRun(keyModeArgs...)
|
||||
|
||||
if err != nil || saved == "" {
|
||||
return func() { drainTTY(); sttyRun("icanon", "echo") } // best effort
|
||||
}
|
||||
return func() { drainTTY(); sttyRun(strings.Fields(saved)...) }
|
||||
}
|
||||
|
||||
// drainTTY discards input already queued on the terminal. `min 0 time 0` makes
|
||||
// a read return whatever is buffered without waiting, so this cannot block when
|
||||
// nothing is pending.
|
||||
// nothing is pending. Its caller restores the terminal afterwards.
|
||||
func drainTTY() {
|
||||
stty("-icanon", "-echo", "min", "0", "time", "0")
|
||||
sttyRun("-icanon", "-echo", "min", "0", "time", "0")
|
||||
buf := make([]byte, 256)
|
||||
for {
|
||||
n, err := os.Stdin.Read(buf)
|
||||
@@ -66,10 +108,16 @@ func drainTTY() {
|
||||
}
|
||||
}
|
||||
|
||||
func stty(args ...string) {
|
||||
// stdinIsTTY reports whether keypresses come from a terminal. A variable so the
|
||||
// tests can exercise the terminal path without one.
|
||||
var stdinIsTTY = func() bool { return readline.IsTerminal(int(os.Stdin.Fd())) }
|
||||
|
||||
// sttyRun runs stty on the terminal and returns its output. Errors are silent:
|
||||
// every caller has a fallback, and a stray "stty: ..." line in the middle of a
|
||||
// half-printed question helps nobody.
|
||||
var sttyRun = func(args ...string) (string, error) {
|
||||
c := exec.Command("stty", args...)
|
||||
c.Stdin = os.Stdin
|
||||
c.Stdout = os.Stdout
|
||||
c.Stderr = os.Stderr
|
||||
c.Run()
|
||||
out, err := c.Output()
|
||||
return string(out), err
|
||||
}
|
||||
|
||||
+226
@@ -0,0 +1,226 @@
|
||||
package main
|
||||
|
||||
// input_test.go — the y/n prompt is the last thing between `init` and a wiped
|
||||
// server repository, so the terminal handling around it is pinned here.
|
||||
|
||||
import (
|
||||
"os"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// fakeTTY stands in for the terminal driver. It models the one detail that made
|
||||
// the skipped-question bug possible: MIN and TIME are not part of the
|
||||
// canonical/non-canonical switch, so they survive `stty icanon` and carry over
|
||||
// into the next prompt.
|
||||
type fakeTTY struct {
|
||||
mu sync.Mutex
|
||||
state map[string]string
|
||||
calls [][]string
|
||||
ready chan struct{} // signalled once single-key mode is in effect
|
||||
}
|
||||
|
||||
func newFakeTTY() *fakeTTY {
|
||||
return &fakeTTY{
|
||||
state: map[string]string{"icanon": "on", "echo": "on", "min": "1", "time": "0"},
|
||||
ready: make(chan struct{}, 4),
|
||||
}
|
||||
}
|
||||
|
||||
func (f *fakeTTY) run(args ...string) (string, error) {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
f.calls = append(f.calls, args)
|
||||
|
||||
switch {
|
||||
case len(args) == 1 && args[0] == "-g":
|
||||
return f.serializeLocked(), nil
|
||||
case len(args) == 1 && strings.HasPrefix(args[0], "saved:"):
|
||||
for _, kv := range strings.Split(strings.TrimPrefix(args[0], "saved:"), ",") {
|
||||
if k, v, ok := strings.Cut(kv, "="); ok {
|
||||
f.state[k] = v
|
||||
}
|
||||
}
|
||||
return "", nil
|
||||
}
|
||||
for i := 0; i < len(args); i++ {
|
||||
switch a := args[i]; a {
|
||||
case "min", "time":
|
||||
if i+1 < len(args) {
|
||||
f.state[a] = args[i+1]
|
||||
i++
|
||||
}
|
||||
default:
|
||||
f.state[strings.TrimPrefix(a, "-")] = boolWord(!strings.HasPrefix(a, "-"))
|
||||
}
|
||||
}
|
||||
if strings.Join(args, " ") == strings.Join(keyModeArgs, " ") {
|
||||
f.ready <- struct{}{} // single-key mode is set; the read comes next
|
||||
}
|
||||
return "", nil
|
||||
}
|
||||
|
||||
func boolWord(on bool) string {
|
||||
if on {
|
||||
return "on"
|
||||
}
|
||||
return "off"
|
||||
}
|
||||
|
||||
// serializeLocked renders the settings as one token, the way `stty -g` does.
|
||||
func (f *fakeTTY) serializeLocked() string {
|
||||
return "saved:icanon=" + f.state["icanon"] + ",echo=" + f.state["echo"] +
|
||||
",min=" + f.state["min"] + ",time=" + f.state["time"]
|
||||
}
|
||||
|
||||
func (f *fakeTTY) get(k string) string {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
return f.state[k]
|
||||
}
|
||||
|
||||
func (f *fakeTTY) snapshot() string {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
return f.serializeLocked()
|
||||
}
|
||||
|
||||
// installFakeTTY points getkey at the fake terminal for the duration of a test.
|
||||
func installFakeTTY(t *testing.T) *fakeTTY {
|
||||
t.Helper()
|
||||
f := newFakeTTY()
|
||||
oldRun, oldIsTTY, oldStdin := sttyRun, stdinIsTTY, os.Stdin
|
||||
sttyRun = f.run
|
||||
stdinIsTTY = func() bool { return true }
|
||||
t.Cleanup(func() {
|
||||
sttyRun, stdinIsTTY, os.Stdin = oldRun, oldIsTTY, oldStdin
|
||||
})
|
||||
return f
|
||||
}
|
||||
|
||||
// askOnce runs one getkey against the fake terminal, answering with keys once
|
||||
// the terminal is actually in single-key mode. It reports the MIN in effect at
|
||||
// the moment of the read — the value the old code got wrong.
|
||||
func askOnce(t *testing.T, f *fakeTTY, keys string) (key string, ok bool, minAtRead string) {
|
||||
t.Helper()
|
||||
r, w, err := os.Pipe()
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
os.Stdin = r
|
||||
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
key, ok = getkey("question? ")
|
||||
close(done)
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-f.ready:
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("the terminal was never put into single-key mode")
|
||||
}
|
||||
minAtRead = f.get("min")
|
||||
w.WriteString(keys)
|
||||
w.Close() // so the drain that follows the keypress sees EOF instead of blocking
|
||||
<-done
|
||||
r.Close()
|
||||
return key, ok, minAtRead
|
||||
}
|
||||
|
||||
// TestEveryQuestionWaitsForAnAnswer is the regression test for the bug where the
|
||||
// second y/n question of a session was skipped: drainTTY left the terminal at
|
||||
// "min 0" ("return what is buffered, do not wait"), the restore only named
|
||||
// icanon and echo, and so the next read returned zero bytes and answered the
|
||||
// question with its default.
|
||||
func TestEveryQuestionWaitsForAnAnswer(t *testing.T) {
|
||||
f := installFakeTTY(t)
|
||||
|
||||
for i, want := range []string{"y", "n", "y"} {
|
||||
key, ok, minAtRead := askOnce(t, f, want)
|
||||
if !ok || key != want {
|
||||
t.Fatalf("question %d: got (%q, %v), want (%q, true)", i+1, key, ok, want)
|
||||
}
|
||||
if minAtRead != "1" {
|
||||
t.Errorf("question %d read the terminal at min=%s, want min=1 — "+
|
||||
"min 0 returns without waiting and answers the question by itself",
|
||||
i+1, minAtRead)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestPromptHandsTheTerminalBackUnchanged: mgsh must not leave the terminal in
|
||||
// a mode it chose. The old restore named only icanon and echo and left "min 0"
|
||||
// behind, which outlived mgsh itself and broke the next program's single-key
|
||||
// reads too.
|
||||
func TestPromptHandsTheTerminalBackUnchanged(t *testing.T) {
|
||||
f := installFakeTTY(t)
|
||||
initial := f.snapshot()
|
||||
|
||||
if _, ok, _ := askOnce(t, f, "y"); !ok {
|
||||
t.Fatal("getkey did not read the key")
|
||||
}
|
||||
if got := f.snapshot(); got != initial {
|
||||
t.Errorf("terminal left as %s, want it back at %s", got, initial)
|
||||
}
|
||||
}
|
||||
|
||||
// TestPromptRestoresFromTheSavedState pins *how* the terminal is restored: from
|
||||
// the state captured with `stty -g`, not by naming the flags we changed.
|
||||
func TestPromptRestoresFromTheSavedState(t *testing.T) {
|
||||
f := installFakeTTY(t)
|
||||
if _, ok, _ := askOnce(t, f, "y"); !ok {
|
||||
t.Fatal("getkey did not read the key")
|
||||
}
|
||||
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
if len(f.calls) == 0 {
|
||||
t.Fatal("stty was never called")
|
||||
}
|
||||
if first := f.calls[0]; len(first) != 1 || first[0] != "-g" {
|
||||
t.Errorf("first stty call was %v, want [-g]: the state has to be captured "+
|
||||
"before it is changed", first)
|
||||
}
|
||||
last := f.calls[len(f.calls)-1]
|
||||
if len(last) != 1 || !strings.HasPrefix(last[0], "saved:") {
|
||||
t.Errorf("last stty call was %v, want the saved state played back", last)
|
||||
}
|
||||
}
|
||||
|
||||
// TestSingleKeyModeSetsMinExplicitly: MIN and TIME are inherited, so they have
|
||||
// to be named on the way in. Without that, a terminal left at "min 0" by an
|
||||
// earlier program (or an earlier mgsh) skips the question.
|
||||
func TestSingleKeyModeSetsMinExplicitly(t *testing.T) {
|
||||
args := strings.Join(keyModeArgs, " ")
|
||||
for _, want := range []string{"-icanon", "-echo", "min 1", "time 0"} {
|
||||
if !strings.Contains(args, want) {
|
||||
t.Errorf("single-key mode is %q, missing %q", args, want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestQuestionSkippedByAPoisonedTerminalIsNotAYes: even if the terminal is
|
||||
// already in the broken state when mgsh starts, the answer must not be the
|
||||
// default — nobody saw the question, so nobody agreed to anything.
|
||||
func TestQuestionSkippedByAPoisonedTerminalIsNotAYes(t *testing.T) {
|
||||
oldRun, oldIsTTY, oldStdin := sttyRun, stdinIsTTY, os.Stdin
|
||||
defer func() { sttyRun, stdinIsTTY, os.Stdin = oldRun, oldIsTTY, oldStdin }()
|
||||
|
||||
sttyRun = func(args ...string) (string, error) { return "", nil }
|
||||
stdinIsTTY = func() bool { return true }
|
||||
|
||||
r, w, _ := os.Pipe()
|
||||
w.Close() // a terminal that returns nothing at all
|
||||
os.Stdin = r
|
||||
defer r.Close()
|
||||
|
||||
if key, ok := getkey("question? "); ok {
|
||||
t.Fatalf("getkey reported a key %q from a terminal that gave none", key)
|
||||
}
|
||||
if yesno("destroy everything?", true) {
|
||||
t.Error("an unanswerable question was taken as yes")
|
||||
}
|
||||
}
|
||||
+1
-1
@@ -1 +1 @@
|
||||
4.0.56
|
||||
4.0.57
|
||||
|
||||
Reference in New Issue
Block a user