Uh oh!
There was an error while loading. Please reload this page.
fix(cli): run --experimental gate before storage's mutex check - #5768
Merged
Coly010 merged 3 commits intoJul 6, 2026
Merged
Conversation
…utex check Go's cobra runs PersistentPreRunE (the experimental gate) before ValidateFlagGroups (mutual-exclusivity checks). The 4 storage leaves (ls/cp/mv/rm) had this backwards, checking --linked/--local exclusivity before the --experimental gate, so `storage ls --linked --local` without --experimental surfaced the mutex error in TS instead of Go's gate error. Also corrects the inline comments in those files and the shared legacy-experimental-gate.ts doc, which claimed Go gates experimental "after flag-group validation" -- backwards per cobra's actual command.go execution order (PersistentPreRunE at :985, ValidateFlagGroups at :1010). Fixes the existing storage.e2e.test.ts assertion that codified the old (wrong) precedence, and adds an integration suite covering the corrected ordering for all four leaves.
Coly010
commented
Jul 2, 2026
ContributorAuthor
@codex review |
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Coly010
marked this pull request as ready for review
July 2, 2026 12:48
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@2394e7f065f7a163f37f0fcb12986090309075d1Preview package for commit |
Single-source the gate-before-mutex ordering rationale in each leaf's comment instead of repeating the full cobra citation four times, and assert on the concrete error class via Cause.findErrorOption/instanceof in the new integration suite instead of stringified-Cause substring matching, matching this repo's established pattern.
Coly010
commented
Jul 2, 2026
ContributorAuthor
@codex review |
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Coly010
commented
Jul 3, 2026
ContributorAuthor
@codex review |
Codex Review: Didn't find any major issues. Breezy! 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
approved these changes
Jul 6, 2026
github-merge-queueBot
removed this pull request from the merge queue due to a conflict with the base branch
Jul 6, 2026
…5-storage-lscpmvrm-run-experimental-gate-before-mutual # Conflicts: # apps/cli/src/legacy/shared/legacy-experimental-gate.ts
Uh oh!
There was an error while loading. Please reload this page.
Coly010
deleted the
columferry/cli-1855-storage-lscpmvrm-run-experimental-gate-before-mutual
branch
July 6, 2026 10:12
pullBot
pushed a commit
to oogalieboogalie/cli
that referenced
this pull request
Jul 8, 2026
…chema declarative (supabase#5828) ## What changed `db schema declarative generate`/`sync` ran their mutual-exclusivity flag check (`db-url`/`linked`/`local`, `apply`/`no-apply`) BEFORE the pg-delta/`--experimental` gate (`legacyRequirePgDelta`). Go's cobra runs `PersistentPreRunE` (the gate) before `ValidateFlagGroups` (mutex check) — confirmed against `apps/cli-go/cmd/db_schema_declarative.go` and the actual `cobra@v1.10.2` source — so invoking either command with conflicting flags and no `--experimental` surfaced the wrong error in the TS shell vs Go. Same bug class already fixed for `storage ls/cp/mv/rm` in CLI-1855 (supabase#5768); this mirrors that precedent as closely as the code structure allows. Declarative's gate needs a config read (`legacyReadDbToml`) that storage's didn't, so the check lives inline in each handler's body rather than at the `.command.ts` level — moving the config read ahead of the mutex check as part of the same reorder is also more correct (Go's `PersistentPreRunE` loads config unconditionally before validating flag groups too). Swaps the order in both handlers, fixes misleading ordering comments (and two stale Go line-number citations found nearby), documents the precedence in both commands' `SIDE_EFFECTS.md`, and adds regression coverage for the "mutex conflict without `--experimental`" case in both `generate` and `sync`. Fixes CLI-1876
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current Behavior
Go's cobra runs
PersistentPreRunE(which includes the--experimentalgate,apps/cli-go/cmd/root.go:91-96) beforeValidateFlagGroups()(mutual-exclusivity checks,cobra@v1.10.2/command.go:985,1010). The TS port'sstorage ls/cp/mv/rmhad this backwards — checking--linked/--localexclusivity before the--experimentalgate — sosupabase storage ls --linked --local(without--experimental) surfaced the mutex error in TS instead of Go's "must set the --experimental flag" error.Fixes CLI-1855
Expected Behavior
All 4 storage leaves now run the experimental gate first, matching Go's precedence exactly (verified against the built binary). Also corrects the inline comments in those files and the shared
legacy-experimental-gate.tsdoc, which claimed Go gates experimental "after flag-group validation" — this was backwards per cobra's actual execution order.An existing e2e test (
storage.e2e.test.ts) asserted the old (wrong) precedence for--linked --localwithout--experimental; fixed it to add--experimentalso it keeps testing the mutex message it originally intended, at the real subprocess boundary. New ordering coverage for all 4 leaves lives in a new integration suite (storage.experimental-gate.integration.test.ts) rather than duplicating it in e2e, per this repo's e2e scope policy.Out of scope: the same gate-vs-mutex-check ordering bug also exists in
db schema declarative generate/sync(verified against Go source), unrelated command family — filing as a separate follow-up rather than expanding this PR.