From 03f13845d65d6f941cab1c1bd7aed76f46cef9d6 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 3 Aug 2026 07:49:05 -0400 Subject: [PATCH] Three defects the catalogue triage found (#104) Classifying all 87 non-validating examples by their actual `validate` output turned up three clusters that were not example problems. Each reported *the pipeline* as broken when the fault was ours, so repairing the examples by hand would have meant "fixing" files that were already correct. 1. A JSON Schema was read as a model specification. Any nested dict carrying a `name` or `model` key counted as a model spec, which makes this look like one: parameters.schema.properties.records.items.properties -> {'id': {...}, 'name': {'type': 'string'}, 'active': {...}} `name` there is a *field* called "name", and its value is a mapping, so the "model name" came back as a dict and `validated_models.add(...)` raised `TypeError: unhashable type: 'dict'`. The TypeError escaped and surfaced as "Model validation failed", blaming the pipeline for a bug in the validator. Six examples. A dict is now a model specification when it is reached under a model key -- `model`, `llm`, `language_model` -- and not merely for having a `name`. The non-string guard stays as well: two independent defences, one test each. My first hypothesis was a task-level `model:` written as a requirements dict. Wrong: none of the three files has one, and a synthetic pipeline with one produces a different, correct error. The traceback located it. 2. `execution['timestamp']` was read as a task id. References were split on `.`, which leaves a subscript attached, so the bracket spelling of something #454 makes valid was reported as an undefined *task* -- on pipelines that run correctly. Same class as `now()` before #451. It hits step outputs too (`get_full_context['keys']`), and `a[0]` now resolves to `a` rather than to a name called "a[0]". 3. `json_encode` did not exist. Registered as an alias of `to_json` -- the same function object, not a second implementation, since two implementations of one filter is exactly the drift #449 removed. Measured: catalogue validating 30 -> 34 of 117; blocking suite 700 -> 711 passed, 0 failed. Four mutations, all killed: - treat any dict with a `name` key as a model -> 1 test - drop the non-string model-name guard -> 1 test - drop subscript normalisation -> 5 tests - give json_encode its own implementation -> 1 test Co-Authored-By: Claude Opus 5 (1M context) --- src/orchestrator/core/template_manager.py | 4 + .../validation/data_flow_validator.py | 13 ++ .../validation/model_validator.py | 40 +++- tests/test_triage_product_defects.py | 183 ++++++++++++++++++ 4 files changed, 236 insertions(+), 4 deletions(-) create mode 100644 tests/test_triage_product_defects.py diff --git a/src/orchestrator/core/template_manager.py b/src/orchestrator/core/template_manager.py index a3ca52bb..86b16968 100644 --- a/src/orchestrator/core/template_manager.py +++ b/src/orchestrator/core/template_manager.py @@ -287,6 +287,10 @@ def now() -> datetime: 'truncate_words': truncate_words, 'regex_search': regex_search, 'to_json': to_json, + # Ansible spells it this way, and two catalogue examples were + # written against that spelling. Same function, not a second + # implementation -- #449 was about exactly that kind of drift. + 'json_encode': to_json, 'json': to_json, 'from_json': from_json, 'date': date_format, diff --git a/src/orchestrator/validation/data_flow_validator.py b/src/orchestrator/validation/data_flow_validator.py index 81c97f8d..07c5ce90 100644 --- a/src/orchestrator/validation/data_flow_validator.py +++ b/src/orchestrator/validation/data_flow_validator.py @@ -21,6 +21,11 @@ logger = logging.getLogger(__name__) +#: `thing['key']` -> `thing.key`, so one spelling reaches the checks below. +_SUBSCRIPT = re.compile(r"""\[\s*['"]([^'"]+)['"]\s*\]""") +#: `thing[0]` -> `thing`; an element of a collection is not a separate name. +_INDEX = re.compile(r"\[\s*\d+\s*\]") + @dataclass class DataFlowError: @@ -465,6 +470,14 @@ def _validate_variable_reference(self, # Loop variables like $item, $index return {"valid": True, "type": "loop_variable"} + # `a['b']` and `a.b` name the same thing. Splitting on `.` alone left + # the subscript attached, so `execution['timestamp']` was looked up as a + # *task id* and reported as an undefined task -- on pipelines that run + # correctly. Same class as `now()` before #451, and it hits step outputs + # too: `get_full_context['keys']`. + var_ref = _SUBSCRIPT.sub(r".\1", var_ref) + var_ref = _INDEX.sub("", var_ref) + # Parse dotted variable reference parts = var_ref.split('.') if len(parts) < 1: diff --git a/src/orchestrator/validation/model_validator.py b/src/orchestrator/validation/model_validator.py index 3595969f..d7dee4b0 100644 --- a/src/orchestrator/validation/model_validator.py +++ b/src/orchestrator/validation/model_validator.py @@ -76,6 +76,11 @@ def summary(self) -> str: return f"Model validation: {', '.join(parts)}" +#: Parameter keys whose value names the model a step should run on. A dict +#: found anywhere else is data, however model-shaped its keys look. +MODEL_KEYS = frozenset({"model", "llm", "language_model"}) + + class ModelValidator: """Validates model requirements at compile time to prevent runtime errors. @@ -313,7 +318,26 @@ def _validate_model_specification( elif isinstance(model_spec, dict): # Handle model specification as dictionary model_name = model_spec.get("name") or model_spec.get("model") - if not model_name: + if model_name is not None and not isinstance(model_name, str): + # A nested mapping under `name:`/`model:` is not a model name. + # Returning it as one made `validated_models.add(...)` raise + # `TypeError: unhashable type: 'dict'`, which escaped the + # validator and was reported as "Model validation failed" -- + # i.e. as though the *pipeline* were invalid. Six catalogue + # examples were held down by it. + errors.append(ModelValidationError( + task_id=task_id, + model_requirement=str(model_spec), + error_type="invalid_model_type", + message=( + f"Model name must be a string, got " + f"{type(model_name).__name__}" + ), + context_path=context_path, + suggestions=["Use a string like 'openai/gpt-4'"], + )) + model_name = None + elif not model_name: errors.append(ModelValidationError( task_id=task_id, model_requirement=str(model_spec), @@ -498,14 +522,22 @@ def _validate_parameters_models( for key, value in parameters.items(): param_path = f"{context_path}.{key}" - if key in ["model", "llm", "language_model"] and isinstance(value, str): + if key in MODEL_KEYS and isinstance(value, str): # Direct model reference if value and not self._is_template_string(value): validated_models.add(value) elif isinstance(value, dict): - # Check if this looks like a model specification - if "model" in value or "name" in value: + # A model specification is one reached *under a model key*. + # This used to accept any nested dict carrying a `name` or + # `model` key, which makes a JSON Schema look like a model: + # + # parameters.schema.properties.records.items.properties + # -> {'id': {...}, 'name': {'type': 'string'}, ...} + # + # `name` there is a *field* called "name", and its value is a + # mapping, so the "model name" came back as a dict. + if key in MODEL_KEYS: result = self._validate_model_specification( value, task_id, param_path ) diff --git a/tests/test_triage_product_defects.py b/tests/test_triage_product_defects.py new file mode 100644 index 00000000..0f8c6239 --- /dev/null +++ b/tests/test_triage_product_defects.py @@ -0,0 +1,183 @@ +"""Three defects the catalogue triage found wearing example failures as a disguise. + +Classifying all 87 non-validating examples by their actual `validate` output +turned up three clusters that were not example problems at all. They are worth +a file together because they share a shape: each one reported *the pipeline* as +broken when the fault was ours, so anyone repairing the examples by hand would +have "fixed" files that were already correct. + +1. A `TypeError` escaped the model validator and was reported as + "Model validation failed", i.e. as though the pipeline were invalid. +2. `execution['timestamp']` was read as a task id, while `execution.timestamp` + -- the same reference -- validated. +3. `json_encode` did not exist. +""" + +import pytest + +from orchestrator.core.template_manager import TemplateManager +from orchestrator.validation.data_flow_validator import DataFlowValidator +from orchestrator.validation.model_validator import ModelValidator + +pytestmark = [pytest.mark.contract] + + +def _reference(ref, **kwargs): + kwargs.setdefault("task_id", "some_task") + kwargs.setdefault("parameter_name", "content") + kwargs.setdefault("task_schemas", {}) + kwargs.setdefault("pipeline_inputs", {}) + return DataFlowValidator()._validate_variable_reference(ref, **kwargs) + + +# --------------------------------------------------------------------------- +# 1. A JSON Schema is not a model specification +# --------------------------------------------------------------------------- + +#: The shape that crashed it: a schema whose `properties` names a field +#: called "name". Six catalogue examples carried one. +SCHEMA_PIPELINE = { + "id": "p", + "steps": [ + { + "id": "validate_data", + "action": "generate", + "parameters": { + "schema": { + "type": "object", + "properties": { + "records": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": {"type": "integer"}, + "name": {"type": "string"}, + "active": {"type": "boolean"}, + }, + }, + } + }, + } + }, + } + ], +} + + +def test_a_json_schema_is_not_read_as_a_model(): + """`properties.name` names a *field* called "name", not a model. + + Any nested dict carrying a `name` or `model` key used to be treated as a + model specification. Here that made the "model name" a dict, and + `validated_models.add(...)` raised `TypeError: unhashable type: 'dict'`. + """ + result = ModelValidator().validate_pipeline_models(SCHEMA_PIPELINE) + + assert result.is_valid, ( + "a JSON Schema was read as a model specification: " + f"{[e.message for e in result.errors]}" + ) + assert not result.validated_models, ( + f"a schema field was recorded as a model: {result.validated_models}" + ) + + +def test_the_validator_does_not_raise_on_a_schema(): + """The crash itself, separately from the diagnosis it produced. + + The `TypeError` escaped and surfaced as "Model validation failed", so the + report blamed the pipeline for a bug in the validator. + """ + try: + ModelValidator().validate_pipeline_models(SCHEMA_PIPELINE) + except TypeError as exc: # pragma: no cover - this is the regression + pytest.fail(f"the model validator raised instead of reporting: {exc}") + + +def test_a_model_under_a_model_key_is_still_found(): + """Narrowing what counts as a model must not stop finding real ones.""" + pipeline = { + "id": "p", + "steps": [ + { + "id": "think", + "action": "generate", + "parameters": {"model": "openai/gpt-4", "prompt": "hi"}, + } + ], + } + result = ModelValidator().validate_pipeline_models(pipeline) + assert "openai/gpt-4" in result.validated_models + + +def test_a_model_name_that_is_not_a_string_is_reported_not_raised(): + """Defence in depth for the same crash, at the other end. + + Scoping detection to model keys removes the way this was reached; a + mapping written directly under `model:` must still be a message. + """ + pipeline = { + "id": "p", + "steps": [ + { + "id": "think", + "action": "generate", + "parameters": {"model": {"name": {"nested": "mapping"}}}, + } + ], + } + result = ModelValidator().validate_pipeline_models(pipeline) + assert not result.is_valid + assert any("must be a string" in e.message for e in result.errors) + + +# --------------------------------------------------------------------------- +# 2. Two spellings of one reference +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "dotted,subscript", + [ + ("execution.timestamp", "execution['timestamp']"), + ("execution.started_at", "execution['started_at']"), + ('execution.id', 'execution["id"]'), + ], +) +def test_both_spellings_of_a_reference_agree(dotted, subscript): + """`a['b']` and `a.b` are the same reference and must be judged the same. + + Splitting on `.` left the subscript attached, so `execution['timestamp']` + was looked up as a *task id* -- on pipelines that run correctly. + """ + assert _reference(dotted)["valid"] == _reference(subscript)["valid"] is True + + +@pytest.mark.parametrize( + "reference", ["execution['bogus']", 'execution["strated_at"]'] +) +def test_a_typo_is_still_caught_in_the_subscript_spelling(reference): + """Normalising the spelling must not smuggle unknown fields past the check.""" + result = _reference(reference) + assert result["valid"] is False + assert result["error_type"] == "unknown_execution_field" + + +def test_an_index_is_not_a_separate_name(): + """`records[0]` refers to `records`, not to something called `records[0]`.""" + assert _reference("records[0]")["valid"] == _reference("records")["valid"] + + +# --------------------------------------------------------------------------- +# 3. json_encode +# --------------------------------------------------------------------------- + +def test_json_encode_is_the_same_function_as_to_json(): + """An alias, not a second implementation. + + Two implementations of one filter is exactly the drift #449 removed, and + the conformance sweep would catch them diverging -- but sharing the object + means they cannot. + """ + filters = TemplateManager().env.filters + assert filters["json_encode"] is filters["to_json"]