From 3ac2fb6b893ea4bbf4fd3dc543afbb051fd7bd87 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 23 Apr 2026 12:56:09 +0000 Subject: [PATCH] =?UTF-8?q?refactor(cmd):=20cobra=20UX=20improvements=20?= =?UTF-8?q?=E2=80=94=20NoArgs,=20MarkFlagFilename,=20preRun=20test=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- internal/cmd/flags.go | 24 +++++------------------- internal/cmd/root.go | 1 + internal/cmd/root_test.go | 18 +++++++++--------- 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go index f0209b1d7..6a38b3293 100644 --- a/internal/cmd/flags.go +++ b/internal/cmd/flags.go @@ -57,25 +57,11 @@ func registerAllFlags(cmd *cobra.Command) { // registerFlagCompletions registers custom completion functions for flags func registerFlagCompletions(cmd *cobra.Command) { - // Custom completion for --config flag (complete with .toml files) - cmd.RegisterFlagCompletionFunc("config", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - return []string{"toml"}, cobra.ShellCompDirectiveFilterFileExt - }) - - // Custom completion for --log-dir flag (complete with directories) - cmd.RegisterFlagCompletionFunc("log-dir", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - return nil, cobra.ShellCompDirectiveFilterDirs - }) - - // Custom completion for --payload-dir flag (complete with directories) - cmd.RegisterFlagCompletionFunc("payload-dir", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - return nil, cobra.ShellCompDirectiveFilterDirs - }) - - // Custom completion for --env flag (complete with .env files) - cmd.RegisterFlagCompletionFunc("env", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - return []string{"env"}, cobra.ShellCompDirectiveFilterFileExt - }) + // File and directory completions using idiomatic cobra helpers + cmd.MarkFlagFilename("config", "toml") + cmd.MarkFlagDirname("log-dir") + cmd.MarkFlagDirname("payload-dir") + cmd.MarkFlagFilename("env", "env") // Enum completions for DIFC flags cmd.RegisterFlagCompletionFunc("guards-mode", cobra.FixedCompletions( diff --git a/internal/cmd/root.go b/internal/cmd/root.go index d1c95630c..f0d60c6f5 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -51,6 +51,7 @@ var rootCmd = &cobra.Command{ Version: cliVersion, Long: `MCPG is a proxy server for Model Context Protocol (MCP) servers. It provides routing, aggregation, and management of multiple MCP backend servers.`, + Args: cobra.NoArgs, SilenceUsage: true, // Don't show help on runtime errors SilenceErrors: true, // Prevent cobra from printing errors — Execute() caller handles display PersistentPreRunE: preRun, diff --git a/internal/cmd/root_test.go b/internal/cmd/root_test.go index bacd128e5..2fd94688d 100644 --- a/internal/cmd/root_test.go +++ b/internal/cmd/root_test.go @@ -40,7 +40,7 @@ func TestRunRequiresConfigSource(t *testing.T) { t.Run("config file provided", func(t *testing.T) { configFile = "test.toml" configStdin = false - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) // Should pass validation when --config is provided assert.NoError(t, err, "Should not error when --config is provided") }) @@ -48,7 +48,7 @@ func TestRunRequiresConfigSource(t *testing.T) { t.Run("config stdin provided", func(t *testing.T) { configFile = "" configStdin = true - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) // Should pass validation when --config-stdin is provided assert.NoError(t, err, "Should not error when --config-stdin is provided") }) @@ -56,7 +56,7 @@ func TestRunRequiresConfigSource(t *testing.T) { t.Run("both config file and stdin provided", func(t *testing.T) { configFile = "test.toml" configStdin = true - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) // When both are provided, should pass validation assert.NoError(t, err, "Should not error when both are provided") }) @@ -78,7 +78,7 @@ func TestPreRunValidation(t *testing.T) { configFile = "test.toml" configStdin = false verbosity = 0 - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) assert.NoError(t, err) }) @@ -86,7 +86,7 @@ func TestPreRunValidation(t *testing.T) { configFile = "" configStdin = true verbosity = 0 - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) assert.NoError(t, err) }) @@ -108,7 +108,7 @@ func TestPreRunValidation(t *testing.T) { configFile = "test.toml" configStdin = false verbosity = 1 - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) assert.NoError(t, err) // Level 1 doesn't set DEBUG env var assert.Empty(t, os.Getenv(logger.EnvDebug)) @@ -129,7 +129,7 @@ func TestPreRunValidation(t *testing.T) { configFile = "test.toml" configStdin = false verbosity = 2 - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) assert.NoError(t, err) assert.Equal(t, "cmd:*,server:*,launcher:*", os.Getenv(logger.EnvDebug)) }) @@ -149,7 +149,7 @@ func TestPreRunValidation(t *testing.T) { configFile = "test.toml" configStdin = false verbosity = 3 - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) assert.NoError(t, err) assert.Equal(t, "*", os.Getenv(logger.EnvDebug)) }) @@ -169,7 +169,7 @@ func TestPreRunValidation(t *testing.T) { configFile = "test.toml" configStdin = false verbosity = 2 - err := preRun(nil, nil) + err := preRun(&cobra.Command{}, nil) assert.NoError(t, err) // Should not override existing DEBUG assert.Equal(t, "custom:*", os.Getenv(logger.EnvDebug))