Prioritize invalid engine typos over downstream schema/import failures#42235
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR improves workflow compilation diagnostics by ensuring invalid engine settings are validated early (before schema/import failures) and by tightening error-location/snippet consistency, with new regression tests to lock in the intended output.
Changes:
- Add an early pre-schema validation step to surface invalid
engine:strings before schema/import errors. - Ensure compiler errors consistently include a single VSCode-navigable location plus source snippets where available.
- Add focused tests covering engine type errors, engine typo precedence, and snippet/location formatting.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_yaml_test.go | Adds regression tests asserting engine-related error precedence, locations, and snippet rendering. |
| pkg/workflow/compiler_string_api.go | Adds pre-schema engine validation for string-based compilation (Wasm/browser entrypoint). |
| pkg/workflow/compiler_orchestrator_frontmatter.go | Introduces shared helper to validate engine early in the file-based frontmatter compilation path. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Low
| if err := c.validateStringEngineBeforeSchema(cleanPath, []byte(content), result, frontmatterForValidation); err != nil { | ||
| compilerStringAPILog.Printf("ParseWorkflowString: string engine pre-validation failed for %s", cleanPath) | ||
| return nil, err | ||
| } |
| func (c *Compiler) validateStringEngineBeforeSchema( | ||
| cleanPath string, | ||
| content []byte, | ||
| result *parser.FrontmatterResult, | ||
| frontmatterForValidation map[string]any, | ||
| ) error { |
Draft ADR documenting the decision to pre-validate the engine field before JSON schema validation to surface actionable engine typo errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 1 ( Build tag: Verdict
References: §28370500282
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (116 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 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.
Review: Prioritize engine typos over downstream schema/import failures
The approach is sound: pre-validating string engine values before schema validation cleanly ensures typos (e.g., copiilot) surface as the primary error, with proper source location and snippet, rather than being shadowed by unrelated bogus-field schema errors. The two-path coverage (file + ParseWorkflowString) and the three-stage line-number fallback (FieldLines → findFrontmatterFieldLine → line 1) are well-structured.
Three non-blocking observations flagged inline:
-
Whitespace-only engine values bypass pre-validation —
TrimSpace == ""early-returns beforegetAgenticEnginesees the input; reassigning the trimmed value first would close the gap (line 16). -
Shared-workflow path has no coverage comment — the gap is intentional (shared workflows don't carry
engine), but a short comment would help future readers (line 172). -
Test column
:4:8:is coupled to schema-validator internals — documenting the origin of column 8, or relaxing to a line-only assertion, would protect the test against library changes (test line 105).
No blocking issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 71.8 AIC · ⌖ 9.45 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/workflow/compiler_orchestrator_frontmatter.go:16
Whitespace-only engine strings silently bypass new pre-validation.
The guard strings.TrimSpace(engineValue) == "" exits early for inputs like engine: " " (whitespace-only), deferring them to schema or later engine-lookup validation — which may not produce the same targeted error quality. Meanwhile, getAgenticEngine treats "" as "use default" but would error on " " when eventually called, potentially surfacing a confusing late error.
Consider reassigning the trimmed value befor…
pkg/workflow/compiler_orchestrator_frontmatter.go:172
Shared-workflow path skips the new pre-validation.
validateStringEngineBeforeSchema is called only in the main-workflow path (after the isSharedWorkflow return on ~line 163). If a shared/included workflow file contains a top-level string engine with a typo, the new early reporting won't fire — the error will surface only if and when the downstream schema or engine-lookup runs.
Shared workflows don't normally carry an engine field, so this is low-risk, but worth an explicit comment…
pkg/workflow/compiler_yaml_test.go:105
Hardcoded column offset :4:8: couples the test to schema-validation internals.
The assertion testFile+":4:8: error:" will pass only as long as the schema validator reports column 8 for engine: 123. If the schema validation library ever changes its column calculation (e.g., from 0-based to 1-based, or from pointing at the colon to pointing at the value), this test will produce a cryptic failure that doesn't hint at the real cause.
Consider using the existing regexp-based check patter…
There was a problem hiding this comment.
REQUEST_CHANGES — The engine-precedence logic is correct and the file-based test coverage is solid, but the ParseWorkflowString change (the primary Wasm/browser entry point) ships without any test, and two medium issues need addressing.
Blocking issues
Missing test for ParseWorkflowString code path
The new validateStringEngineBeforeSchema call in compiler_string_api.go is the only changed behaviour with zero test coverage. All three new tests exercise CompileWorkflow. Add at least one ParseWorkflowString-based test mirroring TestInvalidEngineReportedBeforeSchemaErrors to confirm error precedence and location format on the string/Wasm path.
Non-blocking but worth fixing
- Double
[]byte(content)allocation inParseWorkflowString(lines 111 and 127) — introduce a singlecontentByteslocal. - Column 1 vs column 8 inconsistency between string-typo errors and non-string-type errors — either document the choice or align the conventions.
🔎 Code quality review by PR Code Quality Reviewer · 89.8 AIC · ⌖ 7.37 AIC · ⊞ 5.2K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_string_api.go:111
ParseWorkflowString engine pre-validation is completely untested, leaving the primary Wasm/browser entry point unverified for error precedence, location format, and message content.
<details>
<summary>💡 Details</summary>
All three new tests use CompileWorkflow (file path). The compiler_string_api.go change adds the same call at line 111, but there is no test that:
- Calls
ParseWorkflowStringwith a string engine typo and asserts the error message contains "invalid engine:" - Veri…
pkg/workflow/compiler_orchestrator_frontmatter.go:55
Hardcoded column=1 creates inconsistent editor navigation for engine errors — string typos land at column 1 (key), while non-string type errors land at column 8 (value).
<details>
<summary>💡 Details</summary>
The two new tests codify this divergence:
TestInvalidEngineReportedBeforeSchemaErrorsasserts:3:1:(string typo, key position)TestEngineTypeValidationErrorUsesSingleSourceLocationAndSnippetasserts:4:8:(non-string type, value position via schema validator)
An edit…
pkg/workflow/compiler_string_api.go:127
[]byte(content) is allocated twice on the hot Wasm/browser path — once at line 111 (for validateStringEngineBeforeSchema) and again here for parseResult.content.
<details>
<summary>💡 Details</summary>
Since ParseWorkflowString is documented as the primary Wasm/browser entry point where filesystem access is unavailable, every successful parse now allocates and copies the entire workflow source string twice. For large workflow files this doubles GC pressure unnecessarily. The fix i…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — leaving comments on test coverage gaps and a diagnostic consistency note. No blocking issues; the core fix is correct.
📋 Key Themes & Highlights
Key Themes
ParseWorkflowStringpath untested — the pre-validation is wired into both compiler entry points but tests only exerciseCompileWorkflow. A small additional test would close the gap.t.Fatalfvst.Errorfstyle drift — new test usest.Fatalffor all assertions while existing parallel test (TestInvalidEngineReportedBeforeImportErrors) usest.Errorf, hiding subsequent failures on first error.- Column
1vs value-column inconsistency — engine type errors (schema path) point to the value column; the new string-engine errors always point to column1(key position). Intentional or worth aligning? TestEngineTypeValidationErrorUsesSingleSourceLocationAndSnippetscope — this test exercises the schema validation path, not the newvalidateStringEngineBeforeSchema. A brief comment would prevent confusion.
Positive Highlights
- ✅ Clean, single-purpose
validateStringEngineBeforeSchemafunction with a well-defined early-exit contract - ✅ Dual-site pre-validation (both
parseFrontmatterSectionandParseWorkflowString) avoids a second place to forget this in future - ✅ Three-stage line-resolution fallback (
FieldLines→findFrontmatterFieldLine→ 1) is defensive and resilient - ✅
TestInvalidEngineReportedBeforeImportErrorsalready provides a strong file-path precedence regression test
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 64.8 AIC · ⌖ 7.98 AIC · ⊞ 6.6K
Comment /matt to run again
| return nil, &SharedWorkflowError{Path: cleanPath} | ||
| } | ||
|
|
||
| if err := c.validateStringEngineBeforeSchema(cleanPath, []byte(content), result, frontmatterForValidation); err != nil { |
There was a problem hiding this comment.
[/tdd] ParseWorkflowString engine pre-validation is not covered by any new test — only CompileWorkflow (file-based path) is exercised by the new tests.
💡 Suggested test skeleton
A ParseWorkflowString-based counterpart for TestInvalidEngineReportedBeforeSchemaErrors would close this gap:
func TestParseWorkflowStringInvalidEngineReportedBeforeSchemaErrors(t *testing.T) {
content := `---
on: push
engine: copiilot
bogus-field: true
---
# Test
`
compiler := NewCompiler()
_, err := compiler.ParseWorkflowString(content, "virtual/test.md")
if err == nil {
t.Fatal("expected error")
}
errorStr := err.Error()
if !strings.Contains(errorStr, "invalid engine: copiilot") {
t.Errorf("engine error should take precedence, got: %s", errorStr)
}
if strings.Contains(errorStr, "Unknown property: bogus-field") {
t.Errorf("schema error must not shadow engine error, got: %s", errorStr)
}
}Without this, a regression in the ParseWorkflowString path (e.g., accidentally inserting the pre-validation after schema validation) would go undetected.
@copilot please address this.
| return formatCompilerErrorWithContext( | ||
| cleanPath, | ||
| line, | ||
| 1, |
There was a problem hiding this comment.
[/diagnose] Column is hardcoded to 1, pointing at the engine key rather than the invalid value — this will differ from how engine: 123 (type error) is reported at column 8 (value position), creating an inconsistent UX.
💡 Context
TestEngineTypeValidationErrorUsesSingleSourceLocationAndSnippet asserts testFile+":4:8: error:" for engine: 123 — the schema validator pins the error at column 8 (the value). The new code here always uses column 1, so a typo on engine: copiilot would be flagged at the key rather than the value.
If pointing to the key is intentional (e.g., consistent with how other field errors are formatted), this is fine — but it is worth an explicit comment explaining the choice, and ideally TestInvalidEngineReportedBeforeSchemaErrors should assert :3:1: rather than just :3:1: as a side-effect of another assertion so the intent is clear.
If pointing at the value is preferred, compute the column as len("engine: ") + 1 = 9 or derive it from result.FieldLines if the parser stores value offsets.
@copilot please address this.
|
|
||
| errorStr := err.Error() | ||
| if !strings.Contains(errorStr, "invalid engine: copiilot") { | ||
| t.Fatalf("error should prioritize the invalid engine typo, got: %s", errorStr) |
There was a problem hiding this comment.
[/tdd] Using t.Fatalf for all assertions in TestInvalidEngineReportedBeforeSchemaErrors stops the test at the first failure, hiding subsequent violations in a single run — the existing TestInvalidEngineReportedBeforeImportErrors uses t.Errorf for independent assertions.
💡 Preferred pattern (non-fatal assertions)
Replace t.Fatalf with t.Errorf for independent assertion checks so all failures surface in one run:
if !strings.Contains(errorStr, "invalid engine: copiilot") {
t.Errorf("error should prioritize the invalid engine typo, got: %s", errorStr)
}
if !strings.Contains(errorStr, "Did you mean: copilot?") {
t.Errorf("error should include the closest engine suggestion, got: %s", errorStr)
}
if strings.Contains(errorStr, "Unknown property: bogus-field") {
t.Errorf("schema error should not shadow the invalid engine typo, got: %s", errorStr)
}
if !strings.Contains(errorStr, testFile+":3:1: error:") {
t.Errorf("error should point to the engine field location, got: %s", errorStr)
}Reserve t.Fatalf for setup failures where subsequent assertions would panic (e.g., nil pointer).
@copilot please address this.
| } | ||
| } | ||
|
|
||
| func TestEngineTypeValidationErrorUsesSingleSourceLocationAndSnippet(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestEngineTypeValidationErrorUsesSingleSourceLocationAndSnippet uses engine: 123 (integer), which fails the .(string) type assertion in validateStringEngineBeforeSchema and falls through to schema validation — the new code is never exercised by this test.
💡 Clarification
The test documents existing schema-validation behaviour (integer engine value → schema error at value column 8), not the new validateStringEngineBeforeSchema logic. That is a valid regression guard, but the test name and its placement next to the new schema-precedence tests imply it verifies the new code. A brief comment in the test body — or a small name adjustment — would make the scope clear:
// TestEngineTypeValidationErrorUsesSingleSourceLocationAndSnippet confirms that
// when engine has the wrong *type* (e.g. integer), the schema validator
// (not validateStringEngineBeforeSchema, which only handles string values)
// still produces a single location + snippet.@copilot please address this.
🤖 PR Triage — §28376613466
Score breakdown: Impact 25 + Urgency 15 + Quality 8 Rationale: Fixes compile-error ordering so engine typos (e.g. Batch: Groups with #41824 and #42226 (all need reviewer/author action on
|
|
@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>
Ran the PR-finisher pass. Addressed the remaining review feedback in |
|
@copilot please run the Unresolved reviews (newest first):
The branch has also been queued for a base-branch update — once updated, rerun any failing checks and verify the PR is ready to merge.
|
Ran the PR-finisher pass. The unresolved github-actions feedback is already addressed on this branch in |
|
@copilot merge main and recompile |
|
|
@copilot please address the following unresolved review feedback, then run the Unresolved CHANGES_REQUESTED reviews:
The branch has no conflicts. Ensure
|
Addressed on the current HEAD ( |
Unknown top-level string engine values could be masked by later schema or import errors, so typos like
engine: copiilotsometimes compiled without actionable feedback. Engine type errors also needed regression coverage to preserve consistent location/snippet rendering.Engine validation order
enginevalues before main schema validation/import handling.enginefield location, including suggestion text and valid engine list.Error selection
Regression coverage
Example:
Now this reports the invalid
enginefirst, at theengineline, withDid you mean: copilot?instead of leading with the unrelated schema error.