Fix engine validation error location and improve error detection (#issue)#21023
Merged
Fix engine validation error location and improve error detection (#issue)#21023
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
pelikhan
March 15, 2026 04:03
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Improves workflow compilation diagnostics so invalid engine: values report a precise file:line:col: location, and replaces fragile string-based “already formatted” detection with type-based detection.
Changes:
- Wraps engine-related errors from
ParseWorkflowFilewith a source location pointing to theengine:frontmatter line (when available). - Makes compiler error formatting consistently return
*wrappedCompilerErrorand addsisFormattedCompilerError(err)usingerrors.As. - Adds unit/integration tests for
findFrontmatterFieldLine,isFormattedCompilerError, and engine validation error location formatting.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_orchestrator_workflow.go | Wraps unformatted engine/setup and tools/markdown errors; attempts to pinpoint engine: line for navigation. |
| pkg/workflow/compiler.go | Switches from string-contains formatting detection to isFormattedCompilerError. |
| pkg/workflow/compiler_error_formatter.go | Always returns *wrappedCompilerError; adds isFormattedCompilerError helper using errors.As. |
| pkg/workflow/frontmatter_error.go | Adds findFrontmatterFieldLine helper to locate a top-level frontmatter key line. |
| pkg/workflow/compiler_yaml_test.go | Adds test ensuring invalid engine errors include correct file:line:col: prefix. |
| pkg/workflow/compiler_error_formatter_test.go | Adds tests for isFormattedCompilerError. |
| pkg/workflow/frontmatter_error_test.go | Adds tests for findFrontmatterFieldLine edge cases. |
Comments suppressed due to low confidence (1)
pkg/workflow/frontmatter_error.go:69
- The docstring says “first non-space key matches fieldName”, but the implementation intentionally only matches non-indented lines (
strings.HasPrefix(line, fieldName+":")) and will not match keys with leading whitespace. Please adjust the wording to reflect the actual behavior (e.g., “matches lines that start withfieldName:with no leading whitespace”).
// findFrontmatterFieldLine searches frontmatterLines for a line whose first
// non-space key matches fieldName (e.g., "engine") and returns the 1-based
// document line number. frontmatterStart is the 1-based line number of the
// first frontmatter line (i.e., the line immediately after the opening "---").
// Returns 0 if the field is not found.
//
// Only top-level (non-indented) keys are matched. Nested values that happen
// to contain the field name are ignored.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+71
to
+75
| prefix := fieldName + ":" | ||
| for i, line := range frontmatterLines { | ||
| // Match only non-indented lines so nested YAML values are not confused | ||
| // with top-level keys (e.g. " engine: ..." inside a mapping is ignored). | ||
| if strings.HasPrefix(line, prefix) { |
Comment on lines
+42
to
+45
| name: "fmt.Errorf error is not formatted", | ||
| err: errors.New("wrapped: plain error"), | ||
| expected: false, | ||
| }, |
Comment on lines
+41
to
+53
| // Wrap unformatted errors with file location. Errors produced by | ||
| // formatCompilerError/formatCompilerErrorWithPosition are already | ||
| // console-formatted and must not be double-wrapped. | ||
| if isFormattedCompilerError(err) { | ||
| return nil, err | ||
| } | ||
| // Try to point at the exact line of the "engine:" field so the user can | ||
| // navigate directly to the problem location. | ||
| engineLine := findFrontmatterFieldLine(result.FrontmatterLines, result.FrontmatterStart, "engine") | ||
| if engineLine > 0 { | ||
| return nil, formatCompilerErrorWithPosition(cleanPath, engineLine, 1, "error", err.Error(), err) | ||
| } | ||
| return nil, formatCompilerError(cleanPath, "error", err.Error(), err) |
Comment on lines
+78
to
84
| // ParseWorkflowFile already returns formatted compiler errors; pass them through. | ||
| if isFormattedCompilerError(err) { | ||
| return err | ||
| } | ||
| // Otherwise, create a basic formatted error with wrapping | ||
| // Fallback for any unformatted error that slipped through. | ||
| return formatCompilerError(markdownPath, "error", err.Error(), err) | ||
| } |
Comment on lines
+48
to
54
| // isFormattedCompilerError reports whether err is already a console-formatted compiler error | ||
| // produced by formatCompilerError or formatCompilerErrorWithPosition. Use this instead of | ||
| // fragile string-contains checks to avoid double-wrapping. | ||
| func isFormattedCompilerError(err error) bool { | ||
| var wce *wrappedCompilerError | ||
| return errors.As(err, &wce) | ||
| } |
Contributor
|
@copilot review comments |
Copilot stopped work on behalf of
pelikhan due to an error
March 15, 2026 04:31
This was referenced Mar 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the medium-priority item from the syntax error quality issue: engine validation errors now include a
file:line:col:prefix pointing to theengine:field in the source file, instead of showing no file location at all.Problem
When compiling a workflow with an invalid engine (e.g.,
engine: openai_gpt), the error had no file location:The root cause:
compile_workflow_processor.gocallsParseWorkflowFiledirectly (bypassingCompileWorkflow), so unformatted errors from engine catalog resolution were never wrapped with file location.Changes
pkg/workflow/compiler_error_formatter.goformatCompilerErrorandformatCompilerErrorWithPositionnow always return*wrappedCompilerError(even whencause == nil), enabling reliable type-based detectionisFormattedCompilerError(err)helper usingerrors.As— replaces the previous fragilestrings.Contains(err.Error(), "error:")checkpkg/workflow/compiler.goisFormattedCompilerError(err)pkg/workflow/compiler_orchestrator_workflow.gosetupEngineAndImportsandprocessToolsAndMarkdownnow get wrapped with proper file locationfindFrontmatterFieldLineto pinpoint the exactengine:line for accuratefile:line:col:navigationpkg/workflow/frontmatter_error.gofindFrontmatterFieldLinehelper: scans frontmatter lines for a top-level key and returns its 1-based document line number (0 if not found)Tests
compiler_yaml_test.go: addedTestEngineValidationErrorHasFileLocation— verifies that invalid engine errors include the correctfile:line:col:prefixcompiler_error_formatter_test.go: unit tests forisFormattedCompilerErrorfrontmatter_error_test.go: unit tests forfindFrontmatterFieldLine(including edge cases: absent field, indented keys, prefix collisions)After Fix
Security Summary
No security vulnerabilities were introduced or discovered. The changes are purely error-message formatting improvements with no user-controlled input parsing or external interactions.