Repository navigation
fix: preserve scalar enum and const constraints in MCP schemas - #300
liangzhechen655 wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Source review of 73963771fbfcd18a45b8bf61d0f0c3f8fdb56ee8: schema_transformer.py:103-108 returns a Literal before applying the declared field type. A required string field with enum: ["brief", null] now includes null, and an integer field with enum: [1, "two"] includes a string. Preserve the intersection of the declared type and choices, and test both typed mixed-enum cases in validation and the emitted schema.
The reference-resolution change overlaps #304. Retain one implementation of that branch when reconciling the two. This correctness finding is source-derived; I haven't run this head's tests locally.
The earlier complexity-only rework request is withdrawn.
…constraints # Conflicts: # atomic-agents/atomic_agents/connectors/mcp/schema_transformer.py
Eigenwise
left a comment
There was a problem hiding this comment.
Rechecked 6dbfbe4d22838c5e0126014a5676297f7e88cf41. The earlier string/null and integer/string intersections are repaired, with validation and emitted-schema regressions. Normal CI passed 421 tests with 3 MiniMax skips; formatting and lint passed.
One scalar-choice issue remains at schema_transformer.py:41-55: for type: ["boolean", "integer"], enum: [true], input 1 passes the type guard and Literal[True] converts it to True. Conversely, enum: [1] accepts True and returns 1. The existing Pydantic 2.12.4/core 2.41.5 dependency confirms that literal matching behavior; the complete candidate example is source-derived, not locally executed.
Check JSON scalar choice identity before literal matching, and add boolean/numeric union and untyped enum/const regressions. Preserve ordinary numeric equality such as 1 and 1.0. No broader schema redesign requested.
|
Additional integration-review finding on 6dbfbe4: a non-required string enum emits an anyOf containing the string enum and null, plus a top-level type: string from json_schema_extra. Pydantic accepts None but the emitted schema rejects it. Please add an emitted-schema regression for a non-required enum and attach the type metadata inside the optional branch. The existing boolean/numeric choice-identity review item still applies. Please integrate current main before the next round; the dictionary-schema fix in #313 is now released. |
…constraints # Conflicts: # atomic-agents/tests/connectors/mcp/test_schema_transformer.py
|
Thanks for the concrete counterexamples. I integrated current main (
Of the 19 added review cases, 15 failed on the integrated pre-fix code and 4 were passing controls. All now pass, including emitted-schema validation with |
Eigenwise
left a comment
There was a problem hiding this comment.
Rechecked 06c30e61. The earlier boolean/numeric identity and optional emitted-schema fixes are repaired. All 87 schema-transformer tests pass locally.
One regression remains at schema_transformer.py:140-145: scalar choices bypass composition constraints. For a required property with this schema:
{"enum": [1, "bad"], "anyOf": [{"type": "integer"}]}The generated Pydantic model accepts "bad". Both the advertised JSON Schema and unchanged main reject it; 1 passes on both. The enum path returns before applying anyOf.
Please preserve the intersection with composition constraints in validation and the emitted schema. Add a regression for enum/const combined with these constraints. I reproduced the candidate/base difference offline with real Pydantic and JSON Schema validation. No remote service was used.
|
Thanks for the reproduced composition counterexample. I integrated current main ( The new cases also cover exclusive All 119 transformer cases pass. Of the 32 additional cases, 21 fail against the previous transformer and 11 are passing controls; all pass with the fix. The full core suite passes 906 tests with 3 credential-gated MiniMax skips. Repository-wide Black/Flake8, the whitespace check, locked/offline sync and wheel build pass. The two validator libraries were already MCP transitive dependencies and are now declared directly, without locked version changes. |
MCP scalar
enumandconstconstraints are lost during schema conversion, allowing generated tool input models to accept values outside the server's advertised choices. Returning aLiteralbefore processinganyOf/oneOfalso bypasses composition restrictions: for example,enum: [1, "bad"]withanyOf: [{type: integer}]accepts"bad".Intersect the finite scalar choice set with the advertised JSON Schema once during model construction, then use only the surviving choices for Pydantic validation and emitted schema. This includes declared types,
anyOf, exclusiveoneOf,allOf, combined restrictions and locally referenced constraints. An impossible choice/constraint intersection raisesValueError. Unresolved references alone retain the transformer's permissive fallback, while known sibling constraints still apply; the empty reference registry does not retrieve remote schemas.Keep JSON scalar identity checks before Pydantic's literal matching:
truemust not match1, andfalsemust not match0. Ordinary numeric equality remains valid (1matches1.0), and mixed boolean/numeric enums return the matching scalar kind. Declared-type metadata stays inside the choice annotation so the existing optional-fieldNoneallowance remains consistent with the emitted optional schema.Integrated current
main(14cf8eb), retaining its shared reference resolver and #313 dictionary-schema handling.jsonschemaandreferencing, already MCP transitive dependencies, are declared as direct dependencies; locked package versions do not change. The new validator is used only to intersect scalar choices, not to replace general schema conversion.Validation
Windows / Python 3.13, current locked dependencies:
git diff --check, locked/offline dependency sync and wheel build pass.