Fix error message compliance: pre-activation jobs, GitHub tools validation, evals config, spec, and package manifest files - #52179
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 #52179 does not have the 'implementation' label and has 83 new lines of code in business logic directories (≤100 threshold).
|
|
✅ 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 test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Ponytail Reviewer completed successfully! Diff is purely error-message string rewrites (fmt.Errorf/errors.New text) across 5 files, no logic, control flow, or new abstractions added. Nothing to cut for over-engineering.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look good. All modifications are pure error message improvements across 5 files - no logic changes. Each message now uses a specific problem statement, includes an actionable remediation hint, and adds a concrete YAML example. No correctness, security, or reliability issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 35.1 AIC · ⌖ 6.2 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Updates validation and operational error messages across workflow compilation and CLI package/spec parsing.
Changes:
- Adds expected formats, recovery guidance, and configuration examples.
- Preserves wrapped errors while improving context.
- Several messages remain lint-noncompliant, and one breaks an existing test assertion.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tools_validation_github.go |
Improves GitHub tool validation errors. |
pkg/workflow/evals_config.go |
Adds actionable eval configuration errors. |
pkg/workflow/compiler_pre_activation_job.go |
Improves pre-activation validation diagnostics. |
pkg/cli/spec.go |
Clarifies repository and workflow specification errors. |
pkg/cli/add_package_manifest.go |
Improves package manifest and remote-resolution errors. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (5)
pkg/cli/add_package_manifest.go:247
- This changed message still fails
lint-error-messages: it containsinvalidandrequires/validwording but no literalExample:marker. Add a concrete valid build version example.
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version validation requires a semantic-versioned compiler, but the current compiler version %q is not a valid semantic version. This indicates a build issue; rebuild gh-aw with a proper version tag", manifestPath, currentVersion)
pkg/cli/add_package_manifest.go:251
- This remains lint-noncompliant because
invalidis present without a literalExample:marker. The recovery guidance is useful, but the acceptance check will still flag this line unless a concrete example is added.
return nil, nil, fmt.Errorf("invalid Agentic Workflow manifest %q: min-version %q requires gh-aw %s or newer (current: %s). Upgrade gh-aw or lower min-version in aw.yml", manifestPath, manifest.MinVersion, manifest.MinVersion, currentVersion)
pkg/cli/add_package_manifest.go:774
- This changed validation error still contains
invalidandcannotwithout the literalExample:marker required bylint-error-messages. Add a valid frontmatter example.
return fmt.Errorf("invalid Agentic Workflow manifest %q: workflow %q sets private: true and cannot be included because private workflows cannot be added. Remove 'private: true' from the workflow frontmatter or exclude it from the manifest", manifestPath, installationSource)
pkg/cli/add_package_manifest.go:832
- The linter requires the exact
Example:marker for validation errors;e.g.does not satisfy it. Since this message still begins withinvalid, the target workflow will continue to flag this line.
return nil, true, fmt.Errorf("invalid repository package path %q: path traversal outside the repository is not allowed. Use a path relative to the repository root, e.g. 'packages/my-package'", packagePath)
pkg/cli/add_package_manifest.go:886
- This remains non-compliant because both
invalidandduplicatetriggerlint-error-messages, while the message lacks a literalExample:marker. Add a concrete rename example.
return fmt.Errorf("invalid Agentic Workflow manifest %q: duplicate workflow filename %q in files entries %q and %q. Filenames must be unique across a package; rename one of the workflow files", manifestPath, filenameWithoutExt, previous, installPath)
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
| "jobs.%s.setup-steps is not supported. setup-steps are refused for activation/pre-activation jobs because they can short-circuit protections. Use 'steps', 'outputs', or 'pre-steps' instead", | ||
| jobName, |
| for i, q := range cfg.Questions { | ||
| if _, dup := seen[q.ID]; dup { | ||
| return fmt.Errorf("evals: duplicate id %q at index %d", q.ID, i) | ||
| return fmt.Errorf("evals has a duplicate id %q at index %d. Use a unique 'id' for each question", q.ID, i) |
| return readmePath, nil | ||
| } else if isRepositoryFileNotFound(err) { | ||
| return "", fmt.Errorf("repository %q is not a valid Agentic Workflow package: missing required README.md at %q", packageID, readmePath) | ||
| return "", fmt.Errorf("repository %q is not a valid Agentic Workflow package: missing required README.md at %q. Add a README.md describing the package", packageID, readmePath) |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness issues and two maintainability gaps.
📋 Key Themes & Highlights
Issues
- String concatenation for runtime values (
tools_validation_github.golines 174, 210): Two error messages embed runtime values via+instead offmt.Errorf/%q, diverging from every other error in those files and skipping safe quoting. - Missed peer site (
fetch.go:95): TheresolveRepositoryPackageslug check was updated, but the nearly identical check infetch.gowas not, leaving inconsistent messages for the same failure mode. - Repeated YAML example literal (
evals_config.go): The same 3-line YAML snippet is copy-pasted 10 times; one schema change means 10 manual edits. - No assertion on new message shape (
compiler_pre_activation_job.go): The multi-lineExample:\nblock in job errors has no test, so a future indentation tweak would silently corrupt the message.
Positive Highlights
- ✅ Consistent application of the
[what's wrong]. [what's expected]. [example]template across all five files. - ✅
%wcorrectly kept as the trailing verb on wrapped errors — no regression on sentinel matching. - ✅ Actionable hints on network errors (
check the repository, ref, and network connectivity) follow the existing codebase style. - ✅ Path traversal error is still a substring match with the existing test assertion.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 69.9 AIC · ⌖ 6.98 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/tools_validation_github.go:174
[/diagnosing-bugs] String concatenation for the github.MinIntegrity runtime value makes this message harder to test and diverges from fmt.Errorf used in every other error in this file that embeds a value.
<details>
<summary>💡 Suggested fix</summary>
return fmt.Errorf("'github.min-integrity' must be one of: 'none', 'unapproved', 'approved', 'merged'. Got: %q. Example:\ntools:\n github:\n min-integrity: approved", github.MinIntegrity)Using %q also adds consistent quot…
pkg/workflow/tools_validation_github.go:210
[/diagnosing-bugs] Same string-concatenation pattern as line 174: reposStr is embedded via + rather than fmt.Errorf with %q. This is inconsistent with the rest of the file and makes the value harder to quote/escape safely.
<details>
<summary>💡 Suggested fix</summary>
return fmt.Errorf("'github.allowed-repos' string must be 'all', 'public', or '${{ github.repository }}'. Got: %q. Example:\ntools:\n github:\n allowed-repos: all", reposStr)</details>
@copilot please…
pkg/cli/add_package_manifest.go:82
[/codebase-design] fetch.go line 95 has a near-identical check (strings.SplitN(spec.RepoSlug, "/", 2)) with the old-style message "invalid repository slug: %s" that was not updated in this PR. The two paths produce inconsistent messages for the same failure.
<details>
<summary>📋 Details</summary>
pkg/cli/fetch.go:95:
return nil, fmt.Errorf("invalid repository slug: %s", spec.RepoSlug)Shouldn't this get the same "repository slug %q is not in 'owner/repo' format" trea…
pkg/workflow/evals_config.go:457
[/tdd] The same example YAML snippet (evals:\n - id: readme\n question: Does the README explain setup?) is repeated verbatim in 10 error messages across this file. If the schema changes (e.g., a new required field), all 10 strings need to be updated in sync — a fragile maintenance pattern.
<details>
<summary>💡 Suggested refactor</summary>
Extract a package-level constant or helper:
const evalsExample = "Example:\nevals:\n - id: readme\n question: Does the README explain …
</details>
<details><summary>pkg/workflow/compiler_pre_activation_job.go:677</summary>
**[/tdd]** The `jobs.<name>` errors now embed multi-line YAML examples via `\n`. There are no tests asserting the exact new message text — only the old messages in `compiler_pre_activation_job_test.go`. A future edit to the YAML indentation could silently corrupt the message, and no test would catch it.
<details>
<summary>💡 Suggestion</summary>
Add at least one `require.ErrorContains(t, err, "Example:")` assertion in the existing test for this code path to ensure the example block survives f…
</details>
…markers Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all five review threads in 7b7f35c:
|
🤖 Triage: PR #52179
|
Error messages flagged as low-compliance by the
lint-error-messagesworkflow across five files did not follow the repo's style guide, using standalone negative wording (invalid,must) without pairing it with expected behavior and a concrete fix.Changes
pkg/workflow/compiler_pre_activation_job.go: rewrote type/shape validation errors (jobs.*,on.steps,on.needs,on.restore-memory) to include expected format and a minimal YAML example.pkg/workflow/tools_validation_github.go: rewrote GitHub tool and guard-policy errors (read-only,github-app/github-tokenconflict,min-integrity,allowed-repos, repository patterns) with expected values/formats and example config snippets.pkg/workflow/evals_config.go: rewroteevalsparsing/validation errors (missing/invalidid,question,model, duplicate ids) with example YAML.pkg/cli/spec.go: rewrote repo/workflow spec and URL parsing errors with expected formats and examples.pkg/cli/add_package_manifest.go: rewrote manifest parsing/validation and remote-fetch errors with actionable guidance (e.g. addingaw.yml, fixingmin-version, README requirements).Approach
[what's wrong]. [what's expected]. [example]template consistently.%w), moved actionable hints before the wrap point so%wremains the trailing verb, e.g.: