Apply cobra best practices for testability and conventions

.github/workflows/ci.yml

@@ -84,7 +84,7 @@ jobs:
         run: make lint
 
       - name: Run golangci-lint
-        uses: golangci/golangci-lint-action@ec5d18412c0aeab7936cb16880d708ba2a64e1ae # v6.2.0
+        uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0
         with:
           version: v2.8.0
           args: --timeout=5m

.golangci.yml

@@ -19,38 +19,34 @@ linters:
     - revive
     - testifylint
     - gosec
-
-linters-settings:
-  testifylint:
-    enable-all: true
   # Configuration for disabled linters is kept for reference if they are re-enabled
-  errcheck:
-    exclude-functions:
-      - (*os.File).Close
-      - (*os.File).Sync
-      - os.Chdir
-      - os.Chmod
-      - os.Chtimes
-      - os.MkdirAll
-      - os.Remove
-      - os.RemoveAll
-      - os.WriteFile
-  gocritic:
-    enable-all: true
-    disabled-checks:
-      - ifElseChain
-      - singleCaseSwitch
-      - appendAssign
-      - unlambda
-      - elseif
-      - assignOp
-      - argOrder
-      - dupBranchBody
-      - deprecatedComment
-      - commentFormatting
-      - badCall
-
-issues:
-  exclude-generated: lax
-  exclude-dirs-use-default: false
-  exclude-use-default: false
+  settings:
+    testifylint:
+      enable-all: true
+    errcheck:
+      exclude-functions:
+        - (*os.File).Close
+        - (*os.File).Sync
+        - os.Chdir
+        - os.Chmod
+        - os.Chtimes
+        - os.MkdirAll
+        - os.Remove
+        - os.RemoveAll
+        - os.WriteFile
+    gocritic:
+      enable-all: true
+      disabled-checks:
+        - ifElseChain
+        - singleCaseSwitch
+        - appendAssign
+        - unlambda
+        - elseif
+        - assignOp
+        - argOrder
+        - dupBranchBody
+        - deprecatedComment
+        - commentFormatting
+        - badCall
+  exclusions:
+    generated: lax

internal/cmd/completion.go

@@ -2,7 +2,6 @@ package cmd
 
 import (
 	"fmt"
-	"os"
 
 	"github.com/spf13/cobra"
 )
