feat(cmd): cobra improvements from go-fan module review#7630
Merged
Conversation
- Add MarkFlagsRequiredTogether("tls-cert","tls-key") in flags_tls.go;
keep manual env-var check in run() for non-CLI defaults
- Add flags_tls_test.go with cobra co-requirement tests
- Fix --env completion to show all files (no .env extension filter)
- Add cobra.EnableCommandSorting = false to preserve group ordering
- Add rootCmd.CompletionOptions.DisableDefaultCmd = true
- Improve SilenceErrors/SilenceUsage comments (cobra per-cmd vs root semantics)
- Add --shutdown-timeout flag (MCP_GATEWAY_SHUTDOWN_TIMEOUT, default 5s)
used by both root and proxy serveAndWait calls
Closes #7619
Copilot
AI
changed the title
[WIP] Review go module spf13/cobra
feat(cmd): cobra improvements from go-fan module review
Jun 16, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements several CLI usability and correctness improvements in the internal/cmd Cobra-based CLI layer, based on feedback from the go-fan module review. It focuses on making flag constraints more declarative, improving shell completion behavior, preserving intended command ordering, and introducing a configurable HTTP graceful shutdown timeout used by both gateway and proxy server paths.
Changes:
- Enforced
--tls-cert/--tls-keyco-requirement at Cobra parse time (while retaining runtime validation for env-var defaults) and added focused tests for the constraint. - Fixed
--envshell completion to allow the canonical.envfilename (no extension) and updated tests accordingly. - Introduced a
--shutdown-timeoutflag withMCP_GATEWAY_SHUTDOWN_TIMEOUTenv-var defaulting, and wired it into both root and proxy serve/shutdown flows.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/serve.go | Removes the hardcoded shutdown timeout constant in favor of a configurable timeout passed into serveAndWait. |
| internal/cmd/root.go | Adds Cobra behavior comments, disables command sorting to preserve registration order, disables default completion cmd, and uses shutdownTimeout for graceful shutdown. |
| internal/cmd/proxy.go | Uses the shared shutdownTimeout when running the proxy server via serveAndWait. |
| internal/cmd/flags.go | Fixes --env filename completion to show all files (including .env with no extension). |
| internal/cmd/flags_tls.go | Adds MarkFlagsRequiredTogether("tls-cert","tls-key") to enforce TLS flag pairing at parse time. |
| internal/cmd/flags_tls_test.go | Adds tests validating Cobra’s required-together TLS behavior and verifies TLS flags are registered. |
| internal/cmd/flags_test.go | Updates completion tests to expect no extension filter for --env. |
| internal/cmd/flags_serve.go | Adds the new --shutdown-timeout flag (with env-var default) backing shutdownTimeout. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 4
|
|
||
| func init() { | ||
| RegisterFlag(func(cmd *cobra.Command) { | ||
| cmd.Flags().DurationVar( |
Comment on lines
463
to
+467
| if err := serveAndWait( | ||
| ctx, | ||
| cancel, | ||
| httpServer, | ||
| httpServerShutdownTimeout, | ||
| shutdownTimeout, |
Comment on lines
268
to
+272
| err = serveAndWait( | ||
| ctx, | ||
| cancel, | ||
| httpServer, | ||
| httpServerShutdownTimeout, | ||
| shutdownTimeout, |
Comment on lines
+24
to
+28
| // Cert and key must always be provided together when set via CLI flags. | ||
| // Note: env-var defaults (MCP_GATEWAY_TLS_CERT/MCP_GATEWAY_TLS_KEY) are | ||
| // validated at runtime in run() since MarkFlagsRequiredTogether only fires | ||
| // when flags are explicitly changed on the command line. | ||
| cmd.MarkFlagsRequiredTogether("tls-cert", "tls-key") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the cobra improvement recommendations from the go-fan module review: declarative flag constraints, corrected shell completion, command ordering, and a configurable shutdown timeout.
TLS flag co-requirement (
flags_tls.go)cmd.MarkFlagsRequiredTogether("tls-cert", "tls-key")— cobra now rejects mismatched CLI flags at parse time with a consistent error messagehasCert != hasKeycheck inrun()is kept —MarkFlagsRequiredTogetheronly fires when flags are explicitly changed on the command line; it does not coverMCP_GATEWAY_TLS_CERT/MCP_GATEWAY_TLS_KEYenv-var defaultsNew
flags_tls_test.gocovers cert-only, key-only, both, and neither cases.--envcompletion fix (flags.go)MarkFlagFilename("env", "env")→MarkFlagFilename("env"): was filtering to*.envfiles, missing the canonical.envfile (which has no extension)Command sorting (
root.go)cobra.EnableCommandSorting = false— preserves the intentionalAddGroupregistration order within each group (cobra was overriding it alphabetically)rootCmd.CompletionOptions.DisableDefaultCmd = true— makes intent explicit; a customcompletioncommand is already providedSilenceErrors/SilenceUsagecomments (root.go)SilenceErrorsis read from the root command for all subcommands;SilenceUsageis read from the specific subcommandConfigurable shutdown timeout (
flags_serve.go,serve.go)const httpServerShutdownTimeout = 5 * time.Secondwith a--shutdown-timeoutflag (default5s) andMCP_GATEWAY_SHUTDOWN_TIMEOUTenv-var override, following the existingFlagRegistrarpatternrun()and proxyrunProxy()paths