Use cobra's MarkFlagsMutuallyExclusive for AllowOnly scope flags - #11408
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds native Cobra validation for mutually exclusive AllowOnly scope flags.
Changes:
- Marks public and owner scope flags as mutually exclusive.
- Adds command-level validation tests.
Show a summary per file
| File | Description |
|---|---|
internal/cmd/flags_difc.go |
Registers the Cobra flag constraint. |
internal/cmd/flags_difc_test.go |
Adds scope-flag validation tests. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| // MCP_GATEWAY_ALLOWONLY_SCOPE_PUBLIC and MCP_GATEWAY_ALLOWONLY_SCOPE_OWNER | ||
| // set) is still validated at runtime by config.BuildAllowOnlyPolicy via | ||
| // resolveGuardPolicyFromFlags. | ||
| cmd.MarkFlagsMutuallyExclusive("allowonly-scope-public", "allowonly-scope-owner") |
| cmd.Flags().BoolVar(new(bool), "allowonly-scope-public", false, "Use public AllowOnly scope") | ||
| cmd.Flags().StringVar(new(string), "allowonly-scope-owner", "", "AllowOnly owner scope value") | ||
| cmd.MarkFlagsMutuallyExclusive("allowonly-scope-public", "allowonly-scope-owner") |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot Fix the codeql ci |
The failing CodeQL jobs both stop during |
|
@copilot fix this failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/32065441870/job/95496333438?pr=11408 |
Confirmed the specified job fails before analysis while loading the repository-level remote CodeQL config. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address review feedback |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed the review feedback in 7b3444e and bae11d0. The production registrar is now directly tested, valid CLI clearing of meaningful environment scope defaults is preserved, and empty environment values retain Cobra mutual-exclusion validation. |
🔒 mcpg Read-Only Stress — gVisorSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: INCONCLUSIVE
No writes leaked. All probes were either refused (tool-not-found or no-auth) or blocked before reaching GitHub. Run: §32152954561
|
🔒 mcpg Read-Only Stress — default AWFSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: INCONCLUSIVE
References: §32152954245
|
🔒 mcpg Read-Only Stress — docker-sbxSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: INCONCLUSIVE
No write leaked. No data was created or modified. Run: §32152954327
|
A go-fan review of
spf13/cobrausage flagged that--allowonly-scope-publicand--allowonly-scope-ownerare mutually-exclusive AllowOnly scope variants validated only in custom Go code (config.BuildAllowOnlyPolicy), rather than surfaced natively by cobra as is already done for--routed/--unifiedand--tls-cert/--tls-key.Changes
internal/cmd/flags_difc.go: registercmd.MarkFlagsMutuallyExclusive("allowonly-scope-public", "allowonly-scope-owner")alongside the existing flag definitions, so conflicting CLI flags fail fast during parsing with cobra's standard error/usage output.config.BuildAllowOnlyPolicyis left in place — it's still the only guard for the equivalent conflict when driven by environment variables (MCP_GATEWAY_ALLOWONLY_SCOPE_PUBLIC/MCP_GATEWAY_ALLOWONLY_SCOPE_OWNER), which cobra's flag-parsing constraint can't see.internal/cmd/flags_difc_test.go: new tests mirroring the existing TLSMarkFlagsRequiredTogethercoverage — both flags together are rejected, either alone is accepted, and neither is accepted.Example:
Other suggestions from the review (e.g.
Flag.Deprecatedon CLI flags, a lookup-map refactor incompletion.go) were called out as optional/low-priority with no urgent action needed, so they're left out of this change to keep it focused.