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@e27da21b7a562eaa6786212dd1b5018119f9b2fePreview package for commit |
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
Two minor cleanup suggestions per Claude otherwise LGTM
| // scan; their flag names are registered by hand in | ||
| // `VALUE_CONSUMING_LONG_FLAGS` instead. | ||
| const commandsDir = fileURLToPath(new URL("../commands", import.meta.url)); | ||
| const INDIRECT_NAME_FILES = new Set(["issue.command.ts", "status.command.ts"]); |
There was a problem hiding this comment.
Now that this PR deletes csvStringSliceFlag — the reason status.command.ts was on this list — this entry is stale. Both of status's slice flags go through legacyStringSliceFlag, whose names the scan can't see anyway, and the only remaining direct declaration there is a boolean (excluded from VALUE_FLAG_KINDS). Keeping the exclusion is worse than a no-op: it silently disables the completeness check for any future directly-declared value flag in status.command.ts. Since the surrounding comment was already updated to drop the csvStringSliceFlag reference, the entry itself should go too: new Set(["issue.command.ts"]).
| // Go declares all three domain flags with pflag's `StringSliceVar` | ||
| // (`cmd/sso.go:170-172`); malformed CSV fails at parse time with pflag's | ||
| // exact diagnostic (CLI-2005, see `legacyStringSliceFlag`). | ||
| export const legacySsoUpdateDomainsFlag = legacyStringSliceFlag( |
There was a problem hiding this comment.
Non-blocking: this PR drops Flag.withDefault([]) from all three sso-update flags, and the PR description says the unset→[] behavior is "covered by the existing 'defaults to an empty array when unset' unit tests" — but update.command.unit.test.ts is the one migrated site without that vector (it only has the explicit --domains= empty-value test; sso add, start, and status each have the unset test). The behavior is safe — Effect's Param.variadic with min: 0 returns [] for zero occurrences — but since every sibling site asserts it per-flag, adding the same one-liner here would keep the coverage matrix uniform.
2 participants
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