CLI consistency: reject legacy nested gh fallback under multi-subcommand parents - #49673
Conversation
…arents Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh fallback under multi-subcommand parents
There was a problem hiding this comment.
Pull request overview
Adds a hidden Cobra guard to reject legacy nested gh invocations consistently.
Changes:
- Introduces a reusable hidden
ghguard subcommand. - Applies it to five multi-subcommand parents.
- Adds regression coverage for
gh --help.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/gh_guard_subcommand.go |
Implements the rejection guard. |
pkg/cli/secrets_command.go |
Registers the guard for secrets. |
pkg/cli/mcp.go |
Registers the guard for mcp. |
pkg/cli/project_command.go |
Registers the guard for project. |
pkg/cli/pr_command.go |
Registers the guard for pr. |
pkg/cli/env_command.go |
Registers the guard for env. |
pkg/cli/cli_consistency_help_test.go |
Tests consistent rejection across affected parents. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (53 additions found). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Review: LGTM
The change is focused, correct, and well-tested.
DisableFlagParsing: trueis the right mechanism — it ensures--helpreachesRunEinstead of being intercepted by Cobra's flag parser, which is exactly what blocks the silent parent-help fallback.- Error message format matches the test assertions exactly.
- All 5 affected command groups are covered by the regression test.
- No security or correctness concerns.
Minor nit (non-blocking): The RunE closure names the args parameter args []string but never uses it. Consider renaming to _ for clarity, e.g. func(cmd *cobra.Command, _ []string) error {.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.8 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Coverage scope:
experiments(2 subcommands) is the one remaining multi-subcommand parent that does not yet have the guard. Worth adding for consistency. - Test breadth: Tests confirm the
["gh", "--help"]path;["gh", "subcommand"]with extra args is not exercised.
Positive Highlights
- ✅ Excellent single-responsibility abstraction: one tiny file, one reusable guard, wired uniformly into five commands.
- ✅
DisableFlagParsing: trueis the right knob — prevents Cobra from consuming--helpbefore theRunEfires. - ✅ Regression test is table-driven, covers all affected commands, and reads as a clear specification.
- ✅ Zero deletions — purely additive, low risk.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.7 AIC · ⌖ 7.86 AIC · ⊞ 7.1K
Comment /matt to run again
| Use: "gh", | ||
| Hidden: true, | ||
| DisableFlagParsing: true, | ||
| RunE: func(cmd *cobra.Command, args []string) error { |
There was a problem hiding this comment.
[/diagnosing-bugs] The args parameter is unused — name it _ per Go convention to signal it is intentionally discarded and avoid linter warnings.
💡 Suggested fix
RunE: func(cmd *cobra.Command, _ []string) error {@copilot please address this.
There was a problem hiding this comment.
Updated in dfef025ce8: RunE now uses _ []string to intentionally discard the unused args parameter.
|
|
||
| err := cmd.Execute() | ||
| require.Error(t, err) | ||
| assert.Equal(t, `unknown command "gh" for "`+tt.cmdName+`"`, err.Error()) |
There was a problem hiding this comment.
[/tdd] The test only verifies the error message for the first positional arg (gh), not for deeper paths like gh subcommand --help. Consider adding a case to confirm that gh aw <parent> gh subcommand also returns an error, guarding against args being passed to the guard command.
💡 Example additional table row
{name: "secrets gh with args", newCmd: NewSecretsCommand, cmdName: "secrets", args: []string{"gh", "set", "--help"}},With the current DisableFlagParsing: true this should still error; a test would lock in that guarantee.
@copilot please address this.
There was a problem hiding this comment.
Addressed in dfef025ce8: added a nested-args regression case (gh set --help) in TestLegacyNestedGHHelpIsRejected to verify the guard still errors when extra positional args are present.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
|
@copilot
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented fixes in commit |
🔍 Triage Summary
CLI fix rejecting legacy nested gh fallback for multi-subcommand parents. CI green, approved by automated reviewer. Automated triage — see the triage report for full context.
|
Manual CLI review found that
gh aw <parent> gh --help(secrets|mcp|project|pr|env) rendered parent help, creating a hidden/duplicate-command impression and inconsistent unknown-command behavior. This change makes those invocations fail explicitly as unknown commands.Behavior fix: block legacy nested
ghpathghguard subcommand that always returns an unknown-command error for the parent command path.gh --helpas parent help for these command groups.Scope of application
secretsmcpprojectprenvRegression coverage
gh --helpunder each affected parent returns:unknown command "gh" for "<parent>"