Align CLI docs and deprecation behavior for audit, status, experiments, and legacy --disable-* flags#44312
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
--disable-* flags
🤖 PR Triage
Status: DRAFT — needs undraft + CI before review. CLI consistency sweep: aligns Run §28966928999
|
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44312 does not have the 'implementation' label and has only 63 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This pull request aligns gh aw CLI documentation and help text with current behavior, and formalizes deprecation behavior for legacy --disable-* aliases so users receive migration guidance.
Changes:
- Updated docs/help text for
compile,upgrade,status,audit, and added CLI docs coverage forexperiments. - Replaced hidden legacy aliases (
--disable-security-scanner,--disable-release-bump) with deprecated aliases that emit guidance. - Added/extended tests to lock in deprecated-flag metadata and doc/help consistency assertions.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/upgrade_command.go | Adjusts upgrade long help to clarify default fix/update/compile behavior. |
| pkg/cli/update_command.go | Deprecates legacy --disable-* aliases instead of hiding them. |
| pkg/cli/update_command_test.go | Adds assertions for deprecated flag metadata on update command aliases. |
| pkg/cli/trial_command.go | Deprecates legacy --disable-security-scanner alias in trial. |
| pkg/cli/trial_command_test.go | Adds test asserting deprecated metadata for trial legacy alias. |
| pkg/cli/deploy_command.go | Deprecates legacy --disable-security-scanner alias in deploy. |
| pkg/cli/deploy_command_test.go | Adds test asserting deprecated metadata for deploy legacy alias. |
| pkg/cli/cli_consistency_help_test.go | Adds doc/help consistency checks covering audit, status, experiments, and fix/no-fix wording. |
| pkg/cli/add_command.go | Deprecates legacy --disable-security-scanner alias in add. |
| pkg/cli/add_command_test.go | Adds test asserting deprecated metadata for add legacy alias. |
| docs/src/content/docs/setup/cli.md | Updates CLI guide text for compile, status, audit, upgrade, and adds experiments section. |
| cmd/gh-aw/main.go | Updates compile help text to clarify that codemods are opt-in via --fix. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Low
| Flag behavior: | ||
| - --no-fix skips codemods, action version updates, and workflow compilation | ||
| - upgrade runs the equivalent of fix/action-update/compile work by default; use --no-fix to skip codemods, action version updates, and workflow compilation | ||
| - --no-actions and --no-compile are only applied when --no-fix is not set |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Align CLI docs and deprecation behavior
The changes are well-scoped and correct. All --disable-* legacy flags are consistently migrated from MarkHidden to MarkDeprecated with matching migration messages, tests cover every affected command, and the doc updates accurately reflect actual behavior (removed false schedule-in-console claim, fixed three-mode to two-mode audit description, added the missing experiments section).
One minor issue found: In pkg/cli/update_command_test.go lines 93-95, a comment and require.NotNil message still say "undocumented alias" even though the alias is now deprecated. The assert.Equal assertion added just below is correct, so this is a stale label, not a logic error. Inline comment posted.
Everything else looks good - the upgrade_command.go help text, the compile/upgrade polarity note in both help and docs, and the consistency test additions are all accurate.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 34.8 AIC · ⌖ 6.08 AIC · ⊞ 4.8K
| @@ -93,6 +93,16 @@ func TestNewUpdateCommand_HasDisableSecurityScannerFlag(t *testing.T) { | |||
| // Undocumented alias should still be registered | |||
| deprecated := cmd.Flags().Lookup("disable-security-scanner") | |||
| require.NotNil(t, deprecated, "update command should keep --disable-security-scanner as undocumented alias") | |||
There was a problem hiding this comment.
The surrounding comment (line 93) and the require.NotNil message here still say "undocumented alias", but the alias is now a deprecated alias (not just hidden). The existing assert.Equal added on the next line correctly checks flag.Deprecated, so the intent is right — but the comment and message now contradict it.
Suggest updating both to match the new intent:
// Deprecated alias should still be registered
deprecated := cmd.Flags().Lookup("disable-security-scanner")
require.NotNil(t, deprecated, "update command should keep --disable-security-scanner as a deprecated alias")@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (8 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /grill-with-docs, and /tdd — commenting only, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Stale test comment (correctness) —
update_command_test.gostill says "undocumented alias" after the flag was promoted to a deprecated alias; the assertion message is now misleading. - Terminology drift (docs ↔ help) —
upgrade_command.goandcli.mduse different informal phrases for the same three-step pipeline, undermining the consistency goal of this PR. - Brittle string pinning — the new assertion in
cli_consistency_help_test.gopins an exact sentence fromaudit.go's Long description; any prose refactoring will break the test without a user-visible regression. - Minor redundancy in experiments docs — options are listed twice (root + analyze) when a single line would suffice.
Positive Highlights
- ✅ Clean, uniform application of
MarkDeprecatedacross all five affected commands - ✅ Good pattern of accompanying each flag change with a focused test that pins the
.Deprecatedfield - ✅
TestCLIDocsReflectStatusAuditAndExperimentsCommandsis a valuable cross-file consistency gate — it will catch future regressions where code and docs drift apart - ✅
NotContainson the removed schedule promise is a particularly well-chosen assertion - ✅ PR description is clear and well-structured — the example code snippet makes the deprecation behavior immediately concrete
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 76.1 AIC · ⌖ 5.75 AIC · ⊞ 6.6K
Comment /matt to run again
| @@ -93,6 +93,16 @@ func TestNewUpdateCommand_HasDisableSecurityScannerFlag(t *testing.T) { | |||
| // Undocumented alias should still be registered | |||
| deprecated := cmd.Flags().Lookup("disable-security-scanner") | |||
| require.NotNil(t, deprecated, "update command should keep --disable-security-scanner as undocumented alias") | |||
There was a problem hiding this comment.
[/codebase-design] Stale comment and assertion message still say "undocumented alias" after the flag became a deprecated alias.
Lines 93–95 were not updated to match the change from MarkHidden to MarkDeprecated. The test now lies about what it is verifying, which undermines its value as a specification.
💡 Suggested fix
// Deprecated alias should still be registered
deprecated := cmd.Flags().Lookup("disable-security-scanner")
require.NotNil(t, deprecated, "update command should keep --disable-security-scanner as a deprecated alias")
assert.Equal(t, "use --no-security-scanner instead", deprecated.Deprecated)All four new tests added in this PR use the phrase "deprecated alias" correctly — this pre-existing block is the only outlier.
@copilot please address this.
| gh aw experiments analyze my-workflow --repo owner/repo # Analyze experiments in another repository | ||
| ``` | ||
|
|
||
| **Options:** root command `--repo/-r`, `--json/-j`; `analyze` also accepts `--repo/-r`, `--json/-j` |
There was a problem hiding this comment.
[/grill-with-docs] experiments docs list --repo/-r and --json/-j twice — once for the root command and once for analyze — but they are the same flags, which may confuse readers.
The actual flag registration shows --repo and --json are added to both the root command (via addRepoFlag/addJSONFlag) and to each subcommand independently. The options line documents this correctly but it reads redundantly. Consider clarifying that all subcommands inherit --repo/-r and --json/-j rather than duplicating them.
💡 Suggested wording
**Options:** All subcommands accept `--repo/-r` and `--json/-j`.This is a minor docs readability issue, not a correctness bug.
@copilot please address this.
|
|
||
| assert.Contains(t, cmd.Short, "workflow runs", "audit short description should describe multiple run inputs") | ||
| assert.Contains(t, cmd.Long, "Audit one or more workflow runs", "audit long description should describe multiple run inputs") | ||
| assert.Contains(t, cmd.Long, "When two or more runs are provided", "audit help should document multi-run analysis mode") |
There was a problem hiding this comment.
[/tdd] This assertion pin-tests an internal string literal in audit.go rather than observable behavior — if the phrasing is refactored, the test breaks without any user-visible regression.
Pinning to a specific sentence like "When two or more runs are provided" couples the test to implementation prose. A more durable approach is to test that the command accepts multiple positional args and routes to the multi-run path (e.g., integration test with two run IDs), or at minimum to document why the exact sentence is part of the public contract.
💡 Alternative
If the intent is purely doc-surface consistency, consider a higher-level assertion such as checking that the word multi-run or multiple runs appears in the long description rather than pinning to a sentence-level fragment.
@copilot please address this.
|
|
||
| Flag behavior: | ||
| - --no-fix skips codemods, action version updates, and workflow compilation | ||
| - upgrade runs the equivalent of fix/action-update/compile work by default; use --no-fix to skip codemods, action version updates, and workflow compilation |
There was a problem hiding this comment.
[/grill-with-docs] The help text uses two different informal phrases for the same concept: fix/action-update/compile work (upgrade help) vs fix/update/compile steps (docs). The inconsistency makes it harder to grep or cross-reference.
The docs added in this PR (cli.md line 742) say fix/update/compile steps while the upgrade command's own --help output says fix/action-update/compile work. Users reading both will see different terminology for the same pipeline.
💡 Suggested fix
Pick one canonical phrase — fix/action-update/compile matches the --no-actions and --no-compile flag names better — and use it in both places:
// upgrade_command.go
- upgrade runs the equivalent of fix/action-update/compile work by default; use --no-fix to skip codemods, action version updates, and workflow compilation
+ upgrade runs fix/action-update/compile by default; use --no-fix to skip all three steps// cli.md
- Unlike `gh aw compile --fix`, `gh aw upgrade` runs its fix/update/compile steps by default
+ Unlike `gh aw compile --fix`, `gh aw upgrade` runs fix/action-update/compile by default@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Non-blocking observations on what is otherwise a clean consistency sweep.
### Findings summary
1. Stale "undocumented alias" language in update_command_test.go (medium)
The existing test comment and require.NotNil message were not updated alongside the new deprecation behavior. The three brand-new tests in this PR all say "deprecated alias" correctly; this one old test was missed. Low risk, but creates a misleading paper trail.
2. Lone NotContains in TestCLIDocsReflectStatusAuditAndExperimentsCommands (medium)
The test guards against the old schedule-output text surviving but does not assert the replacement guidance (Use --json to inspect...) is present. Without a paired Contains, the assertion is half a test — it would silently pass if the replacement line were removed or reworded later.
Flag wiring verified: Confirmed that deprecated aliases are still OR-ed into the primary flag read (disableSecurityScanner = disableSecurityScanner || disableSecurityScannerLegacy) in add_command.go, deploy_command.go, trial_command.go, and update_command.go — deprecated flags continue to work functionally.
🔎 Code quality review by PR Code Quality Reviewer · 126.4 AIC · ⌖ 5.18 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/update_command_test.go:95
Stale comment and assertion message still say "undocumented alias" after this flag becomes explicitly deprecated.
The comment on line 93 and the require.NotNil message on line 95 both read undocumented alias, but this PR promotes the alias to a deprecated alias with an explicit migration message. The three new tests added in this PR (TestNewAddCommand_DeprecatesDisableSecurityScannerFlag etc.) all use the correct "deprecated alias" wording — this old test was only partially updated…
pkg/cli/cli_consistency_help_test.go:138
NotContains alone does not verify the replacement text is present — the test can pass while the docs fix is missing.
assert.NotContains(t, text, "enabled/disabled status, schedules, and labels", ...) only guards against the old string surviving. If someone reverts the status-docs change and writes a slightly different sentence (or leaves a blank), this assertion passes while the documented behavior is still wrong.
<details>
<summary>💡 Suggested fix</summary>
Add a positive assertion…
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran PR finisher pass and pushed follow-up commit |
|
🎉 This pull request is included in a new release. Release: |
The CLI consistency sweep found a small set of high-severity mismatches between documented behavior and the current command surface:
auditmode docs were wrong,experimentswas missing from the CLI guide,statuspromised schedule output it does not render in console mode, and legacy--disable-*aliases were silently accepted without migration guidance.Docs: align command reference with actual behavior
auditnow documents the current two-mode model: single-run audit and multi-run analysisstatusno longer claims console output includes schedules; it points readers to--jsonfor rawondataexperimentssection to the CLI guidecompile --fixandupgrade --no-fixCLI help: make fix/no-fix behavior explicit
compilehelp now states that codemods are opt-in via--fixupgradehelp now states that fix/action-update/compile work runs by default and--no-fixdisables itFlag behavior: deprecate legacy aliases instead of hiding them
--disable-security-scanner→--no-security-scanner--disable-release-bump→--no-release-bumpTests: lock in CLI consistency
audit,status,experiments, and fix/no-fix wordingExample of the alias behavior after this change: