Raise error-message compliance in dispatch validation and CLI evaluation/logging paths#52173
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52173 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (82 additions detected, threshold is 100).
|
|
✅ 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.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR only rewrites validator/CLI error message strings to the repo style guide (what failed/expected/example) and updates matching test assertions. No new abstractions, dependencies, or reinvented logic introduced — nothing to cut.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The error-message compliance improvements look good. All changes consistently follow the Expected ... Example: pattern, include the invalid value with '' for clarity, and have thorough test coverage for each updated message. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.3 AIC · ⌖ 6.17 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Improves validation and CLI error messages with clearer expectations and actionable examples.
Changes:
- Reworks dispatch validation messages.
- Improves interactive-run, outcome-evaluation, and logs-command errors.
- Updates focused message assertions.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/dispatch_workflow_validation.go |
Improves dispatch-workflow errors. |
pkg/workflow/dispatch_workflow_validation_test.go |
Updates message assertions. |
pkg/workflow/dispatch_repository_validation.go |
Improves repository dispatch errors. |
pkg/workflow/dispatch_repository_test.go |
Extends error assertions. |
pkg/cli/run_interactive.go |
Improves interactive-run errors. |
pkg/cli/run_interactive_test.go |
Adds CI error coverage. |
pkg/cli/outcome_eval.go |
Improves endpoint and input errors. |
pkg/cli/outcome_eval_test.go |
Updates endpoint assertions. |
pkg/cli/logs_command.go |
Improves option-validation errors. |
pkg/cli/logs_command_test.go |
Adds message-shape tests. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Check if running in CI environment | ||
| if IsRunningInCI() { | ||
| return errors.New("interactive mode cannot be used in CI environments") | ||
| return errors.New("interactive mode is unavailable in CI environments. Expected an interactive terminal session outside CI. Example: run 'gh aw run --interactive' from your local terminal") |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with a few refinement suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Embedded newlines in CLI errors:
run_interactive.goembeds a raw YAML block literal in aerrors.Newstring, which is fine for compiler output but can look awkward in interactive/terminal error contexts. - Internal function name leaked:
buildGraphQLArgs received...exposes an internal identifier in a user-facing error. Alternative Example:label: Deviates from the consistentExample:convention enforced across the rest of the PR.- Conditional style assertion: The
assertStyle boolguard inoutcome_eval_test.goweakens regression protection; all error rows should unconditionally assert the new style. - Redundant
inputNamein prompt error: The interactive required-input message repeats the input name twice without added value.
Positive Highlights
- ✅ Consistent
what failed / Expected / Example:pattern applied across all five files. - ✅ Defensive fallback for
exampleRuntime/exampleEngineguards against empty-slice panics. - ✅ New focused tests in
logs_command_test.goandrun_interactive_test.gopin the new message shape cleanly. - ✅ No validation logic was altered — purely a message quality improvement.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.2 AIC · ⌖ 6.93 AIC · ⊞ 7K
Comment /matt to run again
|
|
||
| if len(workflows) == 0 { | ||
| return errors.New("no runnable workflows found. Workflows must have 'workflow_dispatch' trigger") | ||
| return errors.New("no runnable workflows were found. Expected at least one workflow with 'on: workflow_dispatch'. Example:\non:\n workflow_dispatch: {}") |
There was a problem hiding this comment.
[/codebase-design] The no runnable workflows error embeds a raw newline and YAML snippet in a errors.New string, which breaks single-line log/error formatting and makes programmatic matching harder.
💡 Suggestion
Keep the example concise without embedded newlines, or move multi-line YAML to a separate line using a const:
return errors.New("no runnable workflows were found. Expected at least one workflow with \"on: workflow_dispatch\". Example: on:\n workflow_dispatch: {}")Note this is consistent with dispatch_workflow_validation.go (compiler output), but interactive CLI errors surfaced via huh or a terminal prompt often do better without embedded block literals.
@copilot please address this.
| args = append(args, "-F", fmt.Sprintf("%s=%v", name, value)) | ||
| default: | ||
| return nil, fmt.Errorf("buildGraphQLArgs: unsupported variable type %T for key %q", value, name) | ||
| return nil, fmt.Errorf("buildGraphQLArgs received unsupported variable type %T for key %q. Expected string, int, int32, int64, or bool values. Example: map[string]any{\"number\": 42}", value, name) |
There was a problem hiding this comment.
[/codebase-design] The new error message starts with buildGraphQLArgs received unsupported variable type — exposing the internal function name buildGraphQLArgs in a user-facing error. The previous message had the same issue, but now it is more prominent with the verbose prefix.
💡 Suggestion
Leave internal function names out of user-facing errors. Prefer describing the problem in terms the caller understands:
return nil, fmt.Errorf("unsupported variable type %T for GraphQL key %q. Expected string, int, int32, int64, or bool. Example: map[string]any{\"number\": 42}", value, name)@copilot please address this.
|
|
||
| if !hasRepository && !hasAllowedRepos { | ||
| repoErr := fmt.Errorf("dispatch_repository: tool %q must specify either 'repository' or 'allowed_repositories'\n\nExample with single repository:\n dispatch_repository:\n %s:\n workflow: %s\n event_type: %s\n repository: org/target-repo\n\nExample with multiple repositories:\n dispatch_repository:\n %s:\n workflow: %s\n event_type: %s\n allowed_repositories:\n - org/repo1\n - org/repo2", toolKey, toolKey, tool.Workflow, tool.EventType, toolKey, tool.Workflow, tool.EventType) | ||
| repoErr := fmt.Errorf("dispatch_repository tool %q has no repository target. Expected either 'repository' or 'allowed_repositories'. Example:\n dispatch_repository:\n %s:\n workflow: %s\n event_type: %s\n repository: org/target-repo\nAlternative Example:\n dispatch_repository:\n %s:\n workflow: %s\n event_type: %s\n allowed_repositories:\n - org/repo1\n - org/repo2", toolKey, toolKey, tool.Workflow, tool.EventType, toolKey, tool.Workflow, tool.EventType) |
There was a problem hiding this comment.
[/codebase-design] The error uses Alternative Example: as a second example label — this deviates from the rest of the PR which consistently uses a single Example: marker. Operators and linters looking for the style-guide Example: keyword will find this inconsistent.
💡 Suggestion
Use a single Example: label, showing a concise dual-variant summary inline:
repoErr := fmt.Errorf("dispatch_repository tool %q has no repository target. Expected either 'repository' or 'allowed_repositories'. Example: repository: org/target-repo", toolKey)The YAML block for multiple repos can be kept but should use the same Example: label rather than Alternative Example:.
@copilot please address this.
| {name: "leading slash rejected", endpoint: "/issues/comments/123", wantErr: "must not start"}, | ||
| {name: "dotdot segment rejected", endpoint: "issues/../comments/123", wantErr: "must not contain"}, | ||
| {name: "leading slash rejected", endpoint: "/issues/comments/123", wantErr: "must not start", assertStyle: true}, | ||
| {name: "dotdot segment rejected", endpoint: "issues/../comments/123", wantErr: "must not contain", assertStyle: true}, |
There was a problem hiding this comment.
[/tdd] The new assertStyle bool field in the test table is used to conditionally check Expected and Example: — but all error-path rows in this PR now follow the new style. This guard flag adds complexity without protecting anything: if a future message regresses to the old style, the flag would hide the failure.
💡 Suggestion
Assert the style markers unconditionally on all error cases, removing the assertStyle field:
if tt.wantErr != "" {
require.Error(t, err)
require.ErrorContains(t, err, tt.wantErr)
require.ErrorContains(t, err, "Expected")
require.ErrorContains(t, err, "Example:")
}This makes the test a stronger specification and guards against future regressions.
@copilot please address this.
| field = field.Validate(func(s string) error { | ||
| if s == "" { | ||
| return errors.New("this input is required") | ||
| return fmt.Errorf("input '%s' is required. Expected a non-empty value in the interactive prompt. Example: enter a value for '%s' such as my-value", inputName, inputName) |
There was a problem hiding this comment.
[/codebase-design] The required-input validation error repeats inputName twice in the format string (input '%s' is required ... enter a value for '%s' such as my-value). The second repetition adds noise without new information and makes the message longer than it needs to be in a tight interactive prompt.
💡 Suggestion
return fmt.Errorf("input '%s' is required. Expected a non-empty value. Example: my-value", inputName)This stays within the style guide while keeping the prompt-facing message concise.
@copilot please address this.
There was a problem hiding this comment.
Test Quality Sentinel 🧪 — PR #52173 Analysis
Repository: github/gh-aw
PR: #52173 — "Raise error-message compliance in dispatch validation and CLI evaluation/logging paths"
Actor: pelikhan
Overview
This PR adds 5 new test functions across 5 test files to validate error-message compliance. All tests follow the error-message style guide: error messages must include "Expected" descriptors and "Example:" blocks.
Tests added: 5 behavioral tests (error-message validation)
Test inflation ratio: 0.67 (54 test lines / 81 production lines) ✅
Build tags: All 5 files have correct (go/redacted):build !integration tag ✅
Test Inventory
| File | Tests | Added | Type | Status |
|---|---|---|---|---|
pkg/cli/logs_command_test.go |
3 new | +29 lines | Error message validation | ✅ |
pkg/cli/outcome_eval_test.go |
1 modified | +15 lines | Error message validation (expanded) | ✅ |
pkg/cli/run_interactive_test.go |
1 new | +10 lines | Error message validation | ✅ |
pkg/workflow/dispatch_repository_test.go |
2 modified | +4 lines | Error message assertions added | ✅ |
pkg/workflow/dispatch_workflow_validation_test.go |
1 modified | 0 net lines | String assertion updated | ✅ |
Quality Summary
Scoring
design_tests / total_new_tests = 8 / 8 = 1.0 → 40 points
tests_with_edge_cases / total = 8 / 8 = 1.0 → 30 points
duplicate_clusters = 0 → 20 points
inflation_ratio = 0.67 ≤ 2:1 → 10 points
Total: 40 + 30 + 20 + 10 = 100
Test Quality Score: 100/100 ✅ Excellent
Implementation test ratio: 0/8 = 0% (threshold: 30%) ✅
Compliance
- ✅ All tests include descriptive assertion messages
- ✅ All test files have correct build tags (
(go/redacted):build !integration) - ✅ No Go mock libraries detected
- ✅ No forbidden mock violations
- ✅ All 8 test cases are behavioral contract tests validating error-message compliance
- ✅ Zero duplicated test patterns
- ✅ Well-proportioned test additions (0.67 test:prod ratio)
Key Findings
✅ All tests validate critical design invariants — Each test ensures error messages comply with the established style guide ("Expected" + "Example:" markers)
✅ High-value error coverage — Tests enforce compliance across 5 different validation functions (runtime, engine, report flags, repository format, empty tools, CI detection)
✅ No inflation or duplication — Clean test addition with no bloated or redundant test patterns
Recommendation
✅ APPROVE — This PR demonstrates exemplary test quality. All new tests validate critical design invariants with comprehensive error-path coverage, zero duplicates, and proper proportioning. The 100/100 score reflects strict adherence to error-message compliance standards.
|
@copilot please address the latest review feedback, refresh the branch if GitHub offers it, and run the pr-finisher skill. Open review items (newest first):
Please push a follow-up commit and resolve any addressed review threads.
|
🤖 Triage: PR #52173
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in the latest commits:
Branch is up to date with main and all changes are pushed. |
This PR addresses low compliance reported by
lint-error-messagesin five targeted files by normalizing validation/operator-facing errors to the repository style guide: what failed, what is expected, and a concrete example. Scope is limited to message text and closely-coupled assertions.dispatch-workflowvalidation messaging (pkg/workflow/dispatch_workflow_validation.go)workflow_dispatcherrors to include expected config shape and actionable YAML examples.dispatch_repositoryvalidation messaging (pkg/workflow/dispatch_repository_validation.go)allowed_repositoriesentry format in error text to match runtime validation behavior.Interactive run UX errors (
pkg/cli/run_interactive.go)Outcome evaluation input validation (
pkg/cli/outcome_eval.go)Logs command option validation (
pkg/cli/logs_command.go)--runtime/--engineand--report-fileflag-combination errors to include supported values and concrete command examples.Focused assertion updates (targeted tests only)
Example of the new style used across the touched paths: