fix: make validator diagnostics self-sufficient (backlog batch)

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
+}

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)
+		}
+	}
+}

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

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)
+	}
+}