Uh oh!
There was an error while loading. Please reload this page.
fix(executor): validate nested array item output schemas recursively - #337
Conversation
validate_output recursed into object properties but only type-checked top-level array items, so an array<object> output with missing or mistyped nested fields passed validation silently. Extract a recursive per-field helper so array items are validated at every depth, matching the schema providers already send to the model and the recursion the object branch has always had. Existing error message shapes are unchanged. One integration-test mock that violated its own declared array<object> schema is brought into conformance.
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
The recursive validation fix is correct and closes a real contract gap. The remaining comments are non-blocking suggestions around diagnostics, documentation, and broader call-site coverage. Approved.
| if field_def.type == "array" and field_def.items and isinstance(value, list): | ||
| for i, item in enumerate(value): | ||
| if not _check_type(item, field_def.items.type): | ||
| raise ValidationError( | ||
| f"Array item {i} in '{field_name}' has wrong type: " | ||
| f"expected {field_def.items.type}, got {type(item).__name__}", | ||
| suggestion=f"Ensure all items in '{field_name}' have correct type", | ||
| ) | ||
| _validate_field(field_name, item, field_def.items) |
There was a problem hiding this comment.
Passing field_name down unchanged means the index and the name end up describing different levels. For array<array<object>> with the bad element at matrix[1][0], you get Array item 0 in 'matrix', and matrix[0] is perfectly valid. This path was unreachable before the PR, so there is no message to preserve here.
The one-line change below addresses the array case. The object case still reports a bare Missing required output field: score with no array or index, because validate_output re-derives names from the nested schema keys. Threading an optional _path prefix through validate_output fixes both and lets the double type-check collapse into one call.
| iffield_def.type=="array"andfield_def.itemsandisinstance(value, list): | |
| fori, iteminenumerate(value): | |
| ifnot_check_type(item, field_def.items.type): | |
| raiseValidationError( | |
| f"Array item {i} in '{field_name}' has wrong type: " | |
| f"expected {field_def.items.type}, got {type(item).__name__}", | |
| suggestion=f"Ensure all items in '{field_name}' have correct type", | |
| ) | |
| _validate_field(field_name, item, field_def.items) | |
| iffield_def.type=="array"andfield_def.itemsandisinstance(value, list): | |
| fori, iteminenumerate(value): | |
| # Checked here as well as in the recursive call below, so an element type | |
| # mismatch keeps the indexed "Array item {i}" wording instead of falling | |
| # through to the generic "Output field" one. | |
| ifnot_check_type(item, field_def.items.type): | |
| raiseValidationError( | |
| f"Array item {i} in '{field_name}' has wrong type: " | |
| f"expected {field_def.items.type}, got {type(item).__name__}", | |
| suggestion=f"Ensure all items in '{field_name}' have correct type", | |
| ) | |
| _validate_field(f"{field_name}[{i}]", item, field_def.items) |
| Recursively validates nested object properties and array items so | ||
| ``array<object>`` and deeper combinations are checked at every depth, | ||
| matching the recursion the object branch has always had. | ||
| Args: | ||
| field_name: Field name used in error messages (array items keep the | ||
| parent array's name, matching the existing message style). |
There was a problem hiding this comment.
"matching the recursion the object branch has always had" describes the change rather than the contract, and it will read as stale once nobody remembers what the object branch used to do. The two branches also do not match: one recurses through validate_output, the other through _validate_field.
The field_name note is also not accurate for the paths this PR adds. Nested object properties are renamed from the nested schema keys, so the parent name is dropped entirely.
| Recursivelyvalidatesnestedobjectpropertiesandarrayitemsso | |
| ``array<object>``anddeepercombinationsarecheckedateverydepth, | |
| matchingtherecursiontheobjectbranchhasalwayshad. | |
| Args: | |
| field_name: Fieldnameusedinerrormessages (arrayitemskeepthe | |
| parentarray'sname, matchingtheexistingmessagestyle). | |
| Checks``value``against``field_def.type``, thenrecurses: objectvaluesare | |
| validatedagainst``field_def.properties``andarrayelementsagainst | |
| ``field_def.items``. Fieldsdeclared``array``without``items``or``object`` | |
| without``properties``arecheckedonlyattheirownlevel; theircontentsare | |
| acceptedunexamined. | |
| Args: | |
| field_name: Nameusedinerrormessages. Arrayelementsareaddressedas | |
| ``name[i]``; nestedobjectpropertiesarerenamedby``validate_output`` | |
| fromthenestedschemakeys. |
| """Validate agent output against declared schema. | ||
| Checks that all required fields are present and have the correct types. | ||
| Nested object properties and array items are validated recursively. |
There was a problem hiding this comment.
Worth spelling out the two things a caller cannot infer here, since both decide whether an existing workflow starts failing. OutputField has no optional or nullable flag, so declaring items.properties makes every one of those keys mandatory on every element. And an array declared without items still accepts anything, including nulls and mixed types.
| Nestedobjectpropertiesandarrayitemsarevalidatedrecursively. | |
| Nestedobjectpropertiesandarrayitemsarevalidatedrecursively, atevery | |
| depth. Everyfieldaschemadeclaresisrequired (``OutputField``hasnooptional | |
| flag), sodeclaring``items.properties``requiresthosekeysoneveryelement. | |
| Arrayswithout``items``andobjectswithout``properties``passtheircontents | |
| throughunchecked. |
| def test_validate_output_does_not_call_check_type_directly(self) -> None: | ||
| """After refactor all value checks must flow through _validate_field.""" | ||
| import inspect | ||
| from conductor.executor.output import _validate_field, validate_output | ||
| validate_source = inspect.getsource(validate_output) | ||
| validate_field_source = inspect.getsource(_validate_field) | ||
| assert "_check_type(" not in validate_source | ||
| assert "_check_type(" in validate_field_source |
There was a problem hiding this comment.
I ran the mutation check: comment out the recursive _validate_field call in output.py and four of the seven new tests fail, but this one still passes. It greps source text, so it stays green against a validator with the fix removed, and it goes red on a rename or on the path refactor suggested above. The behavioural tests above it already cover the invariant.
| deftest_validate_output_does_not_call_check_type_directly(self) ->None: | |
| """After refactor all value checks must flow through _validate_field.""" | |
| importinspect | |
| fromconductor.executor.outputimport_validate_field, validate_output | |
| validate_source=inspect.getsource(validate_output) | |
| validate_field_source=inspect.getsource(_validate_field) | |
| assert"_check_type("notinvalidate_source | |
| assert"_check_type("invalidate_field_source |
| ] | ||
| } | ||
| with pytest.raises(ValidationError, match="wrong type"): |
There was a problem hiding this comment.
match="wrong type" matches both message forms at every depth, so this test cannot tell you which message fired or at what level. That matters for a PR whose stated invariant is byte-for-byte message preservation. Tightening it also turns this test into a real detector for the index/name mismatch flagged in output.py.
| withpytest.raises(ValidationError, match="wrong type"): | |
| withpytest.raises( | |
| ValidationError, match=r"Output field 'v' has wrong type: expected number, got str" | |
| ): |
| built = build_json_schema_field(inner) | ||
| assert isinstance(built, dict) |
There was a problem hiding this comment.
isinstance(built, dict) is trivially true against the -> dict[str, Any] annotation. The build call earns its keep, the assertion does not, and the reason for building at all is invisible.
| built=build_json_schema_field(inner) | |
| assertisinstance(built, dict) | |
| # Building first asserts this depth is legal for a provider schema: 5 | |
| # array/object pairs sit exactly on _schema.py's max_depth=10 limit. | |
| build_json_schema_field(inner) |
| def test_array_without_items_unchanged(self) -> None: | ||
| """Arrays declared without items keep historical passthrough behavior.""" | ||
| schema = {"tags": OutputField(type="array")} | ||
| content = {"tags": [1, "two", {"three": 3}]} | ||
| validate_output(content, schema) |
There was a problem hiding this comment.
Good test, and the right guardrail for a tightening change. Two siblings are worth adding next to it. A JSON null inside an array item is the likeliest real-world break from this PR, since models emit null for unknown fields constantly, and it currently produces a type error rather than a missing-field error:
deftest_null_inside_array_item_field_raises(self) ->None:
"""JSON null in an array item field is a type error, not an absent field."""schema= {
"findings": OutputField(
type="array",
items=OutputField(
type="object", properties={"title": OutputField(type="string")}
),
)
}
withpytest.raises(ValidationError, match=r"expected string, got NoneType"):
validate_output({"findings": [{"title": None}]}, schema)An empty list and items: {type: object} with no properties both pass today and are also unpinned.
| validate_output(content, schema) | ||
| class TestValidateOutputArrayRecursion: |
There was a problem hiding this comment.
"(issue regression)" names no issue. The convention elsewhere in the repo is to cite the number, for example engine/workflow.py:1111 ("issue #118"), which is actually followable.
| classTestValidateOutputArrayRecursion: | |
| """Tests for recursive validation of array item schemas (issue #NNN).""" |
| validate_output(content, schema) | ||
| def test_array_of_objects_missing_nested_field_raises(self) -> None: | ||
| """Missing required field inside an array item must raise (previously silently accepted).""" |
There was a problem hiding this comment.
"previously silently accepted" is true relative to one commit and will not be in a year. Same pattern on line 281 ("pinning general recursion").
| """Missing required field inside an array item must raise (previously silently accepted).""" | |
| """Missing required field inside an array item must raise.""" |
| if agent.name == "planner": | ||
| return {"plan": [{"step": "1"}], "confidence": 0.9} | ||
| return {"plan": [{"step": "1", "action": "Research"}], "confidence": 0.9} |
There was a problem hiding this comment.
I reverted this and watched test_existing_full_workflow_unchanged fail, so the fix is real. But it reads as arbitrary test data, and the test name points away from the cause. One line of context saves the next person trimming this fixture.
| return {"plan": [{"step": "1", "action": "Research"}], "confidence": 0.9} | |
| ifagent.name=="planner": | |
| # valid_full.yaml declares plan.items.properties = {step, action}, and | |
| # every declared property is required, so both keys must be present. | |
| return {"plan": [{"step": "1", "action": "Research"}], "confidence": 0.9} |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Continues the output-schema tightening series started in #311 (shared builders extracted into
_schema.py) and #317 (Copilot and Hermes aligned on the shared recursive prompt-schema builder). Those PRs made the schema that providers send to the model fully recursive — butvalidate_output(executor/output.py), which checks the model's response against the declaredoutput:contract, still only type-checked top-levelarrayitems. Anarray<object>(or deeper) output with missing or mistyped nested fields therefore passed validation silently, even though the model had received the full nested schema.This PR closes that gap: the per-field validation logic is extracted into a recursive
_validate_fieldhelper that recurses into bothobject.properties(existing behavior, moved verbatim) andarray.items(new), so nested array item schemas are enforced at every depth — in all fivevalidate_outputcall sites at once (LLM agents, Claude, Hermes,setsteps,scriptsteps).Behavior change
This is a tightening bugfix: workflows whose agents,
setsteps, orscriptsteps emitarray<object>outputs that violate the declared nested schema will now fail withValidationErrorinstead of passing silently. The declaredoutput:contract is finally enforced the same way the schema builders already advertise it to the model.Compatibility
objectnesting: byte-identical behavior and error messages. All pre-existing validation tests pass unmodified, pinning the message shapes (Missing required output field: ...,Output field '...' has wrong type: ...,Array item N in '...' has wrong type: ...).executor/output.pyand propagates through the existing call sites.validate_output: schemas deeper than the builders'max_depth=10are still rejected at schema-build time, before validation ever runs.itemskeep their historical passthrough.Explicitly out of scope
additionalProperties: falseenforcement (follow-up PR).OutputFieldfields (enum,pattern,required,nullable, min/max).findings[2].severity) — messages stay byte-compatible here; a separate diagnostics PR will add paths.Test plan
TestValidateOutputArrayRecursion: validarray<object>passes, missing nested field raises, wrong nested type raises,array<array<object>>deep error raises, no-itemspassthrough preserved, builder-boundary depth-10 schema (5 array/object pairs, verified acceptable tobuild_json_schema_field) rejects invalid content withoutRecursionError, and a source-inspection guard thatvalidate_outputnever calls_check_typedirectly (all value checks flow through_validate_field).test_workflows.py) that violated its own declaredarray<object>schema (missing requiredactioninside aplanitem) is brought into conformance — a one-line mock fix, no test-semantics change. This was the only in-repo consumer relying on the previously silent acceptance.make test: 4338 passed.make check(ruff + format + ty): clean.make validate-examples: green.