diff --git a/docs/memory/wt-cli/index.md b/docs/memory/wt-cli/index.md index 77b3ed3..dabd921 100644 --- a/docs/memory/wt-cli/index.md +++ b/docs/memory/wt-cli/index.md @@ -13,6 +13,6 @@ description: "Behavior contracts for the `wt` CLI binary — commands, exit code | [idle-staleness-contract](idle-staleness-contract.md) | The shared idle predicate, the `wt delete --stale` selector, and the safety invariant that idleness never gates a deletion on its own. | | [init-failure-contract](init-failure-contract.md) | Init-failure behavior of `wt create` / `wt init` — kept-worktree contract, `ExitInitFailed` on every path, the interactive open-anyway prompt, the `wt go` banner hint, SIGINT handling, and terminal-foreground reclaim. | | [list-status-contract](list-status-contract.md) | `wt list` output contract — enrichment-free default, `--status` opt-in dashboard, `--sort` ordering, and pointer-field JSON shape. | -| [menu-navigation-contract](menu-navigation-contract.md) | Arrow-key navigation contract for the shared `ShowMenu` — TTY gating, keybindings, a terminal-height-aware scrolling viewport, and a byte-identical non-TTY fallback. | +| [menu-navigation-contract](menu-navigation-contract.md) | Arrow-key navigation contract for the shared `ShowMenu` — TTY gating, keybindings, a terminal-height-aware scrolling viewport, a byte-identical non-TTY fallback, and the single-reader `MenuSession` extended to the menu→line-prompt seam. | | [recency-ordering-contract](recency-ordering-contract.md) | The single recency definition (`RecencyOf`/`RecencyLess`/`SortByRecency`) and newest-first ordering across `wt list`, `wt open`, and `wt delete`. | | [update-command-contract](update-command-contract.md) | `wt update` self-upgrade contract and the cross-toolkit `--skip-brew-update` flag. | diff --git a/docs/memory/wt-cli/menu-navigation-contract.md b/docs/memory/wt-cli/menu-navigation-contract.md index 09aee0b..ef8eef6 100644 --- a/docs/memory/wt-cli/menu-navigation-contract.md +++ b/docs/memory/wt-cli/menu-navigation-contract.md @@ -1,14 +1,17 @@ --- type: memory -description: "Arrow-key navigation contract for the shared `ShowMenu` — TTY gating, keybindings, a terminal-height-aware scrolling viewport, and a byte-identical non-TTY fallback." +description: "Arrow-key navigation contract for the shared `ShowMenu` — TTY gating, keybindings, a terminal-height-aware scrolling viewport, a byte-identical non-TTY fallback, and the single-reader `MenuSession` extended to the menu→line-prompt seam." --- # wt-cli: Menu Navigation Contract > Post-implementation behavior capture for the arrow-key menu navigation upgrade. > Source change: `260516-dkg7-arrow-key-menu-navigation`. > Amended by `260705-3wr8-menu-viewport-scrolling` (terminal-height-aware scrolling viewport). +> Amended by `260708-wryx-menu-prompt-byte-theft` (the single-reader `MenuSession` contract extended to the menu→line-prompt seam — session-aware `PromptWithDefault`/`ConfirmYesNo`, and `wt create` joining the session-threading pattern). -This file documents the contract that `ShowMenu` honors after the arrow-key navigation change. Future changes touching `src/internal/worktree/menu.go` should preserve these invariants unless an explicit spec amendment supersedes them. The change affects every interactive prompt invoked by `wt` (most prominently the `wt open` / `wt delete` worktree pickers) — but only via the shared `ShowMenu` entry point. No call site under `src/cmd/wt/` was edited. +This file documents the contract that `ShowMenu` honors after the arrow-key navigation change. Future changes touching `src/internal/worktree/menu.go` should preserve these invariants unless an explicit spec amendment supersedes them. The original arrow-key change affected every interactive prompt invoked by `wt` (most prominently the `wt open` / `wt delete` worktree pickers) — but only via the shared `ShowMenu` entry point, editing no call site under `src/cmd/wt/`. That original no-call-site-edited property was later broken *deliberately* by the `260708-wryx` amendment, which edited `wt create`'s `RunE` to thread one `MenuSession` through its interactive flow (see the Scope note below and § `wt create` joins the session-threading pattern). + +> **Scope note (`260708-wryx`).** The original arrow-key change touched no `cmd/` call site; the byte-theft-seam amendment does — `wt create`'s `RunE` was edited to thread one `MenuSession` through its whole interactive flow (see § The menu→line-prompt seam below). The menu *navigation* contract above is unchanged; the amendment sits on the `MenuSession` single-reader invariant this file already carries (§ `wt go` is a new `ShowMenu`/`MenuSession` caller) and extends it from menu→menu to menu→line-prompt. ## Requirements @@ -103,6 +106,26 @@ Before this change the renderer painted its full region (1 prompt + N options + - **Non-TTY fallback**: `wt go`'s no-arg menu degrades through the same byte-identical numbered-prompt fallback as every other caller — `isInteractiveTTY()` gates it, and piped/CI invocations land on the historical numbered prompt automatically. `wt go` adds a separate, earlier guard for the **`--non-interactive` no-arg** case: it refuses with `ExitGeneralError` *before* reaching `selectWorktree` at all (a no-arg selection has no non-interactive default — see `/wt-cli/go-command-contract.md`), so that path never renders even the fallback prompt. `wt go ` and `wt go` interactive both reach the menu only when a menu is actually wanted. - **Shared `MenuSession` for the launch-chaining callers**: `selectAndOpen` and `wt open --go`'s `openGo` pass ONE `MenuSession` to both `selectWorktree` (the "Select worktree…" menu) and `handleAppMenuWithSession` (the "Open in:" menu), so the two consecutive menus share a single stdin reader — the documented byte-theft fix. `wt go` chains no second menu, so it uses a one-shot session. The single-reader requirement and why it matters are the `MenuSession` contract this file's navigation behavior sits on. +### The menu→line-prompt seam: session-aware line prompts (`260708-wryx`) + +Before this change the `MenuSession` single-reader guarantee covered menu→menu seams only. **Line prompts** (`PromptWithDefault`, `ConfirmYesNo`) each built their own fresh `bufio.NewReader(os.Stdin)` and were not session-aware, so a `Show` followed by a package-level line prompt on the same stdin left TWO readers on the fd. A menu's read-ahead pump (`blockingByteReader`) parks in a blocking `ReadByte` on the fd after the menu submits; in cooked mode the kernel delivers the whole typed line to ONE reader, and the orphaned pump (queued first) wins and slurps the line into its buffer — so the real prompt hangs and loses the user's first typed line. This was the live `wt create` bug: after "Continue anyway" on the dirty-state menu, the `Worktree name []:` prompt swallowed the typed name. `260708-wryx` extended the single-reader invariant to line prompts. + +- **`MenuSession.PromptWithDefault(prompt, defaultValue string) string` and `MenuSession.ConfirmYesNo(prompt string) bool`** are the line-prompt analogues of `Show` (`src/internal/worktree/menu.go`). In **interactive mode** they read a full line through the session's SHARED `blockingByteReader` (the same reader `Show` uses) instead of a fresh `bufio.Reader` — so a menu's parked pump and the following prompt consume the *same* reader, and the pump's pending byte is delivered to the prompt instead of being stolen. Prompt rendering is unchanged: `PromptWithDefault` prints `"%s [%s]: "` to **stdout**; `ConfirmYesNo` prints `"%s [Y/n] "` to **stderr**. Only the *reader* seam changed. +- **No raw mode for line prompts.** The session methods do NOT call `term.MakeRaw` — line prompts run in cooked mode exactly as the package-level functions do (the kernel line-buffers and echoes). Raw mode remains entered/restored **per `Show()` call only**, per the deliberate `MenuSession` doc-comment contract (§ Raw mode is restored on every exit path). This is why `readLine` reassembles bytes from the same pump the key reads use rather than reading keys in raw mode. +- **`readLine()` on `blockingByteReader`** (`menu.go`) is the line-read mechanism: it accumulates bytes via `readByteBlocking()` (the same single-pump channel path the key reader drains) until a `'\n'`, then strips a trailing `"\r"` via `strings.TrimSuffix(line, "\r")` so `"\r\n"` and `"\n"` both yield the bare line. It returns `(line, true)` on a complete line and `("", false)` on read failure/EOF before any newline — **partial pre-EOF input is discarded**, matching the `err != nil` short-circuit in the package-level prompt functions. Keeping the accumulation on `blockingByteReader` preserves the single-reader invariant (exactly one outstanding `ReadByte` on the fd at any moment) and is unit-testable via the `sharedStream` seam without a PTY. +- **Interactive-mode `TrimSpace` parity with the fallback path.** Both session methods `strings.TrimSpace` the line *after* `readLine`'s CRLF strip, so interactive and fallback modes behave identically on padded input (`" "` → default; `" y"` → yes). The package-level functions already `TrimSpace`; the session methods trim too so the two paths never diverge. (This was a review-corrected refinement of the intake design.) +- **EOF/empty semantics match the package-level functions exactly.** On read failure/EOF before a newline: `PromptWithDefault` → `defaultValue`, `ConfirmYesNo` → `false`. On empty (or, after trimming, whitespace-only) line: `PromptWithDefault` → `defaultValue`, `ConfirmYesNo` → `true` (the default). `ConfirmYesNo`'s answer parse is `strings.HasPrefix(strings.ToLower(line), "y")`. +- **Shared prompt-format constants + `parseYesNoLine`.** The prompt-text formats live in package-level constants `promptWithDefaultFmt = "%s [%s]: "` (stdout) and `confirmYesNoFmt = "%s [Y/n] "` (stderr), and the Y/n grammar lives in `parseYesNoLine(line string) bool` (empty → `true`; else prefix-`y`). Both the package-level functions and the `MenuSession` methods reference these, so the prompt text and answer grammar cannot drift between the standalone and session-aware paths. +- **Fallback mode delegates to the package-level functions (byte-for-byte).** When the session is not interactive (`!s.interactive` — non-TTY / Windows), `MenuSession.PromptWithDefault`/`ConfirmYesNo` delegate to the package-level `PromptWithDefault`/`ConfirmYesNo` so piped-stdin behavior is byte-for-byte identical to today (fresh `bufio.NewReader(os.Stdin)`, same prompt strings, same streams). `s.interactive` is decided once at `NewMenuSession` from TTY detection; a **raw-mode-entry failure does NOT flip the session to fallback** — it degrades only the affected `Show` call. `runFallbackMenu` and the existing pinned byte-for-byte tests are unmodified. +- **Package-level `PromptWithDefault`/`ConfirmYesNo` keep their fresh-`bufio` cooked-mode bodies — standalone-only.** They are NOT reimplemented as one-shot-`MenuSession` wrappers: a cooked-mode `ReadString('\n')` completes synchronously and leaves no parked goroutine, so standalone use is safe, whereas a one-shot session would orphan a pump and reintroduce theft in the prompt→next-reader direction. Their doc comments carry the constraint: **flows that mix menus and line prompts on the same stdin MUST use the `MenuSession` variants.** (After this change their only remaining callers are the session methods' fallback delegation; they are retained deliberately, not deletable.) + +### `wt create` joins the session-threading pattern (`260708-wryx`) + +- `wt create`'s `RunE` (`src/cmd/wt/create.go`) creates ONE `session := wt.NewMenuSession()` with `defer session.Close()` near the top (before the dirty-state check — the first interactive consumer), and routes **all five** interactive stdin consumers through it: the dirty-state menu (`session.Show`), the "Worktree name" prompt (`session.PromptWithDefault`), the "Initialize worktree?" confirm (`session.ConfirmYesNo`), the "Continue and open the worktree anyway?" confirm (`session.ConfirmYesNo`), and the "Open in:" app menu (`session.Show`). Both former one-shot `wt.ShowMenu` calls are gone. +- This fixes **two** seams at once: the reported menu→line-prompt theft (dirty-state menu → name prompt) AND a latent menu→menu theft (the dirty-state menu's orphaned pump also stole from the later "Open in:" menu — the exact class `MenuSession` fixed for `wt open`). `wt create` now matches the `wt open` / `wt delete` / `wt go` session-threading pattern. +- **`cmd/` stays orchestration-only** (Constitution Principle V): the line-reading logic lives entirely in `internal/worktree/menu.go`; `create.go`'s edits are limited to constructing the session, swapping call sites to session methods, and the warning-string edit — no new business logic in `cmd/`. +- **Dirty-state warning copy corrected** to `wt.Warn("current worktree has uncommitted changes")` (was `"main repo has uncommitted changes"`). The `HasUncommittedChanges()` / `HasUntrackedFiles()` checks run in the process CWD — whatever worktree (linked or main) the user is standing in — so the copy must describe the *current worktree/checkout*, not "main repo". No behavior change to the checks; the warning exists because uncommitted work doesn't carry over to the new worktree, and the "Stash changes first" option remains valid (the git stash is repo-global). The stream is unchanged (`wt.Warn` → stderr, per the stdout=machine / stderr=human convention — see `/wt-cli/create-output-phases.md`, the canonical stream-discipline file that owns the `wt.Warn` emitter). + ### Pure `nextMenuState` and `parseKey` are testable without a PTY - `nextMenuState(prev menuState, key keyEvent) menuStateTransition` is a **pure function** with no I/O, no globals, no clock. It encodes every key-mapping, wrap-around, digit boundary, and Cancel transition. @@ -178,12 +201,25 @@ The viewport arithmetic lives in a pure `menuLayout(numOptions, highlight, prevT Overflow indicators (`↑ N more` / `↓ N more`) spend one option-region row each, so `rowsRendered ≤ height − 1` holds and the cursor-up redraw stays sound. On a cramped viewport (heights 4–5) where indicators plus one option would exceed the budget, the chrome is **dropped** (↓ first, then ↑) so the option wins over the hint. — *Why*: the footprint bound is load-bearing (a full-height `\r\n`-terminated region scrolls the terminal on every repaint — the exact bug the change fixes); showing the option over the "N more" hint is fzf's cramped-viewport behavior, and the hint text *is* the indicator row's payload so it cannot be surfaced without spending the row. This was the resolution of two review rework cycles: cycle 1 reserved the `height − 1` row; cycle 2 added the drop-chrome step after the cycle-1 code still overshot at heights 4–5 (an in-layout `v<1→v=1` clamp fabricated a row the budget never allocated). — *Rejected*: reporting the true hidden count without rendering the row (impossible — the count is the row). +### Extend `MenuSession` to line prompts rather than kill the pump or add a singleton (`260708-wryx`) + +The pump is **not** cancelled or killed after each menu — a blocking `ReadByte` on a TTY fd is not interruptible without closing stdin — so the single-shared-reader design is the only correct shape, and the fix was to finish extending it to *all* stdin consumers within one interactive flow (line prompts, not just menus). — *Why*: reuses the mechanism the codebase already built, tested, and documented for the menu→menu byte-theft class; a shared reader guarantees at most one reader on the fd, so no keystroke is stolen at any seam. — *Rejected*: killing the pump after each menu (can't cleanly cancel a blocking TTY read), a global singleton reader (hidden state, against Constitution I). + +### Methods on `MenuSession`, and package-level prompts kept standalone-only (`260708-wryx`) + +The line prompts are **methods** on `MenuSession` (`session.PromptWithDefault`, `session.ConfirmYesNo`) mirroring `session.Show`, not free functions taking a session parameter — the conventional Go shape for shared-resource access. The package-level `PromptWithDefault`/`ConfirmYesNo` are **kept with their fresh-`bufio` cooked-mode bodies** for standalone use rather than rewritten as one-shot-session wrappers. — *Why*: a one-shot session would spin up a pump whose orphan reintroduces theft in the prompt→next-reader direction, so the standalone functions must stay pump-free; a cooked-mode synchronous `ReadString` is safe standalone. Their doc comments now direct mixed menu+prompt flows to the session variants. — *Rejected*: free functions (`PromptWithDefaultSession(s, …)` — noisier, inconsistent with `Show`); one-shot-session wrappers (reintroduce the bug). + +### `readLine` on `blockingByteReader`; shared prompt constants + `parseYesNoLine` (`260708-wryx`) + +The line-read helper lives on `blockingByteReader` (`readLine`), accumulating via `readByteBlocking()`. — *Why*: the pump already delivers one byte at a time through the single-reader channel, so accumulating there keeps the single-reader invariant and is directly unit-testable via the `sharedStream` seam without a PTY (the same seam the arrow-key state machine uses). The prompt-text formats and the Y/n grammar were extracted into package-level constants (`promptWithDefaultFmt`, `confirmYesNoFmt`) and a `parseYesNoLine` helper shared by both the package-level functions and the `MenuSession` methods — *why*: the interactive and standalone paths must be behaviorally identical, and sharing the constants/parser makes prompt text and answer grammar impossible to drift between them (this and the interactive-mode `TrimSpace` were review-corrected refinements of the intake design, which had left the interactive path without an explicit trim). — *Rejected*: duplicating the prompt strings / parse logic across the two paths (drift risk). + ## Cross-references - Spec doc: none — interactive prompt UX is per-prompt-widget behavior, not part of `docs/specs/cli-surface.md`'s per-subcommand flag surface. This contract lives in memory only. -- Source: `src/internal/worktree/menu.go` — `ShowMenu`, `isInteractiveTTY`, `runInteractiveMenu`, `runInteractiveMenuCore` (now takes `heightFn func() int`), `paintMenu`, `redrawMenu`, `renderRows` (now take `height int`), `nextMenuState`, `parseKey`, `initialHighlight`, `menuState` (now carries `top`), `keyEvent`, `menuStateTransition`, named ANSI / key constants. Windowing (`260705-3wr8`): `menuLayout`, `optionRowsForTop`, `dropUnaffordableIndicators`, `terminalHeight`, `writeIndicatorRow`, and the constants `defaultTerminalHeight`, `menuOverheadRows`, `indicatorUp`, `indicatorDown`. -- Tests: `src/internal/worktree/menu_test.go` — `nextMenuState` table-driven coverage (every keybinding, wrap-around, digit boundaries, seeding), `parseKey` table-driven coverage (arrow sequences, bare-Esc-vs-arrow timeout via fake clock, unknown sequences), fallback-path byte-equality tests, `TestRunInteractiveMenuCore_PanicRestore` (defer-restore guarantee), `TestPaintAndRedrawShareCore` (first-paint / redraw byte-equality). Windowing (`260705-3wr8`): `TestMenuLayout`, `TestMenuLayout_FootprintSweep` (exhaustive `rowsRendered ≤ height−1` sweep, heights 1–12 × options 1–25), `TestMenuLayout_WrapJumpFromTopToBottom`, `TestTerminalHeightFallback` (24-row `GetSize`-failure fallback), `TestRenderRows_WindowedSlicing`, `TestRenderRows_NoIndicatorsAtEdges`, `TestRenderRows_SmallMenuByteIdenticalToUnwindowed`, `TestRedrawMenu_ClearsExtraLinesOnShrink`. -- Constitution: Principle I (Single-Binary CLI — motivated rejecting third-party TUI deps), Principle IV (Test What the User Sees — motivated the pure state-machine seam), Principle VI (Interactive by Default, Scriptable on Demand — motivated the byte-identical non-TTY fallback). +- Source: `src/internal/worktree/menu.go` — `ShowMenu`, `isInteractiveTTY`, `runInteractiveMenu`, `runInteractiveMenuCore` (now takes `heightFn func() int`), `paintMenu`, `redrawMenu`, `renderRows` (now take `height int`), `nextMenuState`, `parseKey`, `initialHighlight`, `menuState` (now carries `top`), `keyEvent`, `menuStateTransition`, named ANSI / key constants. Windowing (`260705-3wr8`): `menuLayout`, `optionRowsForTop`, `dropUnaffordableIndicators`, `terminalHeight`, `writeIndicatorRow`, and the constants `defaultTerminalHeight`, `menuOverheadRows`, `indicatorUp`, `indicatorDown`. Line-prompt seam (`260708-wryx`): `MenuSession.PromptWithDefault`, `MenuSession.ConfirmYesNo`, `blockingByteReader.readLine`, the package-level `PromptWithDefault`/`ConfirmYesNo` (kept standalone-only), the shared `parseYesNoLine` helper, and the `promptWithDefaultFmt` / `confirmYesNoFmt` prompt-format constants. +- Tests: `src/internal/worktree/menu_test.go` — `nextMenuState` table-driven coverage (every keybinding, wrap-around, digit boundaries, seeding), `parseKey` table-driven coverage (arrow sequences, bare-Esc-vs-arrow timeout via fake clock, unknown sequences), fallback-path byte-equality tests, `TestRunInteractiveMenuCore_PanicRestore` (defer-restore guarantee), `TestPaintAndRedrawShareCore` (first-paint / redraw byte-equality). Windowing (`260705-3wr8`): `TestMenuLayout`, `TestMenuLayout_FootprintSweep` (exhaustive `rowsRendered ≤ height−1` sweep, heights 1–12 × options 1–25), `TestMenuLayout_WrapJumpFromTopToBottom`, `TestTerminalHeightFallback` (24-row `GetSize`-failure fallback), `TestRenderRows_WindowedSlicing`, `TestRenderRows_NoIndicatorsAtEdges`, `TestRenderRows_SmallMenuByteIdenticalToUnwindowed`, `TestRedrawMenu_ClearsExtraLinesOnShrink`. Line-prompt seam (`260708-wryx`) in `src/internal/worktree/menu_session_test.go`: `TestUnderlyingReadAhead_MenuToLinePromptTheft` (characterizes the two-reader theft), `TestMenuSession_LinePromptAfterMenuNoTheft` (regression guard — `session.Show` then `session.PromptWithDefault` on one `sharedStream` delivers the typed line intact), `TestBlockingByteReader_ReadLine` (multi-byte / empty / CRLF-strip / EOF-before-newline), `TestMenuSession_PromptWithDefault_Semantics` and `TestMenuSession_ConfirmYesNo_Semantics` (default-on-empty, y/n parsing, EOF paths — via the injected-`sharedStream` seam, no PTY). The corrected warning string is asserted in `src/cmd/wt/create_test.go` `TestCreate_DirtyStateWarningCopy` (dirty repo, piped-stdin fallback menu, `3\n` Abort so no worktree leaks). +- Constitution: Principle I (Single-Binary CLI — motivated rejecting third-party TUI deps; `260708-wryx` rejected a global singleton reader on the same no-hidden-state grounds), Principle IV (Test What the User Sees — motivated the pure state-machine seam and the PTY-free `sharedStream`/`readLine` tests), Principle V (Internal Package Boundary — `260708-wryx` kept all line-reading logic in `internal/worktree`, `create.go` stays orchestration-only), Principle VI (Interactive by Default, Scriptable on Demand — motivated the byte-identical non-TTY fallback, preserved by the fallback-delegation of the session methods). - Sibling memory: `wt-cli/init-failure-contract.md` (different `wt` subcommand, same post-change invariant-capture pattern), `wt-cli/list-status-contract.md` (different subcommand, same pattern). - Sibling memory: `wt-cli/go-command-contract.md` — the `wt go` selector / `wt open --go` composition whose worktree-selection menu is a new caller of this contract (via the shared `selectWorktree` → `ShowMenu`/`MenuSession`). -- Call sites (informational): `src/cmd/wt/open.go` (the shared `selectWorktree` → `session.Show`, plus the "Open in:" menu, plus `wt open --go`'s `openGo`), `src/cmd/wt/go.go` (`wt go`'s no-arg selection menu, via `selectWorktree`; added by `260620-3pp5`), `src/cmd/wt/delete.go` (7 calls), `src/cmd/wt/create.go` (2 calls). +- Sibling memory: `wt-cli/create-output-phases.md` — the canonical stdout/stderr stream-discipline file that owns the single `wt.Warn` emitter; the `260708-wryx` dirty-state warning-copy correction (`"current worktree has uncommitted changes"`) flows through that `wt.Warn` → stderr path (this file documents the *copy* change on the menu-scoped `wt create` flow; the stream discipline itself lives there). +- Call sites (informational): `src/cmd/wt/open.go` (the shared `selectWorktree` → `session.Show`, plus the "Open in:" menu, plus `wt open --go`'s `openGo`), `src/cmd/wt/go.go` (`wt go`'s no-arg selection menu, via `selectWorktree`; added by `260620-3pp5`), `src/cmd/wt/delete.go` (7 calls), `src/cmd/wt/create.go` (`260708-wryx`: one `MenuSession` threaded through `RunE` across all five interactive consumers — 2 `session.Show` menus + 3 session line prompts; both former one-shot `wt.ShowMenu` calls removed). diff --git a/fab/changes/260708-wryx-menu-prompt-byte-theft/.history.jsonl b/fab/changes/260708-wryx-menu-prompt-byte-theft/.history.jsonl new file mode 100644 index 0000000..a56e65c --- /dev/null +++ b/fab/changes/260708-wryx-menu-prompt-byte-theft/.history.jsonl @@ -0,0 +1,17 @@ +{"action":"enter","driver":"fab-new","event":"stage-transition","stage":"intake","ts":"2026-07-08T14:50:52Z"} +{"args":"Fix stdin byte-theft at the menu-to-line-prompt seam (orphaned read-ahead pump steals the next typed line in wt create); extend MenuSession to session-aware PromptWithDefault/ConfirmYesNo; correct mislabeled dirty-state warning copy in wt create","cmd":"fab-new","event":"command","ts":"2026-07-08T14:50:52Z"} +{"delta":"+4.5","event":"confidence","score":4.5,"trigger":"calc-score","ts":"2026-07-08T14:53:21Z"} +{"cmd":"_intake","event":"command","ts":"2026-07-08T14:53:31Z"} +{"cmd":"fab-switch","event":"command","ts":"2026-07-08T14:54:47Z"} +{"cmd":"git-branch","event":"command","ts":"2026-07-08T14:55:30Z"} +{"cmd":"fab-fff","event":"command","ts":"2026-07-08T14:56:37Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-07-08T14:57:10Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-08T14:57:47Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-07-08T15:20:25Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-08T15:21:05Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"hydrate","ts":"2026-07-08T15:37:39Z"} +{"event":"review","result":"passed","ts":"2026-07-08T15:37:39Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-08T15:38:06Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-07-08T15:41:54Z"} +{"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-07-08T15:43:29Z"} +{"event":"review","result":"passed","ts":"2026-07-08T15:56:04Z"} diff --git a/fab/changes/260708-wryx-menu-prompt-byte-theft/.status.yaml b/fab/changes/260708-wryx-menu-prompt-byte-theft/.status.yaml new file mode 100644 index 0000000..b5610bf --- /dev/null +++ b/fab/changes/260708-wryx-menu-prompt-byte-theft/.status.yaml @@ -0,0 +1,57 @@ +id: wryx +name: 260708-wryx-menu-prompt-byte-theft +created: 2026-07-08T14:50:52Z +created_by: sahil-noon +change_type: fix +issues: [] +progress: + intake: done + apply: done + review: done + hydrate: done + ship: done + review-pr: done +plan: + generated: true + task_count: 11 + acceptance_count: 16 + acceptance_completed: 16 +confidence: + certain: 5 + confident: 5 + tentative: 0 + unresolved: 0 + score: 4.5 + fuzzy: true + dimensions: + signal: 73.0 + reversibility: 82.0 + competence: 83.0 + disambiguation: 75.0 +stage_metrics: + intake: {started_at: "2026-07-08T14:50:52Z", driver: fab-new, iterations: 1, completed_at: "2026-07-08T14:57:10Z"} + apply: {started_at: "2026-07-08T14:57:10Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-08T15:20:25Z"} + review: {started_at: "2026-07-08T15:20:25Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-08T15:37:39Z"} + hydrate: {started_at: "2026-07-08T15:37:39Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-08T15:41:54Z"} + ship: {started_at: "2026-07-08T15:41:54Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-08T15:43:29Z"} + review-pr: {started_at: "2026-07-08T15:43:29Z", driver: git-pr, iterations: 1, completed_at: "2026-07-08T15:56:04Z"} +prs: + - https://github.com/sahil87/wt/pull/30 +change_type_source: explicit +true_impact: + added: 863 + deleted: 19 + net: 844 + excluding: + added: 449 + deleted: 13 + net: 436 + tests: + added: 311 + deleted: 0 + net: 311 + computed_at: "2026-07-08T15:43:29Z" + computed_at_stage: ship +summary: Extend MenuSession single-reader contract to the menu->line-prompt seam (session-aware PromptWithDefault/ConfirmYesNo via readLine-through-pump); thread one session through wt create; correct dirty-state warning copy to 'current worktree'. +# true_impact: lazily created on first stage-finish that computes it (no placeholder here). +last_updated: 2026-07-08T15:56:04Z diff --git a/fab/changes/260708-wryx-menu-prompt-byte-theft/intake.md b/fab/changes/260708-wryx-menu-prompt-byte-theft/intake.md new file mode 100644 index 0000000..d862047 --- /dev/null +++ b/fab/changes/260708-wryx-menu-prompt-byte-theft/intake.md @@ -0,0 +1,116 @@ +# Intake: Fix stdin byte-theft at the menu→line-prompt seam; correct mislabeled dirty-state warning in `wt create` + +**Change**: 260708-wryx-menu-prompt-byte-theft +**Created**: 2026-07-08 + +## Origin + +Dispatched promptless by `/fab-proceed` from a live debugging conversation (synthesized description treated as source of truth). The user hit an interactive hang in `wt create`: after selecting "Continue anyway" on the dirty-state menu, the `Worktree name [lively-tamarin]:` prompt appeared hung — the first Enter/typed line was swallowed; a second Enter typically got through but any typed name was lost (observed via user screenshot). The conversation diagnosed the root cause (an orphaned stdin read-ahead pump goroutine stealing the next line), agreed on the fix shape (extend the existing `MenuSession` shared-reader mechanism to line prompts), and additionally identified a mislabeled warning: `wt create` warns "main repo has uncommitted changes" when the dirty state actually belongs to whatever worktree the process is standing in. + +> Fix stdin byte-theft at the menu→line-prompt seam (orphaned read-ahead pump steals the user's next typed line in `wt create`); extend the shared-reader `MenuSession` mechanism to session-aware `PromptWithDefault`/`ConfirmYesNo` variants and thread one session through `wt create`'s interactive flow; correct the mislabeled dirty-state warning copy in `wt create`. + +## Why + +**Problem 1 (primary — interactive hang).** Interactive menus run a read-ahead pump goroutine over stdin (`blockingByteReader` in `src/internal/worktree/menu.go:645-728`): the pump loops `src.ReadByte()` → channel send, so after a menu submits, the pump goroutine is left parked in a blocking read on the stdin fd. In `wt create` (`src/cmd/wt/create.go`), the dirty-state menu (`wt.ShowMenu`, create.go:126-153 — a one-shot `MenuSession`) is immediately followed by `wt.PromptWithDefault("Worktree name", …)` (create.go:176), which constructs a fresh `bufio.NewReader(os.Stdin)` (menu.go:228-240). Two readers now race on the same fd. In cooked mode the kernel delivers the whole typed line to ONE reader; the orphaned pump (queued first) wins, and its `bufio.Reader` slurps the line into its 4KB buffer — the line is lost to the real prompt. The user experiences a hung prompt that swallows their input. + +The codebase already knows this failure mode: `MenuSession` (menu.go:62-148) was built to fix byte-theft between *consecutive menus* (`wt open`, `wt delete` flows — regression tests `TestUnderlyingReadAhead_DemonstratesTheft`, `TestMenuSession_SharesReaderAcrossMenus`, `TestMenuSession_ThreeMenusNoTheft` in `src/internal/worktree/menu_session_test.go`; contract documented in `docs/memory/wt-cli/menu-navigation-contract.md`). But the **menu → line-prompt** seam is uncovered: `PromptWithDefault` (menu.go:228-240) and `ConfirmYesNo` (menu.go:213-225) each build their own fresh `bufio.Reader` and are not session-aware. + +If unfixed, every `wt create` invocation from a dirty worktree produces a broken name prompt — the flagship interactive flow of the tool loses user keystrokes nondeterministically. + +**Why this approach**: extending `MenuSession` (rather than, e.g., killing the pump after each menu, or a global singleton reader) reuses the mechanism the codebase already built, tested, and documented for exactly this class of bug. The pump cannot be cleanly cancelled (a blocking `ReadByte` on a TTY fd is not interruptible without closing stdin), so the single-shared-reader design is the only correct shape — the fix is to finish extending it to all stdin consumers within one interactive flow. + +**Problem 2 (secondary — mislabeled warning).** create.go:127 warns `"main repo has uncommitted changes"`, but the checks `HasUncommittedChanges()`/`HasUntrackedFiles()` (`src/internal/worktree/git.go:11-28`) run `git diff --quiet` / `git diff --cached --quiet` / `git ls-files --others --exclude-standard` in the process CWD — which is whatever worktree the user is standing in, not necessarily the main repo. The user hit this warning from a dirty *linked worktree* while the main worktree was clean. The copy misleads users into inspecting the wrong checkout. + +## What Changes + +### 1. Session-aware line prompts in `src/internal/worktree/menu.go` + +Add two methods on `MenuSession`, mirroring the existing `Show` method pattern: + +```go +// PromptWithDefault prompts for a line of input with a default value, +// reading through the session's shared stdin reader. +func (s *MenuSession) PromptWithDefault(prompt, defaultValue string) string + +// ConfirmYesNo prompts for a Y/n confirmation (default yes), reading +// through the session's shared stdin reader. +func (s *MenuSession) ConfirmYesNo(prompt string) bool +``` + +**Interactive mode** (`s.interactive == true`): read a full line through the shared `blockingByteReader` — the pump delivers one byte at a time, so a line-read helper (e.g. `readLine()` on `blockingByteReader` or a session-internal helper) accumulates bytes via `readByteBlocking()` until `'\n'`, then strips the trailing `"\r\n"`/`"\n"`. No raw mode is entered — line prompts run in cooked mode exactly as today (the kernel line-buffers and echoes; raw mode remains entered/restored per `Show()` call only, per the deliberate `MenuSession` doc-comment contract). Because the parked pump and the prompt now consume the *same* reader, the pump's pending byte is delivered to the prompt instead of being stolen. + +**EOF/error semantics match current behavior exactly**: on read failure/EOF before a newline, `PromptWithDefault` returns `defaultValue` and `ConfirmYesNo` returns `false` (partial input before EOF is discarded, matching the current `err != nil` short-circuit in both functions). Empty line → default (`defaultValue` / `true`). `ConfirmYesNo` answer parsing unchanged: `strings.HasPrefix(strings.ToLower(line), "y")`. + +**Fallback mode** (`s.interactive == false`, i.e. non-TTY / Windows / raw-mode-entry failure): delegate to the existing package-level implementations so behavior under piped stdin is byte-for-byte identical to today (fresh `bufio.NewReader(os.Stdin)`, same prompt strings, same streams). + +**Prompt rendering and output streams are unchanged**: `PromptWithDefault` keeps printing `"%s [%s]: "` to stdout; `ConfirmYesNo` keeps printing `"%s [Y/n] "` to stderr. Only the *reader* seam changes. + +The package-level `ConfirmYesNo`/`PromptWithDefault` functions remain with their current fresh-`bufio.Reader` cooked-mode implementations for standalone use (a plain cooked-mode `ReadString('\n')` completes synchronously and leaves no parked goroutine, so standalone use is safe). Their doc comments gain the constraint: flows that mix menus and line prompts on the same stdin MUST use the session-aware variants. They must NOT be reimplemented as one-shot-session wrappers — that would create a pump whose orphan reintroduces the theft in the other direction (prompt → next reader). + +### 2. Thread one `MenuSession` through `wt create`'s interactive flow (`src/cmd/wt/create.go`) + +Create a single session near the top of `RunE` (`session := wt.NewMenuSession(); defer session.Close()`) and route every interactive stdin consumer in the flow through it: + +| create.go line (today) | Today | After | +|---|---|---| +| :128 dirty-state menu `"How to proceed?"` | one-shot `wt.ShowMenu` | `session.Show` | +| :176 `"Worktree name"` prompt | package-level `wt.PromptWithDefault` | `session.PromptWithDefault` | +| :257 `"Initialize worktree?"` confirm | package-level `wt.ConfirmYesNo` | `session.ConfirmYesNo` | +| :389 `"Continue and open the worktree anyway?"` confirm | package-level `wt.ConfirmYesNo` | `session.ConfirmYesNo` | +| :423 `"Open in:"` app menu | one-shot `wt.ShowMenu` | `session.Show` | + +The one-shot `ShowMenu` at :128 currently orphans a pump that steals from *every* later stdin consumer in the flow — including the second one-shot menu at :423 (menu→menu theft within create, the exact bug class `MenuSession` fixed for `wt open`). Threading one session fixes all seams at once and makes `wt create` consistent with the `wt open`/`wt delete`/`wt go` session pattern. + +**Call-site audit result (performed at intake)**: `grep` over `src/cmd/wt/` and `src/internal/worktree/` shows the ONLY production call sites of `PromptWithDefault`/`ConfirmYesNo` are create.go:176/:257/:389, and the only one-shot `ShowMenu` production call sites are create.go:128/:423. `wt delete` (delete.go:97), `wt open` (open.go:202/:292/:417), and `wt go` (go.go:87) already thread a shared `MenuSession` and contain no line prompts — no changes needed there. + +### 3. Correct the dirty-state warning copy (create.go:127) + +```go +// before +wt.Warn("main repo has uncommitted changes") +// after +wt.Warn("current worktree has uncommitted changes") +``` + +The checks run in the process CWD (any worktree, linked or main), so the copy must describe the *current worktree/checkout*. `wt.Warn` (`src/internal/worktree/errors.go:119`) already targets stderr, satisfying the project stdout/stderr convention (stdout = machine result, stderr = human copy). No behavior change to the checks themselves — `HasUncommittedChanges()`/`HasUntrackedFiles()` are correct as CWD checks; the *warning exists* because uncommitted work doesn't carry over to the new worktree, and the "Stash changes first" option remains valid since the git stash is repo-global. + +### 4. Tests + +- **Primitive characterization** (extend `src/internal/worktree/menu_session_test.go` pattern): a test demonstrating menu→line-prompt theft with two independent readers (analogous to `TestUnderlyingReadAhead_DemonstratesTheft`), and a regression test asserting a `session.Show` followed by a session-aware line prompt on the same `sharedStream` delivers the typed line to the prompt intact (analogous to `TestMenuSession_SharesReaderAcrossMenus`). The line-read helper gets direct unit coverage: multi-byte line, empty line, `"\r\n"` stripping, EOF-before-newline → error path. +- **Prompt semantics**: session-aware `PromptWithDefault`/`ConfirmYesNo` unit tests for default-on-empty, y/n parsing, EOF → default/false — testable via the existing seam discipline (injected reader/writer; no PTY needed, per Constitution Principle IV and the `runInteractiveMenuCore` seam pattern documented in `menu-navigation-contract.md`). +- **Non-TTY contracts preserved**: existing pinned byte-for-byte tests for `runFallbackMenu` and prompts under piped stdin must keep passing unmodified; `cmd/` integration tests (piped stdin → fallback path) keep passing. +- **Copy fix**: update/add the `wt create` test asserting the new warning string on stderr. +- All tests conform to the code-review.md host-isolation rule (no side-effect leakage; use non-side-effecting `--worktree-open`/`--app` targets or `runWt` env isolation). + +## Affected Memory + +- `wt-cli/menu-navigation-contract`: (modify) Extend the single-reader `MenuSession` contract to cover the menu→line-prompt seam — session-aware `PromptWithDefault`/`ConfirmYesNo`, the line-read-through-pump mechanism, the cooked-mode/no-raw-mode rule for line prompts, and `wt create` joining the session-threading pattern. + +## Impact + +- `src/internal/worktree/menu.go` — two new `MenuSession` methods + a line-read helper on `blockingByteReader`; doc-comment updates on the package-level prompts and `MenuSession`. +- `src/cmd/wt/create.go` — one session threaded through `RunE`; five call-site edits; one warning-string edit. `cmd/` stays orchestration-only (Constitution Principle V: the new logic lives in `internal/worktree`). +- `src/internal/worktree/menu_session_test.go`, `menu_test.go`, `src/cmd/wt/create_test.go` — new regression/unit coverage; existing pinned fallback-output tests unchanged. +- No public CLI surface change (no flags, no exit codes, no output-contract change other than the corrected warning string on stderr). No new dependencies. +- Non-goals: `wt create` base-ref semantics unchanged (new worktree still based on current HEAD when `--base` absent — crud.go:35-40; that behavior is the *reason* the dirty warning exists, not part of this fix); raw-mode-per-`Show()` lifecycle unchanged; cooked-mode type-ahead buffering between consecutive line prompts (no parked reader involved) out of scope; `wt delete`/`wt open`/`wt go` untouched. + +## Open Questions + +None — the fix shape, constraints, and scope were resolved in the originating conversation; remaining implementation choices are graded below. + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Certain | Fix mechanism: extend `MenuSession` with session-aware line prompts sharing the single stdin reader; the pump is not cancelled/killed | Agreed fix shape in the originating conversation; reuses the codebase's existing, tested, documented mechanism for exactly this bug class | S:90 R:70 A:90 D:85 | +| 2 | Certain | `wt create` threads ONE session across the whole interactive flow (both menus + all three line prompts), replacing both one-shot `ShowMenu` calls | Discussed; intake audit shows the :128 orphan pump also steals from the :423 menu; matches the established `wt open`/`wt delete`/`wt go` pattern | S:80 R:80 A:85 D:80 | +| 3 | Confident | API shape: methods on `MenuSession` (`session.PromptWithDefault`, `session.ConfirmYesNo`) rather than free functions taking a session parameter | Mirrors the existing `session.Show` method; conventional Go shape for shared-resource access | S:60 R:85 A:85 D:75 | +| 4 | Confident | Package-level `ConfirmYesNo`/`PromptWithDefault` remain with current fresh-`bufio` cooked-mode implementations (NOT one-shot-session wrappers) for standalone use; doc comments direct mixed flows to session variants | A cooked-mode `ReadString` leaves no parked goroutine so standalone use is safe; a one-shot-session wrapper would orphan a pump and reintroduce theft in the prompt→next-reader direction | S:55 R:80 A:75 D:65 | +| 5 | Certain | EOF/error semantics of session-aware prompts match current behavior: `PromptWithDefault` → `defaultValue`, `ConfirmYesNo` → `false`; partial pre-EOF input discarded; empty line → default | Explicit constraint in the synthesized description; preserves every caller's observable behavior | S:85 R:85 A:85 D:85 | +| 6 | Certain | Non-TTY fallback paths preserved byte-for-byte: fallback-mode session prompts delegate to the existing package-level line-read code; `runFallbackMenu` untouched | Explicit constraint; Constitution Principle VI; pinned by existing byte-for-byte tests | S:85 R:80 A:90 D:90 | +| 7 | Confident | Warning copy becomes exactly `"current worktree has uncommitted changes"` (via `wt.Warn`, stderr) | Requirement was "accurately describe what is dirty (the current worktree/checkout)"; exact wording is agent-choosable, trivially reversible; "current worktree" is accurate from both main and linked worktrees | S:65 R:90 A:80 D:60 | +| 8 | Confident | Prompt rendering/streams unchanged: `PromptWithDefault` prompt text stays on stdout, `ConfirmYesNo` on stderr — only the reader seam changes; the stdout/stderr-convention cleanup of `PromptWithDefault`'s prompt stream is NOT taken in this change | Byte-compat mandate ("preserve pinned output contracts") outweighs the convention cleanup; scope discipline for a fix-type change | S:55 R:85 A:70 D:55 | +| 9 | Confident | Test approach: pure-seam unit tests without a PTY (extend `menu_session_test.go` `sharedStream` pattern + injected reader/writer for prompt semantics); host-isolation per code-review.md | Constitution Principle IV + the documented `runInteractiveMenuCore` seam pattern make this the established discipline; exact test names/fixtures are implementation detail | S:70 R:85 A:85 D:70 | +| 10 | Certain | Scope exclusions: base-ref behavior, raw-mode-per-`Show` lifecycle, cooked-mode type-ahead between consecutive line prompts, and `wt delete`/`wt open`/`wt go` are all untouched | Explicitly scoped out in the synthesized description ("Context, not in scope"; "the fix must not change that"); audit confirms the other flows have no menu→line-prompt seam | S:85 R:80 A:85 D:85 | + +10 assumptions (5 certain, 5 confident, 0 tentative, 0 unresolved). diff --git a/fab/changes/260708-wryx-menu-prompt-byte-theft/plan.md b/fab/changes/260708-wryx-menu-prompt-byte-theft/plan.md new file mode 100644 index 0000000..7ebb908 --- /dev/null +++ b/fab/changes/260708-wryx-menu-prompt-byte-theft/plan.md @@ -0,0 +1,186 @@ +# Plan: Fix stdin byte-theft at the menu→line-prompt seam; correct mislabeled dirty-state warning in `wt create` + +**Change**: 260708-wryx-menu-prompt-byte-theft +**Intake**: `intake.md` + +## Requirements + +### Menu Session: Session-aware line prompts + +#### R1: `MenuSession.PromptWithDefault` reads through the shared reader +`MenuSession` SHALL expose a method `PromptWithDefault(prompt, defaultValue string) string` that, in interactive mode, reads a full line of input through the session's shared `blockingByteReader` (the same reader used by `Show`) rather than constructing a fresh `bufio.Reader` over `os.Stdin`. This closes the menu→line-prompt byte-theft seam: because the parked pump and the prompt consume the same reader, the pump's pending byte is delivered to the prompt instead of being stolen. + +- **GIVEN** an interactive `MenuSession` whose shared reader's pump has one byte parked after a preceding `Show` +- **WHEN** `session.PromptWithDefault("Worktree name", "lively-tamarin")` is called and the user types a name followed by Enter +- **THEN** the typed line is returned intact (not swallowed), and no second reader is created on the stdin fd +- **AND** the prompt text `"%s [%s]: "` is printed to stdout unchanged + +#### R2: `MenuSession.ConfirmYesNo` reads through the shared reader +`MenuSession` SHALL expose a method `ConfirmYesNo(prompt string) bool` that, in interactive mode, reads a full line through the session's shared reader and parses the answer as `strings.HasPrefix(strings.ToLower(line), "y")`. The default (empty line) SHALL be `true`. + +- **GIVEN** an interactive `MenuSession` with a shared reader +- **WHEN** `session.ConfirmYesNo("Initialize worktree?")` is called and the user presses Enter (empty line) +- **THEN** it returns `true` (default yes) +- **AND** when the user types a line beginning with `y`/`Y` it returns `true`, otherwise `false` +- **AND** the prompt text `"%s [Y/n] "` is printed to stderr unchanged + +#### R3: Line-read helper accumulates bytes through the pump until newline +A line-read helper (a method on `blockingByteReader`, e.g. `readLine()`) SHALL accumulate bytes via `readByteBlocking()` until a `'\n'` is seen, then strip a trailing `"\r\n"` or `"\n"`, returning the line plus an ok/error signal. On a read failure/EOF before any newline, it SHALL signal failure and discard partial input. No raw mode is entered for line reads — line prompts run in cooked mode exactly as today. + +- **GIVEN** a `blockingByteReader` over a stream delivering `h`, `i`, `\n` +- **WHEN** `readLine()` is called +- **THEN** it returns `"hi"` with an ok signal +- **AND GIVEN** the stream delivers `x`, `y` then EOF (no newline), **THEN** `readLine()` signals failure and the partial `"xy"` is discarded +- **AND GIVEN** the stream delivers `a`, `\r`, `\n`, **THEN** `readLine()` returns `"a"` (CRLF stripped) +- **AND GIVEN** the stream delivers only `\n`, **THEN** `readLine()` returns `""` with an ok signal + +#### R4: EOF/error and empty-line semantics match the package-level functions exactly +The session-aware prompts SHALL preserve every caller-observable behavior of the current package-level functions. On read failure/EOF before a newline: `PromptWithDefault` returns `defaultValue`; `ConfirmYesNo` returns `false`. On empty line: `PromptWithDefault` returns `defaultValue`; `ConfirmYesNo` returns `true`. + +- **GIVEN** an interactive session whose reader hits EOF before any newline +- **WHEN** `session.PromptWithDefault("Name", "def")` is called +- **THEN** it returns `"def"` +- **AND WHEN** `session.ConfirmYesNo("OK?")` is called under the same EOF, **THEN** it returns `false` + +#### R5: Fallback mode delegates to the package-level implementations (byte-for-byte) +When the session is not interactive (`s.interactive == false` — non-TTY or Windows; a raw-mode-entry failure does NOT flip the session to fallback, it degrades only the affected `Show` call), the session-aware methods SHALL delegate to the existing package-level `PromptWithDefault`/`ConfirmYesNo` so behavior under piped stdin is byte-for-byte identical to today (fresh `bufio.NewReader(os.Stdin)`, same prompt strings, same streams). + +- **GIVEN** a `MenuSession` in fallback mode (non-TTY) +- **WHEN** `session.PromptWithDefault`/`session.ConfirmYesNo` are called +- **THEN** they produce output and read behavior byte-for-byte identical to the package-level functions +- **AND** `runFallbackMenu` and existing pinned fallback tests remain unmodified and passing + +#### R6: Package-level `PromptWithDefault`/`ConfirmYesNo` retain their current implementations +The package-level `ConfirmYesNo`/`PromptWithDefault` functions SHALL keep their current fresh-`bufio.Reader` cooked-mode implementations for standalone use. They MUST NOT be reimplemented as one-shot-`MenuSession` wrappers (a one-shot session would orphan a pump and reintroduce theft in the prompt→next-reader direction). Their doc comments SHALL gain the constraint that flows mixing menus and line prompts on the same stdin MUST use the session-aware variants. + +- **GIVEN** the package-level `PromptWithDefault`/`ConfirmYesNo` +- **WHEN** this change is applied +- **THEN** their bodies still construct a fresh `bufio.NewReader(os.Stdin)` and read a line synchronously (no pump, no parked goroutine) +- **AND** their doc comments direct mixed menu+prompt flows to the session-aware methods + +### Create Flow: Single session threaded through `wt create` + +#### R7: One `MenuSession` is created and threaded through every interactive stdin consumer in `RunE` +`wt create`'s `RunE` SHALL create a single `MenuSession` near the top (`session := wt.NewMenuSession(); defer session.Close()`) and route every interactive stdin consumer through it: the dirty-state menu (`session.Show`), the worktree-name prompt (`session.PromptWithDefault`), the "Initialize worktree?" confirm (`session.ConfirmYesNo`), the "Continue and open the worktree anyway?" confirm (`session.ConfirmYesNo`), and the "Open in:" app menu (`session.Show`). Both former one-shot `wt.ShowMenu` calls SHALL be replaced with `session.Show`. This fixes all seams at once (including the menu→menu :128→:423 theft) and makes `wt create` consistent with the `wt open`/`wt delete`/`wt go` session pattern. + +- **GIVEN** a user runs `wt create` from a dirty worktree and selects "Continue anyway" on the dirty-state menu +- **WHEN** the `"Worktree name []:"` prompt appears and the user types a name + Enter +- **THEN** the typed name is captured by the prompt (not stolen by the dirty-state menu's orphaned pump) +- **AND** every subsequent interactive consumer in the flow reads through the same session's shared reader + +#### R8: `cmd/` stays orchestration-only +The new line-reading logic SHALL live in `src/internal/worktree/menu.go`; `create.go` changes SHALL be limited to constructing the session, swapping call sites to session methods, and the warning-string edit — no new business logic in `cmd/` (Constitution Principle V). + +- **GIVEN** the create.go edits +- **WHEN** reviewed against Constitution Principle V +- **THEN** all non-trivial line-reading logic resides in `internal/worktree`, and `cmd/` only parses flags, prompts, and orchestrates + +### Create Flow: Dirty-state warning copy + +#### R9: The dirty-state warning describes the current worktree, not "main repo" +The `wt create` dirty-state warning SHALL read exactly `"current worktree has uncommitted changes"` (via `wt.Warn`, stderr). The `HasUncommittedChanges()`/`HasUntrackedFiles()` checks run in the process CWD (any worktree, linked or main), so the copy must describe the current worktree/checkout. No behavior change to the checks themselves. + +- **GIVEN** `wt create` is run interactively from a checkout with uncommitted or untracked changes +- **WHEN** the dirty-state warning is emitted +- **THEN** the warning text on stderr is `"current worktree has uncommitted changes"` (not `"main repo has uncommitted changes"`) +- **AND** the warning is written to stderr (satisfying the stdout=machine / stderr=human convention) + +### Non-Goals + +- `wt create` base-ref semantics — new worktree still based on current HEAD when `--base` absent (crud.go); that behavior is the *reason* the dirty warning exists, not part of this fix. +- Raw-mode-per-`Show()` lifecycle — unchanged; line prompts do NOT enter raw mode. +- Cooked-mode type-ahead buffering between consecutive line prompts (no parked reader involved) — out of scope. +- `wt delete` / `wt open` / `wt go` — untouched; the intake audit confirms they already thread a shared `MenuSession` and contain no line prompts. +- The stdout/stderr-convention cleanup of `PromptWithDefault`'s prompt stream (it prints to stdout today) is NOT taken — byte-compat mandate outweighs it for a fix-type change. +- No public CLI surface change (no flags, exit codes, or output-contract change other than the corrected warning string). + +### Design Decisions + +1. **Extend `MenuSession` with session-aware line prompts sharing the single stdin reader**: the pump is not cancelled/killed. — *Why*: reuses the codebase's existing, tested, documented mechanism for exactly this byte-theft bug class; a blocking `ReadByte` on a TTY fd is not interruptible without closing stdin, so a single-shared-reader design is the only correct shape. — *Rejected*: killing the pump after each menu (impossible to cleanly cancel), a global singleton reader (hidden state). +2. **Methods on `MenuSession` (`session.PromptWithDefault`, `session.ConfirmYesNo`)** rather than free functions taking a session parameter. — *Why*: mirrors the existing `session.Show` method; conventional Go shape for shared-resource access. — *Rejected*: free functions (`PromptWithDefaultSession(s, …)`) — noisier, inconsistent with `Show`. +3. **Package-level prompts keep fresh-`bufio` cooked-mode implementations, not one-shot-session wrappers.** — *Why*: a cooked-mode `ReadString('\n')` completes synchronously and leaves no parked goroutine, so standalone use is safe; a one-shot-session wrapper would orphan a pump and reintroduce theft in the prompt→next-reader direction. — *Rejected*: wrapping them in one-shot sessions. +4. **Line-read helper lives on `blockingByteReader` (`readLine`)** accumulating via `readByteBlocking()`. — *Why*: the pump already delivers one byte at a time through the single-reader channel; accumulating there keeps the single-reader invariant and is directly unit-testable via the `sharedStream` seam without a PTY. + +## Tasks + +### Phase 1: Core Implementation (internal/worktree/menu.go) + +- [x] T001 Add a `readLine()` line-read helper on `blockingByteReader` in `src/internal/worktree/menu.go` that accumulates bytes via `readByteBlocking()` until `'\n'`, strips a trailing `"\r\n"`/`"\n"`, and returns `(line string, ok bool)` — `ok == false` on read failure/EOF before a newline (partial input discarded). +- [x] T002 Add method `func (s *MenuSession) PromptWithDefault(prompt, defaultValue string) string` in `src/internal/worktree/menu.go`: fallback mode (`!s.interactive`) delegates to the package-level `PromptWithDefault`; interactive mode prints `"%s [%s]: "` to stdout, reads via `s.reader.readLine()`, returns `defaultValue` on `!ok` or empty line, else the trimmed line. +- [x] T003 Add method `func (s *MenuSession) ConfirmYesNo(prompt string) bool` in `src/internal/worktree/menu.go`: fallback mode delegates to the package-level `ConfirmYesNo`; interactive mode prints `"%s [Y/n] "` to stderr, reads via `s.reader.readLine()`, returns `false` on `!ok`, `true` on empty line, else `strings.HasPrefix(strings.ToLower(line), "y")`. +- [x] T004 Update the doc comments on the package-level `ConfirmYesNo`/`PromptWithDefault` in `src/internal/worktree/menu.go` to state they keep fresh-`bufio` cooked-mode reads for standalone use and that flows mixing menus and line prompts on the same stdin MUST use the `MenuSession` variants; do NOT change their bodies. + +### Phase 2: Integration (cmd/wt/create.go) + +- [x] T005 In `src/cmd/wt/create.go` `RunE`, create one `session := wt.NewMenuSession()` with `defer session.Close()` near the top of the function body (before the dirty-state check), and thread it through the flow. +- [x] T006 In `src/cmd/wt/create.go`, replace the dirty-state one-shot `wt.ShowMenu("How to proceed?", …)` (create.go:128) with `session.Show(...)`, the worktree-name `wt.PromptWithDefault(...)` (create.go:176) with `session.PromptWithDefault(...)`, the `"Initialize worktree?"` `wt.ConfirmYesNo(...)` (create.go:257) with `session.ConfirmYesNo(...)`, the `"Continue and open the worktree anyway?"` `wt.ConfirmYesNo(...)` (create.go:389) with `session.ConfirmYesNo(...)`, and the `"Open in:"` one-shot `wt.ShowMenu(...)` (create.go:423) with `session.Show(...)`. +- [x] T007 In `src/cmd/wt/create.go`, change the dirty-state warning `wt.Warn("main repo has uncommitted changes")` (create.go:127) to `wt.Warn("current worktree has uncommitted changes")`. + +### Phase 3: Tests + +- [x] T008 [P] In `src/internal/worktree/menu_session_test.go`: add a characterization test demonstrating menu→line-prompt theft with two independent readers (analogous to `TestUnderlyingReadAhead_DemonstratesTheft`) and a regression test asserting a `session.Show` followed by a session-aware line prompt on the same `sharedStream` delivers the typed line intact (analogous to `TestMenuSession_SharesReaderAcrossMenus`). +- [x] T009 [P] In `src/internal/worktree/menu_session_test.go` (or `menu_test.go`): add direct unit coverage for `readLine()` — multi-byte line, empty line (`\n`), `"\r\n"` stripping, and EOF-before-newline → `ok=false`. +- [x] T010 [P] In `src/internal/worktree/menu_session_test.go`: add session-aware `PromptWithDefault`/`ConfirmYesNo` unit tests via the injected-reader seam (no PTY): default-on-empty, y/n parsing, EOF → `defaultValue`/`false`. +- [x] T011 In `src/cmd/wt/create_test.go`: add an integration test asserting the corrected dirty-state warning string `"current worktree has uncommitted changes"` appears on stderr (dirty repo, interactive/piped stdin choosing Abort so no worktree is created — host-isolation preserved). + +## Execution Order + +- Phase 1 (T001–T004) before Phase 2 (T005–T007): create.go calls the new session methods. +- T001 blocks T002 and T003 (they call `readLine()`). +- Phase 3 tests depend on the implementation in Phases 1–2. + +## Acceptance + +### Functional Completeness + +- [x] A-001 R1: `MenuSession.PromptWithDefault` exists and, in interactive mode, reads through the session's shared reader (no fresh `bufio.Reader` on `os.Stdin`); prompt `"%s [%s]: "` still printed to stdout. +- [x] A-002 R2: `MenuSession.ConfirmYesNo` exists and, in interactive mode, reads through the shared reader; prompt `"%s [Y/n] "` still printed to stderr; answer parsed via `HasPrefix(ToLower(line),"y")`. +- [x] A-003 R3: a `readLine()` helper on `blockingByteReader` accumulates via `readByteBlocking()` to `'\n'`, strips trailing `"\r\n"`/`"\n"`, and reports failure on pre-newline EOF. +- [x] A-004 R7: `wt create` `RunE` constructs one `MenuSession` and routes all five interactive stdin consumers (2 menus + 3 line prompts) through it; both one-shot `wt.ShowMenu` calls are gone. +- [x] A-005 R9: the dirty-state warning reads exactly `"current worktree has uncommitted changes"` via `wt.Warn` (stderr). + +### Behavioral Correctness + +- [x] A-006 R4: session-aware prompt EOF/empty semantics match today — `PromptWithDefault`→`defaultValue` on EOF/empty; `ConfirmYesNo`→`false` on EOF, `true` on empty. +- [x] A-007 R7: the menu→line-prompt seam no longer steals the user's typed name — a `session.Show` followed by a session-aware line prompt on one shared stream delivers the line intact (regression test passes). +- [x] A-008 R6: package-level `PromptWithDefault`/`ConfirmYesNo` bodies are unchanged (fresh `bufio.Reader`, synchronous read); only doc comments gained the session-variant guidance. + +### Scenario Coverage + +- [x] A-009 R3: `readLine()` unit tests cover multi-byte line, empty line, CRLF stripping, and EOF-before-newline. +- [x] A-010 R1 R2: session-aware `PromptWithDefault`/`ConfirmYesNo` unit tests cover default-on-empty, y/n parsing, and EOF paths without a PTY. +- [x] A-011 R9: an integration test asserts the corrected warning string on stderr from a dirty repo without leaking a worktree (aborts). + +### Edge Cases & Error Handling + +- [x] A-012 R5: fallback-mode session methods delegate to the package-level functions; `runFallbackMenu` and all existing pinned byte-for-byte fallback/prompt tests pass unmodified. +- [x] A-013 R3: `readLine()` discards partial pre-EOF input (returns not-ok), matching the `err != nil` short-circuit in the current prompt functions. + +### Code Quality + +- [x] A-014 Pattern consistency: new `MenuSession` methods mirror the existing `Show` method shape and the `showInteractive`/`readByteBlocking` conventions in `menu.go`; create.go session threading matches the `wt open`/`wt delete`/`wt go` pattern. +- [x] A-015 No unnecessary duplication: session-aware prompts reuse `readByteBlocking()` (via `readLine()`) and delegate to the package-level functions in fallback mode rather than duplicating the fresh-`bufio` line-read; `cmd/` adds no line-reading logic (Constitution Principle V). +- [x] A-016 Host isolation: new tests satisfy `code-review.md` § Project-Specific Review Rules — pure-seam tests (no PTY) for the primitive; the create integration test aborts before creating a worktree and relies on `runWt` env isolation, leaking no side effects. + +## Notes + +- Check items as you review: `- [x]` +- All acceptance items must pass before `/fab-continue` (hydrate) + +## Deletion Candidates + +- `ShowMenu` (src/internal/worktree/menu.go:56-60) — after create.go switched both call sites to `session.Show`, the exported one-shot wrapper has zero production call sites (only `menu_test.go` fallback tests drive it); kept deliberately as the documented single-menu convenience API pinned by `docs/memory/wt-cli/menu-navigation-contract.md`, so not recommended for removal — flagged because the change made it production-unused. +- Package-level `PromptWithDefault`/`ConfirmYesNo` (src/internal/worktree/menu.go:272, :294) — their only remaining callers are the session methods' fallback delegation (menu.go:168, :189); retention is mandated by plan R6 (standalone cooked-mode use is safe and one-shot-session wrappers would reintroduce theft), so these are not deletable — noted because this change removed their last direct `cmd/` call sites. + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Certain | Line-read helper is `readLine() (string, bool)` on `blockingByteReader`, accumulating via `readByteBlocking()` until `'\n'` then stripping `"\r\n"`/`"\n"` | Intake §1 names this exact mechanism ("a line-read helper (e.g. `readLine()` on `blockingByteReader`)"); keeps the single-reader invariant | S:85 R:80 A:85 D:80 | +| 2 | Confident | `readLine` returns `(line, ok bool)` where `ok==false` on pre-newline read failure/EOF (partial discarded) | Intake §1/§4 pins EOF-before-newline as an error path; a bool ok mirrors `readByteBlocking`'s `(byte, bool)` signature already in the file | S:70 R:80 A:80 D:70 | +| 3 | Confident | Session-aware prompts `strings.TrimSpace` the line after `readLine`'s CRLF strip, so interactive and fallback modes behave identically on padded input (`" "` → default, `" y"` → yes) | Review-corrected (should-fix): both modes must be behaviorally identical; the package-level funcs trim, so the interactive path trims too | S:70 R:85 A:85 D:75 | +| 4 | Certain | Session created before the dirty-state check and `defer session.Close()` immediately after, matching `wt open`/`wt delete` placement | Intake §2 says "near the top of `RunE`"; the dirty-state menu is the first interactive consumer so the session must precede it | S:85 R:85 A:85 D:85 | +| 5 | Confident | Warning-copy integration test drives a dirty repo with piped stdin (non-TTY → fallback menu) and feeds `3\n` (Abort) so no worktree is created | Only viable host-isolated route to reach the interactive dirty-state block (all existing create tests use `--non-interactive`, which skips it); Abort returns before any filesystem mutation | S:65 R:85 A:80 D:65 | +| 6 | Confident | Package-level `PromptWithDefault` prompt stays on stdout (not migrated to stderr) | Intake assumption #8: byte-compat mandate outweighs the stdout/stderr-convention cleanup for a fix-type change; scope discipline | S:60 R:85 A:80 D:70 | + +6 assumptions (2 certain, 4 confident, 0 tentative). diff --git a/src/cmd/wt/create.go b/src/cmd/wt/create.go index 043a0da..db15b03 100644 --- a/src/cmd/wt/create.go +++ b/src/cmd/wt/create.go @@ -122,10 +122,19 @@ or creates a new branch.`, } } + // One MenuSession threaded through every interactive stdin consumer + // in this flow (both menus + all three line prompts). Sharing one + // reader is what prevents a menu's parked read-ahead pump from + // stealing the next consumer's first line/keystroke — see the + // MenuSession doc comment in internal/worktree/menu.go. Matches the + // session-threading pattern already used by wt open/delete/go. + session := wt.NewMenuSession() + defer session.Close() + // Dirty-state check if !nonInteractive && (wt.HasUncommittedChanges() || wt.HasUntrackedFiles()) { - wt.Warn("main repo has uncommitted changes") - choice, err := wt.ShowMenu("How to proceed?", []string{ + wt.Warn("current worktree has uncommitted changes") + choice, err := session.Show("How to proceed?", []string{ "Continue anyway", "Stash changes first", "Abort", @@ -173,7 +182,7 @@ or creates a new branch.`, } else if nonInteractive { finalName = suggestedName } else { - finalName = wt.PromptWithDefault("Worktree name", suggestedName) + finalName = session.PromptWithDefault("Worktree name", suggestedName) } // Check collision @@ -254,7 +263,7 @@ or creates a new branch.`, // inside the runner). runInit := worktreeInit == "true" if runInit && !(nonInteractive || branchArg == "") { - runInit = wt.ConfirmYesNo("Initialize worktree?") + runInit = session.ConfirmYesNo("Initialize worktree?") } // Emit deferred summary (still under rollback handler). The Git @@ -386,7 +395,7 @@ or creates a new branch.`, // signal, already computed above). Non-interactive / piped / // CI keeps today's exact behavior: banner + exit 7, NO prompt. if !nonInteractive && reclaimTTY { - if !wt.ConfirmYesNo("Continue and open the worktree anyway?") { + if !session.ConfirmYesNo("Continue and open the worktree anyway?") { // No: do not open. The banner's Go: line already shows // how to reach the worktree, so no app menu is shown. worktreeOpen = "skip" @@ -420,7 +429,7 @@ or creates a new branch.`, for i, a := range apps { appNames[i] = a.Name } - choice, err := wt.ShowMenu("Open in:", appNames, defaultIdx) + choice, err := session.Show("Open in:", appNames, defaultIdx) if err == nil && choice > 0 && choice <= len(apps) { selected := apps[choice-1] wt.SaveLastApp(selected.Cmd) diff --git a/src/cmd/wt/create_test.go b/src/cmd/wt/create_test.go index 397e942..6e3370b 100644 --- a/src/cmd/wt/create_test.go +++ b/src/cmd/wt/create_test.go @@ -752,3 +752,28 @@ func TestCreate_WorktreeOpenDefault(t *testing.T) { // VSCode-during-test regression class. assertContains(t, r.Stderr, "[wt-test-no-launch]") } + +// TestCreate_DirtyStateWarningCopy asserts the corrected dirty-state warning +// string. The checks (HasUncommittedChanges/HasUntrackedFiles) run in the +// process CWD — any checkout, linked or main — so the copy must describe the +// "current worktree", not the "main repo". Running interactively (no +// --non-interactive) with a dirty repo reaches the dirty-state menu; feeding +// "3\n" (Abort) via the non-TTY fallback menu returns cleanly BEFORE any +// worktree is created, so the test leaks no side effects to the host. +func TestCreate_DirtyStateWarningCopy(t *testing.T) { + repo := createTestRepo(t) + + // Make the current checkout dirty with an untracked file. + if err := os.WriteFile(filepath.Join(repo, "dirty.txt"), []byte("uncommitted\n"), 0644); err != nil { + t.Fatalf("WriteFile dirty.txt: %v", err) + } + + r := runWtStdin(t, repo, nil, "3\n", "create", "--worktree-name", "dirty-abort-test") + + // The corrected copy is on stderr (human-facing); the stale copy is gone. + assertContains(t, r.Stderr, "current worktree has uncommitted changes") + assertNotContains(t, r.Stderr, "main repo has uncommitted changes") + + // Abort returns before creating anything — no worktree leaked. + assertWorktreeNotExists(t, repo, "dirty-abort-test") +} diff --git a/src/cmd/wt/testutil_test.go b/src/cmd/wt/testutil_test.go index b4bd8d1..d36ddf4 100644 --- a/src/cmd/wt/testutil_test.go +++ b/src/cmd/wt/testutil_test.go @@ -131,7 +131,22 @@ type wtResult struct { // runWt runs the wt binary with the given args, with cwd set to dir. // Environment variables can be passed as "KEY=VALUE" strings via env. +// Stdin is an empty reader (immediate EOF, never a TTY); tests that need to +// feed input to the child use runWtStdin directly. func runWt(t *testing.T, dir string, env []string, args ...string) wtResult { + t.Helper() + return runWtStdin(t, dir, env, "", args...) +} + +// runWtStdin runs the wt binary like runWt but feeds `stdin` to the child's +// standard input. The stdin is a plain string reader (never a TTY), so +// interactive commands land on their non-TTY fallback path — which is exactly +// what lets a test drive a fallback numbered-menu choice deterministically +// (e.g. selecting "Abort" on the dirty-state menu). +// +// This is the single core runner: runWt delegates here with empty stdin, so +// the host-isolation env list below exists exactly once. +func runWtStdin(t *testing.T, dir string, env []string, stdin string, args ...string) wtResult { t.Helper() cmd := exec.Command(wtBinary, args...) cmd.Dir = dir @@ -166,6 +181,7 @@ func runWt(t *testing.T, dir string, env []string, args ...string) wtResult { ) // Append test-provided env vars last so they can override defaults above cmd.Env = append(cmd.Env, env...) + cmd.Stdin = strings.NewReader(stdin) var stdout, stderr strings.Builder cmd.Stdout = &stdout diff --git a/src/internal/worktree/menu.go b/src/internal/worktree/menu.go index 722eae1..0266e24 100644 --- a/src/internal/worktree/menu.go +++ b/src/internal/worktree/menu.go @@ -147,6 +147,63 @@ func (s *MenuSession) showInteractive(w io.Writer, prompt string, options []stri // Close and so the contract survives future changes that acquire resources. func (s *MenuSession) Close() {} +// PromptWithDefault prompts for a line of input with a default value, reading +// through the session's SHARED stdin reader. This is the line-prompt analogue +// of Show: a session that mixes menus and line prompts (e.g. `wt create`'s +// dirty-state menu → "Worktree name" prompt) must route every stdin consumer +// through the one shared reader, or a preceding menu's parked read-ahead pump +// steals the prompt's first typed line (the menu→line-prompt byte-theft seam — +// see the MenuSession doc comment and TestUnderlyingReadAhead_DemonstratesTheft). +// +// In fallback mode (non-TTY / Windows) it delegates to the package-level +// PromptWithDefault so piped-stdin behavior is byte-for-byte identical to the +// standalone function. (A raw-mode-entry failure does NOT flip the session to +// fallback — that degrades only the affected Show call; s.interactive is +// decided once at NewMenuSession from TTY detection.) +// +// No raw mode is entered — line prompts run in cooked mode exactly as the +// package-level function does (the kernel line-buffers and echoes). EOF/empty +// semantics match that function: EOF before a newline or an empty (or +// whitespace-only, after trimming) line returns defaultValue. +func (s *MenuSession) PromptWithDefault(prompt, defaultValue string) string { + if !s.interactive { + return PromptWithDefault(prompt, defaultValue) + } + fmt.Printf(promptWithDefaultFmt, prompt, defaultValue) + line, ok := s.reader.readLine() + if !ok { + return defaultValue + } + line = strings.TrimSpace(line) + if line == "" { + return defaultValue + } + return line +} + +// ConfirmYesNo prompts for a Y/n confirmation (default yes), reading through the +// session's SHARED stdin reader — the confirmation analogue of Show and the +// same byte-theft-avoiding contract as PromptWithDefault above. +// +// In fallback mode (non-TTY / Windows) it delegates to the package-level +// ConfirmYesNo so piped-stdin behavior is byte-for-byte identical. The prompt +// is written to stderr (human copy) and EOF/empty semantics match the +// package-level function: EOF before a newline returns false; an empty (or +// whitespace-only, after trimming) line returns true (the default). Answer +// parsing is shared with that function via parseYesNoLine: a line starting +// with "y"/"Y" is yes. +func (s *MenuSession) ConfirmYesNo(prompt string) bool { + if !s.interactive { + return ConfirmYesNo(prompt) + } + fmt.Fprintf(os.Stderr, confirmYesNoFmt, prompt) + line, ok := s.reader.readLine() + if !ok { + return false + } + return parseYesNoLine(strings.TrimSpace(line)) +} + // runFallbackMenu is the historical numbered-prompt body, preserved verbatim // so the byte-for-byte output contract holds for piped/redirected callers. func runFallbackMenu(prompt string, options []string, defaultIdx int) (int, error) { @@ -206,27 +263,55 @@ func runFallbackMenu(prompt string, options []string, defaultIdx int) (int, erro } } +// Prompt rendering and answer-parsing pieces shared by the package-level +// prompts and their MenuSession variants, so the prompt text and grammar +// cannot drift between the standalone and session-aware paths. +const ( + promptWithDefaultFmt = "%s [%s]: " // stdout — PromptWithDefault + confirmYesNoFmt = "%s [Y/n] " // stderr — ConfirmYesNo +) + +// parseYesNoLine interprets an already-trimmed ConfirmYesNo answer line: +// empty means yes (the default); otherwise a line starting with "y"/"Y" is +// yes and anything else is no. +func parseYesNoLine(line string) bool { + if line == "" { + return true + } + return strings.HasPrefix(strings.ToLower(line), "y") +} + // ConfirmYesNo prompts for a Y/n confirmation. Returns true if yes (default). // The prompt is written to stderr (human-facing copy), keeping stdout reserved // for machine-readable results even when the caller's stdout is redirected // while stdin remains a TTY. +// +// This standalone form reads a line synchronously via a fresh bufio.Reader over +// os.Stdin (cooked mode), so it leaves no parked read-ahead goroutine and is +// safe for one-off use. It MUST NOT be used in a flow that also shows an +// interactive menu on the same stdin — a preceding menu's parked pump would +// steal this prompt's first line. Such mixed flows MUST use +// MenuSession.ConfirmYesNo, which reads through the session's shared reader. func ConfirmYesNo(prompt string) bool { - fmt.Fprintf(os.Stderr, "%s [Y/n] ", prompt) + fmt.Fprintf(os.Stderr, confirmYesNoFmt, prompt) reader := bufio.NewReader(os.Stdin) line, err := reader.ReadString('\n') if err != nil { return false } - line = strings.TrimSpace(line) - if line == "" { - return true - } - return strings.HasPrefix(strings.ToLower(line), "y") + return parseYesNoLine(strings.TrimSpace(line)) } // PromptWithDefault prompts for input with a default value. +// +// This standalone form reads a line synchronously via a fresh bufio.Reader over +// os.Stdin (cooked mode), so it leaves no parked read-ahead goroutine and is +// safe for one-off use. It MUST NOT be used in a flow that also shows an +// interactive menu on the same stdin — a preceding menu's parked pump would +// steal this prompt's first typed line. Such mixed flows MUST use +// MenuSession.PromptWithDefault, which reads through the session's shared reader. func PromptWithDefault(prompt, defaultValue string) string { - fmt.Printf("%s [%s]: ", prompt, defaultValue) + fmt.Printf(promptWithDefaultFmt, prompt, defaultValue) reader := bufio.NewReader(os.Stdin) line, err := reader.ReadString('\n') if err != nil { @@ -727,6 +812,37 @@ func (b *blockingByteReader) readByteWithin(timeout time.Duration) (byte, bool) } } +// readLine reads a full line through the same single pump goroutine that Show's +// key reads use, accumulating one byte at a time via readByteBlocking until a +// '\n' arrives, then stripping a trailing "\r\n" or "\n". It returns +// (line, true) on a complete line and ("", false) on a read failure/EOF before +// any newline (partial input before the failure is discarded — matching the +// err != nil short-circuit in the package-level PromptWithDefault/ConfirmYesNo). +// +// Reading through the shared reader is the whole point: it is what lets a menu +// (Show) and a line prompt (MenuSession.PromptWithDefault/ConfirmYesNo) run back +// to back on one stdin without the menu's parked pump stealing the prompt's +// first line. No raw mode is involved — line prompts run in cooked mode, so the +// kernel line-buffers and echoes; this helper only reassembles the bytes the +// pump forwards. +func (b *blockingByteReader) readLine() (string, bool) { + var sb strings.Builder + for { + bt, ok := b.readByteBlocking() + if !ok { + // EOF/error before a newline: discard partial input, signal failure. + return "", false + } + if bt == byteLF { + line := sb.String() + // Strip a trailing "\r" so "\r\n" and "\n" line endings both yield + // the bare line (the '\n' itself is already excluded above). + return strings.TrimSuffix(line, "\r"), true + } + sb.WriteByte(bt) + } +} + // parseKey reads bytes from `first` (the byte already in hand) plus `rest` // (the follow-up reader, used only for multi-byte escape sequences) and // returns the corresponding keyEvent. diff --git a/src/internal/worktree/menu_session_test.go b/src/internal/worktree/menu_session_test.go index b22d09d..5a2781e 100644 --- a/src/internal/worktree/menu_session_test.go +++ b/src/internal/worktree/menu_session_test.go @@ -174,3 +174,285 @@ func TestMenuSession_ThreeMenusNoTheft(t *testing.T) { } } } + +// ============================================================================= +// REGRESSION: menu → line-prompt byte theft (the `wt create` name-prompt bug) +// ============================================================================= +// +// `wt create` shows the dirty-state menu and then immediately prompts +// "Worktree name []:". Before the fix the menu built its own +// blockingByteReader over os.Stdin and the prompt built a FRESH bufio.Reader +// over the same fd. The menu's pump parked one byte ahead on the shared stream +// after submit — an orphan — and in cooked mode the kernel delivers a typed +// line to ONE reader; the orphan (queued first) won and slurped the line into +// its buffer, so the real prompt hung and the user's name was lost. +// +// The fix routes the line prompt through the session's SHARED reader +// (MenuSession.PromptWithDefault → blockingByteReader.readLine), so there is +// never a second reader to race — the pump's pending byte is delivered to the +// prompt instead of being stolen. + +// pushString feeds each byte of s onto the shared stream. It blocks once more +// than the stream's channel buffer is queued, so callers with a feed longer +// than the buffer must run it concurrently with a consuming reader — use +// feedAsync for that. +func pushString(s *sharedStream, str string) { + for i := 0; i < len(str); i++ { + s.push(str[i]) + } +} + +// feedAsync pushes str onto the stream from a separate goroutine (so a feed +// longer than the channel buffer does not block the test goroutine before a +// reader starts draining), optionally closing the channel afterward to +// simulate EOF. +func feedAsync(s *sharedStream, str string, closeEOF bool) { + go func() { + pushString(s, str) + if closeEOF { + close(s.bytes) + } + }() +} + +// TestUnderlyingReadAhead_MenuToLinePromptTheft characterizes WHY the menu → +// line-prompt bug existed: a blockingByteReader (the menu's) parked on a shared +// stream reads one byte ahead, so an independent second reader (a fresh +// prompt's) does NOT receive the user's typed line intact — the menu's orphan +// steals whatever byte(s) it had already pulled off the stream. This is the +// exact corruption the shared-reader fix routes around. Analogue of +// TestUnderlyingReadAhead_DemonstratesTheft for the menu→line-prompt seam. +func TestUnderlyingReadAhead_MenuToLinePromptTheft(t *testing.T) { + stream := newSharedStream() + + // The "menu" reader consumes its Enter and its pump parks one byte ahead. + menuReader := newBlockingByteReader(stream) + stream.push('\r') + if bt, ok := menuReader.readByteBlocking(); !ok || bt != '\r' { + t.Fatalf("menuReader = (%q,%v); want ('\\r',true)", bt, ok) + } + + // Force the theft deterministically: push the FIRST byte of the typed line + // and drain it back through the menu's own reader. Both operations are hard + // synchronization points (push blocks until the parked pump receives; the + // read-back blocks until that pump forwards), so the 'm' is provably held in + // the menu reader's buffer — off the shared stream, unreachable by any later + // reader — before the prompt reader is even created. No sleep, no goroutine + // race decides who wins: the menu's orphan has already stolen the byte. + stream.push('m') + if bt, ok := menuReader.readByteBlocking(); !ok || bt != 'm' { + t.Fatalf("menuReader stole byte = (%q,%v); want ('m',true) — the orphan must consume the line's first byte", bt, ok) + } + + // A fresh, independent reader for the "prompt" — the pre-fix shape (a fresh + // bufio.Reader over the same fd). Feed the REMAINDER of the line; because the + // menu's orphan already ate the leading 'm', the prompt reader can never + // assemble the intact "my-name" — it sees at most "y-name". This is the exact + // corruption the shared-reader fix routes around. + promptReader := newBlockingByteReader(stream) + feedAsync(stream, "y-name\n", false) + + got := make(chan string, 1) + go func() { + line, ok := promptReader.readLine() + if !ok { + got <- "" + return + } + got <- line + }() + select { + case line := <-got: + if line == "my-name" { + t.Fatalf("expected the menu's orphan to steal the leading byte so the prompt reader cannot read %q intact; it read the full line anyway", "my-name") + } + // Corrupted/partial ("y-name") — the theft the shared-reader fix avoids. + case <-time.After(500 * time.Millisecond): + t.Fatalf("prompt reader neither returned a corrupted line nor was starved within timeout") + } +} + +// TestMenuSession_LinePromptAfterMenuNoTheft is the regression guard for the +// fix. It drives a session.Show (the dirty-state menu) followed by a +// session.PromptWithDefault (the "Worktree name" prompt) over ONE shared +// stream — exactly the wt-create shape — and asserts the prompt receives the +// full typed line. Before the fix (a fresh reader per prompt) the line was +// stolen by the menu's orphaned pump and this would hang/starve. +func TestMenuSession_LinePromptAfterMenuNoTheft(t *testing.T) { + withDisabledColors(t) + + stream := newSharedStream() + session := &MenuSession{ + interactive: true, + reader: newBlockingByteReader(stream), + } + noopRestore := func() {} + + // Menu: submit the default row via Enter (defaultIdx 1 → submit 1). + m := make(chan int, 1) + go func() { + c, _ := session.showInteractive(io.Discard, "How to proceed?", []string{"Continue anyway", "Stash", "Abort"}, 1, noopRestore) + m <- c + }() + stream.push('\r') + select { + case c := <-m: + if c != 1 { + t.Fatalf("menu submitted %d; want 1", c) + } + case <-time.After(500 * time.Millisecond): + t.Fatalf("menu did not submit on Enter") + } + + // Line prompt on the SAME session reader: the user types a name + Enter. + // With the shared reader this line is NOT stolen by the menu's parked pump. + // + // PromptWithDefault writes its prompt to os.Stdout; the test only cares + // about the returned value, so we let that write go to the real stdout + // (harmless) and assert on the return. + p := make(chan string, 1) + go func() { p <- session.PromptWithDefault("Worktree name", "lively-tamarin") }() + pushString(stream, "chosen-name\n") + select { + case name := <-p: + if name != "chosen-name" { + t.Fatalf("BUG: prompt returned %q; want %q — the menu's orphan stole the line", name, "chosen-name") + } + case <-time.After(500 * time.Millisecond): + t.Fatalf("BUG: the line prompt after the menu hung — its input was stolen by the menu's orphaned reader") + } +} + +// ============================================================================= +// blockingByteReader.readLine — unit coverage +// ============================================================================= + +func TestBlockingByteReader_ReadLine(t *testing.T) { + tests := []struct { + name string + feed string // bytes to push + closeEOF bool // close the stream after feeding (simulates EOF) + want string + wantOK bool + }{ + {name: "multi-byte line", feed: "hello\n", want: "hello", wantOK: true}, + {name: "empty line", feed: "\n", want: "", wantOK: true}, + {name: "crlf stripped", feed: "a\r\n", want: "a", wantOK: true}, + {name: "multi-byte crlf", feed: "swift-fox\r\n", want: "swift-fox", wantOK: true}, + {name: "eof before newline discards partial", feed: "xy", closeEOF: true, want: "", wantOK: false}, + {name: "eof immediately", feed: "", closeEOF: true, want: "", wantOK: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + stream := newSharedStream() + r := newBlockingByteReader(stream) + feedAsync(stream, tc.feed, tc.closeEOF) + got := make(chan struct { + line string + ok bool + }, 1) + go func() { + line, ok := r.readLine() + got <- struct { + line string + ok bool + }{line, ok} + }() + select { + case res := <-got: + if res.line != tc.want || res.ok != tc.wantOK { + t.Fatalf("readLine() = (%q, %v); want (%q, %v)", res.line, res.ok, tc.want, tc.wantOK) + } + case <-time.After(500 * time.Millisecond): + t.Fatalf("readLine() did not return within timeout") + } + }) + } +} + +// ============================================================================= +// Session-aware PromptWithDefault / ConfirmYesNo — semantics (no PTY) +// ============================================================================= +// +// These drive the interactive-mode methods through the injected shared-reader +// seam (a MenuSession with interactive=true over a sharedStream), so the prompt +// semantics are exercised without a real terminal. The prompt text itself goes +// to the process's real stdout/stderr (harmless); the tests assert on the +// returned value. + +func newInteractiveSessionOver(stream *sharedStream) *MenuSession { + return &MenuSession{interactive: true, reader: newBlockingByteReader(stream)} +} + +func TestMenuSession_PromptWithDefault_Semantics(t *testing.T) { + tests := []struct { + name string + feed string + closeEOF bool + want string + }{ + {name: "typed value", feed: "my-name\n", want: "my-name"}, + {name: "empty line uses default", feed: "\n", want: "lively-tamarin"}, + {name: "eof before newline uses default", feed: "part", closeEOF: true, want: "lively-tamarin"}, + // Trimming matches the package-level function (fallback mode), so both + // modes behave identically on padded input. + {name: "whitespace-only line uses default", feed: " \n", want: "lively-tamarin"}, + {name: "typed value is trimmed", feed: " my-name \n", want: "my-name"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + stream := newSharedStream() + session := newInteractiveSessionOver(stream) + feedAsync(stream, tc.feed, tc.closeEOF) + got := make(chan string, 1) + go func() { got <- session.PromptWithDefault("Worktree name", "lively-tamarin") }() + select { + case v := <-got: + if v != tc.want { + t.Fatalf("PromptWithDefault = %q; want %q", v, tc.want) + } + case <-time.After(500 * time.Millisecond): + t.Fatalf("PromptWithDefault did not return within timeout") + } + }) + } +} + +func TestMenuSession_ConfirmYesNo_Semantics(t *testing.T) { + tests := []struct { + name string + feed string + closeEOF bool + want bool + }{ + {name: "empty line is default yes", feed: "\n", want: true}, + {name: "y is yes", feed: "y\n", want: true}, + {name: "Y is yes", feed: "Y\n", want: true}, + {name: "yes word is yes", feed: "yes\n", want: true}, + {name: "n is no", feed: "n\n", want: false}, + {name: "no word is no", feed: "no\n", want: false}, + {name: "eof is no", feed: "", closeEOF: true, want: false}, + // Trimming matches the package-level function (fallback mode), so both + // modes behave identically on padded input. + {name: "whitespace-only line is default yes", feed: " \n", want: true}, + {name: "padded y is yes", feed: " y \n", want: true}, + {name: "padded n is no", feed: " n \n", want: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + stream := newSharedStream() + session := newInteractiveSessionOver(stream) + feedAsync(stream, tc.feed, tc.closeEOF) + got := make(chan bool, 1) + go func() { got <- session.ConfirmYesNo("Initialize worktree?") }() + select { + case v := <-got: + if v != tc.want { + t.Fatalf("ConfirmYesNo = %v; want %v", v, tc.want) + } + case <-time.After(500 * time.Millisecond): + t.Fatalf("ConfirmYesNo did not return within timeout") + } + }) + } +}