Improve compiler error messages for invalid engine: types

docs/adr/25821-rust-style-source-context-and-plain-english-synthesis-for-compiler-errors.md

@@ -0,0 +1,80 @@
+# ADR-25821: Rust-Style Source Context and Plain-English Synthesis for Compiler Errors
+
+**Date**: 2026-04-11
+**Status**: Draft
+**Deciders**: pelikhan, Copilot
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+The gh-aw workflow compiler produces error messages when a workflow file contains invalid configuration. Before this decision, two distinct quality problems coexisted. First, when the `engine:` field received a value of the wrong type (e.g., an integer instead of a string), the compiler surfaced raw JSON Schema jargon such as `'oneOf' failed, none matched: got number, want string; got number, want object`, giving the user no actionable guidance. Second, when a valid-type but unrecognised engine name was used (e.g., a typo like `copiilot`), the error message correctly identified the problem but omitted source-file context lines, making it harder to pinpoint the exact location. The two code paths had inconsistent output quality, and the gap was widest precisely when users were most confused.
+
+### Decision
+
+We will adopt Rust-style compiler error rendering as the standard for field-level validation errors in the workflow compiler: errors that can be localised to a specific source line will include ±3 lines of source context to allow the user to see the offending code in-line. Simultaneously, we will introduce plain-English synthesis for JSON Schema `oneOf` type-conflict failures: when every branch of a `oneOf` constraint fails with a type mismatch, the compiler extracts the actual and expected types and produces a sentence such as `expected string or object, got number`. For well-known fields (currently `/engine`), a field-specific hint table appends a list of valid values and a usage example, creating output comparable to modern language compilers (Rust, Go 1.20+).
+
+### Alternatives Considered
+
+#### Alternative 1: Generic "wrong type" message without field hints
+
+The simplest fix was to replace JSON Schema jargon with a static message like `"invalid value: expected a string or object"` for all `oneOf` type failures. This was considered because it requires no per-field maintenance. It was rejected because it omits valid values, which is the most actionable piece of information for the user — knowing that `claude`, `codex`, `copilot`, and `gemini` are accepted is more useful than knowing the abstract type.
+
+#### Alternative 2: Runtime engine list lookup from the catalog
+
+An alternative to the static hint table was to look up the list of valid engine names dynamically from `NewEngineCatalog()` at error-reporting time. This would have made the hint automatically accurate if new built-in engines were added. It was not chosen because error formatting is in `pkg/parser` (a lower-level package), while the engine catalog lives in a higher-level layer; pulling the catalog reference into the parser would create an undesirable dependency. The static list is a deliberate trade-off: it requires a manual update when built-in engines change, but it preserves the package boundary.
+
+#### Alternative 3: Position-only error (status quo for engine name errors)
+
+The existing `formatCompilerErrorWithPosition` path was already used for engine-name typos (e.g., `copiilot`). Keeping that path and only fixing the type-conflict message was considered as a minimal change. It was rejected because source context lines are low-cost to add (the file content is already in memory at this call site) and significantly improve diagnostic value, aligning the output with user expectations set by modern compilers.
+
+### Consequences
+
+#### Positive
+- Type-conflict errors for `engine:` are now actionable without requiring the user to look up documentation.
+- Engine-name typos now show a Rust-style source snippet with a column pointer, matching output quality of schema-validation errors.
+- The `isTypeConflictLine` predicate is now precise: it rejects constraint-violation lines (e.g., `minItems: got 0, want 1`) that were previously false-positived, reducing noise in other `oneOf` error paths.
+
+#### Negative
+- The `knownOneOfFieldHints` table in `pkg/parser/schema_errors.go` is a static list of field paths and valid values. It will silently become stale if built-in engines are added or removed without also updating the table.
+- `readSourceContextLines` always returns a fixed 7-line window (±3). Errors near the start or end of a file receive padding with empty strings, which requires downstream rendering logic to tolerate empty lines gracefully.
+
+#### Neutral
+- Rust-style rendering requires the caller (currently `compiler_orchestrator_workflow.go`) to pass pre-loaded file content to `readSourceContextLines`. This makes the call site slightly more verbose but keeps the I/O concern at the orchestration layer.
+- The `console.CompilerError.Context` field must already be populated by other code paths; this change adds a second call site that exercises it.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Field-Level Error Rendering
+
+1. When a compiler error is localised to a specific source line and the file content is available in memory at the call site, the implementation **MUST** include source context lines in the error using `formatCompilerErrorWithContext`.
+2. Implementations **MUST NOT** call `formatCompilerErrorWithPosition` for errors where file content is already loaded; `formatCompilerErrorWithContext` **MUST** be used instead.
+3. Source context **MUST** span a window of ±3 lines around the error line (7 lines total), padded with empty strings when the window extends before the start of the file.
+4. Implementations **SHOULD** pass `nil` as the `cause` argument to `formatCompilerErrorWithContext` for pure validation errors that have no underlying Go error to wrap.
+
+### oneOf Type-Conflict Error Synthesis
+
+1. When all sub-errors of a `oneOf` constraint are type conflicts (i.e., `cleanOneOfMessage` produces an empty `meaningful` list), implementations **MUST** call `synthesizeOneOfTypeConflictMessage` rather than returning the raw jargon string.
+2. `synthesizeOneOfTypeConflictMessage` **MUST** produce a message of the form `"expected T1 or T2, got G"` where `T1`, `T2` are the distinct expected JSON Schema type names and `G` is the actual type.
+3. When the JSON Schema path of the failing field matches a key in `knownOneOfFieldHints`, the synthesized message **MUST** append the corresponding hint text.
+4. `knownOneOfFieldHints` **MUST** be updated whenever the set of built-in engines in `NewEngineCatalog` changes.
+5. Implementations **MUST NOT** include field-specific hints for paths not present in `knownOneOfFieldHints`; unknown fields **SHOULD** receive the generic type-mismatch message only.
+
+### Type-Conflict Line Detection
+
+1. `isTypeConflictLine` **MUST** validate that both the "got" and "want" tokens in a `"got X, want Y"` pattern are valid JSON Schema type names (`string`, `object`, `array`, `number`, `integer`, `boolean`, `null`) before classifying the line as a type conflict.
+2. Implementations **MUST NOT** classify constraint-violation lines (e.g., `"minItems: got 0, want 1"`) as type conflicts.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24285718804) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/parser/schema_errors.go

