diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index 4ed02b5e3e..ef058469fe 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -319,12 +319,9 @@ Examples: FailFast: failFast, } if _, err := cli.CompileWorkflows(cmd.Context(), config); err != nil { - // Format validation error for better user experience - // The main function will detect the ✗ prefix and print it without double formatting - if !jsonOutput { - // Return a new error with formatted message so main() can print it - return fmt.Errorf("%s", cli.FormatValidationError(err)) - } + // Return error as-is without additional formatting + // Errors from CompileWorkflows are already formatted with console.FormatError + // which provides IDE-parseable location information (file:line:column) return err } return nil @@ -685,9 +682,15 @@ func main() { if err := rootCmd.Execute(); err != nil { errMsg := err.Error() - // Check if error is already formatted (contains suggestions or starts with ✗) - // to avoid double formatting with FormatErrorMessage - if strings.Contains(errMsg, "Suggestions:") || strings.HasPrefix(errMsg, "✗") { + // Check if error is already formatted to avoid double formatting: + // - Contains suggestions (FormatErrorWithSuggestions) + // - Starts with ✗ (FormatErrorMessage) + // - Contains file:line:column: pattern (console.FormatError) + isAlreadyFormatted := strings.Contains(errMsg, "Suggestions:") || + strings.HasPrefix(errMsg, "✗") || + strings.Contains(errMsg, ":") && (strings.Contains(errMsg, "error:") || strings.Contains(errMsg, "warning:")) + + if isAlreadyFormatted { fmt.Fprintln(os.Stderr, errMsg) } else { fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) diff --git a/pkg/cli/compile_helpers.go b/pkg/cli/compile_helpers.go index 1c6dcca64b..b6176f8636 100644 --- a/pkg/cli/compile_helpers.go +++ b/pkg/cli/compile_helpers.go @@ -100,7 +100,8 @@ func compileSingleFile(compiler *workflow.Compiler, file string, stats *Compilat if err := CompileWorkflowWithValidation(compiler, file, verbose, false, false, false, false, false); err != nil { // Always show compilation errors on new line - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error())) + // Note: Don't wrap in FormatErrorMessage as the error is already formatted by console.FormatError + fmt.Fprintln(os.Stderr, err.Error()) stats.Errors++ stats.FailedWorkflows = append(stats.FailedWorkflows, filepath.Base(file)) } else { diff --git a/pkg/console/console.go b/pkg/console/console.go index 1baf9e0e54..34f5232b20 100644 --- a/pkg/console/console.go +++ b/pkg/console/console.go @@ -47,6 +47,7 @@ func applyStyle(style lipgloss.Style, text string) string { } // ToRelativePath converts an absolute path to a relative path from the current working directory +// If the relative path contains "..", returns the absolute path instead for clarity func ToRelativePath(path string) string { if !filepath.IsAbs(path) { return path @@ -64,6 +65,11 @@ func ToRelativePath(path string) string { return path } + // If the relative path contains "..", use absolute path instead for clarity + if strings.Contains(relPath, "..") { + return path + } + return relPath } diff --git a/pkg/console/console_test.go b/pkg/console/console_test.go index d77cb372d9..3be2edcd5e 100644 --- a/pkg/console/console_test.go +++ b/pkg/console/console_test.go @@ -3,6 +3,7 @@ package console import ( + "os" "path/filepath" "strings" "testing" @@ -298,11 +299,28 @@ func TestToRelativePath(t *testing.T) { }, }, { - name: "absolute path converted to relative", + name: "absolute path with .. in relative form returns absolute", path: "/tmp/gh-aw/test.md", expectedFunc: func(result, expected string) bool { - // Should be a relative path that doesn't start with / - return !strings.HasPrefix(result, "/") && strings.HasSuffix(result, "test.md") + // When relative path would contain "..", should return absolute path + // The relative path from /home/runner/work/gh-aw/gh-aw to /tmp/gh-aw/test.md + // would be ../../../../../tmp/gh-aw/test.md (contains ..) + // So we should get the absolute path back + return result == "/tmp/gh-aw/test.md" + }, + }, + { + name: "absolute path within working directory converted to relative", + path: func() string { + // Get current working directory and construct a path within it + wd, _ := os.Getwd() + return filepath.Join(wd, "pkg/console/test.md") + }(), + expectedFunc: func(result, expected string) bool { + // Absolute path within working directory should be converted to relative + // without ".." in the path + // The result should not start with / and should not contain .. + return !strings.HasPrefix(result, "/") && !strings.Contains(result, "..") && strings.HasSuffix(result, "test.md") }, }, } @@ -336,13 +354,14 @@ func TestFormatErrorWithAbsolutePaths(t *testing.T) { // The output should contain test.md and line:column information if !strings.Contains(output, "test.md:5:10:") { - t.Errorf("Expected output to contain relative file path with line:column, got: %s", output) + t.Errorf("Expected output to contain file path with line:column, got: %s", output) } - // The output should not start with an absolute path (no leading /) + // Since tmpDir is outside the working directory (in /tmp), the path should be absolute + // to avoid confusing relative paths with ".." components lines := strings.Split(output, "\n") - if strings.HasPrefix(lines[0], "/") { - t.Errorf("Expected output to start with relative path, but found absolute path: %s", lines[0]) + if !strings.HasPrefix(lines[0], "/") { + t.Errorf("Expected output to start with absolute path for files outside working directory, got: %s", lines[0]) } // Should contain error message diff --git a/pkg/parser/json_path_locator.go b/pkg/parser/json_path_locator.go index e0b2146569..c88cb0fd34 100644 --- a/pkg/parser/json_path_locator.go +++ b/pkg/parser/json_path_locator.go @@ -398,12 +398,12 @@ func findNestedSection(yamlContent string, pathSegments []PathSegment) NestedSec if segment.Type == "key" { // Look for "key:" pattern keyPattern := regexp.MustCompile(`^` + regexp.QuoteMeta(segment.Value) + `\s*:`) - if keyPattern.MatchString(trimmedLine) && lineLevel == currentLevel*2 { + if keyPattern.MatchString(trimmedLine) && lineLevel == currentLevel { // Found a matching key at the correct indentation level if currentLevel == len(pathSegments)-1 { // This is the final segment - we found our target foundLine = lineNum - baseIndentLevel = lineLevel + 2 // Properties inside this object should be indented further + baseIndentLevel = lineLevel + 1 // Properties inside this object should be indented one level further break } else { // Move to the next level @@ -420,7 +420,7 @@ func findNestedSection(yamlContent string, pathSegments []PathSegment) NestedSec // Find the end of this nested section by looking for the next line at the same or lower indentation endLine := len(lines) - 1 // Default to end of file - targetLevel := baseIndentLevel - 2 // The level of the key we found + targetLevel := baseIndentLevel - 1 // The level of the key we found for lineNum := foundLine + 1; lineNum < len(lines); lineNum++ { line := lines[lineNum] diff --git a/pkg/parser/safe_outputs_error_location_test.go b/pkg/parser/safe_outputs_error_location_test.go new file mode 100644 index 0000000000..47d667dd9a --- /dev/null +++ b/pkg/parser/safe_outputs_error_location_test.go @@ -0,0 +1,387 @@ +//go:build !integration + +package parser + +import ( + "testing" +) + +// TestSafeOutputsErrorLocationAtVariousDepths tests that syntax errors in safe-outputs +// are correctly reported at various nesting depths. This addresses the issue where +// nested properties under missing-tool were incorrectly pointing to parent keys. +func TestSafeOutputsErrorLocationAtVariousDepths(t *testing.T) { + tests := []struct { + name string + yamlContent string + jsonPath string + errorMessage string + expectedLine int + expectedCol int + description string + }{ + { + name: "depth 1 - direct child of safe-outputs", + yamlContent: `on: daily +safe-outputs: + invalid-prop: true`, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': additional properties 'invalid-prop' not allowed", + expectedLine: 3, + expectedCol: 3, + description: "Error at first nesting level under safe-outputs", + }, + { + name: "depth 2 - nested under valid handler (original bug)", + yamlContent: `on: daily +safe-outputs: + create-discussion: + missing-tool: + create-discussion: true`, + jsonPath: "/safe-outputs/missing-tool", + errorMessage: "at '/safe-outputs/missing-tool': additional properties 'create-discussion' not allowed", + expectedLine: 5, + expectedCol: 5, + description: "The original bug - error should point to line 5, not line 3", + }, + { + name: "depth 2 - multiple invalid properties at same level", + yamlContent: `on: daily +safe-outputs: + create-issue: + invalid-handler: + title: test + invalid-nested: true`, + jsonPath: "/safe-outputs/invalid-handler", + errorMessage: "at '/safe-outputs/invalid-handler': additional properties 'invalid-nested' not allowed", + expectedLine: 6, + expectedCol: 5, + description: "Multiple properties at depth 2, error on second one", + }, + { + name: "depth 3 - deeply nested invalid property", + yamlContent: `on: daily +safe-outputs: + create-issue: + labels: + invalid-label-prop: value`, + jsonPath: "/safe-outputs/create-issue/labels", + errorMessage: "at '/safe-outputs/create-issue/labels': additional properties 'invalid-label-prop' not allowed", + expectedLine: 5, + expectedCol: 7, + description: "Error at third nesting level", + }, + { + name: "depth 1 - error at root safe-outputs with valid children present", + yamlContent: `on: daily +safe-outputs: + create-issue: + title: Test + unknown-prop: value`, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': additional properties 'unknown-prop' not allowed", + expectedLine: 5, + expectedCol: 3, + description: "Error at root level but after valid nested structure", + }, + { + name: "depth 2 - error in first handler", + yamlContent: `on: daily +safe-outputs: + create-issue: + invalid-field: test + create-discussion: + title: Test`, + jsonPath: "/safe-outputs/create-issue", + errorMessage: "at '/safe-outputs/create-issue': additional properties 'invalid-field' not allowed", + expectedLine: 4, + expectedCol: 5, + description: "Error in first handler at depth 2", + }, + { + name: "depth 2 - error in middle handler", + yamlContent: `on: daily +safe-outputs: + create-issue: + title: Test + invalid-handler: + some-prop: value + create-discussion: + title: Test`, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': additional properties 'invalid-handler' not allowed", + expectedLine: 5, + expectedCol: 3, + description: "Error on invalid handler name between valid handlers", + }, + { + name: "depth 2 - error in last handler", + yamlContent: `on: daily +safe-outputs: + create-issue: + title: Test + create-discussion: + invalid-prop: value`, + jsonPath: "/safe-outputs/create-discussion", + errorMessage: "at '/safe-outputs/create-discussion': additional properties 'invalid-prop' not allowed", + expectedLine: 6, + expectedCol: 5, + description: "Error in last handler", + }, + { + name: "depth 2 - multiple errors, should find first", + yamlContent: `on: daily +safe-outputs: + create-issue: + bad-handler-1: + prop: value + bad-handler-2: + prop: value`, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': additional properties 'bad-handler-1', 'bad-handler-2' not allowed", + expectedLine: 4, + expectedCol: 3, + description: "Multiple errors, should report first one found", + }, + { + name: "depth 3 - nested in github-token config", + yamlContent: `on: daily +safe-outputs: + github-token: + permissions: + invalid-perm: write`, + jsonPath: "/safe-outputs/github-token/permissions", + errorMessage: "at '/safe-outputs/github-token/permissions': additional properties 'invalid-perm' not allowed", + expectedLine: 5, + expectedCol: 7, + description: "Error in deeply nested github-token permissions", + }, + { + name: "depth 2 - error with adjacent valid properties", + yamlContent: `on: daily +safe-outputs: + create-issue: + title: Valid Title + invalid-field: bad + labels: [bug]`, + jsonPath: "/safe-outputs/create-issue", + errorMessage: "at '/safe-outputs/create-issue': additional properties 'invalid-field' not allowed", + expectedLine: 5, + expectedCol: 5, + description: "Error between valid properties", + }, + { + name: "depth 4 - very deeply nested error", + yamlContent: `on: daily +safe-outputs: + create-issue: + labels: + - bug + - invalid: + nested-prop: value`, + jsonPath: "/safe-outputs/create-issue/labels/1", + errorMessage: "at '/safe-outputs/create-issue/labels/1': additional properties 'nested-prop' not allowed", + expectedLine: 7, + expectedCol: 11, + description: "Very deep nesting level error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + location := LocateJSONPathInYAMLWithAdditionalProperties(tt.yamlContent, tt.jsonPath, tt.errorMessage) + + if !location.Found { + t.Errorf("Expected to find error location, but Found=false") + return + } + + if location.Line != tt.expectedLine { + t.Errorf("Expected Line=%d, got Line=%d. %s", tt.expectedLine, location.Line, tt.description) + } + + if location.Column != tt.expectedCol { + t.Errorf("Expected Column=%d, got Column=%d. %s", tt.expectedCol, location.Column, tt.description) + } + }) + } +} + +// TestSafeOutputsErrorLocationWithComplexYAML tests error location in more realistic, +// complex workflow configurations +func TestSafeOutputsErrorLocationWithComplexYAML(t *testing.T) { + tests := []struct { + name string + yamlContent string + jsonPath string + errorMessage string + expectedLine int + expectedCol int + description string + }{ + { + name: "complex workflow with multiple sections", + yamlContent: `name: Complex Workflow +on: + push: + branches: [main] + pull_request: +permissions: + contents: read + issues: write +safe-outputs: + max: 10 + create-issue: + title: Bug Report + missing-tool: + create-discussion: true + create-discussion: + title: Discussion`, + jsonPath: "/safe-outputs/missing-tool", + errorMessage: "at '/safe-outputs/missing-tool': additional properties 'create-discussion' not allowed", + expectedLine: 14, + expectedCol: 5, + description: "Error in safe-outputs section of complex workflow", + }, + { + name: "safe-outputs with all valid handlers plus one invalid", + yamlContent: `on: daily +safe-outputs: + create-issue: + title: Issue + create-discussion: + title: Discussion + invalid-tool: + some-prop: value + github-token: + scopes: [repo]`, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': additional properties 'invalid-tool' not allowed", + expectedLine: 7, + expectedCol: 3, + description: "Invalid handler among valid ones", + }, + { + name: "safe-outputs with comments and empty lines", + yamlContent: `on: daily +safe-outputs: + # Valid handler + create-issue: + title: Test + + # Invalid handler below + bad-handler: + prop: value`, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': additional properties 'bad-handler' not allowed", + expectedLine: 8, + expectedCol: 3, + description: "Error location with comments and empty lines", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + location := LocateJSONPathInYAMLWithAdditionalProperties(tt.yamlContent, tt.jsonPath, tt.errorMessage) + + if !location.Found { + t.Errorf("Expected to find error location, but Found=false. %s", tt.description) + return + } + + if location.Line != tt.expectedLine { + t.Errorf("Expected Line=%d, got Line=%d. %s", tt.expectedLine, location.Line, tt.description) + } + + if location.Column != tt.expectedCol { + t.Errorf("Expected Column=%d, got Column=%d. %s", tt.expectedCol, location.Column, tt.description) + } + }) + } +} + +// TestSafeOutputsEdgeCases tests edge cases in error location detection +func TestSafeOutputsEdgeCases(t *testing.T) { + tests := []struct { + name string + yamlContent string + jsonPath string + errorMessage string + expectedLine int + expectedCol int + shouldFind bool + description string + }{ + { + name: "empty safe-outputs section", + yamlContent: `on: daily +safe-outputs:`, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': expected object, got null", + expectedLine: 2, + expectedCol: 14, // After "safe-outputs:" + shouldFind: true, + description: "Empty safe-outputs section", + }, + { + name: "safe-outputs with only whitespace", + yamlContent: `on: daily +safe-outputs: + `, + jsonPath: "/safe-outputs", + errorMessage: "at '/safe-outputs': expected object, got null", + expectedLine: 2, + expectedCol: 14, // After "safe-outputs:" + shouldFind: true, + description: "Safe-outputs with only whitespace", + }, + { + name: "deeply nested with arrays", + yamlContent: `on: daily +safe-outputs: + create-issue: + labels: + - bug + - feature + invalid: value`, + jsonPath: "/safe-outputs/create-issue", + errorMessage: "at '/safe-outputs/create-issue': additional properties 'invalid' not allowed", + expectedLine: 7, + expectedCol: 5, + shouldFind: true, + description: "Error after array property", + }, + { + name: "non-existent path", + yamlContent: `on: daily +safe-outputs: + create-issue: + title: Test`, + jsonPath: "/safe-outputs/nonexistent", + errorMessage: "at '/safe-outputs/nonexistent': additional properties 'bad' not allowed", + expectedLine: 1, + expectedCol: 1, + shouldFind: false, + description: "Path doesn't exist in YAML", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + location := LocateJSONPathInYAMLWithAdditionalProperties(tt.yamlContent, tt.jsonPath, tt.errorMessage) + + if location.Found != tt.shouldFind { + t.Errorf("Expected Found=%v, got Found=%v. %s", tt.shouldFind, location.Found, tt.description) + } + + if tt.shouldFind { + if location.Line != tt.expectedLine { + t.Errorf("Expected Line=%d, got Line=%d. %s", tt.expectedLine, location.Line, tt.description) + } + + if location.Column != tt.expectedCol { + t.Errorf("Expected Column=%d, got Column=%d. %s", tt.expectedCol, location.Column, tt.description) + } + } + }) + } +} diff --git a/pkg/parser/schema_validation_depth_test.go b/pkg/parser/schema_validation_depth_test.go new file mode 100644 index 0000000000..9948bdde5a --- /dev/null +++ b/pkg/parser/schema_validation_depth_test.go @@ -0,0 +1,152 @@ +//go:build !integration + +package parser + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/testutil" +) + +// TestSchemaValidationErrorLocationAtVariousDepths tests that schema validation errors +// are reported at the correct line and column for errors at different nesting depths. +// This is an end-to-end test that validates the integration of schema validation, +// error location detection, and error formatting. +func TestSchemaValidationErrorLocationAtVariousDepths(t *testing.T) { + tests := []struct { + name string + workflowContent string + expectedLine int + expectedProperty string + description string + }{ + { + name: "depth 1 - invalid property at root level", + workflowContent: `--- +on: daily +invalid-root-prop: value +--- +Test workflow.`, + expectedLine: 3, + expectedProperty: "invalid-root-prop", + description: "Error at root level of frontmatter", + }, + { + name: "depth 2 - invalid property in safe-outputs", + workflowContent: `--- +on: daily +safe-outputs: + invalid-handler: true +--- +Test workflow.`, + expectedLine: 4, + expectedProperty: "invalid-handler", + description: "Error at depth 2 in safe-outputs", + }, + { + name: "depth 3 - invalid property in safe-outputs handler", + workflowContent: `--- +on: daily +safe-outputs: + create-issue: + invalid-field: value +--- +Test workflow.`, + expectedLine: 5, + expectedProperty: "invalid-field", + description: "Error at depth 3 in safe-outputs handler", + }, + { + name: "depth 3 - the original bug case", + workflowContent: `--- +on: daily +safe-outputs: + create-discussion: + missing-tool: + create-discussion: true +--- +Test workflow.`, + expectedLine: 6, + expectedProperty: "create-discussion", + description: "Original bug - error in nested missing-tool", + }, + { + name: "depth 3 - invalid property in MCP server config", + workflowContent: `--- +on: daily +tools: + github: + invalid-config: value +--- +Test workflow.`, + expectedLine: 5, + expectedProperty: "invalid-config", + description: "Error at depth 3 in tools.github", + }, + { + name: "depth 2 - invalid property in permissions", + workflowContent: `--- +on: daily +permissions: + invalid-perm: write +--- +Test workflow.`, + expectedLine: 4, + expectedProperty: "invalid-perm", + description: "Error at depth 2 in permissions", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a temporary directory + tmpDir := testutil.TempDir(t, "test-*") + testFile := filepath.Join(tmpDir, "test.md") + + // Write the workflow content + err := os.WriteFile(testFile, []byte(tt.workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + // Extract frontmatter from the content + result, err := ExtractFrontmatterFromContent(tt.workflowContent) + if err != nil { + t.Fatalf("Failed to extract frontmatter: %v", err) + } + + // Validate the frontmatter - this will trigger schema validation with location + err = ValidateMainWorkflowFrontmatterWithSchemaAndLocation(result.Frontmatter, testFile) + + // We expect an error + if err == nil { + t.Fatalf("Expected schema validation error, got nil. %s", tt.description) + } + + // Check that the error message contains the expected line number + errorStr := err.Error() + expectedLocation := fmt.Sprintf("%s:%d:", filepath.Base(testFile), tt.expectedLine) + + if !strings.Contains(errorStr, expectedLocation) { + t.Errorf("Expected error to contain '%s', got:\n%s\n%s", expectedLocation, errorStr, tt.description) + } + + // Check that the error message contains the expected property name + if !strings.Contains(errorStr, tt.expectedProperty) { + t.Errorf("Expected error to contain property '%s', got:\n%s\n%s", tt.expectedProperty, errorStr, tt.description) + } + + // Verify the error message is properly formatted with context + if !strings.Contains(errorStr, "error:") { + t.Errorf("Expected error to be formatted with 'error:', got:\n%s\n%s", errorStr, tt.description) + } + + // Log the error for manual verification + t.Logf("✓ Correct location (line %d) for %s", tt.expectedLine, tt.description) + }) + } +}