feat: bump jsonschema-pydantic-converter to 0.4.1, add datamodel-code-generator backend - #1055
feat: bump jsonschema-pydantic-converter to 0.4.1, add datamodel-code-generator backend#1055vldcmp-uipath wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a feature-flagged alternative backend for converting JSON Schema into runtime Pydantic models in the ReAct agent stack, keeping the existing create_model/create_output_model façade and contracts stable for all callers.
Changes:
- Introduces a new
datamodel-code-generator-based conversion backend selected by theAgentSchemaCodegenEnabledfeature flag. - Refactors
$refhandling into backend-agnostic helpers and splits legacy vs. codegen implementations into separate modules. - Expands the conversion contract test suite to run against both backends and adds scenario tests covering previously problematic shapes (PC-4965) and runtime lookup contracts.
File summaries
| File | Description |
|---|---|
uv.lock |
Updates lockfile for new dependency and version bumps. |
pyproject.toml |
Adds datamodel-code-generator dependency and raises jsonschema-pydantic-converter minimum version. |
tests/agent/react/test_jsonschema_pydantic_converter.py |
Runs existing contract tests against both backends via an autouse flag fixture. |
tests/agent/react/test_jsonschema_pydantic_converter_scenarios.py |
Adds scenario-based tests covering runtime contracts and backend differences. |
tests/agent/react/test_job_attachments.py |
Switches schema→model creation to the façade (create_model) and widens an assertion to accept backend differences. |
src/uipath_langchain/agent/react/jsonschema_pydantic_converter.py |
Becomes a façade: feature-flag dispatch + exports of shared $ref helpers. |
src/uipath_langchain/agent/react/_schema_refs.py |
New shared $ref resolution + dangling-ref neutralization helpers. |
src/uipath_langchain/agent/react/_legacy_converter.py |
Extracts the previous jsonschema-pydantic-converter implementation into its own module. |
src/uipath_langchain/agent/react/_codegen_converter.py |
New codegen backend with pseudo-module publication, $ref marker tagging, and constraint guarding. |
src/uipath_langchain/agent/react/_codegen_base.py |
Shared base model ensuring serialize_by_alias and default extra behavior. |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The codegen backend uses jsonschema to enforce `not`, `prefixItems` and an empty `enum`, which have no Pydantic annotation. It was only available transitively, and the import sat inside a try/except that logged a warning and returned the model unguarded -- so an install without it would have silently stopped validating those fields. jsonschema is now a declared dependency, imported at module level, and the degradation path is gone: a missing dependency fails loudly at import instead of quietly weakening validation. types-jsonschema joins the dev group, alongside the existing types-protobuf, so the calls are type-checked rather than ignored. Also drops the module-level loggers from both backends. The only logging call lives in the facade now, so they were dead. Raised by review on #1055. Co-Authored-By: Claude <noreply@anthropic.com>
SonarCloud failed the quality gate on the codegen backend: three functions over the cognitive-complexity limit (45, 25 and 22 against 15 allowed) and the "Invalid schema" error title written out three times. The complexity all sat in nested recursive walkers that mixed traversal with the work done at each node. Traversal is now its own generator per walker, so each visit() decides what to do with one node and delegates finding the next: - _tag_referenced_models: _child_nodes() yields each subschema with the models it describes, and _models_for_property() resolves a property to its models - _unenforceable_constraints: _child_paths() yields each subschema with the path to its values, and _is_unenforceable() answers the per-node question - _build_constraint_guard: _values_at_path() moves out to the module level The error title becomes a constant behind an _invalid_schema() helper, which the three raise sites share. Also swaps the constraint validator from a classmethod-wrapped lambda to the plain callable a "before" model validator accepts, which drops an unused argument and a type: ignore, and requests the dual-backend fixture through pytestmark instead of autouse, as the analyzer suggested. No behaviour change: 2830 passed, and the suites still run every test against both backends (70 legacy / 68 codegen variants). Raised by review on #1055. Co-Authored-By: Claude <noreply@anthropic.com>
f6eed11 to
b3c2a39
Compare
vldcmp-uipath
changed the title
b3c2a39 to
6e651d1
Compare
vldcmp-uipath
changed the title
There was a problem hiding this comment.
Reviewed the code-generator backend by running both backends over the same schemas and comparing observable behaviour. The 0.4.1 pin is fine — I reproduced the issue on 0.4.0 in a throwaway venv and confirmed it gone on 0.4.1. The pin alone fixes it and needs no code change.
The second backend does not hold the equivalence the description claims. Flipping the flag changes observable behaviour in several ways that are not in TestBackendDifferences, and two of them break tool execution outright. Everything below is flag-on only, so the default path is unaffected and this is not a merge risk today — but it blocks enabling the flag anywhere.
The suite does not catch any of it: tests/agent passes with UIPATH_FEATURE_EnableDatamodelCodeGeneratorConverter=true except TestBackendSelection::test_defaults_to_the_legacy_backend, which asserts the default.
Blocking:
- non-identifier property names break every tool call
format: passwordsends**********instead of the value- a
$defsname matching the roottitlereturns the wrong model customTypePathin a schema imports and executes an arbitrary module
Also worth noting: FeatureFlags.configure_flags({}) merges rather than clears — only reset_flags() restores the default. The schema_backend fixture gets this right; ad-hoc probes are easy to get wrong.
Details inline.
| ) | ||
| module_name = f"{_DYNAMIC_MODULE_PREFIX}_gen_{next(_dynamic_module_counter)}" | ||
| try: | ||
| models = generate_dynamic_models( |
There was a problem hiding this comment.
Blocking — schema content can make the generated-and-execd source import an arbitrary module.
datamodel-code-generator parses customTypePath (parser/jsonschema.py:857, a first-class aliased field, no opt-in) and x-python-type, and emits them as a real from <module> import <name> into the source dynamic.py execs. The sanitizers here only normalise names and never see these keys; nothing upstream filters schema keys.
Reproduced:
{"title":"T","type":"object",
"properties":{"a":{"type":"string","customTypePath":"marker_mod.Thing"}}}
# -> marker_mod imported, its top-level code ranand the type-confusion variant:
{"properties":{"a":{"type":"string","customTypePath":"builtins.object"}}}
# -> annotation becomes `object`, arbitrary_types_allowed flipped to True,
# model_validate({"a": 12345}) succeeds on a declared stringLegacy is immune — Optional[str], nothing executed.
The value is constrained to a dotted identifier path, so this is "import an arbitrary named module", not token injection. It still runs that module's top-level code in the agent process, and the type confusion is a validation bypass on its own.
Reachability question I could not settle from the repo: mcp_tool.py:263 sets input_schema=tool.inputSchema straight from the remote MCP server and static_args.py:233 calls create_model on it when argument_properties is non-empty. If tenants can connect MCP servers we do not control, this is remote input reaching code execution. Otherwise it is agent-author-to-runtime, which is a lower bar.
Either way: strip or reject customTypePath and x-python-type before calling the generator.
There was a problem hiding this comment.
overall it is a good observation considering the scenario with the MCP, but it should not be marked as blocking. even if this happens, it would include an user-owned module
also the type-confusion path is a very exaggerated, over-engineering scenario
| node: dict[str, Any], models: list[type[BaseModel]] | ||
| ) -> Iterator[tuple[Any, list[type[BaseModel]]]]: | ||
| """Yield each subschema of `node` with the models it describes.""" | ||
| for combiner in _COMBINERS: |
There was a problem hiding this comment.
__uipath_marker_name__ cross-contaminates across combiner branches.
Each anyOf/oneOf/allOf branch is yielded with the parent's unnarrowed models list, so name_models tags every model in that list with that branch's $ref name, hasattr-guarded so the first branch wins permanently.
With result: anyOf[Job_attachment, Note] plus a sibling note: $ref Note:
legacy: get_json_paths_by_type(m, "__Job_attachment") -> ['$.result']
this: -> ['$.result', '$.note']
That false positive is not benign — get_job_attachments then raises AgentRuntimeError(OUTPUT_VALIDATION_ERROR, category=SYSTEM) on valid data, because job_attachments.py treats any non-empty value at an attachment path as an Attachment.
Reverse the branch order and it becomes a false negative instead: with three branches, all classes end up carrying __Other, and a plain att: $ref Job_attachment property is never found, so attachments are silently skipped.
Multi-$ref combiners are the standard shape zod/TypeScript emit for MCP tool schemas, and nothing upstream normalises them.
Narrow the model list per branch, or skip tagging when a branch is a $ref and len(models) > 1 (which is what legacy effectively does for the ambiguous case).
| ) -> list[tuple[tuple[str, ...], dict[str, Any]]]: | ||
| """Locate subschemas using a keyword the generator cannot express as a type.""" | ||
| found: list[tuple[tuple[str, ...], dict[str, Any]]] = [] | ||
| seen: set[int] = set() |
There was a problem hiding this comment.
Shared $defs entries lose their constraint guard after the first reference.
seen is keyed on id(node) for the whole walk, and a $ref'd $defs node is the same object each time. The second occurrence short-circuits before _is_unenforceable, so no validator is registered for it.
{"properties":{"a":{"$ref":"#/$defs/N"},"b":{"$ref":"#/$defs/N"}},
"$defs":{"N":{"type":"object","properties":{"n":{"not":{"type":"string"}}}}}}{"a":{"n":"str"}} rejected on both; {"b":{"n":"str"}} rejected on legacy, accepted here. Same for a $ref reached from array items, and for one Python dict reused in two places.
Sharing a $defs entry across properties is the normal reason $defs exists, so this is the most likely of the guard gaps to be hit.
The memo is genuinely needed for test_recursive_definition. Keying by traversal path does not work — I tried it and it recurses forever, since the path never repeats. A per-branch ancestor set (frozenset of in-progress node ids, not a global set) terminates on both self-recursive and mutually-recursive schemas and fixes this case.
| return node.get("enum") == [] | ||
|
|
||
|
|
||
| def _child_paths( |
There was a problem hiding this comment.
_child_paths misses keyword positions that _child_nodes covers, so guarded keywords silently lapse there.
This walker enumerates properties, dict-form items and the combiners. _child_nodes (line 250) additionally descends additionalProperties. _unenforceable_constraints uses only this one.
additionalProperties value = {"type":"string","enum":[]} legacy rejects | here accepted
additionalProperties value with `not` legacy rejects | here accepted
additionalProperties value with `prefixItems` legacy rejects | here accepted
root-level additionalProperties with `not` legacy rejects | here accepted
draft-7 tuple form items: [ {...not...} ] legacy rejects | here accepted
Adding additionalProperties here alone is not enough — I checked both naive variants. Without a path segment the guard validates the map itself against the value schema and falsely rejects {"meta": {}}; with a "{}" segment but no change to _values_at_path the path never resolves and the guard silently no-ops. Both functions have to change together, mirroring the existing "[]" handling.
The tuple-items gap needs the same treatment in both walkers.
More generally: two hand-rolled walkers over the same document, with different keyword sets and different memo keys, is why this and the two comments above are three separate bugs. One shared context-carrying traversal would close all three and stop the next keyword from leaking.
| ) -> Any: | ||
| """Return a callable enforcing constraints the generated types cannot carry.""" | ||
| validators = [ | ||
| (path, jsonschema.Draft202012Validator(subschema)) |
There was a problem hiding this comment.
The guard builds validators from detached subschemas and never checks them, so two classes of input escape as raw exceptions.
- No
check_schema, andenforceonly converts validation results. Anything raised while interpreting a malformed subschema propagates raw out of themode="before"validator — all three of these build fine and blow up atmodel_validate:
{"a":{"not":"banana"}} -> AttributeError: 'str' object has no attribute 'items'
{"a":{"not":{"type":"string","pattern":"["}}} -> re.error: unterminated character set
{"a":{"not":{"type":"nope"}}} -> jsonschema.exceptions.UnknownType
None is a ValidationError, so BaseUiPathStructuredTool._parse_input does not classify it and the job record ends up as ERROR_AttributeError / category UNKNOWN with no tool or schema context — the opposite of the fail-loudly-at-startup policy stated in create_output_model's docstring.
- The validator is constructed from the isolated node with no registry rooted at the document, so a
$refnested inside a guarded subschema cannot resolve and raisesPointerToNowhereon every validation, valid input included._unenforceable_constraintsresolves a$refencountered as the node itself but appends nodes containing nested refs verbatim.
Call check_schema at build time and raise AgentStartupError, build the validator against the whole document (or a referencing registry rooted at schema), and wrap iter_errors so any residual jsonschema exception becomes a ValueError.
Separate, same function: enforce returns early when data is not a dict, so a root-level array schema is never guarded at all — {"type":"array","items":{..."not"...}} with [{"n":"str"}] is rejected by legacy and accepted here.
|
|
||
| # Prefix for the per-conversion pseudo-modules that let qualified-name lookups | ||
| # resolve each schema's generated classes. | ||
| _DYNAMIC_MODULE_PREFIX = "jsonschema_pydantic_converter._dynamic" |
There was a problem hiding this comment.
The prefix and the counter are duplicated from _legacy_converter.py:32,34 — same prefix string, two independent itertools.count() both starting at 0. The Nth module from each backend collides in sys.modules, and the later registration replaces the earlier one:
dmcg#1 -> ..._dynamic_1
legacy#1 -> ..._dynamic_0
legacy#2 -> ..._dynamic_1 # collision
# sys.modules["..._dynamic_1"].T is now legacy#2's class, not dmcg#1's
That is exactly the failure the per-conversion module exists to prevent, per the docstrings in both files.
It needs both backends to run in one process, so production reachability depends on whether flags are ever reconfigured after startup — I could not establish that. It is guaranteed in the test process, where the fixture flips the flag per test.
Move the prefix, counter and _create_dynamic_module into one module both backends import.
|
Thanks — this is a genuinely good review. I re-derived six of the findings from scratch in a local venv rather than take them on trust, and all six reproduced exactly as described. Measured, legacy vs code-generator, flag flipped via
So I'm not going to argue with any of it. Three points I want to concede explicitly rather than leave implied: The equivalence claim in the description was mine and it was wrong. "The whole conversion contract suite runs against both backends" was true as a statement about parametrization and worthless as evidence — your observation that Non-identifier property names failing is the worst of these, because repairing those names is the reason the backend exists. The headline feature is the broken one. Your On direction. The Tell me if you'd rather it all stayed here and I'll work through the list in this PR instead. Generated by Claude Code |
faac080 to
c3eb04e
Compare
…-generator backend Two changes, one dependency apart. An object written inline rather than under $defs that held a $ref produced a child model which was never completed -- the root reported complete while the child kept a ForwardRef -- so an agent failed at tool-binding time rather than on execution. jsonschema-pydantic-converter fixed that in 0.4.1, so the pin moves from >=0.4.0 to >=0.4.1. That bump is the whole of that fix. Separately, schemas become Pydantic models through two interchangeable backends now, selected by the EnableDatamodelCodeGeneratorConverter feature flag: off (the default) keeps jsonschema-pydantic-converter, on switches to datamodel-code-generator. create_model and create_output_model keep their signatures, so no caller changes. The newer backend names generated types after the schema, homes every class in the conversion's own pseudo-module, and repairs property names that are not valid Python identifiers while keeping the declared names on the wire. The flag is a gradual rollout, not an escape from a broken backend. Where callers depend on behaviour the generator does not provide, it is restored: the original JSON property names on the wire, __uipath_marker_name__ on types reached through a $ref, jsonschema enforcement for not/prefixItems/ empty enum, and an AgentStartupError naming an unresolved type. Review fix: a ``format`` no longer changes the value it annotates. The generator retypes on format by default -- password to SecretStr, which serializes as a mask instead of the credential; email and ulid to types whose validators are not installed, failing conversion outright; date-time to an aware datetime, rejecting the naive stamps models emit; and a dozen more to objects json.dumps cannot serialize. Every (type, format) pair is pinned back to its type's own default, derived from the generator's table so a format added upstream is covered too. Review fix: a declared JSON property name now resolves as an attribute on the generated model. Serializing by alias makes model_dump() alias-keyed, and LangChain's BaseTool._parse_input reads each dumped key back off the instance with getattr -- a lookup by JSON name that a sanitized field did not answer, so any tool whose schema had a property like Content-Type raised AttributeError mid-call. Handled on the base model so the mismatch is closed for every alias-unaware consumer, with a test that invokes a real tool. Review fix: the model handed back is the document's own, not a definition that won its name. The generator asks for the root's name first but reserves it last, so a schema titled Root alongside $defs/Root leaves the definition holding Root and renames the root around it. The requested name is no dependable handle either way -- names are singularized after the callback returns, and it is called more than once for some subschemas. The root is identified by structure instead: it carries the document's own properties, and nothing else refers to it, with the requested names left to break ties structure cannot settle. Until now such a tool showed the model the definition's arguments and rejected the ones its own schema declared, as a prompt mismatch the author could not fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADgPaRAEta3H3toAjM4DXu
c3eb04e to
7a8d14e
Compare
|
Two changes, one dependency apart.
1. fix parsing of inline object - fixed by the converter bump
An object written inline rather than under
$defsthat held a$refproduced a child model which was never completed: the root reported__pydantic_complete__ = Truewhile the child kept aForwardRef('__Dynamictype_N'). It surfaced asonce LangChain copies the field annotations into a model in another module to build the tool definition — so an agent failed at tool-binding time, not on execution.
jsonschema-pydantic-converterfixed that upstream in 0.4.1. The only thing needed here is the pin:TestInlineObjectWithRefasserts the shape against both backends, so the guarantee cannot regress on either path.2. A second schema backend behind a feature flag
Independent of the fix above. Schemas are converted to Pydantic models by two interchangeable backends now, selected by the
EnableDatamodelCodeGeneratorConverterfeature flag:jsonschema-pydantic-converterdatamodel-code-generatorcreate_modelandcreate_output_modelkeep their signatures, so no caller changes — all 11 call sites here (and the 2 inuipath-agents-python) go through the same façade.The flag is a gradual rollout, not an escape from a broken backend — with 0.4.1 in place both resolve
$refs completely. It exists for what the newer one does better:DynamicType_N, and those names reach the language model through$defs$defs, so a model built for an inline object keeps the converter's shared module — where repeated class names across schemas can make a qualified-name lookup land on another schema's classmy-field)Kept equivalent where callers depend on it
serialize_by_aliasfrom a shared base class.additionalProperties: falsestill merges over that base to forbid extras.__uipath_marker_name__on types reached through a$ref, byte-compatible with the legacy names because job-attachment discovery looks types up by them (get_json_paths_by_type(model, "__Job_attachment")). The mapping comes from walking the schema alongside the model tree — the generator sanitizes and de-duplicates class names on its own terms, so no name-based mapping is reliable.not,prefixItemsand an emptyenumhave no Pydantic annotation, so the code generator drops them and the field would accept anything. The legacy backend enforces them itself, so those paths are checked against the original subschema withjsonschemarather than letting validation silently lapse.AgentStartupErrornaming the unresolved type, raised before generation so the message stays the one users already see.Differences that remain
Measured, not assumed, and pinned in
TestBackendDifferences:$ref__module___type_converters(shared)$defsnames the model seesDynamicType_0/1Fields,ProjectadditionalProperties-only mapdict[str, str]Create_IssueCreate_IssueTesting
The whole conversion contract suite runs against both backends — that is what makes this a genuine toggle rather than two divergent paths.
ruff check,ruff format --check,mypyclean (src and the touched test packages)