From 10a4a7aec888e4fcf7323e44572912d44125322b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 15 Mar 2026 03:34:58 +0000 Subject: [PATCH 1/3] Initial plan From 98ace020e0e3a1d3e804a701c643ed53b8931e72 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 15 Mar 2026 03:59:17 +0000 Subject: [PATCH 2/3] Fix engine validation error location and add test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler.go | 7 +- pkg/workflow/compiler_error_formatter.go | 28 ++++--- .../compiler_orchestrator_workflow.go | 19 ++++- pkg/workflow/compiler_yaml_test.go | 78 +++++++++++++++++++ pkg/workflow/frontmatter_error.go | 15 ++++ 5 files changed, 126 insertions(+), 21 deletions(-) diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index a85bd052df..7e3723dc35 100644 --- a/pkg/workflow/compiler.go +++ b/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) } diff --git a/pkg/workflow/compiler_error_formatter.go b/pkg/workflow/compiler_error_formatter.go index 446cf5598b..b5b64b678a 100644 --- a/pkg/workflow/compiler_error_formatter.go +++ b/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} } diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index f68a68f2b8..23ac893f85 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/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 diff --git a/pkg/workflow/compiler_yaml_test.go b/pkg/workflow/compiler_yaml_test.go index ab60ce78c0..79e7792f8f 100644 --- a/pkg/workflow/compiler_yaml_test.go +++ b/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 diff --git a/pkg/workflow/frontmatter_error.go b/pkg/workflow/frontmatter_error.go index 2a73af0abb..edbe66604b 100644 --- a/pkg/workflow/frontmatter_error.go +++ b/pkg/workflow/frontmatter_error.go @@ -59,6 +59,21 @@ 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. +func findFrontmatterFieldLine(frontmatterLines []string, frontmatterStart int, fieldName string) int { + prefix := fieldName + ":" + for i, line := range frontmatterLines { + if strings.HasPrefix(strings.TrimSpace(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 From bdc4627e5b8d6d6534bdd823c4040b6d21a25d8c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 15 Mar 2026 04:02:02 +0000 Subject: [PATCH 3/3] Add unit tests for isFormattedCompilerError and findFrontmatterFieldLine Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_error_formatter_test.go | 60 +++++++++++++ pkg/workflow/frontmatter_error.go | 7 +- pkg/workflow/frontmatter_error_test.go | 86 +++++++++++++++++++ 3 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 pkg/workflow/compiler_error_formatter_test.go create mode 100644 pkg/workflow/frontmatter_error_test.go diff --git a/pkg/workflow/compiler_error_formatter_test.go b/pkg/workflow/compiler_error_formatter_test.go new file mode 100644 index 0000000000..d057250c6c --- /dev/null +++ b/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) + }) + } +} diff --git a/pkg/workflow/frontmatter_error.go b/pkg/workflow/frontmatter_error.go index edbe66604b..d69946180e 100644 --- a/pkg/workflow/frontmatter_error.go +++ b/pkg/workflow/frontmatter_error.go @@ -64,10 +64,15 @@ func translateYAMLMessage(message string) string { // 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 { - if strings.HasPrefix(strings.TrimSpace(line), prefix) { + // 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 } } diff --git a/pkg/workflow/frontmatter_error_test.go b/pkg/workflow/frontmatter_error_test.go new file mode 100644 index 0000000000..29736d75a7 --- /dev/null +++ b/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) + }) + } +}