Skip to content

Fix $ref output schema object detection regression#3420

Merged
jlowin merged 4 commits intomainfrom
codex/fix-ref-output-schemas-misclassification
Mar 7, 2026
Merged

Fix $ref output schema object detection regression#3420
jlowin merged 4 commits intomainfrom
codex/fix-ref-output-schemas-misclassification

Conversation

@jlowin
Copy link
Member

@jlowin jlowin commented Mar 6, 2026

Motivation

  • A recent simplification treated any schema containing both $ref and $defs as an object, which causes non-object referenced schemas (for example Pydantic alias/enums) to be misclassified and skip wrapping. This can lead to ToolResult runtime ValueError because structured_content must be a dict.
  • The goal is to preserve wrapping for non-object referenced schemas by resolving local #/$defs/... targets before deciding whether a schema is object-shaped.

Description

  • Update _is_object_schema in src/fastmcp/tools/function_parsing.py to accept optional _root_schema and _seen_refs parameters and resolve local #/$defs/... references recursively instead of assuming $ref + $defs means an object.
  • Add cycle protection when resolving references so recursive definitions do not loop infinitely.
  • Add a focused regression test test_output_schema_wraps_non_object_ref_schema in tests/server/providers/local_provider_tools/test_output_schema.py which uses a TypeAliasType/Literal alias to ensure non-object $ref schemas are wrapped (x-fastmcp-wrap-result) and runtime structured_content is a dict.
  • Small test import updates to support the new test (Literal, TypeAliasType).

Testing

  • Ran uv sync successfully to install dependencies.
  • Ran the full test suite with uv run pytest -n auto, which completed but reported unrelated failures in the container run (12 failed, 1 error) that appear environment/timeout related and not caused by this change.
  • Ran the focused tests for this change with uv run pytest tests/server/providers/local_provider_tools/test_output_schema.py, and all 20 tests passed. prek static hook run uv run prek run --all-files failed to initialize due to a network/git clone error (403) and could not complete.

Codex Task

🤖 Generated with GPT-5.2-Codex
@marvin-context-protocol marvin-context-protocol bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality. labels Mar 6, 2026
@jlowin
Copy link
Member Author

jlowin commented Mar 6, 2026

/marvin fix static analysis

@marvin-context-protocol
Copy link
Contributor

marvin-context-protocol bot commented Mar 6, 2026

Claude finished @jlowin's task in 1m 17s —— View job


Fixed static analysis

  • Install dependencies
  • Run static analysis to identify issues
  • Fix issues
  • Verify fixes

The ty checker flagged one error: invalid-type-alias-type at test_output_schema.py:289 — the TypeAliasType name "AliasType" didn't match the variable alias_type. Renamed the variable to AliasType to match. All checks pass and all 20 tests in the file still pass.


Marvin Context Protocol | Type /marvin to interact further

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

@marvin-context-protocol
Copy link
Contributor

Test Failure Analysis

Summary: The static_analysis job failed due to a ty (type checker) error in the new test code — a TypeAliasType name doesn't match its variable name.

Root Cause: In tests/server/providers/local_provider_tools/test_output_schema.py:289, the TypeAliasType is constructed with a name string ("AliasType") that doesn't match the variable it's assigned to (alias_type). The ty type checker enforces that these must match:

# line 289 — invalid
alias_type = TypeAliasType("AliasType", Literal["foo", "bar"])
#  variable: alias_type      ^ name: AliasType  (mismatch!)

Suggested Solution: Fix the variable name to match the TypeAliasType name string (or vice versa). Since the string "AliasType" is used for the type name, rename the variable to match:

In tests/server/providers/local_provider_tools/test_output_schema.py:289, change:

alias_type = TypeAliasType("AliasType", Literal["foo", "bar"])

to:

AliasType = TypeAliasType("AliasType", Literal["foo", "bar"])

and update any references to alias_type in the same test to use AliasType instead.

Detailed Analysis

Failing check

ty check.................................................................[Failed]
- hook id: ty
- exit code: 1

error[invalid-type-alias-type]: The name of a `TypeAliasType` (`AliasType`) must match the name of the variable it is assigned to (`alias_type`)
  --> tests/server/providers/local_provider_tools/test_output_schema.py:289:9
    |
287 |         """Root $ref schemas should only skip wrapping when they resolve to objects."""
288 |         mcp = FastMCP()
289 |         alias_type = TypeAliasType("AliasType", Literal["foo", "bar"])
    |         ^^^^^^^^^^
    |
info: rule `invalid-type-alias-type` is enabled by default

Found 1 diagnostic

Other checks

  • ruff check, ruff format, prettier, validate pyproject.toml, and codespell all passed.
  • The loq (file size limits) check reported 7 violations but explicitly states they are "not enforced... yet!" — this did not cause the failure.
Related Files
  • tests/server/providers/local_provider_tools/test_output_schema.py:289 — the test file containing the mismatched TypeAliasType name, introduced in this PR

Marvin Context Protocol and others added 2 commits March 6, 2026 22:34
Co-authored-by: Jeremiah Lowin <jlowin@users.noreply.github.com>
🤖 Generated with Claude Code
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5257deeaa4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +92 to +97
definition_name = ref.removeprefix("#/$defs/")
definitions = root_schema.get("$defs")
if not isinstance(definitions, dict):
return False

target_schema = definitions.get(definition_name)

Choose a reason for hiding this comment

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

P2 Badge Resolve escaped JSON-pointer tokens in $defs refs

The new $ref resolver assumes the definition key is the raw suffix after #/$defs/, but JSON Pointer escapes / and ~ as ~1/~0 (and can include deeper paths), so refs like #/$defs/A~1B or #/$defs/Outer/$defs/Inner won’t resolve even when they point to object schemas. In those cases _is_object_schema returns False, which causes FastMCP to wrap an already-object output and change structured_content shape unexpectedly for valid schemas that previously passed object checks.

Useful? React with 👍 / 👎.

@jlowin
Copy link
Member Author

jlowin commented Mar 7, 2026

Auto-reviewed: all CI checks green, review comments addressed. Merging.

@jlowin jlowin merged commit ceb8ff1 into main Mar 7, 2026
7 checks passed
@jlowin jlowin deleted the codex/fix-ref-output-schemas-misclassification branch March 7, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. codex server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant