diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d23a6c..a33e617 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ and the project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/v > Tags prior to **v0.4.0** were cut in the private repository and produced no > public artifacts; the first publicly released version is v0.4.0. +## [Unreleased] + +### Changed +- **Interactive Yes/No confirmation prompts.** On a TTY, every confirmation + prompt — the pre-send `.commitbrief/**` guard, the secret-scan warning, the + cost preflight, the token/context-window preflight, `cache clear`, and the + `compress` replace prompt — now renders an arrow-key-selectable Yes/No toggle + (←/→ to choose, Enter to confirm), pre-selected on **No**, instead of a typed + `y/N` line. Built on `huh`, which is already used by `setup`. Non-TTY/CI + behaviour is unchanged: these prompts still auto-abort (or honour `--yes` / + `--allow-secrets` where applicable), and piped input continues to drive the + line-based `y/N` fallback. New catalog keys `common.affirmative` / + `common.negative` provide the localized button labels (TR: `Evet` / `Hayır`). + ## [1.4.1] - 2026-05-30 ### Changed diff --git a/internal/cli/cache.go b/internal/cli/cache.go index f0d40e3..c8afdaf 100644 --- a/internal/cli/cache.go +++ b/internal/cli/cache.go @@ -61,11 +61,12 @@ func newCacheClearCmd() *cobra.Command { "cache.clear.summary", files, formatBytes(totalBytes), cacheDir)) if !global.yes { - ok, err := ui.AskYesNo( + ok, err := ui.Confirm( os.Stdin, cmd.OutOrStderr(), app.Catalog.T("cache.clear.confirm"), ui.AskOptions{ + Interactive: ui.IsStdinTTY(os.Stdin), NonInteractive: !ui.IsStdinTTY(os.Stdin), Catalog: app.Catalog, }, diff --git a/internal/cli/compress.go b/internal/cli/compress.go index ec7b83b..82539f7 100644 --- a/internal/cli/compress.go +++ b/internal/cli/compress.go @@ -129,11 +129,12 @@ func newCompressCmd() *cobra.Command { // Confirmation prompt unless --yes. if !global.yes { - ok, err := ui.AskYesNo( + ok, err := ui.Confirm( os.Stdin, cmd.OutOrStderr(), app.Catalog.T("compress.replace_prompt"), ui.AskOptions{ + Interactive: ui.IsStdinTTY(os.Stdin), NonInteractive: !ui.IsStdinTTY(os.Stdin), Catalog: app.Catalog, }, diff --git a/internal/cli/review.go b/internal/cli/review.go index a2337b0..184465f 100644 --- a/internal/cli/review.go +++ b/internal/cli/review.go @@ -142,6 +142,7 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er if res, _ := guard.CheckDiffForLocalConfig(parsed, guard.Options{ AssumeYes: global.yes, NonInteractive: !ui.IsStdinTTY(os.Stdin), + Interactive: ui.IsStdinTTY(os.Stdin), Catalog: app.Catalog, Reader: stdinReader, }); res == guard.Abort { @@ -544,12 +545,14 @@ func handleTokenPreflight(cmd *cobra.Command, app *appContext, prov provider.Pro return true } - _, _ = fmt.Fprint(w, app.Catalog.T("guard.tokens.confirm_prompt")) - answer, err := readPromptLine(stdin) - if err != nil || answer == "" { - return true - } - return !ui.AcceptsYes(answer, app.Catalog) + // Interactive is hardcoded true (not IsStdinTTY) because the abort + // above already guarantees a TTY here. Do NOT soften it to + // IsStdinTTY: that routes the non-TTY case through ui.Confirm's + // line-based fallback, but non-TTY must abort (handled above), + // never line-read a piped answer. + ok, err := ui.Confirm(stdin, w, app.Catalog.T("guard.tokens.confirm_prompt"), + ui.AskOptions{Interactive: true, Catalog: app.Catalog}) + return err != nil || !ok } // suggestCommitMessage runs a second, free-form provider call (ADR-0015) @@ -814,29 +817,14 @@ func handleCostPreflight(cmd *cobra.Command, app *appContext, estCost float64, s return true } - _, _ = fmt.Fprint(w, app.Catalog.T("cost.confirm_prompt")) - answer, err := readPromptLine(stdin) - if err != nil || answer == "" { - return true - } - return !ui.AcceptsYes(answer, app.Catalog) -} - -// readPromptLine pulls one line off the shared runReview-scoped -// bufio.Reader. UC-21: every interactive prompt during a review -// (guard, secret scan, cost preflight) shares the same buffered -// reader so a piped-in `e\ne\ne\n` reaches all three sites instead -// of being swallowed by whichever scanner asked first. Returns the -// trimmed lowercase answer and any read error. -func readPromptLine(r *bufio.Reader) (string, error) { - line, err := r.ReadString('\n') - if err != nil && line == "" { - if err == io.EOF { - return "", nil - } - return "", err - } - return strings.TrimSpace(strings.ToLower(line)), nil + // Interactive is hardcoded true (not IsStdinTTY): the abort above + // guarantees a TTY here. Do NOT soften it to IsStdinTTY — that would + // route non-TTY through the line fallback and let piped input + // auto-approve spend, reopening the hole UC-06 closed (non-TTY must + // abort, --yes must not bypass; PRD §310). + ok, err := ui.Confirm(stdin, w, app.Catalog.T("cost.confirm_prompt"), + ui.AskOptions{Interactive: true, Catalog: app.Catalog}) + return err != nil || !ok } // estimateOutputTokens is a conservative-on-the-high-side guess for @@ -877,10 +865,12 @@ func handleSecretMatches(cmd *cobra.Command, app *appContext, matches []guard.Se return true } - _, _ = fmt.Fprint(w, app.Catalog.T("guard.secrets.prompt")) - answer, err := readPromptLine(stdin) - if err != nil || answer == "" { - return true - } - return !ui.AcceptsYes(answer, app.Catalog) + // Interactive is hardcoded true (not IsStdinTTY): the abort above + // guarantees a TTY here. Do NOT soften it to IsStdinTTY — that would + // route non-TTY through the line fallback and let piped input + // auto-confirm a detected secret, reopening the hole UC-01 closed + // (non-TTY must abort, --yes must not bypass; PRD §309). + ok, err := ui.Confirm(stdin, w, app.Catalog.T("guard.secrets.prompt"), + ui.AskOptions{Interactive: true, Catalog: app.Catalog}) + return err != nil || !ok } diff --git a/internal/cli/shared_stdin_test.go b/internal/cli/shared_stdin_test.go index c435419..b1a2d89 100644 --- a/internal/cli/shared_stdin_test.go +++ b/internal/cli/shared_stdin_test.go @@ -4,59 +4,62 @@ package cli import ( "bufio" + "io" "strings" "testing" + + "github.com/CommitBrief/commitbrief/internal/ui" ) -func TestReadPromptLineConsumesOneLinePerCall(t *testing.T) { - // UC-21 regression guard. The shared *bufio.Reader created at the - // top of runReview must return one line per ReadString('\n') call - // — three sequential prompts (guard → secret → cost) reading from - // the same buffer must each get their own answer instead of one - // scanner gobbling everything via lookahead. +// UC-21 regression guard. runReview creates one *bufio.Reader at the top +// and threads it through every interactive prompt (guard → secret scan → +// token/cost preflight) via ui.Confirm. Each prompt must consume exactly +// one line from the shared buffer; a per-call bufio.Scanner inside +// AskYesNo would over-read via lookahead and swallow the answers meant +// for later prompts. These tests pin the behaviour at the ui.Confirm +// boundary the review pipeline actually uses. + +func TestSharedReaderConsumesOneLinePerConfirm(t *testing.T) { r := bufio.NewReader(strings.NewReader("yes\nno\ny\n")) - answers := make([]string, 0, 3) + got := make([]bool, 0, 3) for i := 0; i < 3; i++ { - ans, err := readPromptLine(r) + ok, err := ui.Confirm(r, io.Discard, "?", ui.AskOptions{}) if err != nil { t.Fatalf("call %d: %v", i, err) } - answers = append(answers, ans) + got = append(got, ok) } - if got, want := strings.Join(answers, ","), "yes,no,y"; got != want { - t.Errorf("answers = %q, want %q (one line per prompt)", got, want) + + want := []bool{true, false, true} + for i := range want { + if got[i] != want[i] { + t.Fatalf("answers = %v, want %v (one line consumed per prompt)", got, want) + } } } -func TestReadPromptLineHandlesNoTrailingNewline(t *testing.T) { - // A user pressing Ctrl-D after typing "y" (no newline) should - // still surface "y" as the answer, not "". +func TestSharedReaderHandlesNoTrailingNewline(t *testing.T) { + // A user pressing Ctrl-D after typing "y" (no newline) should still + // surface "y" as affirmative, not an empty/negative answer. r := bufio.NewReader(strings.NewReader("y")) - got, err := readPromptLine(r) + ok, err := ui.Confirm(r, io.Discard, "?", ui.AskOptions{}) if err != nil { t.Fatal(err) } - if got != "y" { - t.Errorf("answer = %q, want %q", got, "y") - } -} - -func TestReadPromptLineNormalisesCaseAndWhitespace(t *testing.T) { - r := bufio.NewReader(strings.NewReader(" YES \n")) - got, _ := readPromptLine(r) - if got != "yes" { - t.Errorf("answer = %q, want lowercased+trimmed yes", got) + if !ok { + t.Errorf("no-trailing-newline 'y' should be affirmative") } } -func TestReadPromptLineEOFReturnsEmpty(t *testing.T) { +func TestSharedReaderEOFIsNegative(t *testing.T) { + // Drained buffer (immediate EOF) must read as the default-no, not error. r := bufio.NewReader(strings.NewReader("")) - got, err := readPromptLine(r) + ok, err := ui.Confirm(r, io.Discard, "?", ui.AskOptions{}) if err != nil { - t.Errorf("EOF should return (\"\", nil), got err=%v", err) + t.Errorf("EOF should return (false, nil); got err=%v", err) } - if got != "" { - t.Errorf("EOF should return empty string; got %q", got) + if ok { + t.Errorf("EOF should be negative (default no)") } } diff --git a/internal/guard/presend.go b/internal/guard/presend.go index 9295283..74aec4f 100644 --- a/internal/guard/presend.go +++ b/internal/guard/presend.go @@ -38,6 +38,12 @@ type Options struct { Writer io.Writer Reader io.Reader + // Interactive routes the confirm through ui.Confirm's arrow-key + // Yes/No toggle (huh) instead of reading a line off Reader. CLI + // callers set it from ui.IsStdinTTY(os.Stdin); tests leave it + // false so the deterministic line path (Reader) still drives them. + Interactive bool + // Catalog plumbs i18n into the .commitbrief/* write-guard so the // user-visible warning, file lines, prompt, and abort messages // honour the active locale. Nil → English defaults (legacy @@ -74,6 +80,29 @@ func CheckDiffForLocalConfig(d diff.Diff, opts Options) (Result, error) { } prompt := tr(opts.Catalog, "guard.prompt", " Continue?") + + if opts.Interactive { + // Interactive routes through huh and ignores the reader, but pass + // a real one (shared reader, else os.Stdin) so a future change to + // Confirm's routing degrades to the line path with a usable reader + // instead of a nil-deref panic. + r := opts.Reader + if r == nil { + r = os.Stdin + } + ok, err := ui.Confirm(r, w, prompt, ui.AskOptions{ + Interactive: true, + Catalog: opts.Catalog, + }) + if err != nil { + return Abort, fmt.Errorf("guard: confirm: %w", err) + } + if ok { + return Continue, nil + } + return Abort, nil + } + suffix := ui.PromptSuffix(opts.Catalog) _, _ = fmt.Fprintf(w, "%s %s: ", prompt, suffix) diff --git a/internal/i18n/messages.en.yml b/internal/i18n/messages.en.yml index 9dc128b..b890cbc 100644 --- a/internal/i18n/messages.en.yml +++ b/internal/i18n/messages.en.yml @@ -18,16 +18,16 @@ guard.prompt: " Continue?" guard.non_interactive: "Aborting (non-interactive mode); pass --yes to override." guard.secrets.detected: "⚠ Possible secrets detected in diff (%d line(s)):" guard.secrets.line: " line %d: %s" -guard.secrets.prompt: " Send to LLM anyway? [y/N]: " +guard.secrets.prompt: "Send to LLM anyway?" guard.secrets.aborted_user: "aborted: pre-send secret scanner" guard.secrets.aborted_non_interactive: "Aborted (non-interactive); pass --allow-secrets to override." cost.estimate: "⚠ Estimated cost: $%.4f (threshold: $%.4f)" -cost.confirm_prompt: " Proceed with the review? [y/N]: " +cost.confirm_prompt: "Proceed with the review?" cost.aborted_user: "aborted: cost preflight" cost.aborted_non_interactive: "Aborted (non-interactive); pass --no-cost-check or raise cost.warn_threshold_usd to override." guard.tokens.exceeds: "⚠ Estimated prompt is %d tokens; %s's context window for this model is %d." -guard.tokens.confirm_prompt: " Send it anyway? [y/N]: " +guard.tokens.confirm_prompt: "Send it anyway?" guard.tokens.aborted_user: "aborted: token preflight" guard.tokens.aborted_non_interactive: "Aborted (non-interactive); set guard.token_preflight=false to override." @@ -80,6 +80,11 @@ common.error_prefix: "Error:" common.yes_short: "y" common.yes_long: "yes" common.prompt_yn: "[y/N]" +# Button labels for the interactive (huh) Yes/No toggle that +# ui.Confirm renders on a TTY. Title-cased, locale-specific. The +# line-based fallback keeps using prompt_yn / yes_short above. +common.affirmative: "Yes" +common.negative: "No" init.wrote: "Wrote %s" init.skipped: "Skipped %s (already exists; pass --force to overwrite)" diff --git a/internal/i18n/messages.tr.yml b/internal/i18n/messages.tr.yml index 4428e4b..dba9734 100644 --- a/internal/i18n/messages.tr.yml +++ b/internal/i18n/messages.tr.yml @@ -18,16 +18,16 @@ guard.prompt: " Devam edilsin mi?" guard.non_interactive: "İptal ediliyor (etkileşimsiz mod); zorlamak için --yes kullanın." guard.secrets.detected: "⚠ Diff'te olası gizli anahtar tespit edildi (%d satır):" guard.secrets.line: " satır %d: %s" -guard.secrets.prompt: " Yine de LLM'e gönderilsin mi? [e/H]: " +guard.secrets.prompt: "Yine de LLM'e gönderilsin mi?" guard.secrets.aborted_user: "iptal edildi: pre-send secret scanner" guard.secrets.aborted_non_interactive: "İptal edildi (etkileşimsiz); zorlamak için --allow-secrets kullanın." cost.estimate: "⚠ Tahmini maliyet: $%.4f (eşik: $%.4f)" -cost.confirm_prompt: " Review devam etsin mi? [e/H]: " +cost.confirm_prompt: "Review devam etsin mi?" cost.aborted_user: "iptal edildi: cost preflight" cost.aborted_non_interactive: "İptal edildi (etkileşimsiz); zorlamak için --no-cost-check veya cost.warn_threshold_usd değerini yükseltin." guard.tokens.exceeds: "⚠ Tahmini prompt %d token; %s sağlayıcısının bu model için bağlam penceresi %d." -guard.tokens.confirm_prompt: " Yine de gönderilsin mi? [e/H]: " +guard.tokens.confirm_prompt: "Yine de gönderilsin mi?" guard.tokens.aborted_user: "iptal edildi: token preflight" guard.tokens.aborted_non_interactive: "İptal edildi (etkileşimsiz); geçersiz kılmak için guard.token_preflight=false yapın." @@ -78,6 +78,11 @@ common.error_prefix: "Hata:" common.yes_short: "e" common.yes_long: "evet" common.prompt_yn: "[e/H]" +# Etkileşimli (huh) Evet/Hayır toggle'ının buton etiketleri; +# ui.Confirm TTY'de bunu çizer. Satır tabanlı fallback yukarıdaki +# prompt_yn / yes_short'u kullanmaya devam eder. +common.affirmative: "Evet" +common.negative: "Hayır" init.wrote: "%s yazıldı" init.skipped: "%s atlandı (zaten mevcut; üzerine yazmak için --force kullanın)" diff --git a/internal/ui/prompt.go b/internal/ui/prompt.go index 8702f19..c734056 100644 --- a/internal/ui/prompt.go +++ b/internal/ui/prompt.go @@ -6,8 +6,11 @@ import ( "bufio" "fmt" "io" + "os" "strings" + "github.com/charmbracelet/huh" + "github.com/CommitBrief/commitbrief/internal/i18n" ) @@ -15,6 +18,15 @@ type AskOptions struct { AssumeYes bool NonInteractive bool + // Interactive, when true, makes Confirm render an arrow-key + // Yes/No toggle (huh) read from the controlling terminal instead + // of the line-based fallback. Callers derive it from + // IsStdinTTY(os.Stdin) — it is NOT inferred from the reader, + // because the review-scoped shared reader is a *bufio.Reader, not + // the *os.File the TTY check needs. AssumeYes and NonInteractive + // still take precedence. Has no effect on AskYesNo (line-only). + Interactive bool + // Catalog, when non-nil, controls the prompt suffix ("[y/N]" vs // "[e/H]") and the accepted-affirmative vocabulary // (common.yes_short / common.yes_long). When nil, falls back to @@ -47,20 +59,90 @@ func AskYesNo(r io.Reader, w io.Writer, question string, opts AskOptions) (bool, if _, err := fmt.Fprintf(w, "%s %s: ", question, suffix); err != nil { return false, fmt.Errorf("ui: write prompt: %w", err) } - scanner := bufio.NewScanner(r) - if !scanner.Scan() { - if err := scanner.Err(); err != nil { - return false, fmt.Errorf("ui: read answer: %w", err) - } - return false, nil + answer, err := readLine(r) + if err != nil { + return false, fmt.Errorf("ui: read answer: %w", err) } - answer := strings.ToLower(strings.TrimSpace(scanner.Text())) if AcceptsYes(answer, opts.Catalog) { return true, nil } return false, nil } +// readLine reads exactly one line from r without lookahead, returning it +// trimmed and lower-cased for AcceptsYes. When r is already a +// *bufio.Reader (the runReview-scoped shared reader threaded through +// every interactive prompt) it is used directly so each prompt consumes +// its own line; a fresh bufio.Scanner would over-read and swallow the +// answers meant for later prompts (UC-21 — guard → secret scan → token → +// cost all fire in sequence on one review). Non-buffered readers are +// wrapped once. Mirrors guard.readAnswer; both layers share this surgical +// read so the shared-reader contract holds wherever a line is consumed. +func readLine(r io.Reader) (string, error) { + br, ok := r.(*bufio.Reader) + if !ok { + br = bufio.NewReader(r) + } + line, err := br.ReadString('\n') + if err != nil && line == "" { + if err == io.EOF { + return "", nil + } + return "", err + } + return strings.ToLower(strings.TrimSpace(line)), nil +} + +// Confirm asks a yes/no question, defaulting to No. On an interactive +// terminal (opts.Interactive) it renders an arrow-key-selectable Yes/No +// toggle via huh, read from the controlling terminal directly. Otherwise +// it falls back to the line-based AskYesNo over r/w — the path tests and +// non-TTY pipelines exercise. AssumeYes/NonInteractive short-circuit +// before either path, so the interactive toggle never appears in CI. +func Confirm(r io.Reader, w io.Writer, question string, opts AskOptions) (bool, error) { + if opts.AssumeYes { + return true, nil + } + if opts.NonInteractive { + return false, nil + } + if opts.Interactive { + return confirmInteractive(question, opts.Catalog) + } + return AskYesNo(r, w, question, opts) +} + +// confirmInteractive renders the huh Yes/No toggle on the controlling +// terminal (input from os.Stdin, output to os.Stderr so a captured +// stdout — e.g. --json — stays clean). Button labels come from the +// catalog (common.affirmative / common.negative) so non-English locales +// get native Yes/No. The bound value starts false, so "No" is the +// pre-selected default — matching the line-based default-to-no. +func confirmInteractive(question string, catalog *i18n.Catalog) (bool, error) { + affirmative, negative := "Yes", "No" + if catalog != nil { + if s := strings.TrimSpace(catalog.T("common.affirmative")); s != "" && s != "common.affirmative" { + affirmative = s + } + if s := strings.TrimSpace(catalog.T("common.negative")); s != "" && s != "common.negative" { + negative = s + } + } + + confirm := false + form := huh.NewForm(huh.NewGroup( + huh.NewConfirm(). + Title(strings.TrimSpace(question)). + Affirmative(affirmative). + Negative(negative). + Value(&confirm), + )).WithInput(os.Stdin).WithOutput(os.Stderr) + if err := form.Run(); err != nil { + return false, fmt.Errorf("ui: confirm prompt: %w", err) + } + return confirm, nil +} + // AcceptsYes reports whether `answer` is an affirmative response. The // EN defaults ("y"/"yes") are always accepted. When catalog is // non-nil, the catalog's `common.yes_short` / `common.yes_long` diff --git a/internal/ui/ui_test.go b/internal/ui/ui_test.go index ed52bdd..9c8f603 100644 --- a/internal/ui/ui_test.go +++ b/internal/ui/ui_test.go @@ -163,6 +163,40 @@ func TestAskYesNoNonInteractive(t *testing.T) { } } +func TestConfirmAssumeYesShortCircuits(t *testing.T) { + // AssumeYes wins before any reader/TTY path — even with Interactive set, + // no huh form is constructed (which would fail without a real terminal). + got, err := Confirm(strings.NewReader(""), io.Discard, "Continue?", + AskOptions{AssumeYes: true, Interactive: true}) + if err != nil || !got { + t.Errorf("AssumeYes: got=%v err=%v", got, err) + } +} + +func TestConfirmNonInteractiveDeclines(t *testing.T) { + // NonInteractive takes precedence over Interactive and declines without + // touching the terminal. + got, err := Confirm(strings.NewReader("y\n"), io.Discard, "Continue?", + AskOptions{NonInteractive: true, Interactive: true}) + if err != nil || got { + t.Errorf("NonInteractive: got=%v err=%v", got, err) + } +} + +func TestConfirmFallsBackToLineWhenNotInteractive(t *testing.T) { + // Without Interactive (the test/non-TTY case) Confirm delegates to the + // line-based AskYesNo over the supplied reader. + for ans, want := range map[string]bool{"y\n": true, "n\n": false, "\n": false} { + got, err := Confirm(strings.NewReader(ans), io.Discard, "Continue?", AskOptions{}) + if err != nil { + t.Fatalf("answer %q: %v", ans, err) + } + if got != want { + t.Errorf("answer %q: got=%v want=%v", ans, got, want) + } + } +} + func TestAskYesNoAcceptsTurkishWhenCatalogPassed(t *testing.T) { // UC-14 regression guard. With a TR catalog, "e" and "evet" must // count as affirmative. The English forms still work too — locale