Genericize pydantic_ai tool registration via schema-driven factory - #638
Open
AlexanderZ-Band wants to merge 8 commits into
Open
AlexanderZ-Band wants to merge 8 commits into
AlexanderZ-Band wants to merge 8 commits into
Conversation
…factory (INT-1375) Collapse ~25 hand-written per-tool wrapper functions in adapters/pydantic_ai.py into a single schema-driven registrar in the new integrations/pydantic_ai/tools.py, mirroring the pattern already used by integrations/mcp/engine.py and the crewai/parlant adapter splits. Built-in tools are now derived solely from the runtime tool registry and platform_args_schema via public Tool.from_schema, with one generic dispatcher that double-validates before dispatch (required because Tool.from_schema skips schema validation and pydantic-ai discards a custom validator's return, per the installed pydantic-ai-slim 2.18.0 source). Preserves band_read_room_file image BinaryContent output and the band_respond_contact_request failure room-event behavior. The adapter is reduced to lifecycle/prompt/history/streaming/custom-tool orchestration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B91Jw9XjprM6SYSKfYxvQG
The two tests differed only in whether send_event's own call also raised; collapse them into one parametrized test instead of duplicating the arrange/act/assert body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B91Jw9XjprM6SYSKfYxvQG
AlexanderZ-Band
commented
Sep 16, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
Requesting changes for the confirmed uncaught-exception regression (see inline comment on tools.py:129) — GitHub doesn't allow REQUEST_CHANGES on your own PR, so filing as COMMENT; treat the tools.py:129 issue as blocking. The rest are suggestions/nits.
… run dispatch() only wrapped the method call in try/except; result normalization ran after it, so a band_read_room_file block with invalid base64 raised binascii.Error uncaught out of agent.run() instead of returning an LLM-readable error, unlike the pre-refactor wrapper. Move normalization inside the same try, with a regression test that fails without the fix. Also, from the same review pass: - Replace the bare getattr(ctx.deps, method_name) with a local _resolve_method() that raises an actionable RuntimeError instead of a raw AttributeError on a stale registry entry. (Reusing mcp.engine's resolve_tool_method was considered and rejected: the pydantic-ai extra has no mcp dependency, and mcp/engine.py is out of scope for INT-1375.) - Use get_tool_description() for the advertised description instead of reading schema.__doc__ directly, matching every sibling integration. - Include tool_call_id and the traceback (exc_info=True) in the now-shared dispatch failure log. - Collapse the two hand-copied "derives from registry" drift-test bodies (LangGraph, PydanticAI) into one shared assertion helper. Declined: redacting dispatch failure log lines against every possible AgentToolsProtocol exception message. No existing helper covers arbitrary exception text (only tool arguments are redacted today), this is a net logging improvement over the prior no-logging baseline, and inventing a new redaction policy is a product decision beyond this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B91Jw9XjprM6SYSKfYxvQG
AlexanderZ-Band
commented
Sep 16, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
Contains one blocking issue (the contact-request misreport bug, tools.py:142) — GitHub doesn't allow REQUEST_CHANGES on your own PR, so filing as COMMENT. All 4 findings from this cycle are already fixed in commit to follow.
…act response dispatch()'s try block spanned both the method call and result normalization, so once band_read_room_file's decode crash was fixed by routing both through the same except, a downstream serialize_tool_result failure for a *successful* band_respond_contact_request call fell into the same branch that reports a room error event -- misreporting a completed response as one the agent failed to answer. Split into two excepts: only a method-call failure (notify_room=True) can trigger the room event; a normalization failure (notify_room=False) still returns a graceful error to the model without claiming the request was never answered. Regression test added. Also, from the same review pass: - Log inside _resolve_method before its RuntimeError propagates, so a registry/protocol drift bug leaves a diagnostic trail like every other dispatch failure now does, instead of surfacing with none. Added a regression test for the error branch itself (previously uncovered). - Use the existing MessageType.ERROR enum instead of the "error" string literal when reporting the contact-request room event. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B91Jw9XjprM6SYSKfYxvQG
Collaborator
Author
|
Re: the test-coverage finding in the last review body (test_pydantic_ai_adapter.py:1205, _resolve_method's error branch was uncovered) — fixed in 3b626fb with test_missing_backend_method_raises_an_actionable_error in TestBuiltinToolExecution. |
AlexanderZ-Band
commented
Sep 16, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
One confirmed validation regression; fixing it now.
AlexanderZ-Band
commented
Sep 16, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
Submitting as COMMENT (GitHub blocks REQUEST_CHANGES on ones own PR) — the alias-rejection issue on tools.py:104 is blocking and confirmed end-to-end; will fix under --fix before merge.
_reject_unknown_arguments diffed argument names against model_json_schema()['properties'], which only lists a field's primary alias -- never its validation_alias secondary names. band_add_participant and band_remove_participant accept both `identifier` and `name` via AliasChoices, so a call using `name` was wrongly rejected pre-dispatch even though the master model would have accepted it. Replaced the hand-rolled property diff with a local schema subclass (ConfigDict(extra="forbid")) so the one model_validate() call in validate_tool_arguments does the rejecting itself, correctly honoring aliases -- and let the advertised JSON schema pick up the resulting additionalProperties: false for free. Also adds a log line on every argument-validation failure (previously silent) with the tool_call_id, consistent with the rest of this file's failure logging, and collapses _resolve_method's duplicated error-message construction into one string. Updates test_advertised_schema_is_the_master_schema and the pydantic_ai image-passthrough probe (now needs tool_call_id on its stub RunContext) for the same reason. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B91Jw9XjprM6SYSKfYxvQG
AlexanderZ-Band
commented
Sep 16, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
Submitting as COMMENT (GitHub blocks REQUEST_CHANGES on ones own PR) — this is blocking and will be fixed now.
The previous commit's ConfigDict(extra="forbid") schema was reused for both validation and the JSON schema Tool.from_schema advertises to the model, so additionalProperties: false leaked into every built-in tool's advertised schema. pydantic-ai's own GoogleJsonSchemaTransformer doesn't strip that keyword before sending it to Gemini, breaking tool calls for any PydanticAIAdapter pointed at a google-gla:/google-vertex: model -- the same keyword-on-Gemini problem adapters/gemini.py already had to work around once for its own tool schemas. Split the two uses: the strict schema now only feeds the dispatcher and the pre-execution args_validator (both purely internal to validation), while json_schema= advertises the plain, unmodified master schema, same as before either of the last two commits. Reverts test_advertised_schema_is_the_master_schema to its original assertion, which already fully guards this by equality against the unmodified schema. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B91Jw9XjprM6SYSKfYxvQG
AlexanderZ-Band
commented
Sep 16, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
No blocking findings this cycle; will apply the 3 suggestions/nits now.
Fixes from a follow-up code review cycle: - _strict_schema now overrides model_json_schema to raise -- the split between the validation-only strict schema and the plain schema advertised to the model already caused one real regression earlier in this branch's history (the strict schema briefly leaked into json_schema= and broke Gemini tool calls). Nothing structurally stopped a future edit from making the same mistake; this makes it fail loudly at the call site instead of silently regressing a specific provider. - Collapsed the three copies of Callable[..., Coroutine[Any, Any, Any]] into one _BoundToolMethod alias. - Parametrized test_validation_alias_argument_is_not_rejected_as_unknown over both ADD_PARTICIPANT and REMOVE_PARTICIPANT -- the only two tools sharing the validation_alias pattern the fix addresses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B91Jw9XjprM6SYSKfYxvQG
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
Resolves INT-1375.