Skip to content

fix: skip all hidden flags in shell completion, not just bool flags - #2388

Merged
dearchap merged 1 commit into
urfave:mainfrom
vidigoat:fix/hidden-flag-completion
Aug 15, 2026
Merged

fix: skip all hidden flags in shell completion, not just bool flags#2388
dearchap merged 1 commit into
urfave:mainfrom
vidigoat:fix/hidden-flag-completion

Conversation

@vidigoat

@vidigoat vidigoat commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • bug

What this PR does / why we need it:

printFlagSuggestions (used by the default shell-completion handler) is supposed to skip hidden flags, but it only skips hidden *BoolFlag values:

if bflag, ok := flag.(*BoolFlag); ok && bflag.Hidden {
    continue
}

The type assertion is to the concrete *BoolFlag type, so a hidden flag of any other type (*StringFlag, *IntFlag, *DurationFlag, ...) fails the assertion and is emitted into the completion output anyway. That exposes flags the author explicitly marked Hidden: true whenever a user hits <TAB>.

Repro (hidden *StringFlag):

cmd := &cli.Command{
    Name:                  "app",
    EnableShellCompletion: true,
    Commands: []*cli.Command{{
        Name: "sub",
        Flags: []cli.Flag{
            &cli.StringFlag{Name: "secret", Hidden: true},
            &cli.StringFlag{Name: "visible"},
        },
        Action: func(context.Context, *cli.Command) error { return nil },
    }},
}

app sub -<TAB> prints --secret alongside --visible, even though secret is hidden. A hidden *BoolFlag in the same list is correctly omitted.

Changes:

  • help.go: replace the *BoolFlag-specific check with the existing VisibleFlag interface (IsVisible), which every flag type implements via FlagBase. This is the same mechanism visibleFlags and the flag-category code already use, so all hidden flags are now skipped regardless of concrete type.

Which issue(s) this PR fixes:

NONE

Testing

Added typical-flag-suggestion-hidden-non-bool to TestDefaultCompleteWithFlags, mirroring the existing typical-flag-suggestion-hidden-bool case but with a hidden *StringFlag. It fails on main (completion emits the hidden flag) and passes with this change. go test ./..., go vet ./..., and gofmt are clean.

Release Notes

Hidden flags of non-boolean types are no longer offered in shell completion (previously only hidden bool flags were skipped).

printFlagSuggestions only skipped hidden *BoolFlag values because it
type-asserted to the concrete *BoolFlag type. Any other hidden flag
(*StringFlag, *IntFlag, *DurationFlag, etc.) still leaked into shell
completion output.

Use the existing VisibleFlag interface (IsVisible) the same way
visibleFlags and the flag-category code already do, so every hidden
flag is skipped regardless of its concrete type.
@vidigoat
vidigoat requested a review from a team as a code owner July 9, 2026 11:28
@dearchap

Copy link
Copy Markdown
Contributor

Review

Verified locally: the new test typical-flag-suggestion-hidden-non-bool fails on main (emits --excellent) and passes with this change. Full test suite and go vet are clean.

The fix is correct and minimal. Using the VisibleFlag/IsVisible() interface is the same pattern already used by visibleFlags() and category.go, and every built-in flag type satisfies VisibleFlag via FlagBase, plus BoolWithInverseFlag and the external-flag wrapper, so no built-in type regresses. Custom flags implementing only Flag are unaffected.

Notes (non-blocking):

  • This also fixes hidden *BoolWithInverseFlag, which was previously leaked too (the old *BoolFlag assertion didn't match that concrete type). Consistent with the intent, but there's no test covering it — consider adding a BoolWithInverseFlag case to the table.
  • No exported API change, so no doc regeneration needed.

Nothing blocking. LGTM.

@dearchap
dearchap merged commit fd8b8c5 into urfave:main Aug 15, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants