Fix compiler error quality: YAML context offset, import hint, early engine validation#41234
Conversation
… validation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves gh aw compiler error UX by ensuring YAML frontmatter errors include the correct source line, adding actionable hint: text to import errors, and validating engine: early so engine typos aren’t masked by import failures.
Changes:
- Fix frontmatter source-context extraction so the highlighted
> N |line is included in YAML syntax error snippets. - Add tailored import-error hints via
CompilerError.Hintto improve actionability for common import failure modes. - Validate
engine:immediately after builtin-engine import injection (and before import processing) to preserve error precedence for invalid engine names.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/frontmatter_error.go | Updates the regex used to locate the start of YAML source-context output so the highlighted error line isn’t dropped. |
| pkg/workflow/compiler_yaml_test.go | Adds regression tests for YAML snippet inclusion, engine-vs-import precedence, and import hint rendering in full compilation output. |
| pkg/workflow/compiler_orchestrator_engine.go | Adds early engine validation before import processing to prevent import failures from shadowing invalid engine errors. |
| pkg/parser/import_error.go | Adds buildImportErrorHint() and populates CompilerError.Hint in FormatImportError. |
| pkg/parser/import_error_test.go | Updates expectations to require hint: output across import error variants. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
| func buildImportErrorHint(message, importPath string) string { | ||
| switch { | ||
| case strings.Contains(message, "file not found") || strings.Contains(message, "failed to resolve import"): | ||
| return fmt.Sprintf("Ensure `%s` exists relative to the workflow directory, or check the `imports:` path.", importPath) | ||
| case strings.Contains(message, "failed to download"): | ||
| return fmt.Sprintf("Check that the remote import `%s` is accessible and the path is correct.", importPath) | ||
| case strings.Contains(message, "failed to resolve ref"): | ||
| return fmt.Sprintf("Verify the ref (branch, tag, or SHA) in `%s` is valid and accessible.", importPath) | ||
| case strings.Contains(message, "invalid import specification"): | ||
| return "Use the format `owner/repo/path@ref` for remote imports (e.g. `github/my-org/shared.md@main`)." | ||
| default: | ||
| return fmt.Sprintf("Ensure `%s` exists relative to the workflow directory, or check the `imports:` path.", importPath) | ||
| } | ||
| } |
| // Should include a "Did you mean: copilot?" suggestion | ||
| if !strings.Contains(errorStr, "copilot") { | ||
| t.Errorf("error should mention 'copilot' as a suggestion, got: %s", errorStr) | ||
| } |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (135 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 MatterThis PR encodes a new compiler convention — engine errors take precedence over import errors. That ordering is invisible in the code but matters to anyone who later adds a new error kind and has to decide where it slots in. An ADR makes the why searchable and permanent for future contributors (and your future self). 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes for one correctness bug and two follow-up improvements.
📋 Key Themes & Highlights
One correctness bug (blocking)
- Dead hint for ref errors (
import_error.goline 154): thecase strings.Contains(message, "failed to resolve ref")branch is unreachable.FormatImportErrorsetsmessage = "failed to resolve import reference", which contains"failed to resolve import"— so case 1 fires first and users see the wrong "file not found" hint for ref typos. The fix is a one-line change to the case predicate, plus a matching test.
Two improvements (non-blocking but worth fixing)
defaulthint misleads (import_error.goline 159): the fallback returns the same "Ensure the file exists" copy as"file not found". For unknown or unexpected causes it will actively mislead. An empty string or a genuinely generic message is safer.- Double
engineCatalog.Resolve(compiler_orchestrator_engine.goline 55): the early-validation result is discarded;resolveEngineRuntimeConfigresolves again on every successful build. Cheap and idempotent in practice, but a one-line comment would clarify the intent for future readers.
Positive highlights
- ✅ The
sourceContextPatternregex fix is minimal and correct —>?\s*\d+\s*\|precisely captures the highlighted YAML error line. - ✅ 102 new test lines for ~29 net production lines is an excellent ratio.
- ✅ The
TestInvalidEngineReportedBeforeImportErrorstest cleanly specifies the ordering invariant. - ✅ Early engine guard condition (
engineSetting != "" && c.engineOverride == "") is well-documented and handles the "engine from import" case correctly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 76.6 AIC · ⌖ 8.38 AIC · ⊞ 6.5K
| return fmt.Sprintf("Ensure `%s` exists relative to the workflow directory, or check the `imports:` path.", importPath) | ||
| case strings.Contains(message, "failed to download"): | ||
| return fmt.Sprintf("Check that the remote import `%s` is accessible and the path is correct.", importPath) | ||
| case strings.Contains(message, "failed to resolve ref"): |
There was a problem hiding this comment.
[/diagnose] Dead code: this case will never fire. FormatImportError sets message = "failed to resolve import reference" (line 120 above), which contains "failed to resolve import" — so case 1 matches first, and users see the misleading "Ensure the file exists" hint instead of the intended "Verify the ref" hint.
💡 Fix suggestion
Change the switch case to match the actual message string that FormatImportError produces:
case message == "failed to resolve import reference":
return fmt.Sprintf("Verify the ref (branch, tag, or SHA) in `%s` is valid and accessible.", importPath)And add a regression test in TestFormatImportError:
{
name: "ref resolution error",
err: &parser.ImportError{
ImportPath: "owner/repo/file.md@v0.0.0-bad",
FilePath: "test.md",
Line: 3, Column: 3,
Cause: errors.New("failed to resolve ref v0.0.0-bad: not found"),
},
wantContain: []string{"hint:", "ref", "owner/repo/file.md@v0.0.0-bad"},
},| // when engineSetting is empty (engine may come from an import) or when a | ||
| // command-line --engine override is active (it will be validated later). | ||
| if engineSetting != "" && c.engineOverride == "" { | ||
| if _, err := c.engineCatalog.Resolve(engineSetting, engineConfig); err != nil { |
There was a problem hiding this comment.
[/zoom-out] The result of this early Resolve() is discarded (_, err). On every successful compilation, c.engineCatalog.Resolve() is called a second time further down in resolveEngineRuntimeConfig. If resolution is cheap and idempotent this is fine, but the double call is non-obvious to future readers.
💡 Options
Option A — add a comment acknowledging the double call:
// Eagerly validate the engine name; the resolved result is discarded because
// applyEngineImportDefaults may mutate engineConfig before the final
// resolveEngineRuntimeConfig call.
if _, err := c.engineCatalog.Resolve(engineSetting, engineConfig); err != nil {Option B — cache and reuse, threading the result through so resolveEngineRuntimeConfig skips the second Resolve when it already has a valid result.
Either way, a brief comment prevents the next reader from wondering whether the double call is intentional.
| case strings.Contains(message, "invalid import specification"): | ||
| return "Use the format `owner/repo/path@ref` for remote imports (e.g. `github/my-org/shared.md@main`)." | ||
| default: | ||
| return fmt.Sprintf("Ensure `%s` exists relative to the workflow directory, or check the `imports:` path.", importPath) |
There was a problem hiding this comment.
[/diagnose] The default case returns the identical hint as the "file not found" branch — "Ensure the file exists locally". When err.Cause doesn't match any known substring (e.g., a permission error, an unexpected network message), the user is actively misled into looking for a missing file.
💡 Suggestion
Return an empty string for unknown causes, or a genuinely generic fallback:
default:
return "" // no hint for unrecognised errors — don't mislead the userOr:
default:
return fmt.Sprintf("Check the `imports:` configuration for `%s`.", importPath)| "test.md:3:3:", | ||
| "error:", | ||
| "invalid import specification", | ||
| "hint:", |
There was a problem hiding this comment.
[/tdd] No test case covers the "failed to resolve ref" error path (causeMsg containing "failed to resolve ref" → message = "failed to resolve import reference"). This gap is also what let the dead-code bug on import_error.go line 154 slip through — a test asserting "ref" in the hint would have caught it immediately.
💡 Suggested test case to add
{
name: "ref resolution error",
err: &parser.ImportError{
ImportPath: "owner/repo/file.md@v0.0.0-bad",
FilePath: "test.md",
Line: 3,
Column: 3,
Cause: errors.New("failed to resolve ref v0.0.0-bad: not found"),
},
yamlContent: `---
on: push
imports:
- owner/repo/file.md@v0.0.0-bad
---`,
wantContain: []string{
"error:",
"failed to resolve import reference",
"hint:",
"ref",
"owner/repo/file.md@v0.0.0-bad",
},
},
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics & Test Classification (6 tests analyzed)
Go: 6 ( Inflation note: Verdict
References:
|
There was a problem hiding this comment.
REQUEST_CHANGES — ref-error hint is completely unreachable (compound correctness bug)
The YAML-context offset fix and the early engine validation are correct. The import hint work has a compound bug that makes the ref-specific hint dead code.
Blocking findings
buildImportErrorHint: two independent bugs conspire to make the ref hint unreachable
-
(existing comment, import_error.go:161) — Case 1's second predicate
strings.Contains(message, "failed to resolve import")matches"failed to resolve import reference"(the message produced for ref failures), so case 1 fires and returns the wrong file-not-found hint. -
(new, import_error.go:154) — Even if the ordering were fixed by moving case 3 before case 1, case 3 would still never fire. The pattern
"failed to resolve ref"is not a substring of"failed to resolve import reference"— the string"import "sits between"resolve "and"ref", breaking the match.strings.Contains("failed to resolve import reference", "failed to resolve ref") == false.
Both bugs must be fixed together. Minimal fix: change case 3's predicate from strings.Contains(message, "failed to resolve ref") to strings.Contains(message, "import reference"), and move it before case 1.
Test quality (new, compiler_yaml_test.go:600) — TestInvalidEngineReportedBeforeImportErrors omits the source-location assertion (filename:line:col:) that TestEngineValidationErrorHasFileLocation provides for the normal engine-error path. If formatEngineSetupError is later bypassed, users lose IDE navigation but no test catches it.
(existing comment, compiler_yaml_test.go:607) — strings.Contains(errorStr, "copilot") is trivially satisfied by the always-present Example:\nengine: copilot block in the error message; the "Did you mean: copilot?" suggestion line is not actually exercised.
🔎 Code quality review by PR Code Quality Reviewer · 120.3 AIC · ⌖ 7.37 AIC · ⊞ 5.2K
| return fmt.Sprintf("Ensure `%s` exists relative to the workflow directory, or check the `imports:` path.", importPath) | ||
| case strings.Contains(message, "failed to download"): | ||
| return fmt.Sprintf("Check that the remote import `%s` is accessible and the path is correct.", importPath) | ||
| case strings.Contains(message, "failed to resolve ref"): |
There was a problem hiding this comment.
Case 3 is dead code — its pattern never matches the message produced by FormatImportError: "failed to resolve ref" is not a substring of "failed to resolve import reference", so this branch is unreachable even after fixing the case-ordering issue flagged in the existing review comment.
💡 Explanation and fix
FormatImportError (line 119) normalises ref failures to:
message = "failed to resolve import reference"
But case 3 in buildImportErrorHint checks:
case strings.Contains(message, "failed to resolve ref"):"failed to resolve ref" is not a substring of "failed to resolve import reference" (the literal string "import " appears between "resolve " and "ref", breaking the match). Confirming: strings.Contains("failed to resolve import reference", "failed to resolve ref") == false.
As a result:
- Case 1 fires first (because
"failed to resolve import reference"does contain"failed to resolve import"), producing the wrong file-not-found hint. - Even if the ordering bug were fixed, case 3 still would not fire — the ref-specific hint is completely unreachable.
Minimal fix — align case 3 to the actual message string:
case strings.Contains(message, "import reference"):
return fmt.Sprintf("Verify the ref (branch, tag, or SHA) in `%s` is valid and accessible.", importPath)| errorStr := err.Error() | ||
|
|
||
| // Should report the invalid engine, not the missing import | ||
| if !strings.Contains(errorStr, "invalid engine") { |
There was a problem hiding this comment.
TestInvalidEngineReportedBeforeImportErrors doesn't assert source location — the test verifies the error text but never checks that the error includes a filename:line:col: prefix pointing to the engine: field, which is the DX improvement this test is meant to protect.
💡 Why this matters
The CompileWorkflow path annotates the error via formatEngineSetupError, which looks up the engine: field line and wraps the error with source context. If that wrapper is ever removed or bypassed, users silently lose location info, but this test would still pass.
There is already a TestEngineValidationErrorHasFileLocation test that checks filename:line:col: format for normal engine errors. The same assertion is missing here for the early-validation-with-failing-imports scenario.
Add a check such as:
if !strings.Contains(errorStr, testFile) {
t.Errorf("error should include the source file path for IDE navigation, got: %s", errorStr)
}
// e.g. verify format "test.md:2:1: error: invalid engine"
if !regexp.MustCompile(`test\.md:\d+:\d+: error:`).MatchString(errorStr) {
t.Errorf("error should have filename:line:col: format, got: %s", errorStr)
}
|
@copilot run pr-finisher skill |
|
``
|
|
``
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
`` Please rebase onto main, resolve the remaining review threads, and re-run checks.
|
Three DX bugs degraded compiler error quality (avg score 44/100 in the daily syntax-error quality check): a code-context display bug that omits the faulty line, generic actionability hints on import errors, and an invalid engine name silently swallowed when imports also fail.
Changes
Fix YAML context offset (
pkg/workflow/frontmatter_error.go):sourceContextPatternused\s+which doesn't match the> N |highlighted-error-line prefix. When the error falls on the first frontmatter line (line 2), the entire error line was absent from the snippet. Updated to>?\s*\d+\s*\|so context extraction starts from the>marker line.Tailored import hints (
pkg/parser/import_error.go): AddedbuildImportErrorHint()and populatedCompilerError.HintinFormatImportError. Each cause variant (file not found, download failure, bad ref, invalid workflowspec) now gets a specific actionable hint, e.g.:Early engine validation (
pkg/workflow/compiler_orchestrator_engine.go): ValidatesengineSettingagainstengineCatalog.Resolve()immediately afterinjectBuiltinEngineImportIfNeeded, before import processing. A typo likecopiilotnow surfaces "Did you mean: copilot?" even when imports also fail — previously the import error shadowed the engine error entirely.Tests
TestYAMLFormatErrorOutput/error_on_line_2_included_in_snippet— asserts the faulty line appears in the snippet.TestInvalidEngineReportedBeforeImportErrors— asserts engine validation fires before import resolution.TestImportNotFoundHint— assertshint:appears in import-not-found compiler output.TestFormatImportErrorto asserthint:is present for all import error variants.pr-sous-chef: rebase and re-run checks
Run: https://github.com/github/gh-aw/actions/runs/28124550476