-
Notifications
You must be signed in to change notification settings - Fork 495
Improve actionable CLI error messages #52175
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -438,10 +438,13 @@ func TestCreateWorkflowInteractively_InAutomatedEnvironment(t *testing.T) { | |
| t.Error("Expected error in automated environment, got nil") | ||
| } | ||
|
|
||
| expectedErrMsg := "interactive workflow creation cannot be used in automated tests or CI environments" | ||
| expectedErrMsg := "interactive workflow creation is unavailable in automated tests or CI environments" | ||
| if !strings.Contains(err.Error(), expectedErrMsg) { | ||
| t.Errorf("Expected error containing %q, got %q", expectedErrMsg, err.Error()) | ||
| } | ||
| if !strings.Contains(err.Error(), "Expected an interactive terminal outside automation") || !strings.Contains(err.Error(), "Example: run `gh aw new` from a local terminal") { | ||
| t.Errorf("Expected actionable error message, got %q", err.Error()) | ||
| } | ||
| } | ||
|
|
||
| func TestCreateWorkflowInteractively_WithForceFlag(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Test coverage is extended only for The structured error pattern (state, expected, example) is now enforced in one place but unverified in 💡 Suggested approachAdd a parallel assertion for each changed file that checks both an 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
|
|
@@ -795,6 +798,9 @@ func TestPromptNonInteractiveSelect_InvalidValue(t *testing.T) { | |
| if err == nil { | ||
| t.Fatal("expected error for unknown value, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "Expected a number or option value") || !strings.Contains(err.Error(), "Example: 1") { | ||
| t.Errorf("Expected actionable select error message, got %q", err.Error()) | ||
| } | ||
| } | ||
|
|
||
| func TestPromptNonInteractiveSelect_EOF(t *testing.T) { | ||
|
|
@@ -874,6 +880,9 @@ func TestPromptNonInteractiveMultiSelect_OutOfRange(t *testing.T) { | |
| if err == nil { | ||
| t.Fatal("expected error for out-of-range index, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "Expected a number from 1 to 1") || !strings.Contains(err.Error(), "Example: 1,2") { | ||
| t.Errorf("Expected actionable multi-select range error message, got %q", err.Error()) | ||
| } | ||
| } | ||
|
|
||
| func TestPromptNonInteractiveMultiSelect_UnknownValue(t *testing.T) { | ||
|
|
@@ -883,6 +892,9 @@ func TestPromptNonInteractiveMultiSelect_UnknownValue(t *testing.T) { | |
| if err == nil { | ||
| t.Fatal("expected error for unknown value, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "Expected comma-separated numbers or option values") || !strings.Contains(err.Error(), "Example: 1,2") { | ||
| t.Errorf("Expected actionable multi-select value error message, got %q", err.Error()) | ||
| } | ||
| } | ||
|
|
||
| func TestPromptForWorkflowNameFrom_Valid(t *testing.T) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These callback validation errors are internal programming errors (not user-facing CLI messages). The new Consider either omitting @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| return errors.New("orgRunCallbacks.SearchFn is required") | ||
| return errors.New("organization search callback is not configured. Expected orgRunCallbacks.SearchFn to search organization repositories. Example: configure SearchFn before calling runCommandForOrg") | ||
| } | ||
| if cbs.ReportFn == nil { | ||
| return errors.New("orgRunCallbacks.ReportFn is required") | ||
| return errors.New("organization report callback is not configured. Expected orgRunCallbacks.ReportFn to display the run summary. Example: configure ReportFn before calling runCommandForOrg") | ||
| } | ||
| if createPR && cbs.ApplyFn == nil { | ||
| return errors.New("orgRunCallbacks.ApplyFn is required when createPR is true") | ||
| return errors.New("pull request callback is not configured. Expected orgRunCallbacks.ApplyFn when createPR is enabled. Example: configure ApplyFn before calling runCommandForOrg with createPR") | ||
| } | ||
| if createIssue && cbs.IssueFn == nil { | ||
| return errors.New("orgRunCallbacks.IssueFn is required when createIssue is true") | ||
| return errors.New("issue callback is not configured. Expected orgRunCallbacks.IssueFn when createIssue is enabled. Example: configure IssueFn before calling runCommandForOrg with createIssue") | ||
| } | ||
| 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 update --org octo-org --create-pull-request --yes") | ||
| } | ||
|
|
||
| // Handle Ctrl-C / SIGTERM so an interrupted run still renders the report | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The
Example: 1hint is not contextual — the actual valid range varies per call, but the example always shows1. 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:
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
61891c4e78: multi-select range and unknown-value errors now use comma-separated examples (1,2).