fix(cli): suppress the --debug hint when a confirmation prompt is declined - #5946
Merged
Coly010 merged 2 commits intoJul 27, 2026
Conversation
…lined (CLI-1973) Go's recoverAndExit (apps/cli-go/cmd/root.go:287-303) prints only the red 'context canceled' line for errors.Is(err, context.Canceled) and skips the SuggestDebugFlag fallback -- declining a prompt is a user decision, not an error to troubleshoot. The TS text Output.fail appended the hint for every suggestion-less error, including cancellations, on every prompting command (logout, db push/reset, migration fetch/repair/down, functions deploy --prune, gen signing-key, secrets unset, projects delete, branches create, bootstrap). Mirror Go's guard at the render boundary: a shared CONTEXT_CANCELED_MESSAGE sentinel (shared/output/errors.ts) now gates the debug-hint fallback in the text layer, with the explicit-suggestion branch kept first (Go prints a pre-set CmdSuggestion even for canceled errors). All 14 cancellation construction sites reference the constant; the redundant LEGACY_LOGOUT_CANCELLED_MESSAGE is folded into it and logout.errors.ts's intent comment now matches the implemented behavior. Tests: unit byte-assertions on the fail renderer (sentinel suppresses the hint; explicit suggestion still prints), plus declined-prompt stderr-byte e2e per family -- logout (new), migration fetch (extended), db reset (new, Docker-free: the prompt fires before any connection), gen signing-key (extended) -- each asserting the last stderr line is exactly 'context canceled', no debug hint, exit code 1. stripAnsi hoisted to tests/helpers/cli.ts for the touched files.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@3651e9a29a1251d57bd3b3eac93c6585e84c6d6dPreview package for commit |
kanadgupta
approved these changes
Jul 27, 2026
kanadgupta
left a comment
kanadgupta
left a comment
Contributor
There was a problem hiding this comment.
Two non-blocking comments otherwise LGTM
Consolidates 16 near-duplicate local stripAnsi definitions onto a single helper. Moved it into its own tests/helpers/ansi.ts module (re-exported from tests/helpers/cli.ts for existing e2e callers) so unit/integration tests don't drag in cli.ts's subprocess-spawning e2e machinery. The shared implementation strips any CSI escape sequence, matching agent-output.e2e.test.ts's stricter cursor-code-aware version rather than the color-only \x1b[...m regex most copies used.
Coly010
deleted the
columferry/cli-1973-declined-confirmation-prompts-print-a-spurious-debug-hint
branch
July 27, 2026 20:03
pull Bot
pushed a commit
to oogalieboogalie/cli
that referenced
this pull request
Jul 27, 2026
…tion classifiers (supabase#5948) ## What changed Every legacy db command's connection failure previously rendered as ``` failed to connect to postgres: effect/sql/SqlError: PgClient: Failed to connect ``` — host, user, database, and the underlying driver cause were all lost, on every `db push` / `db pull` / `db reset` / `db diff --linked` / `inspect` / `migration` connection failure. The Go CLI renders pgconn's full detail: ``` failed to connect to postgres: failed to connect to `host=… user=… database=…`: dial error (dial tcp …: connect: connection refused) ``` This PR ports pgconn's `connectError` rendering (`errors.go:66-72`, wrapped by `pkg/pgxv5/connect.go:33`) into the legacy connection layer, and fixes the `legacyConnectSuggestion` classifier branches that could never fire on real node-postgres error shapes. ### Message rendering (`legacyConnectFailureMessage`) - `toConnectError` in `legacy-db-connection.sql-pg.layer.ts` now renders `failed to connect to postgres: failed to connect to \`host=… user=… database=…\`: <staged cause>` using the config-level identity (host/user/database — never the password), exactly like pgconn. - The cause is unwrapped through the real `SqlError → ConnectionError → driver error` chain (and through `AggregateError.errors[]`, taking the LAST attempt — pgconn's fallback loop also surfaces the last attempt's error). - Stage labels mirror pgconn where the node error identifies the stage unambiguously: `server error (SEVERITY: message (SQLSTATE code))` (byte-parity with pgconn's `PgError` rendering), `hostname resolving error (…)`, `dial error (…)`, `tls error (…)`. ### Classifier fixes (`legacyConnectSuggestion`) Branch-by-branch verification against real driver shapes (captured empirically under Bun, the CLI's runtime): | Go branch | node-postgres shape | Before | After | |---|---|---|---| | `connect: connection refused` / allow_list → network-restrictions hint | `ECONNREFUSED` code / server text | fired | fires (unchanged, now also covered by real-`SqlError` tests) | | `SSL connection is required` + `--debug` | server error text | fired | fires (unchanged) | | `SCRAM exchange: Wrong password` / `failed SASL auth` → `SUPABASE_DB_PASSWORD` hint | `DatabaseError` 28P01 `password authentication failed` | fired | fires (unchanged, proven against a real wire-protocol ErrorResponse) | | IPv6-only host → IPv6 pooler hint | `EHOSTUNREACH`/`EADDRNOTAVAIL`/`ENETUNREACH` with an IPv6 `address` field | **dead** for EHOSTUNREACH/EADDRNOTAVAIL (node puts the address in a structured field, not libpq's parenthesized literal) | fires via new structured errno+address check (`legacyHasIPv6DialCause`) | | `connect: no route to host` → wrong-profile hint | `connect EHOSTUNREACH <ip>:<port>` | **dead** (node never emits "no route to host") | fires via `EHOSTUNREACH`, after the IPv6 branch, matching Go's branch order | | `Tenant or user not found` → wrong-profile hint | Supavisor server error text | fired | fires (unchanged) | `NODE_ENETUNREACH_PATTERN` now also tolerates a closing paren after the port, since the new message format parenthesizes the driver cause. ### CLI-1942 guard (no accidental divergence) Go has **no** suggestion branch for the session-pooler EOF drop (`unexpected EOF`); node-postgres' equivalent is `Connection terminated unexpectedly`. Tests pin that this shape stays unclassified (generic `--debug` fallback) and that the message still carries the full `host=… user=…` identity plus the cause verbatim. ## Residual driver-text differences (impossible to byte-match) The inner cause text comes from the driver, so exact byte parity with Go is impossible where the drivers word things differently — the **structure** (`failed to connect to postgres:` prefix + `host=… user=… database=…` + cause) and the stage labels match: - Dial errors: node `connect ECONNREFUSED 1.2.3.4:5432` vs Go `dial tcp 1.2.3.4:5432: connect: connection refused`. - Wrong password over SCRAM: pgconn labels by auth phase (`failed SASL auth (…)`), which node-postgres does not expose, so TS renders `server error (…)` — the inner `FATAL: password authentication failed for user "postgres" (SQLSTATE 28P01)` bytes and the fired hint are identical. - Unrecognized causes (e.g. the CLI-1942 EOF, connect timeout) render verbatim with no stage word, where pgconn would say `failed to receive message (unexpected EOF)` — no stage is guessed rather than fabricating a wrong one. - Server-side failures (`server error (SEVERITY: message (SQLSTATE code))`) are byte-identical to Go given the same server bytes. ## Review findings deliberately left open Five-perspective review (architect / engineer / security / DX / go-parity-auditor) all approved. Non-blocking items noted for follow-up rather than churned here: - Consolidating the module's four error-chain traversals (collector / deepest-path / two any-match predicates) into one walk primitive — they have genuinely different semantics and predate this PR; cross-referencing doc comments were added instead. - Pre-existing, negligible divergence: an IPv4 `ENETUNREACH` gets no suggestion in TS while Go's over-broad `network is unreachable` text match would show the IPv6 hint even for IPv4 — the TS IPv6-literal gate is deliberate and pre-existing. - Pre-existing: `password authentication failed` as a wrong-password disjunct would also classify a theoretical non-SCRAM 28P01 that Go leaves unclassified; Supabase auth is always SCRAM. Related: this PR does not touch `output.layer.ts` (PR supabase#5946 / CLI-1973 territory) — the change is confined to the SqlError mapping and the classifier module. Fixes [CLI-1976](https://linear.app/supabase/issue/CLI-1976/db-connection-failure-errors-lose-all-detail-connect-suggestions-may)
pull Bot
pushed a commit
to oogalieboogalie/cli
that referenced
this pull request
Jul 27, 2026
…ly across confirmation prompts (supabase#5947) Routes every legacy-parity confirmation through the single Go-faithful helper `legacyPromptYesNo` (Go's `PromptYesNo`, `apps/cli-go/internal/utils/console.go:38-107`) and deletes the per-command shortcuts, so all three Go prompt properties hold everywhere: 1. **`--yes` OR `SUPABASE_YES` auto-confirms** (viper `AutomaticEnv`; project `.env` consulted exactly where Go's `loadNestedEnv` runs before the prompt). 2. **Auto-confirm echoes `<label> [y/N] y` to stderr** (`console.go:70-72`) instead of being silent. 3. **Non-TTY runs read piped stdin**: the label is printed, one line is scanned (100 ms), a parsed `y`/`n` wins, and only empty/unparseable input falls back to the default (`console.go:38-61,96-102`). Fixes CLI-1974 — https://linear.app/supabase/issue/CLI-1974/honor-yessupabase-yes-and-go-prompt-semantics-consistently-across ## ⚖ Flagged decision: item 3 (non-TTY piped stdin) is IMPLEMENTED The issue left it open whether to match Go's non-TTY piped-stdin reads; this PR takes the recommended option and implements them. Concretely, commands that previously hardcoded a non-TTY default now consume **one stdin line per confirmation** and honor it: - `echo y | supabase projects delete <ref>` now **deletes** (previously silently cancelled — verified against develop with a from-source differential smoke test). - `echo n | supabase secrets unset` now **declines** (previously proceeded unconditionally). - `echo n | supabase branches create` (git-branch auto-name) now cancels; empty stdin keeps Go's Yes default. - Scripts that pipe unrelated data into these commands should redirect `< /dev/null` or pass `--yes`. Veto here if this should be documented-divergence instead — the infrastructure (`Stdin.readLine`) was already in place from db push/reset, so no architectural cost either way. ## Per-site changes | Site | Go ref (default) | Gaps fixed | |---|---|---| | `logout` | `logout.go:16` (N) | `--yes` short-circuited before the stderr echo | | `projects delete` | `delete.go:22` (N) | flag-only yes (no `SUPABASE_YES`); non-TTY hardcoded cancel, no piped read, no label echo; title now aqua like Go | | `branches create` (auto-name) | `create.go:21` (Y) | `--yes`/env ignored entirely (blocked a TTY); no piped read; no echo; title now aqua | | `secrets unset` | `unset.go:34` (Y) | non-TTY hardcoded confirm, no piped read (env-aware yes + echo already existed) | | `bootstrap` overwrite | `bootstrap.go:48` (Y) | no echo under `--yes`; non-TTY went through clack | | `functions deploy --prune` | `deploy.go:190` (N) | legacy passed flag-only yes; no echo; no piped read; label now byte-matches `confirmPruneAll` (` • ` + bold + trailing blank line, both shells) | | `init -i` IDE prompts | `init.go:63,68` (Y/N) | prompted anyway under `--yes`; `--yes` now also lifts the stdout-TTY gate (Go only requires stdin TTY) | | `functions new` IDE prompts | `new.go:99` → `init.go:62` | flag-only yes; non-TTY hardcoded default, no piped read | | `db schema declarative generate`/`sync`/smart-target | `db_schema_declarative.go:269,321,381` + `declarative.go:234` (N/N/Y/N) | flag-only yes (Go reads viper YES after `loadNestedEnv` → now `legacyResolveYesWithProjectEnv`); no echo under yes. The sync apply-decision (`:452-460`) and post-fail reset gate (`:478`) were already faithful 1:1 ports of Go's own short-circuits and are unchanged | Already-correct sites (unchanged): config push, db pull, db push ×3, db reset, gen signing-key, storage rm, seed buckets ×3 (via the helper), migration fetch/down/repair (via `legacyMigrationConfirm`). ## Mechanics - `legacyPromptYesNo` + `legacyParseYesNo` moved from `src/legacy/shared/` to `src/shared/legacy/` (same home as `legacyResolveYes` in `global-flags.ts`) because two shared files (`shared/functions/deploy.ts`, `shared/init/project-init.ts`) now confirm through it. Semantics unchanged; importers updated path-only. - `ProjectInitOptions` gains `yes` (legacy init passes `legacyResolveYes`; next init passes `false` — next exposes no `--yes` on init, so next behavior is unchanged; bootstrap passes its resolved yes with `interactive: false`). - `stdinLayer` added to the layer composition of every command whose confirmation can now read stdin; verified in the real composition by from-source smoke tests (piped y/n/`--yes`/`SUPABASE_YES` differentials vs develop). - `migration.prompt.ts` now reuses the shared `legacyParseYesNo` and both helpers cross-reference why they intentionally diverge (see below). ## Judgement calls deliberately left open (reviewer input welcome) - **Machine mode (`--output-format json|stream-json`) without `--yes`**: `legacyPromptYesNo` keeps its pre-existing contract — never prompt, silently take the call site's Go default. Routing more sites through it changes a few narrow machine-mode cells: `secrets unset` json+TTY now proceeds (default Y) instead of cancelling; `bootstrap` overwrite json now proceeds instead of failing with a prompt error; `declarative generate --local` json with existing files now skips (exit 0) instead of erroring. Go has no json mode; its script-mode analog (non-TTY) is exactly this default-through behavior, and this matches the helper's 7 pre-existing users (config push, db pull/push/reset, storage rm, signing-key, seed buckets). `logout` deliberately keeps its documented fail-loud json contract. A stricter "fail loudly in machine mode when the default is destructive" rule was proposed in internal review — it would change already-shipped behavior of the pre-existing helper users, so it is left as a follow-up decision. New regression test locks the `secrets unset` json contract either way. - **`legacyMigrationConfirm` not consolidated**: the migration family's helper intentionally prompts regardless of output format and uses a raw TTY read (both arguably *more* Go-faithful); merging the two requires deciding the machine-mode question above first. Both files now carry cross-referencing comments so the divergence is explicit. - **`shared/functions/deploy.ts` → `legacy/shared/legacy-colors.ts` import** (for the bold prune slugs) deepens a pre-existing shared→legacy edge in that file (`spawnContainerCli`, `legacy-docker-registry` already do this); hoisting the color primitives is left as a cleanup follow-up. - **Next-shell visible change**: next `functions deploy --prune` inherits Go's exact prompt bytes (` • ` bullets, bold slugs, stderr echo under `--yes`, piped-stdin reads) via the shared file — deliberate, covered by new next-shell tests. ## Expected overlap with supabase#5946 supabase#5946 (CLI-1973, open) replaces `"context canceled"` literals with `CONTEXT_CANCELED_MESSAGE` and suppresses the `--debug` hint on declined prompts, touching many of the same handlers (logout, projects delete, secrets unset, branches create, bootstrap, db push/reset, migration \*, signing-key, `shared/functions/deploy.ts`, `shared/output/*`). This branch is based on develop without it and avoids rewriting those exact lines; expect small, mechanical merge overlap in import blocks and around the cancellation branches whichever lands second.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
When a confirmation prompt is declined, the Go CLI prints only a red
context canceledon stderr and exits 1 —recoverAndExit(apps/cli-go/cmd/root.go:287-303) explicitly skips theSuggestDebugFlagfallback forerrors.Is(err, context.Canceled). The TS textOutput.failrenderer appended the fallback hint for every suggestion-less error, including cancellations, so every prompting command (logout,db push(3 prompts),db reset,migration fetch/repair/down,functions deploy --prune,gen signing-key,secrets unset,projects delete,branches create,bootstrap) printed:This PR mirrors Go's guard at the render boundary:
CONTEXT_CANCELED_MESSAGEinapps/cli/src/shared/output/errors.ts— the byte-for-byte render of Go'scontext.Canceled.fail(apps/cli/src/shared/output/output.layer.ts) skips the debug-hint fallback when the normalized message equals the sentinel. The explicit-suggestion branch stays first, matching Go printing a pre-setutils.CmdSuggestioneven for canceled errors.shared/functions/deploy.ts) now reference the constant instead of an inline literal, so the sentinel is single-sourced like Go's.LEGACY_LOGOUT_CANCELLED_MESSAGEis folded into the shared constant, and the documented intent inlogout.errors.ts(which already described the Go behavior this PR implements) now matches reality.Why the fix is shared rather than legacy-scoped
The failure-rendering seam (
shared/cli/run.tshandledProgram→Output.fail) is shell-agnostic, andnext/genuinely shares a cancellation producer:shared/functions/deploy.ts(used bynext functions deploy --prune) fails with the samecontext canceledmessage on a declined prune prompt. Suppressing a troubleshooting hint for a user-declined prompt is the correct behavior in both shells, so the fix lives in the shared layer instead of forking the renderer per shell. Aside from that,next/handlers treat declines gracefully (nocontext cancelederror), so next-shell text rendering is otherwise unchanged. This is deliberate and reviewed against the CLI-1546 invariant (no spinner/stream routing was touched).Tests
output.layer.unit.test.ts): byte assertions that the sentinel message renders as exactly the redcontext canceledline with no hint (fails without the fix), and that an explicit caller suggestion still prints for a cancellation (Go ordering).context canceled, and no debug-hint line:logout(new),migration fetch(extended),db reset(new file — Docker-free, the destructive prompt fires before any connection is dialed),gen signing-key(extended).run.e2e.test.tsasserts stderr ends with the hint for an unrecognized flag).stripAnsihoisted totests/helpers/cli.tsfor the files this PR touches.Deliberately-open judgement calls
errors.Is: the TS check is message equality, narrower than Go's chain-walkingerrors.Is. A future producer surfacing a wrapped cancellation ("...: context canceled") throughOutput.failwould keep the hint where Go suppresses it. No such producer exists today (mid-flight Ctrl-C takes the interrupt/exit-130 path and never reachesOutput.fail); the invariant is documented on the constant. Reviewers (architect/engineer/parity passes) agreed a tag-registry or normalized-error flag isn't warranted for a single Go sentinel.Operation cancelled./exit 130 vs Gocontext canceled/exit 1; (b) the hint's--debugdetection usesprocess.argvvs Go'sviper.GetBool("DEBUG")(which also honorsSUPABASE_DEBUG) — affects only the non-canceled branch.Fixes CLI-1973
https://linear.app/supabase/issue/CLI-1973/declined-confirmation-prompts-print-a-spurious-debug-hint-context