Document the codebase, and fix six things reading it turned up - #15
Open
siegfriedpammer wants to merge 8 commits into
Open
siegfriedpammer wants to merge 8 commits into
siegfriedpammer wants to merge 8 commits into
Conversation
Everything non-obvious about this tool lived in two places: the comments in the code, which are good but cannot be read in an order, and CLAUDE.md, which is an orientation rather than a reference. Someone picking up the semantic layer or the host layer cold had to reconstruct the shape of it by reading every file in it. Six documents under docs/, one per layer, written to be read cold and to explain why rather than what: the two diff representations and why they disagree on purpose, why a symbol is a file plus a position, why the base-side workspace is derived rather than loaded, why the merge queue pushes without --force, why folds and context gaps are kept apart. The build and test commands in them were run. Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9 Assisted-by: Claude:claude-opus-5:Claude Code
GetIdentifierSpansAsync is public, is not on ISemanticProvider, and has no
call site anywhere in src or tests: the clickable reference segments it was
written for are built from the classified tokens instead, and it duplicated
the filtering half of that. A public method nobody calls reads as a thing to
keep working.
The unused local in TestRunComparison goes the same way - it names a question
("was this test present at base?") the comparison decided not to ask, and
leaving it suggests the answer is used somewhere.
Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9
Assisted-by: Claude:claude-opus-5:Claude Code
Both sat one member too high, stacked above another summary: the quick-info line above GetDocumentTextAsync, and the one about walking up to a member above MemberAtPosition, which decides something else entirely. Each described the member below its neighbour, and each left that neighbour undocumented. Nothing reads them but a person reading the file, which is exactly who they mislead. Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9 Assisted-by: Claude:claude-opus-5:Claude Code
Each semantic provider carried its own copy of the mapping between the repo-relative paths git speaks and the absolute ones it reports, and the two had drifted apart. The Roslyn one requires a separator after the root, with a comment saying why; the language-server one, written later, does not - so /repo-other/x.py read as being inside /repo and came back as "-other/x.py", which names nothing. Every reference hit and navigation target in a sibling directory whose name starts with the worktree's went there. It also compared without regard to case on every platform, where only Windows does that, so two files of one tree differing only in case answered for each other. One helper now, with the reasoning on it and tests of its own, and both providers forward to it. A third provider gets the guards for free, which is the half of this that was actually costing something. Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9 Assisted-by: Claude:claude-opus-5:Claude Code
RefusedException and ToolFailedException mean the same thing to a caller: it did not happen, and the message says why. The class comment said so, and the type hierarchy did not - so a refusal walked past the catch clause written for the failure beside it and reached the reader as a crash dialog. Three places did that. Opening a pull request from a fork on Azure DevOps, which is refused by design, reported itself as an unhandled exception. The offline snapshot path skipped its fallback entirely, because the refusal never reached the catch that reads the cache. And the merge queue abandoned a turn with its lock still held, to be recovered only when the lease ran out five minutes later. Making it a ToolFailedException fixes all three at once, and the ninety-odd existing catch clauses become right by construction rather than by each author remembering. A caller that does want to tell the two apart still can, by catching the refusal first - which is what the rebase does, and the compiler now insists on that order, which is how the one place that had it backwards was found. Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9 Assisted-by: Claude:claude-opus-5:Claude Code
…aiting A request waited for as long as the server took, and the callers that ask are the ones with nothing to cancel them: go to definition, hover, references and the rest all pass CancellationToken.None from the UI thread. A server that stopped answering therefore took those commands down for as long as the review stayed open, silently, with no way back but restarting the application. Worse, when the server ended - crashed, killed, output closed - the read loop broke out and left every outstanding request waiting forever, because nothing completed them. The one case where the tool knows the answer is never coming was the case where it waited hardest. So: thirty seconds for a question about code, two minutes for the handshake (a server installed through npx downloads itself on first use, and the handshake is what waits for that), and no deadline at all for loadBase, whose work IS the waiting - it answers once the server's own solution has loaded, and giving up would abandon a base side that was going to arrive. Giving up answers with nothing rather than throwing, because that is what a server error already does here and what every caller is written for; the log carries the difference between "found nothing" and "never answered". The read loop now fails what it leaves behind, and gives each message's JsonDocument back to the pool instead of dropping one per message. Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9 Assisted-by: Claude:claude-opus-5:Claude Code
Each of these is a place where one caller was careful and its neighbour was not, so the careful one hid the bug until something reached the other. Splicing a comment thread into a unified document indexed the text by the tag count, while the side-by-side splice beside it allows for a model carrying one more tag than its text has lines. The guard belongs in both or neither. The worktree cache abbreviates a revision to nine characters. One of the three places that does it checks the length first; the other two slice blindly, and a scope carries whatever revision resolved it - including an abbreviated one. Generated sources diff a side the other does not have against /dev/null, which on Windows is a path that is not there, so a generator whose output a change adds or removes showed nothing at all. Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9 Assisted-by: Claude:claude-opus-5:Claude Code
SymbolFinder is asked in terms of a Workspace, and the two call sites asserted there was one rather than checking. A view derived for the base side takes the head's workspace, and a head whose load failed part-way can leave a solution behind without one - which is exactly the state where a reader starts pressing F12 to find out what is wrong. Answering nothing is what both callers already do for a file the workspace does not hold, so there is nothing new to handle; the alternative was taking the pane down. Claude-Session: https://claude.ai/code/session_01EgJ3xXgcismuaFmKhF6KD9 Assisted-by: Claude:claude-opus-5:Claude Code
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
1 participant
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.
Six documents under
docs/, one per layer, and the correctness fixes thatcame out of writing them.
The documentation
docs/README.mdindexes the rest. Written to be read cold, and to explainwhy rather than what - the code's own comments are good, but they cannot
be read in an order.
architecture.mdreview-session.mdReviewWorkspace/ReviewScopes/ReviewComments/ startupgit-and-diff.mdpull-request-hosts.mdIPullRequestHostand its two implementations,PrCache,CommentAnchor,ReviewStateStore, the merge queuesemantics.mdISemanticProvider, the Roslyn workspace, the LSP client, server discovery and bootstrap, decompilation, the test parsersui.mdEvery build and test command quoted in them was run.
The fixes
One commit each, all reviewable on their own.
A path outside the tree answered as though it were inside it. Each
semantic provider carried its own copy of the repo-relative/absolute mapping
and the two had drifted: the language-server one has no separator check, so
/repo-other/x.pyread as being inside/repoand came back as-other/x.py. Every reference hit and navigation target in a siblingdirectory whose name starts with the worktree's went there. It also compared
case-insensitively on every platform, where only Windows does. One tested
helper now; both providers forward to it.
A language-server request had no deadline. The callers that ask are the
ones with nothing to cancel them - go to definition, hover, references all
pass
CancellationToken.Nonefrom the UI thread - so a server that stoppedanswering took those commands down for as long as the review stayed open,
silently. And when the server ended, the read loop broke out and left every
outstanding request waiting forever, because nothing completed them. Thirty
seconds for a question about code, two minutes for the handshake (npx
downloads the server on first use), and no deadline for
loadBase, whose workis the waiting.
A refusal was not answered where a failure was.
RefusedExceptionandToolFailedExceptionmean the same thing to a caller, and the comment said sowhile the hierarchy did not. Opening a fork PR on Azure DevOps reported itself
as a crash dialog; the offline snapshot path skipped its fallback; the merge
queue abandoned a turn with its lock still held. Making it a
ToolFailedExceptionfixes all three, and the ninety-odd existing catchclauses become right by construction.
Three sharp edges where one caller was careful and its neighbour was not:
the unified comment-thread splice indexed past its text where the
side-by-side one guards, two of the three short-SHA truncations in the
worktree cache slice blindly, and generated sources diff against
/dev/nullon Windows - so a generator whose output a change adds or removes showed
nothing there.
A symbol was resolved against an asserted workspace. A base-side view
takes the head's, and a head whose load failed part-way can leave a solution
without one - which is exactly when a reader starts pressing F12.
Plus one dead public method with no call site anywhere, one dead local, and
two doc comments that sat a member too high.
Not in here
The structural work the same read turned up -
ReviewWorkspaceat 2802 linesand nine responsibilities,
DiffDocumentViewat 1270,GitServiceat 1040,and the duplication between the unified and side-by-side views - is a separate
proposal, sequenced so each step ships on its own. Putting it in this PR would
have made it unreviewable.
Testing
dotnet buildclean, 0 warnings. 348 tests, 346 passing, 2 skipped (thebasedpyright bootstrap, and one Windows-only path case) in ~18s. Six new test
cases cover the path helper and the splice guard.
🤖 Generated with Claude Code