Repository navigation
Python: Add tool name prefixes to FileAccessProvider - #9654
Jose Alvarez (jpalvarezl) wants to merge 1 commit into
Conversation
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
GitHub Actions
Active
There was a problem hiding this comment.
🟡 Changes recommended
Model-facing guidance and auto-approval rules still use unprefixed names and need to support the configured prefixes.
2 open issues
What changed in this PR
Adds optional tool-name prefixes to Python’s FileAccessProvider, addressing #9646 by allowing independently rooted stores to coexist on one agent without tool-name collisions.
Changes:
- Adds keyword-only
tool_name_prefixand applies it to all eight tools. - Preserves existing tool names when the prefix is omitted or empty.
- Adds regression coverage for collisions, mixed provider configurations, and per-store read/write routing.
| File | Description |
|---|---|
python/packages/core/tests/core/test_harness_file_access.py |
Tests prefix naming, provider coexistence, and isolated store routing. |
python/packages/core/agent_framework/_harness/_file_access.py |
Adds prefix configuration and applies it during tool registration. |
Review effort: Balanced
| self.disable_write_tool_approval = disable_write_tool_approval | ||
| self.session_scoped = session_scoped | ||
| self.scope = scope | ||
| self.tool_name_prefix = tool_name_prefix |
| def _prefixed_tool_name(self, name: str) -> str: | ||
| return f"{self.tool_name_prefix}_{name}" if self.tool_name_prefix else name |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 5e1d6a75ec8b
Model: gpt-5.6-sol
Overview
The PR consistently applies the configured prefix to all eight tools, preserves default names for falsey prefixes, rejects remaining duplicate names centrally, and tests per-store routing. However, renaming leaves the public auto-approval rules authoritative over the old names, which can bypass approval for an unrelated local tool, while the default model instructions still advertise those old names. Prefixes are also accepted verbatim even when they produce names rejected by model providers.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
3 verified findings remained after source verification (1 high, 2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_harness/_file_access.py
| ) | ||
|
|
||
| def _prefixed_tool_name(self, name: str) -> str: | ||
| return f"{self.tool_name_prefix}_{name}" if self.tool_name_prefix else name |
There was a problem hiding this comment.
After a provider is prefixed, this frees the canonical file_access_* names while the public static auto-approval rules still approve those names. An approval-required local tool registered as file_access_write can now coexist and will be auto-approved, bypassing the human gate; meanwhile the real prefixed file tool is not matched. Make the approval rules provider/prefix-bound and match only that provider's effective names, so renaming also removes authority from the old names.
|
|
||
| @tool(name=FileAccessProvider.WRITE_TOOL_NAME, schema=_WriteFileInput, approval_mode=write_approval) | ||
| @tool( | ||
| name=self._prefixed_tool_name(FileAccessProvider.WRITE_TOOL_NAME), |
There was a problem hiding this comment.
When a prefix is set, this renames the registered tools but instructions remains DEFAULT_FILE_ACCESS_INSTRUCTIONS, which tells the model to call unprefixed file_access_* names. In the supported prefixed-plus-default setup, those directions select the default provider, so requests intended for the prefixed store can read or modify the wrong store; with only prefixed providers they name nonexistent tools. Render the injected instructions and cross-tool references from each provider's effective names.
| self.disable_write_tool_approval = disable_write_tool_approval | ||
| self.session_scoped = session_scoped | ||
| self.scope = scope | ||
| self.tool_name_prefix = tool_name_prefix |
There was a problem hiding this comment.
tool_name_prefix is stored verbatim, so whitespace, /, Unicode, or a long prefix can produce function names that violate provider identifier or 64-character limits. Core accepts these names and adapters forward them, causing a remote request failure instead of a usable provider. Validate or normalize the prefix, including the final generated-name length, when constructing the provider.
Motivation & Context
An agent configured with multiple
FileAccessProviderinstances fails while merging tools because every provider exposes the samefile_access_*names, even when the providers have distinct source IDs and independently rooted stores.Allow callers to distinguish each provider's tools so an agent can access multiple file stores without tool-name collisions.
Description & Review Guide
tool_name_prefixconstructor parameter toFileAccessProviderand apply it to all eight registered tool names.specproduces names such asspec_file_access_read.Related Issue
Fixes #9646
Maintainer agreement with the direction was explicitly confirmed by the submitter during preparation of this draft. No competing open PR was found for the issue.
AI Assistance
AI assistance details: GitHub Copilot assisted with code analysis and review, regression tests, validation, and this PR description. The implementation was authored by the contributor.
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.The focused file-access unit test module passes: 131 passed, 7 skipped, with three existing warnings. Ruff lint/format checks and Pyright pass for both changed files. The full core unit suite and a package build have not been run, so their checklist items remain unchecked.