fix: pass correct args to HelpFunc (closes #2154)

[scovl]

Two code paths in command.go were passing wrong arguments to the function registered via SetHelpFunc(func(*Command, []string)):

1. ExecuteC() — when --help/-h triggers flag.ErrHelp, the raw flag slice was passed instead of the post-parse positional arguments. Fixed by passing cmd.Flags().Args() (or the full slice when DisableFlagParsing is set).

2. InitDefaultHelpCmd() — the help _{built-in always passed an empty slice. Fixed by deriving the remaining args from cmd.Root().Find(args) so callers receive the extra tokens (e.g. plugin CLIs that inspect sub-command arguments).}

Also extracts constant fmtSubCmdNameDesc in defaultUsageFunc (S1192) and centralises all duplicate test-string literals into a single const block in command_test.go (S1192 x43, S1186 x4).

Supersedes PR #2158 (open, merge conflicts). @ccoVeille

[CLAassistant]

All committers have signed the CLA.

[ccoVeille]

Could limit the PR to the feature you are testing and not adding changes that are unrelated.

Here you have tons of constants added for stylistic and opinionated reasons.

I'm not saying these stylistic changes are bad.

I'm saying it brings a lot of noises to the PR, file being unrelated to the changes you add, and possible conflicts with other pending PR

[scovl]

@ccoVeille
You're right, and thanks for the feedback. My initial intent was to reduce pattern repetition by extracting constants, but I should have kept that in a separate PR to avoid noise and potential conflicts.

I've force-pushed with only the #2154 fix: the three command.go hunks (doc-comment, ExecuteC(), InitDefaultHelpCmd()) and the new tests appended at the end of command_test.go. No constants, no renames, no changes to existing tests.

Could you take another look?
Ty!

[ccoVeille]

Thanks

[ccoVeille]

Here, it should be clearer that this satisfies the signature expected for SetHelpFunc, a comment maybe

[ccoVeille]

Thanks

[ccoVeille]

I find this plus checkError a bit uncommon, also you are creating multiple TestHelpFunc functions

did you consider something like this? That would match existing code in tests ?

func TestHelpFuncReceivesExpectedArgs_Table(t *testing.T) {
    type tc struct {
        name string

        // command setup
        disableFlagParsing bool
        addChildFlag       bool
        rootRunnable       bool // whether root has Run to avoid non-runnable behavior

        // args to execute
        argv []string

        // expectations
        wantCmd  string
        wantArgs []string
    }

    tests := []tc{
        {
            name:           "help flag passes only positional args (not flags)",
            addChildFlag:   true,
            argv:           []string{"child", "arg1", "--myflag", "val", "arg2", "--help"},
            wantCmd:        "child",
            wantArgs:       []string{"arg1", "arg2"},
        },
        {
            name:     "short -h passes only positional args",
            argv:     []string{"child", "arg1", "arg2", "-h"},
            wantCmd:  "child",
            wantArgs: []string{"arg1", "arg2"},
        },
        {
            name:     "`help child ...` passes args after subcommand path",
            argv:     []string{"help", "child", "arg1", "arg2"},
            wantCmd:  "child",
            wantArgs: []string{"arg1", "arg2"},
        },
        {
            name:     "no positional args with --help => empty slice",
            argv:     []string{"child", "--help"},
            wantCmd:  "child",
            wantArgs: []string{},
        },
        {
            name:              "DisableFlagParsing: help path receives full args (flag-like tokens kept)",
            disableFlagParsing: true,
            // Make child non-runnable (no Run/RunE) so Cobra returns flag.ErrHelp automatically
            argv:     []string{"child", "arg1", "--flag-as-arg", "arg2"},
            wantCmd:  "child",
            wantArgs: []string{"arg1", "--flag-as-arg", "arg2"},
        },
        {
            name:         "root command --help works",
            rootRunnable: true,
            argv:         []string{"--help"},
            wantCmd:      "prog",
            wantArgs:     []string{},
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            rootCmd := &Command{Use: "prog"}
            if tt.rootRunnable {
                rootCmd.Run = emptyRun
            }

            childCmd := &Command{Use: "child"}
            if !tt.disableFlagParsing {
                // Only set Run when runnable cases want normal execution; for the DisableFlagParsing
                // case we want non-runnable behavior (no Run/RunE) as described in the PR.
                childCmd.Run = emptyRun
            }
            childCmd.DisableFlagParsing = tt.disableFlagParsing

            if tt.addChildFlag {
                childCmd.Flags().String("myflag", "", "a flag")
            }
            rootCmd.AddCommand(childCmd)

            helpCalled := false
            rootCmd.SetHelpFunc(func(c *Command, gotArgs []string) {
                helpCalled = true
                if c.Name() != tt.wantCmd {
                    t.Fatalf("expected cmd %q, got %q", tt.wantCmd, c.Name())
                }
                if len(gotArgs) != len(tt.wantArgs) {
                    t.Fatalf("expected args %v (len %d), got %v (len %d)",
                        tt.wantArgs, len(tt.wantArgs), gotArgs, len(gotArgs))
                }
                for i := range tt.wantArgs {
                    if gotArgs[i] != tt.wantArgs[i] {
                        t.Fatalf("expected args %v, got %v", tt.wantArgs, gotArgs)
                    }
                }
            })

            _, err := executeCommand(rootCmd, tt.argv...)
            if err != nil {
                t.Fatalf("unexpected error: %v", err)
            }
            if !helpCalled {
                t.Fatalf("help function was not called")
            }
        })
    }
}

