Fix error message compliance in runs_on_validation, frontmatter_extraction_metadata, reactions, call_workflow_validation, and ambient_folders - #52177
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #52177 for over-engineering. This PR only rewords existing error message strings to comply with the style guide ([what's wrong]. [what's expected]. [example]) — no new abstractions, structures, control flow, or logic were introduced. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52177 does not have the 'implementation' label and has only 36 new lines of code in business logic directories (threshold: 100).
|
|
🧠 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. No new test functions were added in this PR. Changes are limited to updating error message assertions in existing tests (frontmatter_types_test.go, runs_on_validation_test.go, tracker_id_test.go) to match new error message compliance standards. These are test expectation updates, not behavioral test additions.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes consistently improve error messages across runs_on_validation, frontmatter_extraction_metadata, reactions, call_workflow_validation, and ambient_folders to follow the 'has X, expected Y. Example: ...' pattern. Tests are updated to match the new message fragments. No logic changes, no security concerns. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.1 AIC · ⌖ 6.22 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — approving with one minor note on test specificity.
📋 Key Themes & Highlights
Key Themes
- Error message quality: All changes consistently follow the repo's style guide pattern — field name, actual type/value, expected constraint, YAML example. Well done.
- Test updates: Test assertions are updated to match the new wording throughout. One case (tracker-id) loosened the assertion to a very generic substring; see inline comment.
Positive Highlights
- ✅ Pure message-text change — no control-flow or logic mutations, easy to verify
- ✅ Multiline YAML examples in
call_workflow_validationerror messages are a nice touch - ✅ Consistent pattern applied across all five files
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 20.7 AIC · ⌖ 6.49 AIC · ⊞ 7K
Comment /matt to run again
| }, | ||
| { | ||
| name: "Tracker-id too long (129 chars)", | ||
| frontmatter: map[string]any{"tracker-id": strings.Repeat("a", 129)}, |
There was a problem hiding this comment.
[/tdd] Test assertions loosened to generic substrings.
Three cases now assert on 'unsupported character' instead of the prior 'tracker-id contains invalid character'. The new string is generic enough that an unrelated validation path could satisfy it, weakening regression detection.
Consider anchoring with the field name: errorMsg: "tracker-id has unsupported character"
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Test Maintenance Only
📊 Analysis Summary
Modified Files:
Verdict
|
There was a problem hiding this comment.
Pull request overview
Updates workflow validation errors to be more actionable and align with the repository’s error-message style guide.
Changes:
- Adds expected formats, valid options, and YAML examples to validation errors.
- Reports received types and values more clearly.
- Updates affected test expectations.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tracker_id_test.go |
Updates tracker ID error assertions. |
pkg/workflow/runs_on_validation.go |
Improves runs-on validation messages. |
pkg/workflow/runs_on_validation_test.go |
Updates runs-on error assertions. |
pkg/workflow/reactions.go |
Improves reaction configuration errors. |
pkg/workflow/frontmatter_types_test.go |
Updates frontmatter validation assertions. |
pkg/workflow/frontmatter_extraction_metadata.go |
Improves tracker and timeout errors. |
pkg/workflow/call_workflow_validation.go |
Adds valid workflow_call examples. |
pkg/workflow/ambient_folders.go |
Improves ambient-folder validation errors. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
pkg/workflow/ambient_folders.go:61
- This type-mismatch message does not identify the received entry type, leaving users unable to distinguish which kind of value caused validation to fail. Include
%Twithvalue, as the updated type errors elsewhere in this PR do.
return nil, errors.New("ambient-folders entry has an unsupported type, expected a string folder path. Example: ambient-folders: [docs, src/lib]")
pkg/workflow/ambient_folders.go:78
- The stated constraint does not match validation: paths such as
docs/../srccontain..but are accepted afterfilepath.Clean, while.is rejected without containing either forbidden form. Describe the actual normalized-path requirement rather than telling users that all..is disallowed.
return nil, fmt.Errorf("ambient-folders entry %q is not a relative path within the repository, expected a relative folder path without '..' or a leading '/'. Example: ambient-folders: [docs, src/lib]", folder)
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
| default: | ||
| frontmatterMetadataLog.Printf("Invalid tools.timeout type: %T", timeoutValue) | ||
| return "", fmt.Errorf("tools.timeout must be an integer or a GitHub Actions expression, got %T", timeoutValue) | ||
| return "", fmt.Errorf("tools.timeout has type %T, expected an integer or a GitHub Actions expression. Example:\ntools:\n timeout: 60", timeoutValue) |
| return nil | ||
| } | ||
| return fmt.Errorf("call-workflow: workflow '%s' does not support workflow_call trigger (must include 'workflow_call' in the 'on' section)", workflowName) | ||
| return fmt.Errorf("call-workflow: workflow '%s' does not support the workflow_call trigger, expected 'workflow_call' in the 'on' section. Example:\non:\n workflow_call:", workflowName) |
| } | ||
| } else { | ||
| return nil, errors.New("ambient-folders must be an array of folder paths") | ||
| return nil, errors.New("ambient-folders has an unsupported type, expected an array of folder path strings. Example: ambient-folders: [docs, src/lib]") |
| } | ||
| default: | ||
| return fmt.Errorf("invalid runs-on object key %q: expected only group or labels", key) | ||
| return fmt.Errorf("runs-on object key '%s' is not supported, expected 'group' or 'labels'. Example: runs-on:\n group: my-runner-group\n labels: [self-hosted]", key) |
The
lint-error-messagesworkflow flagged fivepkg/workflowfiles as low-compliance with the repo's error-message style guide ([what's wrong]. [what's expected]. [example]), relying instead on standalone negative wording likeinvalid,must,cannotwithout actionable guidance.Changes
runs_on_validation.go— rewrote all 6 type-mismatch messages forruns-onto state the actual type received, the expected shape, and a YAML examplefrontmatter_extraction_metadata.go— rewordedtracker-idlength/character validation andtools.timeout/tools.startup-timeouttype errors to include expected constraints and examplesreactions.go— rewrote reaction value/type validation errors to list valid options and a usage example instead of baremust be one ofcall_workflow_validation.go— added expectedon.workflow_callYAML snippet to the two messages missing itambient_folders.go— reworded type, empty-value, path-traversal, and character-validation errors with expected format and examplesNo behavior or control-flow changes — only error message text. Existing unit tests asserting on exact error strings were updated to match the new wording.
Before/after example: