Uh oh!
There was an error while loading. Please reload this page.
fix(cli): resolve three high-severity CLI consistency issues - #48420
Conversation
…ts command descriptions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Rationale: Draft. Tiny 4-line help-text normalization. No behavior change. Defer until undrafted.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48420 does not have the 'implementation' label and has only 2 new lines of code in business logic directories (threshold is 100). |
✅ Test Quality Sentinel completed test quality analysis. |
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Aligns CLI help text with top-level command listings for issue #48391.
Changes:
- Unifies
--approvehelp text. - Clarifies supported
addsources. - Aligns the
experimentsdescription.
Show a summary per file
| File | Description |
|---|---|
cmd/gh-aw/main.go | Updates run --approve help text. |
cmd/gh-aw/main_help_text_test.go | Updates help-text coverage. |
pkg/cli/add_command.go | Lists all supported workflow sources. |
pkg/cli/experiments_command.go | Aligns long and short descriptions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — No new tests
📊 Analysis Summary
Verdict: This is a CLI consistency fix focused on production code ( Verdict
|
There was a problem hiding this comment.
One blocking issue; cosmetic note alongside
The test weakening in main_help_text_test.go must be addressed before merge. The other changes (help text copy edits) are fine.
Details
Blocking — test contract weakenedassert.Equal → assert.Contains(..., "safe update changes") collapses a precise guard into a trivial substring that matches any --approve description in the codebase. The test no longer verifies the run command's description is distinct or correct.
Non-blocking — missing period
The new run --approve description ends without a period (...skip safe update enforcement), inconsistent with other flag descriptions. Trivial but worth fixing while touching the string.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 22.9 AIC · ⌖ 4.67 AIC · ⊞ 5.7K
Comment /review to run again
| @@ -21,7 +21,7 @@ func TestRunCommandHelpTextConsistency(t *testing.T) { | |||
| require.NotNil(t, runRawField, "run command should define --raw-field") | |||
| require.NotNil(t, compileApprove, "compile command should define --approve") | |||
| assert.Contains(t, compileApprove.Usage, "safe update changes", "compile --approve should describe compiler safe update approval") | |||
There was a problem hiding this comment.
Test precision gutted: assert.Equal → assert.Contains removes the precise contract on the flag description. Any string containing "safe update changes" now passes, letting regressions in the full message go silently undetected.
💡 Suggested fix
Either restore the exact string assertion or assert a meaningfully unique substring:
// Option A — restore exact contractassert.Equal(t,
"Approve all safe update changes. When strict mode is active (the default), the compiler emits warnings for new restricted secrets or unapproved action additions/removals not present in the existing gh-aw-manifest. Use this flag to approve and skip safe update enforcement",
runApprove.Usage,
"run --approve canonical description",
)
// Option B — if some flexibility is intentionalassert.Contains(t, runApprove.Usage,
"approve and skip safe update enforcement",
"run --approve must mention approving/skipping enforcement")The current substring "safe update changes" appears in every command's --approve description, so this test can no longer distinguish the run command's description from a blank or wrong one.
There was a problem hiding this comment.
The changes are clean, low-risk consistency fixes. All four --approve flag descriptions are now aligned across compile, run, update, and upgrade. The add and experiments long descriptions now match their Short fields. The test change from Equal to Contains is appropriate and still validates intent.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18 AIC · ⌖ 5.13 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — commenting with two targeted observations; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test assertion weakened:
Equal→Containsreduces the test's locking power. If the canonical string is now settled, consider assertingrunApprove.Usage == compileApprove.Usageinstead. - Missing trailing period: The new
--approveflag description inmain.gobreaks punctuation consistency with neighbouring flags.
Positive Highlights
- ✅ All four
--approvedescriptions now share consistent semantics — good cross-command alignment. - ✅
add_command.golong description now accurately reflects all three input sources, eliminating the Short/Long mismatch. - ✅
experiments_command.gofirst-line alignment is clean and surgical. - ✅ Small, focused diff — easy to review and revert if needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.4 AIC · ⌖ 6.52 AIC · ⊞ 6.7K
Comment /matt to run again
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pelikhan
commented
Jul 27, 2026
@copilot run pr-finisher skill |
…test assertion - Add `approve bool` param to RunWorkflowInteractively and thread into RunOptions - Add `Approve` field to RunWorkflowOptions; thread into RunSpecificWorkflowInteractively - Add `approve bool` param to buildCommandString so equivalent-command output includes --approve - Update test to compare runApprove.Usage == compileApprove.Usage directly instead of both Contains Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Both review threads have been addressed in commit 3f7d5c1:
|
🤖 PR Triage
Fixes CLI help text consistency (--approve flag description, help listing). Small cosmetic change (35 adds, 11 dels, 6 files). Low-value polish — defer.
|
Uh oh!
There was an error while loading. Please reload this page.
🎉 This pull request is included in a new release. Release: |
Automated CLI consistency inspection flagged three high-severity divergences between command-level help text and the main
gh aw --helplisting.Changes
--approveflag description unified acrossrun,compile,update,upgrade(cmd/gh-aw/main.go)runhad a bespoke description scoped to--push-triggered recompile; all four commands now share the canonical description used bycompile/update/upgrade. Test updated fromEqualtoContainsto avoid re-encoding the exact string.addlong description now mentions all three input sources (pkg/cli/add_command.go)First line changed from
"…from repositories…"to"…from repositories, local files, or URLs…", matching theShortfield and the main listing.experimentslong description aligned with itsShort(pkg/cli/experiments_command.go)First line changed from
"Explore ongoing experiments in the repository."to"List and analyze experiment workflow branches in the repository.", matching theShortfield exactly.run: https://github.com/github/gh-aw/actions/runs/30308333914
Run: https://github.com/github/gh-aw/actions/runs/30311684823
Run: https://github.com/github/gh-aw/actions/runs/30316592253