fix(core): enforce input-model strictness and required params in ToolCatalog [TOO-771]#829
Open
jottakka wants to merge 5 commits into
Open
fix(core): enforce input-model strictness and required params in ToolCatalog [TOO-771]#829jottakka wants to merge 5 commits into
jottakka wants to merge 5 commits into
Conversation
…Catalog Auto-generated Pydantic input models for @tool-decorated functions were silently dropping unknown kwargs and defaulting omitted required params to None, so typos in tool calls produced isError: false with no indication the arguments were ignored. Also fixes nested TypedDict typos being silently dropped inside request dicts. Changes in libs/arcade-core/arcade_core/catalog.py: - Top-level input model now sets ConfigDict(extra='forbid') so unknown kwargs raise ValidationError (surfaced as ToolInputError by ToolExecutor). - Required params (no Optional[...] annotation, no explicit default in the signature) are now emitted without default=, so Pydantic enforces them at validation time. Adds a ParamInfo.has_explicit_default flag to distinguish "no default" from "default=None", preventing regressions on def f(x: str = None) style signatures. - Nested TypedDict input fields are routed through a new strict Pydantic wrapper (create_model_from_typeddict(..., strict=True)) so typos inside request dicts also raise. Output-side TypedDicts remain permissive to preserve pass-through of extra keys from upstream APIs. - @model_serializer(mode='wrap') on _TypedDictBaseModel drops unset fields during nested serialization (Pydantic v2's outer serializer does not invoke the Python-level model_dump override). - Wraps Optional[TypedDict] inside lists and inside parent TypedDict fields so strictness propagates. Param-name-qualified model names prevent $defs collisions when two params share a TypedDict type. Version bumps: arcade-core 4.7.0 → 4.8.0; arcade-tdk and arcade-mcp-server pin arcade-core>=4.8.0 since tool-call validation semantics change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Libraries whose pyproject.toml metadata changes in this PR get a version bump so the behavioral contract tightening (arcade-core>=4.8.0 required for strict input validation) is addressable by consumers via their lockfiles: - arcade-tdk 3.8.0 → 3.9.0 - arcade-mcp-server 1.20.0 → 1.21.0 (also retargets arcade-tdk>=3.9.0, arcade-serve>=3.3.0) - arcade-serve 3.2.3 → 3.3.0 (retargets arcade-core>=4.8.0 — serve calls ToolExecutor.run with the new input_model semantics via BaseWorker) - arcade-mcp (root) 1.14.0 → 1.15.0 (retargets arcade-core>=4.8.0, arcade-mcp-server>=1.21.0) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…models extract_field_info raises ToolInputSchemaError when a parameter lacks an Annotated description, so by the time tool_field_info reaches create_func_models its description is guaranteed non-None. The `or "No description provided."` branch has been unreachable on main and is now removed. Output-side fallbacks in determine_output_model are retained — return annotations may legitimately omit descriptions (e.g. `-> str` without Annotated). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…default The has_explicit_default flag is populated by two code paths — one for plain Python signatures (extract_python_param_info) and one for pydantic.Field(...) declarations (extract_pydantic_param_info). The Python-signature path was covered by existing tests; this adds four tests for the Pydantic-Field path: - Field() with no default/factory → has_explicit_default=False, required - Field(default=None) → has_explicit_default=True, optional, nullable - Field(default="x") → has_explicit_default=True, optional, non-null - Field(default_factory=list) → has_explicit_default=True, optional Follows up on fresh-context reviewer's should-fix #3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolve version-bump conflicts in pyproject.toml files. Bumped arcade-mcp-server to 1.21.2 to layer the PR's dep update on top of main's 1.21.1 from #826.
jottakka
changed the title
|
This pull request has been automatically marked as stale because it has had no activity for 14 days. It will be closed in 14 days if no further activity occurs. If this is still relevant, please leave a comment or remove the stale label. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes TOO-771 (https://linear.app/arcadedev/issue/TOO-771)
Auto-generated Pydantic input models for
@tool-decorated functions currently:extra='forbid'), soedit_requests=...instead ofrequests=...producesisError: falsewith zero indication anything was wrong.None(every field is effectively optional at validation time, becausecreate_func_modelspassesdefault=Noneunconditionally), so the wire schema'srequired=Truenever turns into a Pydantic error.{"txt": "x"}instead of{"text": "x"}), because the raw TypedDict → Pydantic path ignores extras.Reported by francisco@arcade.dev after ~1200 Google Docs edit requests were discarded across a multi-hour session; the agent had no signal that anything was being ignored. Production reproductions against
GoogleDocs.InsertTextAtEndOfDocument@5.2.0andGoogleDocs.EditDocument@5.2.0confirm the same silent drops at the deployed Arcade Cloud MCP.Changes — libs/arcade-core/arcade_core/catalog.py
create_func_modelsnow setsConfigDict(extra='forbid')on the generated input model. Unknown kwargs raiseValidationError, surfaced asToolInputError/ErrorKind.TOOL_RUNTIME_BAD_INPUT_VALUEby the existingToolExecutor._serialize_inputbranch.default=so Pydantic enforces them at validation time. Adds aParamInfo.has_explicit_defaultflag to distinguish "no default" from "default=None", preventing regressions ondef f(x: str = None)style signatures (which are now correctly typed asOptional[str]withdefault=None).create_model_from_typeddict(..., strict=True)path wraps input-side TypedDicts in a_StrictTypedDictBaseModel(extra='forbid'). Output-side TypedDicts keep the permissive default so tools returning dicts with extra keys from upstream APIs (e.g. Google, Slack) don't break.@model_serializer(mode='wrap')on_TypedDictBaseModeldrops unset fields during nested serialization. The previousmodel_dump()Python-level override was bypassed by Pydantic v2's Rust-level outer serializer.Optional[TypedDict],list[TypedDict],list[Optional[TypedDict]],list[TypedDict]inside a parent TypedDict, two params sharing a TypedDict type (param-name-qualified model names to avoid$defscollisions), and barelist(no type arg) destructuring.Version bumps
arcade-core4.7.0 → 4.8.0 (minor: validation semantics change).arcade-tdkandarcade-mcp-serverboth pinarcade-core>=4.8.0,<5.0.0.Review history
This PR went through 3 rounds of local ACR review; round 3 findings about theoretical
model_serializerkwarg forwarding (1/5) and hypothetical non-None TypedDict field defaults (1/5) are documented in-source and not addressed since the actual call paths are covered by tests.Test plan
make checkclean onarcade-core(pre-commit, ruff, mypy).ToolExecutor.runnow returnsErrorKind.TOOL_RUNTIME_BAD_INPUT_VALUEfor both unknown kwargs and missing required fields (previously both silently succeeded).🤖 Generated with Claude Code
Note
Medium Risk
Changes runtime validation semantics for all tool calls, so previously tolerated extra/missing fields will now raise and may break existing clients; behavior is well-covered by new tests but affects a core execution path.
Overview
Auto-generated Pydantic input models for
@toolfunctions are now strict: unknown top-level kwargs are rejected (extra="forbid"), and parameters without an explicit default are truly required at validation time (no more implicitdefault=None). A newhas_explicit_defaultflag distinguishes “no default” vs= Noneso signatures likedef f(x: str = None)(andField(default=None)/default_factory) remain optional and are re-annotated asOptional[...]when needed.Nested
TypedDictinputs are also validated strictly by wrapping them (includinglist[TypedDict]and related edge cases) in generated Pydantic models that forbid extra keys, whileTypedDictoutputs remain permissive; serialization oftotal=FalseTypedDict models is fixed via@model_serializerto drop unset fields even when nested. Adds a comprehensive test suite for these failure modes, and bumps package versions/pins (arcade-core4.8.0 and downstream deps) to reflect the behavior change.Reviewed by Cursor Bugbot for commit 737a177. Bugbot is set up for automated code reviews on this repo. Configure here.