Conversation
… scope validation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/83c0859d-e78d-49c2-839c-1c54589b16bf
Copilot
AI
changed the title
[WIP] Improve compiler error messages for YAML syntax errors and permission scope validation
Improve compiler error messages for YAML syntax errors and permission scope validation
Mar 24, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Improves end-user diagnostics by translating opaque YAML parser errors into actionable messages and enhancing /permissions schema validation errors with “Did you mean?” suggestions and documentation links.
Changes:
- Added a YAML parser error translation table and applied it during
FormatYAMLError()formatting. - Extended schema unknown-property hints for
/permissionswith closest-match suggestions and a docs URL. - Added unit tests covering YAML translation and schema hint behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/parser/yaml_error.go | Adds YAML error translation mapping and applies it in FormatYAMLError(). |
| pkg/parser/yaml_error_test.go | Adds tests for translation behavior and for FormatYAMLError() applying translations. |
| pkg/parser/schema_errors.go | Adds scope-aware “Did you mean?” suggestions and docs link for /permissions unknown-property errors. |
| pkg/parser/schema_errors_test.go | Adds tests verifying valid-values hints, suggestions, and docs links. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/parser/yaml_error.go
Outdated
Comment on lines
+58
to
+71
| func translateYAMLError(formatted string) string { | ||
| for _, t := range yamlErrorTranslations { | ||
| // All pattern strings are lowercase to match goccy/go-yaml's lowercase error messages. | ||
| // Case-insensitive search ensures robustness across library version changes. | ||
| lower := strings.ToLower(formatted) | ||
| if idx := strings.Index(lower, t.pattern); idx >= 0 { | ||
| // Slice using idx from the lowercase string. This is safe because: | ||
| // - All patterns are ASCII (single-byte characters). | ||
| // - strings.ToLower of ASCII preserves byte positions exactly. | ||
| yamlErrorLog.Printf("Translating YAML error pattern %q", t.pattern) | ||
| return formatted[:idx] + t.replacement + formatted[idx+len(t.pattern):] | ||
| } | ||
| } | ||
| return formatted |
There was a problem hiding this comment.
translateYAMLError currently scans and rewrites the entire formatted yaml.FormatError() output. Because that output includes the original source lines, this can accidentally replace text inside the user’s YAML content (e.g., if a YAML string contains "unexpected key name"), producing misleading context. Consider restricting translation to just the parser message portion (e.g., only the first line, or the substring after the leading "[line:col] " up to the first newline) and leaving the source context untouched.
Also note: existing tests assert raw goccy messages (e.g. pkg/workflow/compiler_yaml_test.go expects "unexpected key name"); applying this translation will change those outputs and will require updating those expectations.
Suggested change
| func translateYAMLError(formatted string) string { | |
| for _, t := range yamlErrorTranslations { | |
| // All pattern strings are lowercase to match goccy/go-yaml's lowercase error messages. | |
| // Case-insensitive search ensures robustness across library version changes. | |
| lower := strings.ToLower(formatted) | |
| if idx := strings.Index(lower, t.pattern); idx >= 0 { | |
| // Slice using idx from the lowercase string. This is safe because: | |
| // - All patterns are ASCII (single-byte characters). | |
| // - strings.ToLower of ASCII preserves byte positions exactly. | |
| yamlErrorLog.Printf("Translating YAML error pattern %q", t.pattern) | |
| return formatted[:idx] + t.replacement + formatted[idx+len(t.pattern):] | |
| } | |
| } | |
| return formatted | |
| // | |
| // IMPORTANT: Only the parser message portion of yaml.FormatError() output is translated. | |
| // The formatted error typically looks like: | |
| // [line:col] original parser message | |
| // > 1 | some: yaml | |
| // ^ | |
| // We restrict replacements to the first line (and specifically the substring after "[line:col] ") | |
| // to avoid accidentally rewriting user YAML source shown in the context block. | |
| func translateYAMLError(formatted string) string { | |
| if formatted == "" { | |
| return formatted | |
| } | |
| // Split into first line (header) and the rest (which includes 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, try to locate the parser message after the "[line:col] " prefix. | |
| msgStart := strings.Index(header, "] ") | |
| var prefix, msg string | |
| if msgStart >= 0 { | |
| msgStart += len("] ") | |
| prefix = header[:msgStart] | |
| msg = header[msgStart:] | |
| } else { | |
| // Fallback: treat the entire header as the message if no "[line:col] " prefix is found. | |
| prefix = "" | |
| msg = header | |
| } | |
| translated := msg | |
| lower := strings.ToLower(msg) | |
| for _, t := range yamlErrorTranslations { | |
| // All pattern strings are lowercase to match goccy/go-yaml's lowercase error messages. | |
| // Case-insensitive search ensures robustness across library version changes. | |
| if idx := strings.Index(lower, t.pattern); idx >= 0 { | |
| // Slice using idx from the lowercase string. This is safe because: | |
| // - All patterns are ASCII (single-byte characters). | |
| // - strings.ToLower of ASCII preserves byte positions exactly. | |
| yamlErrorLog.Printf("Translating YAML error pattern %q", t.pattern) | |
| translated = msg[:idx] + t.replacement + msg[idx+len(t.pattern):] | |
| break | |
| } | |
| } | |
| // Reassemble: translated header + untouched rest (source context, pointers, etc.). | |
| return prefix + translated + rest |
pkg/parser/schema_errors.go
Outdated
|
|
||
| // Append documentation link. | ||
| if docsURL != "" { | ||
| result = fmt.Sprintf("%s\nSee: %s", result, docsURL) |
There was a problem hiding this comment.
appendKnownFieldValidValuesHint appends the docs URL using a literal newline ("\nSee: …"). This function is used inside formatSchemaFailureDetail(), and those detail strings are later joined into a bullet list ("Multiple schema validation failures:\n- ...\n- ..."). Embedding newlines here will break that formatting (the "See:" line won’t be prefixed with "- ", and the rendered compiler error may become hard to read). Consider keeping the docs URL on the same line (e.g., " See: …" or " (See: …)") or only adding the newline in contexts where multi-line messages are explicitly supported.
Suggested change
| result = fmt.Sprintf("%s\nSee: %s", result, docsURL) | |
| result = fmt.Sprintf("%s See: %s", result, docsURL) |
Comment on lines
+257
to
266
| if !hintOK { | ||
| for path := range knownFieldValidValues { | ||
| if strings.HasPrefix(jsonPath, path+"/") { | ||
| hint = h | ||
| ok = true | ||
| hint = knownFieldValidValues[path] | ||
| scopes = knownFieldScopes[path] | ||
| docsURL = knownFieldDocs[path] | ||
| hintOK = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
The parent-path lookup iterates over knownFieldValidValues (a map) and breaks on the first prefix match. Because Go map iteration order is randomized, if multiple known paths are added in the future with overlapping prefixes, this could select different parents nondeterministically. Consider selecting the longest matching prefix deterministically (e.g., iterate over keys sorted by descending length, or track the best match).
See below for a potential fix:
// Select the longest matching parent path deterministically.
bestPath := ""
bestLen := 0
for path := range knownFieldValidValues {
if strings.HasPrefix(jsonPath, path+"/") {
if l := len(path); l > bestLen {
bestLen = l
bestPath = path
}
}
}
if bestPath != "" {
hint = knownFieldValidValues[bestPath]
scopes = knownFieldScopes[bestPath]
docsURL = knownFieldDocs[bestPath]
hintOK = true
}
pkg/parser/yaml_error.go
Outdated
Comment on lines
+59
to
+62
| for _, t := range yamlErrorTranslations { | ||
| // All pattern strings are lowercase to match goccy/go-yaml's lowercase error messages. | ||
| // Case-insensitive search ensures robustness across library version changes. | ||
| lower := strings.ToLower(formatted) |
There was a problem hiding this comment.
translateYAMLError recalculates strings.ToLower(formatted) on every iteration of yamlErrorTranslations. Since the input doesn’t change inside the loop, compute it once before the loop to avoid repeated allocations/work (especially since FormatYAMLError may be called frequently).
Suggested change
| for _, t := range yamlErrorTranslations { | |
| // All pattern strings are lowercase to match goccy/go-yaml's lowercase error messages. | |
| // Case-insensitive search ensures robustness across library version changes. | |
| lower := strings.ToLower(formatted) | |
| // Precompute lowercase version once to avoid repeated allocations/work in the loop. | |
| lower := strings.ToLower(formatted) | |
| for _, t := range yamlErrorTranslations { | |
| // All pattern strings are lowercase to match goccy/go-yaml's lowercase error messages. | |
| // Case-insensitive search ensures robustness across library version changes. |
Comment on lines
+21
to
+52
| // 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 formatted error string. | ||
| 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", | ||
| }, | ||
| { | ||
| "string was used where mapping is expected", | ||
| "expected a YAML mapping (key: value pairs) but got a plain string", | ||
| }, | ||
| { | ||
| "tab character cannot use as a map key directly", | ||
| "tab character in key — YAML requires spaces for indentation, not tabs", | ||
| }, | ||
| { | ||
| "could not find expected ':'", | ||
| "missing ':' in key-value pair", | ||
| }, | ||
| { | ||
| "did not find expected key", | ||
| "incorrect indentation or missing key in mapping", | ||
| }, | ||
| } |
There was a problem hiding this comment.
This file introduces a second YAML error translation table/function that overlaps with pkg/workflow/frontmatter_error.go (which already has yamlErrorTranslations + translateYAMLMessage for goccy/go-yaml messages). Keeping two separate translation tables risks inconsistent user-facing messages and duplicated maintenance when goccy changes wording. Consider consolidating translations in one place (or sharing a common helper) so both frontmatter and parser formatting paths stay in sync.
Collaborator
|
@copilot apply comments |
…ocs link, deterministic path lookup Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6fc9b7cb-3dc7-4dda-a277-da4f996f33e4
Contributor
Author
Applied all reviewer comments in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Raw goccy/go-yaml parser messages ("unexpected key name") and permission scope errors lacked actionable guidance. Error quality scored 41/100 for YAML syntax and 68/100 for permission scopes.
YAML error translation (
pkg/parser/yaml_error.go)Added a unified
yamlErrorTranslationstable (consolidating the previously separate table inpkg/workflow/frontmatter_error.go) and exportedTranslateYAMLMessage()as the shared translation function used by both the parser and workflow packages. Translation is scoped to the parser message header only — source context lines shown byyaml.FormatError()are never modified.unexpected key namemissing ':' after key — YAML mapping entries require 'key: value' formatmapping value is not allowed in this contextunexpected ':' — check indentation or if this key belongs in a mapping blockstring was used where mapping is expectedexpected a YAML mapping (key: value pairs) but got a plain stringtab character cannot use as a map key directlytab character in key — YAML requires spaces for indentation, not tabsnon-map value is specifiedexpected a YAML mapping (key: value pairs) — did you forget a colon after the key?found character that cannot start any tokeninvalid character — check indentation uses spaces, not tabsThe duplicate
yamlErrorTranslations/translateYAMLMessageinpkg/workflow/frontmatter_error.gohas been removed; that code path now callsparser.TranslateYAMLMessage().Permission scope error improvements (
pkg/parser/schema_errors.go)Extended
appendKnownFieldValidValuesHintfor/permissionserrors:FindClosestMatches()againstknownFieldScopesSee: https://docs.github.com/...on the same line (avoids breaking multi-error bullet-list formatting)Example output for
issuess: read:✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.