diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 593aa1e4b5..1d4a560c14 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -122,9 +122,9 @@ agent_framework/ - **`AgentFileStore`** - Abstract async store backing the file-access harness. Implementations expose `write`, `read`, `delete`, `list_children`, `file_exists`, `search`, and `create_directory` over forward-slash relative paths. `list_children` returns the direct children (files and subdirectories, subdirectories first) as `FileStoreEntry` instances; `search` accepts a keyword-only `recursive` flag (default `False`) and, when `recursive=True`, walks all descendants and returns `file_name` values relative to the search directory. - **`InMemoryAgentFileStore`** - Dict-backed store suitable for tests and lightweight scenarios. - **`FileSystemAgentFileStore`** - Disk-backed store rooted under a configurable directory. Enforces relative-path normalization, root containment, and rejects symlink/reparse-point segments to prevent escape. -- **`FileSearchResult`** / **`FileSearchMatch`** - `SerializationMixin` DTOs returned by `search`, carrying the matching file name, a context snippet, and the matching lines with 1-based line numbers. +- **`FileSearchResult`** / **`FileSearchMatch`** - `SerializationMixin` DTOs returned by `search`, carrying the matching file name, a context snippet, and the matching lines with 1-based line numbers. Each matching line is verbatim, including its own terminator, so it can be reused as a `file_access_replace_lines` `new_line`; the pattern itself is matched against the line without its trailing `\n`, so `^`/`$` anchor per line as before. - **`FileStoreEntry`** - `SerializationMixin` DTO returned by `list_children`, carrying an entry `name` and `type` (`"file"` or `"directory"`). -- **`FileAccessProvider`** - `ContextProvider` that adds shared file-access tools (`file_access_write`, `file_access_read`, `file_access_delete`, `file_access_ls`, `file_access_grep`, `file_access_replace`, `file_access_replace_lines`) plus default usage instructions to each invocation. `file_access_ls` enumerates direct children (both files and subdirectories) as `{name, type}` entries with an optional `glob_pattern`, so the agent can walk the tree level by level; `file_access_grep` searches recursively from an optional base `directory` and returns relative `file_name` paths, scoped via an `fnmatch` `glob_pattern` (where `*` crosses `/`, e.g. `*.md`, `reports/*`). `file_access_replace` substitutes `old_string` with `new_string` (failing if not found, or if multiple matches and `replace_all` is false); `file_access_replace_lines` replaces whole 1-based lines with literal text (each `new_line` includes its own trailing newline; an empty `new_line` deletes the line, including its line break). All tools are registered with `approval_mode="always_require"` by default, so every file operation needs host approval. Pass `disable_write_tools=True` to advertise only the read-only tools. To run unattended you can disable approval at the source with `disable_readonly_tool_approval=True` (read, ls, grep) and/or `disable_write_tool_approval=True` (write, delete, replace, replace_lines), which register the affected tools with `approval_mode="never_require"`; alternatively, keep approval on and pass one of the static auto-approval rules to `ToolApprovalMiddleware` (via `auto_approval_rules`): `FileAccessProvider.read_only_tools_auto_approval_rule` approves only the read-only tools (read, ls, grep), while `FileAccessProvider.all_tools_auto_approval_rule` approves every file-access tool including the write tools. Both rules reject any call carrying a `server_label` so they stay scoped to this provider's local tools and never auto-approve a same-named hosted tool. The tool names are also exposed as class constants (`WRITE_TOOL_NAME`, `READ_TOOL_NAME`, `DELETE_TOOL_NAME`, `LS_TOOL_NAME`, `GREP_TOOL_NAME`, `REPLACE_TOOL_NAME`, `REPLACE_LINES_TOOL_NAME`). Unlike `MemoryContextProvider`, the store is intentionally shared across sessions and agents. +- **`FileAccessProvider`** - `ContextProvider` that adds shared file-access tools (`file_access_write`, `file_access_read`, `file_access_read_lines`, `file_access_delete`, `file_access_ls`, `file_access_grep`, `file_access_replace`, `file_access_replace_lines`) plus default usage instructions to each invocation. `file_access_ls` enumerates direct children (both files and subdirectories) as `{name, type}` entries with an optional `glob_pattern`, so the agent can walk the tree level by level; `file_access_grep` searches recursively from an optional base `directory` and returns relative `file_name` paths, scoped via an `fnmatch` `glob_pattern` (where `*` crosses `/`, e.g. `*.md`, `reports/*`). `file_access_replace` substitutes `old_string` with `new_string` (failing if not found, or if multiple matches and `replace_all` is false); `file_access_replace_lines` replaces whole 1-based lines with literal text (each `new_line` includes its own trailing newline; an empty `new_line` deletes the line, including its line break). `file_access_read_lines` returns a 1-based inclusive line range, one line per row as `\t`; `end_line` may be omitted to read to the end of the file, and an `end_line` past the last line clamps to it. Everything after the tab is verbatim, including the line's own terminator (which therefore doubles as the row separator), so a row's text can be fed straight back as a `file_access_replace_lines` `new_line` without losing a `\r\n`. Its line numbering comes from the same `_split_lines_keepends` split as `file_access_grep` and `file_access_replace_lines`, so a number reported by grep addresses the same line in all three tools, including the trailing empty line of a newline-terminated file. All tools are registered with `approval_mode="always_require"` by default, so every file operation needs host approval. Pass `disable_write_tools=True` to advertise only the read-only tools. To run unattended you can disable approval at the source with `disable_readonly_tool_approval=True` (read, read_lines, ls, grep) and/or `disable_write_tool_approval=True` (write, delete, replace, replace_lines), which register the affected tools with `approval_mode="never_require"`; alternatively, keep approval on and pass one of the static auto-approval rules to `ToolApprovalMiddleware` (via `auto_approval_rules`): `FileAccessProvider.read_only_tools_auto_approval_rule` approves only the read-only tools (read, read_lines, ls, grep), while `FileAccessProvider.all_tools_auto_approval_rule` approves every file-access tool including the write tools. Both rules reject any call carrying a `server_label` so they stay scoped to this provider's local tools and never auto-approve a same-named hosted tool. The tool names are also exposed as class constants (`WRITE_TOOL_NAME`, `READ_TOOL_NAME`, `READ_LINES_TOOL_NAME`, `DELETE_TOOL_NAME`, `LS_TOOL_NAME`, `GREP_TOOL_NAME`, `REPLACE_TOOL_NAME`, `REPLACE_LINES_TOOL_NAME`). Unlike `MemoryContextProvider`, the store is intentionally shared across sessions and agents. ### File Memory Harness (`_harness/_file_memory.py`) diff --git a/python/packages/core/agent_framework/_harness/_agent.py b/python/packages/core/agent_framework/_harness/_agent.py index 8ad199069f..c366cbb2d0 100644 --- a/python/packages/core/agent_framework/_harness/_agent.py +++ b/python/packages/core/agent_framework/_harness/_agent.py @@ -446,11 +446,11 @@ def create_harness_agent( file access tools. When set, a FileAccessProvider is added, giving the agent shared read/write file tools backed by the supplied store. file_access_disable_write_tools: When True, the FileAccessProvider advertises only its - read-only tools (read, ls, grep); the write tools (write, delete, replace, + read-only tools (read, read_lines, ls, grep); the write tools (write, delete, replace, replace_lines) are hidden. When False (default), all tools are advertised. Only used when file_access_store is set. file_access_disable_readonly_tool_approval: When True, the FileAccessProvider's read-only - tools (read, ls, grep) are registered with ``approval_mode="never_require"`` so they + tools (read, read_lines, ls, grep) are registered with ``approval_mode="never_require"`` so they run without host approval. When False (default), they require approval. Only used when file_access_store is set. file_access_disable_write_tool_approval: When True, the FileAccessProvider's write tools diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 2a5fad18ce..4532eb0110 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -6,9 +6,9 @@ session-scoped memory that may be isolated per session, :class:`FileAccessProvider` operates on a shared, persistent storage area whose contents are visible across sessions and agents. The provider exposes tools — ``file_access_write``, -``file_access_read``, ``file_access_delete``, ``file_access_ls``, -``file_access_grep``, ``file_access_replace``, and ``file_access_replace_lines`` — -by registering them on the per-invocation +``file_access_read``, ``file_access_read_lines``, ``file_access_delete``, +``file_access_ls``, ``file_access_grep``, ``file_access_replace``, and +``file_access_replace_lines`` — by registering them on the per-invocation :class:`~agent_framework.SessionContext` in :meth:`FileAccessProvider.before_run`. The store abstraction is generic so callers can plug in in-memory, local-disk, or @@ -58,7 +58,10 @@ "- Files may be organized into subdirectories. Use `file_access_ls` " "to explore the tree level by level, " "or `file_access_grep` to search file contents recursively across " - "the whole store." + "the whole store.\n" + "- To change part of a file, find the line numbers with `file_access_grep`, " + "read the range around them with `file_access_read_lines`, then edit with " + "`file_access_replace_lines`. Reading the whole file first is rarely necessary." ) # Maximum number of characters of context to include on either side of the first @@ -232,10 +235,10 @@ def _apply_replace(content: str, old_string: str, new_string: str, replace_all: def _split_lines_keepends(content: str) -> list[str]: r"""Split ``content`` into lines on ``\n`` only, keeping the terminator attached. - Splits solely on ``\n`` (a trailing ``\r`` stays as line content), reproducing - :func:`_search_file_content`'s ``content.split("\n")`` enumeration exactly, so a - ``line_number`` obtained from ``grep`` always targets the same line here and stays - in range. This means the result has ``len(content.split("\n"))`` elements: a + This is the single definition of a line shared by ``grep``, ``read_lines`` and + ``replace_lines``, so a ``line_number`` obtained from one always targets the same + line in the others and stays in range. Splitting solely on ``\n`` (a trailing + ``\r`` stays attached to the line) means the result has trailing ``\n`` yields a final empty (editable) line, and empty content yields a single empty line. ``"".join(...)`` reproduces ``content`` verbatim. """ @@ -284,6 +287,32 @@ def _line_edits(edits: list[Any]) -> list[tuple[int, str]]: return normalized +def _slice_lines(content: str, start_line: int, end_line: int | None) -> list[str]: + """Return the 1-based inclusive ``[start_line, end_line]`` slice of ``content``, terminators kept. + + Uses :func:`_split_lines_keepends`, so a ``line_number`` from ``grep`` addresses the + same line here and in ``replace_lines``, including the trailing empty line of a + newline-terminated file. ``end_line`` of ``None`` reads to the end of the file, and an + ``end_line`` past the last line is clamped rather than rejected. Validating + ``start_line`` before clamping keeps every successful slice non-empty. + + Raises: + ValueError: When ``start_line`` or ``end_line`` is not positive, when ``end_line`` + precedes ``start_line``, or when ``start_line`` is past the last line. + """ + lines = _split_lines_keepends(content) + total = len(lines) + if start_line < 1: + raise ValueError(f"start_line must be a positive integer, got {start_line}.") + if end_line is not None and end_line < 1: + raise ValueError(f"end_line must be a positive integer, got {end_line}.") + if end_line is not None and end_line < start_line: + raise ValueError(f"end_line ({end_line}) must not be less than start_line ({start_line}).") + if start_line > total: + raise ValueError(f"start_line {start_line} is out of range (file has {total} lines).") + return lines[start_line - 1 : total if end_line is None else min(end_line, total)] + + @experimental(feature_id=ExperimentalFeature.HARNESS) class FileSearchMatch(SerializationMixin): """Represent one line within a file that matched a search pattern.""" @@ -296,7 +325,7 @@ def __init__(self, line_number: int, line: str) -> None: Args: line_number: The 1-based line number where the match was found. - line: The content of the matching line (trailing ``\r`` removed). + line: The matching line verbatim, including its own terminator. """ if line_number < 1: raise ValueError("line_number must be a positive integer.") @@ -478,27 +507,31 @@ def __repr__(self) -> str: def _search_file_content(file_name: str, content: str, regex: re.Pattern[str]) -> FileSearchResult | None: r"""Search one file's content and return a :class:`FileSearchResult` if any lines match. - Lines are split on ``\n`` (so ``\r`` at the end of each line is stripped on - the matching line itself). A snippet of up to ``±_SEARCH_SNIPPET_RADIUS`` - characters around the first match is included. Returns ``None`` when no - lines match. + Lines are split by :func:`_split_lines_keepends` and reported verbatim, terminator + included, so a match can be fed straight back to ``replace_lines`` as a ``new_line`` + without losing a ``\r\n``. The pattern is matched against the line without its + trailing terminator, so ``$`` anchors to the end of the line's text even on a CRLF + file. A snippet of up to + ``±_SEARCH_SNIPPET_RADIUS`` characters around the first match is included. Returns + ``None`` when no lines match. """ - lines = content.split("\n") + lines = _split_lines_keepends(content) matching_lines: list[FileSearchMatch] = [] first_snippet: str | None = None line_start_offset = 0 for line_number, line in enumerate(lines, start=1): - match = regex.search(line) + scanned = line.removesuffix("\n").removesuffix("\r") + match = regex.search(scanned) if match is not None: - matching_lines.append(FileSearchMatch(line_number=line_number, line=line.rstrip("\r"))) + matching_lines.append(FileSearchMatch(line_number=line_number, line=line)) if first_snippet is None: char_index = line_start_offset + match.start() snippet_start = max(0, char_index - _SEARCH_SNIPPET_RADIUS) snippet_end = min(len(content), char_index + (match.end() - match.start()) + _SEARCH_SNIPPET_RADIUS) first_snippet = content[snippet_start:snippet_end] - # Advance past this line and the implied '\n' separator. - line_start_offset += len(line) + 1 + # Advance past this line; its terminator is already part of its length. + line_start_offset += len(line) if not matching_lines: return None @@ -1117,6 +1150,23 @@ class _ReadFileInput(BaseModel): file_name: Annotated[str, Field(description="Name (relative path) of the file to read.")] +class _ReadLinesInput(BaseModel): + """Input schema for ``file_access_read_lines``.""" + + file_name: Annotated[str, Field(description="Name (relative path) of the file to read.")] + start_line: Annotated[int, Field(description="1-based line number to read from, inclusive.")] + end_line: Annotated[ + int | None, + Field( + default=None, + description=( + "1-based line number to read to, inclusive. Omit to read to the end of the file; " + "a value past the last line is clamped to it." + ), + ), + ] = None + + class _DeleteFileInput(BaseModel): """Input schema for ``file_access_delete``.""" @@ -1209,6 +1259,8 @@ class FileAccessProvider(ContextProvider): - ``file_access_write`` — Write a file (refuses to overwrite by default). - ``file_access_read`` — Read the content of a file by name. + - ``file_access_read_lines`` — Read a range of lines from a file by 1-based + inclusive line number. - ``file_access_delete`` — Delete a file by name. - ``file_access_ls`` — List the direct child files and subdirectories of a directory, optionally filtered by a glob pattern. @@ -1218,7 +1270,7 @@ class FileAccessProvider(ContextProvider): - ``file_access_replace_lines`` — Replace whole lines within a file. When ``disable_write_tools`` is set, only the read-only tools (``file_access_read``, - ``file_access_ls``, ``file_access_grep``) are advertised. + ``file_access_read_lines``, ``file_access_ls``, ``file_access_grep``) are advertised. Unlike :class:`~agent_framework.MemoryContextProvider`, which provides session-scoped memory that may be isolated per session, @@ -1239,7 +1291,7 @@ class FileAccessProvider(ContextProvider): to drive that handshake; otherwise these tools never run. To run unattended you can disable approval at the source with - ``disable_readonly_tool_approval`` (read, ls, grep) and/or + ``disable_readonly_tool_approval`` (read, read_lines, ls, grep) and/or ``disable_write_tool_approval`` (write, delete, replace, replace_lines), which register the affected tools with ``approval_mode="never_require"``. Alternatively, keep approval on and supply one of the static auto-approval @@ -1247,7 +1299,7 @@ class FileAccessProvider(ContextProvider): ``auto_approval_rules``: - :meth:`read_only_tools_auto_approval_rule` — auto-approves only the - read-only tools (read, ls, grep), while still prompting for the tools that + read-only tools (read, read_lines, ls, grep), while still prompting for the tools that modify the store (write, delete, replace, replace_lines). - :meth:`all_tools_auto_approval_rule` — auto-approves every file-access tool, including the write tools. @@ -1264,6 +1316,8 @@ class FileAccessProvider(ContextProvider): WRITE_TOOL_NAME = "file_access_write" #: Name of the tool that reads a file. READ_TOOL_NAME = "file_access_read" + #: Name of the tool that reads a range of lines from a file. + READ_LINES_TOOL_NAME = "file_access_read_lines" #: Name of the tool that deletes a file. DELETE_TOOL_NAME = "file_access_delete" #: Name of the tool that lists the files and subdirectories of a directory. @@ -1278,6 +1332,7 @@ class FileAccessProvider(ContextProvider): #: Names of the tools that only read from (never modify) the file store. _READ_ONLY_TOOL_NAMES: frozenset[str] = frozenset({ READ_TOOL_NAME, + READ_LINES_TOOL_NAME, LS_TOOL_NAME, GREP_TOOL_NAME, }) @@ -1315,12 +1370,12 @@ def __init__( instructions: Optional instruction override. When ``None`` the default file-access instructions are used. disable_write_tools: When ``True``, only the read-only tools - (``file_access_read``, ``file_access_ls``, ``file_access_grep``) + (``file_access_read``, ``file_access_read_lines``, ``file_access_ls``, ``file_access_grep``) are advertised; the write tools (``file_access_write``, ``file_access_delete``, ``file_access_replace``, ``file_access_replace_lines``) are hidden from the model. disable_readonly_tool_approval: When ``True``, the read-only tools - (``file_access_read``, ``file_access_ls``, ``file_access_grep``) + (``file_access_read``, ``file_access_read_lines``, ``file_access_ls``, ``file_access_grep``) are registered with ``approval_mode="never_require"`` so they run without host approval. Defaults to ``False`` (approval required). disable_write_tool_approval: When ``True``, the write tools @@ -1363,10 +1418,10 @@ def read_only_tools_auto_approval_rule(function_call: Content) -> bool: The tools exposed by :class:`FileAccessProvider` always require approval. Pass this rule to :class:`~agent_framework.ToolApprovalMiddleware` (via ``auto_approval_rules``) to automatically approve the tools that read - from the store (``file_access_read``, ``file_access_ls``, and - ``file_access_grep``), while still prompting for the tools that modify it - (``file_access_write``, ``file_access_delete``, ``file_access_replace``, - and ``file_access_replace_lines``). + from the store (``file_access_read``, ``file_access_read_lines``, + ``file_access_ls``, and ``file_access_grep``), while still prompting for + the tools that modify it (``file_access_write``, ``file_access_delete``, + ``file_access_replace``, and ``file_access_replace_lines``). Hosted-tool calls (those carrying a ``server_label``) are never auto-approved, even when their name matches a file-access tool, so the @@ -1375,11 +1430,12 @@ def read_only_tools_auto_approval_rule(function_call: Content) -> bool: .. warning:: **Security — avoid tool-name collisions.** This rule approves local tool calls by tool name only (``file_access_read``, - ``file_access_ls``, and ``file_access_grep``). Any other local tool - registered under one of these names — for example a tool with a - caller-configurable name such as the shell tool — may also be - auto-approved, bypassing the human approval boundary. Ensure no other - tool collides with these reserved names. + ``file_access_read_lines``, ``file_access_ls``, and + ``file_access_grep``). Any other local tool registered under one of + these names — for example a tool with a caller-configurable name such + as the shell tool — may also be auto-approved, bypassing the human + approval boundary. Ensure no other tool collides with these reserved + names. Args: function_call: The pending ``function_call`` content. @@ -1471,6 +1527,26 @@ async def file_access_read(file_name: str) -> str: return f"Could not read file '{file_name}': {exc.strerror or exc}" return content if content is not None else f"File '{file_name}' not found." + @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: + """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. Line numbers match file_access_grep and file_access_replace_lines. 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.""" # ruff:ignore[line-too-long] + try: + normalized = _normalize_relative_path(file_name) + content = await self.store.read(normalized) + if content is None: + return f"File '{file_name}' not found." + sliced = _slice_lines(content, start_line, end_line) + except ValueError as exc: + return f"Could not read lines from file '{file_name}': {exc}" + except OSError as exc: + return f"Could not read lines from file '{file_name}': {exc.strerror or exc}" + # 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) async def file_access_delete(file_name: str) -> str: """Delete a file by name.""" @@ -1561,6 +1637,8 @@ async def file_access_grep( 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. + Each matching line is verbatim, including its own line terminator, so it can be reused as a + file_access_replace_lines new_line. The regex_pattern must be 256 characters or fewer. """ glob_filter = glob_pattern if glob_pattern and glob_pattern.strip() else None @@ -1583,7 +1661,7 @@ async def file_access_grep( return output context.extend_instructions(self.source_id, [self.instructions]) - tools = [file_access_read, file_access_ls, file_access_grep] + tools = [file_access_read, file_access_read_lines, file_access_ls, file_access_grep] if not self.disable_write_tools: tools.extend([file_access_write, file_access_delete, file_access_replace, file_access_replace_lines]) context.extend_tools(self.source_id, tools) 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 19497be97d..9411d7f717 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -31,11 +31,13 @@ from agent_framework._filesystem import is_link_or_reparse_point from agent_framework._harness import _file_access as _file_access_module from agent_framework._harness._file_access import ( + _SEARCH_SNIPPET_RADIUS, DEFAULT_FILE_ACCESS_INSTRUCTIONS, DEFAULT_FILE_ACCESS_SOURCE_ID, _matches_glob, _normalize_relative_path, _run_search_with_timeout, + _slice_lines, ) from .conftest import create_junction_or_skip @@ -174,7 +176,7 @@ async def test_in_memory_store_search_returns_matches_with_snippets() -> None: results = await store.search("", "error", "*.md") assert [result.file_name for result in results] == ["a.md"] matching_lines = results[0].matching_lines - assert matching_lines == [FileSearchMatch(line_number=2, line="This line has ERROR inside")] + assert matching_lines == [FileSearchMatch(line_number=2, line="This line has ERROR inside\n")] assert "ERROR" in results[0].snippet # No glob -> searches every file. @@ -347,7 +349,7 @@ async def test_filesystem_store_search_matches_lines_and_filters_globs(tmp_path: results = await store.search("", "error", "*.md") assert [result.file_name for result in results] == ["a.md"] - assert results[0].matching_lines == [FileSearchMatch(line_number=2, line="ERROR happens")] + assert results[0].matching_lines == [FileSearchMatch(line_number=2, line="ERROR happens\n")] assert "ERROR" in results[0].snippet results_all = await store.search("", "error") @@ -525,6 +527,7 @@ async def test_file_access_provider_registers_tools_and_instructions( expected_names = { "file_access_write", "file_access_read", + "file_access_read_lines", "file_access_delete", "file_access_ls", "file_access_grep", @@ -558,6 +561,7 @@ async def test_file_access_provider_all_tools_require_approval( for name in ( 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, @@ -573,6 +577,7 @@ async def test_file_access_provider_approval_opt_outs( """The approval opt-out flags flip only the affected tool group to ``never_require``.""" readonly_names = ( FileAccessProvider.READ_TOOL_NAME, + FileAccessProvider.READ_LINES_TOOL_NAME, FileAccessProvider.LS_TOOL_NAME, FileAccessProvider.GREP_TOOL_NAME, ) @@ -609,6 +614,7 @@ def test_read_only_tools_auto_approval_rule() -> None: """The read-only rule approves only the non-mutating tools.""" approved = { FileAccessProvider.READ_TOOL_NAME, + FileAccessProvider.READ_LINES_TOOL_NAME, FileAccessProvider.LS_TOOL_NAME, FileAccessProvider.GREP_TOOL_NAME, } @@ -642,6 +648,7 @@ def test_all_tools_auto_approval_rule() -> None: for name in ( 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, @@ -1173,6 +1180,187 @@ async def current() -> str: assert "Duplicate" in _text(dup[0]) +def test_slice_lines_returns_inclusive_range() -> None: + """``_slice_lines`` should slice 1-based inclusive ranges with terminators kept.""" + assert _slice_lines("a\nb\nc\n", 1, 2) == ["a\n", "b\n"] + assert _slice_lines("a\nb\nc\n", 2, 2) == ["b\n"] + + # A trailing newline yields a final empty line that is in range, matching grep/replace_lines. + assert _slice_lines("a\nb\n", 1, None) == ["a\n", "b\n", ""] + assert _slice_lines("a\nb\n", 3, 3) == [""] + + # Empty content is a single empty line, not zero lines. + assert _slice_lines("", 1, None) == [""] + + # An end_line past the last line clamps instead of failing. + assert _slice_lines("a\nb\n", 1, 99) == _slice_lines("a\nb\n", 1, None) + + # Splitting on "\n" only leaves CRLF terminators attached. + assert _slice_lines("a\r\nb", 1, 1) == ["a\r\n"] + + +def test_slice_lines_rejects_invalid_ranges() -> None: + """``_slice_lines`` should reject non-positive, inverted, and past-the-end ranges.""" + with pytest.raises(ValueError, match="start_line must be a positive integer, got 0."): + _slice_lines("a\nb\n", 0, None) + with pytest.raises(ValueError, match="end_line must be a positive integer, got 0."): + _slice_lines("a\nb\n", 1, 0) + with pytest.raises(ValueError, match=r"end_line \(2\) must not be less than start_line \(3\)."): + _slice_lines("a\nb\n", 3, 2) + with pytest.raises(ValueError, match="start_line 99 is out of range"): + _slice_lines("a\nb\n", 99, None) + + +async def test_file_access_read_lines(chat_client_base: SupportsChatGetResponse) -> None: + """``file_access_read_lines`` should return a numbered range and report bad input as a message.""" + tools = await _prepare_access_tools(chat_client_base) + save = _tool_by_name(tools, "file_access_write") + read_lines = _tool_by_name(tools, "file_access_read_lines") + + async def write(content: str) -> None: + await save.invoke(arguments={"file_name": "a.txt", "content": content, "overwrite": True}) + + async def read(**kwargs: object) -> str: + return _text((await read_lines.invoke(arguments={"file_name": "a.txt", **kwargs}))[0]) + + await write("one\ntwo\nthree\n") + assert await read(start_line=2, end_line=3) == "2\ttwo\n3\tthree\n" + + # Omitting end_line reads to the end, including the trailing empty line. + assert await read(start_line=1) == "1\tone\n2\ttwo\n3\tthree\n4\t" + + # An end_line past the last line clamps rather than reporting an error. + clamped = await read(start_line=1, end_line=999) + assert clamped == await read(start_line=1) + assert "out of range" not in clamped + + # Each line keeps its own terminator, so a CRLF line can be reused verbatim. + await write("alpha\r\nbeta\r\n") + assert await read(start_line=1, end_line=1) == "1\talpha\r\n" + + await write("one\ntwo\n") + for kwargs, expected in ( + ({"start_line": 0}, "start_line must be a positive integer"), + ({"start_line": 9}, "start_line 9 is out of range"), + ({"start_line": 2, "end_line": 1}, "must not be less than start_line"), + ): + assert expected in await read(**kwargs) + + missing = await read_lines.invoke(arguments={"file_name": "missing.txt", "start_line": 1}) + assert _text(missing[0]) == "File 'missing.txt' not found." + + +async def test_file_access_read_lines_shows_grep_line_numbers( + chat_client_base: SupportsChatGetResponse, +) -> None: + """A ``line_number`` from grep must address the same line in read_lines and replace_lines.""" + tools = await _prepare_access_tools(chat_client_base) + save = _tool_by_name(tools, "file_access_write") + read = _tool_by_name(tools, "file_access_read") + grep = _tool_by_name(tools, "file_access_grep") + read_lines = _tool_by_name(tools, "file_access_read_lines") + replace_lines = _tool_by_name(tools, "file_access_replace_lines") + + await save.invoke(arguments={"file_name": "a.txt", "content": "a\n\nc\n", "overwrite": True}) + result = await grep.invoke(arguments={"regex_pattern": "^$", "glob_pattern": "a.txt"}) + payload = json.loads(_text(result[0])) + blanks = [match["line_number"] for entry in payload for match in entry["matching_lines"]] + assert blanks == [2, 4] + + # Both blank lines grep reports are readable, including the trailing one. + for number in blanks: + shown = _text( + (await read_lines.invoke(arguments={"file_name": "a.txt", "start_line": number, "end_line": number}))[0] + ) + assert shown.removeprefix(f"{number}\t") == ("\n" if number == 2 else "") + + # The same number is then editable, closing the grep -> read -> edit loop. + edited = await replace_lines.invoke( + arguments={"file_name": "a.txt", "edits": [{"line_number": blanks[-1], "new_line": "d\n"}]} + ) + assert "out of range" not in _text(edited[0]) + assert _text((await read.invoke(arguments={"file_name": "a.txt"}))[0]) == "a\n\nc\nd\n" + + +async def test_file_access_read_lines_round_trips_into_replace_lines( + chat_client_base: SupportsChatGetResponse, +) -> None: + """The text after the gutter is literal, so a CRLF line survives a read-then-edit round trip.""" + tools = await _prepare_access_tools(chat_client_base) + save = _tool_by_name(tools, "file_access_write") + read = _tool_by_name(tools, "file_access_read") + read_lines = _tool_by_name(tools, "file_access_read_lines") + replace_lines = _tool_by_name(tools, "file_access_replace_lines") + + await save.invoke(arguments={"file_name": "a.txt", "content": "alpha\r\nbeta\r\n", "overwrite": True}) + shown = _text((await read_lines.invoke(arguments={"file_name": "a.txt", "start_line": 2, "end_line": 2}))[0]) + assert shown == "2\tbeta\r\n" + + # Rewriting the line with the text it reported keeps the CRLF terminator intact. + await replace_lines.invoke( + arguments={"file_name": "a.txt", "edits": [{"line_number": 2, "new_line": shown.removeprefix("2\t").upper()}]} + ) + assert _text((await read.invoke(arguments={"file_name": "a.txt"}))[0]) == "alpha\r\nBETA\r\n" + + +async def test_file_access_grep_reports_lines_verbatim(chat_client_base: SupportsChatGetResponse) -> None: + """A grep hit keeps its terminator, so it round-trips into replace_lines like a read_lines row.""" + tools = await _prepare_access_tools(chat_client_base) + save = _tool_by_name(tools, "file_access_write") + read = _tool_by_name(tools, "file_access_read") + grep = _tool_by_name(tools, "file_access_grep") + replace_lines = _tool_by_name(tools, "file_access_replace_lines") + + await save.invoke(arguments={"file_name": "a.txt", "content": "alpha\r\nbeta\r\n", "overwrite": True}) + found = await grep.invoke(arguments={"regex_pattern": "beta", "glob_pattern": "a.txt"}) + hit = json.loads(_text(found[0]))[0]["matching_lines"][0] + assert hit["line_number"] == 2 + assert hit["line"] == "beta\r\n" + + await replace_lines.invoke( + arguments={ + "file_name": "a.txt", + "edits": [{"line_number": hit["line_number"], "new_line": hit["line"].upper()}], + } + ) + assert _text((await read.invoke(arguments={"file_name": "a.txt"}))[0]) == "alpha\r\nBETA\r\n" + + +async def test_file_access_grep_end_anchored_pattern_matches_crlf_line( + chat_client_base: SupportsChatGetResponse, +) -> None: + r"""``$`` anchors to the end of the line's text, not to the ``\r`` of a CRLF terminator.""" + tools = await _prepare_access_tools(chat_client_base) + save = _tool_by_name(tools, "file_access_write") + grep = _tool_by_name(tools, "file_access_grep") + + await save.invoke(arguments={"file_name": "a.txt", "content": "alpha\r\nbeta match\r\n", "overwrite": True}) + found = await grep.invoke(arguments={"regex_pattern": "match$", "glob_pattern": "a.txt"}) + + hits = json.loads(_text(found[0]))[0]["matching_lines"] + assert [hit["line_number"] for hit in hits] == [2] + assert hits[0]["line"] == "beta match\r\n" + + +async def test_file_access_grep_snippet_is_anchored_at_the_match( + chat_client_base: SupportsChatGetResponse, +) -> None: + """The per-line offset must count the terminator, or every snippet drifts.""" + tools = await _prepare_access_tools(chat_client_base) + save = _tool_by_name(tools, "file_access_write") + grep = _tool_by_name(tools, "file_access_grep") + + # The first line is long enough that the snippet window is not clamped to the start of the file. + first_line = f"{'x' * 60}\r\n" + content = f"{first_line}needle\r\n" + await save.invoke(arguments={"file_name": "a.txt", "content": content, "overwrite": True}) + found = await grep.invoke(arguments={"regex_pattern": "needle", "glob_pattern": "a.txt"}) + + # The match starts right after the first line, so the window opens _SEARCH_SNIPPET_RADIUS before it. + snippet = json.loads(_text(found[0]))[0]["snippet"] + assert snippet == content[len(first_line) - _SEARCH_SNIPPET_RADIUS :] + + async def test_file_access_grep_line_numbers_are_editable(chat_client_base: SupportsChatGetResponse) -> None: """A ``line_number`` returned by ``file_access_grep`` must be in range for ``replace_lines``. @@ -1226,6 +1414,7 @@ async def test_file_access_disable_write_tools_hides_write_tools( tools = await _prepare_access_tools(chat_client_base, disable_write_tools=True) names = {getattr(tool, "name", None) for tool in tools} assert "file_access_read" in names + assert "file_access_read_lines" in names assert "file_access_ls" in names assert "file_access_grep" in names assert "file_access_write" not in names diff --git a/python/samples/02-agents/harness/README.md b/python/samples/02-agents/harness/README.md index a5215a22d6..20ccc49780 100644 --- a/python/samples/02-agents/harness/README.md +++ b/python/samples/02-agents/harness/README.md @@ -162,10 +162,10 @@ for vetting the external service, agent, skill source, or provider before enabli - **Auto-approval rules** (`FileAccessProvider.read_only_tools_auto_approval_rule` / `all_tools_auto_approval_rule`, and the equivalent `SkillsProvider` rules, passed to `ToolApprovalMiddleware`) — the built-in rules approve local tools by tool name only (e.g. - `file_access_read`, `file_access_ls`, `file_access_grep`). Auto-approval rules may match by name, - so any other local tool registered under one of these names — for example the shell tool given a - caller-configurable name — may also be auto-approved, bypassing the human approval boundary. Ensure - no other tool collides with these reserved names. + `file_access_read`, `file_access_read_lines`, `file_access_ls`, `file_access_grep`). Auto-approval + rules may match by name, so any other local tool registered under one of these names — for example + the shell tool given a caller-configurable name — may also be auto-approved, bypassing the human + approval boundary. Ensure no other tool collides with these reserved names. - **Telemetry** — when observability is enabled, telemetry destinations are developer-configured. Default telemetry is metadata only; enabling sensitive data additionally emits raw message content, tool arguments, and tool results. See the [observability samples](../observability/README.md). diff --git a/python/samples/02-agents/harness/build_your_own_claw/README.md b/python/samples/02-agents/harness/build_your_own_claw/README.md index b1c90ac784..69a4f12137 100644 --- a/python/samples/02-agents/harness/build_your_own_claw/README.md +++ b/python/samples/02-agents/harness/build_your_own_claw/README.md @@ -78,7 +78,8 @@ Teaches the assistant to work with *your* data safely. > ⚠️ **Security — avoid tool-name collisions:** `read_only_tools_auto_approval_rule` > approves local file-access tools by tool name only (`file_access_read`, - > `file_access_ls`, `file_access_grep`). Auto-approval rules may match by name, + > `file_access_read_lines`, `file_access_ls`, `file_access_grep`). Auto-approval + > rules may match by name, > so any other local tool registered under one of these names — for example a > tool with a caller-configurable name such as the shell tool — may also be > auto-approved, bypassing the human approval boundary. Ensure no other tool