Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has exactly 100 new lines of code in business logic directories (threshold is >100, so this PR does not meet the criteria).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #52137 only changes production error messages in pkg/workflow/ files. Test Quality Sentinel skipped.
|
|
✅ 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. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good overall improvement — adding contextual examples to validation errors significantly reduces the feedback loop for users fixing their workflow files. The vast majority of the ~100 updated messages are accurate and consistent.
Two issues found:
-
safe-outputs-github-appexample is wrong (checkout_config_parser.go:145): Shows{owner: github}but the field requires an app config withapp-id/client-idandprivate-key. This mirrors the pattern used correctly in thegithub-appexample. -
setup action referenceexample is misleading (compiler_pre_activation_job.go:33): The error fires when an internal compiler lookup fails — users don't configureactions/setup-node@v4directly — so appending that example may confuse rather than help.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 44.2 AIC · ⌖ 6.3 AIC · ⊞ 5.4K
| appMap, ok := value.(map[string]any) | ||
| if !ok { | ||
| return nil, fmt.Errorf("checkout.%s must be an object", fieldName) | ||
| return nil, fmt.Errorf("checkout.%s must be an object. Example: checkout: {%s: {owner: github}}", fieldName, fieldName) |
There was a problem hiding this comment.
The example for safe-outputs-github-app (and any safe-output-* app fields) is incorrect — it shows {owner: github} but this field is a GitHub App config that requires app-id/client-id and private-key. The next error message even validates that both are present.
Suggested fix:
checkout: {safe-outputs-github-app: {app-id: 123, private-key: '${{ secrets.APP_PRIVATE_KEY }}'}}
@copilot please address this.
| setupActionRef := c.resolveActionReference("./actions/setup", data) | ||
| if setupActionRef == "" { | ||
| return nil, errors.New("setup action reference is required but could not be resolved") | ||
| return nil, errors.New("setup action reference is required but could not be resolved. Example: actions/setup-node@v4") |
There was a problem hiding this comment.
This error is triggered when the compiler's internal resolveActionReference("./actions/setup", data) returns empty — it's an internal resolution failure, not a user-configurable field. The appended example actions/setup-node@v4 is misleading since users don't configure setup action references directly in workflow frontmatter.
Consider removing the example entirely or replacing it with actionable guidance, e.g.:
setup action reference is required but could not be resolved; ensure your workflow specifies a valid actions version in its configuration
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes for one broken test and two misleading examples.
📋 Key Themes & Highlights
Issues Found
- Broken test (
/tdd):checkout_manager_test.go:430asserts"checkout.fetch-depth must be >= 0"but the message was changed to"must be at least 0"— this will fail CI. - Misleading example (
/codebase-design): The internalsetup action referenceerror appendsExample: actions/setup-node@v4which is unrelated to the real failure cause. Users cannot fix this by changing their workflow. - Example re-states the error (
/codebase-design): The self-dependency error message appends aneedsexample that looks like the problematic configuration, not the fix.
Positive Highlights
- ✅ Consistent pattern: every message now ends with
. Example: <inline YAML>— easy to scan and learn from - ✅ Inline YAML style (not block) keeps examples compact in error output
- ✅ Great coverage — 100 messages updated in a single systematic pass
- ✅ No changes to logic paths; purely additive to error strings
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 51 AIC · ⌖ 7.16 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/checkout_config_parser.go:186
[/tdd] Broken test assertion: the message changed from "checkout.fetch-depth must be >= 0" to "checkout.fetch-depth must be at least 0" but checkout_manager_test.go:430 still asserts the old string — this will fail at test time.
<details>
<summary>💡 Fix</summary>
Update checkout_manager_test.go:430:
require.ErrorContains(t, err, "checkout.fetch-depth must be at least 0")</details>
@copilot please address this.
pkg/workflow/compiler_pre_activation_job.go:33
[/codebase-design] Misleading example: setup action reference is required but could not be resolved is an internal resolution failure (the compiler looks up ./actions/setup) that users cannot fix by writing actions/setup-node@v4. The appended example points at the wrong concept and may confuse users.
<details>
<summary>💡 Suggestion</summary>
This error fires when the internal action ref cannot be resolved — it is not user-configurable. Either remove the example entirely, or replace…
pkg/workflow/compiler_custom_jobs.go:229
[/codebase-design] The example for a self-dependency error is jobs: {%s: {needs: agent}} — but this is precisely the problem shape (a job depending on another job), not the fix. The user needs to know what a valid needs looks like when the cycle is removed, not another example that reproduces the error structure.
<details>
<summary>💡 Suggestion</summary>
Consider omitting the example for constraint-violation errors where any example would re-state the problem, or phrase it as the cor…
There was a problem hiding this comment.
Pull request overview
Improves validation diagnostics by adding correction examples to 100 error messages.
Changes:
- Adds YAML examples for configuration and job validation.
- Clarifies model identifier formats.
- Adds safe-output schema examples.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/stop_after.go |
Adds trigger-condition examples. |
pkg/workflow/safe_outputs_data_schema.go |
Adds schema correction examples. |
pkg/workflow/model_identifier.go |
Illustrates valid model identifiers. |
pkg/workflow/frontmatter_extraction_metadata.go |
Improves metadata validation messages. |
pkg/workflow/evals_config.go |
Adds valid eval configuration examples. |
pkg/workflow/compiler_pre_activation_job.go |
Improves pre-activation diagnostics. |
pkg/workflow/compiler_custom_jobs.go |
Adds custom-job configuration examples. |
pkg/workflow/checkout_config_parser.go |
Adds checkout correction examples. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (4)
pkg/workflow/checkout_config_parser.go:145
- This proposed correction contains only
owner, but the next validation requiresclient-id/app-idandprivate-key, so the example itself cannot parse successfully. Show the required credentials in the object.
return nil, fmt.Errorf("checkout.%s must be an object. Example: checkout: {%s: {owner: github}}", fieldName, fieldName)
pkg/workflow/compiler_custom_jobs.go:767
- This helper only processes built-in job names, and
stepsis rejected for built-ins other thanactivation(seevalidateRestrictedBuiltinSteps). Thus examples such asjobs.agent.stepsare not valid corrections. An empty object demonstrates the required type without introducing a prohibited field.
return fmt.Errorf("jobs.%s must be an object, got %T. Example: jobs: {%s: {steps: []}}", configuredJobName, rawConfig, configuredJobName)
pkg/workflow/compiler_custom_jobs.go:796
- Adding
steps: []does not cause a missing compiler-owned job to be generated, so this example does not explain how to resolve the reported failure; for several built-in jobs,stepsis prohibited as well. The correction needs to identify the feature/configuration that generatestargetJobName, or tell the author to remove that augmentation.
return fmt.Errorf("jobs.%s: cannot augment %q because this workflow does not generate that job. Example: jobs: {%s: {steps: []}}", augmentedField, targetJobName, configuredJobName)
pkg/workflow/compiler_custom_jobs.go:812
- Both dynamic examples can be invalid when
configuredJobNameisagent: they recommend the same self-dependency that line 809 rejects. For other built-ins, depending onagentmay also create a cycle. Show removal of the offending dependency as a correction that is valid for every built-in job.
return fmt.Errorf("jobs.%s.needs: %q cannot depend on itself. Example: jobs: {%s: {needs: agent}}", configuredJobName, rawNeed, configuredJobName)
}
if _, known := allJobs[need]; !known {
return fmt.Errorf("jobs.%s.needs: unknown job %q. Example: jobs: {%s: {needs: agent}}", configuredJobName, rawNeed, configuredJobName)
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
| appMap, ok := v.(map[string]any) | ||
| if !ok { | ||
| return nil, errors.New("checkout.github-app must be an object") | ||
| return nil, errors.New("checkout.github-app must be an object. Example: checkout: {github-app: {app-id: 123, private-key: '${{ secrets.APP_PRIVATE_KEY }}'}}") |
| } | ||
| if cfg.FetchDepth != nil && *cfg.FetchDepth < 0 { | ||
| return nil, errors.New("checkout.fetch-depth must be >= 0") | ||
| return nil, errors.New("checkout.fetch-depth must be at least 0. Example: checkout: {fetch-depth: 0}") |
| return needs, nil | ||
| default: | ||
| return nil, fmt.Errorf("jobs.%s.needs must be a string or array of strings, got %T", jobName, needsValue) | ||
| return nil, fmt.Errorf("jobs.%s.needs must be a string or array of strings, got %T. Example: jobs: {%s: {needs: agent}}", jobName, needsValue, jobName) |
| setupActionRef := c.resolveActionReference("./actions/setup", data) | ||
| if setupActionRef == "" { | ||
| return nil, errors.New("setup action reference is required but could not be resolved") | ||
| return nil, errors.New("setup action reference is required but could not be resolved. Example: actions/setup-node@v4") |
|
@copilot Please address the failing checks, refresh the branch if possible, and use the pr-finisher skill before handing back. Failed checks from the compact candidate set:
Please also review unresolved feedback, refresh the branch if GitHub allows it, and summarize what remains blocked.
|
|
Thanks for improving error message quality! 🎯 This PR responds to the validation audit with concrete examples across configuration parsing, model identifiers, and safe outputs—exactly what users need when they hit validation failures. Here is one thing to consider:
The PR is on-topic (error message quality is core to DX) and follows the core team agentic process documented in CONTRIBUTING.md. The diff is focused and tight (100 additions + 100 deletions across validation error messages only). If you'd like to add test coverage, here is a prompt you could give to your coding agent:
|
|
@copilot Please address the unresolved review feedback, refresh the branch if GitHub allows it, and run the Unresolved review threads to address (newest first):
Failed checks from the compact candidate set:
Branch update was requested by sous-chef in run https://github.com/github/gh-aw/actions/runs/31542137961.
|
|
@copilot run pr-finisher skill |
The error-message quality audit reported validation failures without concrete correction examples. This updates exactly 100 messages, raising compliance from 388 unresolved issues to 84%.