Fix error message compliance in 5 low-scoring pkg/workflow files - #52178
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. ✅
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #52178 for over-engineering per ponytail-review skill. This is a text-only change rewording error messages across 5 files for style-guide compliance; no new abstractions, dependencies, or structural complexity introduced. Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. This PR updates error message assertions in existing tests to match improved production error text—test maintenance only, not new test coverage.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories (74 additions detected).
|
|
✅ 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.
|
![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 improvements look good and follow the project style guide (constructive language, examples, actionable guidance). All test assertions updated correctly to match the new message substrings. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 35.4 AIC · ⌖ 6.16 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Rewords workflow validation errors to provide clearer expectations and corrective examples.
Changes:
- Improves schedule, trigger, repository configuration, custom-job, and integrity-reaction errors.
- Preserves existing validation control flow.
- Updates affected test assertions.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/trigger_parser.go |
Improves trigger validation messages. |
pkg/workflow/tools_validation_github_integrity_reactions.go |
Revises integrity-reaction errors. |
pkg/workflow/schedule_preprocessing.go |
Clarifies schedule validation failures. |
pkg/workflow/schedule_preprocessing_test.go |
Updates schedule error assertions. |
pkg/workflow/repo_config.go |
Improves repository configuration errors. |
pkg/workflow/repo_config_test.go |
Updates configuration error assertions. |
pkg/workflow/compiler_custom_jobs.go |
Revises custom-job processing errors. |
pkg/workflow/compiler_custom_jobs_test.go |
Updates custom-job error assertions. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
pkg/workflow/compiler_custom_jobs.go:1076
- The per-entry type error at line 1083 remains in its old noncompliant form even though this collection-level error was updated. Type mismatches require an expected shape and concrete example, so leaving it unchanged prevents the stated 100% compliance for this file.
return nil, fmt.Errorf("%s for job '%s' expects an array of step objects. Example: %s:\n - run: echo hello", fieldName, jobName, fieldName)
pkg/workflow/repo_config.go:408
- This rewritten duplicate-entry error still lacks the
Example:section expected by the referenced compliance check, so it remains one of this file's noncompliant messages and prevents the stated 100% result.
return fmt.Errorf("%s has duplicate maintenance.disabled_jobs entries %q and %q after normalization. Remove the duplicate so each job is listed once", RepoConfigFileName, previous, jobName)
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
| if hasExplicitReactionFields && !featureEnabled { | ||
| toolsValidationLog.Printf("Reaction fields present but integrity-reactions feature flag not enabled in workflow: %s", workflowName) | ||
| return errors.New("invalid guard policy: 'endorsement-reactions', 'disapproval-reactions', 'disapproval-integrity', and 'endorser-min-integrity' require the 'integrity-reactions' feature flag to be enabled. Add 'features: integrity-reactions: true' to your workflow") | ||
| return errors.New("guard policy fields 'endorsement-reactions', 'disapproval-reactions', 'disapproval-integrity', and 'endorser-min-integrity' require the 'integrity-reactions' feature flag. Example: features:\n integrity-reactions: true") |
| return fmt.Errorf("jobs.%s.needs lists %q, but a job should not depend on itself. Remove the self-reference from needs", configuredJobName, rawNeed) | ||
| } | ||
| if _, known := allJobs[need]; !known { | ||
| return fmt.Errorf("jobs.%s.needs: unknown job %q", configuredJobName, rawNeed) |
| } | ||
| if _, ok := validDisabledMaintenanceJobs[normalizedJobName]; !ok { | ||
| return fmt.Errorf("invalid %s: maintenance.disabled_jobs contains unrecognized job %q (valid values: close-expired-entities, apply_safe_outputs, label_disable_agentic_workflow, label_apply_safe_outputs)", RepoConfigFileName, jobName) | ||
| return fmt.Errorf("%s references unrecognized maintenance.disabled_jobs entry %q. Valid values are: close-expired-entities, apply_safe_outputs, label_disable_agentic_workflow, label_apply_safe_outputs", RepoConfigFileName, jobName) |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two structural issues and two test-weakening regressions.
📋 Key Themes & Highlights
Issues Found
-
Multi-line error strings — Several new messages embed literal
\ninfmt.Errorfstrings (compiler_custom_jobs.go lines 207, 315, 357, 377, 405; schedule_preprocessing.go line 280). Go error strings should be single-line; wrapping or logging these produces garbled output. Use inline prose examples instead. -
Inconsistent phrasing —
compiler_custom_jobs.goline 606 still uses"must be an object"while line 767 uses the updated"expects an object"— one instance was missed in the sweep. -
Weakened test assertion in schedule_preprocessing_test.go — The new substring
"schedule expression"(lines 462, 510) is much broader than the old"invalid schedule expression". Any future error containing those words would falsely pass. -
Incomplete test for UTC wrapper message —
repo_config_test.goline 425 only asserts the innerNormalizeUTCOffsetmessage, not the new outer wrapper"has an unsupported utc value", leaving the outer message untested.
Positive Highlights
- ✅ Clean, consistent pattern applied across all five files — the style guide format (what's wrong / expected / example) is well-implemented throughout.
- ✅ The
%wwrapping order fix inschedule_preprocessing.gois a good correctness improvement. - ✅ Test file updates are thorough and correctly track the new error substrings in most cases.
- ✅ PR description is excellent — clear before/after examples and a good summary of the scope.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 60.7 AIC · ⌖ 6.99 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_custom_jobs.go:207
[/codebase-design] Embedding literal in fmt.Errorf strings produces multi-line Go error messages, violating the idiomatic rule that error strings should be single-line. When wrapped or logged, the embedded newlines cause garbled output.
<details>
<summary>💡 Suggestion</summary>
Use inline prose instead of a YAML block in the example, or build a dedicated hint layer to present formatted examples separately from the error value.
// Instead of:
return fmt.Errorf("strategy field…
</details>
<details><summary>pkg/workflow/repo_config.go:307</summary>
**[/codebase-design]** The message "This indicates a bug in gh-aw; please report it" for a schema compilation failure is reasonable, but it would be more actionable if it included a link to the issue tracker or the specific schema symbol, so a user filing a bug report knows what to include.
<details>
<summary>💡 Suggestion</summary>
```go
return fmt.Errorf("could not compile the repo config schema: %w. This is a gh-aw bug — please open an issue at https://github.com/github/gh-aw/issues", err)…
</details>
<details><summary>pkg/workflow/repo_config_test.go:425</summary>
**[/tdd]** The test assertion `require.ErrorContains(t, err, "must be a numeric UTC offset")` will continue to pass because that substring comes from `NormalizeUTCOffset` in `utc_offset.go` (unchanged), not from the new wrapper message in `repo_config.go`. The wrapper message changed to `"has an unsupported utc value"`, but the test doesn't assert the new outer message — it only matches the inner error text.
This means the test would still pass even if the outer wrapper were accidentally remov…
</details>
<details><summary>pkg/workflow/schedule_preprocessing_test.go:462</summary>
**[/tdd]** The substring `"schedule expression"` is very broad — it would match any message that happens to contain those two words. The old assertion was `"invalid schedule expression"`, which was more discriminating. Now the test will pass for any error accidentally containing the word 'schedule', making it a weaker regression guard.
<details>
<summary>💡 Suggestion</summary>
Assert a more specific substring that uniquely identifies the new message, for example:
```go
errorSubstring: "sche…
</details>
<details><summary>pkg/workflow/compiler_custom_jobs.go:606</summary>
**[/codebase-design]** Line 606 still uses `"must be an object"` while line 767 uses the updated `"expects an object"` — inconsistent phrasing within the same file for the same kind of error.
<details>
<summary>💡 Suggestion</summary>
Update line 606 to match:
```go
return fmt.Errorf("jobs.%s expects an object, got %T. Example: jobs:
job-name:
setup-steps: []", jobName, jobConfig)(Though also consider the concern noted at line 207.)
</details>
@copilot please address this.
|
@copilot please address the open review threads below, refresh the branch if needed, and run the pr-finisher skill. Open review threads (newest first):
Please refresh the branch if GitHub offers it, then run the pr-finisher skill.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three open review threads:
Branch is rebased/up to date with |
🤖 Triage: PR #52178
|
…nce-one-more-time
|
@copilot please address the remaining open review threads below, refresh the branch if GitHub offers it, and run the pr-finisher skill. Open review threads (newest first):
The current PR author update says these were addressed, but the review threads still appear unresolved. Please either push the remaining fixes or resolve the threads with the reviewer-facing follow-up.
|
…x-error-message-compliance-one-more-time Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All three threads were already fixed in commit
Branch refreshed against latest |
|
Hey 👋 — thanks for this error message compliance fix! This is a well-scoped chore that improves consistency across the codebase. What's working well:
This looks ready for review.
|
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]), ranging from 0% to 74% compliant. Messages relied on standalone negative wording (invalid,cannot,must,failed) without pairing it with expected behavior or a concrete fix.Changes
schedule_preprocessing.go— reworded schedule/cron/timezone validation errors to state expected format and a concrete example; fixed%wwrapping order so example text no longer sandwiches the wrapped error.trigger_parser.go— reworded trigger shorthand, reaction, command/label conflict, and status-comment validation errors with expected values and examples.repo_config.go— rewordedruns_on/auto_upgrade/maintenanceparsing, file read/parse/schema, UTC offset, and cron field errors; replaced genericfailed to ...: %wwrapping with specific recovery guidance.compiler_custom_jobs.go— reworded job field YAML conversion, step processing, and job structure (needs,if, augmentation) errors to include expected shape and YAML examples.tools_validation_github_integrity_reactions.go— reworded guard-policy validation errors to list valid values and an example.schedule_preprocessing_test.go,repo_config_test.go, andcompiler_custom_jobs_test.gothat checked exact substrings of the old error text.No control flow or behavior changes — message text only.