Fix engine validation error location and improve error detection (#issue)

pkg/workflow/compiler.go

@@ -75,12 +75,11 @@ func (c *Compiler) CompileWorkflow(markdownPath string) error {
 	log.Printf("Parsing workflow file")
 	workflowData, err := c.ParseWorkflowFile(markdownPath)
 	if err != nil {
-		// Check if this is already a formatted console error
-		if strings.Contains(err.Error(), ":") && (strings.Contains(err.Error(), "error:") || strings.Contains(err.Error(), "warning:")) {
-			// Already formatted, return as-is
+		// 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)
 	}
 

pkg/workflow/compiler_error_formatter.go

@@ -40,14 +40,17 @@ func formatCompilerError(filePath string, errType string, message string, cause
 		Message: message,
 	})
 
-	// Preserve the error chain via Unwrap() so errors.Is/As continue to work on the
-	// returned error, while Error() returns only the clean formatted string (no duplication).
-	if cause != nil {
-		return &wrappedCompilerError{formatted: formattedErr, cause: cause}
-	}
+	// Always return a *wrappedCompilerError so isFormattedCompilerError can detect it.
+	// cause may be nil for validation errors that have no underlying cause.
+	return &wrappedCompilerError{formatted: formattedErr, cause: cause}
+}
 
-	// Create new error for validation errors (no underlying cause)
-	return errors.New(formattedErr)
+// 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)
 }
 
 // formatCompilerErrorWithPosition creates a formatted compiler error with specific line/column position.
@@ -70,12 +73,7 @@ func formatCompilerErrorWithPosition(filePath string, line int, column int, errT
 		Message: message,
 	})
 
-	// Preserve the error chain via Unwrap() so errors.Is/As continue to work on the
-	// returned error, while Error() returns only the clean formatted string (no duplication).
-	if cause != nil {
-		return &wrappedCompilerError{formatted: formattedErr, cause: cause}
-	}
-
-	// Create new error for validation errors (no underlying cause)
-	return errors.New(formattedErr)
+	// Always return a *wrappedCompilerError so isFormattedCompilerError can detect it.
+	// cause may be nil for validation errors that have no underlying cause.
+	return &wrappedCompilerError{formatted: formattedErr, cause: cause}
 }

pkg/workflow/compiler_error_formatter_test.go

@@ -0,0 +1,60 @@
+//go:build !integration
+
+package workflow
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestIsFormattedCompilerError verifies that the helper correctly identifies
+// errors produced by formatCompilerError / formatCompilerErrorWithPosition and
+// returns false for other error types.
+func TestIsFormattedCompilerError(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      error
+		expected bool
+	}{
+		{
+			name:     "error from formatCompilerError with nil cause",
+			err:      formatCompilerError("file.md", "error", "something went wrong", nil),
+			expected: true,
+		},
+		{
+			name:     "error from formatCompilerError with non-nil cause",
+			err:      formatCompilerError("file.md", "error", "something went wrong", errors.New("root cause")),
+			expected: true,
+		},
+		{
+			name:     "error from formatCompilerErrorWithPosition",
+			err:      formatCompilerErrorWithPosition("file.md", 5, 3, "error", "bad value", nil),
+			expected: true,
+		},
+		{
+			name:     "plain error is not formatted",
+			err:      errors.New("plain error"),
+			expected: false,
+		},
+		{
+			name:     "fmt.Errorf error is not formatted",
+			err:      errors.New("wrapped: plain error"),
+			expected: false,
+		},
+		{
+			name:     "nil error",
+			err:      nil,
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isFormattedCompilerError(tt.err)
+			assert.Equal(t, tt.expected, got,
+				"isFormattedCompilerError(%v) should be %v", tt.err, tt.expected)
+		})
+	}
+}

pkg/workflow/compiler_orchestrator_workflow.go

@@ -38,13 +38,28 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error)
 	// Setup engine and process imports
 	engineSetup, err := c.setupEngineAndImports(result, cleanPath, content, markdownDir)
 	if err != nil {
-		return nil, err
+		// 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)
 	}
 
 	// Process tools and markdown
 	toolsResult, err := c.processToolsAndMarkdown(result, cleanPath, markdownDir, engineSetup.agenticEngine, engineSetup.engineSetting, engineSetup.importsResult)
 	if err != nil {
-		return nil, err
+		if isFormattedCompilerError(err) {
+			return nil, err
+		}
+		return nil, formatCompilerError(cleanPath, "error", err.Error(), err)
 	}
 
 	// Build initial workflow data structure

pkg/workflow/compiler_yaml_test.go