@@ -107,7 +107,9 @@ func cleanOneOfMessage(message string) string {
 	}
 
 	if len(meaningful) == 0 {
-		return message // Return original if we cannot simplify
+		// All sub-errors were type conflicts — synthesize a plain-English message
+		// instead of returning raw JSON Schema jargon.
+		return synthesizeOneOfTypeConflictMessage(lines)
 	}
 
 	// Strip "- at '/path':" prefixes and format each remaining constraint
@@ -119,22 +121,104 @@ func cleanOneOfMessage(message string) string {
 	return strings.Join(cleaned, "; ")
 }
 
+// typeConflictGotWantPattern extracts "got X, want Y" components from type-conflict lines.
+// Matches both bare "got X, want Y" and embedded "- at '/path': got X, want Y" forms.
+var typeConflictGotWantPattern = regexp.MustCompile(`(?:^|: )got (\w+), want (\w+)$`)
+
+// knownOneOfFieldHints provides field-specific guidance for oneOf type-conflict fallback
+// messages. When all oneOf branches fail with type-mismatch errors (e.g., the user passes
+// an integer where a string or object is expected), these hints are appended to the
+// synthesized plain-English message to help the user fix the problem.
+//
+// The engine list mirrors the built-in engines in NewEngineCatalog.
+// Update this list when built-in engines change.
+var knownOneOfFieldHints = map[string]string{
+	"/engine": "Valid engine names: claude, codex, copilot, gemini.\n\nExample:\nengine: copilot",
+}
+
+// synthesizeOneOfTypeConflictMessage produces a plain-English error message when every
+// sub-error of a oneOf constraint is a type conflict (e.g., "got number, want string"
+// and "got number, want object"). It extracts the actual and expected types from the
+// conflict lines and, for well-known fields, appends guidance with valid values.
+func synthesizeOneOfTypeConflictMessage(lines []string) string {
+	var gotType string
+	var wantTypes []string
+	var path string
+
+	for _, line := range lines {
+		trimmed := strings.TrimSpace(line)
+		if !isTypeConflictLine(trimmed) {
+			continue
+		}
+		// Extract path from "- at '/path': got X, want Y"
+		if match := atPathPattern.FindStringSubmatch(trimmed); match != nil {
+			if path == "" {
+				path = match[1]
+			}
+		}
+		// Extract got/want types
+		if match := typeConflictGotWantPattern.FindStringSubmatch(trimmed); match != nil {
+			if gotType == "" {
+				gotType = match[1]
+			}
+			wantTypes = append(wantTypes, match[2])
+		}
+	}
+
+	if gotType == "" || len(wantTypes) == 0 {
+		return "schema validation failed"
+	}
+
+	// Deduplicate expected types (e.g., multiple "object" branches in oneOf)
+	seen := make(map[string]bool)
+	var uniqueWantTypes []string
+	for _, t := range wantTypes {
+		if !seen[t] {
+			seen[t] = true
+			uniqueWantTypes = append(uniqueWantTypes, t)
+		}
+	}
+
+	result := fmt.Sprintf("expected %s, got %s", strings.Join(uniqueWantTypes, " or "), gotType)
+
+	// Add field-specific hints for known fields
+	if hint, ok := knownOneOfFieldHints[path]; ok {
+		result += ". " + hint
+	}
+
+	return result
+}
+
+// jsonTypeNames is the set of valid JSON Schema type names. Used to distinguish
+// actual type conflicts ("got number, want string") from constraint violations
+// ("minItems: got 0, want 1") in oneOf error messages.
+var jsonTypeNames = map[string]bool{
+	"string": true, "object": true, "array": true, "number": true,
+	"integer": true, "boolean": true, "null": true,
+}
+
+// typeConflictPattern matches "got TYPE, want TYPE" where TYPE must be a JSON type name.
+// This avoids false positives on constraint violations like "minItems: got 0, want 1".
+var typeConflictPattern = regexp.MustCompile(`got (\w+), want (\w+)`)
+
 // 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.
