[AAASM-4848] 🐛 (python): Surface positional tool args to content policy + assert pydantic_ai execution hook#279
Merged
Merged
Conversation
Haystack, CrewAI, LlamaIndex and Microsoft Agent Framework built their governance-visible tool_args from keyword arguments only, so a positionally invoked tool presented an empty/partial mapping to check_tool_start — argument-CONTENT policy went blind to the positional values (the allow/deny gate still fired on the tool name). Add a stdlib-only shared helper that folds positional args into tool_args (mirroring smolagents/mcp) and use it in the four adapters; Microsoft recovers arguments/context from the positional slots. Refs AAASM-4848
apply() tried Tool._run (the <0.3.0 hook) first and only fell back to AbstractToolset.call_tool (the >=0.3.0 execution hook). That is correct today because Tool._run is absent on modern versions, but a future vestigial Tool._run — present yet off the execution path — would satisfy the first branch, leave call_tool unpatched, and make governance a dead method. Select call_tool first, using Tool._run only as the legacy fallback when no toolset hook point exists, and count a concrete-toolset patch as a successful hook. Add unit tests that exercise apply()'s hook selection directly. Refs AAASM-4848
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
Author
🤖 Claude Code — PR reviewScope (AAASM-4848, LOW): ✅ positional tool args now folded into governance-visible tool_args for 4 adapters (content policy sees them); pydantic_ai prefers the real
|
…xity S3776: patched_invoke's cognitive complexity was 16 (>15). Pull the positional-vs-keyword argument recovery into _recover_invoke_call_args so the wrapper reads as a flat sequence of steps. S5778: bind the FakeFunctionTool instance before the pytest.raises block so only the invoke() call — the sole statement expected to raise — sits inside it (AAASM-4848). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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.
Description
Fixes the AAASM-4848 LOW cluster: two governance-visibility gaps in the Python SDK adapters. Neither is a gate bypass today (the allow/deny gate still fires on the tool name), but both narrow what argument-CONTENT policy can see or could silently disable governance on a future framework version.
tool_argsfrom keyword arguments only, so a positionally-invoked tool (tool.invoke("secret")) presented an empty/partial mapping tocheck_tool_start. A new stdlib-only shared helper (_shared/positional_args.py) folds positional args in — mirroring how the smolagents/mcp adapters already handle them — so content policy sees the values. Microsoft recoversarguments/contextfrom the positional slots. The gate behavior is unchanged.apply()could select a vestigial hook. It triedTool._run(the <0.3.0 hook) first and only fell back toAbstractToolset.call_tool(the >=0.3.0 execution hook). Correct today becauseTool._runis absent on modern versions, but a future vestigialTool._runoff the execution path would satisfy the first branch and leavecall_toolungoverned.call_toolis now selected first, withTool._runused only as the legacy fallback.Type of Change
Breaking Changes
Related Issues
Testing
New regression tests:
test/unit/adapters/test_positional_args_content_policy.pydrives each of the 4 adapters' real governance wrappers with a positional secret and asserts a deny-on-content policy both sees the secret and blocks the tool body.test/unit/adapters/pydantic_ai/test_apply_hook_selection.pyexercisesapply()'s hook selection directly (not the_apply_*helpers in isolation), assertingcall_toolwins over a vestigialTool._runand thatTool._runis still used as the legacy fallback.Validation: full suite
pytest test/— 1180 passed, 16 skipped.ruff check+blackclean on changed files.mypy agent_assemblyclean (with--ignore-missing-imports, as the pre-commit hook runs it).Checklist
🤖 Generated with Claude Code