fix(cli): functions delete/download mark --project-ref telemetry-safe (Go parity)#5809
Conversation
…rity) Go marks --project-ref telemetry-safe on both functionsListCmd and functionsDeleteCmd (cmd/functions.go:150-153), but the TS port only did so for list, so delete's project ref was redacted in telemetry unlike Go and unlike list. Fixes CLI-1864
Three reviewers (architect, engineer, DX) independently flagged that the new command-level test never actually exercised delete.command.ts's real wiring — it re-invoked withLegacyCommandInstrumentation directly, so it would stay green even if safeFlags were dropped from the command. Fix by exporting the composed handler (legacyFunctionsDeleteHandler) so the integration test drives the exact effect Command.withHandler uses, with mocked services instead of a real network call. Verified the new test actually fails when the fix is reverted. The engineer also found functions download has the identical gap: Go marks --project-ref telemetry-safe there too (cmd/functions.go:178), but download.command.ts never set safeFlags at all. Fixed the same way, and hoisted FUNCTIONS_PROJECT_REF_SAFE_FLAGS into functions.shared.ts since it's now used by four sibling commands (list, deploy, delete, download) instead of being duplicated as a one-off per-command constant. Also reconciled delete/download's SIDE_EFFECTS.md against list's (missing Files Read/Environment Variables rows, a stale SUPABASE_API_URL claim, and a new Telemetry Events Fired section), per findings from all four reviewers.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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@89d19712193a410b9f745befd13be9525566a3a9Preview package for commit |
![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: ba4c3925a1
ℹ️ 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".
…th delete/list
The previous commit reconciled delete's SIDE_EFFECTS.md against list's in full
but only partially reconciled download's — it added the Files Written rows and
fixed the SUPABASE_API_URL claim, but left the Files Read table, the
Environment Variables table, and the API Routes table out of sync. download's
handler wires LegacyCliConfig/LegacyProjectRefResolver/LegacyLinkedProjectCache/
LegacyTelemetryState identically to delete and list, so the same env vars
(SUPABASE_HOME, SUPABASE_PROFILE, SUPABASE_PROJECT_ID, SUPABASE_WORKDIR), the
same profile/project-ref/telemetry file reads, and the same best-effort
GET /v1/projects/{ref} cache-metadata request apply here too. Two Codex review
comments on PR #5809 caught the Environment Variables and API Routes gaps;
this also closes the identical Files Read gap they didn't flag.
|
@codex review |
|
🤖 pr-autopilot needs a human here: this PR now shows
Per safety rules I'm not auto-resolving conflicts. CI is green on the current head ( |
|
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". |
…4-functions-delete-add-safeflags-project-ref-telemetry-parity # Conflicts: # apps/cli/src/legacy/commands/functions/delete/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f74ece9724
ℹ️ 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".
…ownload Codex's re-review of the merge commit caught four more compatibility-checklist gaps, all verified against the shared legacy layers delete and download both wire (LegacyCliConfig, LegacyProjectRefResolver, LegacyCredentials): - The profile file path honors SUPABASE_HOME (legacy-profile-file.ts:legacyProfileFilePath), not a hardcoded `~/.supabase/profile`. - SUPABASE_NO_KEYRING disables the OS keyring before falling back to the access-token file (legacy-credentials.layer.ts:330), same as login's/logout's documented behavior. - Interactive project-ref resolution without --project-ref/SUPABASE_PROJECT_ID calls GET /v1/projects for the picker prompt (legacy-project-ref.layer.ts:52), matching functions list's documented route. - Workdir resolution walks up from cwd looking for supabase/config.toml before falling back to cwd (legacy-cli-config.layer.ts:resolveWorkdir), not a straight cwd fallback. Applied to both delete's and download's SIDE_EFFECTS.md since both commands share the exact same resolution layers.
…ode quality) Table column widths were left inconsistent after the previous docs commit widened cells, tripping oxfmt --check on functions delete/download docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2747c1965c
ℹ️ 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".
…edaction in functions SIDE_EFFECTS.md (review: #PRRT_kwDOErm0O86O5RVY, #PRRT_kwDOErm0O86O5RVZ) The access-token fallback file honors SUPABASE_HOME via legacySupabaseHome() (legacy-credentials.layer.ts:322-324), not a hardcoded ~/.supabase path. functions download also has three boolean flags (use-api/use-docker/legacy-bundle) that withLegacyCommandInstrumentation records verbatim per Go's isBooleanFlag branch (root_analytics.go:92, 106-108), so "every other flag redacted" was inaccurate for that command.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
…4-functions-delete-add-safeflags-project-ref-telemetry-parity # Conflicts: # apps/cli/src/legacy/commands/functions/download/download.integration.test.ts # apps/cli/src/shared/functions/functions.shared.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
Coly010
deleted the
columferry/cli-1864-functions-delete-add-safeflags-project-ref-telemetry-parity
branch
2 participants
What kind of change does this PR introduce?
Bug fix (Go CLI parity).
What is the current behavior?
Go marks
--project-reftelemetry-safe on all fourfunctionsleaf commands (cmd/functions.go:151,153,165,178) —list,delete,deploy,download— so the flag value is recorded verbatim in thecli_command_executedPostHog event instead of being redacted.The TS legacy port only did this for
listanddeploy.functions deleteandfunctions downloadwere both redacting--project-ref, diverging from Go and from their own sibling commands.What is the new behavior?
safeFlagsfor--project-reftofunctions deleteandfunctions download, matchinglist/deployand Go.["project-ref"]safe-flags list into a sharedFUNCTIONS_PROJECT_REF_SAFE_FLAGSconstant inshared/functions/functions.shared.ts, now referenced by all four commands instead of being duplicated inline.legacyFunctionsDeleteHandler,legacyFunctionsDownloadHandler) so integration tests can drive the exact effectCommand.withHandlerwires up, rather than re-testing the generic instrumentation wrapper in isolation.delete/download'sSIDE_EFFECTS.md(missingFiles Written/Files Read/Environment Variablesrows, a staleSUPABASE_API_URLclaim, and a newTelemetry Events Firedsection), reconciled againstlist's already-accurate doc.Fixes CLI-1864