From 02cd49d6b6663349e35ca7fd262f209fd167099b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 7 Nov 2026 15:53:32 +0000 Subject: [PATCH] Share flag consumption between parsing and completion --- completion_flags.go | 77 +++++++++++----------------- completion_test.go | 37 ++++++++++++++ docs/roadmap/audit/catalog.md | 9 +++- docs/roadmap/implementation.md | 8 ++- internal/cobra/flags.go | 92 +++++++++++----------------------- internal/commandmeta/flags.go | 44 ++++++++++++++++ 6 files changed, 153 insertions(+), 114 deletions(-) diff --git a/completion_flags.go b/completion_flags.go index afd6be8..d7a0bee 100644 --- a/completion_flags.go +++ b/completion_flags.go @@ -53,44 +53,26 @@ func walkCompletionFlags(tokens []string, lookup commandmeta.FlagLookup) complet state.ended = true case commandmeta.OperandToken: state.operands = append(state.operands, token) - case commandmeta.LongFlagToken: - spelling, _, attached := commandmeta.SplitLongFlag(token) - flag, ok := lookup.Find(spelling) - if !ok { - state.invalid = true - return state - } - state.supplied[flag.Name] = true - if !flag.Boolean && !attached { - if index+1 == len(tokens) { - state.pending = &flag - return state - } - index++ - } - case commandmeta.ShortFlagToken: - for offset := 1; offset < len(token); offset++ { - flag, ok := lookup.Find("-" + token[offset:offset+1]) - if !ok { + case commandmeta.LongFlagToken, commandmeta.ShortFlagToken: + for offset := 1; ; { + fact := commandmeta.ConsumeFlag(lookup, token, offset) + if !fact.Found { state.invalid = true return state } - state.supplied[flag.Name] = true - suffix := token[offset+1:] - if flag.Boolean { - if strings.HasPrefix(suffix, "=") { - break - } - continue - } - if suffix == "" { + state.supplied[fact.Flag.Name] = true + if fact.NeedsNext { if index+1 == len(tokens) { + flag := fact.Flag state.pending = &flag return state } index++ } - break + if fact.NextOffset == 0 { + break + } + offset = fact.NextOffset } } } @@ -132,10 +114,10 @@ func completionOptionTarget(current string, flags []commandmeta.Flag, lookup com return completionTarget{kind: completionTargetFlags, choices: completionFlagNames(flags, supplied, current)} } if strings.HasPrefix(current, "--") { - spelling, value, attached := commandmeta.SplitLongFlag(current) - if attached { - if flag, ok := lookup.Find(spelling); ok { - return completionTarget{kind: completionTargetValue, hint: flag.Completion, prefix: spelling + "=", valuePrefix: value} + fact := commandmeta.ConsumeFlag(lookup, current, 1) + if fact.Attached { + if fact.Found { + return completionTarget{kind: completionTargetValue, hint: fact.Flag.Completion, prefix: fact.Spelling + "=", valuePrefix: fact.Value} } return completionTarget{} } @@ -145,25 +127,26 @@ func completionOptionTarget(current string, flags []commandmeta.Flag, lookup com for name, value := range supplied { used[name] = value } - for offset := 1; offset < len(current); offset++ { - flag, ok := lookup.Find("-" + current[offset:offset+1]) - if !ok { + for offset := 1; offset < len(current); { + fact := commandmeta.ConsumeFlag(lookup, current, offset) + if !fact.Found { return completionTarget{} } - used[flag.Name] = true - suffix := current[offset+1:] - if flag.Boolean { - if strings.HasPrefix(suffix, "=") { - return completionTarget{kind: completionTargetValue, hint: flag.Completion, prefix: current[:offset+2], valuePrefix: suffix[1:]} + used[fact.Flag.Name] = true + if fact.Flag.Boolean && fact.Attached { + return completionTarget{kind: completionTargetValue, hint: fact.Flag.Completion, prefix: current[:offset+2], valuePrefix: fact.Value} + } + if !fact.Flag.Boolean { + prefix := current[:offset+1] + if strings.HasPrefix(current[offset+1:], "=") { + prefix += "=" } - continue + return completionTarget{kind: completionTargetValue, hint: fact.Flag.Completion, prefix: prefix, valuePrefix: fact.Value} } - prefix := current[:offset+1] - if strings.HasPrefix(suffix, "=") { - prefix += "=" - suffix = suffix[1:] + if fact.NextOffset == 0 { + break } - return completionTarget{kind: completionTargetValue, hint: flag.Completion, prefix: prefix, valuePrefix: suffix} + offset = fact.NextOffset } var choices []commandmeta.CompletionChoice for _, flag := range flags { diff --git a/completion_test.go b/completion_test.go index 4c986a9..7d9a51a 100644 --- a/completion_test.go +++ b/completion_test.go @@ -79,3 +79,40 @@ func TestCompletionChoicesDoNotRestrictExecution(t *testing.T) { t.Fatalf("outcome=%#v input=%#v", outcome, received) } } + +func TestCompletionConsumesFlagGrammarAndKeepsPendingValues(t *testing.T) { + flags := []commandmeta.Flag{{Name: "verbose", Short: "v", Boolean: true}, {Name: "choice", Short: "c", Completion: commandmeta.CompletionHint{Kind: commandmeta.CompletionChoices}}} + lookup := commandmeta.NewFlagLookup(flags, []commandmeta.FlagAlias{{Source: "-x", Target: "--choice"}}) + for _, tc := range []struct { + name string + tokens []string + pending, operand string + invalid bool + }{ + {name: "cluster attached empty", tokens: []string{"-vc="}, operand: ""}, + {name: "option-looking value", tokens: []string{"-vx", "--other"}, operand: ""}, + {name: "pending", tokens: []string{"-vx"}, pending: "choice"}, + {name: "unknown cluster", tokens: []string{"-vz"}, invalid: true}, + {name: "terminator", tokens: []string{"--", "--choice"}, operand: "--choice"}, + } { + t.Run(tc.name, func(t *testing.T) { + state := walkCompletionFlags(tc.tokens, lookup) + if state.invalid != tc.invalid { + t.Fatalf("state=%#v", state) + } + if tc.pending != "" && (state.pending == nil || state.pending.Name != tc.pending) { + t.Fatalf("pending=%#v", state.pending) + } + if tc.pending == "" && state.pending != nil { + t.Fatalf("unexpected pending=%#v", state.pending) + } + if tc.operand != "" && (len(state.operands) != 1 || state.operands[0] != tc.operand) { + t.Fatalf("operands=%v", state.operands) + } + }) + } + target := completionOptionTarget("-vc=", flags, lookup, nil) + if target.kind != completionTargetValue || target.prefix != "-vc=" || target.valuePrefix != "" { + t.Fatalf("partial target = %#v", target) + } +} diff --git a/docs/roadmap/audit/catalog.md b/docs/roadmap/audit/catalog.md index 23b8db5..3dce289 100644 --- a/docs/roadmap/audit/catalog.md +++ b/docs/roadmap/audit/catalog.md @@ -1,6 +1,6 @@ # Consolidated audit catalog -Status: **13 distinct open findings: 13 P3**, consolidated from 30 +Status: **12 distinct open findings: 12 P3**, consolidated from 30 numbered entries in seven audit documents. Reviewed audit state: `db732e3a1de55c844f77870e1c6db683c2685d5d`. @@ -335,6 +335,11 @@ F14 under the decisions recorded there and in F13. ## F16 — P3: Parsing and completion duplicate flag-consumption grammar +**Complete.** A neutral consumption operation is shared by execution parsing +and completion. Focused grammar, provenance and completion tests and available +Bash interactive checks pass; missing-value and partial-input policies remain +with their callers. + Execution parsing and completion separately implement short clusters, attached/next values, boolean assignments, terminators, and unknown spellings. Shared classification helpers cover only part of the grammar, leaving changes to @@ -350,7 +355,7 @@ CLI behavior; no public parser or replacement backend is selected. Retain caller-specific branches where combining them would require extensive modes or special cases. Repair F06's completion fixture before validating this refactor. The [F16 analysis](remediation-analysis.md#f16--shared-flag-consumption) -records the accepted boundary; implementation remains pending. +records the accepted boundary. **Original:** [03-complexity M3](03-complexity.md#m3--p3-parsing-and-completion-maintain-parallel-flag-consumption-logic). diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 644c61e..7fe6d5a 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,6 +1,6 @@ # Audit remediation implementation plan -Status: in progress; Stages 1–13 complete. +Status: in progress; Stages 1–14 complete. Baseline: `d492c7747175c01a53261206c2b4356e53453621`. Before this sprint, intervening changes recorded decisions and retired the completed older @@ -470,6 +470,12 @@ Keep F24 open until the binding wrappers are handled in stage 20. ## Stage 14 — Share internal flag-consumption facts +**Complete.** Parsing and completion share neutral consumption facts while +retaining their separate failure and partial-input policies. Focused parser, +alias, occurrence and completion tests and both interactive Bash checks passed +in the offline aligned Go 1.27.0 workspace. Zsh and Fish remain unavailable as +recorded in Stage 6. + Findings: F16. Sources: `internal/commandmeta/flags.go`, `internal/cobra/flags.go`, and `completion_flags.go`; stage 6 is prerequisite. diff --git a/internal/cobra/flags.go b/internal/cobra/flags.go index 6cc00df..998fda7 100644 --- a/internal/cobra/flags.go +++ b/internal/cobra/flags.go @@ -2,7 +2,6 @@ package cobra import ( "fmt" - "strings" commandmeta "go.maximumdirect.net/commandkit/internal/commandmeta" ) @@ -20,78 +19,43 @@ func interpretFlagTokens(argv []string, flags []commandmeta.Flag, aliases []comm interpreted = append(interpreted, argv[index:]...) break } - if kind == commandmeta.LongFlagToken { - original, value, attached := commandmeta.SplitLongFlag(token) - flag, ok := lookup.Find(original) - if !ok { - interpreted = append(interpreted, token) - return interpreted, spellings, occurrences, nil - } - spellings[flag.Name] = append(spellings[flag.Name], original) - if attached { - interpreted = append(interpreted, "--"+flag.Name+"="+value) - occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: value}) - index++ - continue - } - if !flag.Boolean && index+1 < len(argv) { - value := argv[index+1] - interpreted = append(interpreted, "--"+flag.Name+"="+value) - occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: value}) - index += 2 - continue - } - if !flag.Boolean { - return interpreted, spellings, occurrences, &commandmeta.Failure{Message: fmt.Sprintf("flag needs an argument: %s", original)} - } - occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: "true"}) - interpreted = append(interpreted, "--"+flag.Name) - index++ - continue - } - if kind == commandmeta.ShortFlagToken { + if kind == commandmeta.LongFlagToken || kind == commandmeta.ShortFlagToken { + offset := 1 consumedNext := false - for offset := 1; offset < len(token); { - original := "-" + token[offset:offset+1] - flag, ok := lookup.Find(original) - if !ok { - interpreted = append(interpreted, "-"+token[offset:]) + for { + fact := commandmeta.ConsumeFlag(lookup, token, offset) + if !fact.Found { + if kind == commandmeta.ShortFlagToken { + interpreted = append(interpreted, "-"+token[offset:]) + } else { + interpreted = append(interpreted, token) + } return interpreted, spellings, occurrences, nil } + flag, original := fact.Flag, fact.Spelling spellings[flag.Name] = append(spellings[flag.Name], original) - suffix := token[offset+1:] - if flag.Boolean { - if strings.HasPrefix(suffix, "=") { - value := suffix[1:] - interpreted = append(interpreted, "--"+flag.Name+"="+value) - occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: value}) - break + value := fact.Value + if fact.NeedsNext { + if index+1 >= len(argv) { + return interpreted, spellings, occurrences, &commandmeta.Failure{Message: fmt.Sprintf("flag needs an argument: %s", original)} } - occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: "true"}) - interpreted = append(interpreted, "--"+flag.Name) - offset++ - continue - } - if suffix != "" { - value := suffix - if strings.HasPrefix(value, "=") { - value = value[1:] - } - interpreted = append(interpreted, "--"+flag.Name+"="+value) - occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: value}) - break - } - if index+1 < len(argv) { - value := argv[index+1] - interpreted = append(interpreted, "--"+flag.Name+"="+value) - occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: value}) - index += 2 + value = argv[index+1] consumedNext = true + } + if !fact.Attached && !fact.NeedsNext { + value = "true" + interpreted = append(interpreted, "--"+flag.Name) + } else { + interpreted = append(interpreted, "--"+flag.Name+"="+value) + } + occurrences = append(occurrences, commandmeta.FlagOccurrence{Name: flag.Name, Spelling: original, RawValue: value}) + if fact.NextOffset == 0 { break } - return interpreted, spellings, occurrences, &commandmeta.Failure{Message: fmt.Sprintf("flag needs an argument: %s", original)} + offset = fact.NextOffset } - if !consumedNext { + index++ + if consumedNext { index++ } continue diff --git a/internal/commandmeta/flags.go b/internal/commandmeta/flags.go index c20860b..82b6aed 100644 --- a/internal/commandmeta/flags.go +++ b/internal/commandmeta/flags.go @@ -65,3 +65,47 @@ func (lookup FlagLookup) Find(spelling string) (Flag, bool) { } return flag, ok } + +// FlagConsumption describes one flag within an option token. NextOffset is zero +// when the token is exhausted; otherwise it identifies the next short spelling. +type FlagConsumption struct { + Flag Flag + Spelling string + Found bool + Value string + Attached bool + NeedsNext bool + NextOffset int +} + +// ConsumeFlag reads a long option or a short option at offset in a cluster. +func ConsumeFlag(lookup FlagLookup, token string, offset int) FlagConsumption { + var result FlagConsumption + if ClassifyFlagToken(token) == LongFlagToken { + result.Spelling, result.Value, result.Attached = SplitLongFlag(token) + result.Flag, result.Found = lookup.Find(result.Spelling) + result.NeedsNext = result.Found && !result.Flag.Boolean && !result.Attached + return result + } + result.Spelling = "-" + token[offset:offset+1] + result.Flag, result.Found = lookup.Find(result.Spelling) + if !result.Found { + return result + } + suffix := token[offset+1:] + if result.Flag.Boolean { + if strings.HasPrefix(suffix, "=") { + result.Attached, result.Value = true, suffix[1:] + } else if suffix != "" { + result.NextOffset = offset + 1 + } + return result + } + if suffix == "" { + result.NeedsNext = true + return result + } + result.Attached = true + result.Value = strings.TrimPrefix(suffix, "=") + return result +}