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>

input.go

@@ -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
+// keyModeArgs put the terminal into single-key input: one keypress is delivered
-// single byte directly from stdin; readline is not reading at this point (we
+// as it is typed, and it is not echoed.
-// are inside command execution), so there is no reader to desync with.
+//
+// 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 {
+	restore := func() {}
-		stty("-icanon", "-echo")
+	if stdinIsTTY() {
+		restore = singleKeyMode()
 	}
 	var buf [1]byte
 	n, err := os.Stdin.Read(buf[:])
-	if tty {
+	restore()
-		drainTTY()
+
-		stty("icanon", "echo")
+	if err != nil || n == 0 {
-	}
+		fmt.Println()
-	key := ""
+		return "", false
-	if err == nil && n > 0 {
-		key = strings.Trim(string(buf[:n]), "\r\n\t")
 	}
+	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
+	out, err := c.Output()
-	c.Stderr = os.Stderr
+	return string(out), err
-	c.Run()
 }

input_test.go

@@ -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")
+	}
+}

version.txt

@@ -1 +1 @@
-4.0.55
+4.0.57