From a02ca706a29b1554aee79fb37c6e072261ebb92f Mon Sep 17 00:00:00 2001 From: Anton Sokolovskyi Date: Fri, 14 Aug 2026 12:59:57 +0200 Subject: [PATCH 1/5] Python: Add file_access_read_lines to the harness file tools The harness file tools are line-precise when editing but all-or-nothing when reading, so there is no way to see the lines around a file_access_grep match before editing them. Models either re-read the whole file, which is expensive and gets truncated on exactly the large files where partial reads matter, or skip the read and guess at the line. Add file_access_read_lines(file_name, start_line, end_line=None): a 1-based inclusive range read returning numbered text, headed by the total line count and the file's line-ending style. Omit end_line to read to the end of the file; a value past the last line clamps to it, while an out-of-range start_line is reported. Line numbers come from the same _split_lines_keepends split that backs 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. The header names the line endings because grep strips terminators while replace_lines takes them literally. The tool is read-only: it joins _READ_ONLY_TOOL_NAMES so both static auto-approval rules cover it, it follows disable_readonly_tool_approval, and it stays advertised under disable_write_tools. Co-Authored-By: Claude Opus 5 (1M context) --- python/packages/core/AGENTS.md | 2 +- .../agent_framework/_harness/_file_access.py | 105 +++++++++++++-- .../tests/core/test_harness_file_access.py | 124 ++++++++++++++++++ python/samples/02-agents/harness/README.md | 8 +- .../harness/build_your_own_claw/README.md | 3 +- 5 files changed, 228 insertions(+), 14 deletions(-) diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 593aa1e4b5..b6076473c1 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -124,7 +124,7 @@ agent_framework/ - **`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. - **`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 as numbered text prefixed by a header carrying the total line count and the file's line-ending style; `end_line` may be omitted to read to the end of the file, and an `end_line` past the last line clamps to it. 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/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 2a5fad18ce..1ef6039a98 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 @@ -284,6 +287,46 @@ def _line_edits(edits: list[Any]) -> list[tuple[int, str]]: return normalized +def _slice_lines(content: str, start_line: int, end_line: int | None) -> tuple[list[str], int]: + """Return the 1-based inclusive ``[start_line, end_line]`` slice of ``content`` and its total line count. + + 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)], total + + +def _strip_line_terminator(line: str) -> str: + r"""Drop a keepends line's terminator, reproducing ``grep``'s ``rstrip("\r")`` rendering.""" + return line.removesuffix("\n").rstrip("\r") + + +def _line_ending_style(content: str) -> str: + """Name ``content``'s line terminators, which reads strip but ``replace_lines`` takes literally.""" + crlf = content.count("\r\n") + lf = content.count("\n") - crlf + if crlf and lf: + return "mixed LF/CRLF" + return "CRLF" if crlf else "LF" + + @experimental(feature_id=ExperimentalFeature.HARNESS) class FileSearchMatch(SerializationMixin): """Represent one line within a file that matched a search pattern.""" @@ -1117,6 +1160,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 +1269,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 +1280,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 +1301,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 +1309,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 +1326,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 +1342,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, }) @@ -1471,6 +1536,30 @@ 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; that gutter is for reference only, never include it in a replacement 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, total = _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}" + header = ( + f"Lines {start_line}-{start_line + len(sliced) - 1} of '{file_name}' " + f"({total} lines total, {_line_ending_style(content)} line endings):" + ) + numbered = (f"{number}\t{_strip_line_terminator(line)}" for number, line in enumerate(sliced, start_line)) + return "\n".join([header, *numbered]) + @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.""" @@ -1583,7 +1672,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..acbe8b69a2 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -33,9 +33,12 @@ from agent_framework._harness._file_access import ( DEFAULT_FILE_ACCESS_INSTRUCTIONS, DEFAULT_FILE_ACCESS_SOURCE_ID, + _line_ending_style, _matches_glob, _normalize_relative_path, _run_search_with_timeout, + _slice_lines, + _strip_line_terminator, ) from .conftest import create_junction_or_skip @@ -525,6 +528,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 +562,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 +578,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 +615,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 +649,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 +1181,121 @@ async def current() -> str: assert "Duplicate" in _text(dup[0]) +def test_slice_lines_returns_inclusive_range_and_total() -> None: + """``_slice_lines`` should slice 1-based inclusive ranges and report the keepends line count.""" + assert _slice_lines("a\nb\nc\n", 1, 2) == (["a\n", "b\n"], 4) + assert _slice_lines("a\nb\nc\n", 2, 2) == (["b\n"], 4) + + # 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", ""], 3) + assert _slice_lines("a\nb\n", 3, 3) == ([""], 3) + + # Empty content is a single empty line, not zero lines. + assert _slice_lines("", 1, None) == ([""], 1) + + # 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"], 2) + + +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) + + +def test_line_rendering_helpers() -> None: + """Displayed line text should match grep's, and the ending style should describe the file.""" + assert _strip_line_terminator("a\r\n") == "a" + assert _strip_line_terminator("a\n") == "a" + assert _strip_line_terminator("a") == "a" + assert _strip_line_terminator("") == "" + + assert _line_ending_style("a\nb\n") == "LF" + assert _line_ending_style("a\r\nb\r\n") == "CRLF" + assert _line_ending_style("a\r\nb\n") == "mixed LF/CRLF" + assert _line_ending_style("abc") == "LF" + + +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") + header = "Lines {}-{} of 'a.txt' (4 lines total, LF line endings):" + assert await read(start_line=2, end_line=3) == f"{header.format(2, 3)}\n2\ttwo\n3\tthree" + + # Omitting end_line reads to the end, including the trailing empty line. + assert await read(start_line=1) == f"{header.format(1, 4)}\n1\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 + + # CRLF terminators are stripped from the displayed text and named in the header. + await write("alpha\r\nbeta\r\n") + crlf_header = "Lines 1-1 of 'a.txt' (3 lines total, CRLF line endings):" + assert await read(start_line=1, end_line=1) == f"{crlf_header}\n1\talpha" + + 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}))[0]) + assert shown.splitlines()[1] == f"{number}\t" + + # 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_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 +1349,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 From e42b672bd25ee5d8b4cda8c517951c628bde7146 Mon Sep 17 00:00:00 2001 From: Anton Sokolovskyi Date: Fri, 14 Aug 2026 19:00:27 +0200 Subject: [PATCH 2/5] Python: Render read_lines rows verbatim and drop the header Review feedback on #7571. The header largely echoed the tool call, and the gutter already reveals the end of the file: an end_line past the end comes back with a lower last number, and omitting end_line reads to EOF by definition. The total line count added nothing the caller could not derive. Keeping each line's terminator instead of stripping it removes the need for a line-ending indicator altogether. Every row carries its own terminator, so a mixed-ending file needs no detection, and the text after the gutter can be reused as a file_access_replace_lines new_line without dropping a \r\n. The terminator doubles as the row separator. Drops _strip_line_terminator and _line_ending_style, and returns _slice_lines to a plain list now that the total is unused. Co-Authored-By: Claude Opus 5 (1M context) --- python/packages/core/AGENTS.md | 2 +- .../agent_framework/_harness/_file_access.py | 32 ++------- .../tests/core/test_harness_file_access.py | 68 ++++++++++--------- 3 files changed, 45 insertions(+), 57 deletions(-) diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index b6076473c1..0b467b31ec 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -124,7 +124,7 @@ agent_framework/ - **`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. - **`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_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 as numbered text prefixed by a header carrying the total line count and the file's line-ending style; `end_line` may be omitted to read to the end of the file, and an `end_line` past the last line clamps to it. 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. +- **`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/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 1ef6039a98..48f9fac87f 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -287,8 +287,8 @@ def _line_edits(edits: list[Any]) -> list[tuple[int, str]]: return normalized -def _slice_lines(content: str, start_line: int, end_line: int | None) -> tuple[list[str], int]: - """Return the 1-based inclusive ``[start_line, end_line]`` slice of ``content`` and its total line count. +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 @@ -310,21 +310,7 @@ def _slice_lines(content: str, start_line: int, end_line: int | None) -> tuple[l 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)], total - - -def _strip_line_terminator(line: str) -> str: - r"""Drop a keepends line's terminator, reproducing ``grep``'s ``rstrip("\r")`` rendering.""" - return line.removesuffix("\n").rstrip("\r") - - -def _line_ending_style(content: str) -> str: - """Name ``content``'s line terminators, which reads strip but ``replace_lines`` takes literally.""" - crlf = content.count("\r\n") - lf = content.count("\n") - crlf - if crlf and lf: - return "mixed LF/CRLF" - return "CRLF" if crlf else "LF" + return lines[start_line - 1 : total if end_line is None else min(end_line, total)] @experimental(feature_id=ExperimentalFeature.HARNESS) @@ -1542,23 +1528,19 @@ async def file_access_read(file_name: str) -> str: 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; that gutter is for reference only, never include it in a replacement line.""" # ruff:ignore[line-too-long] + """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, total = _slice_lines(content, start_line, end_line) + 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}" - header = ( - f"Lines {start_line}-{start_line + len(sliced) - 1} of '{file_name}' " - f"({total} lines total, {_line_ending_style(content)} line endings):" - ) - numbered = (f"{number}\t{_strip_line_terminator(line)}" for number, line in enumerate(sliced, start_line)) - return "\n".join([header, *numbered]) + # 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: 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 acbe8b69a2..aa2ca131e2 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -33,12 +33,10 @@ from agent_framework._harness._file_access import ( DEFAULT_FILE_ACCESS_INSTRUCTIONS, DEFAULT_FILE_ACCESS_SOURCE_ID, - _line_ending_style, _matches_glob, _normalize_relative_path, _run_search_with_timeout, _slice_lines, - _strip_line_terminator, ) from .conftest import create_junction_or_skip @@ -1181,23 +1179,23 @@ async def current() -> str: assert "Duplicate" in _text(dup[0]) -def test_slice_lines_returns_inclusive_range_and_total() -> None: - """``_slice_lines`` should slice 1-based inclusive ranges and report the keepends line count.""" - assert _slice_lines("a\nb\nc\n", 1, 2) == (["a\n", "b\n"], 4) - assert _slice_lines("a\nb\nc\n", 2, 2) == (["b\n"], 4) +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", ""], 3) - assert _slice_lines("a\nb\n", 3, 3) == ([""], 3) + 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) == ([""], 1) + 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"], 2) + assert _slice_lines("a\r\nb", 1, 1) == ["a\r\n"] def test_slice_lines_rejects_invalid_ranges() -> None: @@ -1212,19 +1210,6 @@ def test_slice_lines_rejects_invalid_ranges() -> None: _slice_lines("a\nb\n", 99, None) -def test_line_rendering_helpers() -> None: - """Displayed line text should match grep's, and the ending style should describe the file.""" - assert _strip_line_terminator("a\r\n") == "a" - assert _strip_line_terminator("a\n") == "a" - assert _strip_line_terminator("a") == "a" - assert _strip_line_terminator("") == "" - - assert _line_ending_style("a\nb\n") == "LF" - assert _line_ending_style("a\r\nb\r\n") == "CRLF" - assert _line_ending_style("a\r\nb\n") == "mixed LF/CRLF" - assert _line_ending_style("abc") == "LF" - - 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) @@ -1238,21 +1223,19 @@ 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") - header = "Lines {}-{} of 'a.txt' (4 lines total, LF line endings):" - assert await read(start_line=2, end_line=3) == f"{header.format(2, 3)}\n2\ttwo\n3\tthree" + 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) == f"{header.format(1, 4)}\n1\tone\n2\ttwo\n3\tthree\n4\t" + 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 - # CRLF terminators are stripped from the displayed text and named in the header. + # Each line keeps its own terminator, so a CRLF line can be reused verbatim. await write("alpha\r\nbeta\r\n") - crlf_header = "Lines 1-1 of 'a.txt' (3 lines total, CRLF line endings):" - assert await read(start_line=1, end_line=1) == f"{crlf_header}\n1\talpha" + assert await read(start_line=1, end_line=1) == "1\talpha\r\n" await write("one\ntwo\n") for kwargs, expected in ( @@ -1285,8 +1268,10 @@ async def test_file_access_read_lines_shows_grep_line_numbers( # 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}))[0]) - assert shown.splitlines()[1] == f"{number}\t" + 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( @@ -1296,6 +1281,27 @@ async def test_file_access_read_lines_shows_grep_line_numbers( 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_line_numbers_are_editable(chat_client_base: SupportsChatGetResponse) -> None: """A ``line_number`` returned by ``file_access_grep`` must be in range for ``replace_lines``. From 3d46bd8744af08d57d56131513b1de78e2f936c6 Mon Sep 17 00:00:00 2001 From: Anton Sokolovskyi Date: Fri, 14 Aug 2026 19:06:02 +0200 Subject: [PATCH 3/5] Python: Report file_access_grep matches verbatim file_access_grep reported each hit with its terminator stripped, while file_access_replace_lines takes new_line literally. A model that grepped a CRLF file and then edited by line number had no way to know it should write \r\n, so the edit silently converted the line. That is the same gap file_access_read_lines closes on the read path, and it stays open for anyone who edits straight off a grep hit. _search_file_content now splits with _split_lines_keepends and reports the line verbatim. The pattern is still matched against the line without its trailing \n, so ^ and $ anchor per line as before, and snippet offsets and line numbers are unchanged. file_memory_grep gets the same behaviour, since it shares the store search. Co-Authored-By: Claude Opus 5 (1M context) --- python/packages/core/AGENTS.md | 2 +- .../agent_framework/_harness/_file_access.py | 23 +++++++++------- .../tests/core/test_harness_file_access.py | 27 +++++++++++++++++-- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 0b467b31ec..1d4a560c14 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -122,7 +122,7 @@ 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_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. diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 48f9fac87f..7d0eb687b6 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -325,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.") @@ -507,27 +507,30 @@ 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 ``\n``, so ``^`` and ``$`` anchor to the line as before. 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") + 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 + line_start_offset += len(scanned) + 1 if not matching_lines: return None @@ -1632,6 +1635,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 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 aa2ca131e2..209acf5e12 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -175,7 +175,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. @@ -348,7 +348,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") @@ -1302,6 +1302,29 @@ async def test_file_access_read_lines_round_trips_into_replace_lines( 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_line_numbers_are_editable(chat_client_base: SupportsChatGetResponse) -> None: """A ``line_number`` returned by ``file_access_grep`` must be in range for ``replace_lines``. From d97afaa066c29dc9bd976aac486aadd37e6b7514 Mon Sep 17 00:00:00 2001 From: Anton Sokolovskyi Date: Fri, 14 Aug 2026 20:25:22 +0200 Subject: [PATCH 4/5] Python: List file_access_read_lines in the read-only tool docstrings The new tool joined _READ_ONLY_TOOL_NAMES but several public docstrings still enumerated the read-only set as read/ls/grep. Two of them were actively misleading rather than merely stale: the disable_write_tools docs in both FileAccessProvider and create_harness_agent said only read/ls/grep stay advertised, implying file_access_read_lines is hidden when it is not. The security warning on read_only_tools_auto_approval_rule matters most. It lists the names the rule auto-approves so callers can avoid collisions, so leaving one out understates which names are reserved. Covers _file_access.py (disable_write_tools, disable_readonly_tool_approval, the rule description and its warning) and _agent.py (file_access_disable_write_tools, file_access_disable_readonly_tool_approval). Co-Authored-By: Claude Opus 5 (1M context) --- .../core/agent_framework/_harness/_agent.py | 4 ++-- .../agent_framework/_harness/_file_access.py | 23 ++++++++++--------- 2 files changed, 14 insertions(+), 13 deletions(-) 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 7d0eb687b6..7742924b82 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -1369,12 +1369,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 @@ -1417,10 +1417,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 @@ -1429,11 +1429,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. From 7aa29c64cbca74f5c18cef5f95b85c667cf9548b Mon Sep 17 00:00:00 2001 From: Anton Sokolovskyi Date: Fri, 14 Aug 2026 22:44:24 +0200 Subject: [PATCH 5/5] Python: Strip the whole line terminator before matching in file_access_grep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the fix for the same defect found by review on the .NET side (#7671). _search_file_content removed only the trailing "\n" before matching, so on a CRLF file the pattern was applied to text such as "beta match\r" and an end-anchored pattern like "match$" failed even though the line's text is exactly "beta match". The terminator is not part of the line's text, so it is stripped in full now. The per-line offset had to move with it: it advanced by len(scanned) + 1, which was only correct while scanned still carried the "\r". It now advances by len(line), whose terminator is already included, keeping the snippet anchored at the match. Also drops a stale claim in _split_lines_keepends' docstring, which still said it reproduced _search_file_content's content.split("\n") — that dependency now runs the other way round. Co-Authored-By: Claude Opus 5 (1M context) --- .../agent_framework/_harness/_file_access.py | 17 ++++----- .../tests/core/test_harness_file_access.py | 36 +++++++++++++++++++ 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 7742924b82..4532eb0110 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -235,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. """ @@ -510,7 +510,8 @@ def _search_file_content(file_name: str, content: str, regex: re.Pattern[str]) - 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 ``\n``, so ``^`` and ``$`` anchor to the line as before. A snippet of up to + 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. """ @@ -520,7 +521,7 @@ def _search_file_content(file_name: str, content: str, regex: re.Pattern[str]) - line_start_offset = 0 for line_number, line in enumerate(lines, start=1): - scanned = line.removesuffix("\n") + 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)) @@ -529,8 +530,8 @@ def _search_file_content(file_name: str, content: str, regex: re.Pattern[str]) - 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(scanned) + 1 + # Advance past this line; its terminator is already part of its length. + line_start_offset += len(line) if not matching_lines: return 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 209acf5e12..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,6 +31,7 @@ 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, @@ -1325,6 +1326,41 @@ async def test_file_access_grep_reports_lines_verbatim(chat_client_base: Support 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``.