fix(cli): resolve global/persistent flag values in legacy telemetry#5830
Merged
Coly010 merged 3 commits intoJul 9, 2026
Conversation
…CLI-1896) Go's changedFlags() walks cmd.Parent()'s PersistentFlags() in addition to a command's own flags, so a global boolean flag like --debug reports its real value in cli_command_executed telemetry. The TS port's withLegacyCommandInstrumentation had no way to resolve any global/persistent flag (shared/legacy/global-flags.ts) at all, so a changed global flag always fell back to "<redacted>" -- even --debug/--yes/--experimental/--create-ticket, which Go always shows verbatim via its boolean-is-safe rule. Add legacyGlobalFlagValues (shared/legacy/global-flags.ts) to resolve every global flag's live value, and thread it into buildFlagsMap as a fallback used only when a changed CLI flag name isn't already declared in the invoking command's own flags record -- mirroring Go's ancestor-walk precedence (a command's own flag always wins, e.g. db diff's local --output shadowing the global --output enum). Restrict safeFlags/config-based safety classification to values that actually came from the handler's own flags record, so a future per-command safe/choice annotation can never be misapplied to an unrelated global flag's value purely by CLI-name collision. Scope is deliberately narrow: this fixes the 4 boolean globals; the 3 global choice flags (--output/--dns-resolver/--agent) remain redacted, matching the existing carve-out for CLI-1904.
Contributor
Author
|
@codex review |
This comment was marked as off-topic.
This comment was marked as off-topic.
![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: 08602fa5f0
ℹ️ 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: #5830) Go's changedFlags() reports the canonical flag name for a persistent flag's shorthand too (pflag.Visit), but extractChangedFlagNames only consulted each command's own aliases map, so `-o json` never resolved to `output` and the CLI-1896 global-flag fallback silently dropped it. Derive a short-alias map from LEGACY_GLOBAL_FLAGS itself (today just `-o` -> `output`, cmd/root.go:330) and merge it ahead of each command's own aliases.
…ated global flags (review: #5830) A local value-consuming flag not listed in VALUE_CONSUMING_LONG_FLAGS/ VALUE_CONSUMING_SHORT_FLAGS made extractChangedFlagNames mis-scan its raw space-separated value as a separate flag name (e.g. `secrets set --env-file --debug`, where pflag consumes the literal token `--debug` as --env-file's value). Previously harmless, the CLI-1896 global-flag fallback now turns that stray name into a fabricated `flags.debug` value Go never records. Extend both sets to cover every value-consuming flag declared across legacy/commands/ and add a static completeness test so a future command that adds one and forgets to register it fails CI.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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@98e3da5e1905e20e5675582963a259f8655bda3bPreview package for commit |
jgoux
approved these changes
Jul 9, 2026
Coly010
deleted the
columferry/cli-1896-legacy-telemetry-doesnt-record-globalpersistent-flags-eg
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
changedFlags(cmd)(apps/cli-go/cmd/root_analytics.go:52-76) walkscmd.Parent()at every ancestor collectingPersistentFlags()(global/root flags:--debug,--yes,--experimental,--create-ticket,--output,--dns-resolver,--agent,--profile,--workdir,--network-id) in addition to a command's own flags, then reports booleans and enum-typed flags verbatim incli_command_executedtelemetry.The TS port's
withLegacyCommandInstrumentationhad no way to resolve any global/persistent flag's value — only the invoking command's own locally-declaredflagsoption was ever consulted. So a changed global flag always fell back to the literal string"<redacted>", even a boolean like--debug, diverging from Go'sflags: {debug: true}.This adds
legacyGlobalFlagValues(shared/legacy/global-flags.ts) to resolve every global flag's live value viaEffect.serviceOption(a no-op outside the real CLI tree, so it adds noRrequirement and no per-command wiring), and threads it intobuildFlagsMapas a fallback used only when a changed CLI flag name isn't already declared in the invoking command's ownflagsrecord — mirroring Go's ancestor-walk precedence (a command's own flag always wins on a name collision, e.g.db diff's local--outputfile-path flag shadowing the global--outputenum, exactly as it does in Go's cobra tree).safeFlags/config-based safety classification is now restricted to values that actually came from the handler's ownflagsrecord, so a future per-command safe/choice annotation can never be misapplied to an unrelated global flag's value purely by CLI-name collision (raised in review).Scope is deliberately narrow: this fixes the 4 boolean globals (
--debug,--yes,--experimental,--create-ticket) immediately. The 3 global choice flags (--output,--dns-resolver,--agent) remain redacted — that gap is the already-tracked CLI-1904, not this ticket.Fixes CLI-1896.
Why
Found during CLI-1868's review (architect-reviewer finding) as a systemic gap affecting all ~73
withLegacyCommandInstrumentation({ flags })call sites — it was previously unreachable fortelemetry enable/disablespecifically (hardcoded toanalytics: false), but CLI-1868 made it reachable there, surfacing the gap.Reviewer-relevant context
--output/--dns-resolver/--agent) still redact. This is intentional — CLI-1904 already tracks teaching the safety pipeline about globalEnumFlags, and folding it into this change would blur two independent tickets.flags.debug(andyes/experimental/create-ticket) will now emit real booleans going forward instead of the literal string"<redacted>"for these four flags specifically. Any saved insight/funnel doing exact-string matching on"<redacted>"for these keys will stop matching after this ships.