@@ -471,6 +471,84 @@ Test content.`,
 	}
 }
 
+// TestEngineValidationErrorHasFileLocation verifies that invalid engine errors include
+// the file:line:column: prefix pointing to the "engine:" field in the source file.
+func TestEngineValidationErrorHasFileLocation(t *testing.T) {
+	tmpDir := testutil.TempDir(t, "engine-location-test")
+
+	tests := []struct {
+		name              string
+		content           string
+		expectedErrorLine int
+		description       string
+	}{
+		{
+			name: "invalid_engine_line3",
+			content: `---
+on: push
+engine: openai_gpt
+---
+
+# Test Workflow
+
+Content.`,
+			expectedErrorLine: 3, // "engine: openai_gpt" is on line 3 in the file
+			description:       "invalid engine on line 3 should produce error at line 3",
+		},
+		{
+			name: "invalid_engine_with_other_fields",
+			content: `---
+on: push
+permissions:
+  contents: read
+engine: badengine_xyz
+strict: false
+---
+
+# Test Workflow
+
+Content.`,
+			expectedErrorLine: 5, // "engine: badengine_xyz" is on line 5 in the file
+			description:       "invalid engine after permissions block should produce error at correct line",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			testFile := filepath.Join(tmpDir, tt.name+".md")
+			if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil {
+				t.Fatal(err)
+			}
+
+			compiler := NewCompiler()
+			err := compiler.CompileWorkflow(testFile)
+			if err == nil {
+				t.Errorf("%s: expected compilation to fail for invalid engine", tt.description)
+				return
+			}
+
+			errorStr := err.Error()
+
+			// Verify error contains filename:line:col: format pointing to the engine field
+			expectedPattern := fmt.Sprintf(".md:%d:1:", tt.expectedErrorLine)
+			if !strings.Contains(errorStr, expectedPattern) {
+				t.Errorf("%s: error should contain '%s' pointing to the engine: field, got: %s",
+					tt.description, expectedPattern, errorStr)
+			}
+
+			// Verify the error contains the invalid engine name
+			if !strings.Contains(errorStr, "invalid engine:") {
+				t.Errorf("%s: error should contain 'invalid engine:', got: %s", tt.description, errorStr)
+			}
+
+			// Verify the error type indicator is present
+			if !strings.Contains(errorStr, "error:") {
+				t.Errorf("%s: error should contain 'error:' type indicator, got: %s", tt.description, errorStr)
+			}
+		})
+	}
+}
+
 // TestCommentOutProcessedFieldsInOnSection tests the commentOutProcessedFieldsInOnSection function directly
 
 // TestAddCustomStepsAsIsBasic tests adding custom steps as-is

pkg/workflow/frontmatter_error.go

@@ -59,6 +59,26 @@ func translateYAMLMessage(message string) string {
 	return message
 }
 
+// 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.
+func findFrontmatterFieldLine(frontmatterLines []string, frontmatterStart int, fieldName string) int {
+	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) {
+			return frontmatterStart + i
+		}
+	}
+	return 0
+}
+
 // createFrontmatterError creates a detailed error for frontmatter parsing issues
 // frontmatterLineOffset is the line number where the frontmatter content begins (1-based)
 // Returns error in VSCode-compatible format: filename:line:column: error message

pkg/workflow/frontmatter_error_test.go

@@ -0,0 +1,86 @@
+//go:build !integration
+
+package workflow
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestFindFrontmatterFieldLine verifies that the helper correctly locates a
+// named key within frontmatter lines and handles edge cases.
+func TestFindFrontmatterFieldLine(t *testing.T) {
+	tests := []struct {
+		name             string
+		frontmatterLines []string
+		frontmatterStart int
+		fieldName        string
+		expectedDocLine  int
+		description      string
+	}{
+		{
+			name:             "field found at first line",
+			frontmatterLines: []string{"engine: copilot", "on: push"},
+			frontmatterStart: 2,
+			fieldName:        "engine",
+			expectedDocLine:  2, // frontmatterStart + 0
+			description:      "engine: on the first frontmatter line maps to document line 2",
+		},
+		{
+			name:             "field found after other keys",
+			frontmatterLines: []string{"on: push", "permissions:", "  contents: read", "engine: claude"},
+			frontmatterStart: 2,
+			fieldName:        "engine",
+			expectedDocLine:  5, // frontmatterStart + 3
+			description:      "engine: after other keys maps to correct document line",
+		},
+		{
+			name:             "field not present returns zero",
+			frontmatterLines: []string{"on: push", "permissions:", "  contents: read"},
+			frontmatterStart: 2,
+			fieldName:        "engine",
+			expectedDocLine:  0,
+			description:      "absent field should return 0",
+		},
+		{
+			name:             "field with leading whitespace is not matched",
+			frontmatterLines: []string{"on: push", "  engine: copilot"}, // indented — not a top-level key
+			frontmatterStart: 2,
+			fieldName:        "engine",
+			expectedDocLine:  0,
+			description:      "indented engine: should not be matched as top-level field",
+		},
+		{
+			name:             "frontmatter starts later in the document",
+			frontmatterLines: []string{"on: push", "engine: gemini"},
+			frontmatterStart: 10,
+			fieldName:        "engine",
+			expectedDocLine:  11, // frontmatterStart + 1
+			description:      "correct line number when frontmatter does not start at line 2",
+		},
+		{
+			name:             "empty frontmatter lines returns zero",
+			frontmatterLines: []string{},
+			frontmatterStart: 2,
+			fieldName:        "engine",
+			expectedDocLine:  0,
+			description:      "empty frontmatter should return 0",
+		},
+		{
+			name:             "field name that is a prefix of another key is not confused",
+			frontmatterLines: []string{"engine_custom: value", "engine: copilot"},
+			frontmatterStart: 2,
+			fieldName:        "engine",
+			expectedDocLine:  3, // line 3 (frontmatterStart + 1), NOT line 2 which has engine_custom
+			description:      "engine: should not match engine_custom: (prefix guard via colon suffix)",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := findFrontmatterFieldLine(tt.frontmatterLines, tt.frontmatterStart, tt.fieldName)
+			assert.Equal(t, tt.expectedDocLine, got, tt.description)
+		})
+	}
+}