fix(cli): wire --experimental gate into 7 ungated native leaves#5766
fix(cli): wire --experimental gate into 7 ungated native leaves#5766Coly010 wants to merge 6 commits into
Conversation
…PI leaves
Go gates postgres-config {get,update,delete}, ssl-enforcement {get,update},
and network-bans {get,remove} behind --experimental in root.go's
PersistentPreRunE; the TS port ran these unconditionally.
Wiring the gate required moving each leaf's legacyManagementApiRuntimeLayer
from Command.provide (command-builder level) to an inline Effect.provide
placed after the gate check inside the handler body: the layer eagerly
resolves an access token as part of construction, and Command.provide
builds it before the handler's first yield runs, which would let a missing
token error mask the missing --experimental error before this fix. Verified
against the built CLI binary that the ordering now matches Go exactly.
|
@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: 2443cfc275
ℹ️ 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".
…#5766) Cobra parses flags (rejecting an out-of-enum -o value) before PersistentPreRunE ever runs, so Go always shows an invalid --output error ahead of a missing --experimental error. The 7 leaves this PR gates ran legacyRequireExperimental first, inverting that precedence for e.g. `postgres-config get -o csv` with no --experimental. Exports legacyValidateOutputFormat (previously private to withLegacyCommandInstrumentation) and calls it explicitly as the first step in each gated leaf, ahead of the experimental check.
…review: #5766) The "gate open" case reaches the real legacyManagementApiRuntimeLayer (provided inline inside the command, not by the test's mocked runtime), which reads credentials and SUPABASE_EXPERIMENTAL straight from process.env and falls back to the OS keyring. A developer or CI host with a real SUPABASE_ACCESS_TOKEN, SUPABASE_EXPERIMENTAL, or saved keyring token could make these assertions non-deterministic, or worse, let a "gate closed" test proceed to a real network call. Adds processEnvLayer({ SUPABASE_NO_KEYRING: "1" }) to each suite's runtime, matching the isolation the e2e harness already applies.
…4-wire-the-experimental-gate-into-the-7-ungated-native-leaves # Conflicts: # apps/cli/src/legacy/commands/postgres-config/delete/delete.command.ts # apps/cli/src/legacy/commands/postgres-config/update/update.command.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06509cfad1
ℹ️ 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".
… too (review: #5766) processEnvLayer isolates the env-var and keyring credential sources, but legacyCredentialsLayer also falls back to a token file at <RuntimeInfo.homeDir>/.supabase/access-token. mockRuntimeInfo() defaults homeDir to the fixed /tmp/supabase-cli-test-home, so a stray token file left there by another test or a local run could let the "gate open" case authenticate for real instead of failing with LegacyPlatformAuthRequiredError. Points RuntimeInfo.homeDir at each test's isolated tempRoot instead.
…es' SIDE_EFFECTS.md (review: #5766) These docs said linked-project.json/telemetry.json are written "always" / "on every invocation, including failures," which is no longer accurate: a closed --experimental gate now exits before project-ref resolution, the API call, and both of those writes. Adds SUPABASE_EXPERIMENTAL to Environment Variables, the gate's exit code, and a Notes bullet matching the existing storage/cp precedent for documenting a gated command.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@e5ee27e73811a8f5acbd35d00fa53633cbb3b330Preview package for commit |
|
@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". |
1 participant
Current Behavior
Go gates
postgres-config {get,update,delete},ssl-enforcement {get,update}, andnetwork-bans {get,remove}behind the global--experimentalflag in the rootPersistentPreRunE(apps/cli-go/cmd/root.go:56-93). The TS port'slegacyRequireExperimentalhelper (shared/legacy-experimental-gate.ts) was wired only into the storage leaves, so these 7 commands ran unconditionally, without ever requiring--experimental.Fixes #CLI-1854
Expected Behavior
All 7 leaves now fail with
must set the --experimental flag to run this commandunless--experimental(orSUPABASE_EXPERIMENTAL) is set, matching Go exactly.Wiring this in required more than adding a
yield*line:legacyManagementApiRuntimeLayer(used by all 7 leaves) eagerly resolves an access token as part of its own layer construction. SinceCommand.provide(layer)builds that layer before the handler body's first yield ever runs, leaving the gate check inside the handler while the layer stayed onCommand.providewould let a missing-token error mask the missing---experimentalerror — verified against the built binary before the fix (postgres-config getwith no credentials and no--experimentalshowedLegacyPlatformAuthRequiredError, not the gate error). Each leaf now moveslegacyManagementApiRuntimeLayerto an inlineEffect.provideapplied after the gate check, matching Go's actualPersistentPreRunEorder (experimental check, then theIsManagementAPIlogin check —root.go:91-109). Re-verified against the built binary post-fix for all 7 leaves.Also corrects the
legacy-experimental-gate.tsdoc comment, which previously had cobra'sPersistentPreRunE/ValidateFlagGroupsordering backwards, and now accurately notes that the 4 pre-existing storage leaves still have the mutex-check-before-gate ordering bug (tracked separately) rather than presenting their ordering as the correct example to copy.