Skip to content

fix: reject prefixItems in strict tool schemas - #4753

Open
mikemikimike wants to merge 3 commits into
openai:mainfrom
mikemikimike:codex/issue-4752-prefix-items
Open

fix: reject prefixItems in strict tool schemas#4753
mikemikimike wants to merge 3 commits into
openai:mainfrom
mikemikimike:codex/issue-4752-prefix-items

Conversation

@mikemikimike

Copy link
Copy Markdown

Summary

Reject prefixItems during strict JSON schema conversion so ordinary fixed-length tuple parameters fail during tool construction with guidance for supported homogeneous tuples and lists. Non-strict schemas retain their existing behavior.

Test plan

  • uv run --frozen pytest tests/test_function_schema.py tests/test_strict_schema.py tests/test_strict_schema_oneof.py -q (130 passed)
  • uv run --frozen ruff check src/agents/strict_schema.py tests/test_function_schema.py (passed)
  • uv run --frozen ruff format --check src/agents/strict_schema.py tests/test_function_schema.py (passed)
  • uv run --frozen pyright src/agents/strict_schema.py (0 errors)
  • git diff --check (passed)

The full pytest suite and single-file mypy check did not complete in the local Windows environment; both remained in dependency/test execution without producing a result and were terminated. No production failure was observed.

Issue number

Closes #4752

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

@XuQuanxin04

Copy link
Copy Markdown
Contributor

Automated Codex review identified a compatibility edge case in the current approach: traversing every dict while excluding annotation keys can reject a valid object property literally named prefixItems (and can skip schemas beneath property names such as default/examples). I reproduced this with a strict schema containing properties: {"prefixItems": {"type": "string"}}.

The traversal should follow schema-valued positions and treat entries of properties, $defs, definitions, patternProperties, and dependentSchemas as subschemas regardless of their names. I prepared a focused regression test and corrected walker locally; commit 96cbb4a in my fork branch (push retry may be needed due transient network).

@mikemikimike

Copy link
Copy Markdown
Author

Addressed the compatibility edge case from the review comment in commit 54b6e83c.

The strict-schema rejection now traverses only schema-valued JSON Schema positions, including properties, $defs, definitions, patternProperties, dependentSchemas, unions, and conditional keywords. Literal property names such as prefixItems, and annotation values under default/examples, are preserved without false positives, while an actual prefixItems schema is still rejected during construction.

Validation:

  • uv run --frozen pytest tests/test_function_schema.py tests/test_strict_schema.py tests/test_strict_schema_oneof.py -q — 134 passed
  • ruff format --check src/agents/strict_schema.py tests/test_strict_schema.py — passed
  • ruff check src/agents/strict_schema.py tests/test_strict_schema.py — passed
  • uv run --frozen pyright src/agents/strict_schema.py tests/test_strict_schema.py — 0 errors
  • git diff --check — passed

The repository-level make typecheck is not runnable as written on Windows because its parallel shell recipe uses POSIX syntax; direct Pyright passed, while repository mypy remains blocked by the pre-existing missing numpy stubs in src/agents/voice/imports.py. The full test command was attempted with the Windows-compatible equivalent, but xdist hit a permissions error in the system pytest temp directory; the single-process focused and three-file suites pass.

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

Copy link
Copy Markdown

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: 54b6e83cbb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/agents/strict_schema.py Outdated
Comment on lines +66 to +68
def _reject_prefix_items(schema: object) -> None:
"""Reject ``prefixItems`` in schema-valued positions without inspecting annotations."""
stack = [schema]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reuse the existing strict-schema traversal

For the supported case introduced here, Pydantic emits prefixItems beneath properties, and _ensure_strict_json_schema() already visits that node recursively. This standalone walker and its three keyword tables therefore create a second JSON Schema interpreter solely to validate additional direct-helper shapes; the tables can already diverge from the repository's other schema traversal (for example, ToolOutputTrimmer recognizes legacy dependencies, while this walker does not). Keep the prefixItems check inside _ensure_strict_json_schema() and remove the traversal-only tables and tests so strict conversion retains one source of truth.

AGENTS.md reference: AGENTS.md:L94-L96

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in commit 359c446b: removed the standalone traversal tables and helper, and kept the prefixItems check inside _ensure_strict_json_schema(), which already follows the schema nodes emitted by Pydantic. The focused regression suite remains green: 130 passed.

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.

Strict schemas still emit unsupported prefixItems for ordinary fixed-tuple parameters

2 participants