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.