Skip to content

fix: preserve scalar enum and const constraints in MCP schemas - #300

Open
liangzhechen655 wants to merge 7 commits into
Eigenwise:mainfrom
liangzhechen655:fix/mcp-scalar-value-constraints
Open

liangzhechen655 wants to merge 7 commits into
Eigenwise:mainfrom
liangzhechen655:fix/mcp-scalar-value-constraints

Conversation

@liangzhechen655

@liangzhechen655 liangzhechen655 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

MCP scalar enum and const constraints are lost during schema conversion, allowing generated tool input models to accept values outside the server's advertised choices. Returning a Literal before processing anyOf/oneOf also bypasses composition restrictions: for example, enum: [1, "bad"] with anyOf: [{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, exclusive oneOf, allOf, combined restrictions and locally referenced constraints. An impossible choice/constraint intersection raises ValueError. 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: true must not match 1, and false must not match 0. Ordinary numeric equality remains valid (1 matches 1.0), and mixed boolean/numeric enums return the matching scalar kind. Declared-type metadata stays inside the choice annotation so the existing optional-field None allowance remains consistent with the emitted optional schema.

Integrated current main (14cf8eb), retaining its shared reference resolver and #313 dictionary-schema handling. jsonschema and referencing, 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:

  • All 119 schema-transformer tests pass, retaining the previous 87. The 32 additional cases cover enum/const with compositions, required/optional fields, nullable choices, numeric/boolean identity, exclusive unions, local references, unresolved-reference sibling ordering and impossible intersections. Against the previous PR transformer, 21 fail and 11 are passing controls; all pass with this follow-up.
  • New cases compare real Pydantic validation and the emitted model schema with the advertised JSON Schema across a scalar/invalid-value matrix.
  • Full core suite: 906 passed, 3 credential-gated MiniMax integration tests skipped; 99% total statement coverage.
  • Repository-wide Black and Flake8, git diff --check, locked/offline dependency sync and wheel build pass.
  • No live provider integration run is claimed.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Eigenwise

Copy link
Copy Markdown
Owner

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.

@liangzhechen655

Copy link
Copy Markdown
Contributor Author

Thanks for the concrete counterexamples. I integrated current main (2a26bbc), including #313, and addressed both remaining issues:

  • Choice matching now separates JSON booleans from numbers before Literal validation, including untyped enum/const and boolean/numeric unions. Numeric equality such as 1 and 1.0 is retained; mixed [true, 1] choices also keep the matching scalar kind.
  • Declared-type metadata now lives inside the choice annotation, so optional choices emit a schema that accepts None consistently with their Pydantic model.

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 Draft202012Validator. The full core suite passes 846 tests with 3 credential-gated MiniMax skips; Black and Flake8 pass, and schema_transformer.py has 100% statement coverage in that run.

@Eigenwise Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@liangzhechen655

Copy link
Copy Markdown
Contributor Author

Thanks for the reproduced composition counterexample. I integrated current main (14cf8eb) and now intersect the finite enum/const choice set with the advertised schema during model construction. Only admissible choices become the Literal, so Pydantic validation and the emitted schema both reject "bad" in your example while retaining 1.

The new cases also cover exclusive oneOf, allOf, combinations of composition keywords, local references, nullable choices and required/optional fields. Empty intersections reject the definition instead of widening it. Existing boolean/numeric choice identity, numeric equality and optional-schema behavior remain covered. Unresolved references retain the permissive fallback only for the reference itself, with known sibling restrictions still checked and no remote retrieval.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants