From 87a3852e371b07ca3e639b80a34b0201bbc447ba Mon Sep 17 00:00:00 2001 From: SimonTaurus Date: Fri, 11 Sep 2026 04:55:20 +0200 Subject: [PATCH 1/2] feat(validation): report an unexpected exception as a validator fault A check raising something it does not expect had two possible fates and both were wrong: reported as FAIL it named the user's document for a defect of ours, and left to propagate it discarded the verdicts already computed for every other target in the run. - new `fault` status, fatal to the verdict like `fail` but distinct from it: no verdict was produced, so the document is neither condemned nor cleared - the fault keeps the id of the check that raised it, so no new check id enters the public interface - guards per check section, plus a last-resort guard per file, so one broken check costs one check - faults lead `failure_reasons` and are counted separately in the CLI Closes #145 --- docs/architecture.md | 8 + src/oold/validation/cli.py | 12 +- src/oold/validation/mcp_server.py | 6 +- src/oold/validation/pipeline.py | 361 +++++++++++++++---------- src/oold/validation/report.py | 41 ++- tests/test_validation/test_pipeline.py | 49 ++++ 6 files changed, 316 insertions(+), 161 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index cf7184b..58a1aed 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -151,6 +151,14 @@ in the vendored catalogue, never from a hardcoded column in the check itself. Th one code base validate against several specification versions at once: relaxing a `MUST` to a `SHOULD` upstream turns a failure into a warning here with no code change. +One severity does not come from the catalogue, because it is not about the document at all. A +check that raises something it does not expect reports `fault`: a defect in this package, not a +finding. It fails the run like `fail` does, but says the check produced no verdict, so the +document is neither condemned nor cleared. A fault keeps the id of the check that raised it +rather than reporting under one of its own, so "which part of the validator broke" is answered by +the same identifier that names what it was trying to establish, and the run still reports every +other target - one broken check costs one check. + A rule absent from the selected version's catalogue, or marked deprecated there, is skipped with a message saying so, rather than checked anyway - older meta-schema versions ship no catalogue at all and skip the whole `rule.*` family. A false positive costs far more than a missed finding, diff --git a/src/oold/validation/cli.py b/src/oold/validation/cli.py index 8f886ef..369f23e 100644 --- a/src/oold/validation/cli.py +++ b/src/oold/validation/cli.py @@ -21,7 +21,7 @@ from .meta_store import MetaSchemaError, describe_store, fetch_remote, load_index, resolve_selection from .meta_vendor import vendor_version from .pipeline import Options, run_compliance, validate_directory, validate_instance, validate_schema -from .report import FAIL, OK, SKIP, WARN, Report +from .report import FAIL, FAULT, OK, SKIP, WARN, Report EXIT_OK = 0 EXIT_FAILED = 1 @@ -86,6 +86,9 @@ def _classify(target: Path) -> str: FAIL: {"fg": "red", "bold": True}, WARN: {"fg": "yellow"}, SKIP: {"fg": "cyan"}, + # Brighter than FAIL on purpose: a fault is our bug, and the reader should not spend time + # looking for it in their own document. + FAULT: {"fg": "magenta", "bold": True}, } _meta_option = click.option( @@ -136,9 +139,12 @@ def _print_human(report: Report, verbose: bool) -> None: versions = ", ".join(report.meta_versions) or "none" click.echo(f"{status} {report.source}") click.echo(f" meta-schema: {versions}") + # Faults appear only when there are some. A permanent "0 fault(s)" would train the reader to + # skip the field, which is the opposite of what it is for. + faults = f", {counts[FAULT]} VALIDATOR FAULT(S)" if counts[FAULT] else "" click.echo( f" {counts[OK]} ok, {counts[FAIL]} failed, {counts[WARN]} warning(s), " - f"{counts[SKIP]} skipped, across {len(report.targets())} target(s)" + f"{counts[SKIP]} skipped{faults}, across {len(report.targets())} target(s)" ) shown = report.checks if verbose else [c for c in report.checks if c.status != OK] @@ -146,7 +152,7 @@ def _print_human(report: Report, verbose: bool) -> None: click.echo() for check in shown: style = _STATUS_STYLE.get(check.status, {}) - label = click.style(check.status.upper().ljust(4), **style) + label = click.style(check.status.upper().ljust(5), **style) rule = click.style(f" {check.rule}", fg="blue") if check.rule else "" version = f" [{check.meta_version}]" if check.meta_version else "" message = f": {check.message}" if check.message else "" diff --git a/src/oold/validation/mcp_server.py b/src/oold/validation/mcp_server.py index dbd2ec8..d29e7b7 100644 --- a/src/oold/validation/mcp_server.py +++ b/src/oold/validation/mcp_server.py @@ -60,7 +60,11 @@ class CheckResult(BaseModel): id: str = Field(description="The check id, e.g. lint.container or rule.id-fragment.") target: str = Field(description="What was checked: a schema file, an instance, or a directory entry.") - status: str = Field(description="ok, fail, warn, or skip.") + status: str = Field( + description="ok, fail, warn, skip, or fault. A fault is a defect in the validator: the " + "check raised something it does not expect, so it produced no verdict and the document " + "is neither condemned nor cleared. Like fail it makes the run not pass." + ) message: str = Field(default="", description="Why the check produced this status, when it is not ok.") detail: dict[str, Any] | None = Field( default=None, description="Extra structured detail behind the message. Only present with verbosity='full'." diff --git a/src/oold/validation/pipeline.py b/src/oold/validation/pipeline.py index 68d632f..02309ed 100644 --- a/src/oold/validation/pipeline.py +++ b/src/oold/validation/pipeline.py @@ -13,6 +13,9 @@ from __future__ import annotations import json +import traceback +from collections.abc import Iterator +from contextlib import contextmanager from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -32,7 +35,7 @@ from .meta_store import MetaBundle, MetaSchemaError, Rule, resolve_selection from .pattern_lint import lint from .predicates import check_predicates -from .report import FAIL, OK, SKIP, WARN, Report +from .report import FAIL, FAULT, OK, SKIP, WARN, Report from .resolve import Resolver, SchemaResolutionError, bound_schema from .roundtrip import roundtrip from .schema_checks import check_usable_as_validator, validate_against_meta @@ -171,6 +174,32 @@ def _read(path: Path) -> Any: # ---------------------------------------------------------------------------- schema checks +@contextmanager +def _guard(run: _Run, check_id: str, target: str) -> Iterator[None]: + """Record an unexpected exception as a fault of ``check_id`` rather than a finding. + + A check raising something it does not expect is a defect in this package, and the two ways + of handling it without this are both wrong: reporting FAIL names the user's document for a + fault of ours, and letting it propagate discards the verdicts already computed for every + other target in the run. + + The guard is deliberately broad where the code it wraps is not. Narrowing a call site (#127) + is what turns a swallowed exception into a propagating one, and this is where the propagating + one is allowed to land. The traceback goes in ``detail`` because a fault is a bug report + against this package and the message alone will not locate it. + """ + try: + yield + except Exception as exc: + run.add( + check_id, + target, + FAULT, + f"{type(exc).__name__}: {exc}", + {"traceback": traceback.format_exc()}, + ) + + def _check_schema(run: _Run, name: str) -> None: """Every check that applies to one schema file.""" try: @@ -179,29 +208,35 @@ def _check_schema(run: _Run, name: str) -> None: return # -- meta-schema well-formedness (per version) --------------------------------- - for bundle in run.bundles: - result = validate_against_meta(raw, bundle) - problems = list(result.errors) + check_usable_as_validator(raw) - if problems: - extra = f" (+{result.truncated} more)" if result.truncated else "" - run.add( - "schema.meta", - name, - FAIL, - problems[0] + extra, - {"errors": problems}, - bundle.version, - ) - else: - run.add("schema.meta", name, OK, meta_version=bundle.version) + with _guard(run, "schema.meta", name): + for bundle in run.bundles: + result = validate_against_meta(raw, bundle) + problems = list(result.errors) + check_usable_as_validator(raw) + if problems: + extra = f" (+{result.truncated} more)" if result.truncated else "" + run.add( + "schema.meta", + name, + FAIL, + problems[0] + extra, + {"errors": problems}, + bundle.version, + ) + else: + run.add("schema.meta", name, OK, meta_version=bundle.version) # -- $ref composition (version independent) ------------------------------------ + # Not guarded as a section: everything downstream needs `deref`, so a fault here has to stop + # this schema rather than let the later checks raise on a name that was never bound. try: resolved = run.resolver.load(run.directory / name) deref = run.resolver.dereference(resolved) except SchemaResolutionError as exc: run.add("schema.refs", name, FAIL, str(exc)) return + except Exception as exc: + run.add("schema.refs", name, FAULT, f"{type(exc).__name__}: {exc}", {"traceback": traceback.format_exc()}) + return if deref.unresolved: run.add("schema.refs", name, FAIL, deref.unresolved[0], {"unresolved": deref.unresolved}) @@ -210,77 +245,91 @@ def _check_schema(run: _Run, name: str) -> None: # -- pattern lint -------------------------------------------------------------- first = None - for bundle in run.bundles: - result = lint(raw, bundle) - first = first or result - gate = run.catalog_gate("lint.pattern", bundle) - if gate is not None: - run.add("lint.pattern", name, gate.status, gate.message, meta_version=bundle.version) - elif result.schema_errors: - run.add( - "lint.pattern", - name, - FAIL, - result.schema_errors[0], - {"errors": result.schema_errors}, - bundle.version, - ) - else: - run.add("lint.pattern", name, OK, meta_version=bundle.version) + with _guard(run, "lint.pattern", name): + for bundle in run.bundles: + result = lint(raw, bundle) + first = first or result + gate = run.catalog_gate("lint.pattern", bundle) + if gate is not None: + run.add("lint.pattern", name, gate.status, gate.message, meta_version=bundle.version) + elif result.schema_errors: + run.add( + "lint.pattern", + name, + FAIL, + result.schema_errors[0], + {"errors": result.schema_errors}, + bundle.version, + ) + else: + run.add("lint.pattern", name, OK, meta_version=bundle.version) if first is not None: # These two correlate `properties` with `@context`, so no meta-schema version can # express them and they are reported once rather than per version, gated against the # first selected bundle - the same one `first` was computed from. - gate = run.catalog_gate("lint.container", run.bundles[0]) - if gate is not None: - run.add("lint.container", name, gate.status, gate.message) - elif first.missing_container: - joined = ", ".join(first.missing_container) - plural = "ies" if len(first.missing_container) > 1 else "y" - run.add( - "lint.container", - name, - FAIL, - f"strict array propert{plural} without @container @set/@list: {joined}", - {"properties": first.missing_container}, - ) - else: - run.add("lint.container", name, OK) - - gate = run.catalog_gate("lint.iri-format", run.bundles[0]) - if gate is not None: - run.add("lint.iri-format", name, gate.status, gate.message) - elif first.missing_iri_format: - joined = ", ".join(first.missing_iri_format) - plural = "ies" if len(first.missing_iri_format) > 1 else "y" - run.add( - "lint.iri-format", - name, - WARN, - f"IRI reference propert{plural} without an iri-reference/uri* format: {joined}", - {"properties": first.missing_iri_format}, - ) + with _guard(run, "lint.container", name): + gate = run.catalog_gate("lint.container", run.bundles[0]) + if gate is not None: + run.add("lint.container", name, gate.status, gate.message) + elif first.missing_container: + joined = ", ".join(first.missing_container) + plural = "ies" if len(first.missing_container) > 1 else "y" + run.add( + "lint.container", + name, + FAIL, + f"strict array propert{plural} without @container @set/@list: {joined}", + {"properties": first.missing_container}, + ) + else: + run.add("lint.container", name, OK) + + with _guard(run, "lint.iri-format", name): + gate = run.catalog_gate("lint.iri-format", run.bundles[0]) + if gate is not None: + run.add("lint.iri-format", name, gate.status, gate.message) + elif first.missing_iri_format: + joined = ", ".join(first.missing_iri_format) + plural = "ies" if len(first.missing_iri_format) > 1 else "y" + run.add( + "lint.iri-format", + name, + WARN, + f"IRI reference propert{plural} without an iri-reference/uri* format: {joined}", + {"properties": first.missing_iri_format}, + ) _check_schema_jsonld(run, name, raw) def _check_schema_jsonld(run: _Run, name: str, raw: dict[str, Any]) -> None: """Generation, round-trip, remote-context and attribution for one schema.""" + from jsonschema import Draft202012Validator from pyld import jsonld - schema = run.bounded(name) - # -- satisfiability ------------------------------------------------------------ - generated = generate(schema) - if not generated.ok: - run.add("generate.satisfiable", name, FAIL, generated.error or "generation failed") + # An early return on fault rather than a guarded section: every check below needs `schema`, + # `generated` and `validator`, so carrying on would raise on names that were never bound and + # report one defect once per section that tripped over it. + try: + schema = run.bounded(name) + generated = generate(schema) + if not generated.ok: + run.add("generate.satisfiable", name, FAIL, generated.error or "generation failed") + return + validator = Draft202012Validator(schema, format_checker=OOLD_FORMAT_CHECKER) + errors = sorted(validator.iter_errors(generated.instance), key=lambda e: list(e.absolute_path)) + except Exception as exc: + run.add( + "generate.satisfiable", + name, + FAULT, + f"{type(exc).__name__}: {exc}", + {"traceback": traceback.format_exc()}, + ) return - from jsonschema import Draft202012Validator - - validator = Draft202012Validator(schema, format_checker=OOLD_FORMAT_CHECKER) - errors = sorted(validator.iter_errors(generated.instance), key=lambda e: list(e.absolute_path)) if errors: run.add( "generate.satisfiable", @@ -297,78 +346,85 @@ def _check_schema_jsonld(run: _Run, name: str, raw: dict[str, Any]) -> None: promoted = promoted_terms(raw) # -- generated-instance round-trip --------------------------------------------- - if cyclic: - run.add("roundtrip.generated", name, SKIP, CYCLIC_NOTE) - else: - result = roundtrip(schema, generated.instance, context_url, run.loader, promoted=promoted) - # A property with no @context term does not reach RDF, so it cannot come back. That is - # permitted (OOLD-SCH-2d05) and is context.coverage's finding, not a round-trip defect. - # Reporting it here as well would fail the schema for something the specification allows, - # under a check that cites no rule. What is left is a genuine loss: a property that was - # mapped and still did not survive. - unmapped = _unmapped_properties(run, name, raw, schema, generated.instance) - lost = [key for key in result.lost if key.split("[")[0].split(".")[0] not in unmapped] - if result.error: - run.add("roundtrip.generated", name, FAIL, result.error) - elif lost: - joined = ", ".join(lost) - plural = "ies" if len(lost) > 1 else "y" - run.add( - "roundtrip.generated", - name, - FAIL, - f"propert{plural} lost through RDF despite being mapped: {joined}", - {"lost": lost, "unmapped": sorted(unmapped)}, - ) + with _guard(run, "roundtrip.generated", name): + if cyclic: + run.add("roundtrip.generated", name, SKIP, CYCLIC_NOTE) else: - re_errors = sorted(validator.iter_errors(result.restored), key=lambda e: list(e.absolute_path)) - if re_errors: + result = roundtrip(schema, generated.instance, context_url, run.loader, promoted=promoted) + # A property with no @context term does not reach RDF, so it cannot come back. That is + # permitted (OOLD-SCH-2d05) and is context.coverage's finding, not a round-trip defect. + # Reporting it here as well would fail the schema for something the specification allows, + # under a check that cites no rule. What is left is a genuine loss: a property that was + # mapped and still did not survive. + unmapped = _unmapped_properties(run, name, raw, schema, generated.instance) + lost = [key for key in result.lost if key.split("[")[0].split(".")[0] not in unmapped] + if result.error: + run.add("roundtrip.generated", name, FAIL, result.error) + elif lost: + joined = ", ".join(lost) + plural = "ies" if len(lost) > 1 else "y" run.add( "roundtrip.generated", name, FAIL, - "reconstruction fails its schema (shape not preserved by @context?): " + re_errors[0].message, - {"restored": result.restored}, + f"propert{plural} lost through RDF despite being mapped: {joined}", + {"lost": lost, "unmapped": sorted(unmapped)}, ) else: - run.add( - "roundtrip.generated", - name, - OK, - "", - {"triples": result.triples, "method": result.method}, - ) + re_errors = sorted(validator.iter_errors(result.restored), key=lambda e: list(e.absolute_path)) + if re_errors: + run.add( + "roundtrip.generated", + name, + FAIL, + "reconstruction fails its schema (shape not preserved by @context?): " + re_errors[0].message, + {"restored": result.restored}, + ) + else: + run.add( + "roundtrip.generated", + name, + OK, + "", + {"triples": result.triples, "method": result.method}, + ) # -- schema usable as a remote context ----------------------------------------- - if cyclic: - run.add("context.remote", name, SKIP, CYCLIC_NOTE) - else: - try: - jsonld.expand( - {"@context": context_url, "@id": "https://example.org/dummy"}, - run.loader.options(base=run.loader.base_url), - ) - run.add("context.remote", name, OK) - # Kept broad deliberately. This calls jsonld.expand on the same shared pyld.jsonld - # module predicates.py calls it through, so an unexpected exception here has the same - # nowhere-to-go problem as the one documented there: no attribution path of its own, and - # no per-file guard in validate_directory to land in if it propagates. Narrowing this - # waits on #145 for the same reason predicates.py's catch does. - except Exception as exc: - from .loader import describe_jsonld_error + with _guard(run, "context.remote", name): + if cyclic: + run.add("context.remote", name, SKIP, CYCLIC_NOTE) + else: + try: + jsonld.expand( + {"@context": context_url, "@id": "https://example.org/dummy"}, + run.loader.options(base=run.loader.base_url), + ) + run.add("context.remote", name, OK) + # Still broad, so nothing reaches the guard above yet. Narrowing it to JsonLdError is + # #127's remaining work; what changed is that there is now somewhere for the + # unexpected case to land, which is what that narrowing was waiting for. + except Exception as exc: + from .loader import describe_jsonld_error - run.add("context.remote", name, FAIL, describe_jsonld_error(exc)) + run.add("context.remote", name, FAIL, describe_jsonld_error(exc)) - _check_predicates(run, name, raw, schema, generated.instance) + # Attributed to `context.predicates` although the call also produces `context.coverage`: the + # guard has to name one id before knowing where the fault came from, and coverage is derived + # from the predicate attribution rather than computed independently. + with _guard(run, "context.predicates", name): + _check_predicates(run, name, raw, schema, generated.instance) # -- per-branch variant coverage ----------------------------------------------- if cyclic: return - variants, total = collect_variants(schema, limit=run.options.max_variants) - if total > len(variants): - run.report.notes.append(f"{name}: {total} oneOf/anyOf branches, checking the first {len(variants)}") - for variant in variants: - _check_variant(run, name, schema, variant, validator, context_url, promoted) + with _guard(run, "variants", name): + variants, total = collect_variants(schema, limit=run.options.max_variants) + if total > len(variants): + run.report.notes.append(f"{name}: {total} oneOf/anyOf branches, checking the first {len(variants)}") + for variant in variants: + # Per variant, so one broken branch does not cost the verdicts of the others. + with _guard(run, "variants", f"{name} {variant.label}"): + _check_variant(run, name, schema, variant, validator, context_url, promoted) def _check_variant( @@ -616,25 +672,26 @@ def _check_instance_file(run: _Run, name: str, instance: Any = None) -> None: run.add("roundtrip.instance", name, SKIP, f"its schema {CYCLIC_NOTE}") return - rt = roundtrip_instance(instance, schema, run.loader, run.loader.url_for(name), run.loader.url_for(schema_ref)) - if rt.error: - run.add("roundtrip.instance", name, FAIL, rt.error) - elif not rt.lossless: - run.add( - "roundtrip.instance", - name, - FAIL, - "instance != roundtrip (incomplete @context?)", - {"in": rt.original_canonical, "out": rt.restored_canonical}, - ) - else: - run.add( - "roundtrip.instance", - name, - OK, - f"{rt.triples} triples, lossless ({rt.method})", - {"triples": rt.triples, "method": rt.method}, - ) + with _guard(run, "roundtrip.instance", name): + rt = roundtrip_instance(instance, schema, run.loader, run.loader.url_for(name), run.loader.url_for(schema_ref)) + if rt.error: + run.add("roundtrip.instance", name, FAIL, rt.error) + elif not rt.lossless: + run.add( + "roundtrip.instance", + name, + FAIL, + "instance != roundtrip (incomplete @context?)", + {"in": rt.original_canonical, "out": rt.restored_canonical}, + ) + else: + run.add( + "roundtrip.instance", + name, + OK, + f"{rt.triples} triples, lossless ({rt.method})", + {"triples": rt.triples, "method": rt.method}, + ) # ---------------------------------------------------------------------------- entry points @@ -672,10 +729,16 @@ def validate_directory(path: str | Path, options: Options | None = None) -> Repo return run.report _collect(run, schema_names) + # The last-resort guard. The per-check ones above attribute a fault precisely; this one + # exists so that a fault raised between them - or by the guard machinery itself - still + # costs one file rather than the whole directory, which is what collect-everything-then- + # report means. for name in schema_names: - _check_schema(run, name) + with _guard(run, "schema.meta", name): + _check_schema(run, name) for name in instance_names: - _check_instance_file(run, name) + with _guard(run, "instance.schema", name): + _check_instance_file(run, name) return run.report diff --git a/src/oold/validation/report.py b/src/oold/validation/report.py index 407e120..4267bdf 100644 --- a/src/oold/validation/report.py +++ b/src/oold/validation/report.py @@ -4,8 +4,15 @@ sync. A run is a flat list of :class:`Check` records; grouping (by target, by check id, by meta-schema version) is done at render time rather than baked into the structure. -Only ``fail`` is fatal to the verdict. ``warn`` marks a SHOULD-level finding, ``skip`` marks a -check that could not run for a documented reason (a cyclic scoped ``@context``, for instance). +``fail`` and ``fault`` are both fatal to the verdict, and they say different things. ``fail`` is a +finding about the document under test. ``fault`` is a defect in this package: the check raised +something it does not expect, so it produced no verdict at all and the document is neither +condemned nor cleared. ``warn`` marks a SHOULD-level finding, ``skip`` marks a check that could +not run for a documented reason (a cyclic scoped ``@context``, for instance). + +A fault keeps the id of the check that raised it rather than reporting under one of its own, so +"which part of the validator broke" is answered by the same identifier that names what it was +trying to establish. """ from __future__ import annotations @@ -14,13 +21,14 @@ from dataclasses import dataclass, field from typing import Any, Literal -Status = Literal["ok", "fail", "warn", "skip"] +Status = Literal["ok", "fail", "warn", "skip", "fault"] Verbosity = Literal["summary", "full"] OK: Status = "ok" FAIL: Status = "fail" WARN: Status = "warn" SKIP: Status = "skip" +FAULT: Status = "fault" @dataclass @@ -52,6 +60,13 @@ class Check: def failed(self) -> bool: return self.status == FAIL + @property + def is_fault(self) -> bool: + """The check broke rather than the document. Kept separate from :attr:`failed` so that + "how many findings" and "how many of our own defects" are never the same number. + """ + return self.status == FAULT + def to_dict(self, verbosity: Verbosity = "summary") -> dict[str, Any]: payload: dict[str, Any] = { "id": self.id, @@ -72,7 +87,7 @@ def line(self) -> str: """A single-line rendering: status, rule, check id, target, version and message, each in a fixed-width column. """ - label = self.status.upper().ljust(4) + label = self.status.upper().ljust(5) rule = f" {self.rule}" if self.rule else "" version = f" [{self.meta_version}]" if self.meta_version else "" message = f": {self.message}" if self.message else "" @@ -120,12 +135,14 @@ def extend(self, checks: list[Check]) -> None: @property def passed(self) -> bool: - return self.fatal_error is None and not any(c.failed for c in self.checks) + # A fault is not a finding, but it is not a pass either: the check produced no verdict, + # so the run cannot claim the document is clean. + return self.fatal_error is None and not any(c.failed or c.is_fault for c in self.checks) @property def counts(self) -> dict[str, int]: tally = Counter(c.status for c in self.checks) - return {status: tally.get(status, 0) for status in (OK, FAIL, WARN, SKIP)} + return {status: tally.get(status, 0) for status in (OK, FAIL, WARN, SKIP, FAULT)} def by_status(self, status: Status) -> list[Check]: return [c for c in self.checks if c.status == status] @@ -133,6 +150,9 @@ def by_status(self, status: Status) -> list[Check]: def failures(self) -> list[Check]: return self.by_status(FAIL) + def faults(self) -> list[Check]: + return self.by_status(FAULT) + def warnings(self) -> list[Check]: return self.by_status(WARN) @@ -172,7 +192,12 @@ def to_dict(self, verbosity: Verbosity = "summary") -> dict[str, Any]: def failure_reasons(report: Report) -> list[str]: - """Human-readable reasons the run did not pass, most important first.""" + """Human-readable reasons the run did not pass, most important first. + + Faults lead. A finding tells the reader to change their document; a fault tells them part of + the answer is missing, which they need to know before acting on any of the rest. + """ if report.fatal_error: return [report.fatal_error] - return [f"{c.id} {c.target}: {c.message}" for c in report.failures()] + faults = [f"{c.id} {c.target}: validator fault, {c.message}" for c in report.faults()] + return faults + [f"{c.id} {c.target}: {c.message}" for c in report.failures()] diff --git a/tests/test_validation/test_pipeline.py b/tests/test_validation/test_pipeline.py index 9f45e38..fdf20b5 100644 --- a/tests/test_validation/test_pipeline.py +++ b/tests/test_validation/test_pipeline.py @@ -308,6 +308,55 @@ def test_a_processor_failure_is_not_downgraded_to_a_coverage_warning(broken_dir, assert "context.coverage" not in failures +def test_an_unexpected_exception_is_a_fault_of_its_check_not_a_finding(data_dir, monkeypatch): + """A defect in this package must not read as a defect in the user's schema. + + The fault carries the id of the check that raised it, so the report answers "which part of + the validator broke" with the same identifier that names what it was trying to establish. + """ + from oold.validation import pipeline + + monkeypatch.setattr( + pipeline, "roundtrip", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("frame derivation broke")) + ) + report = validate_directory(data_dir, OFFLINE) + + faults = report.faults() + assert faults, "a raising check produced no fault" + # Both checks that call `roundtrip` fault, each under its own id rather than a shared one, + # and nothing else is implicated. + assert {c.id for c in faults} == {"roundtrip.generated", "variants"} + assert all("RuntimeError: frame derivation broke" in c.message for c in faults) + assert all("frame derivation broke" in c.detail["traceback"] for c in faults) + # A fault is not a finding: nothing was concluded about the document. + assert not {c.id for c in report.failures()} & {"roundtrip.generated", "variants"} + + +def test_a_fault_fails_the_run_without_discarding_the_other_verdicts(data_dir, monkeypatch): + """Collect-everything-then-report is the model, so one broken check must cost one check. + + Before this, the two available outcomes were a FAIL naming the user's schema or a propagating + exception that threw away every verdict already computed in the directory. + """ + from oold.validation import pipeline + + monkeypatch.setattr( + pipeline, "roundtrip", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("frame derivation broke")) + ) + report = validate_directory(data_dir, OFFLINE) + + assert not report.passed, "a fault must not read as a pass" + assert report.counts["fault"] == len(report.faults()) + # Every other check still ran, over every target, including on the faulting schemas. + assert report.counts["ok"] > 0 + assert {c.id for c in report.checks} > {"roundtrip.generated"} + assert len(report.targets()) > 1 + # And it leads the reasons, because it means part of the answer is missing. + from oold.validation.report import failure_reasons + + assert "validator fault" in failure_reasons(report)[0] + + def test_strict_promotes_an_unmapped_term_to_a_failure(broken_dir): report = validate_schema( broken_dir / "missing_context_term.schema.json", From a24316079d4f40b0ff8c7dd244d66b6b1bb02e80 Mon Sep 17 00:00:00 2001 From: SimonTaurus Date: Fri, 11 Sep 2026 05:34:59 +0200 Subject: [PATCH 2/2] test(validation): cover the fault paths the guard-shaped tests missed codecov reported the patch as covered; measuring the lines showed the two bespoke paths were not. Both sections that return early instead of using _guard had no test, and neither did the CLI rendering. Also drops Check.line(), which had no callers and duplicated the CLI's own rendering closely enough that widening the status column had to be done in both. --- src/oold/validation/report.py | 10 ------- tests/test_validation/test_cli.py | 27 ++++++++++++++++++ tests/test_validation/test_pipeline.py | 38 ++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) diff --git a/src/oold/validation/report.py b/src/oold/validation/report.py index 4267bdf..db3c2d1 100644 --- a/src/oold/validation/report.py +++ b/src/oold/validation/report.py @@ -83,16 +83,6 @@ def to_dict(self, verbosity: Verbosity = "summary") -> dict[str, Any]: payload["detail"] = self.detail return payload - def line(self) -> str: - """A single-line rendering: status, rule, check id, target, version and message, each in - a fixed-width column. - """ - label = self.status.upper().ljust(5) - rule = f" {self.rule}" if self.rule else "" - version = f" [{self.meta_version}]" if self.meta_version else "" - message = f": {self.message}" if self.message else "" - return f"{label}{rule} {self.id:<24} {self.target}{version}{message}" - @dataclass class Report: diff --git a/tests/test_validation/test_cli.py b/tests/test_validation/test_cli.py index 78f6e2e..977236c 100644 --- a/tests/test_validation/test_cli.py +++ b/tests/test_validation/test_cli.py @@ -35,6 +35,33 @@ def test_a_broken_schema_exits_nonzero(run, broken_dir): assert "context.predicates" in result.output +def test_a_validator_fault_is_named_as_ours_and_exits_nonzero(run, data_dir, monkeypatch): + """A fault must not send the reader looking for a defect in their own document. + + So the count is reported separately from failures, and only when there are some: a permanent + "0 fault(s)" would train people to skip the field, which is the opposite of what it is for. + """ + from oold.validation import pipeline + + monkeypatch.setattr( + pipeline, "roundtrip", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("frame derivation broke")) + ) + result = run("validate", str(data_dir), "--offline") + + assert result.exit_code == 1 + assert "VALIDATOR FAULT(S)" in result.output + assert "FAULT roundtrip.generated" in result.output + assert "RuntimeError: frame derivation broke" in result.output + # Nothing was concluded about the documents, so nothing is reported as failing. + assert "0 failed" in result.output + + +def test_a_clean_run_does_not_mention_faults(run, data_dir): + result = run("validate", str(data_dir), "--offline") + assert result.exit_code == 0, result.output + assert "FAULT" not in result.output + + def test_failures_are_shown_by_default_and_passes_are_not(run, data_dir): result = run("validate", str(data_dir), "--offline") assert "hidden" in result.output diff --git a/tests/test_validation/test_pipeline.py b/tests/test_validation/test_pipeline.py index fdf20b5..07c644d 100644 --- a/tests/test_validation/test_pipeline.py +++ b/tests/test_validation/test_pipeline.py @@ -332,6 +332,44 @@ def test_an_unexpected_exception_is_a_fault_of_its_check_not_a_finding(data_dir, assert not {c.id for c in report.failures()} & {"roundtrip.generated", "variants"} +def test_ref_resolution_faults_rather_than_blaming_the_schema(data_dir, monkeypatch): + """`schema.refs` guards by hand rather than with `_guard`, because it must return. + + Everything below it needs the dereferenced schema, so carrying on would report one defect + once per section that tripped over the missing name. + """ + from oold.validation import pipeline + + monkeypatch.setattr( + pipeline.Resolver, "dereference", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("resolver broke")) + ) + report = validate_directory(data_dir, OFFLINE) + faults = {c.id: c for c in report.faults()} + + assert "schema.refs" in faults, f"no schema.refs fault: {sorted(faults)}" + assert "RuntimeError: resolver broke" in faults["schema.refs"].message + assert not report.passed + # The early return is the point: one fault per schema, and no finding from the same section. + assert "schema.refs" not in {c.id for c in report.failures()} + assert "roundtrip.generated" not in faults, "the return did not stop the dependent sections" + + +def test_generation_faults_rather_than_blaming_the_schema(data_dir, monkeypatch): + """`generate.satisfiable` returns for the same reason: it produces the instance others reuse.""" + from oold.validation import pipeline + + monkeypatch.setattr(pipeline, "generate", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("generator broke"))) + report = validate_directory(data_dir, OFFLINE) + faults = {c.id: c for c in report.faults()} + + assert "generate.satisfiable" in faults, f"no generate.satisfiable fault: {sorted(faults)}" + assert "RuntimeError: generator broke" in faults["generate.satisfiable"].message + assert not report.passed + assert "generate.satisfiable" not in {c.id for c in report.failures()} + # Checks that do not depend on the generated instance still ran and still reported. + assert "schema.meta" in {c.id for c in report.checks} + + def test_a_fault_fails_the_run_without_discarding_the_other_verdicts(data_dir, monkeypatch): """Collect-everything-then-report is the model, so one broken check must cost one check.