fix: skip flag-completion registration outside completion path#598
Merged
fix: skip flag-completion registration outside completion path#598
Conversation
Cobra keeps completion callbacks in a package-global map keyed by *pflag.Flag with no removal path, so registrations made during Build() outlive the command itself. Route all seven call sites through cmdutil.RegisterFlagCompletion and enable registration only when the invocation actually serves a __complete request. Measured over 30 dropped Builds: ~202 KB / 2180 retained objects per Build before, ~0 after. Change-Id: I734d598a4c91a92c33b02e0f292f640cc0e224c6
📝 WalkthroughWalkthroughRefactors flag completion registration to a centralized helper and adds a process-wide toggle to enable/disable flag completions; root command now enables completions only for completion-related invocations. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (os.Args)
participant Root as cmd/root.Execute
participant Config as configureFlagCompletions
participant CmdUtil as internal/cmdutil
participant Cobra as cobra.Command
CLI->>Root: start Execute()
Root->>Config: configureFlagCompletions(args)
Config->>CmdUtil: SetFlagCompletionsDisabled(!isCompletionCommand)
note right of CmdUtil: atomic toggle set
Root->>Cobra: build commands and register flags
Cobra->>CmdUtil: RegisterFlagCompletion(cmd, flag, fn)
alt completions enabled
CmdUtil->>Cobra: cmd.RegisterFlagCompletionFunc(flag, fn)
else completions disabled
CmdUtil-->>Cobra: no-op (skip registration)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
liangshuo-1
previously approved these changes
Apr 22, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #598 +/- ##
==========================================
+ Coverage 59.75% 59.81% +0.06%
==========================================
Files 403 404 +1
Lines 42513 42526 +13
==========================================
+ Hits 25403 25439 +36
+ Misses 15105 15082 -23
Partials 2005 2005 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@dce3c931cbe8fa1a504e023b6f432d896f5d7fc3🧩 Skill updatenpx skills add larksuite/cli#fix/cobra-flag-completion-leak -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/root.go (1)
151-158:⚠️ Potential issue | 🟠 MajorInclude Cobra's no-description completion request command.
Cobra uses both
__completeand__completeNoDescfor shell completion flows (set as primary command and alias). The helper currently checks only for__complete, which disables flag completions for the no-description path, so custom flag completions (e.g.,--format,--as) can be missing. Use Cobra's exported constants instead of string literals.🐛 Proposed fix
func isCompletionCommand(args []string) bool { for _, arg := range args { - if arg == "completion" || arg == "__complete" { + if arg == "completion" || + arg == cobra.ShellCompRequestCmd || + arg == cobra.ShellCompNoDescRequestCmd { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 151 - 158, The isCompletionCommand helper only checks literal "__complete" and "completion", missing Cobra's no-description completion; update isCompletionCommand to detect Cobra's exported constants (cobra.BashCompRequestCmd and cobra.BashCompNoDescRequestCmd) instead of string literals so both completion paths are recognized and custom flag completions (e.g., --format, --as) are enabled; reference the existing isCompletionCommand function and replace the string checks with comparisons to cobra.BashCompRequestCmd and cobra.BashCompNoDescRequestCmd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmdutil/completion_test.go`:
- Around line 39-56: The test must assert that no Cobra registration occurred by
either capturing cmd in the completion callback or performing a negative lookup;
modify the test around RegisterFlagCompletion so the closure captures the
specific cmd (e.g., use func(c *cobra.Command) { ... } capturing cmd) and have
the callback reference cmd (or call cmd.Flags().Lookup("foo") after
RegisterFlagCompletion and assert it is nil), keep the
runtime.SetFinalizer(collected.Add) logic intact and then assert
collected.Load() == N to ensure the finalizers only fire when completions are
disabled; update the test body that calls RegisterFlagCompletion, the completion
callback, and the negative lookup to guarantee the regression would fail.
---
Outside diff comments:
In `@cmd/root.go`:
- Around line 151-158: The isCompletionCommand helper only checks literal
"__complete" and "completion", missing Cobra's no-description completion; update
isCompletionCommand to detect Cobra's exported constants
(cobra.BashCompRequestCmd and cobra.BashCompNoDescRequestCmd) instead of string
literals so both completion paths are recognized and custom flag completions
(e.g., --format, --as) are enabled; reference the existing isCompletionCommand
function and replace the string checks with comparisons to
cobra.BashCompRequestCmd and cobra.BashCompNoDescRequestCmd.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd50fe19-98d9-4b3e-926d-86cd9d927c9d
📒 Files selected for processing (9)
cmd/api/api.gocmd/auth/login.gocmd/root.gocmd/schema/schema.gocmd/service/service.gointernal/cmdutil/completion.gointernal/cmdutil/completion_test.gointernal/cmdutil/identity_flag.goshortcuts/common/runner.go
- Extract configureFlagCompletions helper in cmd/root.go so the gate
between normal invocations and __complete requests can be unit-tested.
- Add TestConfigureFlagCompletions with table cases for plain commands,
--help, __complete, and the completion subcommand.
- Add TestShortcutMount_FlagCompletions{Registered,Disabled} in
shortcuts/common to exercise the two RegisterFlagCompletion call
sites in registerShortcutFlagsWithContext (enum flag and --format).
Change-Id: Idcb302bb40045b1c5b34580ec90d586eae8b8707
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/root.go (1)
149-162:⚠️ Potential issue | 🟠 MajorInclude Cobra's
__completeNoDescrequest in the completion gate.The
isCompletionCommandfunction only checks for"completion"and"__complete", but Cobra v1.10.2 also invokes a__completeNoDescpath for no-description completion scripts. Without handling this, users with no-description completions will have custom flag completions disabled. Use Cobra's exported constants to align with the dependency API.The test coverage at
cmd/root_test.goTestConfigureFlagCompletions is also missing a case for the__completeNoDescrequest.Proposed fix
func isCompletionCommand(args []string) bool { for _, arg := range args { - if arg == "completion" || arg == "__complete" { + switch arg { + case "completion", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd: return true } } return false }Add test case to TestConfigureFlagCompletions:
{"__complete request", []string{"__complete", "im", "+send", ""}, false}, + {"__completeNoDesc request", []string{"__completeNoDesc", "im", "+send", ""}, false}, {"completion subcommand", []string{"completion", "bash"}, false},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 149 - 162, The completion gate currently only checks for "completion" and "__complete" in isCompletionCommand, missing Cobra's no-description completion; update isCompletionCommand to also check Cobra's exported no-desc constant (use cobra.ShellCompRequestCmd and the corresponding no-desc constant such as cobra.ShellCompRequestNoDesc or the exact exported name in your Cobra version) instead of hardcoded strings, and ensure configureFlagCompletions still calls cmdutil.SetFlagCompletionsDisabled(!isCompletionCommand(args)); also add a TestConfigureFlagCompletions unit test case that passes the __completeNoDesc request (using the same Cobra constant) to verify custom flag completions are enabled for that request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/root.go`:
- Around line 149-162: The completion gate currently only checks for
"completion" and "__complete" in isCompletionCommand, missing Cobra's
no-description completion; update isCompletionCommand to also check Cobra's
exported no-desc constant (use cobra.ShellCompRequestCmd and the corresponding
no-desc constant such as cobra.ShellCompRequestNoDesc or the exact exported name
in your Cobra version) instead of hardcoded strings, and ensure
configureFlagCompletions still calls
cmdutil.SetFlagCompletionsDisabled(!isCompletionCommand(args)); also add a
TestConfigureFlagCompletions unit test case that passes the __completeNoDesc
request (using the same Cobra constant) to verify custom flag completions are
enabled for that request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a9fdd88-f28e-4709-967a-a37b50030215
📒 Files selected for processing (3)
cmd/root.gocmd/root_test.goshortcuts/common/runner_flag_completion_test.go
liangshuo-1
approved these changes
Apr 22, 2026
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.
Summary
RegisterFlagCompletionFuncentry in a package-global map keyed by*pflag.Flagwith no removal path (cobra/completions.go: flagCompletionFunctions). Each entry retains the flag's backing value pointer, which in turn keeps the owning command tree alive after the caller drops its reference.cmdutil.RegisterFlagCompletionwrapper with a global switch; routed all seven in-repo call sites through it.cmd/root.go:Executeenables registration only whenos.Argscontains__complete/completion, matching the existingisCompletionCommandgate already used for update checks.Impact
Measured over 30 dropped
cmd.Build()calls withGCforced between:Only
cmd/service/service.go:192(--formatregistered withStringVar(&opts.Format, ...)) transitively pulled*cmdutil.Factoryinto the global map; the other six sites retained only the flag objects themselves. Both classes are addressed uniformly.Test plan
go test ./internal/cmdutil/ -race— three new tests (switch round-trip, finalizer-based retention check, end-to-end register/lookup).go test ./cmd/... ./shortcuts/common/ -race— no regressions.go build ./...green.lark-cli --help,lark-cli im --help,lark-cli api --helprender as before.lark-cli __complete api --as ""→user,bot.lark-cli __complete api --format ""→json/ndjson/table/csv.lark-cli __complete im +chat-search --sort-by ""→ shortcut enum values.lark-cli __complete auth login --domain "cal"→ dynamiccalendarcompletion works.lark-cli completion bashgenerates the full 426-line script.lark-cli api GET /dummy --dry-runruns normally (completion off path).Summary by CodeRabbit