Skip to content

[cli-consistency] Normalize trial option ordering and logs cache-before help text #47834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Jul 25, 2026
+2 −1
Merged

[cli-consistency] Normalize trial option ordering and logs cache-before help text #47834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
492e7d8
Initial plan
Copilot Jul 24, 2026
0e91360
chore: plan cli consistency fixes
Copilot Jul 24, 2026
05f205b
fix: align trial docs and logs cache-before help style
Copilot Jul 24, 2026
c9e0f9f
fix: restore cache-before help prefix text
Copilot Jul 24, 2026
5120e07
fix: revert unrelated SKILL.md change, restore Codespaces download gu…
Copilot Jul 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/src/content/docs/setup/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ gh aw trial ./workflow.md --host-repo owner/repo # Run directly in repository
gh aw trial ./workflow.md --dry-run # Preview without executing
```

**Options:** `-e/--engine`, `--repeat`, `--delete-host-repo-after`, `--logical-repo/-l`, `--clone-repo`, `--trigger-context`, `--host-repo`, `--dry-run`, `--append`, `--auto-merge-prs`, `--no-security-scanner`, `--delete-host-repo-before`, `--json/-j`, `--timeout`, `--yes/-y`
**Options:** `--engine/-e`, `--repeat`, `--delete-host-repo-after`, `--logical-repo/-l`, `--clone-repo`, `--trigger-context`, `--host-repo`, `--dry-run`, `--append`, `--auto-merge-prs`, `--no-security-scanner`, `--delete-host-repo-before`, `--json/-j`, `--timeout`, `--yes/-y`

**Secret Handling:** API keys required for the selected engine are automatically checked. If missing from the target repository, they are prompted for interactively and uploaded.

Expand Down
1 change: 1 addition & 0 deletions pkg/cli/logs_command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ func TestNewLogsCommand(t *testing.T) {
assert.NotNil(t, cacheBeforeFlag, "Should have 'cache-before' flag")
assert.Contains(t, cacheBeforeFlag.Usage, "-1d", "cache-before flag should document day deltas")
assert.Contains(t, cacheBeforeFlag.Usage, "-30d", "cache-before flag should document explicit day-count deltas")
assert.Contains(t, cacheBeforeFlag.Usage, "(Cache eviction)", "cache-before flag usage should retain cache-eviction prefix text")

@github-actions github-actions Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/grill-with-docs] The PR description says the (Cache eviction) prefix is removed, but this assertion guards that it is retained — the two contradict each other. The description should be updated to match the actual final state (prefix kept per reviewer request), so future audits and bisects aren't misled.

@copilot please address this.

@github-actions github-actions Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test assertion contradicts the PR description: the new assertion verifies that (Cache eviction) IS present in the flag usage string, but the PR description says this prefix was removed from logs_command.go.

💡 Details

From the PR body:

Updated pkg/cli/logs_command.go to remove the unique parenthetical prefix from the --cache-before flag description.

But logs_command.go is not in the diff — it was not changed — and the existing code on line 395 still reads:

logsCmd.Flags().String("cache-before", "", "(Cache eviction) Evict locally cached run folders ...")

So the new assertion:

assert.Contains(t, cacheBeforeFlag.Usage, "(Cache eviction)", "cache-before flag usage should retain cache-eviction prefix text")

...passes today because the text was never removed, which means the test adds no regression protection. If someone later actually removes (Cache eviction) (as the PR description implies was intended), this assertion would catch it — which is the opposite of what you want.

Either:

  1. The PR description is wrong and the intent is to keep the prefix (the test is then correct, but the description must be fixed), or
  2. The prefix was supposed to be removed from logs_command.go (the change was missed), in which case the test should assert the absence of the prefix, not its presence.

This needs clarification before merge.


// Backward-compatible alias should remain registered but hidden from help output
afterAliasFlag := flags.Lookup("after")
Expand Down
Loading