From 5e1d6a75ec8bf1a0eba78e980012fe17feb3ff67 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Fri, 23 Oct 2026 18:00:51 +0200 Subject: [PATCH] Added tool_prefix field for FileAccessProvider to prevent dupe name error --- .../agent_framework/_harness/_file_access.py | 46 +++++++-- .../tests/core/test_harness_file_access.py | 93 +++++++++++++++++-- 2 files changed, 124 insertions(+), 15 deletions(-) diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 0aa2d139b34..b97571879a6 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -1877,6 +1877,7 @@ def __init__( disable_write_tool_approval: bool = False, session_scoped: bool = False, scope: str | None = None, + tool_name_prefix: str | None = None, ) -> None: """Initialize the file access provider. @@ -1917,6 +1918,7 @@ def __init__( ``session_id`` is used. The value is treated as an opaque key rather than a path: it is mapped onto exactly one folder by :func:`~agent_framework._filesystem._storage_key_segment`. + tool_name_prefix: Optional prefix for tool names to avoid collisions. """ super().__init__(source_id) self.store = store @@ -1926,6 +1928,7 @@ def __init__( 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 _resolve_session_key(self, context: SessionContext) -> str: """Resolve the working folder key for session-scoped mode. @@ -2046,6 +2049,9 @@ def all_tools_auto_approval_rule(function_call: Content) -> bool: and function_call.name in FileAccessProvider._ALL_TOOL_NAMES ) + def _prefixed_tool_name(self, name: str) -> str: + return f"{self.tool_name_prefix}_{name}" if self.tool_name_prefix else name + async def before_run( self, *, @@ -2071,7 +2077,11 @@ def _session_path(relative_path: str) -> str: if session_key: instructions += _SESSION_SCOPED_INSTRUCTIONS_SUFFIX - @tool(name=FileAccessProvider.WRITE_TOOL_NAME, schema=_WriteFileInput, approval_mode=write_approval) + @tool( + name=self._prefixed_tool_name(FileAccessProvider.WRITE_TOOL_NAME), + schema=_WriteFileInput, + approval_mode=write_approval, + ) async def file_access_write(file_name: str, content: str, overwrite: bool = False) -> str: """Write a file with the given name and content. By default, does not overwrite an existing file unless overwrite is set to true.""" # ruff:ignore[line-too-long] try: @@ -2096,7 +2106,11 @@ async def file_access_write(file_name: str, content: str, overwrite: bool = Fals return f"Could not write file '{file_name}': {exc.strerror or exc}" return f"File '{file_name}' written." - @tool(name=FileAccessProvider.READ_TOOL_NAME, schema=_ReadFileInput, approval_mode=readonly_approval) + @tool( + name=self._prefixed_tool_name(FileAccessProvider.READ_TOOL_NAME), + schema=_ReadFileInput, + approval_mode=readonly_approval, + ) async def file_access_read(file_name: str) -> str: r"""Read the content of a file by name. Returns the file content or a message indicating the file could not be read. Line numbers count lines split on \n only: a lone \r never starts a new line, each line keeps its own terminator, and content ending in a newline has a final empty line.""" # ruff:ignore[line-too-long] try: @@ -2109,7 +2123,7 @@ async def file_access_read(file_name: str) -> str: return content if content is not None else f"File '{file_name}' not found." @tool( - name=FileAccessProvider.READ_LINES_TOOL_NAME, + name=self._prefixed_tool_name(FileAccessProvider.READ_LINES_TOOL_NAME), schema=_ReadLinesInput, approval_mode=readonly_approval, ) @@ -2128,7 +2142,11 @@ async def file_access_read_lines(file_name: str, start_line: int, end_line: int # Each line keeps its terminator, so it doubles as the row separator. return "".join(f"{number}\t{line}" for number, line in enumerate(sliced, start_line)) - @tool(name=FileAccessProvider.DELETE_TOOL_NAME, schema=_DeleteFileInput, approval_mode=write_approval) + @tool( + name=self._prefixed_tool_name(FileAccessProvider.DELETE_TOOL_NAME), + schema=_DeleteFileInput, + approval_mode=write_approval, + ) async def file_access_delete(file_name: str) -> str: """Delete a file by name.""" try: @@ -2142,7 +2160,11 @@ async def file_access_delete(file_name: str) -> str: return f"Could not delete file '{file_name}': {exc.strerror or exc}" return f"File '{file_name}' deleted." if deleted else f"File '{file_name}' not found." - @tool(name=FileAccessProvider.LS_TOOL_NAME, schema=_ListInput, approval_mode=readonly_approval) + @tool( + name=self._prefixed_tool_name(FileAccessProvider.LS_TOOL_NAME), + schema=_ListInput, + approval_mode=readonly_approval, + ) async def file_access_ls( directory: str | None = None, glob_pattern: str | None = None, @@ -2164,7 +2186,11 @@ async def file_access_ls( {"name": entry.name, "type": entry.type} for entry in listed if _matches_glob(entry.name, glob_pattern) ] - @tool(name=FileAccessProvider.REPLACE_TOOL_NAME, schema=_ReplaceInput, approval_mode=write_approval) + @tool( + name=self._prefixed_tool_name(FileAccessProvider.REPLACE_TOOL_NAME), + schema=_ReplaceInput, + approval_mode=write_approval, + ) async def file_access_replace( file_name: str, old_string: str, @@ -2188,7 +2214,7 @@ async def file_access_replace( return f"Replaced {count} occurrence(s) in '{file_name}'." @tool( - name=FileAccessProvider.REPLACE_LINES_TOOL_NAME, + name=self._prefixed_tool_name(FileAccessProvider.REPLACE_LINES_TOOL_NAME), schema=_ReplaceLinesInput, approval_mode=write_approval, ) @@ -2209,7 +2235,11 @@ async def file_access_replace_lines(file_name: str, edits: list[_LineEdit]) -> s return f"Could not edit file '{file_name}': {exc.strerror or exc}" return f"Replaced {len(edits)} line(s) in '{file_name}'." - @tool(name=FileAccessProvider.GREP_TOOL_NAME, schema=_SearchFilesInput, approval_mode=readonly_approval) + @tool( + name=self._prefixed_tool_name(FileAccessProvider.GREP_TOOL_NAME), + schema=_SearchFilesInput, + approval_mode=readonly_approval, + ) async def file_access_grep( regex_pattern: str, glob_pattern: str | None = None, diff --git a/python/packages/core/tests/core/test_harness_file_access.py b/python/packages/core/tests/core/test_harness_file_access.py index 377c5768f5d..227e914be31 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -844,13 +844,15 @@ async def test_filesystem_store_does_not_create_root_until_write(tmp_path: Path) assert await store.read("a.txt") == "alpha" +@pytest.mark.parametrize("tool_name_prefix", [None, "", "spec_files"]) async def test_file_access_provider_registers_tools_and_instructions( chat_client_base: SupportsChatGetResponse, + tool_name_prefix: str | None, ) -> None: - """``FileAccessProvider.before_run`` should add the canonical instructions and all tools.""" + """Register all tools with the configured prefix while preserving default names.""" session = AgentSession(session_id="session-1") store = InMemoryAgentFileStore() - provider = FileAccessProvider(store=store) + provider = FileAccessProvider(store=store, tool_name_prefix=tool_name_prefix) agent = Agent(client=chat_client_base, context_providers=[provider]) _, options = await agent._prepare_session_and_messages( # pyright: ignore[reportPrivateUsage] @@ -870,13 +872,90 @@ async def test_file_access_provider_registers_tools_and_instructions( "file_access_replace", "file_access_replace_lines", } - assert {getattr(tool, "name", None) for tool in tools} >= expected_names + if tool_name_prefix: + expected_names = {f"{tool_name_prefix}_{name}" for name in expected_names} + assert {getattr(tool, "name", None) for tool in tools} == expected_names + + if not tool_name_prefix: + instructions = options.get("instructions") + if isinstance(instructions, str): + assert DEFAULT_FILE_ACCESS_INSTRUCTIONS in instructions + else: + assert any(DEFAULT_FILE_ACCESS_INSTRUCTIONS in chunk for chunk in (instructions or [])) + - instructions = options.get("instructions") - if isinstance(instructions, str): - assert DEFAULT_FILE_ACCESS_INSTRUCTIONS in instructions +@pytest.mark.parametrize( + ("spec_prefix", "sdk_prefix"), + [(None, None), ("spec_files", "sdk_files"), ("spec_files", None)], + ids=["duplicate-names", "distinct-prefixes", "prefixed-and-default"], +) +async def test_file_access_providers_with_multiple_stores( + chat_client_base: SupportsChatGetResponse, + spec_prefix: str | None, + sdk_prefix: str | None, +) -> None: + """Reproduce #9646 and allow an agent run when the stores have distinct tool names.""" + agent = Agent( + client=chat_client_base, + context_providers=[ + FileAccessProvider( + InMemoryAgentFileStore(), + source_id="spec_files", + tool_name_prefix=spec_prefix, + disable_readonly_tool_approval=True, + disable_write_tool_approval=True, + ), + FileAccessProvider( + InMemoryAgentFileStore(), + source_id="sdk_files", + tool_name_prefix=sdk_prefix, + disable_readonly_tool_approval=True, + disable_write_tool_approval=True, + ), + ], + ) + + if spec_prefix is None and sdk_prefix is None: + with pytest.raises(ValueError, match="Duplicate tool name 'file_access_read'"): + await agent.run("test") else: - assert any(DEFAULT_FILE_ACCESS_INSTRUCTIONS in chunk for chunk in (instructions or [])) + response = await agent.run("test") + assert response.text == "test response - test" + + +async def test_file_access_provider_prefixed_tools_route_to_their_own_store( + chat_client_base: SupportsChatGetResponse, +) -> None: + """Prefixed tools must keep same-named files in different stores independent.""" + spec_store = InMemoryAgentFileStore() + sdk_store = InMemoryAgentFileStore() + agent = Agent( + client=chat_client_base, + context_providers=[ + FileAccessProvider(spec_store, source_id="spec_files", tool_name_prefix="spec"), + FileAccessProvider(sdk_store, source_id="sdk_files", tool_name_prefix="sdk"), + ], + ) + _, options = await agent._prepare_session_and_messages( # pyright: ignore[reportPrivateUsage] + session=AgentSession(session_id="session-1"), + input_messages=[Message(role="user", contents=["work with files"])], + ) + tools = options["tools"] + assert isinstance(tools, list) + assert len(tools) == 16 + + for prefix, content in (("spec", "specification"), ("sdk", "implementation")): + write = _tool_by_name(tools, f"{prefix}_file_access_write") + result = await write.invoke(arguments={"file_name": "shared.txt", "content": content}) + assert "written" in _text(result[0]) + + for prefix, content in (("spec", "specification"), ("sdk", "implementation")): + read = _tool_by_name(tools, f"{prefix}_file_access_read") + result = await read.invoke(arguments={"file_name": "shared.txt"}) + assert _text(result[0]) == content + + assert await spec_store.read("shared.txt") == "specification" + assert await sdk_store.read("shared.txt") == "implementation" async def test_file_access_provider_all_tools_require_approval(