Fix error message compliance in 5 pkg/workflow files - #52180
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. ✅
|
|
✅ 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.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed PR #52180 diff for over-engineering: all changes are mechanical error-message text additions (Example/Expected clauses) via the pre-existing NewValidationError helper. No new abstractions, dependencies, or dead flexibility introduced — nothing to cut.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (228 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Pull request overview
Updates workflow validation errors to be actionable and example-driven.
Changes:
- Adds expected formats and YAML examples to validation errors.
- Converts engine-driver errors to structured
NewValidationErrorvalues. - Adds representative error-quality tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/checkout_config_parser.go |
Improves checkout errors. |
pkg/workflow/safe_outputs_data_schema.go |
Clarifies schema errors. |
pkg/workflow/stop_after.go |
Adds trigger configuration examples. |
pkg/workflow/model_identifier.go |
Expands identifier syntax guidance. |
pkg/workflow/engine_driver_validation.go |
Introduces structured validation errors. |
pkg/workflow/error_message_quality_test.go |
Tests representative messages. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
pkg/workflow/stop_after.go:358
- The singular
Example:combines both accepted forms into one YAML document with duplicateskip-if-no-matchkeys. That document is rejected or interpreted ambiguously rather than demonstrating correct usage. Keep one valid example.
return nil, fmt.Errorf("skip-if-no-match value must be a string or object, got %T. Example:\n skip-if-no-match: \"is:pr is:open\"\n skip-if-no-match:\n query: \"is:pr is:open\"\n min: 3", skipIfNoMatch)
pkg/workflow/stop_after.go:495
- This
Example:contains three duplicateskip-if-check-failingkeys, including a null-valued one, so it is not a single valid corrective example. Show one accepted object form instead.
return nil, fmt.Errorf("skip-if-check-failing value must be true or an object, got %T. Example:\n skip-if-check-failing:\n skip-if-check-failing: true\n skip-if-check-failing:\n include:\n - build\n branch: main\n allow-pending: true", skipIfCheckFailing)
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Balanced
| cfg, err := checkoutConfigFromMap(singleMap) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid checkout configuration: %w", err) | ||
| return nil, fmt.Errorf("invalid checkout configuration, expected an object with checkout fields such as repository, ref, or path: %w", err) |
| }, nil | ||
| default: | ||
| return nil, fmt.Errorf("skip-if-match value must be a string or object, got %T. Examples:\n skip-if-match: \"is:issue is:open\"\n skip-if-match:\n query: \"is:pr is:open\"\n max: 3", skipIfMatch) | ||
| return nil, fmt.Errorf("skip-if-match value must be a string or object, got %T. Example:\n skip-if-match: \"is:issue is:open\"\n skip-if-match:\n query: \"is:pr is:open\"\n max: 3", skipIfMatch) |
| return fmt.Errorf("model identifier: character %q is not allowed in model token %q (segment type: model)", r, token) | ||
| } | ||
| return fmt.Errorf("model identifier: model token %q is syntactically invalid (segment type: model)", token) | ||
| return fmt.Errorf("model identifier: model token %q is syntactically invalid (segment type: model). Expected letters, digits, '-', '_', or '.'. Example: model: openai/gpt-4o", token) |
| return fmt.Errorf("model identifier: character %q is not allowed in bare name %q (segment type: alias)", r, name) | ||
| } | ||
| return fmt.Errorf("model identifier: bare name %q is syntactically invalid (segment type: alias)", name) | ||
| return fmt.Errorf("model identifier: bare name %q is syntactically invalid (segment type: alias). Expected letters, digits, '-', '_', or '.'. Example: model: gpt-4o", name) |
| "engine.driver", | ||
| workflowData.EngineConfig.ID, | ||
| "inline engine.driver sources are only supported for the copilot engine", | ||
| fmt.Sprintf("Set engine.id to copilot, or use a driver file path instead of inline source.\n\nExample:\nengine:\n id: copilot\n driver:\n node: |\n console.log(\"hello\")\n\nSee: %s", constants.DocsEnginesURL), |
| "engine.driver."+inlineDriver.Runtime, | ||
| "", | ||
| fmt.Sprintf("engine.driver.%s must not be empty", inlineDriver.Runtime), | ||
| fmt.Sprintf("Provide the inline driver source.\n\nExample:\nengine:\n driver:\n %s: |\n console.log(\"hello\")\n\nSee: %s", inlineDriver.Runtime, constants.DocsEnginesURL), |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Error Message Compliance
This PR systematically improves error messages across 5 pkg/workflow files, adding context and Example: blocks following the project's error-message style guide. Overall the changes are correct and well-structured.
What's good
NewValidationErroris used correctly inengine_driver_validation.go, with separatefield,reason, andsuggestionparameters.Examples:→Example:normalisation is applied consistently throughoutstop_after.go.- New test cases in
error_message_quality_test.goverify that each improved message contains the expected substrings. - Model identifier, safe-outputs data schema, and stop-after errors all now include actionable examples.
One issue
See inline comment on checkout_config_parser.go line 124: the checkout.token (deprecated field) error message example uses github-token without explaining why, mixing type-error guidance with deprecation migration guidance in a confusing way.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 76.8 AIC · ⌖ 6.39 AIC · ⊞ 5.4K
| s, ok := v.(string) | ||
| if !ok { | ||
| return nil, errors.New("checkout.token must be a string") | ||
| return nil, errors.New("checkout.token must be a string. Example:\ncheckout:\n github-token: ${{ secrets.MY_TOKEN }}") |
There was a problem hiding this comment.
The error for the deprecated token key says checkout.token must be a string but the example redirects to github-token without explaining why:
return nil, errors.New("checkout.token must be a string. Example:\ncheckout:\n github-token: ${{ secrets.MY_TOKEN }}")A user who passes token: 123 (wrong type) sees an example using a different field name, conflating type guidance with deprecation migration guidance. This can be confusing — they may think the fix is to rename the key, not to change the value type.
Suggested improvement:
return nil, errors.New("checkout.token must be a string (\"token\" is deprecated; prefer \"github-token\"). Example:\ncheckout:\n github-token: ${{ secrets.MY_TOKEN }}")This makes the deprecation intent explicit so the example makes sense in context.
@copilot please address this.
Test Quality Sentinel Report — PR #52180PR Overview
Test Files Modified (1 file)
Test Modifications BreakdownCommit 162a3ba ("Improve error message compliance in 5 workflow files"):
Commit 0d15c20 ("Fix expected error path in data schema quality test"):
New Test Cases Added (5 total)Test Case Classification
Assertion Quality AnalysisFramework: Go with Pattern Per Test:
Red Flag Check:
Test vs Production Ratio
Design Contracts Verified
Quality Score✅ Test Quality Score: 100/100 — Excellent CalculationPass Criteria
Conclusion✅ APPROVE — Exemplary test quality This PR demonstrates excellent testing practices:
Test Quality Sentinel verdict: Ready to merge ✅
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three targeted issues.
📋 Key Themes & Highlights
Key Themes
- Example field mismatch: the
checkout.tokenbackward-compat error shows agithub-tokenexample, which misdirects users to the wrong field. - Lost diagnostic context: two
NewValidationErrorcalls inengine_driver_validation.gopassvalue: "", silently suppressing theValue:section users rely on for triage. - Inconsistent example format:
model_identifier.goappends. Example: model: openai/gpt-4o(space-separated, same-line) while all other changed files use a newline-separated YAML block. - Missing test case: the
skip-if-no-matcherror messages updated instop_after.gohave no correspondingTestErrorMessageQualitycase.
Positive Highlights
- ✅ Systematic and focused change — no control-flow or logic touches
- ✅
engine_driver_validation.gocorrectly migrated toNewValidationError()per the*_validation.goconvention - ✅ 80 lines of new test coverage exercise the new message format across all five files
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 112.8 AIC · ⌖ 6.91 AIC · ⊞ 7K
Comment /matt to run again
| s, ok := v.(string) | ||
| if !ok { | ||
| return nil, errors.New("checkout.token must be a string") | ||
| return nil, errors.New("checkout.token must be a string. Example:\ncheckout:\n github-token: ${{ secrets.MY_TOKEN }}") |
There was a problem hiding this comment.
[/codebase-design] The checkout.token type error shows a github-token example — a user who wrote token: 123 sees a fix pointing to the new field name instead of the field they actually used.
💡 Suggested fix
Show the deprecated field in the example, with a note to prefer the new field:
return nil, errors.New("checkout.token must be a string. Example:
checkout:
token: \${{ secrets.MY_TOKEN }}
# Prefer checkout.github-token in new workflows")@copilot please address this.
| return fmt.Errorf("engine.driver: exactly one runtime key is allowed (node, python, go, java); found multiple.\n\nSee: %s", constants.DocsEnginesURL) | ||
| return NewValidationError( | ||
| "engine.driver", | ||
| "", |
There was a problem hiding this comment.
[/codebase-design] value: "" passed to NewValidationError for the MultipleRuntime case drops diagnostic context — the rendered error will silently skip the Value: section, giving less signal than the old fmt.Errorf message.
💡 Suggested fix
Provide a meaningful value string so the user sees which field triggered the error. For the multiple-runtime case you could stringify the keys found:
return NewValidationError(
"engine.driver",
"multiple runtime keys",
"engine.driver accepts exactly one runtime key (node, python, go, java); found multiple",
...
)Same pattern applies to the empty-source case at line ~230: pass inlineDriver.Runtime as value so the field name in context is visible.
@copilot please address this.
| modelIdentifierLog.Printf("Parsing model identifier: %q", s) | ||
| if s == "" { | ||
| return nil, errors.New("model identifier must not be empty") | ||
| return nil, errors.New("model identifier must not be empty. Expected a bare alias, or a provider-scoped name. Example: model: openai/gpt-4o") |
There was a problem hiding this comment.
[/codebase-design] The Example: guidance is appended inline as . Example: model: openai/gpt-4o (space-separated, same line) while every other changed file uses the pattern . Example: key: field: value (newline-separated YAML block). The inconsistency makes the output less readable in terminals, which render multi-line errors with indented context.
💡 Suggested fix
Use a newline-prefixed YAML block for consistency:
return nil, errors.New("model identifier must not be empty. Expected a bare alias, or a provider-scoped name.
Example:
model: openai/gpt-4o")The same pattern should be applied to all the other appended examples in this file (lines ~233, ~240, ~245, ~253, ...).
@copilot please address this.
| }, | ||
| shouldNotBeVague: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
[/tdd] A test case was added for skip-if-match but the parallel skip-if-no-match error messages updated in the same diff (lines ~722–732 of stop_after.go) have no coverage here — only the skip-if-match path is exercised.
💡 Suggested addition
Add a sibling test case:
{
name: "skip-if-no-match query type error includes example",
testFunc: func() error {
c := NewCompiler()
frontmatter := map[string]any{
"on": map[string]any{
"skip-if-no-match": map[string]any{
"query": 123,
},
},
}
_, err := c.extractSkipIfNoMatchFromOn(frontmatter)
return err
},
shouldContain: []string{
"skip-if-no-match 'query' field must be a string",
"Example:",
"query:",
},
shouldNotBeVague: true,
},@copilot please address this.
The
errormessagelinter flagged fivepkg/workflowfiles as low-compliance (13–48%): error messages used negative wording (invalid,must,cannot,failed) without stating expected behavior or showing a fix. This rewrites the flagged messages per.github/skills/error-messages/SKILL.md([what's wrong]. [what's expected]. [example]). No control-flow or validation-logic changes.Changes
checkout_config_parser.go— each type/shape error now names the expected type and shows the YAML snippet. The one wrapping error was rephrased so the wrapped cause stays at the end of the message rather than mid-sentence.safe_outputs_data_schema.go— schema errors state the expected shape and include a schema fragment (properties:,required: [...],additionalProperties: false).stop_after.go—on:,stop-after, andskip-if-*errors gained expected-format text and examples. Pre-existingExamples:headers were changed toExample:; the linter matches whole words, so the plural form was not counted as guidance.model_identifier.go— ABNF grammar violations now describe the expected token shape with a concrete identifier example.engine_driver_validation.go— converted all 14fmt.Errorfcalls toNewValidationError(field, value, reason, suggestion), as the linter requires for*_validation.go. Substrings asserted by existing tests (safe basename,unsupported extension,empty path segments, …) are preserved inreason.error_message_quality_test.go— one newTestErrorMessageQualitycase per touched file.Example
The analyzer (
/tmp/gh-aw-linters -errormessage.changed-files=…) reports no findings for these five files after the change.