cmd/prompt: use ReadString instead of Fscanln, fix default value, and remove uses github.com/AlecAivazis/survey/v2

[thaJeztah]

cmd/prompt: use ReadString instead of Fscanln and fix default value

Use a buffered line reader for non-interactive confirmation prompts
instead of fmt.Fscanln.

This preserves empty input, allowing callers to use the configured
default value, and avoids tokenizing input on whitespace.

cmd/prompt: remove uses of github.com/AlecAivazis/survey/v2

The github.com/AlecAivazis/survey/v2 module was archived and is no longer
maintained. Looking at the code, we didn't really use most of its features;
current callers already include the confirmation hint in the prompt
message, so survey only adds terminal rendering behaviour around a simple
yes/no question. Using the local implementation keeps the exported prompt
types intact while removing the dependency.

Use the same line-based confirmation implementation for terminal and
non-terminal prompts.

What I did

Related issue

(not mandatory) A picture of a cute animal, if possible in relation to what you did

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

[docker-agent]

[MEDIUM] Unrecognized user input silently returns false instead of re-prompting or returning an error

utils.StringToBool discards the parse error and returns false for any string it doesn't recognise (e.g. a typo like "yse" or "noo"):

// pkg/utils/stringutils.go
func StringToBool(s string) bool {
    s = strings.ToLower(strings.TrimSpace(s))
    if s == "y" { return true }
    b, _ := strconv.ParseBool(s)   // error silently ignored
    return b
}

Before this PR, User.Confirm used survey.AskOne, which re-prompted the user when the input was not a valid yes/no answer. The new confirm() helper calls utils.StringToBool directly, so a typo now silently resolves to false — the same as if the user had said "No" — with no feedback.

Consider validating the trimmed answer against an explicit set of accepted strings (e.g. y, yes, n, no, true, false, 1, 0) and returning an error (or re-prompting) for anything else. Since utils.StringToBool does not return an error, this would require a small local validation step before delegating to it.

[thaJeztah]

This is an existing issue; at least it was already inconsistent; if a TTY is present, it would use "survey", which would handle invalid input, but the other implementation didn't.

It's probably fine as-is for y/n prompts, but we could improve this (as a follow-up)?

[docker-agent]

Fair point — thanks for the context. Since this PR unifies the TTY and non-TTY paths and the silent-false behavior was already present in the non-TTY case, it's not a regression. Agree that improving validation as a follow-up makes sense rather than blocking this PR.

[docker-agent]

[LOW] bufio.NewReader created per call may silently discard buffered stdin bytes on repeated Confirm calls

bufio.NewReader(stdin) wraps the underlying io.Reader in a new 4096-byte buffer on every call to confirm(). If the user types a response faster than the read consumes it, the bufio.Reader may buffer bytes beyond the newline delimiter. When the reader goes out of scope at the end of confirm(), those buffered-ahead bytes are lost; the next confirm() call creates a fresh bufio.Reader from the (already-advanced) underlying reader and misses them.

Current call-sites each make only a single Confirm call, so this does not trigger today. However, the design is fragile: any future caller that prompts more than once on the same stdin (e.g. a loop, or two sequential prompts in the same command handler) would silently drop input.

Consider storing a single *bufio.Reader on the User and Pipe structs (or accepting one as a constructor parameter) so the buffer is shared across calls rather than recreated each time.

[thaJeztah]

Looks like a pre-existing issue

[docker-agent]

You're absolutely right — this pattern was already present before this PR. Since it's a latent issue that doesn't affect current usage and would require a small refactor to fix, it makes sense to handle it separately rather than blocking this change.

Feel free to ignore this comment for now, or we can track it as a potential future improvement if you think it's worth addressing.

[codecov]

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/prompt/prompt.go	70.00%	2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR updates the confirmation prompt implementation to use a line-based reader (preserving empty input/defaults and avoiding whitespace tokenization) and removes the archived github.com/AlecAivazis/survey/v2 dependency by using a local confirm implementation for both terminal and non-terminal cases.

Changes:

• Replaced survey-based terminal confirmation prompts with a shared local confirm() helper.
• Switched non-interactive confirmation input parsing from fmt.Fscanln to bufio.Reader.ReadString('\n') to preserve empty input and allow defaults.
• Removed survey (and now-unused indirect deps) from go.mod/go.sum.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File	Description
cmd/prompt/prompt.go	Replaces survey confirm with a shared line-based confirm() helper for both terminal and pipe input.
go.mod	Drops the direct survey/v2 dependency and related indirect requirements.
go.sum	Removes checksum entries for survey/v2 and its transitive dependencies no longer needed.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

This PR correctly removes the archived github.com/AlecAivazis/survey/v2 dependency and fixes the empty-input / default-value bug in the non-interactive path. Two issues introduced by the new unified confirm helper are worth addressing before merge.

[docker-agent]

[MEDIUM] Unrecognised user input silently returns false instead of re-prompting or returning an error

The new unified confirm helper calls utils.StringToBool(answer), which silently returns false for any input it cannot parse (e.g. "maybe", "oui", a typo like "yse"):

// pkg/utils/stringutils.go
func StringToBool(s string) bool {
    s = strings.ToLower(strings.TrimSpace(s))
    if s == "y" { return true }
    b, _ := strconv.ParseBool(s)  // error silently discarded
    return b
}

The old survey-based User.Confirm would re-prompt on unrecognised input. The new code — used for both User and Pipe — returns false silently with no error and no indication to the user that their input was not understood. For destructive operations (e.g. removing containers), a typo could cause the prompt to silently proceed as "no" rather than asking again.

Consider one of:

1. Return an error for unrecognised input so callers can surface it.
2. Fall back to defaultValue for unrecognised input (with an optional warning on stdout).
3. Loop and re-prompt (matching the old survey behaviour) up to a maximum retry count.

[docker-agent]

[LOW] A new bufio.Reader per call may silently discard buffered stdin if confirm is called more than once

bufio.NewReader(stdin) allocates a 4096-byte read-ahead buffer. On each call to confirm a brand-new reader is created; when the function returns the reader is dropped, along with any bytes it read ahead past the \n delimiter. A second call creates another fresh reader reading from the same underlying stdin, but those already-buffered bytes are gone.

In the current codebase confirm is only called once per command invocation, so this is latent. However it is a footgun for any future caller that asks multiple confirmation questions, and the behaviour is introduced by this PR (replacing the old survey-based path, which maintained its own state).

A safer pattern is to create the bufio.Reader once at the User/Pipe construction level and pass it into confirm, ensuring all reads share a single buffer.

[thaJeztah]

Pre-existing issue; same as #13900 (comment)

[docker-agent]

You're right that the Pipe path had inline confirmation before, but the previous implementation used fmt.Fscanln which reads unbuffered from the underlying io.Reader. This PR replaces that with bufio.NewReader(stdin).ReadString('\n'), which introduces the buffer-per-call issue.

The User path previously delegated to survey, which would have managed buffering internally. So while the code structure existed, the buffering footgun is new to this PR's rewrite.

That said, if this doesn't align with the project's priorities for this refactor, happy to defer it. Just wanted to flag the behaviour change.

[docker-agent]

Assessment: 🟢 APPROVE

The changes look correct and well-implemented. The new confirm helper function properly replaces both survey.Confirm (TTY path) and fmt.Fscanln (pipe path) with a unified bufio.NewReader.ReadString('\n') approach. The default value fix works correctly: empty input (trimmed to "") now returns defaultValue instead of passing "" to StringToBool. The io.EOF handling is appropriate for non-interactive/piped inputs. Removing the archived github.com/AlecAivazis/survey/v2 dependency and its transitives (kballard/go-shellquote, mattn/go-colorable, mattn/go-isatty, mgutz/ansi) is clean.

[glours]

I have concerns about how sequential confirmations are handled with this change, in particular for the publish flow, which can fire up to 4 prompts in a single run (bind mounts → sensitive data → env findings → literal config.content). Because
confirm() creates a fresh bufio.Reader on every call, the first prompt reads ahead and swallows the following answers on piped stdin, so e.g. printf "y\ny\n" | docker compose publish ... silently cancels on the second prompt, something that
worked with the previous Fscanln-based implementation. I think the current implementation will break that user flow, so I'd like this addressed before we merge.

IMHO we should, at least, add a e2e test on publish with multiple approvals to ensure it won't break UX

[thaJeztah]

in particular for the publish flow, which can fire up to 4 prompts in a single run

Ah, good point; I thought it would be called once, not repeated, but didn't think of such situations.

I'll have a look at updating; it's not urgent, just this dependency caught my attention and I thought we would be able to get rid of it.