This is pseudo code generated by Copilot when I asked him to tell me what would be the classic way to test.

I'm not expecting you to change it, but to tell me why you applied the pattern I can see in the PR

[scovl]

You're right, I overcomplicated it. Replaced the whole thing with a single table-driven TestHelpFuncArgs using t.Run subtests, same pattern as TestStripFlags and TestFind.

Dropped the overridingHelp struct, assertions are now inline with reflect.DeepEqual. TestDirectHelpCallNoPanic stays separate since it calls cmd.Help() directly. Force-pushed, thanks for the review.

I'll pay more attention next time.

[ccoVeille]

The error possibly returned by find is discarded

[scovl]

@ccoVeille The concern is valid, but this behavior actually predates this PR. CheckErr(cmd.Help()) was a no-op because cmd.Help() unconditionally returns nil:

func (c *Command) Help() error {
    c.HelpFunc()(c, []string{})
    return nil // always nil
}

The original code used "false logic" it looked like error handling, but didn't actually guard anything.

I used an explicit _ on line 1319 as a semantic marker. It’s more honest to make the silence visible than to hide it behind a wrapper that implies handling. This makes it clear that we eventually need to either propagate the error properly or document why it's safe to ignore.

I'm happy to add a //nolint or an explanatory comment if you prefer!

[ccoVeille]

I agree with your analysis of existing checkErr calling something that always return nil

But here you use Find, find can return an error, in some edge cases

And here you are ignoring it.

https://github.com/spf13/cobra/blob/main/command.go#L775-L777

[scovl]

Hey @ccoVeille, thanks for the thorough review.

I studied your feedback more carefully and ran the full test suite to understand the issue. I've squashed everything into a single commit (5240b43) with the fix:

cmd, remainingArgs, e := c.Root().Find(args)

One call, all three return values captured. The error e is checked immediately after (if cmd == nil || e != nil), so nothing is silently discarded anymore. I also added a comment in ExecuteC explaining why return cmd, nil is safe after calling HelpFunc (it doesn't return an error, and cmd.Help() always returns nil).

Could you take another look and let me know if this is good now?

[ccoVeille]

Thanks this approach seems good enough

[ccoVeille]

I'm annoyed here

CheckErr(cmd.Help())

was here to exist on some condition, here you are not handling the error returned by cmd.HelpFunc, but here you always return nil.

Does it work as intended then?