From fa870ab7975dee355d28112a4c495b2fe282c4ad Mon Sep 17 00:00:00 2001 From: Brian Jin <35789537+kikashy@users.noreply.github.com> Date: Fri, 24 Jul 2026 19:47:00 -0400 Subject: [PATCH] fix: make validator diagnostics self-sufficient (backlog batch) Enrich the diagnostics an author acts on so each names its own fix without opening the schema, and make the condition oneOf branch selection report consistently. No conformance outcome changes (all 47 cases pass) and no machine-contract change: messages and provisional codes only. - ENUM lists the allowed values. - LOCAL-ID and the sibling pattern codes (pack-version, fact-path, decimal-operand) name the offending value and give a concrete example instead of dumping the grammar regex. - COLLECTION-ARITY, and its condition and escalation variants, states the required minimum and the actual count. - CARRIER-INVALID-JSON reports the line, column, and byte offset and preserves the decoder's own (sanitized) reason, instead of a vague "incomplete object". - A bad or ambiguous condition op now reports one consistent JPS-STRUCTURE-CONDITION-SHAPE at the condition, listing the valid ops, rather than three different codes and locations; and a valid op is no longer spuriously flagged. A losing branch's bare op-discriminator failure is now rejected like the grouped case, completing the oneOf fix from the seam work and clearing the empty-object condition firehose. Surfaced by the agent-testing protocol and confirmed by three independent adversarial sweeps: no document is wrongly accepted or rejected. Each change has a focused test; the condition-diagnostic guard test and the nested-operand test still pass. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Brian Jin <35789537+kikashy@users.noreply.github.com> --- internal/carrier/decode.go | 50 ++++++++++-- internal/carrier/decode_test.go | 21 +++++- internal/validation/validator.go | 82 +++++++++++++++++--- internal/validation/validator_test.go | 105 ++++++++++++++++++++++++++ 4 files changed, 240 insertions(+), 18 deletions(-) diff --git a/internal/carrier/decode.go b/internal/carrier/decode.go index 84a7c44..85aa043 100644 --- a/internal/carrier/decode.go +++ b/internal/carrier/decode.go @@ -8,6 +8,7 @@ import ( "io" "unicode/utf8" + "github.com/Judgment-Pack/judgment-pack-runtime/internal/display" "github.com/Judgment-Pack/judgment-pack-runtime/internal/result" ) @@ -45,6 +46,7 @@ type parser struct { decoder *json.Decoder limits Limits nodes int + data []byte } func Decode(data []byte, limits Limits) (any, *Failure) { @@ -53,7 +55,7 @@ func Decode(data []byte, limits Limits) (any, *Failure) { } decoder := json.NewDecoder(bytes.NewReader(data)) decoder.UseNumber() - p := parser{decoder: decoder, limits: limits} + p := parser{decoder: decoder, limits: limits, data: data} value, failure := p.value(nil, 0) if failure != nil { return nil, failure @@ -71,7 +73,7 @@ func (p *parser) value(location []string, depth int) (any, *Failure) { p.nodes++ token, err := p.decoder.Token() if err != nil { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input is not valid complete JSON.") + return nil, p.invalidJSON(Pointer(location), "", err) } switch typed := token.(type) { case json.Delim: @@ -84,7 +86,7 @@ func (p *parser) value(location []string, depth int) (any, *Failure) { case '[': return p.array(location, depth+1) default: - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an unexpected JSON delimiter.") + return nil, p.invalidJSON(Pointer(location), "unexpected JSON delimiter", nil) } case string: if len(typed) > p.limits.MaxStringBytes { @@ -99,7 +101,7 @@ func (p *parser) value(location []string, depth int) (any, *Failure) { case bool, nil: return typed, nil default: - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains a value outside the JSON data model.") + return nil, p.invalidJSON(Pointer(location), "value outside the JSON data model", nil) } } @@ -108,11 +110,11 @@ func (p *parser) object(location []string, depth int) (map[string]any, *Failure) for p.decoder.More() { token, err := p.decoder.Token() if err != nil { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an incomplete JSON object.") + return nil, p.invalidJSON(Pointer(location), "", err) } key, ok := token.(string) if !ok { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an invalid JSON object member.") + return nil, p.invalidJSON(Pointer(location), "object member name is not a string", nil) } memberLocation := appendLocation(location, key) if len(key) > p.limits.MaxStringBytes { @@ -128,7 +130,7 @@ func (p *parser) object(location []string, depth int) (map[string]any, *Failure) value[key] = item } if token, err := p.decoder.Token(); err != nil || token != json.Delim('}') { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an incomplete JSON object.") + return nil, p.invalidJSON(Pointer(location), "expected the end of the object", err) } return value, nil } @@ -144,7 +146,7 @@ func (p *parser) array(location []string, depth int) ([]any, *Failure) { value = append(value, item) } if token, err := p.decoder.Token(); err != nil || token != json.Delim(']') { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an incomplete JSON array.") + return nil, p.invalidJSON(Pointer(location), "expected the end of the array", err) } return value, nil } @@ -182,3 +184,35 @@ func invalid(code, location, message string) *Failure { func resource(code, location, message string) *Failure { return &Failure{Resource: true, Diagnostic: result.ErrorDiagnostic(code, "operation", location, message)} } + +// invalidJSON reports a carrier parse failure with its line, column, and byte +// offset, preserving (and sanitizing) the decoder's own reason when one is +// available so an author can find and fix the exact spot. +func (p *parser) invalidJSON(location, detail string, err error) *Failure { + offset := p.decoder.InputOffset() + line, column := lineColumn(p.data, offset) + message := fmt.Sprintf("Input is not valid JSON at line %d, column %d (byte offset %d)", line, column, offset) + if err != nil { + detail = display.Sanitize(err.Error()) + } + if detail != "" { + message += ": " + detail + } + return invalid("JPS-CARRIER-INVALID-JSON", location, message+".") +} + +func lineColumn(data []byte, offset int64) (int, int) { + if offset > int64(len(data)) { + offset = int64(len(data)) + } + line, column := 1, 1 + for index := int64(0); index < offset; index++ { + if data[index] == '\n' { + line++ + column = 1 + } else { + column++ + } + } + return line, column +} diff --git a/internal/carrier/decode_test.go b/internal/carrier/decode_test.go index 5a21a65..3efd192 100644 --- a/internal/carrier/decode_test.go +++ b/internal/carrier/decode_test.go @@ -1,6 +1,9 @@ package carrier -import "testing" +import ( + "strings" + "testing" +) func TestDuplicateMemberReportsNestedPointer(t *testing.T) { _, failure := Decode([]byte(`{"outer":{"a":1,"\u0061":2}}`), DefaultLimits()) @@ -24,3 +27,19 @@ func TestDepthLimitIsOperational(t *testing.T) { t.Fatalf("unexpected failure: %#v", failure) } } + +func TestMissingCommaReportsLocationAndCause(t *testing.T) { + // A missing comma between members is a parse error; the diagnostic should + // carry a line, column, and byte offset rather than a vague "incomplete + // object" message. + _, failure := Decode([]byte("{\n \"a\": 1\n \"b\": 2\n}"), DefaultLimits()) + if failure == nil || failure.Resource || failure.Diagnostic.Code != "JPS-CARRIER-INVALID-JSON" { + t.Fatalf("expected a carrier invalid-JSON failure: %#v", failure) + } + message := failure.Diagnostic.Message + for _, want := range []string{"line", "column", "byte offset"} { + if !strings.Contains(message, want) { + t.Fatalf("carrier message should include %q: %q", want, message) + } + } +} diff --git a/internal/validation/validator.go b/internal/validation/validator.go index 7bb9898..7970921 100644 --- a/internal/validation/validator.go +++ b/internal/validation/validator.go @@ -332,7 +332,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics handled = true code := patternCode(typed.Want) if code != "JPS-STRUCTURE-EXTENSION-NAME" { - add(code, location, structuralMessage(code)) + add(code, location, patternMessage(code, typed)) } case *kind.MinItems: handled = true @@ -342,7 +342,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics } else if last(location) == "triggers" { code = "JPS-STRUCTURE-ESCALATION-TRIGGERS" } - add(code, location, structuralMessage(code)) + add(code, location, arityMessage(code, typed)) case *kind.Type: handled = true switch { @@ -360,7 +360,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics } case *kind.Enum: handled = true - add("JPS-STRUCTURE-ENUM", location, "Value is not one of the allowed values.") + add("JPS-STRUCTURE-ENUM", location, enumMessage(typed)) case *kind.MinLength: handled = true add("JPS-STRUCTURE-MIN-LENGTH", location, "String is shorter than the required minimum length.") @@ -377,7 +377,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics relevantCauses = append(relevantCauses, cause) } if len(relevantCauses) == 0 { - add("JPS-STRUCTURE-CONDITION-SHAPE", location, "Condition does not match one declared condition shape.") + add("JPS-STRUCTURE-CONDITION-SHAPE", location, "Condition does not match one declared shape; its op must be one of: literal, all, any, not, fact, evidence-present.") return } for _, cause := range relevantCauses { @@ -444,6 +444,63 @@ func structuralMessage(code string) string { } } +// enumMessage lists the allowed values so an author does not have to open the +// schema to learn them. +func enumMessage(typed *kind.Enum) string { + allowed := make([]string, 0, len(typed.Want)) + for _, value := range typed.Want { + allowed = append(allowed, stringifyValue(value)) + } + if len(allowed) == 0 { + return "Value is not one of the allowed values." + } + return "Value is not one of the allowed values: " + joinCapped(allowed, 12) + "." +} + +func stringifyValue(value any) string { + if text, ok := value.(string); ok { + return text + } + return fmt.Sprint(value) +} + +// patternMessage names the offending value and gives a concrete example rather +// than dumping the grammar regex, for each pattern-constrained member. +func patternMessage(code string, typed *kind.Pattern) string { + switch code { + case "JPS-STRUCTURE-LOCAL-ID": + return fmt.Sprintf("Local identifier %q has an invalid shape; use lowercase words joined by single hyphens, for example %q.", typed.Got, "needs-review") + case "JPS-STRUCTURE-PACK-VERSION": + return fmt.Sprintf("Pack version %q has an invalid shape; use MAJOR.MINOR.PATCH, for example %q.", typed.Got, "1.0.0") + case "JPS-STRUCTURE-FACT-PATH": + return fmt.Sprintf("Fact path %q is not a valid JSON Pointer; it must be empty or begin with %q, for example %q.", typed.Got, "/", "/amount") + case "JPS-STRUCTURE-DECIMAL-OPERAND": + return fmt.Sprintf("Ordered comparison operand %q must be a decimal string, for example %q.", typed.Got, "5000") + default: + return fmt.Sprintf("Value %q does not satisfy the required pattern.", typed.Got) + } +} + +// arityMessage states the required minimum and the actual count for a +// too-short collection. +func arityMessage(code string, typed *kind.MinItems) string { + switch code { + case "JPS-STRUCTURE-CONDITION-ARITY": + return fmt.Sprintf("Condition needs at least %d child condition%s but has %d.", typed.Want, plural(typed.Want), typed.Got) + case "JPS-STRUCTURE-ESCALATION-TRIGGERS": + return fmt.Sprintf("Escalation needs at least %d trigger%s but has %d.", typed.Want, plural(typed.Want), typed.Got) + default: + return fmt.Sprintf("Collection needs at least %d item%s but has %d.", typed.Want, plural(typed.Want), typed.Got) + } +} + +func plural(count int) string { + if count == 1 { + return "" + } + return "s" +} + func last(values []string) string { if len(values) == 0 { return "" @@ -498,12 +555,19 @@ func typeMessage(typed *kind.Type) string { // more than one invalid child collapses to a single generic shape error and // loses the specific, well-located diagnostics for each child. func branchRejectedByDiscriminator(cause *jsonschema.ValidationError) bool { - if _, isGroup := cause.ErrorKind.(*kind.Group); !isGroup { - return false + // A losing branch that failed on its op discriminator directly, as a single + // bare failure rather than a group -- otherwise that lone op-const/enum + // failure leaks out as a diagnostic anchored at an op that is actually valid. + if discriminatorMismatch(cause) { + return true } - for _, sub := range cause.Causes { - if discriminatorMismatch(sub) { - return true + // A losing branch whose failures were grouped, one of which is the op + // discriminator. + if _, isGroup := cause.ErrorKind.(*kind.Group); isGroup { + for _, sub := range cause.Causes { + if discriminatorMismatch(sub) { + return true + } } } return false diff --git a/internal/validation/validator_test.go b/internal/validation/validator_test.go index 3032d53..a06b65a 100644 --- a/internal/validation/validator_test.go +++ b/internal/validation/validator_test.go @@ -430,3 +430,108 @@ func TestNestedOperandErrorsAreNotMaskedByCompositeShape(t *testing.T) { t.Fatalf("composite should not report a generic shape error when its children have specific errors: %#v", actual.Diagnostics) } } + +func TestEnumDiagnosticListsAllowedValues(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["rules"].([]any)[0].(map[string]any)["onUnknown"] = "bogus-mode" + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-ENUM", "/rules/0/onUnknown") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected an enum diagnostic at /rules/0/onUnknown: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "escalate") { + t.Fatalf("enum message should list the allowed values: %q", message) + } +} + +func TestLocalIDDiagnosticNamesValueAndExample(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["outcomes"].([]any)[0].(map[string]any)["id"] = "Bad Id" + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-LOCAL-ID", "/outcomes/0/id") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected a local-id diagnostic at /outcomes/0/id: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "Bad Id") || !strings.Contains(message, "needs-review") { + t.Fatalf("local-id message should name the offending value and give an example: %q", message) + } +} + +func TestArityDiagnosticStatesMinimumAndActual(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["outcomes"] = document["outcomes"].([]any)[:1] // one outcome; the minimum is two + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-COLLECTION-ARITY", "/outcomes") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected a collection-arity diagnostic at /outcomes: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "at least 2") || !strings.Contains(message, "has 1") { + t.Fatalf("arity message should state the required minimum and the actual count: %q", message) + } +} + +func TestBadConditionOpIsConsistentAndNamesValidOps(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + // The same mistake -- an op that selects no condition branch -- must report + // the same code and location regardless of sibling members, list the valid + // ops, and never spuriously flag the op member itself. + for _, when := range []map[string]any{ + {"op": "maybe"}, + {"op": "maybe", "value": true}, + {"op": "maybe", "conditions": []any{map[string]any{"op": "literal", "value": true}}}, + } { + document := validDocument(t) + document["rules"].([]any)[0].(map[string]any)["when"] = when + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-CONDITION-SHAPE", "/rules/0/when") + if actual.Status != "invalid" || !ok { + t.Fatalf("bad op %v should report CONDITION-SHAPE at /rules/0/when: %#v", when, actual.Diagnostics) + } + if !strings.Contains(message, "fact") || !strings.Contains(message, "evidence-present") { + t.Fatalf("condition-shape message should list the valid op values: %q", message) + } + if hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-SCHEMA", "/rules/0/when/op") { + t.Fatalf("a bad op must not spuriously flag the op member: %#v", actual.Diagnostics) + } + } +} + +func TestValidConditionOpIsNotSpuriouslyFlagged(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + // op "all" is valid, but the object is shaped like a "not" (has "condition"). + // The losing branch's op-const failure must not leak onto the valid op; the + // object's real errors must still surface. + document := validDocument(t) + document["rules"].([]any)[0].(map[string]any)["when"] = map[string]any{ + "op": "all", + "condition": map[string]any{"op": "literal", "value": true}, + } + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + if actual.Status != "invalid" { + t.Fatalf("expected invalid: %#v", actual) + } + if hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-SCHEMA", "/rules/0/when/op") { + t.Fatalf("a valid op must not be flagged: %#v", actual.Diagnostics) + } + if !hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-REQUIRED-MEMBER", "/rules/0/when/conditions") { + t.Fatalf("the matched branch's real errors should surface: %#v", actual.Diagnostics) + } +}