Python: [BREAKING] Issue 7571 file access read lines

[Anton Sokolovskyi (antsok)]

Motivation & Context

The harness file tools are line-precise when editing: file_access_replace_lines takes 1-based line numbers and file_access_grep reports them — but all-or-nothing when reading. There is no way to see the lines around a grep match before editing them, so a model either re-reads the whole file (expensive, and truncated by host result caps on exactly the large files where a partial read matters) or skips the read and guesses at the line, which is the wrong-line edit the line-precise editor exists to prevent.

A second, related gap surfaced while implementing this. file_access_grep reported each match with its terminator stripped, while file_access_replace_lines takes new_line literally. An agent that grepped a CRLF file and edited by line number had
no way to know it should write \r\n, so the edit silently converted that line - and the only way to detect the file's line-ending style was a whole-file read, the very fallback this change removes.

Description & Review Guide

• What are the major changes?

file_access_read_lines(file_name, start_line, end_line=None) reads a 1-based inclusive range. Omit end_line to read to the end of the file; an end_line past the last line clamps to the last line, while an out-of-range start_line is reported. Rows are <line_number>\t<line>, and everything after the tab is verbatim, including the line's own terminator — which therefore doubles as the row separator - so a row's text feeds straight back into file_access_replace_lines without losing a \r\n.

Line numbering comes from _split_lines_keepends, the same split used by grep and replace_lines, so a number maps to the same line in all three tools, including the trailing empty line of a newline-terminated file.

The clamping is a deliberate contrast with file_access_replace_lines, which rejects an out-of-range line_number outright: reading to the end of a file is a legitimate request, whereas editing a line that does not exist is a mistake.

file_access_grep now reports matches verbatim on the same contract. The pattern is matched against the line with its whole terminator removed — not just the \n — so $ anchors to the end of the line's text on a CRLF file as it already did on an LF one. Line numbers and snippet offsets are unchanged.

DEFAULT_FILE_ACCESS_INSTRUCTIONS gains a clause teaching the grep → read the range edit loop, which is what makes models reach for the new tool rather than defaulting to whole-file reads.

• What is the impact of these changes?

The new tool is read-only. It joins _READ_ONLY_TOOL_NAMES, so both static auto-approval rules cover it with no new approval logic; it follows disable_readonly_tool_approval, and it stays advertised under disable_write_tool. Consumers get one more tool definition per request; anyone using read_only_tools_auto_approval_rule picks it up automatically, while anyone with a hand-rolled rule listing the read-only tools by name will see it prompt for approval
until they add it.

The breaking part is grep, in two ways. matching_lines[].line now carries its terminator, and FileSearchMatch.line changes meaning with it. And because the whole terminator is stripped before matching, an end-anchored pattern such as match$ now matches on a CRLF line where it previously could not, while a pattern targeting a literal \r no longer matches the one such a line ends with. The first direction is the fix — match$ failing on a line whose text is exactly match was a defect — and the second is its unavoidable cost. Line numbers and snippet offsets are unchanged in both cases.

file_memory_grep inherits all of the above, since it shares the store search. file_memory_read_lines is deliberately not added: it would widen this PR by a tool and its approval surface, and the .NET port does not add the memory counterpart either. That does leave FileMemoryProvider with the same read asymmetry this PR removes from FileAccessProvider, which I am happy to file as a follow-up covering both languages.

The instruction changes alter injected
prompt text for consumers on the default; anyone passing their own instructions= is unaffected. There is no change to AgentFileStore and no change to any other tool's signature or behaviour.

• What do you want reviewers to focus on?

Whether the grep alignment belongs in this PR or should be split out - it is a separate commit so it can be dropped. And the verbatim-terminator contract itself, since that is what the tool's usefulness rests on.

Related Issue

#7571

Deliberately linked without a closing keyword: this covers the Python half only, and the .NET port follows in a separate PR. Please leave the issue open until that lands. Will change to Closes in the last one.

Contribution Checklist

• The code builds cleanly without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR is linked to an issue, and there is no other open PR for this issue (see Related Issue above).
• This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

[Copilot]

Pull request overview

Adds line-range reads to Python harness file tools while preserving line-number and terminator semantics.

