fix(cli): exempt global choice flags from telemetry redaction (Go isEnumFlag parity)#5855
Merged
Coly010 merged 2 commits intoJul 10, 2026
Conversation
…numFlag parity) --output, --dns-resolver, and --agent are registered as *utils.EnumFlag on Go's rootCmd.PersistentFlags() (cmd/root.go:330-333), so Go's isEnumFlag (cmd/root_analytics.go:110-116) always reports their changed value verbatim in cli_command_executed telemetry. The TS port's global-flag fallback in buildFlagsMap only ever fell through to REDACTED_VALUE for these three, since CLI-1866's config-based choice detection only covers a command's own locally-declared flags, not the shared global registry. Add GLOBAL_CHOICE_FLAG_NAMES, derived from LEGACY_GLOBAL_FLAGS the same way GLOBAL_SHORT_ALIASES already is, and consult it only on the global-fallback path (a changed flag not present in the invoking command's own `flags` record). A command that registers its own differently-typed local flag under the same CLI name (e.g. db diff's local string --output) still shadows the global and stays redacted, matching Go's isEnumFlag type-asserting that command's own non-enum flag object instead of root's persistent EnumFlag. CLI-1904
Review follow-up on the CLI-1904 fix (all 4 review-changes agents approved, no blocking findings): - GLOBAL_CHOICE_FLAG_NAMES now derives from the existing getChoiceFlagNames helper instead of re-implementing its unwrap-and-classify predicate, so the two can never silently drift apart (architect-reviewer + engineer-reviewer). - Reworked the two AGENTS.md telemetry bullets into shorter sentences/a list and made the "config cannot cover global flags" reasoning explicit inline (supabase-dx-reviewer).
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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". |
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@d70116e5a76861bd1b760c2045b74680746663faPreview package for commit |
avallete
approved these changes
Jul 10, 2026
Coly010
deleted the
columferry/cli-1904-telemetry-global-choice-flags-output-dns-resolver-agent
branch
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 free
to 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.
What changed
Go's
isEnumFlag(apps/cli-go/cmd/root_analytics.go:110-116) unconditionally reports the value of any*utils.EnumFlag-backed pflag verbatim incli_command_executedtelemetry — no per-flag annotation needed.--output,--dns-resolver, and--agentare all registered as*utils.EnumFlagonrootCmd.PersistentFlags()(cmd/root.go:330,331,333), so Go always sends their real value.The TS port's
withLegacyCommandInstrumentationalready auto-detectsFlag.choiceflags declared in a command's ownconfig(CLI-1866), and already resolves global/persistent flag values as a fallback when a command doesn't declare that CLI name locally (CLI-1896). But CLI-1896 deliberately scoped out the 3 global choice flags — nothing taught the global-fallback path that a resolved value might itself be a safe enum, so--output/--dns-resolver/--agentalways fell through to"<redacted>".This adds
GLOBAL_CHOICE_FLAG_NAMES(legacy-command-instrumentation.ts), derived fromLEGACY_GLOBAL_FLAGSby reusing the existinggetChoiceFlagNameshelper (so the choice-detection predicate has exactly one home), and consults it only on the global-fallback path — i.e. only when the invoking command's ownflagsrecord doesn't already declare that CLI name. A command that registers its own differently-typed local flag under the same CLI name (db diff's local stringoutput: Flag.string("output"),cmd/db.go:622) still shadows the global and stays redacted, matching Go'sisEnumFlagtype-asserting that command's own non-enum flag object instead of root's persistentEnumFlag. Verified this is the real wiring, not a hypothetical:db diffpassesoutputin its ownflagsrecord with no matchingconfigentry, soisFromHandleris structurallytruefor it and the new global set is never consulted.Also verified Go's separate
output_formattelemetry property (PropOutputFormat) is independent of the genericflags.outputentry — both already fire in Go too, so no double-counting concern from un-redactingflags.output.Fixes CLI-1904.
Why
Found during CLI-1866's review as a pre-existing gap in the same problem class, tracked separately since it was out of scope for that PR's diff, then explicitly deferred again by CLI-1896 ("the 3 global choice flags ... remain redacted — that gap is the already-tracked CLI-1904, not this ticket").
Reviewer-relevant context
review-changesagents (architect, engineer, security, DX) reviewed this diff and approved with no blocking findings. Two converging nits were fixed in a follow-up commit: deduped the choice-detection predicate (GLOBAL_CHOICE_FLAG_NAMESnow derives fromgetChoiceFlagNamesinstead of re-implementing it) and clarified the twoAGENTS.mdtelemetry bullets.*.integration.test.tstoday exerciseswithLegacyCommandInstrumentation's PostHog-capture behavior for any flag (that coverage is entirely the job oflegacy-command-instrumentation.unit.test.ts's own scenario suite) — this is a pre-existing, repo-wide pattern, not something this PR narrows or regresses. Adding integration-tier telemetry assertions todb diff/db queryspecifically would set a new cross-cutting test precedent well beyond this fix's scope, so left as a possible future hardening rather than folded in here.