| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Adds line-range reading and aligns .NET grep results with line-editing semantics.
Changes:
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| InMemoryAgentFileStoreTests.cs | Tests updated grep semantics. |
| FileEditorTests.cs | Tests splitting and slicing. |
| FileAccessProviderTests.cs | Tests the new tool and approvals. |
| HarnessAgentTests.cs | Verifies tool exposure. |
| InMemoryAgentFileStore.cs | Aligns grep line handling. |
| FileSystemAgentFileStore.cs | Aligns filesystem grep behavior. |
| FileSearchMatch.cs | Documents verbatim lines. |
| FileEditor.cs | Adds shared splitting and slicing. |
| FileAccessProviderOptions.cs | Documents read-only tool behavior. |
| FileAccessProvider.cs | Implements file_access_read_lines. |
| Harness_Step03_DataProcessing/README.md | Updates security guidance. |
| Claw_Step02_WorkingWithData/README.md | Updates security guidance. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
…as the schema does Addresses both review comments on microsoft#7671. TrimTrailingNewline removed only "\n", so grep matched against text such as "match\r" on CRLF and lone-CR lines and an end-anchored pattern like "match$" failed even though the line's text was exactly "match". Renamed to TrimLineTerminator and it now strips "\r\n", "\n", or a lone "\r". The file_access_read_lines description and the SliceLines failure messages referred to end_line/start_line, but the generated schema exposes the arguments as endLine/startLine, so the model could be prompted to emit an invalid argument name. Both now use the schema's names. (new_line is left as-is: FileLineEdit sets it explicitly via JsonPropertyName.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s_grep Ports the fix for the same defect found by review on the .NET side (microsoft#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) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)dotnet/src/Microsoft.Agents.AI/Harness/FileStore/FileSystemAgentFileStore.cs:209
// Lines keep their terminators, so these line numbers address the same lines that
// replace_lines edits and each reported line can be reused as a literal new_line.
List<string> lines = FileEditor.SplitLinesKeepEnds(fileContent);
dotnet/src/Microsoft.Agents.AI/Harness/FileAccess/FileAccessProvider.cs:326
[Description("Read part of a file by 1-based inclusive line number; omit endLine to read to the end of the file, and an endLine 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.")]
private async Task<string> ReadLinesAsync(string fileName, int startLine, int? endLine = null, CancellationToken cancellationToken = default)
Sorry, something went wrong.
…s_grep Ports the fix for the same defect found by review on the .NET side (microsoft#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) <noreply@anthropic.com>
file_access_grep runs through AgentFileStore.search, whose contract says nothing about how content is split or whether terminators survive, while read_lines and replace_lines split through the module-private _split_lines_keepends. A custom store can therefore report a line number that addresses a different line than the two editing tools do — the wrong-line edit this branch exists to prevent, moved to custom stores. The claim was written as unconditional in four places, so _split_lines_keepends, _slice_lines, FileSearchMatch.line and AGENTS.md now say where it holds and where it does not. AGENTS.md also still described matching as stripping only the trailing "\n" and anchoring "as before", which stopped being true in 7aa29c6. Corrected to the whole terminator, in the same wording as the PR description. The read_lines tool docstring is left unhedged on purpose: it is prompt text, and teaching the model to doubt the line numbers would send it back to whole-file reads, which is the cost this branch exists to remove. Found while reviewing the .NET port (microsoft#7671), where Copilot raised the same gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
Motivation & Context
Two defects, one of which only became visible while fixing the other.
The reading gap (#7571). 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 match without reading the whole file, so agents either re-read entire files or edit by a line number they never looked at.
Grep and the editor did not agree on what a line is. The stores split on '\n' and stripped '\r'; FileEditor split on '\n', '\r\n' and a lone '\r' and kept terminators. That is not a cosmetic difference:
The same hole, one level down — and this is new scope since the first version of this PR. Unifying the splitter fixes the two stores in this package, but AgentFileStore.SearchAsync was abstract with no numbering contract at all, while read_lines and replace_lines re-count from ReadAsync. So a custom store could make "line 5" mean two different things, and replace_lines would edit the wrong line — in range, reporting success. FileMemoryProvider had the identical hole.
The previous revision of this PR documented that as a caveat and called it "a maintainer call". @moonbox3 made that call on the Python PR #7669: the rule should live on the contract, not in a doc comment. This PR now mirrors #7669 so both SDKs carry the same contract.
Description & Review Guide
What are the major changes?
The reading gap:
The contract, now on the base class:
4. SplitLines() publishes the split every line_number addresses. Per-SDK by design: it need not match Python, only be consistent here, because a line number never crosses runtimes.
5. ScanContent() is the numbering primitive. Both shipped stores report through it, which also removes the scan loop that was duplicated between them.
6. FindMatchingFilesAsync() is a new hook for narrowing to the files worth reading. The regex goes down as a hint with superset semantics — over-returning is harmless because the base re-scans, under-returning loses matches. A backend with a native index overrides it and narrows server-side. Its default is built from ListChildrenAsync, so a store implementing nothing beyond the mandatory members gets aligned numbers for free.
7. SearchAsync is no longer abstract. It reads and numbers candidates itself, and re-applies the glob and the non-recursive rule, since the hook may over-return.
8. Overriding SearchAsync stays first-class — a backend that can do the whole job natively should — but then it owns numbering, and nothing checks it at runtime. SearchAsync documents what is owed: LineNumber is a 1-based coordinate into SplitLines of the content ReadAsync returns, ScanContent produces that correctly, and getting it wrong fails silently — the search looks right and a later line edit lands on a line the caller never saw. See focus item 3.
9. FileLineEdit.ExpectedLine — when supplied, the edit is refused unless the target line still says what the caller saw. Catches splitter drift, a stale line number, and the file changing between read and write.
Added during review, after the items above:
What is the impact of these changes?
Breaking, in these ways:
The whole surface is [Experimental("MAAI001")]. ApiCompat does not flag any of it — FileEditor is internal and Line keeps its type — so a passing Release build is not evidence of compatibility. Removing abstract while keeping the member virtual does pass Package Validation; verified by building the package in Release with IsReleased=true.
Two further changes are not API breaks but do change what the model is told: every line-addressing tool description gains the counting rule, and the expected_line mismatch message no longer echoes the line it found — raised in review as a read oracle where write tools are auto-approved while read tools are not, and applied in 30115b0.
Cost. None: no tool re-reads a file, and SearchAsync returns its results directly.
The narrowing hook is not a speed-up and is not sold as one — it is "same speed, now safe". A selectivity sweep against a store doing the whole job in its own SearchAsync:
Per-method timings across local disk, in-memory, Azure Blob and Redis show no method-level effect; happy to attach the full table if useful.
file_access_read_lines joins the read-only tool set, so it is exposed under DisableWriteTools and covered by ReadOnlyToolsAutoApprovalRule — the auto-approval docs and sample security notes are updated accordingly.
What do you want reviewers to focus on?
Whether the contract belongs on AgentFileStore at all — that is the substantive question, and it is a maintainer call being made here rather than assumed.
The superset semantics of FindMatchingFilesAsync. Over-returning is harmless, under-returning silently loses matches. Whether that is documented clearly enough for someone implementing it against a native index.
Whether documentation alone is enough to carry the numbering contract. Earlier revisions tracked "did the base number these?" by tagging the returned list and had the providers verify it; that is removed per change 11 above. What replaces it is the <remarks> on SearchAsync, plus SplitLines and ScanContent being the published primitives an implementer is meant to build on. The open question is whether an implementer overriding SearchAsync will actually read it there, given the failure mode is silent and shows up in someone else's edit.
This is where the two PRs now diverge. Python: [BREAKING] Add file_access_read_lines and move the line-numbering contract onto AgentFileStore #7669 (Python) still verifies at runtime and was hardened in that direction on 25 Aug. That divergence is not deliberate design — it is one maintainer preference applied to one PR — so it is worth a view on whether Python should follow.
The per-line snippet offset arithmetic in both stores now that terminators are part of each line. FileSystemAgentFileStoreTests previously asserted no Line values at all, so the six search tests mirrored into it are where that arithmetic is now pinned for the disk-backed store.
Two deliberate divergences from the Python half (Python: [BREAKING] Add file_access_read_lines and move the line-numbering contract onto AgentFileStore #7669):
Line-rule parity note: Python addresses a trailing empty line on "a\nb\n"; .NET has two lines there, because .NET's line editor never had that phantom line. Each language stays self-consistent, which is what the grep → read → edit round trip actually depends on.
Related Issue
#7571 — linked without a closing keyword on purpose: the Python half ships as #7669, and the issue should stay open until both land. Will change to Closes in the last one.
Note that #7571's body still says no store-protocol change is needed; that predates the review discussion above and is no longer accurate for either language.
Contribution Checklist