Python: fix(python): project the per-call effective tool set on agent-hooks pre_model_call - #7747
Merged
Evan Mattson (moonbox3) merged 7 commits intoSep 2, 2026
Conversation
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 18, 2026 22:57 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 18, 2026 22:57 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 18, 2026 22:57 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 18, 2026 22:58 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
Contributor
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
Contributor
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||
Contributor
|
/review |
MohammadHaroonAbuomar
added a commit
to MohammadHaroonAbuomar/agent-framework
that referenced
this pull request
Aug 18, 2026
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>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 18, 2026 23:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 18, 2026 23:16 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
added a commit
to MohammadHaroonAbuomar/agent-framework
that referenced
this pull request
Aug 18, 2026
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>
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 18, 2026 23:37 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
added a commit
to MohammadHaroonAbuomar/agent-framework
that referenced
this pull request
Aug 19, 2026
…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>
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 19, 2026 04:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 19, 2026 04:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MohammadHaroonAbuomar
added a commit
to MohammadHaroonAbuomar/agent-framework
that referenced
this pull request
Aug 19, 2026
…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>
MohammadHaroonAbuomar
temporarily deployed
to
github-app-auth
August 19, 2026 14:19 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Evan Mattson (@moonbox3) gentle ping — all review threads are addressed at head cf6c0f8/1e167e8de (the observer-fidelity and single-resolution changes you suggested, plus the nested-container and losing-route consumption fixes, each with a regression test). Happy to take another round if anything looks off. |
Contributor
|
Sorry for the delay. Could you also look at the current merge conflict, MohammadHaroonAbuomar. Thanks. |
…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. Fixes microsoft#7560 Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
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>
…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>
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>
…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>
…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>
MohammadHaroonAbuomar
force-pushed
the
mhabuomar/agent-hooks-percall-tools
branch
from
September 2, 2026 01:18
1e167e8 to
92ddee5
Compare
MohammadHaroonAbuomar
deployed
to
github-app-auth
September 2, 2026 01:18 — with
GitHub Actions
Active
GitHub Actions
Active
Contributor
Author
|
Thanks Evan Mattson (@moonbox3) — rebased onto current main (92ddee5, conflict-free merge now). One small conflict during the rebase: #7918's |
Address a review follow-up from microsoft#7747 (probe-confirmed on both run-level routes): normalize_tools flattens a wrapper object through its iterable .tools attribute — before its Mapping and BaseModel exclusions — but the boundary materialization treated those wrappers as leaves. A generator-backed .tools collection was therefore drained by the startup projection, and the run silently executed without the tool. _materialize_tool_container now walks the container shapes flattening walks in flattening's own order: tool leaves (FunctionTool, dict specs, MCPTool, strings, callables) pass through first, then wrapper objects with an iterable .tools collection get their contents materialized. A wrapper whose collection is safely re-iterable (a Collection) with untouched contents keeps its identity; a one-shot (or one-shot-holding) collection cannot be preserved without mutating the caller's object, so exactly that case is expanded into its materialized tools — the same contents flattening would have produced from it. One new mutation-verified, parametrized test (one-shot backing: run keeps the tool, snapshot and per-call projections agree; reusable backing: wrapper identity preserved through the pipeline); the earlier identity-fidelity and nested-container tests still pass. Feature suite, full core suite, ruff, pyright/mypy/ty/zuban/pyrefly, prek green. Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
MohammadHaroonAbuomar
deployed
to
github-app-auth
September 2, 2026 01:46 — with
GitHub Actions
Active
GitHub Actions
Active
Evan Mattson (moonbox3)
approved these changes
Sep 2, 2026
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
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.
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.