@@ -51,15 +50,16 @@ PowerShell:
 		ValidArgs:             []string{"bash", "zsh", "fish", "powershell"},
 		Args:                  cobra.MatchAll(cobra.ExactArgs(1), cobra.OnlyValidArgs),
 		RunE: func(cmd *cobra.Command, args []string) error {
+			out := cmd.OutOrStdout()
 			switch args[0] {
 			case "bash":
-				return cmd.Root().GenBashCompletionV2(os.Stdout, true)
+				return cmd.Root().GenBashCompletionV2(out, true)
 			case "zsh":
-				return cmd.Root().GenZshCompletion(os.Stdout)
+				return cmd.Root().GenZshCompletion(out)
 			case "fish":
-				return cmd.Root().GenFishCompletion(os.Stdout, true)
+				return cmd.Root().GenFishCompletion(out, true)
 			case "powershell":
-				return cmd.Root().GenPowerShellCompletionWithDesc(os.Stdout)
+				return cmd.Root().GenPowerShellCompletionWithDesc(out)
 			default:
 				// This default case should never be reached due to Args validation
 				// above, but is included for defensive programming.

internal/cmd/flags.go

@@ -76,15 +76,12 @@ func registerFlagCompletions(cmd *cobra.Command) {
 	if err := cmd.MarkFlagFilename("config", "toml"); err != nil {
 		debugLog.Printf("Failed to register --config filename completion: %v", err)
 	}
-	cmd.RegisterFlagCompletionFunc("log-dir", func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) {
-		return nil, cobra.ShellCompDirectiveFilterDirs
-	})
-	cmd.RegisterFlagCompletionFunc("payload-dir", func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) {
-		return nil, cobra.ShellCompDirectiveFilterDirs
-	})
-	cmd.RegisterFlagCompletionFunc("wasm-cache-dir", func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) {
-		return nil, cobra.ShellCompDirectiveFilterDirs
-	})
+	// Use MarkFlagDirname for directory flags (cobra best practice)
+	for _, dirFlag := range []string{"log-dir", "payload-dir", "wasm-cache-dir"} {
+		if err := cmd.MarkFlagDirname(dirFlag); err != nil {
+			debugLog.Printf("Failed to register --%s dirname completion: %v", dirFlag, err)
+		}
+	}
 	// Show all files for --env — the canonical .env file has no extension.
 	if err := cmd.MarkFlagFilename("env"); err != nil {
 		debugLog.Printf("Failed to register --env filename completion: %v", err)

internal/cmd/flags_test.go

@@ -117,31 +117,31 @@ func TestRegisterFlagCompletions(t *testing.T) {
 	t.Run("log-dir flag completion returns directory filter", func(t *testing.T) {
 		cmd := setupCmd(t)
 
-		compFunc, ok := cmd.GetFlagCompletionFunc("log-dir")
-		require.True(t, ok, "log-dir flag should have a completion function")
-		_, directive := compFunc(cmd, nil, "")
-		assert.Equal(t, cobra.ShellCompDirectiveFilterDirs, directive,
-			"log-dir flag should have directory completion directive")
+		flag := cmd.Flags().Lookup("log-dir")
+		require.NotNil(t, flag, "log-dir flag should be registered")
+		require.NotNil(t, flag.Annotations, "log-dir flag should have dirname annotations")
+		_, ok := flag.Annotations[cobra.BashCompSubdirsInDir]
+		assert.True(t, ok, "log-dir flag should have directory completion annotation via MarkFlagDirname")
 	})
 
 	t.Run("payload-dir flag completion returns directory filter", func(t *testing.T) {
 		cmd := setupCmd(t)
 
-		compFunc, ok := cmd.GetFlagCompletionFunc("payload-dir")
-		require.True(t, ok, "payload-dir flag should have a completion function")
-		_, directive := compFunc(cmd, nil, "")
-		assert.Equal(t, cobra.ShellCompDirectiveFilterDirs, directive,
-			"payload-dir flag should have directory completion directive")
+		flag := cmd.Flags().Lookup("payload-dir")
+		require.NotNil(t, flag, "payload-dir flag should be registered")
+		require.NotNil(t, flag.Annotations, "payload-dir flag should have dirname annotations")
+		_, ok := flag.Annotations[cobra.BashCompSubdirsInDir]
+		assert.True(t, ok, "payload-dir flag should have directory completion annotation via MarkFlagDirname")
 	})
 
 	t.Run("wasm-cache-dir flag completion returns directory filter", func(t *testing.T) {
 		cmd := setupCmd(t)
 
-		compFunc, ok := cmd.GetFlagCompletionFunc("wasm-cache-dir")
-		require.True(t, ok, "wasm-cache-dir flag should have a completion function")
-		_, directive := compFunc(cmd, nil, "")
-		assert.Equal(t, cobra.ShellCompDirectiveFilterDirs, directive,
-			"wasm-cache-dir flag should have directory completion directive")
+		flag := cmd.Flags().Lookup("wasm-cache-dir")
+		require.NotNil(t, flag, "wasm-cache-dir flag should be registered")
+		require.NotNil(t, flag.Annotations, "wasm-cache-dir flag should have dirname annotations")
+		_, ok := flag.Annotations[cobra.BashCompSubdirsInDir]
+		assert.True(t, ok, "wasm-cache-dir flag should have directory completion annotation via MarkFlagDirname")
 	})
 
 	t.Run("env flag completion shows all files (no extension filter)", func(t *testing.T) {

internal/cmd/output.go

@@ -8,12 +8,13 @@ import (
 	"os"
 
 	"github.com/github/gh-aw-mcpg/internal/config"
+	"github.com/spf13/cobra"
 )
 
 // writeGatewayConfigToStdout writes the rewritten gateway configuration to stdout
 // per MCP Gateway Specification Section 5.4
-func writeGatewayConfigToStdout(cfg *config.Config, listenAddr, mode string, tlsEnabled bool) error {
-	return writeGatewayConfig(cfg, listenAddr, mode, tlsEnabled, os.Stdout)
+func writeGatewayConfigToStdout(cmd *cobra.Command, cfg *config.Config, listenAddr, mode string, tlsEnabled bool) error {
+	return writeGatewayConfig(cfg, listenAddr, mode, tlsEnabled, cmd.OutOrStdout())
 }
 
 func writeGatewayConfig(cfg *config.Config, listenAddr, mode string, tlsEnabled bool, w io.Writer) error {

internal/cmd/proxy.go

@@ -137,6 +137,13 @@ Local usage:
 	cmd.RegisterFlagCompletionFunc("guards-mode", cobra.FixedCompletions(
 		difc.ValidModes, cobra.ShellCompDirectiveNoFileComp))
 
+	// Use MarkFlagDirname for directory flags (cobra best practice)
+	for _, dirFlag := range []string{"log-dir", "wasm-cache-dir", "tls-dir"} {
+		if err := cmd.MarkFlagDirname(dirFlag); err != nil {
+			logProxyCmd.Printf("Failed to register --%s dirname completion: %v", dirFlag, err)
+		}
+	}
+
 	return cmd
 }
 
@@ -290,22 +297,23 @@ func runProxy(cmd *cobra.Command, args []string) error {
 			logger.LogInfo("startup", "Proxy listening on %s://%s", scheme, actualAddr)
 
 			// Print connection info
-			fmt.Fprintf(os.Stderr, "\nMCPG GitHub API Proxy\n")
-			fmt.Fprintf(os.Stderr, "  Listening: %s://%s\n", scheme, actualAddr)
-			fmt.Fprintf(os.Stderr, "  Upstream:  %s\n", apiURL)
-			fmt.Fprintf(os.Stderr, "  Mode:      %s\n", proxyDIFCMode)
-			fmt.Fprintf(os.Stderr, "  Guard:     %s\n", proxyGuardWasm)
+			stderr := cmd.ErrOrStderr()
+			fmt.Fprintf(stderr, "\nMCPG GitHub API Proxy\n")
+			fmt.Fprintf(stderr, "  Listening: %s://%s\n", scheme, actualAddr)
+			fmt.Fprintf(stderr, "  Upstream:  %s\n", apiURL)
+			fmt.Fprintf(stderr, "  Mode:      %s\n", proxyDIFCMode)
+			fmt.Fprintf(stderr, "  Guard:     %s\n", proxyGuardWasm)
 			if tlsCfg != nil {
-				fmt.Fprintf(os.Stderr, "  CA cert:   %s\n", tlsCfg.CACertPath)
-				fmt.Fprintf(os.Stderr, "\nConnect with:\n")
-				fmt.Fprintf(os.Stderr, "  export GH_HOST=%s\n", clientAddr(actualAddr))
-				fmt.Fprintf(os.Stderr, "  export NODE_EXTRA_CA_CERTS=%s\n", tlsCfg.CACertPath)
-				fmt.Fprintf(os.Stderr, "  export SSL_CERT_FILE=%s\n", tlsCfg.CACertPath)
-				fmt.Fprintf(os.Stderr, "  export GIT_SSL_CAINFO=%s\n", tlsCfg.CACertPath)
-				fmt.Fprintf(os.Stderr, "  gh issue list -R org/repo\n\n")
+				fmt.Fprintf(stderr, "  CA cert:   %s\n", tlsCfg.CACertPath)
+				fmt.Fprintf(stderr, "\nConnect with:\n")
+				fmt.Fprintf(stderr, "  export GH_HOST=%s\n", clientAddr(actualAddr))
+				fmt.Fprintf(stderr, "  export NODE_EXTRA_CA_CERTS=%s\n", tlsCfg.CACertPath)
+				fmt.Fprintf(stderr, "  export SSL_CERT_FILE=%s\n", tlsCfg.CACertPath)
+				fmt.Fprintf(stderr, "  export GIT_SSL_CAINFO=%s\n", tlsCfg.CACertPath)
+				fmt.Fprintf(stderr, "  gh issue list -R org/repo\n\n")
 			} else {
-				fmt.Fprintf(os.Stderr, "\nConnect with:\n")
-				fmt.Fprintf(os.Stderr, "  curl http://%s/repos/org/repo/issues\n\n", actualAddr)
+				fmt.Fprintf(stderr, "\nConnect with:\n")
+				fmt.Fprintf(stderr, "  curl http://%s/repos/org/repo/issues\n\n", actualAddr)
 			}
 
 			return httpServer.Serve(listener)

internal/cmd/root.go

@@ -77,6 +77,11 @@ func init() {
 	// Set custom error prefix for better branding
 	rootCmd.SetErrPrefix("MCPG Error:")
 
+	// Provide user-friendly flag parse error messages that include the usage hint
+	rootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error {
+		return fmt.Errorf("%w\nSee '%s --help' for usage", err, cmd.CommandPath())
+	})
+
 	// Set custom version template with enhanced formatting
 	rootCmd.SetVersionTemplate(`MCPG Gateway {{.Version}}
 `)
@@ -88,6 +93,10 @@ func init() {
 	// Register all flags from feature modules (flags_*.go files)
 	registerAllFlags(rootCmd)
 
+	// Preserve flag registration order in help output (cobra sorts alphabetically by default)
+	rootCmd.Flags().SortFlags = false
+	rootCmd.PersistentFlags().SortFlags = false
+
 	// Register custom flag completions
 	registerFlagCompletions(rootCmd)
 
@@ -453,7 +462,7 @@ func run(cmd *cobra.Command, args []string) error {
 	}
 
 	// Write gateway configuration to stdout per spec section 5.4
-	if err := writeGatewayConfigToStdout(cfg, listenAddr, mode, tlsEnabled); err != nil {
+	if err := writeGatewayConfigToStdout(cmd, cfg, listenAddr, mode, tlsEnabled); err != nil {
 		log.Printf("Warning: failed to write gateway configuration to stdout: %v", err)
 	}
 
@@ -479,7 +488,7 @@ func run(cmd *cobra.Command, args []string) error {
 // Execute runs the root command
 func Execute() {
 	if err := rootCmd.Execute(); err != nil {
-		fmt.Fprintln(os.Stderr, err)
+		fmt.Fprintln(rootCmd.ErrOrStderr(), err)
 		os.Exit(1)
 	}
 }