Skip to content

Show section subcommand help when a global flag precedes the section #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
evanphx merged 1 commit into from
Jul 9, 2026
Merged

Show section subcommand help when a global flag precedes the section #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 14 additions & 18 deletions dispatcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,34 +473,35 @@ func (d *Dispatcher) findCommandWithInterspersedFlags(args []string) (*CommandEn
fullArgs = append(fullArgs, args[lastCommandIndex+1:]...)
}

// Try to validate our flag assumptions were correct
// Only validate flags that came before the command
// Validate that our value-consumption guesses were correct for the
// flags that came before the command. We only reject a match when a
// KNOWN flag was wrongly assumed to take a value (bool flags don't).
//
// We deliberately do NOT reject a match just because a pre-command
// flag is absent from this command's flagset: callers routinely place
// global flags (e.g. -C/--cluster) before a command whose own flagset
// doesn't declare them (such as section/namespace commands with empty
// flagsets). If a flag is genuinely bogus, fs.Parse reports it as a
// clear "unknown flag: X" instead of a misleading "unknown command".
valid := true
for _, fi := range skippedItems {
// Only check flags that came before the end of the command
// Only check flags that came before the end of the command; flags
// after the command are handled by Parse.
if fi.index > lastCommandIndex {
continue // This flag is after the command, will be handled by Parse
continue
}

flagName := strings.TrimPrefix(fi.flag, "--")
flagName = strings.TrimPrefix(flagName, "-")

// Check if this flag exists in the command's flagset
flagFound := false
fs.VisitAll(func(f *Flag) {
if (len(flagName) == 1 && f.Short == rune(flagName[0])) || f.Name == flagName {
flagFound = true
// Check if our assumption about the flag taking a value was correct
// Check if our assumption about the flag taking a value was correct.
if fi.hasValue && f.Value.IsBool() {
valid = false // Bool flags don't take values
}
}
})

if !flagFound && !isHelpFlag(fi.flag) {
// Unknown flag (unless it's a help flag which is always valid)
valid = false
}
}

if valid {
Expand All @@ -513,11 +514,6 @@ func (d *Dispatcher) findCommandWithInterspersedFlags(args []string) (*CommandEn
return nil, args
}

// isHelpFlag checks if a flag is a help flag
func isHelpFlag(flag string) bool {
return flag == "-h" || flag == "--help"
}

// normalizeCommandPath normalizes a command path for consistent lookup
func normalizeCommandPath(path string) string {
// Split by spaces, filter empty strings, and rejoin
Expand Down
66 changes: 65 additions & 1 deletion dispatcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,71 @@ func TestDispatcherNamespaceDiscovery(t *testing.T) {
})
}

// TestDispatcherInterspersedValueFlagBeforeSection reproduces the regression where
// a value-taking global flag placed before a section-style command (a command with
// an empty flagset that returns ErrShowHelp) caused the resolver to reject the match
// and fall back to top-level help instead of the section's sub-commands.
func TestDispatcherInterspersedValueFlagBeforeSection(t *testing.T) {
// newDispatcher builds a dispatcher with a section-style "auth" command whose
// flagset declares no flags and whose Run returns ErrShowHelp, plus two
// sub-commands. A real value-taking flag "-C" is only declared on a sibling
// command, so it is unknown to the "auth" section's flagset.
newDispatcher := func() *Dispatcher {
Comment on lines +1248 to +1252

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Declare -C as a known value-taking flag in the fixture.

The fixture never registers -C; it is unknown to every flagset. Consequently, these cases do not verify the regression described by the test name and PR objective—handling a known value-taking global flag before a section. Add the real global/sibling flag registration and retain auth without that flag so the candidate-validation path is exercised.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dispatcher_test.go` around lines 1248 - 1252, The test fixture for
newDispatcher is missing a real registration for the value-taking -C flag, so it
never exercises the intended known-global-flag-before-section path. Update the
fixture to declare -C on the appropriate global or sibling command while keeping
the auth section’s flagset without it, so Dispatcher candidate validation and
Run behavior are tested against a flag that is known somewhere in the command
tree but unknown to auth.

d := NewDispatcher("myapp")

authFs := NewFlagSet("auth")
authFs.AllowUnknownFlags(true)
d.Dispatch("auth", NewCommand(authFs,
func(fs *FlagSet, args []string) error { return ErrShowHelp },
WithUsage("Authentication commands")))

d.Dispatch("auth generate", NewCommand(NewFlagSet("auth generate"),
func(fs *FlagSet, args []string) error { return nil },
WithUsage("Generate auth config")))

d.Dispatch("auth provider", NewCommand(NewFlagSet("auth provider"),
func(fs *FlagSet, args []string) error { return nil },
WithUsage("Manage identity providers")))

return d
}

run := func(t *testing.T, args []string) string {
t.Helper()
old := os.Stdout
r, w, _ := os.Pipe()
os.Stdout = w

err := newDispatcher().Execute(args)

w.Close()
os.Stdout = old

var buf bytes.Buffer
io.Copy(&buf, r)

Comment on lines +1272 to +1285

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Avoid synchronous pipe capture and restore stdout with defer.

io.Copy runs only after Execute returns, so the test can deadlock if help output fills the pipe buffer. Also, a panic during execution leaves the process-wide os.Stdout modified. Capture output through the project’s existing test helper, or read concurrently and restore stdout with defer.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dispatcher_test.go` around lines 1272 - 1285, The stdout capture in the test
helper inside run is brittle because io.Copy happens only after
newDispatcher().Execute returns, which can deadlock if help output fills the
pipe and can also leave os.Stdout altered on panic. Update this helper to use
the project’s existing output-capture test utility if available, or otherwise
read from the pipe concurrently while Execute runs and restore os.Stdout with
defer so the original stream is always reset even on failure.

assert.NoError(t, err)
return buf.String()
}

// Each of these must render the "auth" section's sub-commands, NOT top-level help.
cases := [][]string{
{"-C", "prod", "auth", "help"},
{"-C", "prod", "auth"},
{"-C", "prod", "auth", "--help"},
{"-C", "prod", "auth", "-h"},
}

for _, args := range cases {
t.Run(fmt.Sprintf("%v", args), func(t *testing.T) {
output := run(t, args)
assert.Contains(t, output, "Authentication commands")
assert.Contains(t, output, "generate")
assert.Contains(t, output, "provider")
})
}
}

// TestDispatcherHelpShowsTypes tests that help output shows specific types for flags
func TestDispatcherHelpShowsTypes(t *testing.T) {
d := NewDispatcher("myapp")
Expand Down Expand Up @@ -1773,4 +1838,3 @@ func TestHelpJSONIncludesGroup(t *testing.T) {
json := string(data)
assert.Contains(t, json, `"group": "Operator"`)
}

Loading