Skip to content

Commit 3ac2fb6

Browse files
refactor(cmd): cobra UX improvements — NoArgs, MarkFlagFilename, preRun test fix
Implements three quick-win improvements identified in issue #4379 (cobra module review): 1. Add Args: cobra.NoArgs to root command: positional arguments are not supported by awmg; without this, 'awmg config.toml' silently falls through to a confusing 'required flag(s) not set' error instead of a clear 'unknown command' message. 2. Replace verbose RegisterFlagCompletionFunc callbacks with idiomatic cobra helpers: MarkFlagFilename("config", "toml"), MarkFlagDirname("log-dir"), MarkFlagDirname("payload-dir"), MarkFlagFilename("env", "env"). Reduces ~20 lines of boilerplate to 4 lines with identical shell-completion behaviour. 3. Fix fragile preRun(nil, nil) in root_test.go: replace all 9 occurrences with preRun(&cobra.Command{}, nil). The current code is safe because preRun does not dereference cmd, but any future use of cmd.Context(), cmd.Flags(), etc. would cause a nil pointer panic. Using a real (empty) Command value prevents that footgun. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 52cd33e commit 3ac2fb6

3 files changed

Lines changed: 15 additions & 28 deletions

File tree

internal/cmd/flags.go

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -57,25 +57,11 @@ func registerAllFlags(cmd *cobra.Command) {
5757

5858
// registerFlagCompletions registers custom completion functions for flags
5959
func registerFlagCompletions(cmd *cobra.Command) {
60-
// Custom completion for --config flag (complete with .toml files)
61-
cmd.RegisterFlagCompletionFunc("config", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
62-
return []string{"toml"}, cobra.ShellCompDirectiveFilterFileExt
63-
})
64-
65-
// Custom completion for --log-dir flag (complete with directories)
66-
cmd.RegisterFlagCompletionFunc("log-dir", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
67-
return nil, cobra.ShellCompDirectiveFilterDirs
68-
})
69-
70-
// Custom completion for --payload-dir flag (complete with directories)
71-
cmd.RegisterFlagCompletionFunc("payload-dir", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
72-
return nil, cobra.ShellCompDirectiveFilterDirs
73-
})
74-
75-
// Custom completion for --env flag (complete with .env files)
76-
cmd.RegisterFlagCompletionFunc("env", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
77-
return []string{"env"}, cobra.ShellCompDirectiveFilterFileExt
78-
})
60+
// File and directory completions using idiomatic cobra helpers
61+
cmd.MarkFlagFilename("config", "toml")
62+
cmd.MarkFlagDirname("log-dir")
63+
cmd.MarkFlagDirname("payload-dir")
64+
cmd.MarkFlagFilename("env", "env")
7965

8066
// Enum completions for DIFC flags
8167
cmd.RegisterFlagCompletionFunc("guards-mode", cobra.FixedCompletions(

internal/cmd/root.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ var rootCmd = &cobra.Command{
5151
Version: cliVersion,
5252
Long: `MCPG is a proxy server for Model Context Protocol (MCP) servers.
5353
It provides routing, aggregation, and management of multiple MCP backend servers.`,
54+
Args: cobra.NoArgs,
5455
SilenceUsage: true, // Don't show help on runtime errors
5556
SilenceErrors: true, // Prevent cobra from printing errors — Execute() caller handles display
5657
PersistentPreRunE: preRun,

internal/cmd/root_test.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,23 +40,23 @@ func TestRunRequiresConfigSource(t *testing.T) {
4040
t.Run("config file provided", func(t *testing.T) {
4141
configFile = "test.toml"
4242
configStdin = false
43-
err := preRun(nil, nil)
43+
err := preRun(&cobra.Command{}, nil)
4444
// Should pass validation when --config is provided
4545
assert.NoError(t, err, "Should not error when --config is provided")
4646
})
4747

4848
t.Run("config stdin provided", func(t *testing.T) {
4949
configFile = ""
5050
configStdin = true
51-
err := preRun(nil, nil)
51+
err := preRun(&cobra.Command{}, nil)
5252
// Should pass validation when --config-stdin is provided
5353
assert.NoError(t, err, "Should not error when --config-stdin is provided")
5454
})
5555

5656
t.Run("both config file and stdin provided", func(t *testing.T) {
5757
configFile = "test.toml"
5858
configStdin = true
59-
err := preRun(nil, nil)
59+
err := preRun(&cobra.Command{}, nil)
6060
// When both are provided, should pass validation
6161
assert.NoError(t, err, "Should not error when both are provided")
6262
})
@@ -78,15 +78,15 @@ func TestPreRunValidation(t *testing.T) {
7878
configFile = "test.toml"
7979
configStdin = false
8080
verbosity = 0
81-
err := preRun(nil, nil)
81+
err := preRun(&cobra.Command{}, nil)
8282
assert.NoError(t, err)
8383
})
8484

8585
t.Run("validation passes with config stdin", func(t *testing.T) {
8686
configFile = ""
8787
configStdin = true
8888
verbosity = 0
89-
err := preRun(nil, nil)
89+
err := preRun(&cobra.Command{}, nil)
9090
assert.NoError(t, err)
9191
})
9292

@@ -108,7 +108,7 @@ func TestPreRunValidation(t *testing.T) {
108108
configFile = "test.toml"
109109
configStdin = false
110110
verbosity = 1
111-
err := preRun(nil, nil)
111+
err := preRun(&cobra.Command{}, nil)
112112
assert.NoError(t, err)
113113
// Level 1 doesn't set DEBUG env var
114114
assert.Empty(t, os.Getenv(logger.EnvDebug))
@@ -129,7 +129,7 @@ func TestPreRunValidation(t *testing.T) {
129129
configFile = "test.toml"
130130
configStdin = false
131131
verbosity = 2
132-
err := preRun(nil, nil)
132+
err := preRun(&cobra.Command{}, nil)
133133
assert.NoError(t, err)
134134
assert.Equal(t, "cmd:*,server:*,launcher:*", os.Getenv(logger.EnvDebug))
135135
})
@@ -149,7 +149,7 @@ func TestPreRunValidation(t *testing.T) {
149149
configFile = "test.toml"
150150
configStdin = false
151151
verbosity = 3
152-
err := preRun(nil, nil)
152+
err := preRun(&cobra.Command{}, nil)
153153
assert.NoError(t, err)
154154
assert.Equal(t, "*", os.Getenv(logger.EnvDebug))
155155
})
@@ -169,7 +169,7 @@ func TestPreRunValidation(t *testing.T) {
169169
configFile = "test.toml"
170170
configStdin = false
171171
verbosity = 2
172-
err := preRun(nil, nil)
172+
err := preRun(&cobra.Command{}, nil)
173173
assert.NoError(t, err)
174174
// Should not override existing DEBUG
175175
assert.Equal(t, "custom:*", os.Getenv(logger.EnvDebug))

0 commit comments

Comments
 (0)