Apply cobra best practices for testability and conventions by lpcox · Pull Request #8347 · github/gh-aw-mcpg · GitHub
Skip to content

Apply cobra best practices for testability and conventions#8347

Merged
lpcox merged 4 commits into
mainfrom
lpcox-fix-cobra-best-practices
Jun 30, 2026
Merged

Apply cobra best practices for testability and conventions#8347
lpcox merged 4 commits into
mainfrom
lpcox-fix-cobra-best-practices

Conversation

@lpcox

@lpcox lpcox commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Applies cobra maintainer-recommended best practices to the CLI code in internal/cmd/, improving testability and following conventions without changing any runtime behavior.

Fixes #8327

Changes

1. cmd.ErrOrStderr() / cmd.OutOrStdout() over direct os.Stderr / os.Stdout

Cobra provides cmd.ErrOrStderr() and cmd.OutOrStdout() methods that return the command's configured output writers, defaulting to os.Stderr/os.Stdout. Using these instead of direct os references allows tests to capture and verify output by calling cmd.SetOut() / cmd.SetErr() without monkey-patching global state.

Files changed:

  • root.go — Execute() now uses rootCmd.ErrOrStderr() for error output
  • completion.go — Shell completion output uses cmd.OutOrStdout() instead of os.Stdout
  • output.go — writeGatewayConfigToStdout() accepts *cobra.Command and uses cmd.OutOrStdout()
  • proxy.go — Connection info banner uses cmd.ErrOrStderr() instead of os.Stderr

2. MarkFlagDirname() for directory flags

Replaces manual RegisterFlagCompletionFunc closures that returned cobra.ShellCompDirectiveFilterDirs with cobra's built-in MarkFlagDirname(), which is the idiomatic way to indicate a flag accepts directory paths.

Flags updated:

  • Root command: --log-dir, --payload-dir, --wasm-cache-dir
  • Proxy subcommand: --log-dir, --wasm-cache-dir, --tls-dir

3. SortFlags = false on root command FlagSet

Sets SortFlags = false on both Flags() and PersistentFlags() to preserve the intentional flag registration order in help output, matching the existing cobra.EnableCommandSorting = false for commands.

4. SetFlagErrorFunc for user-friendly flag parse error messages

Adds a custom flag error function that appends a See '<command> --help' for usage hint to flag parse errors, making it easier for users to recover from typos or missing values.

Testing

  • Updated flags_test.go to verify MarkFlagDirname annotations (via cobra.BashCompSubdirsInDir) instead of checking for completion functions
  • All existing tests pass — make agent-finished ✓

@lpcox
Apply cobra best practices for testability and conventions
15fc18d 
- Use cmd.ErrOrStderr()/cmd.OutOrStdout() instead of direct os.Stderr/os.Stdout
  references in root.go (Execute), completion.go, output.go, and proxy.go for
  improved testability
- Replace manual RegisterFlagCompletionFunc with MarkFlagDirname() for directory
  flags (log-dir, payload-dir, wasm-cache-dir, tls-dir) per cobra conventions
- Add SetFlagErrorFunc on root command for user-friendly flag parse error
  messages that include usage hints
- Set SortFlags = false on root command's Flags and PersistentFlags to preserve
  flag registration order in help output
- Update tests to verify MarkFlagDirname annotations instead of completion funcs

Fixes #8327

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 30, 2026 13:30
GitHub Advanced Security started work on behalf of lpcox June 30, 2026 13:30 View session
GitHub Advanced Security finished work on behalf of lpcox June 30, 2026 13:31
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the internal/cmd/ Cobra-based CLI wiring to follow maintainer-recommended patterns that improve testability and help/UX consistency, without changing intended runtime behavior.

Changes:

  • Route CLI output through Cobra’s configured writers (cmd.OutOrStdout() / cmd.ErrOrStderr()) instead of direct os.Stdout/os.Stderr.
  • Replace manual “directory-only” completion functions with MarkFlagDirname() annotations and preserve declared flag ordering in help output.
  • Add a SetFlagErrorFunc to append a usage hint to flag-parse errors.
Show a summary per file
File Description
internal/cmd/root.go Adds flag parse error handler, disables flag sorting, and switches root execution error output to ErrOrStderr(); passes cmd into stdout writer helper.
internal/cmd/proxy.go Switches proxy banner output to cmd.ErrOrStderr() and annotates directory flags via MarkFlagDirname().
internal/cmd/output.go Makes gateway-config stdout writer injectable via *cobra.Command (OutOrStdout()).
internal/cmd/flags.go Replaces manual directory completion closures with MarkFlagDirname() plus error logging.
internal/cmd/flags_test.go Updates completion tests to assert MarkFlagDirname() annotations rather than completion functions.
internal/cmd/completion.go Uses cmd.OutOrStdout() for generated shell completion output.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread internal/cmd/proxy.go
@lpcox
Potential fix for pull request finding
3539a39 
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
GitHub Advanced Security started work on behalf of lpcox June 30, 2026 13:34 View session
GitHub Advanced Security finished work on behalf of lpcox June 30, 2026 13:34
@lpcox

lpcox commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

@copilot fix the failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/28448349690/job/84303548609?pr=8347

Copilot AI commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

@copilot fix the failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/28448349690/job/84303548609?pr=8347

The lint job was failing because golangci/golangci-lint-action@v6.2.0 uses Node.js 20, which GitHub Actions now runs on Node.js 24 by default — causing the binary download to return 404. Fixed in 27bf08d by updating the action to v9.3.0 (built for node24), keeping the same v2.8.0 golangci-lint version.

Copilot finished work on behalf of lpcox June 30, 2026 13:41
@lpcox

lpcox commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

@copilot fix failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/28448828457/job/84307702581?pr=8347

Copilot AI commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Copilot finished work on behalf of lpcox June 30, 2026 14:02
@github-actions github-actions Bot mentioned this pull request Jun 30, 2026
Closed
@lpcox lpcox merged commit 2af6d3e into main Jun 30, 2026
25 checks passed
@lpcox lpcox deleted the lpcox-fix-cobra-best-practices branch June 30, 2026 14:59
This was referenced Jun 30, 2026
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[go-fan] Go Module Review: spf13/cobra

3 participants

@lpcox