diff --git a/cmd/openai/completion_test.go b/cmd/openai/completion_test.go new file mode 100644 index 00000000..421cc2cc --- /dev/null +++ b/cmd/openai/completion_test.go @@ -0,0 +1,67 @@ +package main + +import ( + "bytes" + "os" + "os/exec" + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestInheritedCompletionMainHelper(t *testing.T) { + if os.Getenv("OPENAI_CLI_INHERITANCE_HELPER") != "1" { + return + } + os.Args = os.Args[slices.Index(os.Args, "--")+1:] + main() + os.Exit(0) +} + +func TestInheritedCompletionMain(t *testing.T) { + binary, err := os.Executable() + require.NoError(t, err) + // Keep real credentials and environment-selected client configuration out of + // subprocesses; completion never needs an API request or credential values. + var env []string + for _, e := range os.Environ() { + if !strings.HasPrefix(e, "OPENAI_") && !strings.HasPrefix(e, "COMPLETION_STYLE=") { + env = append(env, e) + } + } + env = append(env, "OPENAI_CLI_INHERITANCE_HELPER=1", "COMPLETION_STYLE=bash") + for _, tc := range []struct { + args []string + output string + code int + }{ + {[]string{"--fo"}, "--format\n--format-error\n", 0}, + {[]string{"models", "list", "--fo"}, "--format\n--format-error\n", 0}, + {[]string{"models", "list", "--format", ""}, "", 11}, + // requestflag.Flag declares IsLocal=true, including root credential flags. + {[]string{"models", "list", "--api-"}, "", 0}, + {[]string{"models", "--format", "list", "li"}, "list\n", 0}, + {[]string{"models", "list", "-r", "--fo"}, "--format\n--format-error\n", 0}, + } { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + args := append([]string{"-test.run=^TestInheritedCompletionMainHelper$", "--", "openai", "__complete", "--"}, tc.args...) + child := exec.Command(binary, args...) + child.Env = env + var out, errs bytes.Buffer + child.Stdout = &out + child.Stderr = &errs + err := child.Run() + code := 0 + if exit, ok := err.(*exec.ExitError); ok { + code = exit.ExitCode() + } else { + require.NoError(t, err) + } + require.Equal(t, tc.code, code) + require.Equal(t, tc.output, out.String()) + require.Empty(t, errs.String()) + }) + } +} diff --git a/internal/autocomplete/autocomplete.go b/internal/autocomplete/autocomplete.go index 056bfbc6..a61b5b9e 100644 --- a/internal/autocomplete/autocomplete.go +++ b/internal/autocomplete/autocomplete.go @@ -4,6 +4,7 @@ import ( "context" "embed" "fmt" + "iter" "os" "slices" "strings" @@ -101,7 +102,7 @@ func isFlag(arg string) bool { return strings.HasPrefix(arg, "-") } -func findFlag(cmd *cli.Command, arg string) *cli.Flag { +func findFlag(cmd *cli.Command, arg string, ancestors []*cli.Command) *cli.Flag { name := strings.TrimLeft(arg, "-") for _, flag := range cmd.Flags { if vf, ok := flag.(cli.VisibleFlag); ok && !vf.IsVisible() { @@ -112,9 +113,44 @@ func findFlag(cmd *cli.Command, arg string) *cli.Flag { return &flag } } + for flag := range inheritedFlags(cmd, ancestors) { + if slices.Contains(flag.Names(), name) { + return &flag + } + } return nil } +// Match urfave's applied flags: nearest ancestors first, and any collision +// with a command's own names or aliases excludes the entire ancestor flag. +// Keep ancestry explicitly while walking raw trees, whose parent links are unset. +func inheritedFlags(cmd *cli.Command, ancestors []*cli.Command) iter.Seq[cli.Flag] { + return func(yield func(cli.Flag) bool) { + localNames := map[string]bool{} + for _, flag := range cmd.Flags { + for _, name := range flag.Names() { + localNames[name] = true + } + } + for _, ancestor := range ancestors { + flags: + for _, flag := range ancestor.Flags { + if local, ok := flag.(cli.LocalFlag); !ok || local.IsLocal() { + continue + } + for _, name := range flag.Names() { + if localNames[name] { + continue flags + } + } + if !yield(flag) { + return + } + } + } + } +} + func findChild(cmd *cli.Command, name string) *cli.Command { for _, c := range cmd.Commands { if !c.Hidden && c.Name == name { @@ -224,12 +260,13 @@ func getAllPossibleCompletions(completionStyle CompletionStyle, root *cli.Comman current := args[len(args)-1] preceding := args[0 : len(args)-1] cmd := root + ancestors := root.Lineage()[1:] i := 0 for i < len(preceding) { arg := preceding[i] if isFlag(arg) { - flag := findFlag(cmd, arg) + flag := findFlag(cmd, arg, ancestors) if flag == nil { i++ } else if docFlag, ok := (*flag).(cli.DocGenerationFlag); ok && docFlag.TakesValue() { @@ -241,6 +278,7 @@ func getAllPossibleCompletions(completionStyle CompletionStyle, root *cli.Comman } else { child := findChild(cmd, arg) if child != nil { + ancestors = append([]*cli.Command{cmd}, ancestors...) cmd = child } i++ @@ -248,10 +286,10 @@ func getAllPossibleCompletions(completionStyle CompletionStyle, root *cli.Comman } // Check if the previous arg was a flag expecting a value - if len(preceding) > 0 { + if len(preceding) > 0 && i > len(preceding) { prev := preceding[len(preceding)-1] if isFlag(prev) { - flag := findFlag(cmd, prev) + flag := findFlag(cmd, prev, ancestors) if flag != nil { if fb, ok := (*flag).(*cli.StringFlag); ok && fb.TakesFile { return CompletionResult{Completions: completions, Behavior: ShellCompletionBehaviorFile} @@ -267,6 +305,17 @@ func getAllPossibleCompletions(completionStyle CompletionStyle, root *cli.Comman for _, flag := range cmd.Flags { completions = builder.createFromFlag(current, &flag, completions) } + seen := map[string]bool{} + for flag := range inheritedFlags(cmd, ancestors) { + for _, candidate := range builder.createFromFlag(current, &flag, nil) { + if !seen[candidate.Name] { + seen[candidate.Name] = true + if visible, ok := flag.(cli.VisibleFlag); !ok || visible.IsVisible() { + completions = append(completions, candidate) + } + } + } + } } for _, child := range cmd.Commands { diff --git a/internal/autocomplete/inheritance_test.go b/internal/autocomplete/inheritance_test.go new file mode 100644 index 00000000..ef48587d --- /dev/null +++ b/internal/autocomplete/inheritance_test.go @@ -0,0 +1,120 @@ +package autocomplete + +import ( + "context" + "io" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" +) + +func inheritedTree() (*cli.Command, *cli.Command) { + leaf := &cli.Command{Name: "leaf", HideHelp: true, Flags: []cli.Flag{&cli.BoolFlag{Name: "override", Aliases: []string{"o"}}}} + middle := &cli.Command{Name: "middle", HideHelp: true, Flags: []cli.Flag{ + &cli.StringFlag{Name: "near", Aliases: []string{"shared"}, Usage: "near"}, + }, Commands: []*cli.Command{leaf, {Name: "value", HideHelp: true}}} + root := &cli.Command{Name: "test", HideHelp: true, Writer: io.Discard, ErrWriter: io.Discard, + ExitErrHandler: func(context.Context, *cli.Command, error) {}, + Flags: []cli.Flag{ + &cli.StringFlag{Name: "format", Aliases: []string{"f"}}, + &cli.StringFlag{Name: "file", TakesFile: true}, + &cli.StringFlag{Name: "private", Hidden: true}, + &cli.StringFlag{Name: "root-only", Local: true}, + &cli.StringFlag{Name: "replaced", Aliases: []string{"o"}}, + &cli.StringFlag{Name: "far", Aliases: []string{"shared"}, Usage: "far"}, + }, Commands: []*cli.Command{middle}} + return root, leaf +} + +func TestInheritedCompletion(t *testing.T) { + for _, initialized := range []bool{false, true} { + t.Run(map[bool]string{false: "raw", true: "initialized"}[initialized], func(t *testing.T) { + root, leaf := inheritedTree() + if initialized { + leaf.Action = func(context.Context, *cli.Command) error { return nil } + require.NoError(t, root.Run(context.Background(), []string{"test", "middle", "leaf"})) + } + for _, tc := range []struct { + args []string + names []string + behavior ShellCompletionBehavior + }{ + {[]string{"middle", "leaf", "--fo"}, []string{"--format"}, 0}, + {[]string{"middle", "leaf", "-f"}, []string{"-f"}, 0}, + {[]string{"middle", "leaf", "--format", ""}, nil, 11}, + {[]string{"middle", "leaf", "--format", "--file", "--fo"}, []string{"--format"}, 0}, + {[]string{"middle", "leaf", "--format=synthetic", "--fo"}, []string{"--format"}, 0}, + {[]string{"middle", "leaf", "-f", "--fo"}, nil, 11}, + {[]string{"middle", "leaf", "--file", ""}, nil, 10}, + {[]string{"middle", "leaf", "--private", "--fo"}, nil, 11}, + {[]string{"middle", "leaf", "--pr"}, nil, 0}, + {[]string{"middle", "leaf", "--root-"}, nil, 0}, + {[]string{"middle", "leaf", "--repl"}, nil, 0}, + {[]string{"middle", "leaf", "-o", "--fo"}, []string{"--format"}, 0}, + {[]string{"middle", "--format", "value", "le"}, []string{"leaf"}, 0}, + {[]string{"middle", "leaf", "--"}, []string{"--override", "--near", "--shared", "--format", "--file", "--far"}, 0}, + } { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + beforeRoot, beforeLeaf := root.Root(), leaf.Root() + rootFlags, leafFlags := append([]cli.Flag(nil), root.Flags...), append([]cli.Flag(nil), leaf.Flags...) + got := GetCompletions(CompletionStyleBash, root, tc.args) + var names []string + for _, c := range got.Completions { + names = append(names, c.Name) + } + require.Equal(t, tc.names, names) + require.Equal(t, tc.behavior, got.Behavior) + require.Same(t, beforeRoot, root.Root()) + require.Same(t, beforeLeaf, leaf.Root()) + require.Equal(t, rootFlags, root.Flags) + require.Equal(t, leafFlags, leaf.Flags) + }) + } + }) + } +} + +func TestInheritedFlagShadowing(t *testing.T) { + for _, hidden := range []bool{false, true} { + for _, parentName := range []string{"override", "global"} { + root, leaf := inheritedTree() + leaf.Flags = []cli.Flag{&cli.BoolFlag{Name: "override", Aliases: []string{"o"}, Hidden: hidden}} + root.Flags = []cli.Flag{&cli.StringFlag{Name: parentName, Aliases: []string{"o", "other"}}} + // Even a hidden child declaration suppresses every parent alias. + got := GetCompletions(CompletionStyleBash, root, []string{"middle", "leaf", "--other"}) + require.Empty(t, got.Completions) + } + } +} + +// Pin the parser's contract: a child alias suppresses the entire parent flag, +// while overlapping inherited aliases resolve to the nearest ancestor. +func TestInheritedParserContract(t *testing.T) { + for _, tc := range []struct { + flag string + accepted bool + }{ + {"--format", true}, {"-f", true}, {"--private", true}, {"--file", true}, + {"--root-only", false}, {"--replaced", false}, {"--near", true}, {"--far", true}, {"--shared", true}, + } { + t.Run(tc.flag, func(t *testing.T) { + root, leaf := inheritedTree() + called := false + leaf.Action = func(context.Context, *cli.Command) error { called = true; return nil } + err := root.Run(context.Background(), []string{"test", "middle", "leaf", tc.flag, "synthetic"}) + if tc.accepted { + require.NoError(t, err) + require.True(t, called) + } else { + require.Error(t, err) + require.False(t, called) + } + if tc.flag == "--shared" { + require.Equal(t, "synthetic", root.Commands[0].Flags[0].(*cli.StringFlag).Get()) + require.Equal(t, "", root.Flags[5].(*cli.StringFlag).Get()) + } + }) + } +} diff --git a/internal/autocomplete/protocol_test.go b/internal/autocomplete/protocol_test.go index 907f9351..c720c47d 100644 --- a/internal/autocomplete/protocol_test.go +++ b/internal/autocomplete/protocol_test.go @@ -63,6 +63,10 @@ func TestShellCompletionProtocol(t *testing.T) { }{ {"root value", []string{"--format", "candidate-"}, 11, "", ""}, {"nested local value", []string{"models", "list", "--max-items", "candidate-"}, 11, "", ""}, + {"nested inherited name", []string{"models", "list", "--fo"}, 0, "--format\n", "--format\n"}, + {"nested inherited value", []string{"models", "list", "--format", "candidate-"}, 11, "", ""}, + {"nested inherited file", []string{"models", "list", "--file", "candidate-"}, 10, "", "candidate-fixture.txt\n"}, + {"nested explicit file prefix", []string{"models", "list", "--format", "@candidate-"}, 11, "", "@candidate-fixture.txt\n"}, {"file value", []string{"--file", "candidate-"}, 10, "", "candidate-fixture.txt\n"}, {"spaced preceding value", []string{"--format", "two words", "--file", "candidate-"}, 10, "", "candidate-fixture.txt\n"}, {"empty preceding value", []string{"--format", "", "--file", "candidate-"}, 10, "", "candidate-fixture.txt\n"},