-
Notifications
You must be signed in to change notification settings - Fork 295
fix: "Did you mean?" suggestions for nested enum violations (e.g., permission level typos) #19925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
61749f7
Initial plan
Copilot 57d98fc
fix: improve Did you mean? suggestions for nested enum constraint vio…
Copilot 06bf238
fix: address code review - improve test naming and remove redundant c…
Copilot 1afc55f
fix: address review comments on YAML path extraction correctness
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,13 @@ func generateSchemaBasedSuggestions(schemaJSON, errorMessage, jsonPath, frontmat | |
| if strings.Contains(strings.ToLower(errorMessage), "value must be one of") { | ||
| schemaSuggestionsLog.Print("Detected enum constraint violation") | ||
| enumValues := extractEnumValuesFromError(errorMessage) | ||
| userValue := extractYAMLValueAtPath(frontmatterContent, jsonPath) | ||
| // For oneOf errors, the path points to the container (e.g., "/permissions") but | ||
| // the enum constraint is on a nested field (e.g., "/permissions/contents"). | ||
| // Try to extract the actual sub-path from the message. | ||
| actualPath := extractEnumConstraintPath(errorMessage, jsonPath) | ||
| userValue := extractYAMLValueAtPath(frontmatterContent, actualPath) | ||
| if userValue != "" && len(enumValues) > 0 { | ||
| closest := FindClosestMatches(userValue, enumValues, maxClosestMatches) | ||
| closest := sliceutil.Deduplicate(FindClosestMatches(userValue, enumValues, maxClosestMatches)) | ||
| if len(closest) == 1 { | ||
| return fmt.Sprintf("Did you mean '%s'?", closest[0]) | ||
| } else if len(closest) > 1 { | ||
|
|
@@ -468,38 +472,129 @@ func extractEnumValuesFromError(errorMessage string) []string { | |
| return values | ||
| } | ||
|
|
||
| // extractYAMLValueAtPath extracts the scalar value at a simple top-level JSON path | ||
| // (e.g., "/engine") from raw YAML frontmatter content. | ||
| // Only top-level paths are supported; nested paths return an empty string. | ||
| // extractYAMLValueAtPath extracts the scalar value at a JSON path from raw YAML frontmatter. | ||
| // Supports top-level paths ("/field") and two-level nested paths ("/parent/child"). | ||
| // Deeper paths return an empty string. | ||
| func extractYAMLValueAtPath(yamlContent, jsonPath string) string { | ||
| if yamlContent == "" || jsonPath == "" { | ||
| return "" | ||
| } | ||
| // Only handle simple top-level paths like "/engine" (one slash, one segment) | ||
| if strings.Count(jsonPath, "/") != 1 { | ||
| segments := strings.SplitN(strings.TrimPrefix(jsonPath, "/"), "/", 3) | ||
| switch len(segments) { | ||
| case 1: | ||
| return extractTopLevelYAMLValue(yamlContent, segments[0]) | ||
| case 2: | ||
| return extractNestedYAMLValue(yamlContent, segments[0], segments[1]) | ||
| default: | ||
| return "" | ||
| } | ||
| fieldName := strings.TrimPrefix(jsonPath, "/") | ||
| } | ||
|
|
||
| // extractTopLevelYAMLValue extracts the scalar value of a top-level key from raw YAML. | ||
| // Uses horizontal-only whitespace between the colon and value to avoid matching multi-line blocks. | ||
| // Only keys at column 0 (no indentation) are matched, preventing false matches against | ||
| // nested keys with the same name. | ||
| func extractTopLevelYAMLValue(yamlContent, fieldName string) string { | ||
| escapedField := regexp.QuoteMeta(fieldName) | ||
|
|
||
| // Try single-quoted value: field: 'value' | ||
| reSingle := regexp.MustCompile(`(?m)^\s*` + escapedField + `\s*:\s*'([^'\n]+)'`) | ||
| // Try single-quoted value: field: 'value' (anchored to column 0, no leading whitespace) | ||
| reSingle := regexp.MustCompile(`(?m)^` + escapedField + `[ \t]*:[ \t]*'([^'\n]+)'`) | ||
| if match := reSingle.FindStringSubmatch(yamlContent); len(match) >= 2 { | ||
| return strings.TrimSpace(match[1]) | ||
| } | ||
| // Try double-quoted value: field: "value" | ||
| reDouble := regexp.MustCompile(`(?m)^\s*` + escapedField + `\s*:\s*"([^"\n]+)"`) | ||
| reDouble := regexp.MustCompile(`(?m)^` + escapedField + `[ \t]*:[ \t]*"([^"\n]+)"`) | ||
| if match := reDouble.FindStringSubmatch(yamlContent); len(match) >= 2 { | ||
| return strings.TrimSpace(match[1]) | ||
| } | ||
| // Try unquoted value: field: value | ||
| reUnquoted := regexp.MustCompile(`(?m)^\s*` + escapedField + `\s*:\s*([^'"\n#][^\n#]*?)(?:\s*#.*)?$`) | ||
| reUnquoted := regexp.MustCompile(`(?m)^` + escapedField + `[ \t]*:[ \t]*([^'"\n#][^\n#]*?)(?:[ \t]*#.*)?$`) | ||
| if match := reUnquoted.FindStringSubmatch(yamlContent); len(match) >= 2 { | ||
|
Comment on lines
+493
to
512
|
||
| return strings.TrimSpace(match[1]) | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| // extractNestedYAMLValue extracts the scalar value of a direct child key under a parent key in raw YAML. | ||
| // It finds the parent key's block (by indentation), determines the direct-child indent level from | ||
| // the first non-blank line inside the block, and only matches keys at that exact indent level. | ||
| // This prevents false matches against grandchildren that share the same key name. | ||
| func extractNestedYAMLValue(yamlContent, parentKey, childKey string) string { | ||
| lines := strings.Split(yamlContent, "\n") | ||
|
|
||
| escapedParent := regexp.QuoteMeta(parentKey) | ||
| parentPattern := regexp.MustCompile(`^(\s*)` + escapedParent + `[ \t]*:`) | ||
| escapedChild := regexp.QuoteMeta(childKey) | ||
|
|
||
| parentIndent := -1 | ||
| childIndent := -1 // indent of direct children (set on first non-blank line inside the block) | ||
| inParentBlock := false | ||
|
|
||
| for _, line := range lines { | ||
| if !inParentBlock { | ||
| if match := parentPattern.FindStringSubmatch(line); match != nil { | ||
| parentIndent = len(match[1]) | ||
| inParentBlock = true | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Inside parent block: skip blank lines | ||
| if strings.TrimSpace(line) == "" { | ||
| continue | ||
| } | ||
| lineIndent := len(line) - len(strings.TrimLeft(line, " \t")) | ||
|
|
||
| // Left parent block if indentation returned to parent level or less | ||
| if lineIndent <= parentIndent { | ||
| break | ||
|
Comment on lines
+546
to
+550
|
||
| } | ||
|
|
||
| // Establish the direct-child indentation from the first non-blank child line | ||
| if childIndent == -1 { | ||
| childIndent = lineIndent | ||
| } | ||
|
|
||
| // Only match keys at the direct-child indent level (not grandchildren deeper) | ||
| if lineIndent != childIndent { | ||
| continue | ||
| } | ||
|
|
||
| // Try to match child key with its value (single-quoted, double-quoted, unquoted). | ||
| childPrefix := `^\s+` + escapedChild + `[ \t]*:[ \t]*` | ||
| reSingle := regexp.MustCompile(childPrefix + `'([^'\n]+)'`) | ||
| if match := reSingle.FindStringSubmatch(line); len(match) >= 2 { | ||
| return strings.TrimSpace(match[1]) | ||
| } | ||
| reDouble := regexp.MustCompile(childPrefix + `"([^"\n]+)"`) | ||
| if match := reDouble.FindStringSubmatch(line); len(match) >= 2 { | ||
| return strings.TrimSpace(match[1]) | ||
| } | ||
| reUnquoted := regexp.MustCompile(childPrefix + `([^'"\n#][^\n#]*?)(?:[ \t]*#.*)?$`) | ||
| if match := reUnquoted.FindStringSubmatch(line); len(match) >= 2 { | ||
| return strings.TrimSpace(match[1]) | ||
| } | ||
| } | ||
|
|
||
| return "" | ||
| } | ||
|
|
||
| // extractEnumConstraintPath finds the JSON path of an enum constraint violation in an error message. | ||
| // For simple errors like "value must be one of 'a', 'b'", it returns the provided fallbackPath. | ||
| // For oneOf errors that contain a nested sub-path such as: | ||
| // | ||
| // "- at '/permissions/contents': value must be one of 'read', 'write', 'none'" | ||
| // | ||
| // it extracts "/permissions/contents" as the actual constraint path. | ||
| var enumConstraintPathPattern = regexp.MustCompile(`at '(/[^']+)':\s*value must be one of`) | ||
|
|
||
| func extractEnumConstraintPath(errorMessage, fallbackPath string) string { | ||
| if match := enumConstraintPathPattern.FindStringSubmatch(errorMessage); len(match) >= 2 { | ||
| return match[1] | ||
| } | ||
| return fallbackPath | ||
| } | ||
|
|
||
| // collectSchemaPropertyPaths recursively collects all (fieldName, parentPath) pairs from a JSON schema document. | ||
| // It traverses properties, oneOf/anyOf/allOf, and items to build a complete picture of valid fields across the schema. | ||
| func collectSchemaPropertyPaths(schemaDoc any, currentPath string, depth int) []schemaFieldLocation { | ||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR’s main behavior change is improving enum "Did you mean?" for nested oneOf errors, but the tests currently only cover
extractEnumConstraintPathandextractYAMLValueAtPathseparately. Add an end-to-end test forgenerateSchemaBasedSuggestionswherejsonPathis the container (e.g./permissions), the error message contains a nested enum line (e.g.- at '/permissions/contents': value must be one of ...), and frontmatter has a typo (contents: raed), asserting that the returned suggestion includesDid you mean 'read'?.