+//
+// Only matches when both X and Y are JSON Schema type names (string, object, array,
+// number, integer, boolean, null), to avoid misidentifying constraint violations
+// (e.g., "minItems: got 0, want 1") as type conflicts.
 func isTypeConflictLine(line string) bool {
-	// Direct "got X, want Y" format (bare form)
-	if strings.HasPrefix(line, "got ") && strings.Contains(line, ", want ") {
-		return true
+	// Fast-path: skip regex for lines that clearly aren't type conflicts
+	if !strings.Contains(line, "got ") || !strings.Contains(line, ", want ") {
+		return false
 	}
-	// Embedded form: "- at '/path': got X, want Y"
-	// Look for ": got " followed by ", want " later in the line
-	if _, after, ok := strings.Cut(line, ": got "); ok {
-		afterGot := after
-		return strings.Contains(afterGot, ", want ")
+	match := typeConflictPattern.FindStringSubmatch(line)
+	if match == nil {
+		return false
 	}
-	return false
+	return jsonTypeNames[match[1]] && jsonTypeNames[match[2]]
 }
 
 // stripAtPathPrefix removes "- at '/path': " or "at '/path': " prefixes from schema error lines

pkg/parser/schema_errors_test.go

@@ -48,12 +48,21 @@ func TestCleanOneOfMessage(t *testing.T) {
 			wantAny: []string{"deployments"},
 		},
 		{
-			name: "message unchanged when all sub-errors are type conflicts",
+			name: "all type conflicts synthesizes plain-English message",
 			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"},
+			// When all sub-errors are type conflicts, synthesize a plain-English message
+			wantNot: []string{"oneOf", "got string, want object"},
+			wantAny: []string{"expected object or array, got string"},
+		},
+		{
+			name: "engine type conflict produces actionable message",
+			input: "at '/engine': 'oneOf' failed, none matched\n" +
+				"- at '/engine': got number, want string\n" +
+				"- at '/engine': got number, want object",
+			wantNot: []string{"oneOf", "got number, want string"},
+			wantAny: []string{"expected string or object, got number", "Valid engine names", "copilot"},
 		},
 	}
 
@@ -81,6 +90,68 @@ func TestCleanOneOfMessage(t *testing.T) {
 	}
 }
 
