Enforce jsonschema formats and structural error path lookup#56325
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #56325 does not have the implementation label and has 86 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The format enforcement is a good direction, but the new additional-properties path lookup still drops valid locations for anyOf/oneOf branches whenever the nested error tree is wrapped in kind.Reference nodes. That means the exact class of composite validation failures this patch is trying to improve will still regress back to the parent object location for schemas that factor branches through $ref.
Blocking theme
The recursive walker only descends through cause.Causes, and only recognizes leaf *kind.AdditionalProperties. In jsonschema/v6, composite failures frequently include kind.Reference wrappers around branch errors, so the walker never reaches the nested AdditionalProperties leaf in those cases. Please either unwrap *kind.Reference explicitly or add a regression test covering an anyOf/oneOf branch that comes from a $ref.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 5.97 AIC · ⌖ 6.95 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Review: Enforce jsonschema formats and structural error path lookup
The structural improvements here are solid — replacing regex fallback with proper Causes traversal is the right approach and the tests clearly demonstrate the behavior change.
Two blocking issues found:
-
compiler.AssertFormat()is a global breaking change (line 113,schema_compiler.go): Format enforcement is advisory by spec; enabling it globally may silently reject previously-valid YAML that passes schemas with format annotations. At minimum this needs a comment justifying the choice and confirming existing schemas were audited. -
Nil
ErrorKindpanic risk (line 134,json_path_locator.go):cause.ErrorKindcan be nil on a*jsonschema.ValidationError, but the type-assertion on the next line will panic. The nil guard should be extended to cover this case.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 23.3 AIC · ⌖ 9.21 AIC · ⊞ 6.2K
| @@ -112,6 +112,7 @@ func CompileSchema(schemaJSON, schemaURL string) (*jsonschema.Schema, error) { | |||
|
|
|||
| // Create a new compiler | |||
There was a problem hiding this comment.
Harden: AssertFormat() is now globally enforced — potentially breaking
compiler.AssertFormat() changes format keywords (e.g. date, email, uri) from advisory annotations to hard validation failures. Per the JSON Schema spec, format enforcement is opt-in, so adding this globally could silently break previously-valid workflow YAML files that use schemas with format annotations.
Consider adding a comment explaining the deliberate backward-compatibility trade-off, or audit existing schemas to confirm no valid values will now fail.
@copilot please address this.
| continue | ||
| } | ||
| if ap, ok := cause.ErrorKind.(*kind.AdditionalProperties); ok { | ||
| names = append(names, ap.Properties...) |
There was a problem hiding this comment.
Harden: nil ErrorKind not guarded in additionalPropertyNamesFromCauses
In additionalPropertyNamesFromCauses, there is a nil-pointer guard for cause == nil, but no guard for cause.ErrorKind == nil. A *jsonschema.ValidationError with a nil ErrorKind would panic at the type-assertion cause.ErrorKind.(*kind.AdditionalProperties) on the next line.
Suggested fix:
if cause == nil || cause.ErrorKind == nil {
continue
}@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Enforces JSON Schema formats and improves structural lookup of nested validation errors.
Changes:
- Enables format assertions in the shared schema compiler.
- Preserves and recursively inspects composite validation causes.
- Adds focused tests for both behaviors.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schema_compiler.go |
Enables format assertions. |
pkg/parser/schema_compiler_test.go |
Tests date-format enforcement. |
pkg/parser/json_path_locator.go |
Traverses nested validation causes. |
pkg/parser/json_path_locator_improvements_test.go |
Tests structural property discovery. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
||
| // Create a new compiler | ||
| compiler := jsonschema.NewCompiler() | ||
| compiler.AssertFormat() |
| if ap, ok := cause.ErrorKind.(*kind.AdditionalProperties); ok { | ||
| names = append(names, ap.Properties...) | ||
| continue | ||
| } | ||
| if len(cause.Causes) > 0 { | ||
| names = append(names, additionalPropertyNamesFromCauses(cause.Causes)...) |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on 3 targeted issues; overall the approach is solid.
📋 Key Themes & Highlights
Key Themes
- Behaviour contract gap:
additionalPropertyNamesForcomment still says it "falls back to regex on Message only when no structural error kind is available" — but for composite kinds with emptyCausesthe regex path is now permanently bypassed. The comment should be updated to reflect the new contract. - Test coverage gaps: the
nil-cause guard inadditionalPropertyNamesFromCauseshas no test; the format-assertion test only coversdateeven thoughuriis also called out in the PR description. - Coupling:
JSONPathInfo.Causesexposes the internal*jsonschema.ValidationErrorslice — worth considering whether this belongs in the struct or only in the helper.
Positive Highlights
- ✅ Replacing regex fallback with structured
Causestraversal is the right long-term direction - ✅
compiler.AssertFormat()is a minimal, well-placed single-line fix - ✅ Test renaming (
UsesRegexFallback...→UsesGroupCauses...,DoesNotRegexComposite...) properly reflects the new semantics - ✅ New tests use unparseable message strings to enforce that the structural path is taken — good use of negative evidence
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 36.5 AIC · ⌖ 10.3 AIC · ⊞ 7.6K
Comment /matt to run again
| // Composite errors may wrap an additional-properties leaf; regex fallback | ||
| // preserves the historical location behavior for these aggregate kinds. | ||
| return extractAdditionalPropertyNames(info.Message) | ||
| return additionalPropertyNamesFromCauses(info.Causes) |
There was a problem hiding this comment.
[/diagnosing-bugs] Silent regression: when a composite error (Group/OneOf/etc.) has a non-empty Message but empty Causes, additionalPropertyNamesFromCauses returns nil where the old regex fallback would have returned property names.
💡 Details
Before this change, extractAdditionalPropertyNames(info.Message) was called for all composite kinds regardless of Causes. After this change, empty Causes silently returns nil — the location degrades to parent-path level with no diagnostic.
TestLocateJSONPathForPathInfoDoesNotRegexCompositeErrorKind documents this as intentional, but the function comment on additionalPropertyNamesFor still says "falls back to regex on Message / only when no structural error kind is available" — the comment now contradicts the code for composite kinds with parseable messages.
Consider adding a note in the function doc that composite-kind errors with empty Causes return nil by design, so future readers don't reinstate the regex fallback under the assumption it was an oversight.
@copilot please address this.
| return extractAdditionalPropertyNames(info.Message) | ||
| } | ||
|
|
||
| func additionalPropertyNamesFromCauses(causes []*jsonschema.ValidationError) []string { |
There was a problem hiding this comment.
[/tdd] The new additionalPropertyNamesFromCauses function has no test for a nil cause element in the slice — despite the explicit nil guard in the code (if cause == nil { continue }).
💡 Suggested test
func TestAdditionalPropertyNamesFromCausesHandlesNilCause(t *testing.T) {
info := JSONPathInfo{
Path: "/on",
Message: "irrelevant",
ErrorKind: &kind.Group{},
Causes: []*jsonschema.ValidationError{
nil,
{ErrorKind: &kind.AdditionalProperties{Properties: []string{"foobar"}}},
},
}
location := LocateJSONPathForPathInfo(yamlContent, info)
// expect foobar to be found, not a panic
...
}The guard is good, but without a test the intention is invisible and the guard could be removed in a cleanup pass.
@copilot please address this.
| t.Fatalf("Validate(valid date) returned error: %v", err) | ||
| } | ||
|
|
||
| if err := schema.Validate(map[string]any{"expires": "not-a-date"}); err == nil { |
There was a problem hiding this comment.
[/tdd] TestCompileSchemaAssertsFormat only tests format: "date" — it doesn't cover uri, which is the other format explicitly called out in the PR description as previously broken.
💡 Suggestion
Add a parallel sub-test (or a table-driven case) for format: "uri" so the fix is confirmed for the full set of formats mentioned in the PR body:
{"type": "string", "format": "uri"}
// valid: "(example.com/redacted)"
// invalid: "not a uri"This mirrors the PR description's own claim: "This makes declared formats such as date and uri validation errors."
@copilot please address this.
| Location []string // Instance location from jsonschema (e.g., ["tools", "1"]) | ||
| ErrorKind jsonschema.ErrorKind // Structural error kind; nil when built from a string-only context | ||
| Path string // JSON path like "/tools/1" or "/age" | ||
| Message string // Error message |
There was a problem hiding this comment.
[/codebase-design] JSONPathInfo.Causes leaks an internal jsonschema type ([]*jsonschema.ValidationError) into a struct that is otherwise expressed in domain terms (Path, Message, Location, ErrorKind).
💡 Suggestion
If JSONPathInfo is ever serialised, logged, or passed across package boundaries, callers will transitively depend on the jsonschema package. Consider whether additionalPropertyNamesFromCauses should live as a closure or helper that accepts the raw *jsonschema.ValidationError directly (bypassing JSONPathInfo) — keeping the struct focused on describing a single error rather than carrying a full cause tree.
This is a minor coupling concern, not a blocker, but worth calling out given JSONPathInfo is already used in several other places in the package.
@copilot please address this.
formatconstraints in gh-aw schemas were not consistently asserted during schema validation, allowing invaliddateandurivalues to pass in some validation paths. Composite jsonschema errors also still depended on message parsing to locate nestedadditionalPropertiesfailures.AssertFormat()in the shared jsonschema compiler used by parser and workflow schema validation.dateandurivalidation errors instead of annotations.Validation error location
ValidationError.CausesthroughJSONPathInfo.oneOf,anyOf,allOf,Group) forkind.AdditionalProperties.Coverage
additionalPropertiesdiscovery inside composite validation errors.