diff --git a/pkg/parser/schema_compiler.go b/pkg/parser/schema_compiler.go index 76629adb87a..7132fef5376 100644 --- a/pkg/parser/schema_compiler.go +++ b/pkg/parser/schema_compiler.go @@ -276,7 +276,8 @@ func validateWithSchemaAndLocation(frontmatter map[string]any, schemaJSON, conte } // Rewrite "additional properties not allowed" errors to be more friendly - message := rewriteAdditionalPropertiesError(primaryPath.Message) + // Also clean up oneOf jargon (e.g., "got string, want object") to plain English + message := rewriteAdditionalPropertiesError(cleanOneOfMessage(primaryPath.Message)) // Add schema-based suggestions suggestions := generateSchemaBasedSuggestions(schemaJSON, primaryPath.Message, primaryPath.Path) diff --git a/pkg/parser/schema_errors.go b/pkg/parser/schema_errors.go index a0527353974..b18acce5f4c 100644 --- a/pkg/parser/schema_errors.go +++ b/pkg/parser/schema_errors.go @@ -6,6 +6,9 @@ import ( "strings" ) +// atPathPattern matches "- at '/path': " or "at '/path': " prefixes in error messages +var atPathPattern = regexp.MustCompile(`^-?\s*at '([^']*)': (.+)$`) + // cleanJSONSchemaErrorMessage removes unhelpful prefixes from jsonschema validation errors func cleanJSONSchemaErrorMessage(errorMsg string) string { // Split the error message into lines @@ -37,7 +40,100 @@ func cleanJSONSchemaErrorMessage(errorMsg string) string { return "schema validation failed" } - return result + // Apply oneOf cleanup to the full cleaned message + return cleanOneOfMessage(result) +} + +// cleanOneOfMessage simplifies 'oneOf failed, none matched' error messages by: +// 1. Removing "got X, want Y" type-mismatch lines (from the wrong branch of a oneOf) +// 2. Removing the "oneOf failed, none matched" wrapper line +// 3. Extracting the most meaningful sub-error (e.g., enum constraint violations) +// +// This converts confusing schema jargon like: +// +// "'oneOf' failed, none matched\n- at '/engine': value must be one of...\n- at '/engine': got string, want object" +// +// into plain language: +// +// "value must be one of 'claude', 'codex', 'copilot', 'gemini'" +func cleanOneOfMessage(message string) string { + if !strings.Contains(message, "'oneOf' failed") { + return message + } + + lines := strings.Split(message, "\n") + var meaningful []string + + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + // Skip the "oneOf failed" wrapper line — it's schema jargon, not user guidance + if strings.Contains(trimmed, "'oneOf' failed, none matched") { + continue + } + // Skip "got X, want Y" type-mismatch lines from the wrong oneOf branch + if isTypeConflictLine(trimmed) { + continue + } + meaningful = append(meaningful, trimmed) + } + + if len(meaningful) == 0 { + return message // Return original if we cannot simplify + } + + // Strip "- at '/path':" prefixes and format each remaining constraint + var cleaned []string + for _, line := range meaningful { + cleaned = append(cleaned, stripAtPathPrefix(line)) + } + + return strings.Join(cleaned, "; ") +} + +// isTypeConflictLine returns true for "got X, want Y" lines that arise from the +// wrong branch of a oneOf constraint. These lines are generated when the user's value +// matches one branch's type but not the other, and they are confusing to display. +// Handles both bare "got X, want Y" and embedded "- at '/path': got X, want Y" forms. +func isTypeConflictLine(line string) bool { + // Direct "got X, want Y" format (bare form) + if strings.HasPrefix(line, "got ") && strings.Contains(line, ", want ") { + return true + } + // Embedded form: "- at '/path': got X, want Y" + // Look for ": got " followed by ", want " later in the line + if idx := strings.Index(line, ": got "); idx >= 0 { + afterGot := line[idx+len(": got "):] + return strings.Contains(afterGot, ", want ") + } + return false +} + +// stripAtPathPrefix removes "- at '/path': " or "at '/path': " prefixes from schema error lines +// and formats nested path references to be more readable. +// +// Examples: +// - "- at '/engine': value must be one of..." → "value must be one of..." +// - "- at '/permissions/deployments': value must be..." → "'deployments': value must be..." +func stripAtPathPrefix(line string) string { + match := atPathPattern.FindStringSubmatch(line) + if match == nil { + return line + } + path := match[1] + msg := match[2] + + // For nested paths (e.g., /permissions/deployments), keep the last component + // so users know which sub-field has the error + if idx := strings.LastIndex(path, "/"); idx > 0 { + subField := path[idx+1:] + return fmt.Sprintf("'%s': %s", subField, msg) + } + + // For top-level field errors, just return the constraint message + return msg } // findFrontmatterBounds finds the start and end indices of frontmatter in file lines diff --git a/pkg/parser/schema_errors_test.go b/pkg/parser/schema_errors_test.go new file mode 100644 index 00000000000..e3002b76e1b --- /dev/null +++ b/pkg/parser/schema_errors_test.go @@ -0,0 +1,223 @@ +//go:build !integration + +package parser + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestCleanOneOfMessage tests that oneOf error messages are simplified to plain English +func TestCleanOneOfMessage(t *testing.T) { + tests := []struct { + name string + input string + wantNot []string // substrings that must NOT appear in output + wantAny []string // at least one of these must appear in output + }{ + { + name: "engine typo removes got-string-want-object branch", + input: "at '/engine': 'oneOf' failed, none matched\n" + + "- at '/engine': value must be one of 'claude', 'codex', 'copilot', 'gemini'\n" + + "- at '/engine': got string, want object", + wantNot: []string{"oneOf", "got string, want object"}, + wantAny: []string{"value must be one of 'claude', 'codex', 'copilot', 'gemini'"}, + }, + { + name: "permissions typo removes got-object-want-string branch", + input: "at '/permissions': 'oneOf' failed, none matched\n" + + "- at '/permissions': got object, want string\n" + + "- at '/permissions/deployments': value must be one of 'read', 'write', 'none'", + wantNot: []string{"oneOf", "got object, want string"}, + wantAny: []string{"value must be one of 'read', 'write', 'none'"}, + }, + { + name: "non-oneOf message is returned unchanged", + input: "value must be one of 'a', 'b', 'c'", + wantNot: []string{"oneOf"}, + wantAny: []string{"value must be one of 'a', 'b', 'c'"}, + }, + { + name: "nested path context preserved for sub-field errors", + input: "at '/permissions': 'oneOf' failed, none matched\n" + + "- at '/permissions': got object, want string\n" + + "- at '/permissions/deployments': value must be one of 'read', 'write', 'none'", + wantNot: []string{}, + wantAny: []string{"deployments"}, + }, + { + name: "message unchanged when all sub-errors are type conflicts", + input: "at '/x': 'oneOf' failed, none matched\n" + + "- at '/x': got string, want object\n" + + "- at '/x': got string, want array", + // When nothing meaningful remains, return the original + wantAny: []string{"oneOf"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := cleanOneOfMessage(tt.input) + + for _, unwanted := range tt.wantNot { + assert.NotContains(t, result, unwanted, + "Result should not contain %q\nResult: %s", unwanted, result) + } + + if len(tt.wantAny) > 0 { + found := false + for _, wanted := range tt.wantAny { + if strings.Contains(result, wanted) { + found = true + break + } + } + assert.True(t, found, + "Result should contain at least one of %v\nResult: %s", tt.wantAny, result) + } + }) + } +} + +// TestIsTypeConflictLine tests detection of "got X, want Y" lines in oneOf errors +func TestIsTypeConflictLine(t *testing.T) { + tests := []struct { + name string + line string + want bool + }{ + { + name: "bare got-want format", + line: "got string, want object", + want: true, + }, + { + name: "embedded in at-path format", + line: "- at '/engine': got string, want object", + want: true, + }, + { + name: "embedded got-object-want-string", + line: "- at '/permissions': got object, want string", + want: true, + }, + { + name: "enum constraint is not a type conflict", + line: "- at '/engine': value must be one of 'claude', 'codex'", + want: false, + }, + { + name: "additional property error is not a type conflict", + line: "additional property 'foo' not allowed", + want: false, + }, + { + name: "empty line is not a type conflict", + line: "", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isTypeConflictLine(tt.line) + assert.Equal(t, tt.want, got, + "isTypeConflictLine(%q) = %v, want %v", tt.line, got, tt.want) + }) + } +} + +// TestStripAtPathPrefix tests removal of "at '/path':" prefixes from error lines +func TestStripAtPathPrefix(t *testing.T) { + tests := []struct { + name string + line string + want string + }{ + { + name: "top-level path stripped entirely", + line: "- at '/engine': value must be one of 'claude', 'codex'", + want: "value must be one of 'claude', 'codex'", + }, + { + name: "nested path keeps last component", + line: "- at '/permissions/deployments': value must be one of 'read', 'write', 'none'", + want: "'deployments': value must be one of 'read', 'write', 'none'", + }, + { + name: "line without at-path prefix is unchanged", + line: "value must be one of 'a', 'b'", + want: "value must be one of 'a', 'b'", + }, + { + name: "at-path without dash prefix", + line: "at '/engine': some error", + want: "some error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := stripAtPathPrefix(tt.line) + assert.Equal(t, tt.want, got, + "stripAtPathPrefix(%q) = %q, want %q", tt.line, got, tt.want) + }) + } +} + +// TestCleanJSONSchemaErrorMessage tests the full cleanup pipeline for jsonschema errors +func TestCleanJSONSchemaErrorMessage(t *testing.T) { + tests := []struct { + name string + input string + wantNot []string + wantAny []string + }{ + { + name: "removes jsonschema validation failed header", + input: "jsonschema validation failed with 'http://contoso.com/schema.json#'\n" + + "- at '/engine': 'oneOf' failed, none matched\n" + + "- at '/engine': value must be one of 'claude', 'codex'\n" + + "- at '/engine': got string, want object", + wantNot: []string{"jsonschema validation failed", "contoso.com", "got string, want object", "oneOf"}, + wantAny: []string{"value must be one of 'claude', 'codex'"}, + }, + { + name: "removes at-root prefix", + input: "jsonschema validation failed with '...'\n" + + "- at '': additional property 'foo' not allowed", + wantNot: []string{"jsonschema validation failed", "at '': "}, + wantAny: []string{"additional property 'foo' not allowed"}, + }, + { + name: "empty result falls back to generic message", + input: "jsonschema validation failed with '...'", + wantAny: []string{"schema validation failed"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := cleanJSONSchemaErrorMessage(tt.input) + + for _, unwanted := range tt.wantNot { + assert.NotContains(t, result, unwanted, + "Result should not contain %q\nResult: %s", unwanted, result) + } + + if len(tt.wantAny) > 0 { + found := false + for _, wanted := range tt.wantAny { + if strings.Contains(result, wanted) { + found = true + break + } + } + assert.True(t, found, + "Result should contain at least one of %v\nResult: %s", tt.wantAny, result) + } + }) + } +} diff --git a/pkg/workflow/frontmatter_error.go b/pkg/workflow/frontmatter_error.go index 91b2d342a27..2cc4c4ba342 100644 --- a/pkg/workflow/frontmatter_error.go +++ b/pkg/workflow/frontmatter_error.go @@ -16,6 +16,37 @@ var ( sourceContextPattern = regexp.MustCompile(`\n(\s+\d+\s*\|)`) ) +// yamlErrorTranslations maps raw goccy/go-yaml internal messages to user-friendly plain English. +// These messages are parser internals that are not helpful to end users. +var yamlErrorTranslations = []struct { + pattern string + translation string +}{ + { + "non-map value is specified", + "Invalid YAML syntax: expected 'key: value' format (did you forget a colon after the key?)", + }, + { + "mapping values are not allowed", + "Invalid YAML syntax: unexpected ':' — check your indentation", + }, + { + "did not find expected", + "Invalid YAML syntax: check indentation or missing key", + }, +} + +// translateYAMLMessage converts raw YAML parser messages to user-friendly plain English. +// This prevents internal library jargon from reaching the end user. +func translateYAMLMessage(message string) string { + for _, t := range yamlErrorTranslations { + if strings.Contains(message, t.pattern) { + return t.translation + } + } + return message +} + // 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 @@ -37,6 +68,8 @@ func (c *Compiler) createFrontmatterError(filePath, content string, err error, f if idx := strings.Index(message, "\n"); idx != -1 { message = message[:idx] } + // Translate raw YAML parser messages to user-friendly plain English + message = translateYAMLMessage(message) // Format as: filename:line:column: error: message // This is compatible with VSCode's problem matcher diff --git a/pkg/workflow/jobs_full_spec_test.go b/pkg/workflow/jobs_full_spec_test.go index ad9f49ef0ba..f7ec7568565 100644 --- a/pkg/workflow/jobs_full_spec_test.go +++ b/pkg/workflow/jobs_full_spec_test.go @@ -248,7 +248,7 @@ jobs: `, // Schema validation correctly rejects steps without 'uses' or 'run' shouldError: true, - errorMsg: "oneOf", + errorMsg: "missing property", }, { name: "step with both uses and run should be invalid", diff --git a/pkg/workflow/yaml_message_translation_test.go b/pkg/workflow/yaml_message_translation_test.go new file mode 100644 index 00000000000..033e628636d --- /dev/null +++ b/pkg/workflow/yaml_message_translation_test.go @@ -0,0 +1,75 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestTranslateYAMLMessage tests that raw goccy/go-yaml error messages are translated to plain English +func TestTranslateYAMLMessage(t *testing.T) { + tests := []struct { + name string + input string + wantNot []string // substrings that must NOT appear in output + wantAny []string // at least one of these must appear in output + }{ + { + name: "non-map value translated to user-friendly message", + input: "non-map value is specified", + wantNot: []string{"non-map value is specified"}, + wantAny: []string{"Invalid YAML syntax", "key: value", "colon"}, + }, + { + name: "mapping values not allowed translated", + input: "mapping values are not allowed in this context", + wantNot: []string{"mapping values are not allowed"}, + wantAny: []string{"Invalid YAML syntax", "indentation"}, + }, + { + name: "did not find expected translated", + input: "did not find expected key", + wantNot: []string{"did not find expected"}, + wantAny: []string{"Invalid YAML syntax"}, + }, + { + name: "unrecognized message returned unchanged", + input: "found unknown escape character 'z'", + wantNot: []string{}, + wantAny: []string{"found unknown escape character 'z'"}, + }, + { + name: "empty message returned unchanged", + input: "", + wantNot: []string{}, + wantAny: []string{""}, + }, + { + name: "partially matching message translated", + input: "[3:1] non-map value is specified as a key\n 2 | foo: bar\n> 3 | baz qux\n ^", + wantNot: []string{"non-map value is specified"}, + wantAny: []string{"Invalid YAML syntax"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := translateYAMLMessage(tt.input) + + for _, unwanted := range tt.wantNot { + assert.NotContains(t, result, unwanted, + "Result should not contain %q\nResult: %s", unwanted, result) + } + + for _, wanted := range tt.wantAny { + if wanted == "" { + continue + } + assert.Contains(t, result, wanted, + "Result should contain %q\nResult: %s", wanted, result) + } + }) + } +}