+// TestSynthesizeOneOfTypeConflictMessage tests the fallback message synthesis for oneOf
+// errors where all sub-errors are type conflicts.
+func TestSynthesizeOneOfTypeConflictMessage(t *testing.T) {
+	tests := []struct {
+		name    string
+		lines   []string
+		want    string
+		wantAny []string // at least one of these must appear in output
+	}{
+		{
+			name: "engine field with number type produces actionable message",
+			lines: []string{
+				"at '/engine': 'oneOf' failed, none matched",
+				"- at '/engine': got number, want string",
+				"- at '/engine': got number, want object",
+			},
+			wantAny: []string{"expected string or object, got number", "Valid engine names", "copilot"},
+		},
+		{
+			name: "unknown field produces generic message without hints",
+			lines: []string{
+				"at '/foo': 'oneOf' failed, none matched",
+				"- at '/foo': got boolean, want string",
+				"- at '/foo': got boolean, want array",
+			},
+			want: "expected string or array, got boolean",
+		},
+		{
+			name: "bare got-want without path",
+			lines: []string{
+				"'oneOf' failed, none matched",
+				"got integer, want string",
+				"got integer, want object",
+			},
+			want: "expected string or object, got integer",
+		},
+		{
+			name: "no type conflicts returns fallback",
+			lines: []string{
+				"'oneOf' failed, none matched",
+			},
+			want: "schema validation failed",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := synthesizeOneOfTypeConflictMessage(tt.lines)
+
+			if tt.want != "" {
+				assert.Equal(t, tt.want, result,
+					"synthesizeOneOfTypeConflictMessage should produce expected message")
+			}
+
+			for _, wanted := range tt.wantAny {
+				assert.Contains(t, result, wanted,
+					"Result should contain %q\nResult: %s", wanted, result)
+			}
+		})
+	}
+}
+
 // TestIsTypeConflictLine tests detection of "got X, want Y" lines in oneOf errors
 func TestIsTypeConflictLine(t *testing.T) {
 	tests := []struct {
@@ -118,6 +189,26 @@ func TestIsTypeConflictLine(t *testing.T) {
 			line: "",
 			want: false,
 		},
+		{
+			name: "minItems constraint is not a type conflict",
+			line: "- at '/safe-outputs/add-labels/allowed': minItems: got 0, want 1",
+			want: false,
+		},
+		{
+			name: "minimum constraint is not a type conflict",
+			line: "- at '/tools/timeout': minimum: got 0, want 1",
+			want: false,
+		},
+		{
+			name: "maximum constraint is not a type conflict",
+			line: "maximum: got 120, want 60",
+			want: false,
+		},
+		{
+			name: "got number want integer is a type conflict",
+			line: "- at '/timeout-minutes': got number, want integer",
+			want: true,
+		},
 	}
 
 	for _, tt := range tests {

pkg/workflow/compiler_error_formatter.go

@@ -72,6 +72,33 @@ func formatCompilerErrorWithPosition(filePath string, line int, column int, errT
 	return &wrappedCompilerError{formatted: formattedErr, cause: cause}
 }
 
+// formatCompilerErrorWithContext creates a formatted compiler error with specific
+// line/column position and source-code context lines for Rust-style rendering.
+// Use this when source context is available; otherwise use formatCompilerErrorWithPosition.
+//
+// filePath: the file path to include in the error
+// line: the line number where the error occurred
+// column: the column number where the error occurred
+// errType: the error type ("error" or "warning")
+// message: the error message text
+// cause: optional underlying error to wrap (use nil for validation errors)
+// context: source code lines around the error for Rust-like snippet rendering
+func formatCompilerErrorWithContext(filePath string, line int, column int, errType string, message string, cause error, context []string) error {
+	compilerErrorLog.Printf("Formatting compiler error with context: file=%s, line=%d, column=%d, type=%s, context=%d lines", filePath, line, column, errType, len(context))
+	formattedErr := console.FormatError(console.CompilerError{
+		Position: console.ErrorPosition{
+			File:   filePath,
+			Line:   line,
+			Column: column,
+		},
+		Type:    errType,
+		Message: message,
+		Context: context,
+	})
+
+	return &wrappedCompilerError{formatted: formattedErr, cause: cause}
+}
+
 // formatCompilerMessage creates a formatted compiler message string (for warnings printed to stderr)
 // filePath: the file path to include in the message (typically markdownPath or lockFile)
 // msgType: the message type ("error" or "warning")

pkg/workflow/compiler_orchestrator_workflow.go

@@ -50,7 +50,9 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error)
 		// 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)
+			// 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, formatCompilerError(cleanPath, "error", err.Error(), err)
 	}

pkg/workflow/compiler_yaml_test.go

@@ -217,7 +217,7 @@ strict: false
 Invalid YAML with invalid number format.`,
 			expectedErrorLine:   3, // The timeout-minutes field is on line 3
 			expectedErrorColumn: 17,
-			expectedMessagePart: "got number, want integer", // Schema validation catches this
+			expectedMessagePart: "expected integer or string, got number", // Synthesized from oneOf type conflicts
 			description:         "invalid number format should trigger schema validation error",
 		},
 		{