Changes:

• Adds file_access_read_lines with approval integration.
• Changes grep matches to retain line terminators.
• Adds tests and documentation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
python/samples/02-agents/harness/README.md	Documents the new auto-approved tool name.
python/samples/02-agents/harness/build_your_own_claw/README.md	Updates security guidance.
python/packages/core/tests/core/test_harness_file_access.py	Tests range reads, terminators, and approvals.
python/packages/core/AGENTS.md	Documents new behavior and API.
python/packages/core/agent_framework/_harness/_file_access.py	Implements range reads and grep changes.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

python/packages/core/agent_framework/_harness/_file_access.py:524

• This also changes which grep results match: previously content.split("\n") searched a CRLF line as "...\r", whereas this removes both terminator characters. Patterns such as match$ therefore start matching CRLF files, and patterns targeting \r stop matching, despite the PR description saying matching remains as before. Either remove only \n to preserve the old behavior, or explicitly document this additional breaking change and its impact.

        scanned = line.removesuffix("\n").removesuffix("\r")

python/packages/core/agent_framework/_harness/_file_access.py:243

• The element-count phrase was dropped, leaving “means the result has trailing … yields” grammatically incomplete. Restore the count clause so the split contract is readable.

    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.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

python/packages/core/agent_framework/_harness/_file_access.py:1541

