diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 0aa2d139b34..54882f766a4 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -31,7 +31,7 @@ import unicodedata import weakref from abc import ABC, abstractmethod -from collections.abc import Awaitable, Mapping, MutableMapping +from collections.abc import Awaitable, Callable, Mapping, MutableMapping from pathlib import Path from typing import Annotated, Any, ClassVar, Final, Protocol, cast @@ -1680,8 +1680,9 @@ class _LineEdit(BaseModel): default=None, description=( "Optional: the text you believe is currently on that line, as reported by " - "file_access_grep. Give the line's own text only: file_access_read_lines prefixes " - "each line with its number and a tab, and that prefix is not part of the line. " + "`file_access_grep`, which may have the same configured prefix as the enclosing tool. " + "Give the line's own text only: `file_access_read_lines`, with the same optional prefix, " + "prefixes each line with its number and a tab, and that prefix is not part of the line. " "When supplied, the edit is rejected unless it matches, which catches an " "out-of-date line number or a file that changed since you looked. The trailing " "newline is ignored in the comparison." @@ -1877,6 +1878,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,15 +1919,21 @@ 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 - self.instructions = instructions or DEFAULT_FILE_ACCESS_INSTRUCTIONS + self.instructions = instructions or ( + DEFAULT_FILE_ACCESS_INSTRUCTIONS.replace("file_access_", f"{tool_name_prefix}_file_access_") + if tool_name_prefix + else DEFAULT_FILE_ACCESS_INSTRUCTIONS + ) self.disable_write_tools = disable_write_tools self.disable_readonly_tool_approval = disable_readonly_tool_approval 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 +2054,27 @@ 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 + + def get_read_only_tools_auto_approval_rule(self) -> Callable[[Content], bool]: + """Create an auto-approval rule for this provider's read-only tool names.""" + tool_names = frozenset(self._prefixed_tool_name(name) for name in self._READ_ONLY_TOOL_NAMES) + + def rule(function_call: Content) -> bool: + return self._is_local_tool_call(function_call) and function_call.name in tool_names + + return rule + + def get_all_tools_auto_approval_rule(self) -> Callable[[Content], bool]: + """Create an auto-approval rule for all of this provider's tool names.""" + tool_names = frozenset(self._prefixed_tool_name(name) for name in self._ALL_TOOL_NAMES) + + def rule(function_call: Content) -> bool: + return self._is_local_tool_call(function_call) and function_call.name in tool_names + + return rule + async def before_run( self, *, @@ -2071,7 +2100,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 +2129,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,12 +2146,12 @@ 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, ) async def file_access_read_lines(file_name: str, start_line: int, end_line: int | None = None) -> str: - r"""Read part of a file by 1-based inclusive line number; omit end_line to read to the end of the file, and an end_line past the last line is clamped. Each line is prefixed with its number and a tab; everything after that tab is verbatim, including the line's own terminator, so it can be reused as a file_access_replace_lines new_line. 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] + r"""Read part of a file by 1-based inclusive line number; omit end_line to read to the end of the file, and an end_line past the last line is clamped. Each line is prefixed with its number and a tab; everything after that tab is verbatim, including the line's own terminator, so it can be reused as a `new_line` value by `file_access_replace_lines`, which may have the same configured prefix as this tool. 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: normalized = _normalize_relative_path(file_name) content = await self.store.read(_session_path(normalized)) @@ -2128,7 +2165,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 +2183,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 +2209,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 +2237,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 +2258,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, @@ -2225,10 +2278,12 @@ async def file_access_grep( or ``"reports/*"`` to restrict the search to the ``reports`` subtree. Leave empty or omit to search all files. Returns matching results whose file_name values are paths relative to the store root - (directly usable with file_access_read), along with snippets and matching lines with line numbers. + (directly usable with `file_access_read`, which may have the same configured prefix as this tool), + along with snippets and matching lines with line numbers. Stores are expected to report each matching line verbatim, including its own line - terminator, so it can normally be reused as a file_access_replace_lines new_line; a - custom store may not, so prefer file_access_read_lines when the exact text matters. + terminator, so it can normally be reused as a `new_line` value by `file_access_replace_lines` + with the same optional prefix; a custom store may not, so prefer `file_access_read_lines` + with the same optional prefix when the exact text matters. Line numbers count lines split on \n only, with a final empty line when content ends in a newline. The regex_pattern must be 256 characters or fewer. 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..ed3c2abdb53 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,94 @@ 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 + + expected_instructions = ( + DEFAULT_FILE_ACCESS_INSTRUCTIONS.replace("file_access_", f"{tool_name_prefix}_file_access_") + if tool_name_prefix + else DEFAULT_FILE_ACCESS_INSTRUCTIONS + ) instructions = options.get("instructions") if isinstance(instructions, str): - assert DEFAULT_FILE_ACCESS_INSTRUCTIONS in instructions + assert expected_instructions in instructions else: - assert any(DEFAULT_FILE_ACCESS_INSTRUCTIONS in chunk for chunk in (instructions or [])) + assert any(expected_instructions in chunk for chunk in (instructions or [])) + + +@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: + 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( @@ -979,6 +1062,28 @@ def test_read_only_tools_auto_approval_rule() -> None: assert FileAccessProvider.read_only_tools_auto_approval_rule(hosted) is False +def test_prefixed_provider_read_only_auto_approval_rule() -> None: + """A provider-bound rule should approve only that provider's prefixed read-only tools.""" + provider = FileAccessProvider(InMemoryAgentFileStore(), tool_name_prefix="spec") + rule = provider.get_read_only_tools_auto_approval_rule() + + prefixed_read = Content("function_call", call_id="c1", name="spec_file_access_read", arguments="{}") + unprefixed_read = Content("function_call", call_id="c2", name="file_access_read", arguments="{}") + prefixed_write = Content("function_call", call_id="c3", name="spec_file_access_write", arguments="{}") + hosted_read = Content( + "function_call", + call_id="c4", + name="spec_file_access_read", + arguments="{}", + hosted=True, + ) + + assert rule(prefixed_read) is True + assert rule(unprefixed_read) is False + assert rule(prefixed_write) is False + assert rule(hosted_read) is False + + def test_all_tools_auto_approval_rule() -> None: """The all-tools rule approves every file-access tool but nothing else.""" for name in ( @@ -1007,6 +1112,31 @@ def test_all_tools_auto_approval_rule() -> None: assert FileAccessProvider.all_tools_auto_approval_rule(unrelated) is False +def test_prefixed_provider_all_tools_auto_approval_rule() -> None: + """A provider-bound rule should approve only that provider's prefixed file-access tools.""" + provider = FileAccessProvider(InMemoryAgentFileStore(), tool_name_prefix="spec") + rule = provider.get_all_tools_auto_approval_rule() + tool_names = ( + FileAccessProvider.WRITE_TOOL_NAME, + FileAccessProvider.READ_TOOL_NAME, + FileAccessProvider.READ_LINES_TOOL_NAME, + FileAccessProvider.DELETE_TOOL_NAME, + FileAccessProvider.LS_TOOL_NAME, + FileAccessProvider.GREP_TOOL_NAME, + FileAccessProvider.REPLACE_TOOL_NAME, + FileAccessProvider.REPLACE_LINES_TOOL_NAME, + ) + + for name in tool_names: + prefixed = Content("function_call", call_id="c1", name=f"spec_{name}", arguments="{}") + unprefixed = Content("function_call", call_id="c2", name=name, arguments="{}") + hosted = Content("function_call", call_id="c3", name=f"spec_{name}", arguments="{}", hosted=True) + + assert rule(prefixed) is True + assert rule(unprefixed) is False + assert rule(hosted) is False + + async def test_file_access_provider_tools_round_trip_files( chat_client_base: SupportsChatGetResponse, ) -> None: