Repository navigation
fix(types): drop the empty-model arm codegen emits beside a constrained array - #1658
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean fix for #1616 with no blocking or medium findings.
What I checked:
drop_empty_model_arms_from_array_fieldsinscripts/post_generate_fixes.pyis a sound AST pass: strips the emptyAdCPBaseModel: passarm from unions that also carry alist[...]arm, with correct guards against emptying the union (not kept), no-ops (len(kept)==len(arms)), and non-list unions (has_list). Reverse-ordered offset splicing avoids overlap; col_offset is byte-safe (ASCII union lines).- The three regenerated domain modules (
refine_proposals_request.py,refine_proposals_response.py,request_proposals_response.py) match the transformation exactly (list[Loss] | Losses→list[Loss]) and are paired with the generator rule — not hand-edits insidedomains/**, so the "generated code is not source" gate is satisfied. - Semver signal:
fix(types):is correct. No schema-valid document ever produced the Empty arm (array fields with min_length), public classes stay defined, snapshot unchanged, nothing that deserialized before breaks — not a breaking public-surface change. - Tests are the right shape: tree-wide AST invariant, bundle examples, ValidationError-not-TypeError, REFUSING shrinks, new no-allowlist raise guard.
- Bulk of the diff is black paren-wrapping (semantically inert).
No Critical/High/Medium findings. gated_paths false, no high-risk paths, no no-auto-approve team match. Falls through to row 9 → approve. (Note: local CI gates ruff/mypy/pytest remain the floor and are presumed enforced in CI.)
…ed array Validating a schema-valid McpWebhookPayload or AsyncResponseData raised ``TypeError: Unable to apply constraint 'min_length' ... object of type 'Losses' has no len()`` instead of returning a model or a ValidationError (#1616). An array schema that carries an ``allOf`` of ``contains``/``if``-``then`` constraints (``losses`` on the legacy purchase continuation, ``refinements`` on refine_proposals_request, ``results`` on refine_proposals_response) has no Python shape codegen can give those keywords, so datamodel-code-generator renders the composition as an empty object model and widens the field to ``list[Item] | Empty``. Pydantic applies the array's min_length/max_length to the whole union, and a dict reaching the model arm trips the constraint with a raw TypeError. Add a generic post-generation rule, drop_empty_model_arms_from_array_fields, that strips every empty AdCPBaseModel arm from a union that also carries a ``list[...]`` arm. The class itself stays, since it is a public adcp.types name and removing it would be a 9.x surface change for nothing. The existing fix_legacy_purchase_accepted_losses stays as the model-specific validator injection it also performs. Tests: the four bundle examples of core/mcp-webhook-payload.json validate both as written and in the completed form the sweep grades; a dict on each affected field is a ValidationError; the generated tree carries no empty-model arm beside a list. Remove the two schemas from the round-trip sweep's REFUSING allowlist, and add a guard that no schema-valid document may make a generated model raise anything but ValidationError. Closes #1616 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7291ce0 to
20a3d55
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked: the new drop_empty_model_arms_from_array_fields generator rule in scripts/post_generate_fixes.py correctly strips the codegen-artifact empty-model arm from list[...] | Empty unions (#1616) — it only drops empty AdCPBaseModel subclasses beside a list[...] arm, leaves public classes (Losses/Refinements/Results) defined and exported, is idempotent, and uses AST-surgical reverse-sorted bottom-up offset edits that keep replacement arithmetic safe. The three domain files are regeneration output, not hand-edits; pre-fix behavior was a raw TypeError crash, so narrowing to list[...] breaks no deserialization contract — fix(types): is the correct semver signal. New tests enforce the invariant tree-wide and lock the round-trip sweep against regression.
Decision path: no critical/high (row 1 n/a); gated_paths false (row 2 n/a); high_risk false (rows 3/5 n/a); no medium findings (rows 4/8 n/a); prior decision was approve (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve. Consistent with the prior approve on the preceding head.
Closes #1616.
Validating a schema-valid
McpWebhookPayloadorAsyncResponseDatadocument raised a rawTypeErrorfrom inside pydantic instead of returning a model or aValidationError:Cause
Not the
allOfflattening, and notfix_legacy_purchase_accepted_losses(which only editslegacy_purchase_continuation_input.py). An array schema that carries anallOfofcontains/if-thenconstraints has no Python shape codegen can give those keywords, so datamodel-code-generator renders the composition as an empty object model and widens the field tolist[Item] | Empty. Pydantic then applies the array'smin_length/max_lengthto the whole union, and a dict reaching the model arm trips the constraint with aTypeErrorrather than aValidationError. Three generated fields had the defect:PurchaseContinuation1.losses: list[Loss] | Losses(media_buy/request_proposals_response.py,min_length=2)RefineProposalsRequest.refinements: list[ProposalRefinement] | Refinements(min_length=1, max_length=25)RefineProposalsResponse1/2.results: list[Results8] | Results(min_length=1)Every schema-valid webhook or async-response document whose
resultunion visited theRequestProposalsResponsearm while being resolved crashed the same way, which is why all four bundle examples ofcore/mcp-webhook-payload.jsonand the synthesized documents were refused.Fix (generator rule, not the generated file)
scripts/post_generate_fixes.pygainsdrop_empty_model_arms_from_array_fields, a generic AST pass: for every annotation union that carries alist[...]arm together with an arm naming an emptyAdCPBaseModelsubclass (class X(AdCPBaseModel): pass), drop the empty arm, in either order, and leave the class in place. The class is a publicadcp.typesname (Losses,Refinements,Resultsare in_generated.pyand the public API snapshot), so removing it would be a 9.x surface change for nothing; the public API snapshot is unchanged. The rule is idempotent and registered inpost_generation_manifest.jsonas firing. It fired on exactly the three modules above;make validate-generatedpasses.A dict on any of those fields is now a
ValidationError(list_type), the list constraints still apply (losseswith one entry is still refused bymin_length=2), and the three empty classes keep validating as before for anyone who imported them.Tests
tests/test_constrained_array_empty_arms.py: all four bundle examples ofcore/mcp-webhook-payload.jsonvalidate, both as written (data) and in the completed form the sweep grades (the documents that raised); the sweep's synthesizedcore/async-response-data.jsondocument resolves through the union; a dict onlosses/refinements/resultsis aValidationError; the legacy continuation still accepts and length-checks its losses; and an AST scan asserts the generated tree has no empty-model arm beside a list anywhere. 9 of the 14 fail against the unfixed tree.tests/conformance/test_wire_round_trip.py:core/mcp-webhook-payload.jsonandcore/async-response-data.jsonare removed fromREFUSING, so the sweep enforces the fix (the "only shrinks" assertion fails if they come back). Newtest_no_schema_valid_document_raises_outside_validationforbids any refusal recorded under an exception class name, with no allowlist;_round_trip.pynames those classes (RAISED_OUTSIDE_VALIDATION) so the sweep and the test share them.python -m tests.conformance._round_tripon this branch: 1,008 graded documents, 5 refusals (all declaredValidationErrorrefusals; the 6TypeErrorones are gone).Validation
uv sync,make lint,make typecheck-all,make validate-generatedgreen; the round-trip, schema/model parity, public API, codegen invariant, hierarchy and code-generation suites pass. The full CI-equivalent suite runs locally after #1657's; CI is the gate.Based on
main; it touches three regenerated modules that #1657 does not, and will be rebased ontomainonce #1657 lands so CI grades the combined tree before merge.🤖 Generated with Claude Code