fix(cli): use cobra's mutual-exclusivity error template for sso update#5832
Conversation
sso update's --domains/--add-domains/--remove-domains checks emitted custom messages gated on `.length > 0` truthiness rather than cobra's real pflag.Changed semantics, and didn't match cobra's exact error text. Reuse the hasExplicitLongFlag/cobraMutuallyExclusiveErrorMessage helpers (hoisted in #5804 for functions deploy/download) so all three of sso update's mutex groups -- including --metadata-file/--metadata-url, found to have the same divergence during review -- emit cobra's byte-exact template, and reorder the checks ahead of the provider-ID validation to match cobra's ValidateFlagGroups-before-RunE precedence (cmd/sso.go:90-91, 178-180). Fixes CLI-1902
|
@codex review |
![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: 8612439520
ℹ️ 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".
…review: #5832) pflag's parseLongArg (flag.go:1013-1031) unconditionally consumes the next argv token as a value-taking long flag's value, even if that token looks like another flag — so `--metadata-file --metadata-url` gives cobra metadata-file.Changed=true (value "--metadata-url") and never parses metadata-url as its own flag. hasExplicitLongFlag scanned each flag name independently and had no notion of a token being "already consumed", so it misreported both as explicitly set and raised a mutex error Go never would. Add hasExplicitValueFlag, which knows which of sso update's flags take a value and skips the token a bare occurrence consumes before continuing the scan.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@ebf1ec0afb0b8ee97630e6df3ebb67c917abc4c5Preview package for commit |
Coly010
deleted the
columferry/cli-1902-sso-update-use-cobras-mutual-exclusivity-error-template-for
branch
2 participants
What kind of change does this PR introduce?
Bug fix (Go-parity divergence).
What is the current behavior?
sso update's--domains/--add-domains/--remove-domainsmutex checks emitted custom, hand-written messages ("only one of --domains or --add-domains may be set") instead of cobra's realvalidateExclusiveFlagGroupstemplate (flag_groups.go:204), and gated "is this flag set" on the parsed value's.length > 0rather than cobra's actualpflag.Changedsemantics — so--domains=(explicit but empty) was silently treated as unset, the same "changed vs truthy" class of bug CLI-1860 fixed forfunctions download's--use-docker.Fixes CLI-1902.
What is the new behavior?
Reuses the
hasExplicitLongFlag/cobraMutuallyExclusiveErrorMessagehelpers already hoisted toshared/cli/cobra-flag-groups.ts(#5804) sosso updateemits cobra's byte-exact error text for--domains/--add-domains/--remove-domains, keyed off raw argv rather than the parsed flag value.While reviewing, three independent reviewer passes (architect/engineer/DX) converged on two more in-scope gaps in the same handler that were cheap to close alongside the ticket's stated fix:
--metadata-file/--metadata-urlhad the identical divergence (Go also registers this pair withMarkFlagsMutuallyExclusive,cmd/sso.go:178) — folded into the same cobra-parity check rather than leaving one mutex group byte-exact and the other hand-written in the same command.ValidateFlagGroupsbeforeRunE(command.go:1010,1014) while Go's UUID check lives insideRunE(cmd/sso.go:90-91) — so a flag-group violation must win over an invalid provider ID when both apply. Reordered to match.Added regression tests for the exact cobra error text (both
--domainsgroups and the metadata group), the changed-vs-truthy gap, the two-groups-not-a-3-way-group behavior, the group-check ordering when all three domain flags collide, and the mutex-before-UUID precedence — plus a flag-parser unit test proving--domains=really resolves to[].update.handler.ts's pre-existing branch-coverage gaps (GET/PUT network-failure paths,--skip-url-validation, no-access-token, malformed GET body) are unrelated to this change and were left alone; independently verified viagit stashthat none of them were introduced by this diff.