• This model-facing claim is unconditional, but the provider accepts custom AgentFileStore implementations whose search contract does not require _split_lines_keepends; the new helper and AGENTS documentation explicitly acknowledge that their grep numbers may differ. As written, the model can trust the claim and edit the wrong line. Scope the guarantee to the built-in stores (or strengthen the base search contract).

            """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]

python/packages/core/agent_framework/_harness/_file_access.py:1646

• This guarantee also depends on the store implementation: file_access_grep serializes store.search() results unchanged, while the public base contract does not require custom stores to retain terminators (as the updated FileSearchMatch docs note). Calling every returned line verbatim may lead the model to feed a terminator-less custom result into replace_lines and join lines accidentally. Scope this text to the built-in stores or define the behavior in AgentFileStore.search.

            Each matching line is verbatim, including its own line terminator, so it can be reused as a
            file_access_replace_lines new_line.

python/packages/core/agent_framework/_harness/_file_access.py:1340

• The new name is folded into _ALL_TOOL_NAMES, so all_tools_auto_approval_rule now approves it, but that rule's security warning at lines 1472–1481 still gives an exhaustive reserved-name list that omits file_access_read_lines. Add the new name there so users do not accidentally reuse an auto-approved tool name.

        READ_LINES_TOOL_NAME,

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[Evan Mattson (moonbox3)]

Could the line-numbering rule live in AgentFileStore instead of being split between store.search() and _slice_lines() here? As written, file_access_grep gets numbers from a custom store's search(), while file_access_read_lines and file_access_replace_lines reinterpret the text returned by read() with _split_lines_keepends, so a valid custom store can make the same number refer to different lines. Defining one store-level range-read/line contract, with the built-in stores sharing a default implementation, would keep the grep -> read -> edit workflow safe without callers needing to know which store they were given.

[Anton Sokolovskyi (antsok)]

Could the line-numbering rule live in AgentFileStore instead of being split between store.search() and _slice_lines() here? As written, file_access_grep gets numbers from a custom store's search(), while file_access_read_lines and file_access_replace_lines reinterpret the text returned by read() with _split_lines_keepends, so a valid custom store can make the same number refer to different lines. Defining one store-level range-read/line contract, with the built-in stores sharing a default implementation, would keep the grep -> read -> edit workflow safe without callers needing to know which store they were given.

Yes — excellent feedback. The rule will live on AgentFileStore, and both shipped stores will go through it.

What the split costs today

search() is @abstractmethod with no numbering contract, so a custom store is free to count lines differently. The obvious idiom does: str.splitlines(keepends=True) breaks on \f, \v, \x1c–\x1e, \x85, \u2028 and \u2029, which _split_lines_keepends does not. One form feed and every subsequent number is off by one.

Reproduced end to end against a store doing exactly that:

document: 'header\x0cintro\nDEBUG = 1\nkeep me\nDEBUG = 2\n'

grep says 'keep me' is line 4
replace_lines(4) -> Replaced 1 line(s) in 'cfg.txt'.
file now: 'header\x0cintro\nDEBUG = 1\nkeep me\nREDACTED\n'

The agent asked to change keep me and destroyed DEBUG = 2. In range, no error, reported success. An out-of-range number raises; an off-by-one just edits the wrong line.

Worth adding: FileMemoryProvider has the same hole, and its tools are approval_mode="never_require" — so there the wrong-line edit lands with no host approval at all.

Proposed design

The regex will go down, the line numbers will come up from the base.

• split_lines() will publish the \n-only keepends rule every line_number addresses. Deliberately per-SDK: a line number never crosses runtimes, so it will only need intra-SDK coherence, and unifying it with .NET would be a behavioural break on one side or the other.
• scan_content() will be the numbering primitive. Both shipped stores will report through it, so the rule will cover them literally rather than by convention.
• find_matching_files() will be a new hook that lets a store narrow the search to files worth reading, with superset semantics: over-returning will be harmless because the base re-scans, under-returning will lose matches. Conservative predicate pushdown, so a dialect mismatch in a backend index will cost recall and nothing else. Its default implementation works, built from list_children + read — which is what lets the migration machinery go away: no gate, no DeprecationWarning, no planned @abstractmethod flip.
• search() will no longer be abstract. It will read and number candidates itself, re-apply the glob and the non-recursive rule, and skip files whose read() raises ValueError (non-UTF-8 and symlinked paths).

store that	will get
implements nothing extra	default path — aligned, unoptimised
overrides find_matching_files	native index narrows, base numbers — guaranteed aligned
overrides search()	full control, must follow split_lines — verified at grep

Overriding search() will stay first-class; a backend that can do the whole job natively should. It will own numbering then, so file_access_grep and file_memory_grep will re-match each reported line against the line the editor would touch and refuse the whole call on a mismatch. They will compare by pattern, not by string — a custom store is not bound to report the line verbatim, and string equality would reject correct stores. The check will run in a worker thread under the existing search timeout, so re-running a model-supplied pattern cannot stall the loop.

Two opt-outs will exist: a store will set reports_aligned_line_numbers to declare its numbers are split_lines coordinates, or a provider will take disable_search_alignment_check. The store flag will be the narrower one and the one to prefer. Both will be promises rather than hints, and a test will pin that hazard on purpose.

Separately, _LineEdit will gain an optional expected_line — the edit will be refused unless the target line still says what the caller saw. That will catch splitter drift, stale numbers, and concurrent modification.

Verification

Verification is the guard that stops a store's grep line numbers being trusted when they might not match the editor's.

The problem it solves: file_access_grep gets line_number: 5 from the store. file_access_replace_lines gets its line 5 by re-counting read() itself. Two different counters. If a store counts differently — say it uses str.splitlines(), which breaks on \f — grep says line 4 while the editor sees line 3, and the edit lands on the wrong line. In range, no error, reported success.

What it does: before handing grep's results to the model, the provider re-reads each matched file and re-runs the pattern against the line the editor would touch. Disagreement → the whole grep call is refused with a message the model can act on.

Three details that matter:

• It compares by pattern, not by string. A custom store isn't required to report the line verbatim with its terminator — .NET's own store trims \r — so comparing text would reject correct stores.
• It runs in a worker thread under the existing search timeout, because re-running a model-supplied regex on the event loop would reintroduce the ReDoS hole.
• It's detection, not proof. A pattern matching every line (.) verifies vacuously even if the numbering is skewed.

When it runs: only for a store that overrides search() and isn't trusted. The shipped stores, the base path, and pruning stores skip it entirely — their numbering is correct by construction.

What it costs: exactly one extra read per matched file. In the benchmark, 12 → 15 reads: +79.5 ms on Azure, +1.57 ms on Redis, zero everywhere else. It scales with hits, never with corpus size.

How to switch it off: reports_aligned_line_numbers on the store (narrower, preferred — the store promises its numbers are split_lines coordinates), or disable_search_alignment_check on the provider (turns it off for every store that provider is given).

The separate, complementary guard is expected_line on _LineEdit — that one sits at the write boundary and catches the same failure plus stale numbers and concurrent edits, but it's opt-in per call.

Measurements

Prototyped and measured locally; nothing pushed. Four stores, 5 rounds after 2 discarded warm-ups, containers pre-warmed. Two of them (Azure Blob, Redis) are fixtures written for this — MAF ships no remote file store. CI has never run on this PR, so none of it is corroborated by the pipeline.

Per-method: no effect on any backend, in either mode. Store methods do not touch verification. Sample avg ms, current / verif ON / verif OFF: Redis read 0.66 / 0.56 / 0.59; Azure read 40.09 / 39.72 / 38.55; local shipped search 2.40 / 2.78 / 2.31. Untouched methods drift both directions run to run, which brackets the noise.

file_access_grep end to end — min/avg/max ms, and reads = the number of store.read() calls one grep makes:

store	current	verification ON	verification OFF
local disk — shipped store	9.58/10.31/10.84 reads=0	12.79/13.33/13.88 reads=0	10.77/11.07/11.57 reads=0
local disk — base path store	n/a	16.94/17.97/18.73 reads=12	15.24/15.89/16.78 reads=12
azure blob	550.15/612.53/650.25 reads=12	617.24/680.20/786.18 reads=15	572.19/578.05/588.56 reads=12
redis	10.06/11.12/11.65 reads=12	10.69/11.01/11.34 reads=15	9.56/9.84/10.23 reads=12

The read counts are the mechanism: 12 → 15 is verification re-reading each matched file, and switching it off returns them to 12. So the cost is exactly one extra read per match — +102 ms Azure, +1.17 ms Redis against the same store with the check off — scaling with hits, never with corpus size, and zero for the shipped store and the base path, which are trusted by identity and never checked.

(The shipped store shows reads=0 because _search_files_sync reads bytes directly rather than through read(), so the counter does not see them. It still does the I/O; what the zero shows is that no verification reads are added.)

The base-path store cannot exist today — search is abstract — so it has no "current" column. Its search averages 8.33 ms against the shipped store's 2.78 ms (~3x): one read() per file versus one batched scan. That is why the shipped stores would keep their own search() rather than migrate.

Does the hook cost anything? This is the comparison that matters, and it is the one I would push back on if I were reviewing. Narrowing a search with an index is already possible — search() is abstract, so a store can consult its index there today. So: same index, same files read, same corpus; the only difference is whether the narrowing lives in the store's own search() or in find_matching_files.

store	selectivity	current (own search)	proposed (hook)	delta
local disk	5%	1.34	1.26	−6.0%
local disk	25%	3.62	3.49	−3.6%
local disk	50%	6.77	6.67	−1.5%
local disk	100%	13.46	13.12	−2.5%
azure blob	5%	156.04	158.85	+1.8%
azure blob	25%	233.16	231.24	−0.8%
azure blob	50%	354.04	332.32	−6.1%
azure blob	100%	603.62	549.88	−8.9%
redis	5%	1.46	1.23	−15.8%
redis	25%	2.78	2.54	−8.6%
redis	50%	5.39	4.76	−11.7%
redis	100%	9.03	8.27	−8.4%

At or slightly below current across all twelve points. The hook does not make search faster, and it does not cost anything either — what it changes is that narrowing no longer requires the store to hand-roll line numbering, which is the thing that goes wrong. Same speed, now safe.

For completeness on the other axis: an index is worth roughly 1/selectivity — 6-19x at 5% matching, converging to 1.0x and below when everything matches, since a predicate that excludes nothing still costs the lookup. That is true today and this PR neither adds nor removes it.

Decision

1. Push this into Python: [BREAKING] Issue 7571 file access read lines #7669, or split it into a follow-up? It would widen the PR.
2. .NET: [BREAKING] Issue 7571 file access read lines #7671 would need the same treatment to keep the SDKs in step. Happy to mirror it.

(Disclaimer: The reply text is prepared by AI, but fully reviewed by me personally)

[Anton Sokolovskyi (antsok)]

#7671 (comment) the .NET part proposed in parallel