fix: address 7 CLI consistency issues from automated inspection#43802
Conversation
…, fix wording Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates gh aw CLI help text and interactive add-wizard wiring to resolve consistency findings from an automated checker, primarily by aligning flag documentation/defaults and ensuring add-wizard exposes the same relevant flags as add.
Changes:
- Added
--appendand--no-security-scannerflags toadd-wizard, and threaded them throughAddInteractiveConfigintoAddOptions. - Standardized help text wording/formatting across commands (
upgrade,audit,audit-diff,logs,env,init,project). - Clarified artifact default behavior in
audit/logshelp output.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/upgrade_command.go | Aligns --pre-releases example wording with flag spelling and description intent. |
| pkg/cli/project_command.go | Removes trailing whitespace in examples. |
| pkg/cli/logs_command.go | Clarifies --artifacts default meaning in help text. |
| pkg/cli/init_command.go | Fixes column alignment in examples. |
| pkg/cli/env_command.go | Clarifies that file.yml default is in the current directory. |
| pkg/cli/audit.go | Clarifies --artifacts default behavior/rationale in help text. |
| pkg/cli/audit_diff_command.go | Same as audit.go for audit-diff help text consistency. |
| pkg/cli/add_wizard_command.go | Adds --append / --no-security-scanner flags and passes values into interactive config. |
| pkg/cli/add_interactive_orchestrator.go | Extends AddInteractiveConfig to carry append/scanner settings through the wizard flow. |
| pkg/cli/add_interactive_git.go | Threads AppendText and DisableSecurityScanner from wizard config into AddOptions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/10 changed files
- Comments generated: 1
- Review effort level: Low
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43802 does not have the 'implementation' label and has only 41 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnosing-bugs, and /codebase-design — requesting changes on two issues, one of which (the DisableSecurityScanner override) is a functional correctness concern.
📋 Key Themes & Highlights
Key Themes
- Incomplete fix (functional):
add_workflow_pr.gohard-codesprOpts.DisableSecurityScanner = false, silently nullifying the new--no-security-scannerflag in the wizard's PR-creation path. The flag will appear to work inaddbut do nothing inadd-wizard. This is the most important issue. - Test coverage gap: The new flags are wired up correctly but have zero test assertions. The previous bug — hardcoded
""/false— is exactly the category of regression that flag-presence tests catch. Two small tests would close this gap. - Flag description parity: Descriptions are identical right now (✅), but there's no test to keep them in sync as the codebase evolves.
Positive Highlights
- ✅ Clean struct extension with well-placed comments in
AddInteractiveConfig - ✅ All documentation changes are accurate and improve clarity (artifact defaults, file path wording, pre-release spelling)
- ✅ Cosmetic fixes are surgical and don't risk regressions
- ✅ Good discipline in excluding the H1 false positive with an explanation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 57.6 AIC · ⌖ 5.5 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
The changes are clean and correct. All 7 consistency fixes are well-scoped and properly implemented.
M3 (add-wizard --append and --no-security-scanner): The most substantive change — correctly adds both fields to AddInteractiveConfig and wires them through both the wizard command (flag reads → struct init) and add_interactive_git.go (pass-through to AddOptions). Parity with the add command is exact. The wizard intentionally omits the --disable-security-scanner legacy alias, which is appropriate since this is a new flag surface.
M1/M2/M4/L1/L3 (doc/cosmetic): All accurate and self-consistent:
- Audit
--artifactsdefault clarifications are identical in bothaudit.goandaudit_diff_command.go✓ - Logs
--artifactsgains a descriptive reason phrase ✓ env_command.go"current directory" additions correctly placed in both get/update ✓upgradepre-releases wording now matches the flag name spelling ✓- Alignment and trailing-whitespace fixes ✓
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 62.5 AIC · ⌖ 5.79 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Must fix before merging
The flag propagation in M3 is incomplete — --no-security-scanner is dead code for add-wizard.
🔍 Blocking issue and non-blocking note
Blocking — --no-security-scanner is silently nullified (add_interactive_git.go:47)
add-wizard always creates a PR (CreatePR: true is hardcoded). AddResolvedWorkflows therefore always calls addWorkflowsWithPR, which unconditionally resets prOpts.DisableSecurityScanner = false before calling addWorkflowsWithTracking. The user flag is accepted, wired through the struct, but thrown away at exactly the point it would have mattered. This is not a theoretical risk — anyone using --no-security-scanner with add-wizard gets no indication the flag was ignored.
Fix: either propagate opts.DisableSecurityScanner in addWorkflowsWithPR, or reject the flag early with an explicit error (add-wizard always scans when creating a PR).
Non-blocking — new flags absent from Example block (add_wizard_command.go:130)
--append and --no-security-scanner are not illustrated in the Example field. --help output and doc generators miss them; add has examples for both. Straightforward fix.
The doc-copy, wording, and whitespace changes in the other files (M1, M2, M4, L1, L3) are fine.
🔎 Code quality review by PR Code Quality Reviewer · 105.5 AIC · ⌖ 5.7 AIC · ⊞ 5.5K
Comment /review to run again
🤖 PR Triage
Summary: 7 CLI consistency fixes (missing flags on
|
|
@copilot please run the
|
No additional code changes were needed. I replied directly on the unresolved |
|
@copilot please run the
|
🤖 PR Triage — Run §28834001067
Score Breakdown:
CI: CHANGES_REQUESTED → APPROVED (dismissed) by Rationale: 7 user-visible CLI fixes. Non-breaking. Fast-track for human review. Labels: already applied from prior run.
|
I verified the remaining review threads are resolved and reran local validation on |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
@copilot please run the
|
No additional code changes were needed. On current HEAD |
|
@copilot please run the
|
No further code changes were needed on |
|
🎉 This pull request is included in a new release. Release: |
Automated CLI consistency checker found 8 issues across help text and documentation. Addresses 7 of them (H1 is a false positive —
github.github.comis the intentional configured docs host, validated by existing tests).Changes
M3 —
add-wizardmissing--appendand--no-security-scannerflagsadd-wizard, matching their counterparts inaddAppendTextandDisableSecurityScannerfields toAddInteractiveConfigadd_interactive_git.gonow passes these through toAddOptionsinstead of hardcoding empty/falseM2 —
upgrade --pre-releasesexample wording"prerelease"→"pre-release"(matches flag name spelling)"self-upgrading the extension"→"upgrading the extension"(matches flag description wording)M1 —
audit/logs--artifactsdefault mismatch undocumentedlogs:"default: usage — compact summary for faster downloads"(also fixes L2 terminology gap)audit+audit-diff:"default: all, because auditing requires comprehensive artifacts for analysis"M4 —
env get/env updatefile.ymlplaceholder ambiguity"writes to file.yml"→"writes to file.yml in the current directory"(literal, not a placeholder)L1/L3 — Cosmetic
init --codespaces ""example column alignment (off by one space vs aligned column)# Create org projectinproject newexamples