Resolve CLI help consistency gaps across init, trial, mcp add, and logs#27861
Resolve CLI help consistency gaps across init, trial, mcp add, and logs#27861
init, trial, mcp add, and logs#27861Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/dfc7a04f-f6a0-4185-b149-eb4ed7d7917c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
init, trial, mcp add, and logs
There was a problem hiding this comment.
Pull request overview
Improves CLI help-text consistency across several commands (init, trial, mcp add, logs) and adds/updates tests to guard against regressions in help output.
Changes:
- Removed embedded
Usage:block frominitlong help and added a test assertion to prevent reintroduction. - Updated
trial --clone-repohelp text to match actual behavior and added a targeted help-text test. - Expanded
logs --safe-outputexamples/flag description to includenoopandreport-incomplete, with corresponding test coverage.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/trial_command.go | Updates --clone-repo flag help copy for clarity/accuracy. |
| pkg/cli/trial_command_test.go | Adds a unit test to enforce the updated --clone-repo help semantics. |
| pkg/cli/mcp_add.go | Changes --transport help text to list stdio, HTTP, Docker. |
| pkg/cli/mcp_add_test.go | Updates the transport help-text assertion to require HTTP and Docker. |
| pkg/cli/logs_command.go | Adds missing --safe-output example values and extends flag description. |
| pkg/cli/logs_command_test.go | Adds assertions ensuring new --safe-output examples and flag usage mention the added values. |
| pkg/cli/init_command.go | Removes embedded Usage: from init long help. |
| pkg/cli/init_command_test.go | Adds an assertion ensuring init long help does not include a Usage: section. |
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: 2
|
|
||
| cmd.Flags().StringVar(®istryURL, "registry", "", "MCP registry URL (default: https://api.mcp.github.com/v0.1)") | ||
| cmd.Flags().StringVar(&transportType, "transport", "", "Preferred transport type (stdio, http, docker)") | ||
| cmd.Flags().StringVar(&transportType, "transport", "", "Preferred transport type (stdio, HTTP, Docker)") |
There was a problem hiding this comment.
The --transport help text lists values "HTTP" and "Docker", but the implementation only accepts lowercase values (see createMCPToolConfig switch on preferredTransport: "stdio", "http", "docker"). As written, users following the help and passing "--transport HTTP" or "--transport Docker" will get an unsupported transport error. Either keep the help values lowercase, or normalize the flag value (e.g., strings.ToLower) and update the error message so the documented values are actually accepted.
| cmd.Flags().StringVar(&transportType, "transport", "", "Preferred transport type (stdio, HTTP, Docker)") | |
| cmd.Flags().StringVar(&transportType, "transport", "", "Preferred transport type (stdio, http, docker)") |
| func TestMCPAddTransportFlagDescriptionIncludesCapitalizedHTTPAndDocker(t *testing.T) { | ||
| cmd := NewMCPAddSubcommand() | ||
| transportFlag := cmd.Flags().Lookup("transport") | ||
| if transportFlag == nil { | ||
| t.Fatal("expected --transport flag to exist") | ||
| } | ||
|
|
||
| if !strings.Contains(transportFlag.Usage, "docker") { | ||
| t.Fatalf("expected --transport usage to include docker, got: %s", transportFlag.Usage) | ||
| if !strings.Contains(transportFlag.Usage, "HTTP") { | ||
| t.Fatalf("expected --transport usage to include HTTP, got: %s", transportFlag.Usage) | ||
| } | ||
|
|
||
| if !strings.Contains(transportFlag.Usage, "Docker") { | ||
| t.Fatalf("expected --transport usage to include Docker, got: %s", transportFlag.Usage) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test enforces capitalized "HTTP"/"Docker" in the --transport usage, but the command currently only supports lowercase transport values ("http", "docker"). Unless the implementation is updated to accept case-insensitive values, this will lock in help text that suggests invalid inputs. Consider asserting for the actual accepted tokens or updating the transport parsing to accept the capitalized forms documented in help.
See below for a potential fix:
func TestMCPAddTransportFlagDescriptionIncludesAcceptedTransportTokens(t *testing.T) {
cmd := NewMCPAddSubcommand()
transportFlag := cmd.Flags().Lookup("transport")
if transportFlag == nil {
t.Fatal("expected --transport flag to exist")
}
if !strings.Contains(transportFlag.Usage, "http") {
t.Fatalf("expected --transport usage to include http, got: %s", transportFlag.Usage)
}
if !strings.Contains(transportFlag.Usage, "docker") {
t.Fatalf("expected --transport usage to include docker, got: %s", transportFlag.Usage)
🧪 Test Quality Sentinel ReportTest Quality Score: 60/100
Test Classification DetailsView Per-Test Classifications (4 tests)
Flagged Area — Error Coverage GapAll 4 changed tests verify observable CLI help text and flag descriptions, which is exactly right for this "help consistency" PR. However, none test error paths (e.g., what happens when a command is given invalid arguments, when This isn't a blocker for this PR given the narrow scope, but future work on these commands should add error-path coverage. Test Inflation Notei️ Inflation detail (3 files exceed 2:1 ratio)
The inflation is expected here: the PR's production changes are primarily help-text tweaks (1-line flag description edits, removing an embedded section), which naturally require verbose assertions to verify. This is not harmful test inflation; the tests are appropriate in scope. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24786157643
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 60/100. Test quality is acceptable — 0% of new/modified tests are implementation tests (threshold: 30%). All 4 tests verify observable user-facing CLI behavior (help text and flag descriptions), appropriate for this consistency-fix PR. No coding-guideline violations detected.
CLI consistency inspection found four help-text mismatches across command docs and flag descriptions. This PR applies focused copy updates so help output is structurally clean, semantically accurate, and internally consistent.
init: remove duplicate/embedded usage sectionUsage:block frominitLong help so Cobra’s canonicalUsage:section is the single source shown to users.trial: clarify--clone-reposemantics--clone-repoflag help to describe its actual behavior (cloning source repo contents into the host repo) instead of framing it as an alternative to--logical-repo.mcp add: normalize transport value capitalization--transporthelp text values tostdio, HTTP, Dockerto match capitalization used elsewhere in MCP command help/prose.logs: complete--safe-outputexamplesnoop,report-incomplete) to both command examples and the--safe-outputflag description.Targeted guardrails in tests