fix(cli): use cobra mutual-exclusivity template in sso add - #5974
fix(cli): use cobra mutual-exclusivity template in sso add#5974Coly010 wants to merge 18 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb94ec7ee8
ℹ️ 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: PR #5974) The raw-argv mutex scan and the Effect parser could disagree on which flags pflag would consider set: pflag consumes flag-shaped tokens as values unconditionally, the Effect parser does not. The scan matched pflag for the mutex check, but the handler then acted on the parsed Options — so 'sso add --type saml --project-ref --metadata-file x.xml --metadata-url u' suppressed the mutex yet read the metadata file and POSTed metadata_xml, an API call the Go CLI never makes (Go fails AssertProjectRefIsValid on the consumed value '--metadata-file' in PersistentPreRunE, cmd/root.go:113, before ValidateFlagGroups). Replace the per-flag hasExplicitValueFlag scan with a single pflag-faithful pflagLongFlagOccurrences pass that also yields each flag's effective value(s), and reconcile everything both sso handlers act on (project-ref, metadata-file, metadata-url, attribute-mapping-file, the domain slices, and set-ness of name-id-format/skip-url-validation) to those values. sso update shares the same gap and gets the same fix.
|
🤖 pr-autopilot needs a human here: this branch now conflicts with |
…2-sso-add-mutual-exclusivity-error-is-not-cobra-format # Conflicts: # apps/cli/src/legacy/commands/sso/update/update.handler.ts
|
Merged origin/develop (887811b). The one conflict was the domains block in |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 887811b4e1
ℹ️ 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".
…ective argv in sso (review: PR #5974) The pflag reconciliation kept handlers from acting on values pflag never set, but two cobra validations that run before RunE were still invisible to it (Codex review on 887811b): - 'sso add --domains --type saml': pflag hands --type to --domains, so Go fails ValidateRequiredFlags (command.go:1007, required "type" per cmd/sso.go:165) and never POSTs — the TS handler POSTed with domains ["--type"]. - 'sso update --domains --metadata-url u <id>': the consumed token orphans u into the positionals, so Go fails ValidateArgs/ExactArgs(1) (command.go:968, cmd/sso.go:87) before any hook — the TS handler performed the GET and PUT. Evolve the raw-argv scan (pflagLongFlagOccurrences -> pflagArgvScan) into a fuller pflag walk over the closed set of flags the parser can let through: root persistent value flags (cmd/root.go:324-333, incl. the TS-only --output-format), value shorthands (-t/-o per parseSingleShortArg, flag.go:1080-1094), pflag-effective positionals, and consumed long-flag tracking. sso add emulates the required-type check ahead of the mutex block (required beats groups); sso update emulates ExactArgs(1) ahead of the mutex and UUID checks (arity beats both) — binary-verified orderings. Naive per-thread fixes would have regressed '-t saml' and '-o json <id>' / '--workdir . <id>'; the update test that expected ref validation for '--project-ref --metadata-file x.xml ...' now expects the arity error Go actually raises there (x.xml is positional to pflag).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25df67c562
ℹ️ 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".
…es, and interspersed flags (review: PR #5974) Three narrow gaps in the pflag-faithful argv scan, all binary-verified against apps/cli-go (Codex review round 3 on 25df67c): - 'sso add --domains -t saml': pflag hands the -t token to --domains, so type is never Changed and Go fails ValidateRequiredFlags — the TS handler POSTed type "saml" with domains ["-t"]. The scan's consumed-token tracking now maps shorthand tokens (-t, -t=saml, -tsaml) to their canonical long names (consumedLongFlagNames -> consumedFlagNames). - 'sso update <id> --domains': pflag fails ParseFlags (cobra command.go:919) with 'flag needs an argument: --domains' (pflag errors.go:75,78 for the 't' in -t shorthand form) before ValidateArgs, every hook, and RunE — the TS handlers performed the GET and PUT / the POST. The scan now reports missingValueError instead of inventing an empty occurrence, and both handlers reject it first (parse beats arity: 'sso update a b --domains' names the missing argument, not the arg count). - 'sso --profile foo update --domains --metadata-url u <id>': cobra's Find/stripFlags routes through persistent flags between path segments and Go still raises 'accepts 1 arg(s), received 2', but the contiguous anchor failed and the TS handler skipped the arity re-count and proceeded. Anchoring now walks argv matching segments while stepping over flag tokens and their consumed values. (The reviewer's original '--project-ref' example cannot reach the handler — the Effect parser rejects leaf flags at the group level — but root globals like --profile/-o reproduce it.) Known shared residual, unchanged from the round-2 arity emulation: Go prints a usage block for errors raised before PersistentPreRunE sets SilenceUsage (cmd/root.go:97), i.e. parse and arity errors; the TS handler-level emulations render the single error line + --debug suggestion only.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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@5e0a544a53ce55f3c0b1386199b5ad9515848df9Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2de9121e1
ℹ️ 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".
…tics in sso (review: PR #5974) The round-2 value reconciliation covered sso's string and slice flags but left --skip-url-validation, --name-id-format, and --type on the Effect- parsed values gated on set-ness. Those values can disagree with pflag in two ways, both binary-verified against apps/cli-go (Codex review round 4 on e2de912): - Repeated flags: the Effect parser resolves first-wins, pflag Sets every occurrence so the last wins. 'sso update <id> --skip-url-validation --skip-url-validation=false --metadata-url http://x' skips URL validation in TS but validates (and rejects non-HTTPS) in Go; the mirror argv diverges the other way, and '--domains' consuming one of two --name-id-format occurrences flips which URN lands in the PUT body. - Set-rejected values: pflag aborts ParseFlags on any occurrence its Value.Set rejects, before every hook, validation, and request. The Effect parser accepts yes/no booleans (strconv.ParseBool does not) and never validates the repeats it discards, so '--type saml --type bogus' and '--skip-url-validation=yes' reached the handlers and called the API where Go exits 1 with 'invalid argument ...'. The scan already records every non-consumed occurrence with its raw value, so the fix stays at the established handler-emulation level: two new helpers in sso.pflag-reconcile.ts re-run pflag's per-occurrence Set over the scan output (ParseBool set; enum membership; byte-exact pflag error messages incl. the '-t, --type' shorthand label) and yield the last-occurrence effective values the handlers now act on. Checks sit ahead of the missing-value emulation, matching pflag's sequential walk (an occurrence always precedes a trailing bare flag; binary-verified '--skip-url-validation=yes --domains' names the invalid argument). The shared NAME_ID_FORMATS list is hoisted to sso.saml.ts per the family-root rule. Known residuals unchanged: parse errors raised before Go sets SilenceUsage print cobra's usage block in Go only, and boolean literals ParseBool accepts but the Effect parser rejects (t/T/TRUE/...) still fail at the TS parse layer with the parser's own error.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07b145b174
ℹ️ 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".
…2-sso-add-mutual-exclusivity-error-is-not-cobra-format
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e21b4aafc
ℹ️ 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: codex) pflag records NoOptDefVal "true" for a bare boolean occurrence and the literal empty string for --flag= (flag.go:1013-1019), then hands "" to strconv.ParseBool, which rejects it. The argv scan conflated the two, so --skip-url-validation=false --skip-url-validation= slipped past the handler as true and issued requests the Go CLI never makes (binary-verified: Go aborts ParseFlags with 'invalid argument "" for "--skip-url-validation" flag' before any request). Record the bare form as "true" in the scan so the bool reconciler fails inline-empty values exactly like Go.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fb04b67b7
ℹ️ 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".
… sso add/update (review: codex)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 050108e854
ℹ️ 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".
…so add/update (review: codex)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57e608cce1
ℹ️ 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".
…ike Go in sso add/update (review: codex) - an undecodable 200 body from the reconciled GET aborts with Go's failed-to-get error before any PUT (update.go:42-45); a 200 without a JSON content type falls into Go's nil-JSON200 gate branch - the reconciled raw GET stitches identity through the shared per-command guard, like Go's identityTransport on every response - the upgrade-gate fallback GETs and the linked-project cache fill target the reconciled profile host (Go's CurrentProfile is process-wide)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22f27acf90
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b9e88f1e1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cc1bcc75a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4d15a0045
ℹ️ 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".
…2-sso-add-mutual-exclusivity-error-is-not-cobra-format
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbc2bba60c
ℹ️ 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".
kanadgupta
left a comment
There was a problem hiding this comment.
One refactoring suggestion and one stale comment suggestion per Claude otherwise LGTM
| readonly name: string; | ||
| } | ||
|
|
||
| export function legacySsoLoadProfile( |
There was a problem hiding this comment.
Scope: this file — and the wider profile-reconciliation stack it anchors (legacySsoResolvePflagProfile, legacyAccessTokenForProfile in legacy-credentials.layer.ts, the apiUrl overrides threaded through legacy-upgrade-suggest.ts and legacy-linked-project-cache.*, and legacy-profile.ts/legacy-cli-config.layer.ts changes) — is only reachable when pflag and the Effect parser disagree about --profile (consumed/flag-shaped tokens, repeated flags, untrimmed profile file). None of it is needed for CLI-1982's mutex-template fix, and it re-implements viper + mapstructure + go-playground/validator error text (down to lipgloss padding) per command family, hand-maintained.
Two concerns with keeping it in this PR:
- The Effect-parser-vs-pflag divergence is CLI-wide, not sso-specific. Emulating it inside two handlers doesn't scale to the next command with the same bug class; the durable fix lives in the shared parser/run layer (or a tree-derived scan), which deserves its own ticket and design discussion rather than accreting through review rounds here.
- It is demonstrably not finished: four codex threads on this machinery are still unresolved — including the P1 about auxiliary requests (
legacySuggestUpgrade, linked-project cache) still authenticating with the config-layer profile's token against the reconciled profile's host, plus theregionsdecode gap, the reconciled-GET schema validation gap, and the stale dashboard URL. Merging a partially-plugged token-crossing surface is worse than not having the reconciliation at all, since develop today can't reach these hosts.
Suggestion: split commits 820f0e7..4b9e88f into a follow-up PR under a dedicated ticket (pflag/viper reconciliation for sso, or the parser-layer alternative), and land this PR at the bb94ec7 scope — the shared mutex template, Changed-semantics detection via the pre-existing hasExplicitValueFlag, and the exact-message tests — which fully resolves CLI-1982.
| // pflag fails `ParseFlags` (cobra `command.go:919`) when a bare | ||
| // value-taking flag is the final token (`sso add --type saml --domains`) | ||
| // — before every validation, hook, and `RunE`, so no POST is ever made. | ||
| // The Effect parser accepts that argv (the flag parses as unset), hence | ||
| // the emulation. Binary-verified against `apps/cli-go` (PR #5974 review | ||
| // round 3). Keep this the very first check. | ||
| if (scan.missingValueError !== undefined) { | ||
| return yield* Effect.fail( | ||
| new LegacySsoFlagNeedsArgumentError({ message: scan.missingValueError }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
"Keep this the very first check" is stale — this check is deliberately third, after the enum/bool value validations, per the block at lines 129-144 ("These checks precede the missing-value check because a missing value can only arise at the final argv token"). The two directives contradict each other, and in this file the ordering comments are the parity spec future edits will follow. update.handler.ts:264 has the correct phrasing for its twin ("Keep this ahead of the arity check"); update this one to match reality, e.g. "Keep this ahead of the profile/workdir/required-flag/mutex checks."
What changed
sso addemitted a hand-written mutual-exclusivity error (only one of --metadata-file or --metadata-url may be set) and detected the conflict viaOption.isSomeon parsed flag values. The Go CLI enforces this group via cobra'sMarkFlagsMutuallyExclusive("metadata-file", "metadata-url")(apps/cli-go/cmd/sso.go:164), whose error template is:sso updatewas already migrated to the sharedcobraMutuallyExclusiveErrorMessagehelper plus raw-argvpflag.Changed-semantics detection (CLI-1902);addwas never migrated. This PR mirrors update's pattern inadd.handler.ts:cobraMutuallyExclusiveErrorMessage(group in Go's registration order; the violating subset sorted, per cobra'svalidateExclusiveFlagGroups).hasExplicitValueFlagraw-argv scan, so an explicit empty value (--metadata-file= --metadata-url x) still trips the mutex, while a bare--metadata-file --metadata-url(pflag consuming the second token as the first flag's value) correctly does not.ValidateFlagGroups-before-RunEprecedence.Integration tests cover the exact-message case (byte-match), the explicit-empty
--metadata-file=case, the consumed-value non-violation case, and single-flag happy paths.SIDE_EFFECTS.mdnow documents the cobra template andChangedsemantics.Review findings deliberately left open
A four-perspective review pass (architect / engineer / security / DX) approved the change; these pre-existing, cross-cutting observations were noted rather than fixed here:
sso addandsso update— a family-root helper (e.g.sso.mutex.ts) is a reasonable follow-up.SSO_ADD_VALUE_FLAG_NAMES(like update's equivalent) is a hand-maintained mirror of the command's declared value flags with no compile-time sync guarantee.sso update.-tshorthand — documented limitation shared withsso update; pflag fails-t's enum validation before flag groups anyway.--domains=explicit-empty edge onadd(parity audit §3.10) is a separate issue and is not addressed here.Fixes CLI-1982
https://linear.app/supabase/issue/CLI-1982/sso-add-mutual-exclusivity-error-is-not-cobra-format