Repository navigation
Conversation
Edit paths parse the same bytes more than once (the pre-write gate, the indexer's extraction, syntax health), and nothing measures how many parses a request actually costs. Count every source ParseFile hands to tree-sitter and expose the total as ParseCount, so a test can pin the parse budget of a code path by comparing two readings.
The pre-write parse gate parsed the candidate and then always parsed the file's prior content to compare error counts, so every clean edit paid for two parses on the ack path. A candidate with no parse errors cannot be a regression whatever the prior bytes held, so the gate now parses them only when the candidate has errors to compare against: a clean edit costs one parse and a broken one still costs two. One response changes: an edit that fixes a broken file no longer carries a parse_gate block, since the prior error count is no longer computed for it. parse_gate still reports every edit whose candidate has errors. Refs #1012
Readers and editors that hold a file's bytes need to know whether the spans of the file's stored nodes describe those bytes, or whether an edit has moved them since the last index. The watcher already answers that question for its own replay check, but only after re-reading the file itself, so the bytes it vouches for are not the caller's. IndexedContentMatches compares the caller's bytes with the persisted content identity (size and BLAKE3 over the transformed bytes) and answers false whenever the backend cannot tell. Refs #1014
A symbol's stored line range describes the bytes the file held when it was last indexed. Between an edit and the reindex that follows it, a reader that slices that range out of the current file serves the wrong text, and an editor that matches a fragment inside it can land in a neighbouring symbol. nodeLinesIn trusts the stored range only while the index records exactly the caller's bytes and its graph still holds that range: a reindex writes the nodes before the content identity, and the caller may have read the node before it committed. Otherwise it parses the bytes once and locates the symbol again by its order among the declarations whose ID shares its base. An ID with no position in it is unique, so that is an exact match. A redeclared init's _L<line>, an object member's @<line> and a local binding's @+<offset> are assigned by position, so they pair only while the number of such declarations holds; errSymbolAmbiguous refuses the rest rather than serve a neighbour, and errSymbolRemoved reports a symbol that is gone. readNodeSource reads a file once and resolves the range against what it read, keeping editor overlays on the overlay view's own range. Refs #1014
In single-repo mode with a watcher running, get_symbol_source trusts the stored line range of a file the watcher has not reindexed yet, so after an edit that shifts lines it cuts the old range out of the new bytes and serves the wrong text. The test edits the fixture above the symbol without reindexing and asks for the symbol's current body and range. It fails until the handler reads through readNodeSource. Refs #1014
edit_symbol, the plain batch and the transactional batch each carry a copy of the same matching code: find old_source in the symbol's lines, widen the region over the doc comment above, then turn line offsets into byte offsets. The batch copies also fall back to a whole-file match when the range looks stale, which is how a fragment repeated in two functions can be applied to the wrong one. locateSymbolEdit is that matching once, over a range that nodeLinesIn resolved against the same bytes. It requires the fragment to occur exactly once in the region it matches and has no whole-file fallback, because a resolved range is never stale. Refs #1012
Some writers change source files without the mutation path lock or a commit record (move and inline, overlay merge, safe-delete propagate, the transactional batch), so a concurrent edit to the same file can be lost. Before they are routed through one write choke point, list them. The census walks the package's non-test files and reports every reference, called or passed as a value, to a file writer outside the choke point and a short list of writers of non-source outputs. It only reports for now and becomes a gate once every writer is routed. Refs #1012
Every source-mutating request announces store write intent for its whole duration, although a primary-checkout edit in watcher mode writes only the source file and the watcher's patch announces its own store writes. While the intent is held, WAL reclaim stands down. The test spies on the store across an edit_file in watcher mode and fails while the request arms write intent; it passes once intent is armed only on paths that write the store. Refs #1012
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft, partial. Red by design:
TestPrimaryEditAnnouncesNoWriteIntentfails until write intent is armed only on paths that write the store.TestSymbolSourceSingleRepoWatcherLineShift, carried from mcp: resolve a symbol's range against the bytes being read #1225, fails untilget_symbol_sourceuses the new reader.CI on this PR is red.
Includes #1219 and #1225. The first five commits belong to those PRs: 78e02bd and d5bbf8f are #1219, and 7d466c6, 4bae951 and 69ccf0b are #1225. Both must land first. This PR's own commits are the last three (733ea76, 4d2bb42, f0a051f).
This is groundwork for one write choke point for source edits (#1012). None of the edit handlers change yet.
Changes
locateSymbolEdit(internal/mcp/symbol_span.go).edit_symbol, the plain batch and the transactional batch each carry a copy of the same matching code: findold_sourcein the symbol's lines, widen the region over the doc comment above, then turn line offsets into byte offsets. The batch copies also fall back to a whole-file match when the range looks stale, which is how a fragment repeated in two functions can be applied to the wrong one.locateSymbolEditdoes that matching once, over a rangenodeLinesIn(from mcp: resolve a symbol's range against the bytes being read #1225) resolved against the same bytes. The fragment must occur exactly once in the region, and there is no whole-file fallback, because a resolved range is never stale.Source-write census (
source_write_census_test.go, report mode). Some writers change source files without the mutation path lock or a commit record, so a concurrent edit to the same file can be lost. The census walks the package's non-test files and reports every reference, called or passed as a value, to a file writer outsideServer.commitFileMutation. A short list of writers of non-source outputs (tool outputs, batch journals, spill files) is exempt. Today it reports:runBatchTransaction→batchDurability().writeFileapplyRemoveLinePatches→agents.AtomicWriteFilewriteMoveFile→agents.AtomicWriteFilediskWriteForMerge→agents.AtomicWriteFileandos.RemoveIt becomes a failing gate once every writer is routed.
Write-intent spy test (
TestPrimaryEditAnnouncesNoWriteIntent). Every source-mutating request announces store write intent for its whole duration, and WAL reclaim stands down while the intent is held. A primary-checkout edit in watcher mode writes only the source file, and the watcher's patch announces its own store writes. The test spies on the store across anedit_filein watcher mode and fails while the request arms write intent.Status
edit_symbol, the batch and the transactional batch callingnodeLinesInandlocateSymbolEdit, and deleting the duplicated matching and the whole-file fallback;safe_deletespans throughnodeLinesIn.Tests
Before the rebase onto mcp: resolve a symbol's range against the bytes being read #1225's review fix, these passed under
-race:TestLocateSymbolEdit, 7 cases: first, second, doc comment above, CRLF region, repeated inside the symbol, outside the symbol, range past the end;TestSymbolEditFollowsLineShiftPastRepeatedFragment;TestSourceWriteChokePointCensus,TestPrimaryMutationFallbackKeepsOrdinaryWriteIntent;TestEditParsesOncePerCleanEdit,TestReadNodeSourceResolvesCurrentLines,TestIndexedContentMatches.go vetpassed, and golangci-lint 2.14.0 on./internal/mcp/...,./internal/indexer/...and./internal/parser/...reported 0 issues.Red by design:
TestPrimaryEditAnnouncesNoWriteIntentfails with "the edit request announced a store writer".Untested at this head. The branch was rebased onto mcp: resolve a symbol's range against the bytes being read #1225's reviewed resolver. The run at this head, with the
get_symbol_sourcecall-site patch applied, was queued but did not finish.The branch is based on 017db84 and merges cleanly with main at 7096db7.
Refs #1012