From e47ae799d7a9bb8e9340e17833f0d3cbd0a8ae50 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 20 Oct 2026 01:07:23 +0000 Subject: [PATCH] Keep empty and dash operands with runnable parents --- docs/roadmap/remediation.md | 14 ++++++- internal/cobra/parse.go | 5 ++- route_operands_test.go | 77 +++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 route_operands_test.go diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 123af19..0e937d0 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Alias interaction audit remediation -Status: in progress; completed findings are marked individually below. +Status: complete; R-1 through R-3 are addressed and marked individually below. ## Review scope and evidence @@ -140,7 +140,7 @@ passed on Go 1.26.8. ## R-3: Preserve empty and dash operands before child-name tokens -Status: open. Priority: P2. Category: inherited parser/routing inconsistency. +Status: complete. Priority: P2. Category: inherited parser/routing inconsistency. **Evidence:** [Session.Parse](../../internal/cobra/parse.go#L55) gives the entire leading non-option token sequence to Cobra's `Find`. Cobra's command search @@ -186,3 +186,13 @@ values, and expose parent invocation paths. Cover parent-local flag aliases and help on the same inputs, plus genuine child selection and ordinary terminator behavior. Tests should establish CommandKit's boundary, not depend on a specific replacement routing algorithm or backend type. + +**Completed:** The adapter ends command search before empty or single-dash +operand tokens, preserving them and subsequent tokens for the parent's ordinary +flag/operand parser. All four canonical/alias regressions failed before the fix +and now preserve the ordered operands, parent-local flag aliases, and invocation +paths. The same cases cover metadata-only parent help and explicit terminators; +immediate child selection remains intact. No syntax restriction or public API +was added. Final uncached offline suites passed on Go 1.26.8: root with the +workspace disabled, plus both integrations and their maintained examples in the +existing workspace against the local core. diff --git a/internal/cobra/parse.go b/internal/cobra/parse.go index 82e55a8..d1f8e6a 100644 --- a/internal/cobra/parse.go +++ b/internal/cobra/parse.go @@ -408,7 +408,10 @@ func flagUsage(flag commandmeta.Flag) string { func splitRoutingPrefix(argv []string) ([]string, []string) { for index, argument := range argv { - if argument == "--" || (strings.HasPrefix(argument, "-") && argument != "-") { + // Empty strings and "-" can be operands but never command names. + // Keep them out of Find, which could skip them and select a child + // from a later operand instead of retaining the runnable parent. + if argument == "" || strings.HasPrefix(argument, "-") { return commandpath.CloneTokens(argv[:index]), commandpath.CloneTokens(argv[index:]) } } diff --git a/route_operands_test.go b/route_operands_test.go new file mode 100644 index 0000000..4053e8d --- /dev/null +++ b/route_operands_test.go @@ -0,0 +1,77 @@ +package commandkit + +import ( + "bytes" + "context" + "io" + "reflect" + "strings" + "testing" +) + +func TestRunnableParentKeepsNonCommandTokensAsOperands(t *testing.T) { + type input struct { + words []string + format Supplied[string] + } + var received input + var entered, canonical string + built, parentRuns, childRuns := 0, 0, 0 + child := NewCommand(CommandSpec[struct{}]{Name: "child", Run: func(inv Invocation, _ struct{}) Outcome { + childRuns++ + entered, canonical = inv.EnteredPath(), inv.CanonicalPath() + return Success() + }}) + parent := NewCommand(CommandSpec[input]{ + Name: "run", Children: []Command{child}, NewInput: func() input { built++; return input{} }, + Arguments: []Argument[input]{RepeatArgument(ArgumentMeta{Name: "ARG"}, 0, -1, + func(value string) (string, error) { return value, nil }, + func(target *input, value Supplied[[]string]) { target.words = value.Value })}, + Flags: []Flag[input]{ValueFlag(FlagMeta{Name: "format", Short: "f"}, (*string)(nil), + func(value string) (string, error) { return value, nil }, + func(target *input, value Supplied[string]) { target.format = value })}, + Run: func(inv Invocation, value input) Outcome { + parentRuns++ + received, entered, canonical = value, inv.EnteredPath(), inv.CanonicalPath() + return Success() + }, + }) + app, err := New(Identity{Name: "Demo"}, parent, Alias("r", "run"), FlagAliasFor("run", "--fmt", "--format")) + if err != nil { + t.Fatal(err) + } + for _, spelling := range []string{"run", "r"} { + for _, operand := range []string{"", "-"} { + t.Run(spelling+"/"+operand, func(t *testing.T) { + beforeBuilt, beforeRuns := built, parentRuns + args := []string{spelling, operand, "child", "--fmt=yaml"} + var output, diagnostics bytes.Buffer + outcome := app.Execute(context.Background(), args, Streams{Out: &output, Err: &diagnostics}) + if outcome.Kind != OutcomeSuccess || built != beforeBuilt+1 || parentRuns != beforeRuns+1 || childRuns != 0 { + t.Fatalf("outcome = %#v, builds/runs = %d/%d/%d, diagnostics %q", outcome, built, parentRuns, childRuns, diagnostics.String()) + } + if !reflect.DeepEqual(received.words, []string{operand, "child"}) || received.format != (Supplied[string]{Value: "yaml", Set: true}) || + entered != spelling || canonical != "run" { + t.Fatalf("parent input = %#v, paths = %q/%q", received, entered, canonical) + } + outcome = app.Execute(context.Background(), append(args, "--help"), Streams{Out: &output, Err: &diagnostics}) + if outcome.Kind != OutcomeSuccess || built != beforeBuilt+1 || parentRuns != beforeRuns+1 || + !strings.Contains(output.String(), "Demo "+spelling+" [ARG...]") || !strings.Contains(output.String(), "--fmt → --format") { + t.Fatalf("parent help = %#v, output %q, diagnostics %q", outcome, output.String(), diagnostics.String()) + } + outcome = app.Execute(context.Background(), []string{spelling, "--", operand, "child", "--fmt=yaml"}, Streams{Out: io.Discard, Err: &diagnostics}) + if outcome.Kind != OutcomeSuccess || !reflect.DeepEqual(received.words, []string{operand, "child", "--fmt=yaml"}) || received.format.Set { + t.Fatalf("terminated parent invocation = %#v, input %#v, diagnostics %q", outcome, received, diagnostics.String()) + } + }) + } + } + for _, spelling := range []string{"run", "r"} { + beforeBuilt, beforeRuns, beforeChildren := built, parentRuns, childRuns + outcome := app.Execute(context.Background(), []string{spelling, "child"}, Streams{Out: io.Discard, Err: io.Discard}) + if outcome.Kind != OutcomeSuccess || built != beforeBuilt || parentRuns != beforeRuns || childRuns != beforeChildren+1 || + entered != spelling+" child" || canonical != "run child" { + t.Fatalf("genuine child selection = %#v, paths %q/%q, builds/runs %d/%d/%d", outcome, entered, canonical, built, parentRuns, childRuns) + } + } +}