diff --git a/pkg/parser/yaml_error.go b/pkg/parser/yaml_error.go index 682c9d344d2..d26ce59d3a4 100644 --- a/pkg/parser/yaml_error.go +++ b/pkg/parser/yaml_error.go @@ -28,42 +28,52 @@ var ( var yamlErrorTranslations = []struct { pattern string replacement string + example string }{ { - "unexpected key name", - "missing ':' after key — YAML mapping entries require 'key: value' format", + pattern: "unexpected key name", + replacement: "missing ':' after key — YAML mapping entries require 'key: value' format", + example: "\n\nExample:\nengine: copilot", }, { - "mapping value is not allowed in this context", - "unexpected ':' — check indentation or if this key belongs in a mapping block", + pattern: "mapping value is not allowed in this context", + replacement: "unexpected ':' — check indentation or if this key belongs in a mapping block", + example: "\n\nExample:\npermissions:\n contents: read", }, { - "mapping values are not allowed", - "unexpected ':' — check indentation or if this key belongs in a mapping block", + pattern: "mapping values are not allowed", + replacement: "unexpected ':' — check indentation or if this key belongs in a mapping block", + example: "\n\nExample:\npermissions:\n contents: read", }, { - "string was used where mapping is expected", - "expected a YAML mapping (key: value pairs) but got a plain string", + pattern: "string was used where mapping is expected", + replacement: "expected a YAML mapping (key: value pairs) but got a plain string", + example: "\n\nExample:\nengine: copilot", }, { - "non-map value is specified", - "expected a YAML mapping (key: value pairs) — did you forget a colon after the key?", + pattern: "non-map value is specified", + replacement: "expected a YAML mapping (key: value pairs) — did you forget a colon after the key?", + example: "\n\nExample:\nengine: copilot", }, { - "tab character cannot use as a map key directly", - "tab character in key — YAML requires spaces for indentation, not tabs", + pattern: "tab character cannot use as a map key directly", + replacement: "tab character in key — YAML requires spaces for indentation, not tabs", + example: "\n\nExample:\npermissions:\n contents: read", }, { - "found character that cannot start any token", - "invalid character — check indentation uses spaces, not tabs", + pattern: "found character that cannot start any token", + replacement: "invalid character — check indentation uses spaces, not tabs", + example: "\n\nExample:\npermissions:\n contents: read", }, { - "could not find expected ':'", - "missing ':' in key-value pair", + pattern: "could not find expected ':'", + replacement: "missing ':' in key-value pair", + example: "\n\nExample:\nengine: copilot", }, { - "did not find expected key", - "incorrect indentation or missing key in mapping", + pattern: "did not find expected key", + replacement: "incorrect indentation or missing key in mapping", + example: "\n\nExample:\npermissions:\n contents: read", }, } @@ -80,7 +90,7 @@ func TranslateYAMLMessage(message string) string { if idx := strings.Index(lower, t.pattern); idx >= 0 { yamlErrorLog.Printf("Translating YAML message pattern %q", t.pattern) // Slice using idx from the lowercase string. Safe because all patterns are ASCII. - return message[:idx] + t.replacement + message[idx+len(t.pattern):] + return message[:idx] + t.replacement + t.example + message[idx+len(t.pattern):] } } return message diff --git a/pkg/parser/yaml_error_test.go b/pkg/parser/yaml_error_test.go index c6ab19a4e8f..95937f0789c 100644 --- a/pkg/parser/yaml_error_test.go +++ b/pkg/parser/yaml_error_test.go @@ -242,24 +242,28 @@ func TestTranslateYAMLMessage(t *testing.T) { input string contains string excludes string + example string }{ { name: "unexpected key name", input: "unexpected key name", contains: "missing ':' after key", excludes: "unexpected key name", + example: "Example:\nengine: copilot", }, { name: "non-map value is specified", input: "non-map value is specified", contains: "expected a YAML mapping", excludes: "non-map value is specified", + example: "Example:\nengine: copilot", }, { name: "found character that cannot start any token", input: "found character that cannot start any token", contains: "invalid character", excludes: "found character that cannot start any token", + example: "Example:\npermissions:\n contents: read", }, { name: "unrecognized message is unchanged", @@ -284,6 +288,10 @@ func TestTranslateYAMLMessage(t *testing.T) { assert.NotContains(t, result, tt.excludes, "TranslateYAMLMessage should not contain %q\nResult: %s", tt.excludes, result) } + if tt.example != "" { + assert.Contains(t, result, tt.example, + "TranslateYAMLMessage should contain fix example %q\nResult: %s", tt.example, result) + } }) } } @@ -292,16 +300,18 @@ func TestTranslateYAMLMessage(t *testing.T) { // to the underlying goccy/go-yaml error messages. func TestFormatYAMLErrorTranslation(t *testing.T) { tests := []struct { - name string - yamlContent string - shouldContain string - shouldExclude string + name string + yamlContent string + shouldContain string + shouldContainExtra string + shouldExclude string }{ { - name: "missing colon translated", - yamlContent: "engine claude\nmodel: gpt-4", - shouldContain: "missing ':' after key", - shouldExclude: "unexpected key name", + name: "missing colon translated", + yamlContent: "engine claude\nmodel: gpt-4", + shouldContain: "missing ':' after key", + shouldContainExtra: "Example:\nengine: copilot", + shouldExclude: "unexpected key name", }, { name: "extra colon translated", @@ -328,6 +338,10 @@ func TestFormatYAMLErrorTranslation(t *testing.T) { formatted := FormatYAMLError(err, 1, tt.yamlContent) assert.Contains(t, formatted, tt.shouldContain, "FormatYAMLError should contain %q\nResult: %s", tt.shouldContain, formatted) + if tt.shouldContainExtra != "" { + assert.Contains(t, formatted, tt.shouldContainExtra, + "FormatYAMLError should contain %q\nResult: %s", tt.shouldContainExtra, formatted) + } if tt.shouldExclude != "" { assert.NotContains(t, formatted, tt.shouldExclude, "FormatYAMLError should not contain %q\nResult: %s", tt.shouldExclude, formatted) diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index 166339555d6..39ae6527492 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -43,11 +43,11 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) } // 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") + engineLine, engineCol := findFrontmatterFieldValuePosition(result.FrontmatterLines, result.FrontmatterStart, "engine") if engineLine > 0 { // Read source context lines (±3 lines around the error) for Rust-style rendering contextLines := readSourceContextLines(content, engineLine) - return nil, formatCompilerErrorWithContext(cleanPath, engineLine, 1, "error", err.Error(), err, contextLines) + return nil, formatCompilerErrorWithContext(cleanPath, engineLine, engineCol, "error", err.Error(), err, contextLines) } return nil, formatCompilerError(cleanPath, "error", err.Error(), err) } diff --git a/pkg/workflow/compiler_string_api.go b/pkg/workflow/compiler_string_api.go index 0a9982d4655..9a47357fd94 100644 --- a/pkg/workflow/compiler_string_api.go +++ b/pkg/workflow/compiler_string_api.go @@ -117,7 +117,15 @@ func (c *Compiler) ParseWorkflowString(content string, virtualPath string) (*Wor // Setup engine and process imports engineSetup, err := c.setupEngineAndImports(parseResult.frontmatterResult, parseResult.cleanPath, parseResult.content, parseResult.markdownDir) if err != nil { - return nil, err + if isFormattedCompilerError(err) { + return nil, err + } + engineLine, engineCol := findFrontmatterFieldValuePosition(result.FrontmatterLines, result.FrontmatterStart, "engine") + if engineLine > 0 { + contextLines := readSourceContextLines(parseResult.content, engineLine) + return nil, formatCompilerErrorWithContext(cleanPath, engineLine, engineCol, "error", err.Error(), err, contextLines) + } + return nil, formatCompilerError(cleanPath, "error", err.Error(), err) } // Process tools and markdown diff --git a/pkg/workflow/compiler_string_api_test.go b/pkg/workflow/compiler_string_api_test.go index 230d9b4ace1..de6ac0a5092 100644 --- a/pkg/workflow/compiler_string_api_test.go +++ b/pkg/workflow/compiler_string_api_test.go @@ -68,6 +68,29 @@ on: {{{ require.Error(t, err) } +func TestParseWorkflowString_InvalidEngineHasLocationContext(t *testing.T) { + markdown := `--- +on: push +engine: claaude +--- + +# Broken +` + + compiler := NewCompiler( + WithNoEmit(true), + WithSkipValidation(true), + ) + + _, err := compiler.ParseWorkflowString(markdown, "workflow.md") + require.Error(t, err) + + errStr := err.Error() + assert.Contains(t, errStr, "workflow.md:3:9:", "invalid engine should include file:line:column location") + assert.Contains(t, errStr, "invalid engine:", "invalid engine should be preserved in message") + assert.Contains(t, errStr, "engine: claaude", "error should include source snippet context") +} + func TestParseWorkflowString_SharedWorkflowDetection(t *testing.T) { // Shared workflows have no 'on' trigger field markdown := `--- diff --git a/pkg/workflow/compiler_yaml_test.go b/pkg/workflow/compiler_yaml_test.go index 451af36025c..e12ccea11d7 100644 --- a/pkg/workflow/compiler_yaml_test.go +++ b/pkg/workflow/compiler_yaml_test.go @@ -402,7 +402,7 @@ engine: copilot Test content.`, expectedLineCol: "[3:1]", // Line 3 in file (permissions without colon) - expectedInError: []string{"missing ':' after key", "permissions"}, + expectedInError: []string{"missing ':' after key", "permissions", "Example:\nengine: copilot"}, expectPointer: true, description: "missing colon shows formatted output", }, @@ -480,6 +480,7 @@ func TestEngineValidationErrorHasFileLocation(t *testing.T) { name string content string expectedErrorLine int + expectedErrorCol int description string }{ { @@ -493,6 +494,7 @@ engine: openai_gpt Content.`, expectedErrorLine: 3, // "engine: openai_gpt" is on line 3 in the file + expectedErrorCol: 9, // value starts after "engine: " description: "invalid engine on line 3 should produce error at line 3", }, { @@ -509,6 +511,7 @@ strict: false Content.`, expectedErrorLine: 5, // "engine: badengine_xyz" is on line 5 in the file + expectedErrorCol: 9, // value starts after "engine: " description: "invalid engine after permissions block should produce error at correct line", }, } @@ -530,7 +533,7 @@ Content.`, errorStr := err.Error() // Verify error contains filename:line:col: format pointing to the engine field - expectedPattern := fmt.Sprintf(".md:%d:1:", tt.expectedErrorLine) + expectedPattern := fmt.Sprintf(".md:%d:%d:", tt.expectedErrorLine, tt.expectedErrorCol) if !strings.Contains(errorStr, expectedPattern) { t.Errorf("%s: error should contain '%s' pointing to the engine: field, got: %s", tt.description, expectedPattern, errorStr) diff --git a/pkg/workflow/frontmatter_error.go b/pkg/workflow/frontmatter_error.go index 9b5613caef9..eb8e39dda09 100644 --- a/pkg/workflow/frontmatter_error.go +++ b/pkg/workflow/frontmatter_error.go @@ -59,15 +59,39 @@ func readSourceContextLines(content []byte, targetLine int) []string { // 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 { + line, _ := findFrontmatterFieldValuePosition(frontmatterLines, frontmatterStart, fieldName) + return line +} + +// findFrontmatterFieldValuePosition searches frontmatterLines for a top-level +// key that matches fieldName and returns the 1-based document line and column of +// the field value. For "engine: claaude", this points to the "c" in "claaude". +// Returns (0, 0) if the field is not found. +func findFrontmatterFieldValuePosition(frontmatterLines []string, frontmatterStart int, fieldName string) (int, 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 + docLine := frontmatterStart + i + colonIndex := strings.Index(line, ":") + if colonIndex < 0 { + return docLine, 1 + } + + valueStart := colonIndex + 1 // 0-based index right after ':' + for valueStart < len(line) && line[valueStart] == ' ' { + valueStart++ + } + if valueStart >= len(line) { + // No value present - point at the key/value separator. + return docLine, colonIndex + 1 + } + + return docLine, valueStart + 1 // convert to 1-based column } } - return 0 + return 0, 0 } // createFrontmatterError creates a detailed error for frontmatter parsing issues @@ -87,13 +111,36 @@ func (c *Compiler) createFrontmatterError(filePath, content string, err error, f line := matches[1] col := matches[2] message := matches[3] - // Extract just the first line of the message (before newline) - if idx := strings.Index(message, "\n"); idx != -1 { - message = message[:idx] + + // Preserve any fix-guidance block that may be emitted after the header line + // (for example "Example:\nengine: copilot"), while keeping source context + // lines separate for Rust-style rendering below. + if _, parsedErrorBody, found := strings.Cut(errorStr, frontmatterParseErrPrefix); found { + messageSection := strings.TrimSpace(parsedErrorBody) + if loc := sourceContextPattern.FindStringIndex(parsedErrorBody); loc != nil { + messageSection = strings.TrimSpace(parsedErrorBody[:loc[0]]) + } + + headerLine := messageSection + trailingGuidance := "" + if firstLine, remainder, foundNewline := strings.Cut(messageSection, "\n"); foundNewline { + headerLine = firstLine + trailingGuidance = remainder + } + if headerMatches := lineColPattern.FindStringSubmatch(headerLine); len(headerMatches) >= 4 { + message = headerMatches[3] + } else { + message = strings.TrimSpace(headerLine) + } + + // Translate raw YAML parser messages to user-friendly plain English. + // Uses the shared translation table from pkg/parser to keep both code paths in sync. + message = parser.TranslateYAMLMessage(message) + + if guidance := strings.TrimSpace(trailingGuidance); guidance != "" { + message = message + "\n" + guidance + } } - // Translate raw YAML parser messages to user-friendly plain English. - // Uses the shared translation table from pkg/parser to keep both code paths in sync. - message = parser.TranslateYAMLMessage(message) // Format as: filename:line:column: error: message // This is compatible with VSCode's problem matcher diff --git a/pkg/workflow/frontmatter_error_test.go b/pkg/workflow/frontmatter_error_test.go index 3a207dd2b27..42174e34723 100644 --- a/pkg/workflow/frontmatter_error_test.go +++ b/pkg/workflow/frontmatter_error_test.go @@ -86,6 +86,58 @@ func TestFindFrontmatterFieldLine(t *testing.T) { } } +func TestFindFrontmatterFieldValuePosition(t *testing.T) { + tests := []struct { + name string + frontmatterLines []string + frontmatterStart int + fieldName string + expectedLine int + expectedColumn int + }{ + { + name: "engine value with space after colon", + frontmatterLines: []string{"on: push", "engine: claaude"}, + frontmatterStart: 2, + fieldName: "engine", + expectedLine: 3, + expectedColumn: 9, + }, + { + name: "engine value without space after colon", + frontmatterLines: []string{"engine:claaude"}, + frontmatterStart: 2, + fieldName: "engine", + expectedLine: 2, + expectedColumn: 8, + }, + { + name: "engine key without value points at colon", + frontmatterLines: []string{"engine:"}, + frontmatterStart: 2, + fieldName: "engine", + expectedLine: 2, + expectedColumn: 7, + }, + { + name: "missing field returns zeros", + frontmatterLines: []string{"on: push"}, + frontmatterStart: 2, + fieldName: "engine", + expectedLine: 0, + expectedColumn: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + line, col := findFrontmatterFieldValuePosition(tt.frontmatterLines, tt.frontmatterStart, tt.fieldName) + assert.Equal(t, tt.expectedLine, line, "line should match expected value") + assert.Equal(t, tt.expectedColumn, col, "column should match expected value") + }) + } +} + // TestReadSourceContextLines verifies that reading source context lines around a // target line produces the expected context window for Rust-style error rendering. func TestReadSourceContextLines(t *testing.T) {