Uh oh!
There was an error while loading. Please reload this page.
feat: add output field constraints (enum, pattern, range, length, optional, nullable) - #372
Conversation
…d sanitize pydantic-ai tool schema
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Read through the constraint plumbing end to end and ran the new code against this branch to check the behaviour below.
The direction here is great. Extending the two shared builders in _schema.py and keeping a single validate_output is what makes cross-provider parity reachable at all, and routing pattern through Python re on every provider rather than letting pydantic's Rust engine diverge is a good call. Load-time validation is thorough, and test_output_schema.py asserting full-dict equality is the right instinct.
Two things stop it working as advertised, both on the Claude path, both invisible to the current tests. A nullable number that also carries minimum/maximum loses its null branch entirely. And a nullable object or array item is accepted by validate_output but rejected by the dynamic model, so the same YAML runs on four providers and fails on the fifth. I verified both against a real build_agent tool schema rather than by reading.
Separately, pattern is evaluated before minLength/maxLength, which puts a catastrophic regex within reach of model output. ^(a+)+$ under maxLength: 40 never returns on a 32 character input, and because re holds the GIL while backtracking it takes the event loop and every neighbouring agent with it.
If you only change one thing first, make it the parity fixture. It is what let the other two through: deleting all range and length enforcement from the Claude path leaves test_output_constraints_parity.py green, and so does deleting nullable handling outright.
Details inline. Several are small cleanups rather than problems, and I have marked which is which. Happy to talk any of them through.
Thanks again for your contributions!
| merged = dict(value_branch) | ||
| value_type = merged.pop("type", None) | ||
| if value_type is not None: | ||
| merged["type"] = [value_type, "null"] |
There was a problem hiding this comment.
NumberType is int | float, so a nullable number nests its union: anyOf: [{anyOf: [integer, number]}, {null}]. The value branch carries no type key, so merged.pop("type", None) returns None, line 83 is skipped, and the null branch is dropped without a word.
I ran a real build_agent for OutputField(type="number", nullable=True, minimum=0, maximum=10). The schema actually sent to Anthropic:
{"anyOf": [{"type":"integer"},{"type":"number"}], "ge":0.0, "le":10.0, "maximum":10.0, "minimum":0.0}No "null" anywhere, while build_json_schema_field advertises {"type": ["number","null"], ...} to every other provider. Same YAML, opposite contract. Without a range pydantic flattens to a 3-branch union and the null survives, which is exactly why the tests pass.
| merged=dict(value_branch) | |
| value_type=merged.pop("type", None) | |
| ifvalue_typeisnotNone: | |
| merged["type"] = [value_type, "null"] | |
| merged=dict(value_branch) | |
| value_type=merged.pop("type", None) | |
| ifvalue_typeisnotNone: | |
| merged["type"] = [value_type, "null"] | |
| elif"anyOf"inmerged: | |
| inner=merged.pop("anyOf") | |
| types= [b["type"] forbininnerifisinstance(b, dict) and"type"inb] | |
| iflen(types) ==len(inner): | |
| merged["type"] = [*types, "null"] | |
| else: | |
| merged["anyOf"] = [*inner, null_branch] | |
| else: | |
| merged["anyOf"] = [value_branch, null_branch] |
There was a problem hiding this comment.
Fixed. _convert_anyof_nullable now flattens the nested int | float union — when the value branch has no type but an anyOf whose branches all declare one, it emits type: ["integer", "number", "null"]; any unrecognized shape returns the node unchanged, so the null branch is never silently dropped. The regression test goes through a real build_agent and inspects the attached tool schema: a nullable ranged number now advertises {"type": ["integer", "number", "null"], "minimum": 0, "maximum": 10}.
| if field.type in ("string", "number", "integer", "boolean"): | ||
| base_type = _wrap_scalar_field_type(field, base_type) | ||
| field_info = _build_field_info(field) | ||
| return Annotated[base_type, field_info] | ||
| return base_type |
There was a problem hiding this comment.
Nullability is applied only to scalars here, but _build_pydantic_model applies it unconditionally. A nullable object or array item therefore diverges:
validate_output({"a": [None]}, ...)accepts it (output.py:138)output_schema_to_pydantic_model(...).model_validate({"a": [None]})raises
I confirmed both. The shared builder advertises items: {"type": ["object","null"]}, so this workflow runs on copilot, hermes, aca and claude_agent_sdk and hard-fails on claude.
"integer" is also unreachable: OutputField.type is Literal["string","number","boolean","array","object"]. Dropping it here is behaviour preserving.
| iffield.typein ("string", "number", "integer", "boolean"): | |
| base_type=_wrap_scalar_field_type(field, base_type) | |
| field_info=_build_field_info(field) | |
| returnAnnotated[base_type, field_info] | |
| returnbase_type | |
| iffield.typein ("string", "number", "boolean"): | |
| base_type=_wrap_scalar_field_type(field, base_type) | |
| field_info=_build_field_info(field) | |
| returnAnnotated[base_type, field_info] | |
| iffield.nullable: | |
| base_type=base_type|None | |
| returnbase_type |
There was a problem hiding this comment.
Fixed. _build_array_item_type now unions non-scalar item types with None when nullable: true, so {"rows": [None]} validates identically on the pydantic path and in validate_output (pinned by the new payload-matrix parity test). The unreachable "integer" entries are dropped from the new tuples; the pre-existing IntegerType branch in _map_output_field_type predates this PR and is left alone.
| suggestion=f"Ensure '{field_name}' is one of {field_def.enum!r}", | ||
| ) | ||
| if field_def.pattern is not None and re.search(field_def.pattern, value) is None: |
There was a problem hiding this comment.
pattern runs before the minLength/maxLength block below, so a bounded field is not actually bounded when the regex sees it. Model output is untrusted input, and Python's re has no timeout.
OutputField(type="string", pattern="^(a+)+$", maxLength=40) loads without complaint. A 32 character input then never returns. I let it run 40 seconds before killing it.
The part that makes this more than a slow function: re holds the GIL while it backtracks. My watchdog thread could not even print its timeout message. validate_output is called synchronously from executor/agent.py, so this stalls the event loop, every agent in a parallel: or for_each: group, and the Esc-to-pause listener. The run does not fail, it just stops.
Worth noting that reordering alone is not enough. 32 characters is already under maxLength: 40. Options that do work: reject nested quantifiers at load time, or run search against a deadline and raise ValidationError.
There was a problem hiding this comment.
Fixed on both axes. Length checks now run before the pattern check, and matching moved from stdlib re to the regex engine under a 1s wall-clock deadline (PATTERN_MATCH_TIMEOUT_SECONDS). A timeout raises ValidationError here, and a ValueError inside the pydantic AfterValidator, so it drives the in-session output retry instead of hanging the run. Verified empirically: ^(a|aa)+$ on a 61-char input raises in ~1s; your ^(a+)+$ example is optimized by the regex engine and returns instantly. The compiled pattern is cached on OutputField.compiled_pattern, so the deadline has a single home.
| if field.enum is not None: | ||
| schema["enum"] = field.enum |
There was a problem hiding this comment.
type and enum are conjunctive in JSON Schema, so {"type": ["string","null"], "enum": ["a","b"]} forbids null regardless of the type list. Meanwhile output.py:115 returns early for None and the pydantic model accepts it too, so every enforcement layer permits a value every published schema rejects.
This combination is not hypothetical. schema.py:161 refuses null inside enum and tells the author to use nullable: true instead, so the validator points people straight at it.
| iffield.enumisnotNone: | |
| schema["enum"] =field.enum | |
| iffield.enumisnotNone: | |
| schema["enum"] =[*field.enum, None] iffield.nullableelsefield.enum |
Same change is needed in build_prompt_schema_field and in _field_json_schema_extra.
There was a problem hiding this comment.
Fixed in all three emission points — both _schema.py flavours and _field_json_schema_extra. A nullable enum now emits [*enum, None], so the advertised schema no longer forbids the null that every enforcement layer accepts. The author-facing rule is unchanged: YAML enum still cannot contain null (rejected at load), nullable: true remains the sole mechanism.
| @classmethod | ||
| def model_json_schema(cls, *args: Any, **kwargs: Any) -> dict[str, Any]: | ||
| schema = super().model_json_schema(*args, **kwargs) | ||
| _sanitize_json_schema(schema) | ||
| return schema |
There was a problem hiding this comment.
This override is never called. I wrapped it in a spy and ran a real build_agent with a rich schema: zero calls. Deleting it leaves the final tool schema byte identical, and so does replacing the whole class with BaseModel plus ConfigDict(extra="allow").
_build_output_type always wraps the model in ToolOutput, and pydantic-ai builds tool schemas through its own GenerateJsonSchema pipeline, which does not route through Model.model_json_schema. The real sanitisation is the pass in agent_builder.py.
Two knock-on effects worth deciding on together. The docstring above describes stripping default, $ref, $defs and flattening anyOf, none of which happens here. And because agent_builder.py is the only production caller, the unresolvable-$ref branch below can never be reached by the second pass its own docstring promises.
There was a problem hiding this comment.
Confirmed with a spy through a real build_agent — zero calls, and deleting the override leaves the attached tool schema byte-identical. Removed it, renamed the class to _OutputBaseModel (its remaining job is extra="allow"), and corrected both docstrings, including the phantom final-inlining-pass promise on _sanitize_schema_node (the agent_builder.py pass is the only one).
| if isinstance(value, bool): | ||
| if not (field_def.type == "boolean" and value in field_def.enum): | ||
| raise ValidationError( | ||
| f"Output field '{field_name}' must be one of {field_def.enum!r}, got {value!r}", | ||
| suggestion=f"Ensure '{field_name}' is one of {field_def.enum!r}", | ||
| ) | ||
| elif value not in field_def.enum: | ||
| raise ValidationError( | ||
| f"Output field '{field_name}' must be one of {field_def.enum!r}, got {value!r}", | ||
| suggestion=f"Ensure '{field_name}' is one of {field_def.enum!r}", | ||
| ) |
There was a problem hiding this comment.
_check_constraints runs only after check_type passed, and check_type admits a bool solely when type == "boolean". The first conjunct on line 58 is therefore always true, making this branch an exact duplicate of the elif below, same message and all.
| ifisinstance(value, bool): | |
| ifnot (field_def.type=="boolean"andvalueinfield_def.enum): | |
| raiseValidationError( | |
| f"Output field '{field_name}' must be one of {field_def.enum!r}, got {value!r}", | |
| suggestion=f"Ensure '{field_name}' is one of {field_def.enum!r}", | |
| ) | |
| elifvaluenotinfield_def.enum: | |
| raiseValidationError( | |
| f"Output field '{field_name}' must be one of {field_def.enum!r}, got {value!r}", | |
| suggestion=f"Ensure '{field_name}' is one of {field_def.enum!r}", | |
| ) | |
| ifvaluenotinfield_def.enum: | |
| raiseValidationError( | |
| f"Output field '{field_name}' must be one of {field_def.enum!r}, got {value!r}", | |
| suggestion=f"Ensure '{field_name}' is one of {field_def.enum!r}", | |
| ) |
_make_enum_validator in converters.py carries the same mirrored branch. Worth collapsing both together so the two enforcement paths stay in step.
There was a problem hiding this comment.
Collapsed in both places — _check_constraints and the mirrored _make_enum_validator are now a single plain-membership check each. The pinned semantics are unchanged and tested on both paths: a number enum: [1] rejects True (via the type check / _reject_bool before the enum check) and accepts 1.0.
| if item is None and field_def.items.nullable: | ||
| continue |
There was a problem hiding this comment.
The recursive call on line 147 passes the parent array's name through, so a constraint failure on element 37 reports Output field 'tags' must be one of [...] with no index, while the type error four lines above correctly says Array item 37 in 'tags'. Two different conventions inside one loop.
None of the six _check_constraints messages use _describe_value either, so the offending value never appears. That helper was added precisely because naming only the types makes log-only diagnosis impractical, and the same reasoning applies here.
Passing f"array item {i} in '{field_name}'" into the recursive call would line both up.
There was a problem hiding this comment.
Done. The recursion now passes array item {i} in '{field}', so a constraint failure on element 37 names the index, lining up with the existing type-error convention (which is unchanged). The pattern/length/range messages also include the offending value via _describe_value (containers reduced to shape, scalars repr-truncated at 200 chars).
| if field.minLength is not None: | ||
| extra["minLength"] = field.minLength | ||
| if field.maxLength is not None: | ||
| extra["maxLength"] = field.maxLength |
There was a problem hiding this comment.
These two are already emitted by pydantic from the min_length/max_length kwargs in _build_field_info. I ablated _field_json_schema_extra to lambda field: None and diffed: the length keys were identical across plain, nullable, array-item and nested-object shapes, contributing nothing.
enum, pattern, minimum and maximum are load bearing and must stay, so this is a small trim rather than a rewrite.
Related, and more visible to users: Field(ge=..., le=...) cannot attach to the int | float union, so pydantic emits raw ge/le alongside the correct minimum/maximum. They are pydantic-isms rather than JSON Schema keywords and they reach the wire.
There was a problem hiding this comment.
Both done. minLength/maxLength are removed from json_schema_extra (pydantic already emits them from the Field kwargs — your ablation finding holds), and the raw ge/le keys are stripped in the sanitize pass while the standard minimum/maximum remain; a tool-schema hygiene test asserts recursively that neither ge/le nor default reaches the attached schema.
| if value is None: | ||
| return value |
There was a problem hiding this comment.
This guard cannot fire. _wrap_scalar_field_type attaches the AfterValidator to base_type before the | None union is added, so in Annotated[str, AfterValidator(...)] | None a None matches the NoneType member and the validator is never entered. I counted 396 validations across enum and pattern, string, number and boolean, nullable and not: value is None was reached zero times.
The same applies to the None guard in _make_pattern_validator.
There was a problem hiding this comment.
Removed from both _make_enum_validator and _make_pattern_validator — the NoneType union member short-circuits before the AfterValidator runs, exactly as you measured.
| { | ||
| name: field.model_dump(mode="json", exclude_none=True, exclude_defaults=True) | ||
| for name, field in agent.output.items() | ||
| } |
There was a problem hiding this comment.
exclude_defaults=True gives the wire format the meaning "absent equals whatever the receiver's default is". Both sides agree today and I checked the nested round trip is lossless, so nothing is broken right now.
The part worth a decision is version skew. The in-container runner reconstructs through OutputField.model_validate, which has no extra="forbid", so an older runner silently drops enum, pattern, minimum and the rest. The inner Copilot session then builds a prompt schema with no constraints, the model is never told about them, the session closes, and the host rejects the output afterwards with no chance at the in-session recovery the #343 contract is built around. The only signal is _warn_on_version_skew, whose text never mentions output.
An output_constraints capability on /health, with a clear ProviderError when an agent declares constraints the runner cannot honour, would turn a quiet degradation into something actionable.
There was a problem hiding this comment.
Agreed this deserves a proper design, and I would rather not grow this PR further — an output_constraints capability on /health with a clear ProviderError is a protocol change on both host and runner. Partial mitigation landed here: extra="forbid" on OutputField makes a newer-host/older-runner skew fail loudly at reconstruction instead of silently degrading. Want me to open a follow-up issue for the capability handshake?
…ld constraints Claude-path parity blockers: - Keep the null branch when a nullable number carries minimum/maximum: pydantic nests the int|float union, and _convert_anyof_nullable dropped the null branch for such shapes. Nested unions now flatten to type: [integer, number, "null"], and unrecognized shapes are returned unchanged so null is never silently lost. - Accept nullable object and array items in the dynamic model, matching validate_output; nullable was previously applied only to scalar items, so the same YAML passed on four providers and failed on claude. Pattern matching safety: - Evaluate patterns with the regex engine (re-compatible) under a 1s wall-clock deadline (PATTERN_MATCH_TIMEOUT_SECONDS): a pathological pattern on model output now raises instead of stalling the event loop, and the pydantic path surfaces it as a ValueError driving the in-session output retry. Length checks now run before pattern matching. - Cache the compiled pattern on OutputField.compiled_pattern. Schema honesty: - Emit enum with null appended when a field is both enum-constrained and nullable, in both shared builders and the pydantic json_schema_extra; type and enum are conjunctive in JSON Schema, so the previous shape forbade the null that every enforcement layer accepted. - Strip pydantic-internal ge/le keys from the generated tool schema (the standard minimum/maximum remain). - Reject unknown OutputField keys (extra="forbid") so a constraint typo fails at load instead of silently unconstraining the field. - Keep integral minimum/maximum as int (no 0.0 in schemas and messages). Cleanups: - Remove the dead model_json_schema override (pydantic-ai never calls it; agent_builder performs the real tool-schema sanitization) and rename the base model to _OutputBaseModel. - Collapse unreachable bool/None branches in the enum/pattern validators. - Name the element index in array-item constraint errors and include the offending value in pattern/length/range messages. - Warn on undeclared output keys so a misspelled optional key is visible instead of silently dropped. Tests: the parity fixture now co-locates the previously-breaking combinations (nullable+range, enum+nullable, nullable object items, nested optional properties), a payload matrix asserts the Claude dynamic model and validate_output agree on accept/reject, and tool-schema hygiene tests pin the absence of ge/le/default keys through the real build_agent seam.
hertznsk
commented
Aug 6, 2026
Thank you for the thorough review — the mutation testing of the parity fixture in particular caught real gaps. Both blockers are fixed and verified through a real The ReDoS vector is closed by moving pattern matching to the The parity fixture is rebuilt around your four cases, plus a 16-payload accept/reject agreement matrix between the dynamic model and Two items intentionally not in this PR: the event-based retry for undeclared-key typos (a warning naming the keys landed instead), and the ACA Everything else is addressed inline in the threads. Full suite green: 4969 passed, lint/typecheck/examples clean. |
hertznsk
commented
Aug 6, 2026
Holding this PR for now — while dogfooding this branch on my own workflows I ran into a problem with the undeclared-fields warning, and I want to think it through before merge.
The soft alternative to extra-key rejection clearly needs more thought. Options I see: scope the warning to agent/provider steps only, exclude the script baseline keys, or drop the warning here and leave extra-key handling to the follow-up where it was planned. Jason Robert (@jrob5756) curious about your take before I push anything. |
The undeclared-keys warning is now opt-in via warn_undeclared_keys on validate_output. Provider-backed agent call sites enable it; script and set step validation leave it off. This eliminates the two main false-positive classes: script steps always carry the injected stdout/stderr/exit_code baseline keys, and shared scripts or set steps may intentionally emit a superset of the declared schema.
hertznsk
commented
Aug 9, 2026
Pushed a resolution for the warning scoping problem (9c76781), taking option A from my earlier comment. What changed: Why this shape: the silent-data-loss repro above was specific to a non-deterministic producer — a model misspelling an optional key under Tests pin all three behaviors: a provider-path typo warns; script baseline keys and superset payloads stay silent; set-step extras stay silent. Full suite green: 4974 passed, lint/typecheck clean. This also sharpens the follow-up picture for real extra-key handling: given baseline keys and the superset-reuse pattern, rejection cannot be a default — it would have to be opt-in closed-object semantics ( Jason Robert (@jrob5756) — ready for another pass when you have a chance. |
…credentials TestAcaWireBoundary called AcaRuntimeProvider._build_request, which resolves the inner Copilot credential from the host environment (COPILOT_PROVIDER_BASE_URL -> GitHub token env vars -> gh auth token). The tests passed locally thanks to a signed-in gh but failed in CI, where no credential is available, with ProviderError. Add an autouse fixture pinning COPILOT_PROVIDER_BASE_URL so the resolver takes the BYOK branch and never shells out to gh, making the tests independent of machine auth state.
hertznsk
commented
Aug 10, 2026
Heads up Jason Robert (@jrob5756) — I noticed the CI test job failed on the latest run. That was my mistake. The new Fixed in 1b1dd08: an autouse fixture now pins |
…example The `conductor` skill's yaml-schema.md is the reference Conductor ships so Conductor-aware agents can author workflows. It documented only type, description, items and properties, so an agent working from it would never emit the new constraint keywords and could strip them as invalid. - yaml-schema.md: add the eight keywords to the agent output block and a Field Constraints section covering applicability, load-time rejection of illegal combinations, and the StrictUndefined guards for nullable and optional fields. - examples/output-constraints.yaml: guard the two fields that need it. `notes` is nullable, so an unguarded null rendered the literal string "None" into the workflow result; `details.comments` is `required: false`, which raises a template error when omitted. The example declared that property and never referenced it, so the trap was invisible. - CHANGELOG: entry under Unreleased. Note the bundled skill now injects 128,938 bytes against the 131,072-byte runtime.skill_injection.max_bytes default, leaving ~2KB of headroom for eager-injection providers (claude, hermes). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #372 +/- ##
=======================================
Coverage ? 91.16% =======================================
Files ? 103 Lines ? 16554 Branches ? 0 =======================================
Hits ? 15091 Misses ? 1463 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Summary
Workflow authors can now declare real constraints in their agents'
output:blocks — allowed value lists (enum), regex patterns (pattern), numeric ranges (minimum/maximum), string length limits (minLength/maxLength), optional fields (required: false), and nullable fields (nullable: true). Every provider (Claude, Copilot, Hermes, Claude Agent SDK, ACA) enforces them the same way, and on Claude the model gets an automatic in-session chance to fix a constraint violation before the workflow fails.What changed
OutputFieldschema model gains eight optional fields (enum,pattern,minimum,maximum,minLength,maxLength,required,nullable) with Pydantic validators rejecting type-inappropriate combinations (e.g.patternontype: number,Noneentries inenum— usenullable: trueinstead)._schema.py) emit the new keywords in both flavours (JSON schema and prompt schema);requiredarrays list only required fields;nullablerenders astype: [<T>, "null"]; nodefaultkeys ever appear in generated schemas.AfterValidator(strict Python semantics identical to the final validator — noLiteral[...]/ Rust-regex surprises) while advertising them to the model viajson_schema_extra. Constraint violations raise inside the pydantic-ai output-retry loop, giving the model a free in-session self-correction. Optional fields useexclude_unset=Trueso an omitted key is never materialized asNone.validate_output) checks all eight constraints after the type check, recursion-aware (object properties and array items). Legacy error messages remain byte-identical when no new fields are used.required: falseis rejected at load time via aWorkflowConfigmodel validator (optional output fields are only allowed inside object properties).output_formatpayload, pydantic constraints on the Claude dynamic model, and identicalValidationErrorfromvalidate_output. The ACA wire boundary is covered too (constraint fields survive the round trip).docs/workflow-syntax.mdgains a "Field constraints" subsection; new exampleexamples/output-constraints.yamlexercising every field; CHANGELOG entry under Unreleased.Deliberately out of scope
additionalProperties: false/ extra-key rejection (deferred to a follow-up).multipleOf,format,const, etc.).Test plan
uv run pytest -m "not performance" -q— full suite greenmake lint/make typecheck— cleanmake validate-examples— greenFunctionModelreturning a violating payload then a valid one)