fix(cli): migrate remaining StringSlice flags onto legacyStringSliceFlag builder (CLI-2005) - #6010
Conversation
…lag builder (CLI-2005) All seven remaining hand-rolled pflag StringSliceVar call sites (sso add --domains; sso update --domains/--add-domains/--remove-domains; postgres-config update/delete --config; start --exclude/-x; status --override-name/--exclude) now route through the shared legacyStringSliceFlag builder, so their malformed-CSV stderr byte-matches Go pflag's 'invalid argument %q for %q flag: ...' diagnostic. The builder gains an optional alias parameter for start's StringSliceVarP shorthand, which pflag frames as '-x, --exclude'. Every rendered line and the multiline/blank-only semantics were verified against the real Go binary. Fixes CLI-2005
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
…5-migrate-remaining-pflag-stringslice-call-sites-onto
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@55a06c905f841f47172bfe70748a9080e0f1f3abPreview package for commit |
kanadgupta
left a comment
There was a problem hiding this comment.
Two minor cleanup suggestions per Claude otherwise LGTM
…unset-default coverage (CLI-2005) status.command.ts's slice flags now go through legacyStringSliceFlag (whose names the completeness scan already can't see), so the exclusion entry was a no-op that only risked hiding future direct value-flag declarations there. Also adds the "defaults to an empty array when unset" test for sso update's three domain flags, matching the coverage already present for sso add, start, and status.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b9dd061df
ℹ️ 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".
…5-migrate-remaining-pflag-stringslice-call-sites-onto # Conflicts: # apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/sso/add/add.command.ts # apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/sso/update/update.command.ts # apps/cli/src/legacy/commands/start/SIDE_EFFECTS.md
Merging HEAD's legacyStringSliceFlag domains-flag definitions with develop's auto-merged LEGACY_SSO_NAME_ID_FORMATS hoisting left an orphaned local NAME_ID_FORMATS const in sso add/update; drop it. Also reflow the SIDE_EFFECTS.md exit-code tables merged in the previous commit to satisfy oxfmt.
…lag tests (CLI-2005) Command.withGlobalFlags only excludes each global flag's context requirement from the R accumulated on the command SO FAR; applying it before withSubcommands means it never sees (and can't discharge) the GlobalFlag context the subcommand's own handler chain reads. Reorder withSubcommands before withGlobalFlags in both files so the exclusion covers the whole tree, closing R to never without an `as` cast.
What changed
Follow-up to CLI-1983 (#5975), from kanadgupta's review: the seven remaining hand-rolled pflag
StringSliceVarcall sites still mapped malformed-CSV failures to a bareerr.message, so their stderr missed pflag'sinvalid argument %q for %q flag: ...framing. All of them now route through the sharedlegacyStringSliceFlagbuilder (src/legacy/shared/legacy-string-slice-flag.ts):sso add --domainssso update --domains/--add-domains/--remove-domainspostgres-config update --configpostgres-config delete --configstart --exclude/-xstatus --override-name/--excludeThe builder gains an optional
{ alias }parameter becausestart --excludeis the one site whose Go counterpart is aStringSliceVarPwith a shorthand (cmd/start.go:58): pflag frames such diagnostics with both spellings —invalid argument %q for "-x, --exclude" flag: ...(pflag v1.0.10errors.go:108-117branches onflag.Shorthand) — regardless of which spelling the user typed, so the alias has to be registered inside the builder for the framing to come out right.Flag.withDefault([] as ReadonlyArray<string>)was dropped from the migrated flag definitions:Flag.atLeast(0)already yields[]when the flag is unset (covered by the existing "defaults to an empty array when unset" unit tests), and--helpoutput was verified byte-identical before/after for all six commands.Per-site Go parity verification
Every rendered line was verified against the Go binary built from
apps/cli-go(pflag v1.0.10 →encoding/csv). All seven sites' malformed-CSV stderr changes user-visibly — from the bare parse-error text to the full pflag line:sso add --domains"--domains"invalid argument "a\"b" for "--domains" flag: parse error on line 1, column 2: bare " in non-quoted-fieldsso update --domains"--domains"sso update --add-domains"--add-domains"invalid argument "\"x" for "--add-domains" flag: parse error on line 1, column 3: extraneous or missing " in quoted-fieldsso update --remove-domains"--remove-domains"--add-domainspostgres-config update --config"--config"invalid argument "a\"b" for "--config" flag: parse error on line 1, column 2: bare " in non-quoted-fieldpostgres-config delete --config"--config"invalid argument "\"max_connections" for "--config" flag: parse error on line 1, column 17: extraneous or missing " in quoted-fieldstart --exclude/-x"-x, --exclude"invalid argument "a\"b" for "-x, --exclude" flag: parse error on line 1, column 2: bare " in non-quoted-fieldstatus --override-name"--override-name"invalid argument "\"api.url=FOO" for "--override-name" flag: parse error on line 1, column 13: extraneous or missing " in quoted-fieldstatus --exclude"--exclude"invalid argument "a\"b" for "--exclude" flag: parse error on line 1, column 2: bare " in non-quoted-fieldFor
postgres-config, the parse error also correctly precedes the--experimentalgate (cobra parses flags beforePersistentPreRunE), asserted in the experimental-gate integration suite.Multiline / blank-line semantics findings
CLI-1983's parser rewrite changed
legacyParseStringSliceFlagitself, so all seven sibling sites silently inherited the first-record-only / EOF-on-blank semantics. I verified each site against the Go binary:--<flag> $'a\nb"c'raises no parse error in Go at any of the seven sites (pflag callscsv.Reader.Read()once; the malformed second line is silently dropped). Observable proof forstart:start -x $'a\nb"c'warnsThe following container names are not valid to exclude: a— only the first record survives. TS matches.--<flag> $'\n'fails in Go withinvalid argument "\n" for "--<flag>" flag: EOFat every site (with the-x, --excludeframing onstart). TS matches.Exit.isFailurewithout the message. This PR adds exact-message assertions per flag plus first-record-only and blank-only-EOF vectors per site.Test coverage added
start), first-record-only multiline vectors, blank-only EOF vectors.Command.runWith) and asserting the exact rendered message vianormalizeCause, mirroring the network-bans/network-restrictions prior art from CLI-1983: newsso.string-slice-flags.integration.test.ts,start.string-slice-flags.integration.test.ts,status.string-slice-flags.integration.test.ts, plus malformed-CSV cases in the existingpostgres-config.experimental-gate.integration.test.ts.start's--excludeflag is hoisted to an exportedlegacyStartExcludeFlag(mirroringstatus/ssoconventions) so it is unit-testable.csvStringSliceFlaghelper inlegacy-db-target-flags.ts/.unit.test.tswere updated; all helper-built flag names remain hand-registered inVALUE_CONSUMING_LONG_FLAGS, so telemetry argv parsing is unaffected.Overlap note: PR #5974
Open PR #5974 (
columferry/cli-1982-...) touches sso command files (sso.pflag-reconcile.ts, add/update handlers). This PR's sso changes are deliberately minimal — the flag definition blocks inadd.command.ts/update.command.ts, their unit tests, one new family-level integration test file, and one SIDE_EFFECTS.md row. Whoever merges second should re-verify the sso flag definitions still route throughlegacyStringSliceFlagafter conflict resolution.Review notes (deliberately left open)
--schemaslice-flag family (gen types,db lint/dump/pull/diff,db schema declarative generate) still uses the hand-rolledFlag.mapTryCatch(legacyParseSchemaFlags, err => err.message)pattern vialegacy-schema-flags.ts. It is not in CLI-2005's scope (and several of those areStringSliceVarPwith-sshorthands needing their own per-site Go verification) — candidate for a follow-up issue.-o badplus malformed CSV in one invocation): TS surfaces the CSV parse error while Go's winner depends on argv order; both exit non-zero. Same accepted approximation as CLI-1983, already documented on the network-bans/network-restrictions flag comments.cli-go:lint:checkfails with 5 pre-existing gosec findings unrelated to this change (no Go files touched).Fixes CLI-2005