Uh oh!
There was an error while loading. Please reload this page.
Python: fix(python): project the per-call effective tool set on agent-hooks pre_model_call - #7747
Conversation
…re_model_call The agent-hooks pre_model_call emission carried no tools projection, and the agent_startup tools_registered projection read getattr(agent, "tools", None) — an attribute Agent does not expose (constructor tools live in default_options["tools"]) — so constructor-registered tools never surfaced anywhere, and per-call tool changes (run-level tools, context-provider tools, progressive tool exposure, MCP expansion) were invisible to interceptors. Mirror the .NET resolution from microsoft#7564 (per-call ChatOptions.Tools projection): - Emit the spec's optional pre_model_call tools field ({name, description?}) from the call's effective options["tools"] — the completed set for each model call, including the loop's tool_choice="none" final call, which projects its effective options unchanged. An unprojectable or empty set omits the optional field rather than claiming "no tools". - Make tools_registered the honest run-start snapshot: agent-declared tools (Agent.default_options["tools"], with a tools-attribute fallback for custom agents) plus this invocation's run-level tools, and document that dynamically registered tools surface per call and are bracketed by the tool seam when invoked. Five new tests (per-call effective set, constructor-tools startup projection, provider-contributed tools, tools-disabled final call, omitted field); core suite, pyright/mypy/ty/zuban/pyrefly, ruff, and prek hooks green. Fixesmicrosoft#7560 Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds accurate tool visibility to Python agent-hooks projections.
Changes:
- Projects effective tools on each
pre_model_call. - Includes constructor and run-level tools in startup snapshots.
- Adds five regression tests covering dynamic and tool-less calls.
Show a summary per file
| File | Description |
|---|---|
_agent_hooks.py | Implements startup and per-call tool projection. |
test_agent_hooks.py | Tests tool projection scenarios. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Python Test Coverage Report •
Python Unit Test Overview
| ||||||||||||||||||||||||||||||||||||||||
Evan Mattson (moonbox3)
commented
Aug 18, 2026
/review |
Address the three review findings on microsoft#7747 (all reproduced empirically): - Guard the tools value's truthiness evaluation: the emptiness check now runs inside normalize_tools under the projection guard, so a tools container whose __bool__/__len__/__iter__ raises degrades to omitting the optional field (with a warning) instead of aborting the emission mid-run. - Project hosted-tool mappings faithfully: provider factories return top-level-field mappings without a nested "function" object (OpenAI {"type": "web_search"}, Anthropic {"type": "web_search_20250305", "name": "web_search"}), which projected as {"name": "dict"} and dropped descriptions. Top-level name/description are now read, with the tool type naming unnamed hosted tools. - Restore the legacy tools-attribute fallback for custom agents whose default_options mapping has no "tools" entry: the snapshot now falls back to the attribute whenever the mapping lookup yields None, so such agents keep their pre-existing startup projection. Three new mutation-verified tests; feature suite, core suite, ruff, pyright/mypy/ty/zuban/pyrefly, and prek hooks green. Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 188312abfe80
Model:gpt-5.6-sol
Overview
The PR adds per-call effective tool projections and expands the startup snapshot to include constructor and explicit run-level tools, with tests covering provider-contributed tools, final settlement calls, and tool-less calls. The per-call path is well aligned with the effective chat options, but the startup snapshot misses tools passed through the public options mapping, leaving that audit event incomplete for a supported invocation path.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas:python/packages/core/agent_framework/_agent_hooks.py
Uh oh!
There was an error while loading. Please reload this page.
…p snapshot
Run-level tools can arrive either as Agent.run(tools=...) or inside the
run's options dict (options={"tools": [...]}); the run treats the named
parameter as taking precedence. The startup snapshot only read
context.tools (the named parameter), so options-route tools executed
and appeared in every pre_model_call projection but were missing from
agent_startup's tools_registered.
Mirror the run's own precedence in the snapshot: fall back to
context.options["tools"] when the named parameter is absent, so both
supported invocation forms report the same registered set.
One new mutation-verified test covering the options route end to end
(the tools= route was already asserted); feature suite, core suite,
ruff, pyright/mypy/ty/zuban/pyrefly, and prek hooks green.
Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Address two review points from microsoft#7747 (both probe-confirmed): - One-shot iterable tool containers (normalize_tools flattens any iterable collection) were consumed by the agent-hooks startup projection before the run could use them: enabling the middleware silently removed every run-level tool the iterable contained, on both the tools= and options={"tools": ...} routes. AgentMiddlewareLayer.run now materializes the run-level tools exactly once before building the AgentContext, so the context the middleware observes and the run executed beneath it share the same list — observation can no longer consume the run's tool source. - The startup snapshot no longer re-derives the run-option rules (constructor-tool location, the legacy tools attribute, the tools=/options precedence) inside the agent-hooks module. AgentContext._resolve_run_start_tools is the framework's one statement of the run-start tool policy, kept next to the run-option rules it mirrors; _tool_names is projection-only, guarded so an unresolvable set degrades to a warning and an empty snapshot. Two new mutation-verified tests (one-shot iterables survive the snapshot, both routes) plus hostile-resolution coverage for the snapshot path; feature suite, core suite, ruff, pyright/mypy/ty/ zuban/pyrefly, and prek hooks green. Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ecedence Address two review follow-ups from microsoft#7747 (both probe-confirmed): - Agent middleware must see the caller's original tool objects. The previous materialization normalized before the pipeline, replacing a bare callable with a FunctionTool wrapper, so an identity-based guard (e.g. rejecting one specific privileged callable) missed it and the run invoked the tool anyway. AgentMiddlewareLayer.run now materializes only the outer container — list() on one-shot iterables, elements untouched, re-iterable shapes passed through — restoring observation fidelity while keeping the exhaustion fix: the pipeline, the run-start resolution, and the run share one re-iterable container of original objects. - One statement of the run-level tool precedence. _select_run_level_tools (named parameter wins over an options-dict entry) is now consumed by both AgentContext._resolve_run_start_tools and Agent._prepare_run_context, which also pops the options entry unconditionally. This fixes the pre-existing disagreement when both routes were supplied: the losing options["tools"] used to survive in the remaining options and ride **opts into the request, silently overriding the resolved list — the run executed one set while the run-start view reported another. Behavior change (edge case): with both routes supplied, the named parameter now wins end to end, matching the long-documented comment. Three new mutation-verified tests (identity fidelity + identity-based enforcement governs execution; named-parameter precedence end to end; snapshot/per-call agreement on the both-routes case); feature suite, full core suite, ruff, pyright/mypy/ty/zuban/pyrefly, prek green. Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
| """ | ||
| from pydantic import BaseModel | ||
| if tools is None or isinstance(tools, (str, bytes, bytearray, Mapping, BaseModel, Sequence)): |
There was a problem hiding this comment.
Following up on the earlier report: this commit makes a top-level one-shot container reusable, but a supported nested collection is still consumed by the startup projection. _materialize_tool_container leaves Sequence containers and their elements untouched, so tools=[(t for t in [nested_tool])] is drained by _resolve_run_start_tools, and _prepare_run_context later sees an empty source; with agent-hooks enabled, the model loses that tool. Could the materialization cover the nested collection forms that normalize_tools recursively flattens, or share one materialized source across both passes?
There was a problem hiding this comment.
Confirmed: tools=[(t for t in [nested_tool])] on the previous commit gave tools_registered == ['nested_tool'] while the run lost the tool entirely (pre_model_call carried no tools, the tool never executed) — the same silent degradation one level down, exactly as you said. Fixed in 1e167e8: _materialize_tool_container now walks exactly the container shapes normalize_tools recursively flattens. Leaf shapes (FunctionTool, MCPTool, dict specs, mapping-like collections flattened via their re-iterable .tools attribute, pydantic models, strings, callables) pass through untouched — element identity is preserved for the identity-based policy checks from the earlier thread — and an already re-iterable container whose elements needed no materialization keeps its own identity, so the common all-list case still aliases the caller's objects exactly as before. Regression test: test_nested_one_shot_collection_survives_the_snapshot (snapshot, per-call projection, and actual execution all see the tool; mutation-verified — fails on the previous commit).
| # identity-based policy checks (for example rejecting one specific privileged | ||
| # callable) keep seeing exactly what the caller supplied. | ||
| tools = _materialize_tool_container(tools) | ||
| if options is not None and (options_tools := options.get("tools")) is not None: |
There was a problem hiding this comment.
Following up on the precedence fix: _select_run_level_tools now centralizes which route wins, but the middleware boundary still materializes both routes before that selection. A losing one-shot options["tools"] source is consumed, and can raise or trigger side effects, even when tools= wins and the run ignores it. Could the entrypoint select the winner first and materialize only that source, then pass the selected value through to AgentContext and execution?
There was a problem hiding this comment.
Done in 1e167e8 — and the probe found the consumption went deeper than the middleware boundary: with tools= winning, the losing one-shot options["tools"] was iterated not only by the boundary's materialization but also by the telemetry layer's span-attribute serialization (_serialize_tool_definitions normalizes options["tools"]), which both drained the owner's iterator and recorded tools the run would never use. The boundary now selects the winner via _select_run_level_tools first, materializes only that source, and drops the losing entry from the forwarded options on a copy — so nothing below (telemetry, run setup) can consume or record it, and its owner can still iterate it after the run. The block is hoisted above the no-middleware fast path so both paths behave identically. _prepare_run_context's unconditional pop stays as the backstop for direct RawAgent.run callers. Regression test: test_losing_run_tool_route_is_never_iterated (asserts zero iterations during the run and that the owner can still drain the source afterwards; mutation-verified — fails on the previous commit).
…e winning route Address two review follow-ups from microsoft#7747 (both probe-confirmed): - normalize_tools flattens iterable tool collections recursively, so a one-shot iterable nested inside a list is a supported shape the outer- container materialization missed: the startup projection drained the inner iterator and the run lost that tool (the same silent degradation one level down). _materialize_tool_container now walks exactly the container shapes flattening walks — leaf shapes (FunctionTool, MCPTool, dict specs, mapping-like collections, pydantic models, strings, callables) pass through untouched, and an already re-iterable container whose elements needed no materialization keeps its identity. - The middleware boundary materialized both run-level routes before selecting the winner, so a losing one-shot options["tools"] source was consumed (and could raise or trigger side effects) even though the run ignores it. The boundary now selects via _select_run_level_tools first and materializes only the winner; the losing options entry is dropped from the forwarded options on a copy, so no layer below — including telemetry span-attribute serialization, which the probe caught iterating and recording the losing source — ever consumes or records tools the run will not use. Hoisted above the no-middleware fast path so both paths behave identically. Two new mutation-verified tests (nested one-shot survives the snapshot and executes; losing route never iterated and still consumable by its owner); feature suite, full core suite, ruff, pyright/mypy/ty/zuban/ pyrefly, prek green. Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
Motivation & Context
#7560 reported that constructor-registered tools never appear in the agent-hooks
agent_startupprojection:_tool_namesfalls back togetattr(agent, "tools", None), butAgentstores constructor tools indefault_options["tools"], so the fallback is alwaysNone. The review thread on the .NET port (#7564, with westey (@westey-m)) surfaced the wider version of the same gap: tools registered dynamically during a run (context providers, MCP expansion, progressive tool exposure) can never be known atagent_startuptime, so any startup-time projection is inherently a partial view of what each model call is actually offered.The .NET feature resolved this on #7564 (commit
5ffe0fa58there) by projecting the per-call effectiveChatOptions.Toolsinto eachpre_model_callemission's optionaltoolsfield and documentingtools_registeredas the run-start snapshot. This PR mirrors that resolution in the Python feature so both languages carry the same AGENT-HOOKS-0.1 projection contract — the parity contract established in the #7444/#7515 discussions.Description & Review Guide
pre_model_callnow carries the spec's optionaltoolsfield, projected as{name, description?}from the call's effectiveoptions["tools"]— the completed set for that model call, including run-level tools, context-provider tools, connected MCP-server functions, and progressive tool exposure. The function-invocation loop'stool_choice="none"final/settlement calls project their effective options unchanged (the tools are still in the options for those calls), matching the .NET semantics. When the call offers no tools — or the set cannot be normalized — the optional field is omitted rather than misreported as an empty set._tool_names(theagent_startuptools_registeredprojection) now produces the honest run-start snapshot: agent-declared tools (Agent.default_options["tools"], with the oldtoolsattribute as a fallback for custom agent implementations) plus the invocation's run-level tools — the same two sources the .NETResolveToolNamesreads. Its docstring documents the snapshot semantics: dynamically registered tools surface per call in thepre_model_callprojection and are bracketed bypre_tool_call/post_tool_calllike any other tool when invoked.pre_model_call, andtools_registeredentries that were previously silently dropped now present). No enforcement-path behavior changes; the fail-closed posture is untouched. Auditors and interceptors now see exactly which tools each model call was offered.context.options["tools"]at the chat seam is the per-call effective set (the function-invocation layer normalizes it and shares the run-local mutable list per iteration), anddefault_options["tools"]+context.toolsat the agent seam is the run-start snapshot. Also the deliberate choice to omit the optionaltoolsfield (instead of emitting[]) when a tool set cannot be projected.Five new tests: per-call effective set across a multi-call tool run (constructor + run-level tools), the #7560 constructor-tools startup repro, context-provider-contributed tools (startup snapshot excludes, per-call projection includes), the loop's
tool_choice="none"final call, and omission of the field on tool-less calls. Core suite, ruff, prek hooks, and pyright/mypy/ty/zuban/pyrefly all green.Cross-reference for reviewers: the .NET counterpart is commit
5ffe0fa58on #7564 — same field shape ({name, description?}), same snapshot-vs-per-call semantics, so the two implementations stay reviewable against one contract.Behavior note (edge case, from review)
Review follow-ups pulled the run-level tool resolution into one framework statement (
_select_run_level_tools, consumed by both the run-start resolution andAgent._prepare_run_context). This fixes a pre-existing inconsistency: when bothtools=andoptions={"tools": [...]}were supplied, the losingoptions["tools"]entry survived in the remaining options and rode**optsinto the request, silently overriding the resolved list — the run executed the options set while the documented precedence (and now the run-start view) said the named parameter wins. With this PR, the named parameter wins end to end; the single-route behaviors are unchanged. Also per review: agent middleware now observes the caller's original tool objects (one-shot iterable containers are materialized outer-container-only), so identity-based tool policy checks keep working.Related Issue
Fixes#7560
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.