diff --git a/docs/agent-testing.md b/docs/agent-testing.md new file mode 100644 index 0000000..84a43af --- /dev/null +++ b/docs/agent-testing.md @@ -0,0 +1,167 @@ +# Exercising the runtime with an agent + +This runtime validates Judgment Pack Specification (JPS) documents; it does not author them. A +practical way to test both the specification's *authorability* and this runtime's *diagnostics* is +to let an LLM agent author packs with the validator as its only feedback, then report where the +diagnostics helped and where they did not. + +This is a manual, exploratory protocol, distinct from the automated contributor checks in +[CONTRIBUTING.md](../CONTRIBUTING.md) and the bundled `spec test-conformance` suite. It complements +the specification's schema-validator protocol (`judgment-pack-spec`'s `TESTING.md`), which drives a +generic Draft 2020-12 validator rather than an agent. Why the runtime, rather than a bespoke UI, is +the integration and testing surface is recorded in +[ADR-0003](adr/0003-mcp-integration-and-testing-surface.md). + +Today the agent drives the command-line binary directly. A first-class MCP surface is future work +(ADR-0003); the loop below needs only the CLI. + +## What you need + +- A `judgment-pack` binary (installed or built below). +- Any agent client that can run a shell command and read and write files, configured with **your + own** model API key. The runtime holds no key, opens no network connection, and evaluates nothing, + so the key lives entirely in the client. Any of the many such clients works — for example Claude + Code, Cline, Cursor, or Continue. + +## Get the validator + +Either install a released binary or build from source. + +**Install a tagged release.** Download the archive for your operating system and architecture from +the repository's [GitHub Releases](https://github.com/Judgment-Pack/judgment-pack-runtime/releases) +and put the binary on your `PATH`. Each archive also ships a `jpack` short alias. + +```bash +tar -xzf judgment-pack___.tar.gz # on Windows, expand the .zip instead +install -m 0755 judgment-pack "$HOME/.local/bin/judgment-pack" +judgment-pack version +``` + +**Build from source.** With a currently supported Go toolchain, from a checkout of this repository. +If `go env GO111MODULE` reports `off`, force module mode as shown: + +```bash +env GO111MODULE=on go build -o ~/.local/bin/judgment-pack ./cmd/judgment-pack +judgment-pack version +``` + +## How the loop works + +The agent edits a candidate document and validates it after every change: + +```bash +judgment-pack spec validate --format json +``` + +- `status` is `valid`, `invalid`, or `unsupported`; the loop stops at `valid`. +- The process exit code is `0` for a valid document and non-zero otherwise. +- `layers[]` reports `carrier`, `structural`, and `semantic`; a later layer runs only after the + earlier ones pass. +- `diagnostics[]` carries one entry per problem, each naming a `code`, its `layer`, a `severity`, an + `instancePath` (a JSON Pointer to the exact location), and a `message`: + +```json +{ + "code": "JPS-SEMANTIC-UNRESOLVED-OUTCOME", + "codeStability": "provisional", + "layer": "semantic", + "severity": "error", + "instancePath": "/rules/0/outcome", + "message": "Outcome reference does not resolve." +} +``` + +The agent fixes each diagnostic at its `instancePath` and re-runs. `--format human` prints the same +result as a scannable summary; `spec schema 0.1.0-draft --write -` prints the exact bundled schema; +`--through carrier|structural|semantic` stops after a chosen layer. + +## Brief the agent + +Give the agent these instructions however your client accepts project instructions -- a rules file +(for example Cline's `.clinerules` or Cursor's `.cursorrules`), the client's custom-instructions +setting, or simply the agent's first message. Replace `` with the path to a +`judgment-pack-spec` checkout. + +```markdown +# JPS pack authoring -- validation loop + +You author and revise Judgment Pack Specification (JPS) documents. The judgment-pack CLI is your +validation oracle. It checks document conformance only -- carrier (bytes/JSON), structural (schema), +and semantic (references) -- for spec version 0.1.0-draft. It does not evaluate rules, choose an +outcome, fetch sources, or judge whether a pack is correct, authorized, safe, or fit. Structural +validity means well-formed, nothing more. + +## The loop +After every edit, validate and fix what it reports: + + judgment-pack spec validate --format json + +- status: "valid" | "invalid" | "unsupported". Stop at "valid". +- exit code: 0 = valid, non-zero = not valid. +- diagnostics[]: each has code, layer (carrier|structural|semantic), severity, instancePath (a JSON + Pointer), and message. Fix by instancePath and re-run. +- layers[]: a later layer runs only after earlier layers pass. + +To see the full schema: judgment-pack spec schema 0.1.0-draft --write - + +## Where to start +Read one example first to learn the shape, then build your own -- do not copy it verbatim: +- /examples/minimal-expense-approval.json (smallest) +- /examples/supplier-invoice-approval.json + +## Guardrails +- Work on a scratch copy. Never modify the checked-in examples. +- Never put real, personal, confidential, regulated, or production data in a pack. Invented, + low-risk data only. +- Never add or fetch a real URL in a source locator. Validation stays offline. + +## Report as you go +The goal is to judge whether these diagnostics are enough to author by. Call out every diagnostic +that was unclear, misleading, wrongly located, or missing when you expected one. That list is the +deliverable. +``` + +## Tasks + +Give the agent one or more of these, in rough order of difficulty: + +1. **Create** -- author a new pack for an everyday approval scenario (travel reimbursement, time-off, + ...), seeded by reading the minimal example. Iterate to `status:valid`. +2. **Update, then break a reference** -- add a rule or exception to a scratch copy of a larger + example and keep it valid; then point that rule at an outcome id that exists nowhere, and confirm + the semantic layer reports the dangling reference. +3. **Delete** -- remove an outcome or rule and its now-orphaned references; confirm the diagnostics. +4. **Adversarial** -- break a valid pack three structural ways and predict each diagnostic's + `instancePath` before running. +5. **Report** -- list which diagnostics were clear, which confused, and any case where an error was + expected but none appeared. + +## Guardrails + +The same rules the agent is given apply to whoever runs the protocol: work on scratch copies, never +edit the checked-in examples, use only invented low-risk data, and keep validation offline -- never +add or fetch a real URL. This mirrors the data-handling warning in `judgment-pack-spec`'s +`TESTING.md`. + +## Reading the result + +The deliverable is the agent's list of diagnostics that confused it, were mislocated, or were missing +when it expected one. Judge each diagnostic by whether a first-time author could act on it *without* +reading the schema or an example: + +- A strong diagnostic names the exact location and the specific problem -- for instance, distinct + semantic codes per reference kind (`JPS-SEMANTIC-UNRESOLVED-OUTCOME`, + `JPS-SEMANTIC-UNRESOLVED-EVIDENCE`), each pointing at the offending array element. +- A weak diagnostic states only that something is wrong -- for instance, a structural type error that + does not name the expected type -- leaving the author to guess or to copy an example. + +Findings of the second kind are the point: they are the backlog for improving the runtime's +diagnostics or the specification's clarity. + +## See also + +- `judgment-pack-spec`'s `TESTING.md` -- the schema-validator protocol for a generic Draft 2020-12 + validator. +- [ADR-0003](adr/0003-mcp-integration-and-testing-surface.md) -- why MCP is the integration and + testing surface. +- [CONTRIBUTING.md](../CONTRIBUTING.md) -- the automated checks expected before a pull request. diff --git a/internal/cli/app.go b/internal/cli/app.go index 5118380..32b82ce 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -26,6 +26,7 @@ type App struct { errOut io.Writer engine *validation.Engine runner *conformance.Runner + pretty bool } type handledExit struct { @@ -40,7 +41,7 @@ func Run(args []string, in io.Reader, out, errOut io.Writer) int { if err != nil { message := "Bundled JPS artifacts failed their integrity check." if requestedFormat(args) == "json" { - if writeJSON(out, result.NewOperationalError(requestedCommand(args), "JPS-ARTIFACT-INTEGRITY", message)) != nil { + if writeJSON(out, result.NewOperationalError(requestedCommand(args), "JPS-ARTIFACT-INTEGRITY", message), false) != nil { return result.ExitIO } } else { @@ -58,7 +59,7 @@ func Run(args []string, in io.Reader, out, errOut io.Writer) int { } message := display.Sanitize(err.Error()) if requestedFormat(args) == "json" { - if writeJSON(out, result.NewOperationalError(requestedCommand(args), "JPR-INVOCATION-ARGUMENTS", message)) != nil { + if writeJSON(out, result.NewOperationalError(requestedCommand(args), "JPR-INVOCATION-ARGUMENTS", message), false) != nil { return result.ExitIO } } else { @@ -87,6 +88,7 @@ func (a *App) rootCommand() *cobra.Command { root.SetErr(a.errOut) root.SetVersionTemplate("judgment-pack {{.Version}}\n") root.CompletionOptions.DisableDefaultCmd = true + root.PersistentFlags().BoolVar(&a.pretty, "pretty", false, "indent JSON output") root.AddCommand(a.versionCommand(), a.specCommand()) return root } @@ -121,7 +123,7 @@ func (a *App) versionCommand() *cobra.Command { } output := describe.Runtime(set, "version") if format == "json" { - if err := writeJSON(a.out, output); err != nil { + if err := a.writeJSON(output); err != nil { return &handledExit{code: result.ExitIO} } } else { @@ -160,6 +162,9 @@ func (a *App) validateCommand() *cobra.Command { } data, err := a.readPack(args[0], maxBytes) if err != nil { + if errors.Is(err, fssecure.ErrTooLarge) { + return a.operational("spec validate", format, result.ExitIO, "JPS-RESOURCE-INPUT-BYTE-LIMIT", fmt.Sprintf("Input exceeds the %d-byte limit.", maxBytes)) + } return a.operational("spec validate", format, result.ExitIO, "JPS-INPUT-READ", "Input could not be read as one bounded regular file or standard input stream.") } output, operational := a.engine.Validate(data, validation.Options{Through: through, Limits: carrier.DefaultLimits()}) @@ -291,7 +296,7 @@ func readBounded(reader io.Reader, limit int64) ([]byte, error) { return nil, err } if int64(len(data)) > limit { - return nil, errors.New("input exceeds byte limit") + return nil, fssecure.ErrTooLarge } return data, nil } diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index e737e00..21b005e 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -194,3 +194,77 @@ func first(value string, count int) string { } return value[:count] } + +func TestPrettyFlagIndentsJSONOutput(t *testing.T) { + set, err := artifacts.Load(artifacts.DraftVersion) + if err != nil { + t.Fatal(err) + } + valid, err := set.Case("valid/minimal-literal.json") + if err != nil { + t.Fatal(err) + } + validPath := writeFixture(t, valid) + code, stdout, stderr := runTest(t, []string{"spec", "validate", validPath, "--format", "json", "--pretty"}, "") + if code != 0 || stderr != "" { + t.Fatalf("exit=%d stderr=%q", code, stderr) + } + if strings.Count(stdout, "\n") < 2 { + t.Fatalf("pretty JSON should span multiple lines: %q", stdout) + } + var output map[string]any + if err := json.Unmarshal([]byte(stdout), &output); err != nil { + t.Fatal(err) + } + if output["status"] != "valid" { + t.Fatalf("unexpected result: %#v", output) + } +} + +func TestInputSizeAndReadErrorsUseDistinctCodes(t *testing.T) { + set, err := artifacts.Load(artifacts.DraftVersion) + if err != nil { + t.Fatal(err) + } + valid, err := set.Case("valid/minimal-literal.json") + if err != nil { + t.Fatal(err) + } + validPath := writeFixture(t, valid) + + // A byte limit below the document size reports a dedicated resource code, + // distinct from a generic unreadable-input failure. + code, stdout, stderr := runTest(t, []string{"spec", "validate", validPath, "--max-bytes", "10", "--format", "json"}, "") + if code != 4 || stderr != "" { + t.Fatalf("oversized: exit=%d stderr=%q", code, stderr) + } + if got := diagnosticCode(t, stdout); got != "JPS-RESOURCE-INPUT-BYTE-LIMIT" { + t.Fatalf("oversized input should report the byte-limit code, got %q", got) + } + + // A genuinely unreadable input keeps the generic read code. + missing := filepath.Join(t.TempDir(), "absent.json") + code, stdout, stderr = runTest(t, []string{"spec", "validate", missing, "--format", "json"}, "") + if code != 4 || stderr != "" { + t.Fatalf("missing: exit=%d stderr=%q", code, stderr) + } + if got := diagnosticCode(t, stdout); got != "JPS-INPUT-READ" { + t.Fatalf("missing input should report the generic read code, got %q", got) + } +} + +func diagnosticCode(t *testing.T, jsonOutput string) string { + t.Helper() + var output struct { + Diagnostics []struct { + Code string `json:"code"` + } `json:"diagnostics"` + } + if err := json.Unmarshal([]byte(jsonOutput), &output); err != nil { + t.Fatalf("decode: %v (output %q)", err, jsonOutput) + } + if len(output.Diagnostics) != 1 { + t.Fatalf("expected exactly one diagnostic, got %d: %q", len(output.Diagnostics), jsonOutput) + } + return output.Diagnostics[0].Code +} diff --git a/internal/cli/render.go b/internal/cli/render.go index 3bd95fb..776c5b6 100644 --- a/internal/cli/render.go +++ b/internal/cli/render.go @@ -10,19 +10,26 @@ import ( "github.com/Judgment-Pack/judgment-pack-runtime/internal/result" ) -func writeJSON(writer io.Writer, value any) error { +func writeJSON(writer io.Writer, value any, pretty bool) error { encoder := json.NewEncoder(writer) encoder.SetEscapeHTML(false) + if pretty { + encoder.SetIndent("", " ") + } return encoder.Encode(value) } +func (a *App) writeJSON(value any) error { + return writeJSON(a.out, value, a.pretty) +} + func (a *App) operational(command, format string, exitCode int, code, message string) error { status := "error" if exitCode == result.ExitUnsupported { status = "unsupported" } if format == "json" { - if err := writeJSON(a.out, result.NewOperationalResult(command, status, code, message)); err != nil { + if err := a.writeJSON(result.NewOperationalResult(command, status, code, message)); err != nil { return &handledExit{code: result.ExitIO} } } else if status == "unsupported" { @@ -35,7 +42,7 @@ func (a *App) operational(command, format string, exitCode int, code, message st func (a *App) renderValidation(format string, output result.Validation) error { if format == "json" { - return writeJSON(a.out, output) + return a.writeJSON(output) } if output.Status == "valid" { if output.ValidationScope.FullDocumentConformance { @@ -61,7 +68,7 @@ func (a *App) renderValidation(format string, output result.Validation) error { func (a *App) renderSuite(format string, output result.Suite) error { if format == "json" { - return writeJSON(a.out, output) + return a.writeJSON(output) } if output.Status == "valid" { fmt.Fprintf(a.out, "passed: %d/%d conformance cases matched expectations\n", output.Summary.Passed, output.Summary.Total) @@ -83,7 +90,7 @@ func (a *App) renderSuite(format string, output result.Suite) error { func (a *App) renderSchema(format string, output result.Schema) error { if format == "json" { - return writeJSON(a.out, output) + return a.writeJSON(output) } fmt.Fprintf(a.out, "JPS schema %s\n", display.Sanitize(output.SpecVersion)) fmt.Fprintf(a.out, "id: %s\n", display.Sanitize(output.SchemaID)) diff --git a/internal/fssecure/read.go b/internal/fssecure/read.go index ce36c24..6fc1613 100644 --- a/internal/fssecure/read.go +++ b/internal/fssecure/read.go @@ -5,6 +5,11 @@ import ( "io" ) +// ErrTooLarge reports that a read stopped because the input exceeded the +// caller's byte limit, rather than being missing or not a regular file. It lets +// a caller distinguish a size-limit failure from a generic read failure. +var ErrTooLarge = errors.New("input exceeds the byte limit") + func ReadRegular(filePath string, limit int64) ([]byte, error) { file, err := openRegular(filePath) if err != nil { @@ -16,14 +21,14 @@ func ReadRegular(filePath string, limit int64) ([]byte, error) { return nil, errors.New("path is not a regular file") } if info.Size() > limit { - return nil, errors.New("file exceeds limit") + return nil, ErrTooLarge } data, err := io.ReadAll(io.LimitReader(file, limit+1)) if err != nil { return nil, err } if int64(len(data)) > limit { - return nil, errors.New("file exceeds limit") + return nil, ErrTooLarge } return data, nil } diff --git a/internal/validation/semantic.go b/internal/validation/semantic.go index f44e182..f1b5448 100644 --- a/internal/validation/semantic.go +++ b/internal/validation/semantic.go @@ -27,34 +27,34 @@ func semanticDiagnostics(root map[string]any) ([]result.Diagnostic, bool) { sources := idSet(root, "sources") for index, rule := range objectList(root["rules"]) { - if !unresolved(diagnostics, stringValue(rule["outcome"]), outcomes, []string{"rules", fmt.Sprint(index), "outcome"}, "JPS-SEMANTIC-UNRESOLVED-OUTCOME", "Outcome reference does not resolve.") { + if !unresolved(diagnostics, stringValue(rule["outcome"]), outcomes, []string{"rules", fmt.Sprint(index), "outcome"}, "JPS-SEMANTIC-UNRESOLVED-OUTCOME", "outcome") { return diagnostics.sorted(), diagnostics.truncated } for refIndex, reference := range stringList(rule["evidenceRequirementRefs"]) { - if !unresolved(diagnostics, reference, evidence, []string{"rules", fmt.Sprint(index), "evidenceRequirementRefs", fmt.Sprint(refIndex)}, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "Evidence requirement reference does not resolve.") { + if !unresolved(diagnostics, reference, evidence, []string{"rules", fmt.Sprint(index), "evidenceRequirementRefs", fmt.Sprint(refIndex)}, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "evidence requirement") { return diagnostics.sorted(), diagnostics.truncated } } for refIndex, reference := range stringList(rule["sourceRefs"]) { - if !unresolved(diagnostics, reference, sources, []string{"rules", fmt.Sprint(index), "sourceRefs", fmt.Sprint(refIndex)}, "JPS-SEMANTIC-UNRESOLVED-SOURCE", "Source reference does not resolve.") { + if !unresolved(diagnostics, reference, sources, []string{"rules", fmt.Sprint(index), "sourceRefs", fmt.Sprint(refIndex)}, "JPS-SEMANTIC-UNRESOLVED-SOURCE", "source") { return diagnostics.sorted(), diagnostics.truncated } } if !walkCondition(rule["when"], []string{"rules", fmt.Sprint(index), "when"}, func(reference string, location []string) bool { - return unresolved(diagnostics, reference, evidence, location, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "Evidence requirement reference does not resolve.") + return unresolved(diagnostics, reference, evidence, location, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "evidence requirement") }) { return diagnostics.sorted(), diagnostics.truncated } } if fallback, ok := root["fallbackOutcome"].(string); ok { - if !unresolved(diagnostics, fallback, outcomes, []string{"fallbackOutcome"}, "JPS-SEMANTIC-UNRESOLVED-OUTCOME", "Outcome reference does not resolve.") { + if !unresolved(diagnostics, fallback, outcomes, []string{"fallbackOutcome"}, "JPS-SEMANTIC-UNRESOLVED-OUTCOME", "outcome") { return diagnostics.sorted(), diagnostics.truncated } } if applicability, ok := root["applicability"]; ok { if !walkCondition(applicability, []string{"applicability"}, func(reference string, location []string) bool { - return unresolved(diagnostics, reference, evidence, location, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "Evidence requirement reference does not resolve.") + return unresolved(diagnostics, reference, evidence, location, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "evidence requirement") }) { return diagnostics.sorted(), diagnostics.truncated } @@ -62,22 +62,22 @@ func semanticDiagnostics(root map[string]any) ([]result.Diagnostic, bool) { for index, exception := range objectList(root["exceptions"]) { if target, ok := exception["targetRule"].(string); ok { - if !unresolved(diagnostics, target, rules, []string{"exceptions", fmt.Sprint(index), "targetRule"}, "JPS-SEMANTIC-UNRESOLVED-RULE", "Rule reference does not resolve.") { + if !unresolved(diagnostics, target, rules, []string{"exceptions", fmt.Sprint(index), "targetRule"}, "JPS-SEMANTIC-UNRESOLVED-RULE", "rule") { return diagnostics.sorted(), diagnostics.truncated } } if outcome, ok := exception["outcome"].(string); ok { - if !unresolved(diagnostics, outcome, outcomes, []string{"exceptions", fmt.Sprint(index), "outcome"}, "JPS-SEMANTIC-UNRESOLVED-OUTCOME", "Outcome reference does not resolve.") { + if !unresolved(diagnostics, outcome, outcomes, []string{"exceptions", fmt.Sprint(index), "outcome"}, "JPS-SEMANTIC-UNRESOLVED-OUTCOME", "outcome") { return diagnostics.sorted(), diagnostics.truncated } } for refIndex, reference := range stringList(exception["sourceRefs"]) { - if !unresolved(diagnostics, reference, sources, []string{"exceptions", fmt.Sprint(index), "sourceRefs", fmt.Sprint(refIndex)}, "JPS-SEMANTIC-UNRESOLVED-SOURCE", "Source reference does not resolve.") { + if !unresolved(diagnostics, reference, sources, []string{"exceptions", fmt.Sprint(index), "sourceRefs", fmt.Sprint(refIndex)}, "JPS-SEMANTIC-UNRESOLVED-SOURCE", "source") { return diagnostics.sorted(), diagnostics.truncated } } if !walkCondition(exception["when"], []string{"exceptions", fmt.Sprint(index), "when"}, func(reference string, location []string) bool { - return unresolved(diagnostics, reference, evidence, location, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "Evidence requirement reference does not resolve.") + return unresolved(diagnostics, reference, evidence, location, "JPS-SEMANTIC-UNRESOLVED-EVIDENCE", "evidence requirement") }) { return diagnostics.sorted(), diagnostics.truncated } @@ -126,13 +126,48 @@ func idSet(root map[string]any, collection string) map[string]bool { return output } -func unresolved(diagnostics *diagnosticCollector, reference string, targets map[string]bool, location []string, code, message string) bool { +func unresolved(diagnostics *diagnosticCollector, reference string, targets map[string]bool, location []string, code, noun string) bool { if !targets[reference] { - return diagnostics.add(result.ErrorDiagnostic(code, "semantic", carrier.Pointer(location), message)) + return diagnostics.add(result.ErrorDiagnostic(code, "semantic", carrier.Pointer(location), unresolvedMessage(noun, reference, targets))) } return true } +// unresolvedMessage names the reference that did not resolve and the ids that +// were actually declared, so an author can see both the mistake and the valid +// choices without opening the document elsewhere. +func unresolvedMessage(noun, reference string, targets map[string]bool) string { + base := fmt.Sprintf("%s reference %q does not resolve to a declared %s.", capitalize(noun), reference, noun) + declared := sortedKeys(targets) + if len(declared) == 0 { + return base + fmt.Sprintf(" No %s ids are declared.", noun) + } + return base + fmt.Sprintf(" Declared %s ids: %s.", noun, joinCapped(declared, 12)) +} + +func sortedKeys(set map[string]bool) []string { + keys := make([]string, 0, len(set)) + for key := range set { + keys = append(keys, key) + } + slices.Sort(keys) + return keys +} + +func joinCapped(values []string, limit int) string { + if len(values) <= limit { + return strings.Join(values, ", ") + } + return strings.Join(values[:limit], ", ") + fmt.Sprintf(" (and %d more)", len(values)-limit) +} + +func capitalize(text string) string { + if text == "" { + return text + } + return strings.ToUpper(text[:1]) + text[1:] +} + func walkCondition(value any, location []string, visit func(string, []string) bool) bool { condition, ok := value.(map[string]any) if !ok { diff --git a/internal/validation/validator.go b/internal/validation/validator.go index 31ab59c..7bb9898 100644 --- a/internal/validation/validator.go +++ b/internal/validation/validator.go @@ -345,11 +345,14 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics add(code, location, structuralMessage(code)) case *kind.Type: handled = true - code := "JPS-STRUCTURE-TYPE" - if last(location) == "value" && contains(typed.Want, "array") { - code = "JPS-STRUCTURE-IN-OPERAND" + switch { + case comparisonOperandValue(location) && contains(typed.Want, "array"): + add("JPS-STRUCTURE-IN-OPERAND", location, structuralMessage("JPS-STRUCTURE-IN-OPERAND")) + case comparisonOperandValue(location) && contains(typed.Want, "string"): + add("JPS-STRUCTURE-DECIMAL-OPERAND", location, structuralMessage("JPS-STRUCTURE-DECIMAL-OPERAND")) + default: + add("JPS-STRUCTURE-TYPE", location, typeMessage(typed)) } - add(code, location, structuralMessage(code)) case *kind.Const: if last(location) == "specVersion" { handled = true @@ -366,17 +369,18 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics add("JPS-STRUCTURE-UNIQUE-ITEMS", location, "Array items must be unique.") case *kind.OneOf: handled = true - directCauses := make([]*jsonschema.ValidationError, 0, len(validationError.Causes)) + relevantCauses := make([]*jsonschema.ValidationError, 0, len(validationError.Causes)) for _, cause := range validationError.Causes { - if _, irrelevantBranch := cause.ErrorKind.(*kind.Group); !irrelevantBranch { - directCauses = append(directCauses, cause) + if branchRejectedByDiscriminator(cause) { + continue } + relevantCauses = append(relevantCauses, cause) } - if len(directCauses) == 0 { + if len(relevantCauses) == 0 { add("JPS-STRUCTURE-CONDITION-SHAPE", location, "Condition does not match one declared condition shape.") return } - for _, cause := range directCauses { + for _, cause := range relevantCauses { collectStructural(cause, diagnostics) } return @@ -422,7 +426,7 @@ func structuralMessage(code string) string { case "JPS-STRUCTURE-FACT-PATH": return "Fact path is not a valid JSON Pointer." case "JPS-STRUCTURE-DECIMAL-OPERAND": - return "Ordered comparison operand is not a valid decimal string." + return "Ordered comparison operand must be a decimal string, for example \"5000\"." case "JPS-STRUCTURE-EXTENSION-NAME": return "Extension name must use an allowed reverse-domain namespace." case "JPS-STRUCTURE-CONDITION-ARITY": @@ -456,6 +460,69 @@ func contains(values []string, wanted string) bool { return false } +// comparisonOperandValue reports whether an instance location denotes the +// "value" operand of a fact comparison condition, as opposed to another member +// that happens to be named "value" (such as a source locator value). Only a +// comparison operand should carry an operator-specific diagnostic. +func comparisonOperandValue(location []string) bool { + n := len(location) + if n < 2 || location[n-1] != "value" { + return false + } + switch location[n-2] { + case "when", "applicability", "condition": + return true + } + return n >= 3 && location[n-3] == "conditions" +} + +// typeMessage names the expected and actual JSON types so an author does not +// have to consult the schema to learn what a member should have been. +func typeMessage(typed *kind.Type) string { + want := strings.Join(typed.Want, " or ") + switch { + case want == "": + return "Value has the wrong JSON type." + case typed.Got == "": + return fmt.Sprintf("Value must be of type %s.", want) + default: + return fmt.Sprintf("Value must be of type %s, but is %s.", want, typed.Got) + } +} + +// branchRejectedByDiscriminator reports whether a oneOf branch failed solely +// because its "op" discriminator did not match, which makes its remaining +// errors noise from an alternative the author never intended. A branch whose op +// matched but that failed deeper is kept, even when the schema library groups +// several of its sub-failures together -- otherwise a composite condition with +// 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 + } + for _, sub := range cause.Causes { + if discriminatorMismatch(sub) { + return true + } + } + return false +} + +// discriminatorMismatch reports whether a sub-error is a failure of the "op" +// discriminator itself: a wrong constant or enum value at op, or a missing op +// member. Only the branches whose op did not match produce these. +func discriminatorMismatch(ve *jsonschema.ValidationError) bool { + switch ve.ErrorKind.(type) { + case *kind.Const, *kind.Enum: + return last(ve.InstanceLocation) == "op" + } + if required, ok := ve.ErrorKind.(*kind.Required); ok { + return contains(required.Missing, "op") + } + return false +} + func uniqueSorted(values []string) []string { seen := map[string]bool{} output := []string{} diff --git a/internal/validation/validator_test.go b/internal/validation/validator_test.go index a021ce3..3032d53 100644 --- a/internal/validation/validator_test.go +++ b/internal/validation/validator_test.go @@ -316,3 +316,117 @@ func hasDiagnostic(diagnostics []result.Diagnostic, code, location string) bool } return false } + +func diagnosticMessage(diagnostics []result.Diagnostic, code, location string) (string, bool) { + for _, diagnostic := range diagnostics { + if diagnostic.Code == code && diagnostic.InstancePath == location { + return diagnostic.Message, true + } + } + return "", false +} + +func TestTypeDiagnosticNamesExpectedType(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["title"] = float64(7) + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-TYPE", "/title") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected a type diagnostic at /title: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "string") { + t.Fatalf("type diagnostic should name the expected type: %q", message) + } +} + +func TestNumericOrderedOperandReportsDecimalDiagnostic(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + rule := document["rules"].([]any)[0].(map[string]any) + rule["when"] = map[string]any{ + "op": "fact", "path": "/amount", "operator": "greater-than", "value": float64(5000), + } + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-DECIMAL-OPERAND", "/rules/0/when/value") + if actual.Status != "invalid" || !ok { + t.Fatalf("a numeric ordered operand should report the decimal-operand diagnostic, not a generic type error: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "decimal string") { + t.Fatalf("decimal-operand message should teach the requirement: %q", message) + } +} + +func TestSourceLocatorValueStaysGenericType(t *testing.T) { + // A member named "value" that is not a comparison operand (here a source + // locator value) must keep the generic type diagnostic, not be misreported + // as an operand-specific one. + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["sources"] = []any{ + map[string]any{ + "id": "s1", + "title": "Example source", + "locator": map[string]any{"kind": "uri", "value": float64(3)}, + }, + } + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + if actual.Status != "invalid" || !hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-TYPE", "/sources/0/locator/value") { + t.Fatalf("a locator value type error should stay a generic type diagnostic: %#v", actual.Diagnostics) + } +} + +func TestUnresolvedReferenceNamesOffendingIDAndDeclaredSet(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["rules"].([]any)[0].(map[string]any)["outcome"] = "does-not-exist" + actual := validateDocument(t, engine, document, Options{Through: "semantic", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-SEMANTIC-UNRESOLVED-OUTCOME", "/rules/0/outcome") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected an unresolved-outcome diagnostic: %#v", actual.Diagnostics) + } + if !strings.Contains(message, `"does-not-exist"`) || !strings.Contains(message, "Declared outcome ids:") { + t.Fatalf("unresolved message should name the offending id and the declared set: %q", message) + } +} + +func TestNestedOperandErrorsAreNotMaskedByCompositeShape(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + rule := document["rules"].([]any)[0].(map[string]any) + rule["when"] = map[string]any{ + "op": "all", + "conditions": []any{ + map[string]any{"op": "fact", "path": "/a", "operator": "greater-than", "value": float64(5)}, + map[string]any{"op": "fact", "path": "/b", "operator": "less-than", "value": float64(9)}, + }, + } + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + if actual.Status != "invalid" { + t.Fatalf("expected invalid: %#v", actual) + } + // Both nested operands must report at their own paths, not collapse to a + // single generic shape error at the composite condition. + if !hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-DECIMAL-OPERAND", "/rules/0/when/conditions/0/value") || + !hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-DECIMAL-OPERAND", "/rules/0/when/conditions/1/value") { + t.Fatalf("both nested operand errors should surface at their own paths: %#v", actual.Diagnostics) + } + if hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-CONDITION-SHAPE", "/rules/0/when") { + t.Fatalf("composite should not report a generic shape error when its children have specific errors: %#v", actual.Diagnostics) + } +}