Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #52175 for over-engineering per Ponytail skill. This diff only rewords existing error strings (adding 'Expected...Example:' context) — no new abstractions, functions, dependencies, or structural complexity introduced. Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52175 does not have the 'implementation' label and has only 29 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.
Review: Improve actionable CLI error messages
The changes consistently apply the <What happened>. Expected <what was expected>. Example: <concrete example> pattern across CLI error messages. This is a clear, user-friendly improvement. The test updates in interactive_test.go correctly validate the new pattern.
One non-blocking suggestion left inline on org_runner.go.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.7 AIC · ⌖ 6.25 AIC · ⊞ 5.4K
| @@ -146,19 +146,19 @@ func runCommandForOrg(ctx context.Context, org string, repoGlobs []string, cbs o | |||
| return errors.New("createPR and createIssue are mutually exclusive") | |||
| } | |||
| if cbs.SearchFn == nil { | |||
There was a problem hiding this comment.
These callback validation errors are internal programming errors (not user-facing CLI messages). The new Example: values expose raw Go struct literal syntax (orgRunCallbacks{SearchFn: searchFn}) which is not actionable to CLI users.
Consider either omitting Example: for developer-facing internal errors, or replacing with a user-meaningful CLI example (e.g., gh aw run --org octo-org).
@copilot please address this.
There was a problem hiding this comment.
Addressed in 61891c4e78: the org callback examples are now developer-oriented and no longer expose raw Go struct literal syntax.
🧪 Test Quality Sentinel ReportSummaryScore: ✅ Excellent (100/100) This PR improves error message actionability. The test modification adds focused assertions to verify that error messages include actionable guidance ("Expected an interactive terminal", "Example: unset CI"), not just problem statements. AnalysisTest Files Changed
Modified Test:
|
| Aspect | Rating | Notes |
|---|---|---|
| Design Invariant | behavioral_contract |
Error messages must include actionable guidance for CI environments |
| Value if Deleted | high_value |
Regression: error messages could become less helpful |
| Coverage Type | design_test |
Tests user-visible error message content |
| Assertions | 3 error assertions | All focus on error path and message clarity |
| Edge Cases | Error path only | Tests failure mode (CI environment) |
| Code Quality | Excellent | Setup/teardown with t.Cleanup(), clear failure messages |
Changes in Detail
Lines 441-444 (unchanged context):
- Asserts error is not nil
- Checks for main error message
Lines 445-447 (NEW - added in this PR):
+ Verifies "Expected an interactive terminal" in error
+ Verifies "Example: unset CI" in error
+ Ensures error message is actionable, not just descriptive
Why This Test Matters
This modification ensures the PR's goal of "actionable error messages" is actually tested. Without these assertions, the new error message strings could be mistakenly removed or truncated without detection. The test confirms:
- Problem clarity: Main error statement is present
- Actionable guidance: Users get clear direction ("Expected an interactive terminal")
- Concrete examples: Users see remediation ("Example: unset CI")
Scoring Breakdown
| Component | Points | Reasoning |
|---|---|---|
| Design tests (40 pts) | 40 | 100% of tests are design-focused (error message contract) |
| Edge cases (30 pts) | 30 | All assertions cover error path (non-happy path) |
| No duplication (20 pts) | 20 | No duplicate patterns detected |
| No inflation (10 pts) | 10 | 3 lines added to test is proportional |
| Total | 100 |
Verdict
✅ APPROVED — Test quality is excellent. The PR adds focused, high-value assertions that verify the core design contract: error messages must be actionable, not just informative. No violations detected.
Infrastructure notes: No TestMain or goroutine-leak guards in this PR.
🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 13.5 AIC · ⌖ 3.14 AIC · ⊞ 7.6K · ◷
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on three issues.
📋 Key Themes & Highlights
Issues
- Thin test coverage: only
interactive_test.gois updated; the other five changed files have no test assertions for the new error format - Internal API leaks Go syntax to users:
org_runner.goexamples showorgRunCallbacks{SearchFn: searchFn}— either make them developer-facing panics or replace with CLI examples - Embedded newline in error string:
add_workflow_resolution.goline 411 breaks the single-line error convention and will cause issues in log aggregation and test assertions - Multi-select example is single-select:
Example: 1for comma-separated input is misleading
Positive Highlights
- ✅ Consistent three-part pattern (condition → expected → example) applied uniformly
- ✅
audit_diff_command.goandinteractive.goerrors are now genuinely actionable - ✅ The new test assertions in
interactive_test.goare a good model for other files
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.7 AIC · ⌖ 6.82 AIC · ⊞ 7K
Comment /matt to run again
| } | ||
| } | ||
|
|
||
| func TestCreateWorkflowInteractively_WithForceFlag(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Test coverage is extended only for interactive.go — the five other changed files have no new or updated tests.
The structured error pattern (state, expected, example) is now enforced in one place but unverified in add_workflow_resolution.go, audit_diff_command.go, add_interactive_orchestrator.go, and org_runner.go. A regression in any of those files will go undetected.
💡 Suggested approach
Add a parallel assertion for each changed file that checks both an Expected and an Example: fragment:
if !strings.Contains(err.Error(), "Expected") || !strings.Contains(err.Error(), "Example:") {
t.Errorf("Expected actionable error message, got %q", err.Error())
}@copilot please address this.
There was a problem hiding this comment.
Addressed in 61891c4e78: added focused assertions for the other changed validation paths, including org runner, audit diff, current-repository resolution, private workflow resolution, and add-wizard automation errors.
| } | ||
| if cbs.SearchFn == nil { | ||
| return errors.New("orgRunCallbacks.SearchFn is required") | ||
| return errors.New("organization search callback is not configured. Expected orgRunCallbacks.SearchFn to search organization repositories. Example: orgRunCallbacks{SearchFn: searchFn}") |
There was a problem hiding this comment.
[/grill-with-docs] The example text exposes internal Go struct syntax (orgRunCallbacks{SearchFn: searchFn}) to end users, but runCommandForOrg is an internal function — this error is only reachable by Go callers, not CLI users.
If this error is truly internal-only, the example should use Go call-site syntax. If it can surface to CLI users, the example should be a CLI command like the other files.
💡 Suggested fix
For an internal programming error, follow the same pattern as the existing createPR and createIssue are mutually exclusive message — keep it terse and developer-oriented, or use a panic:
return errors.New("orgRunCallbacks.SearchFn must not be nil")For a user-facing error, use a CLI example consistent with every other changed file:
return errors.New("organization search callback is not configured. Expected orgRunCallbacks.SearchFn. Example: gh aw run --org octo-org")@copilot please address this.
There was a problem hiding this comment.
Addressed in 61891c4e78: the internal org callback messages now use developer-oriented recovery text and avoid raw struct literal examples.
| manifestPath := joinRepositoryPackagePath(spec.PackagePath, repositoryPackageManifestFileName) | ||
| return fmt.Errorf( | ||
| "invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added", | ||
| "agentic workflow manifest %q sets workflow %q to private: true. Expected an installable workflow with private: false. Example:\nprivate: false", |
There was a problem hiding this comment.
[/codebase-design] The error string contains a literal \n newline character, breaking from the single-line error convention used everywhere else in the file and across all other changed files.
Embedded newlines in error values complicate log aggregation, JSON marshalling, and string comparison in tests.
💡 Suggested fix
Keep the example on one line, consistent with all other changed errors:
"agentic workflow manifest %q sets workflow %q to private: true. Expected an installable workflow with private: false. Example: private: false",@copilot please address this.
There was a problem hiding this comment.
Addressed in 61891c4e78: the private workflow guidance is now single-line and preserves the existing workflow-first diagnostic.
| if idx, err := strconv.Atoi(input); err == nil { | ||
| if idx < 1 || idx > len(options) { | ||
| return "", fmt.Errorf("selection out of range (must be 1-%d)", len(options)) | ||
| return "", fmt.Errorf("selection is out of range. Expected a number from 1 to %d. Example: 1", len(options)) |
There was a problem hiding this comment.
[/tdd] The Example: 1 hint is not contextual — the actual valid range varies per call, but the example always shows 1. This is fine for the single-select path, but the multi-select path (promptNonInteractiveMultiSelect) allows comma-separated values; the example should reflect that.
💡 Suggested fix
For the multi-select error at line ~456 and ~475, a comma-separated example would be more instructive:
return nil, fmt.Errorf("selection %d is out of range. Expected a number from 1 to %d. Example: 1,2", idx, len(options))@copilot please address this.
There was a problem hiding this comment.
Addressed in 61891c4e78: multi-select range and unknown-value errors now use comma-separated examples (1,2).
There was a problem hiding this comment.
Pull request overview
Improves CLI validation errors with expected inputs and recovery examples.
Changes:
- Expands error guidance across five CLI workflows.
- Adds interactive-environment message assertions.
- Clarifies audit and workflow-resolution validation failures.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/org_runner.go |
Improves organization-runner errors. |
pkg/cli/interactive.go |
Improves interactive selection guidance. |
pkg/cli/interactive_test.go |
Tests automated-environment guidance. |
pkg/cli/audit_diff_command.go |
Adds actionable audit validation errors. |
pkg/cli/add_workflow_resolution.go |
Improves workflow-resolution errors. |
pkg/cli/add_interactive_orchestrator.go |
Improves interactive-add errors. |
Review details
Suppressed comments (1)
pkg/cli/interactive.go:475
- The prompt displays
opt.label, but this parser acceptsopt.value, so “displayed option name” is inaccurate whenever labels and values differ. Point users to an option value or the displayed numeric index.
return nil, fmt.Errorf("option %q is not available. Expected a displayed option name or number. Example: 1", tok)
- Files reviewed: 6/6 changed files
- Comments generated: 6
- Review effort level: Balanced
| } | ||
| if (createPR || createIssue) && !cbs.AutoYes && isRunningInCIFn() { | ||
| return errors.New("confirmation is required for --org create operations in CI; re-run with --yes to auto-accept") | ||
| return errors.New("organization create operations in CI need confirmation. Expected --yes to auto-accept in non-interactive environments. Example: gh aw run --org octo-org --yes") |
There was a problem hiding this comment.
Addressed in 61891c4e78: the CI confirmation example now uses a valid org update create operation with --yes.
| // IsRunningInCI centralizes the broader CI environment detection logic. | ||
| if envutil.GetBoolFromEnv("GO_TEST_MODE", false, interactiveLog) || IsRunningInCI() { | ||
| return errors.New("interactive workflow creation cannot be used in automated tests or CI environments") | ||
| return errors.New("interactive workflow creation is unavailable in automated tests or CI environments. Expected an interactive terminal. Example: unset CI and run the command from a terminal") |
There was a problem hiding this comment.
Addressed in 61891c4e78: the interactive workflow recovery now points to running gh aw new from a local terminal outside automation, and the test assertion was updated.
| } | ||
| } | ||
| return "", fmt.Errorf("invalid selection %q", input) | ||
| return "", fmt.Errorf("selection %q is not available. Expected a displayed option name or number. Example: 1", input) |
There was a problem hiding this comment.
Addressed in 61891c4e78: non-interactive selection errors now describe accepted option values rather than displayed labels.
| } | ||
| if spec.RepoSlug == currentRepoSlug { | ||
| return fmt.Errorf("cannot add workflows from the current repository (%s). The 'add' command is for installing workflows from other repositories", currentRepoSlug) | ||
| return fmt.Errorf("workflow source %q is the current repository. Expected a workflow from another repository. Example: gh aw add github/gh-aw/example-workflow", currentRepoSlug) |
There was a problem hiding this comment.
Addressed in 61891c4e78: restored the current-repository diagnostic identity and changed the example to an unambiguously different repository.
| manifestPath := joinRepositoryPackagePath(spec.PackagePath, repositoryPackageManifestFileName) | ||
| return fmt.Errorf( | ||
| "invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added", | ||
| "agentic workflow manifest %q sets workflow %q to private: true. Expected an installable workflow with private: false. Example:\nprivate: false", |
There was a problem hiding this comment.
Addressed in 61891c4e78: restored the workflow-first sets private: true diagnostic and kept the actionable guidance on one line.
| // IsRunningInCI centralizes the broader CI environment detection logic. | ||
| if envutil.GetBoolFromEnv("GO_TEST_MODE", false, addInteractiveLog) || IsRunningInCI() { | ||
| return errors.New("interactive add cannot be used in automated tests or CI environments") | ||
| return errors.New("interactive add is unavailable in automated tests or CI environments. Expected an interactive terminal. Example: unset CI and run the command from a terminal") |
There was a problem hiding this comment.
Addressed in 61891c4e78: the add-wizard recovery now points to running gh aw add-wizard from a local terminal outside automation.
|
@copilot please address the open review threads below, fix the failed checks, refresh the branch if needed, and run the pr-finisher skill. Open review threads (newest first):
Failed checks:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
🤖 Triage: PR #52175
|
|
@copilot please finish the remaining review cleanup below, refresh the branch if GitHub offers it, and run the pr-finisher skill. Open review threads (newest first):
The PR author replied, but these threads still appear unresolved. Please resolve the outstanding reviewer concerns or explicitly close the threads as appropriate.
|
Updates low-compliance CLI validation errors in five scoped files to consistently state the invalid condition, expected input, and a concrete recovery example.
Organization runner
Interactive workflows
Audit diff
Workflow resolution
Interactive add orchestration
run: https://github.com/github/gh-aw/actions/runs/31547117706> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.19 AIC · ⌖ 5.2 AIC · ⊞ 8.5K · ◷