fix(parser): report all safe-output schema failures with per-field line/column locations#17524
fix(parser): report all safe-output schema failures with per-field line/column locations#17524
Conversation
…res with additional tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main, fix tests in https://github.com/github/gh-aw/actions/runs/22260319728/job/64397617808#step:10:1 |
There was a problem hiding this comment.
Pull request overview
This pull request improves schema validation error reporting for safe-outputs frontmatter by ensuring all validation failures are reported with their respective line and column locations, rather than only reporting the first failure.
Changes:
- Modified
validateWithSchemaAndLocationto aggregate all schema failures and include per-field location details - Added
formatSchemaFailureDetailhelper function to format individual failure details with path and location - Added comprehensive test coverage for multiple failures, empty path normalization, and single vs. multiple failure formatting
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/parser/schema_compiler.go | Implements aggregated error reporting with location details for all failures; adds formatSchemaFailureDetail helper |
| pkg/parser/safe_outputs_error_location_test.go | Adds four new tests covering multiple failures, empty path handling, line/column formatting, and single failure behavior |
Comments suppressed due to low confidence (2)
pkg/parser/schema_compiler.go:307
- When the primary error path's location cannot be found in the YAML (location.Found is false), the code falls through to the fallback case and discards all the detail lines that were just created for multiple errors. This means that if we have multiple schema failures but the first one's location cannot be determined, we fall back to showing only a single error message instead of reporting all failures as intended.
Consider either: (1) using the detail lines even when location.Found is false, with a default position, or (2) iterating through jsonPaths to find the first one with a locatable position to use as the primary path.
if location.Found {
// Adjust line number to account for frontmatter position in file
adjustedLine := location.Line + frontmatterStart - 1
// Create context lines around the adjusted line number in the full file
var adjustedContextLines []string
if filePath != "" {
// Use the same sanitized path
if content, readErr := os.ReadFile(cleanPath); readErr == nil {
allLines := strings.Split(string(content), "\n")
// Create context around the adjusted line (±3 lines)
// The console formatter expects context to be centered around the error line
contextSize := 7 // ±3 lines around the error
contextStart := max(0, adjustedLine-contextSize/2-1) // -1 for 0-based indexing
contextEnd := min(len(allLines), contextStart+contextSize)
for i := contextStart; i < contextEnd; i++ {
adjustedContextLines = append(adjustedContextLines, allLines[i])
}
}
}
// If we couldn't create adjusted context, fall back to frontmatter context
if len(adjustedContextLines) == 0 {
adjustedContextLines = contextLines
}
// Include every schema failure with path + line + column.
message := ""
if len(detailLines) == 1 {
message = detailLines[0]
} else {
message = "Multiple schema validation failures:\n- " + strings.Join(detailLines, "\n- ")
}
// Create a compiler error with precise location information
compilerErr := console.CompilerError{
Position: console.ErrorPosition{
File: filePath,
Line: adjustedLine,
Column: location.Column, // Use original column, we'll extend to word in console rendering
},
Type: "error",
Message: message,
Context: adjustedContextLines,
// Hints removed as per requirements
}
// Format and return the error
formattedErr := console.FormatError(compilerErr)
return errors.New(formattedErr)
}
pkg/parser/safe_outputs_error_location_test.go:440
- The word "normalised" should be changed to "normalized" to match the American English spelling convention used consistently throughout the rest of the codebase.
// TestFormatSchemaFailureDetailEmptyPath verifies that an empty path is normalised to "/".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…aFailureDetail Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 7971531. Merged main and fixed |
pkg/parser/schema_compiler.goTestValidateWithSchemaAndLocationReportsAllSafeOutputFailures(from PR fix(parser): include per-field locations for safe-output schema failures #17484)TestFormatSchemaFailureDetailEmptyPath,TestFormatSchemaFailureDetailLineColumn,TestValidateWithSchemaAndLocationSingleFailureNoBulletPrefixschemaJSONtoformatSchemaFailureDetailand restoregenerateSchemaBasedSuggestionscall soTestStrictModeDeprecatedFieldErrorMessagegets "Did you mean 'timeout-minutes'?" suggestion backOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.