fix(cli): suppress vendored effect CLI's stdout help-dump and duplicate error on parse failures (CLI-1901)#5844
Conversation
…LI-1901) A required-flag/choice parse failure (missing required flag, invalid Flag.choice value, unrecognized flag, etc.) dumped the full help doc to stdout and printed the error twice: once from the vendored effect CLI library's own showHelp() catch-all in Command.runWith, once from this repo's own Go-parity renderer in normalize-error.ts/run.ts. Go cobra shows neither: PersistentPreRunE sets SilenceUsage=true before flag validation, so a required-flag failure is a single clean stderr line with nothing on stdout. withoutParseErrorHelpDump wraps Command.runWith's returned effect with a buffering Console.Console, replaying the buffered writes only when the run didn't fail with a genuine parse error (ShowHelp with a non-empty errors array) — success, --help, --version, and the bare group-command help dump are all unaffected. Since this also removes the vendored library's own duplicate render for error tags with no Go-parity-specific mapping in normalize-error.ts (UnrecognizedOption, DuplicateOption, MissingArgument, UnknownSubcommand, and non-doubled-prefix InvalidValue), extend the ShowHelp-envelope unwrap to fall back to the inner error's own message instead of the useless "Help requested" envelope text.
…CLI-1901) Go cobra only suppresses its usage block for a missing required flag (ValidateRequiredFlags, which runs after PersistentPreRunE sets SilenceUsage=true). Every other parse-error tag (unrecognized flag, invalid Flag.choice value, missing positional argument, unknown subcommand) is raised during ParseFlags/ValidateArgs, before SilenceUsage is set — Go still shows a usage block for those, always on stderr, never stdout. Verified directly against the built apps/cli-go/supabase-go binary. The previous version of this fix suppressed the help dump uniformly for any ShowHelp-with-errors failure, which incorrectly matched Go's behavior for missing-required-flag but diverged for every other tag (dropping discoverability Go actually provides). classifyParseErrorConsoleOutput now returns a three-way disposition (drop / flush-help-doc-to-stderr / flush-unchanged) that mirrors the real split, redirecting the buffered help doc to stderr instead of dropping it when Go would still show one. Also documents that Effect's default logger resolves through the same Console reference withoutParseErrorHelpDump overrides, with a regression test proving logger output during a successful run is still flushed, not lost.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@c1fbe34d5cd0fd8a575cfd136cd2d6f8cf64bee3Preview package for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd6d2de440
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
… split (ci: e2e shard 1/3, 3/3) CLI-1901 intentionally changed where parse-error usage/help output lands (stderr only, never stdout, with the vendored effect CLI's duplicate JSON render dropped). These two pre-existing e2e suites still asserted the old, buggy double-printed shape and started failing once the fix shipped; update them to assert the new, verified-against-Go behaviour instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36f8e3874e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…lback (review: preserve-parser-suggestions) withoutParseErrorHelpDump (CLI-1901) drops the vendored effect CLI's own duplicate error render, which was the only place buildSubcommandFlagHint's "Pass it after the subcommand" guidance reached the user. Thread the same CliErrorSuggestionContext run.ts already builds for the output formatter into normalizeCause's single-inner-error fallback so it can reuse formatCliErrorsForDisplay instead of the raw error message, restoring the hint (verified against a rebuilt binary: `branches --persistent create`).
…eview: preserve-usage-for-value-missing-flags) The vendored effect CLI library raises the same MissingOption tag whether a required flag was never given at all, or given with no value following it (e.g. `sso add --type` as the last token) — it has no distinct "value required" error. Go's pflag DOES distinguish these: a present-but-valueless flag is a ParseFlags-time error, raised before PersistentPreRunE sets SilenceUsage, so Go still prints its usage block for that input (verified against the real apps/cli-go/supabase-go binary). classifyParseErrorConsoleOutput now checks raw argv (isMissingFlagTokenPresent) to tell the two cases apart, so only a genuinely-absent flag drops the help dump.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 937e323c79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…ck to "Help requested" (review: handle-multi-error-showhelp-fallbacks) The single-inner-error ShowHelp fallback added earlier in this PR only handled errors.length === 1, so a real two-error ShowHelp (e.g. a child flag with a value placed before its subcommand, which Effect parses as both an UnrecognizedOption and an UnknownSubcommand) fell through to the generic "Help requested" envelope message once CLI-1901 suppressed the vendored library's own duplicate render. mappedError's ShowHelp case now reuses formatCliErrorsForDisplay for any non-empty errors array, not just a single one, so hints and per-error detail survive for N>1 too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11c6928c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…equired flags (review: recognize-aliases-required-flags) Go/pflag's parseSingleShortArg raises the same value-required parse error (and shows usage) for a present-but-valueless short alias as parseLongArg does for the long form. `isMissingFlagTokenPresent` only checked the canonical `--option` token, so `sso add -t` (missing its value) was misclassified as the flag being genuinely absent and its usage dropped - verified against the real apps/cli-go/supabase-go binary. Threads the existing CliErrorSuggestionContext (rootCommand + args) one level deeper into classifyParseErrorConsoleOutput/withoutParseErrorHelpDump and reuses subcommand-flag-suggestions.ts's command-tree walk (new flagAliasesFor) to resolve a flag's aliases before deciding "absent".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59b083ec6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…ification (review: ignore-terminator-tokens-missing-flags)
`isMissingFlagTokenPresent` did a raw scan of the whole argv to decide
whether a required flag was genuinely absent (Go: SilenceUsage-suppressed)
or present but missing its value (Go: usage still shown). Tokens after a
literal `--` are always positional, never a flag occurrence, so
`migration repair -- 20230101000000 --status` was misclassified as
"present but missing its value" and wrongly dumped a usage block Go never
shows for a truly-unset required flag. Mirrors the vendored effect lexer's
own `argv.indexOf("--")` cutoff.
…ffering them (review: pin-config-warnings-to-real-console)
`withoutParseErrorHelpDump` buffers all Console.log/error writes for the
whole command run (parser and handler alike) so it can classify and
redirect the vendored effect CLI library's own parse-failure renders.
`@supabase/config`'s loadProjectConfigFile writes its deprecated
[inbucket]/auth.external.{linkedin,slack} warnings through that same
ambient Console service, and is reachable from real command handlers via
ProjectConfigStore/loadProjectConfig — so those warnings were getting
captured and held until the whole run exited. For a long-running command
like functions serve, that meant the warning never appeared until
shutdown instead of at startup, unlike Go's synchronous
fmt.Fprintln(os.Stderr, ...). Both call sites now pin themselves to the
real console so they always flush immediately, regardless of what
Console.Console is ambient.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd333e1a84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
…lue (review: ignore-flag-tokens-consumed-as-other-option-values)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37fe8ffeda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…-flag scan (review: include-global-flags-and-consumed-terminator)
Two Codex follow-up findings on isMissingFlagTokenPresent's argv scan,
both verified against vendored pflag's parseLongArg/parseArgs (flag.go):
- isValueTakingFlagTokenFor only inspects the resolved leaf command's own
flags, so a value-taking GLOBAL flag (e.g. --network-id) consuming the
next token (migration repair --network-id --status ...) left the
required --status token looking "present" even though Go/pflag hands it
to --network-id as a value and suppresses usage for the genuinely-absent
status flag. Now OR globalFlagsWithValues into the value-taking-token
predicate at the classifyParseErrorConsoleOutput call site.
- The scan precomputed args.indexOf("--") as a hard terminator before
scanning for consumed values, but pflag only treats "--" as the
terminator when it's reached as a live token — a "--" immediately
consumed as a preceding value-taking flag's value (sso add
--project-ref -- --type) never reaches that check in Go, so parsing
resumes normally past it. Folded the terminator check into the same
left-to-right loop that already skips consumed-value tokens.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
avallete
left a comment
avallete
left a comment
There was a problem hiding this comment.
This looks like a lot of code just to circumvent Effect default behavior.
Could we fill an upstream issue see if that's something we could evolve (maybe just a way to skip the automatic help message) so we can clean that all up at some point ?
…around Filed Effect-TS/effect#6313 for runWith having no supported way to opt out of, or redirect, its own showHelp Console.log/Console.error render on a ShowHelp parse failure -- the reason withoutParseErrorHelpDump has to swap out the ambient Console service and buffer/classify writes from outside the library. Add a TODO so this workaround is easy to find and remove/simplify once upstream ships a fix.
Coly010
deleted the
columferry/cli-1901-legacy-cli-required-flagchoice-parse-errors-double-print-and
branch
2 participants
What kind of change does this PR introduce?
Bug fix (legacy shell shared CLI runtime).
What is the current behavior?
Fixes CLI-1901, originally surfaced as a side-finding during #5803's review (CLI-1859,
gen bearer-jwt --role required).Any legacy command whose flag parsing fails with a
CliError.ShowHelp-wrapped error carrying a non-emptyerrorsarray (missing required flag, invalidFlag.choicevalue, unrecognized flag, etc.) printed a broken, non-Go-parity error:Root cause: the vendored
effect@4.0.0-beta.93CLI library'sCommand.runWith(.repos/effect/packages/effect/src/unstable/cli/Command.ts) always dumps the full help doc viaConsole.logand, whenerrors.length > 0, alsoConsole.errors the same errors — before this repo's own Go-parity renderer (normalize-error.ts+run.ts'shandledProgram) renders its own line for the same failure. This breaks stdout-piping scripts (e.g.TOKEN=$(supabase gen bearer-jwt --role "$ROLE")with an empty$ROLE) and violates this repo's own Go-parity contract (apps/cli/CLAUDE.md).What is the new behavior?
run.tsgainswithoutParseErrorHelpDump, which wrapsCommand.runWith(rootCommand, ...)(args)with a bufferingConsole.Consoleand, once the run's outcome is known, disposes of the buffered writes perclassifyParseErrorConsoleOutput— a three-way split that mirrors Go cobra's actual behavior (verified directly against the builtapps/cli-go/supabase-gobinary, not just assumed):drop— a missing required flag (MissingOption). Go'sPersistentPreRunEsetsSilenceUsage = true(cmd/root.go:97) beforeValidateRequiredFlagsruns, so this is a single clean stderr line, nothing on stdout.flush-help-doc-to-stderr— every other genuine parse/validation failure (unrecognized flag, invalidFlag.choicevalue, missing positional argument, unknown subcommand). These are raised duringParseFlags/ValidateArgs, beforeSilenceUsageis set — Go still shows a usage block for these, always on stderr, never stdout. The library's help doc isn't byte-identical to cobra's shorter usage template (see judgement calls below), but it's now on the right stream with no duplicate.flush-unchanged— success,--help,--version,--completions, and the bare-group-command help dump (errors: [], exit 0) — all untouched.In every genuine-failure case, the library's own duplicate
Console.errorrender is dropped; this repo's ownhandledProgram+normalizeCauserender the single Go-parity line instead.normalize-error.ts'sShowHelp-envelope unwrap was also extended: since the library's duplicate render is now gone, a wrapped inner error with no Go-parity-specific mapping (UnrecognizedOption,DuplicateOption,MissingArgument,UnknownSubcommand, or a non-doubled-prefixInvalidValue) now falls back to that inner error's own.messageinstead of the useless generic "Help requested" envelope text — otherwise removing the duplicate would have silently regressed those tags from "informative, but printed twice" to "printed once, uselessly."Tests: pure-predicate unit tests for the three-way classifier (
run.unit.test.ts), integration tests exercising the real buffering/flush wiring againstlegacyBranchesCommandand a synthetic required-flag command via aConsole.Consoletest double (run.integration.test.ts— notvi.spyOn, which reliably breaks call detection when spyingconsole.logandconsole.errorin the same test under this repo's Bun+Vitest combination), and e2e tests proving the real subprocess stdout/stderr streams againstbranches --bogus-flagandsso add(missing/invalid--type, the issue's own named repro target).Judgement calls left open
required flag(s) "type" not setvs this repo'sError: required flag(s) "type" not setwith an extraError:prefix; Go'sinvalid argument "bogus" for "-t, --type" flag: must be one of [ saml ]vs this repo'sInvalid value for flag --type: "bogus". Expected "saml", got "bogus"). Both pre-date this fix and are unrelated to the stdout-pollution/duplicate-print bug it targets — flagged by review as a possible follow-up, not fixed here to keep this PR's diff focused.flush-help-doc-to-stderrclass, but its content is this library's own (longer) help doc, not cobra's shorter usage template. True byte-parity there would need a second, purpose-built "usage-only" formatter — out of scope for this fix.internal/command.ts:243misreportingFlag.optionalstatus in--output-format jsonhelp docs) is a separate, unrelated code path in the same vendored module — deliberately left unaddressed here to keep this PR scoped to the double-print/stdout-dump bug.