Improve compiler error messages for YAML syntax errors and permission scope validation (#22581)

Author: Copilot

pkg/parser/schema_errors.go

@@ -217,30 +217,109 @@ var knownFieldValidValues = map[string]string{
 	"/permissions": "Valid permission scopes: actions, all, attestations, checks, contents, deployments, discussions, id-token, issues, metadata, models, organization-projects, packages, pages, pull-requests, repository-projects, security-events, statuses, vulnerability-alerts",
 }
 
-// appendKnownFieldValidValuesHint appends a "Valid values: …" hint to message when the
-// jsonPath matches a well-known field and the message is an unknown-property error.
+// knownFieldScopes maps well-known JSON schema paths to a slice of valid scope names.
+// This enables spell-check ("Did you mean?") suggestions for unknown-property errors.
+//
+// The permissions scope list mirrors permissions.oneOf[1].properties in main_workflow_schema.json.
+// Update both when the schema changes.
+var knownFieldScopes = map[string][]string{
+	"/permissions": {
+		"actions", "all", "attestations", "checks", "contents", "deployments",
+		"discussions", "id-token", "issues", "metadata", "models",
+		"organization-projects", "packages", "pages", "pull-requests",
+		"repository-projects", "security-events", "statuses", "vulnerability-alerts",
+	},
+}
+
+// knownFieldDocs maps well-known JSON schema paths to documentation URLs.
+var knownFieldDocs = map[string]string{
+	"/permissions": "https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token",
+}
+
+// unknownPropertyPattern extracts the property name(s) from a rewritten "Unknown property(ies):" message.
+var unknownPropertyPattern = regexp.MustCompile(`(?i)^Unknown propert(?:y|ies): (.+)$`)
+
+// appendKnownFieldValidValuesHint appends a "Valid values: …" hint, "Did you mean?" suggestions,
+// and a documentation link to message when the jsonPath matches a well-known field and the
+// message is an unknown-property error.
 // It returns the message unchanged for unknown paths or non-additional-properties messages.
 func appendKnownFieldValidValuesHint(message string, jsonPath string) string {
 	// Use truncated prefix "unknown propert" to match both singular ("Unknown property")
 	// and plural ("Unknown properties") forms produced by rewriteAdditionalPropertiesError.
 	if !strings.Contains(strings.ToLower(message), "unknown propert") {
 		return message
 	}
-	hint, ok := knownFieldValidValues[jsonPath]
-	if !ok {
-		// Check if the path is nested under a known parent (e.g. /permissions/contents)
-		for path, h := range knownFieldValidValues {
+
+	// Find the best matching known path: exact match first, then the longest matching parent.
+	hint, hintOK := knownFieldValidValues[jsonPath]
+	scopes := knownFieldScopes[jsonPath]
+	docsURL := knownFieldDocs[jsonPath]
+	if !hintOK {
+		// Select the longest matching parent path deterministically to avoid
+		// random map iteration order when multiple known paths share a common prefix.
+		bestPath := ""
+		bestLen := 0
+		for path := range knownFieldValidValues {
 			if strings.HasPrefix(jsonPath, path+"/") {
-				hint = h
-				ok = true
-				break
+				if l := len(path); l > bestLen {
+					bestLen = l
+					bestPath = path
+				}
 			}
 		}
+		if bestPath != "" {
+			hint = knownFieldValidValues[bestPath]
+			scopes = knownFieldScopes[bestPath]
+			docsURL = knownFieldDocs[bestPath]
+			hintOK = true
+		}
 	}
-	if !ok {
+	if !hintOK {
 		return message
 	}
-	return message + " (" + hint + ")"
+
+	result := message + " (" + hint + ")"
+
+	// Add "Did you mean?" suggestions when the unknown property name is close to a valid scope.
+	if len(scopes) > 0 {
+		// unknownPropertyPattern has exactly one capture group, so a successful match
+		// returns [fullMatch, captureGroup1], giving len(m) == 2.
+		if m := unknownPropertyPattern.FindStringSubmatch(message); len(m) == 2 {
+			unknownProps := strings.Split(m[1], ", ")
+			var allSuggestions []string
+			for _, prop := range unknownProps {
+				prop = strings.TrimSpace(prop)
+				if prop == "" {
+					continue
+				}
+				// maxClosestMatches is defined in schema_suggestions.go in the same package.
+				closest := FindClosestMatches(prop, scopes, maxClosestMatches)
+				allSuggestions = append(allSuggestions, closest...)
+			}
+			// Deduplicate suggestions
+			seen := make(map[string]bool)
+			var unique []string
+			for _, s := range allSuggestions {
+				if !seen[s] {
+					seen[s] = true
+					unique = append(unique, s)
+				}
+			}
+			if len(unique) == 1 {
+				result = fmt.Sprintf("%s. Did you mean '%s'?", result, unique[0])
+			} else if len(unique) > 1 {
+				result = fmt.Sprintf("%s. Did you mean: %s?", result, strings.Join(unique, ", "))
+			}
+		}
+	}
+
+	// Append documentation link on the same line to avoid breaking bullet-list formatting
+	// when this message is embedded in "Multiple schema validation failures:" output.
+	if docsURL != "" {
+		result = fmt.Sprintf("%s See: %s", result, docsURL)
+	}
+
+	return result
 }
 
 // rewriteAdditionalPropertiesError rewrites "additional properties not allowed" errors to be more user-friendly

pkg/parser/schema_errors_test.go

@@ -306,3 +306,75 @@ func TestRewriteAdditionalPropertiesErrorOrdering(t *testing.T) {
 		})
 	}
 }
+
+// TestAppendKnownFieldValidValuesHint tests that the hint function appends valid values,
+// "Did you mean?" suggestions, and documentation links for well-known schema paths.
+func TestAppendKnownFieldValidValuesHint(t *testing.T) {
+	tests := []struct {
+		name     string
+		message  string
+		jsonPath string
+		contains []string // substrings that must appear
+		excludes []string // substrings that must NOT appear
+	}{
+		{
+			name:     "no hint for non-unknown-property message",
+			message:  "value must be one of 'read', 'write', 'none'",
+			jsonPath: "/permissions",
+			contains: []string{"value must be one of"},
+			excludes: []string{"Valid permission scopes", "See:"},
+		},
+		{
+			name:     "hint appended for permissions path",
+			message:  "Unknown property: issuess",
+			jsonPath: "/permissions",
+			contains: []string{"Valid permission scopes", "See: https://docs.github.com"},
+		},
+		{
+			name:     "did you mean suggestion for close typo",
+			message:  "Unknown property: issuess",
+			jsonPath: "/permissions",
+			contains: []string{"Did you mean 'issues'?"},
+		},
+		{
+			name:     "did you mean suggestion for another typo",
+			message:  "Unknown property: contnets",
+			jsonPath: "/permissions",
+			contains: []string{"Did you mean 'contents'?"},
+		},
+		{
+			name:     "no did you mean for unrelated word",
+			message:  "Unknown property: completely-unknown-xyz",
+			jsonPath: "/permissions",
+			excludes: []string{"Did you mean"},
+			contains: []string{"Valid permission scopes"},
+		},
+		{
+			name:     "no hint for unknown path",
+			message:  "Unknown property: foo",
+			jsonPath: "/some-unknown-path",
+			contains: []string{"Unknown property: foo"},
+			excludes: []string{"Valid permission scopes", "See:"},
+		},
+		{
+			name:     "hint works for nested permissions path",
+			message:  "Unknown property: issuess",
+			jsonPath: "/permissions/issuess",
+			contains: []string{"Valid permission scopes", "See: https://docs.github.com"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := appendKnownFieldValidValuesHint(tt.message, tt.jsonPath)
+			for _, want := range tt.contains {
+				assert.Contains(t, result, want,
+					"appendKnownFieldValidValuesHint should contain %q\nResult: %s", want, result)
+			}
+			for _, exclude := range tt.excludes {
+				assert.NotContains(t, result, exclude,
+					"appendKnownFieldValidValuesHint should not contain %q\nResult: %s", exclude, result)
+			}
+		})
+	}
+}

pkg/parser/yaml_error.go

@@ -18,6 +18,117 @@ var (
 	sourceLinePattern    = regexp.MustCompile(`(?m)^(>?\s*)(\d+)(\s*\|)`)
 )
 
+// yamlErrorTranslations maps common goccy/go-yaml internal error messages to
+// user-friendly plain-language descriptions with actionable fix guidance.
+// Each pattern is matched case-insensitively against the parser message text only
+// (not the surrounding source-context lines in yaml.FormatError() output).
+//
+// These translations are the single source of truth shared between the parser
+// and workflow packages. See TranslateYAMLMessage for public access.
+var yamlErrorTranslations = []struct {
+	pattern     string
+	replacement string
+}{
+	{
+		"unexpected key name",
+		"missing ':' after key — YAML mapping entries require 'key: value' format",
+	},
+	{
+		"mapping value is not allowed in this context",
+		"unexpected ':' — check indentation or if this key belongs in a mapping block",
+	},
+	{
+		"mapping values are not allowed",
+		"unexpected ':' — check indentation or if this key belongs in a mapping block",
+	},
+	{
+		"string was used where mapping is expected",
+		"expected a YAML mapping (key: value pairs) but got a plain string",
+	},
+	{
+		"non-map value is specified",
+		"expected a YAML mapping (key: value pairs) — did you forget a colon after the key?",
+	},
+	{
+		"tab character cannot use as a map key directly",
+		"tab character in key — YAML requires spaces for indentation, not tabs",
+	},
+	{
+		"found character that cannot start any token",
+		"invalid character — check indentation uses spaces, not tabs",
+	},
+	{
+		"could not find expected ':'",
+		"missing ':' in key-value pair",
+	},
+	{
+		"did not find expected key",
+		"incorrect indentation or missing key in mapping",
+	},
+}
+
+// TranslateYAMLMessage translates a raw goccy/go-yaml parser message to a user-friendly
+// description. It is the public entry point used by both the parser and workflow packages
+// so that both code paths share a single translation table.
+//
+// The function performs a case-insensitive substring replacement of the first matching
+// pattern, leaving any surrounding text intact. This is safe for ASCII patterns because
+// strings.ToLower preserves byte positions exactly for ASCII characters.
+func TranslateYAMLMessage(message string) string {
+	lower := strings.ToLower(message)
+	for _, t := range yamlErrorTranslations {
+		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
+}
+
+// translateYAMLError translates cryptic goccy/go-yaml parser messages to user-friendly descriptions.
+// It operates on the full yaml.FormatError() output, which includes a header line and source context:
+//
+//	[line:col] original parser message
+//	>  1 | some: yaml
+//	       ^
+//
+// Only the parser message portion (the header line, after the "[line:col] " prefix) is translated.
+// Source-context lines are left untouched to avoid accidentally replacing text inside user YAML content.
+func translateYAMLError(formatted string) string {
+	if formatted == "" {
+		return formatted
+	}
+
+	// Split into the header line (which contains the parser message) and the rest (source context).
+	var header, rest string
+	if nl := strings.IndexByte(formatted, '\n'); nl >= 0 {
+		header = formatted[:nl]
+		rest = formatted[nl:]
+	} else {
+		header = formatted
+		rest = ""
+	}
+
+	// Within the header, locate the parser message text after the "[line:col] " prefix.
+	// If the prefix is absent (unusual), treat the entire header as the message.
+	msgStart := strings.Index(header, "] ")
+	var prefix, msg string
+	if msgStart >= 0 {
+		msgStart += len("] ")
+		prefix = header[:msgStart]
+		msg = header[msgStart:]
+	} else {
+		prefix = ""
+		msg = header
+	}
+
+	// Translate only the message portion, leaving prefix and source context intact.
+	translated := TranslateYAMLMessage(msg)
+
+	return prefix + translated + rest
+}
+
 // FormatYAMLError formats a YAML error with source code context using yaml.FormatError()
 // frontmatterLineOffset is the line number where the frontmatter content begins in the document (1-based)
 // Returns the formatted error string with line numbers adjusted for frontmatter position
@@ -28,6 +139,9 @@ func FormatYAMLError(err error, frontmatterLineOffset int, sourceYAML string) st
 	// colored=false to avoid ANSI escape codes, inclSource=true to include source lines
 	formatted := yaml.FormatError(err, false, true)
 
+	// Translate cryptic parser messages to user-friendly descriptions (header line only)
+	formatted = translateYAMLError(formatted)
+
 	// Adjust line numbers in the formatted output to account for frontmatter position
 	if frontmatterLineOffset > 1 {
 		formatted = adjustLineNumbersInFormattedError(formatted, frontmatterLineOffset-1)