From 73cd4beec73410b8fb4b5a50e8a67df45d652cc8 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:19:30 +0200 Subject: [PATCH 1/8] Write down how the codebase works 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 --- docs/README.md | 40 ++ docs/architecture.md | 213 +++++++++ docs/git-and-diff.md | 808 +++++++++++++++++++++++++++++++++ docs/pull-request-hosts.md | 624 ++++++++++++++++++++++++++ docs/review-session.md | 243 ++++++++++ docs/semantics.md | 896 +++++++++++++++++++++++++++++++++++++ docs/ui.md | 730 ++++++++++++++++++++++++++++++ 7 files changed, 3554 insertions(+) create mode 100644 docs/README.md create mode 100644 docs/architecture.md create mode 100644 docs/git-and-diff.md create mode 100644 docs/pull-request-hosts.md create mode 100644 docs/review-session.md create mode 100644 docs/semantics.md create mode 100644 docs/ui.md diff --git a/docs/README.md b/docs/README.md new file mode 100644 index 0000000..d8c6030 --- /dev/null +++ b/docs/README.md @@ -0,0 +1,40 @@ +# Stampeded! developer documentation + +A maintenance manual for the codebase: what each layer does, what its public surface is, and - where +it matters - *why* the code is shaped the way it is. Written to be read cold. + +Start with [architecture.md](architecture.md). The rest can be read in any order. + +| Document | Covers | +| --- | --- | +| [architecture.md](architecture.md) | the four projects, the layering, the five decisions that shape everything, where state lives on disk, environment switches, build and test | +| [review-session.md](review-session.md) | `ReviewWorkspace`, `ReviewScopes`, `ReviewComments`, `MainViewModel`, startup - how a PR number becomes a window full of documents | +| [git-and-diff.md](git-and-diff.md) | `Stampeded.Core/{Git,Diff,Infra}` - the git plumbing, the two diff representations, folding and context gaps, the process runner and the log | +| [pull-request-hosts.md](pull-request-hosts.md) | `IPullRequestHost` and its two implementations, the data model, `PrCache`, `CommentAnchor`, `ReviewStateStore`, the merge queue | +| [semantics.md](semantics.md) | `ISemanticProvider`, the Roslyn workspace, the LSP client, language-server discovery and bootstrap, the Python interpreter, decompilation, the test runner and its parsers | +| [ui.md](ui.md) | the Avalonia layer: docking, documents, panes, the AvaloniaEdit extension points, syntax painting, comment threads, the keyboard model, the screenshot harness | + +`CLAUDE.md` in the repository root is the short orientation version of the same material. + +## The shortest possible tour + +A review is opened (`ReviewWorkspace.OpenPrAsync`), which fetches the PR head, computes the merge +base, and asks git for the diff. That diff - `IReadOnlyList` - is the review. Opening a +file reads both blobs out of the object database through one long-lived `git cat-file --batch`, and +re-diffs them in process into a `DiffDocumentModel` whose every line is a verbatim blob line. That +invariant is what lets a semantic provider's `(line, column)` answer be carried straight onto a +document row. + +In the background, a Roslyn workspace loads over a detached worktree of the head, and a second one is +*derived* from it with the review's files reading as they did at the base - so removed code is +navigable without a second checkout. A language server is started per other language the review +actually touches. + +The reader walks the file list, marking files viewed. That, and any draft comments, go into a JSON +file keyed by the repository and the PR number, stamped with the head they were read at. When the +author pushes, the next open notices the head moved, carries over the viewed flags for files the push +did not touch, and can show the diff *since that pass alone* - by replaying the work already read +onto the current base as a tree, which is the only thing that survives a rebase. + +Submitting the review batches the drafts that still sit on commentable lines into one host call, and +keeps the rest local. diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..69d9846 --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,213 @@ +# Architecture + +Stampeded! is a keyboard-driven desktop code-review tool. A pull request or a local branch is read +as a diff with real semantic navigation (go to definition, find references, hover docs), blame, CI +state, test results and coverage, in one Avalonia window. + +## The four projects + +| Project | What it is | May reference | +| --- | --- | --- | +| `src/Stampeded.Core/` | everything that does not need a UI: git and host access, diff and fold building, Roslyn hosting, the LSP client, the review store | no Avalonia - **keep it that way** | +| `src/Stampeded/` | the Avalonia app: panes, documents, controls, view models | Core | +| `src/Stampeded.RoslynLsp/` | Roslyn as a language server, for reading C# out of process | Core | +| `tests/Stampeded.Core.Tests/` | NUnit, covering `Stampeded.Core` only. **The UI layer has no automated tests** | Core | + +`Stampeded.slnx` builds all four. + +## The layering + +``` + MainWindow / MainViewModel + | + ReviewWorkspace <- the session hub (App.Workspace) + / | | \ + ReviewScopes ReviewComments Documents/ Panes/ + \ / | + \ / Editor/, Diff/, Controls/ + \ / | + ------------------------------------------------------------------ Stampeded.Core + | | | | + Git/ + Diff/ PullRequests/ Semantics/ Review/ + ExternalTool GitHub/ AzureDevOps/ Roslyn/ Lsp/ MergeQueue/ + Decompilation/ Testing/ + | + Infra/ (ExternalTool, CliLog, CachePath, ...) +``` + +Four interfaces carry all the polymorphism in the codebase, and there are exactly four: + +- **`IPullRequestHost`** - GitHub over `gh`, Azure DevOps over `az`. See + [pull-request-hosts.md](pull-request-hosts.md). +- **`ISemanticProvider`** - Roslyn in process, or a language server over stdio. See + [semantics.md](semantics.md). +- **`IDecompileTargets`** - a capability a provider *may* also have, deliberately not part of + `ISemanticProvider`, because only a provider with real metadata behind it can answer. +- **`IDiffDocument` / `IReviewDocumentView`** - the unified and the side-by-side diff layouts. See + [ui.md](ui.md). + +## Five decisions that shape everything + +### 1. Everything external is a CLI + +`git`, `gh`, `az`, `dotnet`, `code` and `xdg-open` are the only ways out of the process, all through +`ExternalTool.RunAsync` - which logs the command, and on failure the first line of its output, since +an exit code alone never says what went wrong. **There are no API tokens of the tool's own**: auth, +SSO and token refresh ride on the user's `gh auth` and `az login`. Do not add an HTTP client for any +host. + +A language server is the one exception, because it is not a command with an exit code: it starts +once and answers until the review closes, over JSON-RPC on its stdin and stdout. Everything it does +still reaches the log. + +`CliLog.Write` is the sink the Log pane shows. Anything a user might have to explain to someone else +belongs in it. + +### 2. GitHub's words are the model's words + +`APPROVE` / `REQUEST_CHANGES` / `COMMENT`, `APPROVED` / `CHANGES_REQUESTED`, `LEFT` / `RIGHT`, +`MERGEABLE` / `BLOCKED`: the panes and the pure functions under them already speak them, and Azure +DevOps - which counts votes from 10 to -10 and has no review object at all - maps onto them inside +its own implementation and nowhere else. Nothing above `IPullRequestHost` knows which host answered; +what a pane shows the reader comes from `HostName`. + +### 3. A symbol is a file plus a position + +`SymbolRef` carries `(RelPath, Line, Column, Display, Name, IsType, ContainingType?)` - never a +compiler object, because that is all a language server can be handed back. The consequence: +**the position stored in a `SymbolRef` has to re-resolve to the same symbol.** + +### 4. Reads never touch the user's working tree + +`GitService` reads come from the object database, or from a checkout's files for a review of +uncommitted work. Writes that need a checkout use a throwaway worktree. Review worktrees are +**detached**, under `~/.cache/stampeded/worktrees`, so they never hold the branch being reviewed. + +**A branch lives in one checkout at a time.** Anything that moves a branch ref has to ask +`ListWorktreesAsync` whether some checkout has it: if one does, the operation runs *there*, so its +working tree and index move with the ref. + +### 5. There are two diffs, and they disagree on purpose + +`FileDiff` is git's opinion, parsed from `git diff -U3`, and it decides where a comment may be +anchored - because the host computes its anchors from the same diff. `DiffDocumentModel` is built +from the whole blobs and re-diffed in process, and it is what the reader looks at, so they can +scroll out of a hunk into untouched code. See [git-and-diff.md](git-and-diff.md). + +## Where state lives on disk + +| Path | Written by | Contents | +| --- | --- | --- | +| `~/.cache/stampeded/worktrees//` | `WorktreeManager` | detached review checkouts, LRU-capped at 6 | +| `~/.cache/stampeded/prs/_pr.json` | `PrCache` | the snapshot that lets a review open offline | +| `~/.cache/stampeded/python-lsp/` | `LanguageServers` | a venv with basedpyright, installed on demand | +| `$LOCALAPPDATA/stampeded/reviews/*.json` | `ReviewStateStore` | viewed flags, drafts, pass heads | +| `$LOCALAPPDATA/stampeded/*.txt` | `UserData` | zoom, window placement, recent repos, preferences | +| `refs/stampeded/review//head` | `GitService.PinReviewHeadsAsync` | pins the head a pass was read at, so a force-push cannot prune it | +| `refs/stampeded/pr/` | `GitService.FetchPrHeadAsync` | the fetched PR head | +| `refs/stampeded/merge-queue` (on origin) | `MergeQueueService` | the shared merge queue | + +Deleting any of the cache directories costs a reader nothing but time. + +## Environment switches + +All optional. + +| Variable | Effect | +| --- | --- | +| `STAMPEDED_PR_HOST=github\|azdo` | override the host decided from origin's URL | +| `STAMPEDED_SEMANTICS=lsp` | read C# through `Stampeded.RoslynLsp` instead of in process | +| `STAMPEDED_PYTHON_LSP` / `STAMPEDED_CSHARP_LSP` | a server command line to use instead of the search | +| `STAMPEDED_PYTHON_PATH` | the interpreter, for an environment none of the usual places would find | +| `STAMPEDED_LSP_TRACE=1` | log every request with what came back, and ask the server for trace-level logging. **This is the thing to turn on when a review reads a language on one machine and not on another.** | +| `OPENSSL_ENABLE_SHA1_SIGNATURES=1` | required by the local OpenSSL setup; set process-wide by `Program.Main`, and needed on the command line for `dotnet` | + +## Tech stack + +- **Avalonia 12** with the **Simple** theme (not Fluent), **AvaloniaEdit** for the diff views, + **Dock** for the pane layout, **Markdown.Avalonia** for rendered descriptions. +- **CommunityToolkit.Mvvm** (`[ObservableProperty]`) for view models; `Dock.Model.Mvvm` `Tool` / + `Document` for panes and documents. +- **Roslyn** for source semantics: two workspaces per review, head and merge base, so removed code + stays navigable. +- **TextMateSharp** for syntax colours: VS Code's grammars and themes. The editor's own `.xshd` + definitions answer for what the bundle does not carry, which is ILAsm. +- **CliWrap** for every external process. **DiffLib** for the in-process alignment. + **ICSharpCode.Decompiler** for sourceless definitions. +- Target framework `net10.0`. Nullable enabled, implicit usings, `TreatWarningsAsErrors`, central + package management (a new `PackageReference` needs a `PackageVersion` in + `Directory.Packages.props`), and `AvaloniaUseCompiledBindingsByDefault` - so a typo in a binding + path is a build error, not a silent blank. + +`Directory.Build.props` also carries two non-obvious lines: + +- `` + - MSBuildLocator loads the MSBuild assemblies out of the installed SDK, so the copies Roslyn's + MSBuild workspace drags in transitively must not land next to ours: two + `Microsoft.Build.Framework` identities in one process is exactly the load failure the locator + exists to avoid (MSBL001). +- `` - the semantic vocabulary (tokens, symbols, hits) is + named unqualified everywhere, the way the framework's own types are. + +## Build and test + +Prefix `dotnet` with `OPENSSL_ENABLE_SHA1_SIGNATURES=1`: + +``` +OPENSSL_ENABLE_SHA1_SIGNATURES=1 dotnet build Stampeded.slnx +OPENSSL_ENABLE_SHA1_SIGNATURES=1 dotnet test --solution Stampeded.slnx --report-trx --results-directory test-results +``` + +`dotnet test` runs through Microsoft.Testing.Platform (`global.json` pins the runner), so the +solution is named with `--solution` - the bare positional form is VSTest syntax and errors. +`--report-trx` leaves a TRX per test assembly under `test-results/`, which is how a failure survives +the run. + +As of this writing the suite is **342 tests, 341 passing, 1 skipped** (`PythonServerInstallTests`, +which installs a real language server into a temporary cache) in about 18 seconds. + +Tests that exercise git create real repositories in temp directories and shell out to `git` - that is +deliberate: the interesting behaviour is git's, and a mock would only assert what we already believe. +See `GitRebaseTests` / `GitPushTests` for the fixture shape, including how to script `merge.tool` so +a conflicted rebase runs without anything interactive. + +CI (`.github/workflows/build.yml`) builds and tests Release on windows-latest, ubuntu-latest and +macos-latest, uploading the TRX files as artifacts. + +## Verifying UI changes + +The app screenshots itself when a trigger file appears, because Wayland blocks external capture of +its window: write the target PNG path into `/tmp/stampeded-screenshot-request`, optionally followed +by command lines. See the command table in [ui.md](ui.md). Only one instance can serve a request, so +shut down extra instances first. + +Two lessons that cost a session each: + +- **Position bugs need a driven click, not a screenshot of colours.** Highlighting looked fixed while + the clickable spans were still wrong. `press:` and `release:` take separate modifiers, which is + what tells a gesture read from the press apart from one read from the release. +- **Reproduce red before claiming a fix.** A crash that did not reproduce under the first hypothesis + needed a live selection to trigger; without that step the fix would have been a guess. + +## Code conventions + +- **Tabs**, en-US English, ASCII-only in code and comments. +- **No license headers on new files.** The vendored tree-view files keep their original ILSpy + headers; nothing else in the repository has one. +- **Comments must stand on their own.** They describe the code as it is, for someone reading the file + cold. Never reference "the change", "the previous version", "as requested", or anything else that + only means something inside the conversation that wrote it. A comment explains *why* the code is + the way it is; the code already says what it does. +- **Report what happened, do not swallow it.** A failed external command surfaces its reason; a + status line says which of the possible outcomes occurred, not just "done". +- Commit subject is a phrase describing the change, under ~72 characters, no area prefix. The body + explains *why* - the constraint, the decision, what was rejected - not what the diff already shows. + +## Vendored code + +`src/Stampeded.Core/TreeView/` and `src/Stampeded/Controls/TreeView/` are vendored from ILSpy. They +are meant to stay close to upstream so fixes can move both ways - read +`src/Stampeded.Core/TreeView/README.md` before changing them, and prefer fixing a bug upstream too +over diverging. `Navigation/NavigationHistory` and the XML half of `Infra/GuessFileType` come +from the same place. The diff-view concepts are inspired by +[Aehnlich](https://github.com/Dirkster99/Aehnlich) (MIT). diff --git a/docs/git-and-diff.md b/docs/git-and-diff.md new file mode 100644 index 0000000..dbe9602 --- /dev/null +++ b/docs/git-and-diff.md @@ -0,0 +1,808 @@ +# Git, diff and infrastructure + +`src/Stampeded.Core/{Git,Diff,Infra}` - the part of the tool that turns a repository into +something reviewable. No Avalonia anywhere in it. + +## The shape of the layer + +``` +Infra/ ExternalTool, CliLog, CachePath, GuessFileType, LogFileRefs, SolutionTarget + ^ ^ + | | +Git/ GitService -- GitDiffParser --+ + GitBlobReader | + GitLogParser, GitBlameParser | + WorktreeManager, BranchSync | + v +Diff/ DiffModel (FileDiff/DiffHunk/PatchLine) + DiffDocumentModel + DiffDocumentBuilder (uses DiffSlider, DiffLib) + PatchDocumentBuilder, ChangedLines, ContextGaps, DiffFolding +``` + +`Git/` depends on `Diff/` (for `FileDiff`) and on `Infra/`. `Diff/` depends on nothing but +`Semantics.MemberFoldRegion` (one type, in `DiffFolding`) and the external `DiffLib` package. + +### The one architectural fact that explains everything else + +**There are two entirely separate diff representations, produced by different machinery, and they +do not agree line for line on purpose.** + +1. **`FileDiff` / `DiffHunk` / `PatchLine`** - parsed out of `git diff -U3 --find-renames` by + `GitDiffParser`. This is *git's* opinion. It drives the file list, the change kinds, the rename + detection, and - critically - `ChangedLines`, which decides where a review comment may be + anchored. It has to be git's opinion, because GitHub and Azure DevOps compute their comment + anchors from the same diff; a comment offered on a line the host's diff never printed would fail + to post. + +2. **`DiffDocumentModel`** - built by `DiffDocumentBuilder.Build` from the *whole* old blob and the + *whole* new blob, re-diffed in process with DiffLib and post-processed by `DiffSlider`. This is + what the reader looks at. It contains every line of both files, so the reader can scroll out of a + hunk into untouched code, ask for a definition, blame, and fold members. Context hiding is layered + on top by `ContextGaps` rather than baked into the text. + +The blobs for (2) come from `GitBlobReader` (`ReviewWorkspace.cs:634, 777, 1286`), not a checkout. + +## Infra + +### ExternalTool + +The single door out of the process for every CLI (`git`, `gh`, `az`, `dotnet`, `code`, `xdg-open`). + +```csharp +public sealed class ToolFailedException(string tool, int exitCode, string stdErr) : Exception +public sealed class RefusedException(string message) : Exception + +public static Task RunAsync(string exe, IReadOnlyList args, string workingDir, + CancellationToken ct = default, IReadOnlyDictionary? env = null, + IReadOnlyList? okExitCodes = null); +public static string Explain(ToolFailedException failure); +public static string FailureReason(string stdErr, string stdOut); +``` + +`RunAsync` returns **stdout only**. A non-zero exit throws `ToolFailedException` carrying the whole +of stderr, unless the code is listed in `okExitCodes`. + +**The two exception types are not the same thing.** `ToolFailedException` means *a CLI said no* and +has an exit code and stderr. `RefusedException` means *this tool said no*, before or instead of +running anything - only two producers in this layer: `GitService.RemoveWorktreeAsync` (`:514`) and +`GitService.RebaseBranchAsync` (`:594`). Callers should treat them identically: "it did not happen, +and the message says why". + +Non-obvious behaviour: + +- **`Win32Exception` is converted, not propagated** (`:56`). A tool that is not installed throws it + from `Process.Start`, which no caller catches; it would escape and leave a pane spinning forever. + It becomes `ToolFailedException(exe, -1, "...not installed, or not on PATH.")`. **Exit code `-1` + is the sentinel for "never started".** +- **MSBuildLocator variables are stripped from *every* child**, git included (`:28`, `:49`). + `MSBuildLocator.RegisterDefaults()` pins `MSBUILD_EXE_PATH` / `MSBuildSDKsPath` / + `MSBuildExtensionsPath` process-wide to the SDK matching the *host* runtime. A child `dotnet` + whose `global.json` resolves a different SDK then gets foreign targets forced on it and fails + restore with no output at all. +- **Every invocation is logged** (`:65`): `argsText -> exit N (M ms)`, with the failure reason + appended when it failed. Args truncated at 160 characters. +- `FailureReason` takes the first non-blank line of stderr, falling back to stdout (not every tool + reports failure on stderr - `gh` in particular), capped at 200 chars. +- **Cancellation kills the process tree** - CliWrap's behaviour, relied on rather than implemented. + +### CliLog + +```csharp +public static Action? Sink { get; set; } +public static void Write(string category, string message); +``` + +Line format `HH:mm:ss.fff [category] message`. `Backlog = 2000` lines are kept, and setting `Sink` +**replays the whole history into the new sink, inside the lock** (`:26`). Two reasons: the window is +built *after* the workspace it shows (and on Windows there is no console behind it), and switching +repositories builds a new Log pane. The replay is inside the lock so a concurrent `Write` queues +behind it rather than interleaving. + +**Gotcha:** writers are on arbitrary threads. The sink installed by the UI must marshal to the UI +thread itself; `CliLog` does not. + +### CachePath + +`$XDG_CACHE_HOME/stampeded/`, falling back to `LocalApplicationData` on Windows and `~/.cache` +elsewhere. `SpecialFolder` is deliberately not used for the non-Windows case: it has no reliable +cache mapping and **can resolve to an empty string**, which would silently turn the path relative. +Known kinds: `worktrees`, `prs`, `python-lsp`. + +### GuessFileType + +```csharp +public enum FileType { Text, Xml, Json } +public static FileType DetectTextType(string text); +``` + +Carries an ILSpy/SharpDevelop MIT header - the XML half is lifted from +`ICSharpCode.ILSpyX/Util/GuessFileType.cs`. It exists because `.props`, `.targets`, `.axaml`, +`.slnx` and `.resx` are XML that no TextMate or xshd definition claims by extension, so the file is +highlighted by content instead. + +**The one real divergence from ILSpy:** ILSpy stops at `MoveToContent()` because it already knows +the blob is a resource. Here the question is asked of any file whose extension said nothing - and +`"(T value) => value"` opens with something `XmlTextReader` will happily call an element. So this +reads **to the end of the document** (`:77`); whether the whole thing closes is what tells markup +from code that merely starts with a bracket. + +`XmlResolver = null` and `DtdProcessing.Ignore`: a DTD reference in a file *under review* must never +be followed. JSON requires a leading `{` or `[` (a bare number or string is valid JSON and is also +the first line of most text files) and allows comments and trailing commas. + +### LogFileRefs + +Finds `file:line` references inside a log line so the Log pane can make them clickable. Three forms, +because three families of tool write into this log: `Foo.cs(12,5)` (MSBuild), `Foo.cs:12` (git, gcc, +this tool), `Foo.cs:line 12` (.NET stack traces). + +Gotchas encoded in the regex: the extension must start with a **letter**, or `1.5:30` is a file at +line 30; no colon is allowed inside a path, because the colon is the separator in two of the three +forms; URLs are rejected *after* matching by looking at the characters before the path (`:36`) - a +preceding `:` or `//` means `host:port` or a URL path. `Start`/`Length` span the **whole match**, +which is the clickable span. + +### SolutionTarget + +```csharp +public static string? ForRoot(string root, string? chosen = null); +public static IReadOnlyList Candidates(string root); +public static string? ForSemantics(string root, string? chosen = null); +``` + +`ForRoot`, in order: root missing -> `null`; `chosen` wins *but only if the checkout still has it* +(a review worktree of an older revision may not); off Windows a `*.slnf` whose name contains `xplat` +wins (a full solution usually holds net472 add-ins and Windows-only test hosts, and the filter is +the repository's own statement of what builds elsewhere); otherwise the **largest** `*.sln`, else +the largest `*.slnx` - size as a proxy for "the product's own", since an installer or extension +solution holds a project or two; `null` means "let dotnet work it out", correct when there is +exactly one. + +It exists because `dotnet` errors with MSB1011 rather than guessing, and because the tests pane and +the generated-sources build were picking separately. + +**`ForSemantics` is deliberately different.** Roslyn opens a *solution*, not a filter, so when +`ForRoot` answers a `.slnf` this reads the filter's `solution.path` and returns that (`:69`). The +path in a `.slnf` is written with **Windows separators even in repositories that never see +Windows**, so it is rewritten to `Path.DirectorySeparatorChar`. + +## Git + +### BranchSync + +```csharp +public enum BranchSyncState { InSync, Ahead, Behind, Diverged, Unfetched } +public sealed record BranchSync(BranchSyncState State, int Ahead, int Behind) +``` + +`Unfetched` is the important state: the heads differ but the PR head is **not in the local object +database**, so by how much cannot be said without fetching. `GitService.GetSyncStateAsync` cannot +produce it itself (it returns `null`); the caller substitutes `BranchSync.Unfetched`. The +`Ahead`/`Behind` convention matches `git rev-list --left-right --count local...remote`: left = +local-only = ahead. + +### GitBlameParser + +Parses `git blame --porcelain`: + +``` +<40-hex sha> [] +author Jane Doe +author-time 1699999999 +summary Fix the thing +\t +<40-hex sha> <- same commit again: NO headers this time +\t +``` + +The header block appears **only on a commit's first occurrence**. So the parser keeps a +`Dictionary` cache (`:18`, `:44`) and re-points `current` at the cached entry. +Emission is driven by the **content line** (`line.StartsWith('\t')`, `:25`): a tab-prefixed line is +the only unambiguous marker, because header values can be anything. + +**Gotcha:** `int.Parse`/`long.Parse` are unguarded. Malformed porcelain throws rather than degrading. + +### GitLogParser + +```csharp +public sealed record CommitInfo(string Sha, string ShortSha, string Author, string Date, + string Subject, string Body = "", string Parents = "") +public sealed record BranchInfo(string Name, string Sha, string Date, string Subject); +``` + +**`CommitInfo.WorkingTree`** is the pseudo-commit: `new("", "uncommitted", "", "", "the work in your +checkout, not committed yet")`. An empty `Sha` *is* the marker (`IsWorkingTree`). A review of a +checkout with uncommitted work appends this to the commit series so a reader stepping through the +change reaches the part nobody else can see yet. **Anything that does `sha[..9]` or `git show sha` +on a `CommitInfo` must check `IsWorkingTree` first.** + +`Parents` is carried on the record rather than looked up, because asking git for a commit's parents +one at a time is a process each and reading a series asks for every one of them. + +**The log record format** (`GitService.cs:359`): + +``` +--format=%H%x09%h%x09%an%x09%ad%x09%P%x09%s%n%b%x00 +``` + +Two ordering decisions, both load-bearing: + +- **The body needs a terminator no commit message can contain** - hence NUL - and it comes after a + *newline* rather than a tab so a subject containing tabs stays whole. +- **The subject (`%s`) is last on the header line**, because it is the one field that can hold a + tab. `Split('\t', 6)` therefore splits only the five separators before it. Anything added after + `%s` would be cut out of a subject containing a tab. + +`ParseShortStat` reads `git log --format=%H --shortstat A..B`. **A commit that changed nothing prints +no summary and is absent from the result** - "no lines" is expressed by absence, not by a `(0,0)` +entry. + +`ParseNameStatus` takes `parts[0][0]` as the status char and **`parts[^1]` as the path** - renames +and copies (`R100`, `C75`) carry old *and* new path, and the last one is the current one. + +`ParseBranches` serves two callers: `for-each-ref refs/heads` output **and** `git stash list +--format=%gd%x09%H%x09%cs%x09%gs`, deliberately shaped the same so both go through one parser. + +### GitDiffParser + +Parses `git diff -U3 --find-renames` unified output into `FileDiff`s. + +The `diff --git a/ b/` line is **deliberately not used for paths** (`:29`). It is +authoritative only when the two paths are equal, and paths containing spaces cannot be split out of +it without heuristics. Instead: renames from the explicit `rename from ` / `rename to ` lines, +adds/deletes from `new file mode` / `deleted file mode`, paths from `--- ` / `+++ ` with `/dev/null` +filtered out, and **binary paths from the `Binary files ... and ... differ` line only**, because git +emits no `---`/`+++` for a binary add or delete (`:70`). + +**`ParseHunk` - content is bounded by the header's counts.** The body loop (`:120`) runs **while +`oldRemaining > 0 || newRemaining > 0`**, never by sniffing for the next `@@` or `diff --git`. That +is what keeps trailing blank lines and any `+`/`-`-looking content unambiguous - a removed line +whose text begins with `-` would otherwise be indistinguishable from a header. + +- `\ No newline at end of file` is skipped as metadata (`:124`). +- A **completely empty line is treated as an empty context line** (`:140`): git emits a lone space + for an empty context line, and some transports strip trailing whitespace. +- `ParseRange` handles the `,len`-omitted form, which means length 1. + +**The `i--` dance:** `ParseFile` iterates with a `for` loop; `ParseHunk` takes `ref i` and leaves it +**one past** its last consumed line, so `ParseFile` does `i--` at `:87` to cancel the increment. The +two are coupled; easy to break. + +**Known limit: combined diffs (`@@@ ... @@@`, merge commits) are not handled.** +`GitService.DiffAsync` never asks for one and `git show` of a merge goes to `PatchDocumentBuilder` +instead, but anything routing combined output here would find the wrong position. + +### GitBlobReader + +```csharp +public sealed class GitBlobReader : IDisposable +public Task ReadAsync(string revision, string relativePath, CancellationToken ct = default); +``` + +One `git cat-file --batch` per repository, kept alive, asked for one blob at a time. A review needs +the text of a few dozen files at a revision nobody has checked out. Every `.cs` file of a mid-sized +repository comes back through one batch in well under a tenth of a second; a checkout to serve the +same reads takes hundreds of milliseconds *and* a copy of the tree, and a process per file costs +more than reading them all does. **The trade is the point of the thing**, so the reader is owned by +the review that started it (`ReviewWorkspace.Blobs`, disposed at `ReviewWorkspace.cs:1359`). Do not +generalise this into "the way we run git". + +The batch protocol: request `:\n` on stdin; response ` \n`, then +exactly `size` bytes, then **one newline of git's own**. The header shares a stream with blob +content, which is read by length and may hold anything at all. So `ReadLineAsync` (`:108`) cannot use +a `StreamReader`, which would buffer past the header and eat the start of the blob - it reads single +bytes until `\n`, from `git.StandardOutput.BaseStream`. + +Anything that is not ` blob ` - `missing`, `ambiguous` - means the object database has +nothing to read, and `ReadAsync` returns `null`. **`null` is an answer, not a failure**: a file the +change adds is absent from the base, and asking is how that is discovered. + +Gotchas: + +- **Serialized by a `SemaphoreSlim(1,1)`** - the protocol is a single request/response stream. +- **A dead reader restarts silently** (`:67`): `IOException`/`InvalidOperationException` -> log, + `Stop()`, return `null`. So the *next* call works, but **this** call returns `null`, + indistinguishable from "the revision does not have that file". If you are debugging a + mysteriously-empty base side, check the log for `blob reader restarting`. +- **UTF-8 only** (`:65`). Binary blobs come back mangled; `FileDiff.IsBinary` is the guard. +- `size` is parsed as `int` - a blob over 2 GB fails the header parse and reads as `null`. +- `CreateNoWindow = true` (`:96`) is not cosmetic on Windows: a console child of a windowless + desktop process gets its own console window, and this one **outlives the review**. + +### WorktreeManager + +Layout: `//`. + +``` +git worktree prune # a stale registration for a deleted dir blocks re-adding +git worktree add --detach +``` + +`--detach` is the invariant: **review worktrees never hold a branch**, so they can never collide +with the user's checkout or block a branch operation. + +The reuse path (`:64`) checks `Directory.Exists(dir) && File.Exists(dir/.git)` - a linked worktree +has `.git` as a *file*, so this both proves it exists and proves it is still a worktree. It then +**touches the directory's last-write time**: reuse counts as use, so what the LRU keeps is what a +reader comes back to, not what happened to be built in most recently. + +`PruneToRecentAsync`: pinned SHAs survive unconditionally; of the rest, the `recent` most recently +written survive. A worktree is a copy of the whole tree **plus whatever building it leaves behind**, +and one is made for every revision ever reviewed - left alone these are the largest thing this tool +puts on a disk. + +`LinkSubmodulesFromSource`: `git worktree add` leaves submodules as **empty stubs**, so a test run in +a worktree cannot find its fixtures (the motivating case is ILSpy's `ILSpy-tests` submodule with its +offline nuget cache). `.gitmodules` is scanned with a crude `path =` line parse and each submodule +directory replaced with a **symlink to the source clone's checkout**, only when the source is +populated and the target is not. Failures are logged, never thrown. The parse is line-based, not +INI-section-aware. + +### GitService + +The class doc states the invariant: + +> Reads never touch the user's working tree or index: they come from the object database (fetch, +> merge-base, diff, show) or, for a review of uncommitted work, from a checkout's files. The +> operations that write (branch creation, rebase) touch refs only, running any checkout they need in +> a throwaway worktree - the one exception being a rebase of a branch that a checkout already has. + +So: **reviewing cannot disturb what the user has checked out; only an explicit rebase can.** + +One private helper carries most calls; note the **cancellation token comes first** so `args` can be +`params`: + +```csharp +Task RunAsync(CancellationToken ct, params string[] args) + => ExternalTool.RunAsync("git", args, repoPath, ct); +``` + +#### Result types declared in the same file + +| Type | Meaning | +| --- | --- | +| `WorktreeCheckout(Path, Branch?)` | a checkout and the branch it holds; `null` = detached | +| `PullResult(PullOutcome, Sha)` | `Created`, `FastForwarded`, `AlreadyUpToDate`, `Diverged` | +| `PushResult(PushOutcome, Sha)` | `Created`, `Pushed`, `ForcePushed`, `AlreadyUpToDate` | +| `BranchDeletion(Sha, RemovedWorktree?)` | the commit the branch pointed at (the recovery point) | +| `RebaseResult(Before, Checkout?, RebaseOutcome, WorkingDirectory)` | see below | +| `InProgressOperation(Kind, WorkingDirectory, Branch?, IsScratch, Unmerged)` | see below | + +**`RebaseResult.RecoveryCommand(branch)` - why two different commands:** + +```csharp +Checkout is null + ? $"git branch -f {branch} {Before[..9]}" + : $"git -C {Checkout} reset --hard {Before[..9]}" +``` + +A branch no checkout holds is moved with `git branch -f`, which git **refuses** for a checked-out +branch. That one is recovered with `reset --hard` *in the checkout*, so its working tree follows the +ref back. `Checkout` being non-null is precisely the signal that the rebase ran in a real checkout. + +**`InProgressOperation` - the decision table:** + +- `CanResolve => Unmerged > 0 && Kind is not Bisect` +- `CanContinue => Unmerged == 0 && Kind is not Bisect` - **git refuses to continue with conflicts + present, and offering a button git will refuse is how the merge tool came to look optional.** +- `CanSkip => Kind is Rebase or CherryPick or Revert` - a merge has one commit to make. + +#### Probing and rev resolution + +| Method | Command | Why this command | +| --- | --- | --- | +| `IsRepositoryAsync` | `rev-parse --is-inside-work-tree` | exit code is the answer | +| `RevParseAsync` / `TryRevParseAsync` | `rev-parse --verify ` | throws / `null` | +| `HasCommitAsync` | `rev-parse --verify ^{commit}` | **`rev-parse --verify` answers a full SHA with itself whether or not the object is there.** Only asking for the *commit it names* actually reads the database. | +| `IsAncestorAsync(a,d)` | `rev-list --count d..a` == "0" | **not** `merge-base --is-ancestor`, whose answer is its exit code - which this tool runner reports as a *failed command with a log line to match*, so a normal "no" would look like an error in the Log pane | +| `ReplayTreeAsync` | `merge-tree --write-tree [--merge-base=X] onto head` | see below | + +`ReplayTreeAsync` computes **the tree a rebase would produce, entirely in the object database** - no +worktree, index or ref touched. `mergeBase` is passed explicitly for the case where the branch was +rebased somewhere else entirely. **Exit code 1 means the replay conflicted**, and it is converted to +`null`: merge-tree still prints a tree then, but one with conflict markers in it, and there is no +honest way to show that as the author's code. Used by `ReviewScopes.cs:567`. + +#### Ref pinning and fetching + +`PinReviewHeadsAsync(key, head, previousHead)` -> `update-ref refs/stampeded/review//head` and +`.../prev`. Why a ref of the tool's own is required: the PR head ref is force-updated by every +fetch, `refs/stampeded/pr/N` has no reflog, and a rewritten branch's old tip is referenced by nothing +at all. Without this, **the commit the reader compared against last time is prunable** and the +re-review diff evaporates. + +`FetchPrHeadAsync(refspec, number)` takes its refspec **from the host** - GitHub advertises every PR +head as a ref, Azure DevOps does not and the source branch is fetched instead. + +#### Diff reading + +``` +DiffAsync(baseRev, headRev) -> git diff -U3 --find-renames +DiffWorkingTreeAsync(worktreePath, base) -> git diff -U3 --find-renames (cwd = worktree) +ShowFileAsync(rev, path) -> git show : +DiffPatchAsync(base, head) -> git diff (raw patch text) +DiffNameStatusAsync(a, b) -> git diff --name-status --find-renames a b +ListFilesAsync(rev) -> git ls-tree -r --name-only -z +``` + +`DiffWorkingTreeAsync` compares against the **working tree**, so it reports staged and unstaged +alike. Untracked files are not in it - git does not track them and neither does a review. It sorts +explicitly because git's working-tree diff order differs from its commit-diff order. + +`ListFilesAsync` uses `-z` so paths with newlines survive, and reads from the object database rather +than a checkout - it is the *revision's* list, and it costs nothing when no checkout exists yet. + +#### Worktree enumeration and status + +`ListWorktreesAsync` parses `git worktree list --porcelain`. Detached checkouts have no `branch` +line, so `Branch == null`. + +`IsDirtyAsync` -> `git status --porcelain --untracked-files=no`. **Untracked files deliberately do +not count**: they are not part of the change under review, and a checkout holding nothing but build +output is not a review step. + +`FindCheckoutAsync(branch)` encodes the invariant: *a branch can be in only one checkout.* + +#### Branch listing and merge analysis + +**The two-question merge test.** `git branch --merged` is an ancestry test: cheap, one call for +every branch, and it answers "is this in there as it stands". A **rebase-merged branch is not among +them** - its commits were replayed and none of the originals survives. That is what +`IsMergedByPatchAsync` is for: `git cherry` marks a commit `-` when upstream has one with the same +patch id and `+` when it does not, so the branch is in when **nothing is marked `+`**. Documented +edge cases: a branch with no commits of its own answers true (correct - nothing left to merge); a +**squash-merged** branch of more than one commit answers **false**, because its commits were combined +into one whose patch matches none of them. + +**`%(ahead-behind:...)` requires git 2.41+.** It was chosen because git can answer for all branches +at once; asked branch by branch it was one process each. + +**`ListStashesAsync` reuses `ParseBranches`** by shaping `git stash list` output into the same four +tab-separated fields. The payoff (`:550`): a stash's own commit holds the stashed working tree and +its **first parent is the commit it was taken on**, so `sha^..sha` is exactly what `git stash show` +reports - and a stash therefore reviews as an ordinary local range with no special case anywhere +above. + +#### Branch mutation + +**`DeleteBranchAsync`** - three decisions: + +1. Returns the commit the branch pointed at, because that is what it takes to offer the branch back. +2. **The worktree holding the branch is removed first** - git refuses to delete a branch some + checkout has. +3. **`-D`, not `-d`, and that is not a shortcut.** `git branch -d` tests the branch against its + *upstream*, or against *HEAD* when it has none - neither of which is the default branch. It gets + the answer wrong in both directions: it refuses a branch that is an ancestor of the default branch + while HEAD happens to lag behind it, and it cannot recognise a rebase merge at all. The caller + establishes the fact that matters against the ref that matters. + +**`RemoveWorktreeAsync`** - the submodule escape hatch. `git worktree remove` **rejects any worktree +containing submodules outright**; the check runs before `--force` is even consulted, so a repository +with a submodule could otherwise never have a worktree removed here. The fallback catches +specifically `ex.StdErr.Contains("submodules")`, then **establishes for itself what git would have +enforced**: `git status --porcelain --ignore-submodules=none` must be empty, or it throws +`RefusedException` and deletes nothing. Only then `Directory.Delete(recursive)` plus `git worktree +prune` - because **the administrative entry outlives the directory, and the branch stays checked out +as far as git is concerned until it is gone.** Note this is *stricter* than git's own `--force`, +which would discard uncommitted work. + +**`PullBranchAsync`** never merges. Divergence needs a rebase, which is a different decision offered +separately. A checkout that holds the branch must move *with* it rather than be left behind. + +**`PushBranchAsync`**: `remote == local` -> up to date; fast-forward -> plain push; otherwise `git +push --force-with-lease`. **Nothing here fetches first, deliberately** (`:857`): fetching would +refresh the very ref the lease is compared against and turn `--force-with-lease` back into a plain +`--force`. The lease is what makes this safe. + +#### The rebase driver + +```csharp +public async Task RebaseBranchAsync(string branch, string onto, + IProgress? progress = null, CancellationToken ct = default); +``` + +1. **Refuse if an operation on that branch is already in progress** - and the check is **by branch, + not by checkout**. A checkout in the middle of a rebase is *detached*, so it does not look like it + holds the branch at all; that is how a retry used to reach git and come back with a fatal about a + `rebase-merge` directory, having never run the merge tool. +2. `before = rev-parse ` - the recovery point. +3. Find the checkout holding the branch; if none, `git worktree add --quiet `. + **This worktree is not detached** - it must hold the branch, because that is what the rebase + moves. (Contrast `WorktreeManager`, which always detaches.) The `stampeded-rebase-` prefix is how + `ListInProgressAsync` later recognises a scratch checkout. +4. `git rebase `. +5. On failure with **nothing unmerged**, the rebase never started - nothing is in progress to abort, + the branch is untouched, so rethrow. +6. Otherwise drive `ResolveConflictsAsync`. If it does not finish, **the scratch worktree is + deliberately left behind**, because the rebase is still in progress in it and discarding it would + throw away the resolutions the user just made. +7. `finally`: a scratch worktree not being left in place is removed with `worktree remove --force`, + falling back to `worktree prune` **plus `Directory.Delete`** - pruning deregisters but does not + remove the directory. + +**`ResolveConflictsAsync`** - the conflict loop, up to **50 steps** (a rebase stops once per +conflicting commit). Each step: snapshot the conflicted paths, run `git mergetool -y` (**`-y` +because git otherwise prompts on a terminal this process does not have**), then three checks in +order: still unmerged -> give up; **conflict markers still in the files** -> give up; `git rebase +--continue` with `GIT_EDITOR=true`. + +Check two is the one that matters (`:789`): *git marks a file resolved when the tool exits without +saying otherwise - for most tools that means "the file was touched", which an editor closed without +a decision also does. Continuing on that word commits the markers themselves, and the rebase reports +success. What the file says is the only thing that cannot be faked.* `WithConflictMarkersAsync` +requires **both** `<<<<<<<` and `>>>>>>>` at the **start of a line**: one alone is ordinary text +often enough (a diff quoted in a comment). + +`progress` exists because the merge tool is a child process with no terminal of its own and a window +that need not come to the front, so a rebase stopped on a conflict otherwise looks like one that +stopped responding. + +#### In-progress detection, by reading `.git` directly + +**No git process is started to answer this.** `AdminDirectoryAsync` resolves `/.git` - a +directory for the main worktree, a file containing `gitdir: ` for a linked one - and +`FindOperation` tests for: + +| Marker | Operation | +| --- | --- | +| `rebase-merge/` or `rebase-apply/` | Rebase | +| `MERGE_HEAD` | Merge | +| `CHERRY_PICK_HEAD` | CherryPick | +| `REVERT_HEAD` | Revert | +| `BISECT_LOG` | Bisect | + +Why: *a repository with forty worktrees is a repository where one process per worktree is the +difference between a check that can run whenever the window is focused and one that cannot.* This is +polled on focus. + +`RebasingBranchAsync` reads `/rebase-merge/head-name` and strips `refs/heads/`, because **a +rebase detaches HEAD while it runs, so the worktree listing reports no branch for exactly the +checkout that has one at stake. Git wrote it down; read it.** + +`ListInProgressAsync` asks **every** checkout git knows about, not just the ones this tool made: a +reader who merged by hand in their own checkout is stuck in exactly the same way. + +## Diff + +### DiffModel + +```csharp +public enum FileChangeKind { Modified, Added, Deleted, Renamed } +public sealed record GeneratedSource(string? BaseFile, string? HeadFile); +public sealed record FileDiff(string OldPath, string NewPath, FileChangeKind Kind, bool IsBinary, + IReadOnlyList Hunks, GeneratedSource? Generated = null) +public sealed record DiffHunk(int OldStart, int OldLength, int NewStart, int NewLength, + string Header, IReadOnlyList Lines); +public enum PatchLineKind { Context, Added, Removed } +``` + +- **`Path` is the display key and the review-state key**: `NewPath`, except `OldPath` for a + deletion. A rename has two paths; a deletion is only ever asked about by its old one. +- `GeneratedSource` holds **filesystem paths, not git paths** - a generated file is not in git. +- `IsGenerated` earns its own concept because such a file *has no history to blame, no place on the + host to carry a comment, and no claim on the reader's time in the way handwritten code has.* +- `DiffHunk.Header` is git's trailing function-context hint, not the `@@` text. + +### ChangedLines + +```csharp +public IReadOnlySet Added(string path); // new-side lines added +public IReadOnlySet Removed(string oldPath); // old-side lines removed +public IReadOnlySet CommentableNew(string path); // every new-side line a hunk printed +public IReadOnlySet CommentableOld(string oldPath); // every old-side line a hunk printed +``` + +**The "commentable" concept:** a comment can be attached to **every line a hunk prints on that side, +context included** - because that is what the host will take a comment on. Not just the changed +lines. Getting this wrong means offering a comment the host rejects. + +**The single-walk rule.** A hunk carries its own starting line numbers and **nothing else does**, so +following them is the only way to turn a run of `+`/`-` markers into numbers. All four sets come +from **one walk** (`:36`) precisely because *doing that walk in more than one place is how two +answers about the same diff come to disagree.* If you need a fifth question about changed lines, add +it to this walk. + +### DiffSlider + +```csharp +public static List Shift(IReadOnlyList oldLines, IReadOnlyList newLines, + IReadOnlyList runs); +``` + +**The problem:** when a run's first line repeats immediately after it, the run can start a line later +and still describe the same change - **the diff is genuinely ambiguous, and the aligner's choice is +arbitrary.** In brace-delimited code this is constant: an added method comes out starting at the +*closing brace of the method before it* and ending inside itself. Valid, and unreadable. + +The algorithm, per run: + +- Only a **one-sided** run can slide (insert or delete); a replacement is anchored by the lines it + stands against. **Both neighbours must be matches**, since sliding trades lines with them. +- `MaxUp`: each step moves the run's last line out and the line before it in, which **only describes + the same change while those two are equal** - `lines[start-s-1] == lines[start+length-s-1]`. + Bounded by the previous match run's length. `MaxDown` is the mirror. +- `BestShift` evaluates `Rank` at every reachable position, keeping any rank **better or equal**; + ascending order plus `<= 0` means **ties resolve to the largest (most downward) shift**. +- `Rank(lines, start) = (StartsParagraph ? 0 : 1, Indent)` - *a paragraph boundary first, then the + shallower indentation.* Lower is better. A block that starts where the code steps outward reads as + a block. + +**Why ties go downward:** *an inserted block belongs after the closing line of the one before it +rather than at it, which is the whole shape of the problem in brace-delimited code: two members that +end alike leave the cut free to sit on either one's brace, and only the later reads as the new +member.* + +Called from exactly one place: `DiffDocumentBuilder.Align`, between `DiffLib.Diff.CalculateSections` +and `DiffLib.Diff.AlignElements`. + +### DiffDocumentModel + +```csharp +public enum DiffLineKind { Context, Added, Removed, Filler, Comment } +public readonly record struct IntraLineSpan(int Start, int Length); +public readonly record struct DiffLineTag(DiffLineKind Kind, int OldLine, int NewLine, + IReadOnlyList? WordDiffs); +public readonly record struct HunkSpan(int FirstDocLine, int LastDocLine); // 1-based inclusive +``` + +`OldLine`/`NewLine` are **1-based blob line numbers, 0 when the line does not exist on that side**. +Everything in the layer keys off that: `Context` both non-zero; `Added` `OldLine == 0`; `Removed` +`NewLine == 0`; `Filler` (side-by-side padding) both 0; `Comment` (synthetic thread row) both 0. + +**The invariant that makes the whole editor work** (class doc, `:132`): + +> The full NEW file text with REMOVED lines interleaved as verbatim old-blob lines. **Every document +> line is a verbatim copy of a blob line**, so `(docLine, column)` maps exactly to `(blobLine, +> column)` on whichever side the line exists. All position translation between the editor and the +> old/new blobs goes through this map. + +No `+`/`-` prefix column, no padding, no tab expansion. That is why a semantic provider's answer at +`(line, character)` can be carried straight onto a document row, and why a comment anchor round-trips +exactly. + +`GetSideText(oldSide)` reconstructs one side's text by dropping rows where that side's line number is +0, and returns the parallel `SideToDocLine` map. **The document itself is not valid source of either +side** (removed lines interleave), so anything that wants to parse - the structure provider, the fold +computation, the syntax painter - takes the reconstructed side text and maps results back. + +**`WithThreadLines` is a pure splice.** It inserts a synthetic row `@@thread:@@` **below** each +anchor's document row; the view replaces that text with an interactive control. The diff is **not** +recomputed. An anchor with `BlobLine == 0` (an outdated comment) is pinned before the first line. The +inserted row gets `DiffLineKind.Comment` with **both line numbers 0**, so nothing that reads a side's +own text sees it. Hunk spans shift through a prefix-sum array; the asymmetry at `:256` is deliberate: +**an insertion sits below its anchor, so a hunk ending exactly at the anchor stretches over the +thread while a hunk starting after it just moves down.** + +**`SideBySideModel`** keeps **equal line counts on both sides, always**, with `Filler` rows. A thread +row must be inserted into **both** documents at the same index (`:72`): *the two panes are kept in +step by copying one scroll offset to the other, which is only exact while they hold the same number +of rows.* + +**`DiffDocumentBuilder`:** + +- `#nullable disable warnings` covers the whole builder (`:276`): DiffLib annotates its generic + parameters as `IList`, making every call site a nullability mismatch for `T = string`. +- `Align` is the shared pipeline: `CalculateSections` -> `DiffRun[]` -> `DiffSlider.Shift` -> + `DiffSection[]` -> `AlignElements` with `StringSimilarityDiffElementAligner`. The aligner is what + turns a `Delete`+`Insert` pair of *similar* lines into a `Replace`/`Modify` element, which is what + makes word-level highlighting possible. +- `Build` emits GitHub-style: within a changed run, **all removals first, then all additions**. +- **`SplitLines`** encodes git's line model: + ```csharp + if (text.Length == 0) return []; // an empty blob has NO lines + text = text.ReplaceLineEndings("\n"); + if (text.EndsWith('\n')) text = text[..^1]; // exactly ONE trailing newline stripped + ``` + `"a\n"` is **one** line. Get this wrong and every added or deleted file gains a phantom trailing + line. +- **`Tokenize`** splits a line into a run of identifier characters, a run of whitespace, or one + character of anything else. Why not per-character: *comparing single characters makes a renamed + identifier light up as fragments of the letters it happens to share with the old name - "oldName" + against "newName" matching on "N", "ame" and lighting the rest - which reads as noise rather than + as one thing having been replaced.* `SpanOver` emits **one span per changed run**, not one per + token. + +### PatchDocumentBuilder + +Turns a unified patch as git prints it (`git show`, `git diff a b`) into the same +`DiffDocumentModel` a review file uses, so a whole commit reads with the colouring, the line-kind +margin and the hunk navigation *instead of as grey text with `+`/`-` in it*. + +**The deliberate difference:** the patch text is kept **verbatim, prefix characters and all**. Two +reasons (`:8`): the document spans many files, so its lines cannot be blob lines of any one of them; +and **the `+`/`-` column is the only thing that still says which file a line belongs to once you +scroll.** This knowingly breaks the `DiffDocumentModel` invariant - anything treating a patch +document as navigable source will be off by one column. + +The state machine turns on `inHunk`: **outside a hunk, every line is prose or a header**, tagged +`Context(0,0)`. That is the whole point - *a commit message body is indented, so `' '` and `'-'` +there must not be read as context and removal.* A commit message line starting with `-` is a bullet. + +### ContextGaps + +```csharp +public const int Context = 5; // lines left visible on each side of a hunk +public const int Step = 20; // lines revealed per click, as GitHub does it +public const int MinHidden = 6; // below this, a control costs more than the lines +``` + +**This is deliberately *not* folding** (`:15`): *Folds are the code's own structure - types, members, +`#region`s - and a reader collapses and expands them for reasons that have nothing to do with the +diff. Hiding context with the same mechanism made the two fight: expanding a method to read it also +unhid unrelated context, "collapse all" swallowed the change, and the two kinds of region cannot +always nest.* The same warning is repeated in `DiffFolding`'s doc. **Do not merge these two +mechanisms.** + +`MinHidden = 6` is a considered number: *a bar standing for three lines costs the reader more +attention than reading the three lines does.* + +The algorithm is two passes. **Pass 1** scans for maximal runs of `Context` and trims `Context` lines +off each end - **except at the document edges**, where the run has a hunk on one side only. +`hasChanges == false` returns no gaps at all, which is **how a plain source view stays whole**. + +**Pass 2** cuts around declaration headers, only when `declarations` is non-empty. `Headers` picks +the declarations that **contain a change** - *a type declared five hundred lines above still says +what the change is part of, while the member just above the one being changed says nothing about it +however close it sits.* The containment test is a **prefix sum of changed lines** (`:87`), so asking +about a range is one subtraction. `Split` cuts one gap around the headers inside it: what lies above +a header stays hidden, the header is shown, and what lies between it and the next header is hidden +only when it clears `MinHidden`. + +`RevealTop`/`RevealBottom` are pure: `null` means the gap opened completely and the control +disappears. + +### DiffFolding + +```csharp +public sealed record FoldRange(int StartLine, int EndLine, string Name, bool DefaultClosed, + bool FromHeaderEnd, int HeaderEndLine); +``` + +Expressed in **document lines, before being turned into offsets** against a particular editor's +document - *the side-by-side view installs the same ranges in two editors whose line lengths differ, +so ranges and offsets have to stay separate.* + +- `FromHeaderEnd` - fold from the *end* of the first line, so a member's signature stays visible + while its body collapses. +- `HeaderEndLine` - the last line of the declaration itself. *A header wrapped over several lines is + one thing to read.* + +`Members` is a pure coordinate translation from `MemberFoldRegion` (1-based side lines, from +`ISemanticProvider`) through `sideToDocLine` into document lines. The indirection exists because *a +diff document is not valid source of either side, so the regions are found in one side's own text - +by whichever provider serves that language - and mapped back here through the line map.* + +`FoldRange` is also the input type for `ContextGaps.Compute(declarations:)` - the same records feed +both mechanisms even though the mechanisms are kept apart. + +## Call graph inside the layer + +``` +GitService + |- ExternalTool.RunAsync (every git invocation) + |- GitDiffParser.Parse <- DiffAsync, DiffWorkingTreeAsync + |- GitLogParser.Parse <- LogAsync, LogPickaxeAsync + |- GitLogParser.ParseShortStat <- GetCommitStatsAsync + |- GitLogParser.CountPathTouches <- GetChurnAsync + |- GitLogParser.ParseBranches <- ListBranchesAsync, ListStashesAsync + |- GitLogParser.ParseNameStatus <- DiffNameStatusAsync + |- GitBlameParser.Parse <- BlameAsync + |- BranchSync.From <- GetSyncStateAsync + \- GitHub.GitHubUrl.TryParse <- GetOriginOwnerAsync + +WorktreeManager -> CachePath.For("worktrees"), ExternalTool.RunAsync, CliLog.Write +GitBlobReader -> CliLog.Write; raw Process, NOT ExternalTool + +DiffDocumentBuilder + |- Align -> DiffLib.CalculateSections, DiffSlider.Shift, DiffLib.AlignElements + |- Build / BuildPair -> Align, SplitLines, ComputeWordDiffs -> Tokenize -> SpanOver + \- Build -> ComputeHunks +PatchDocumentBuilder.Build -> ParseHunkStarts, DiffDocumentBuilder.ComputeHunks +ContextGaps.Compute -> Add, Headers, Split (Headers/Split take FoldRange from DiffFolding) +``` + +Test coverage in `tests/Stampeded.Core.Tests/`: `ChangedLinesTests`, `ContextGapTests`, +`DiffDocumentBuilderTests`, `SideBySideBuilderTests`, `DiffFoldingTests`, `DiffSliderTests`, +`PatchDocumentBuilderTests`, `ThreadLineTests`, `GitBlobReaderTests`, `WorktreeCacheTests`, +`GuessFileTypeTests`, `LogFileRefsTests`, `SolutionTargetTests`, plus `GitRebaseTests` / +`GitPushTests` which build real repositories in temp directories. diff --git a/docs/pull-request-hosts.md b/docs/pull-request-hosts.md new file mode 100644 index 0000000..07245d1 --- /dev/null +++ b/docs/pull-request-hosts.md @@ -0,0 +1,624 @@ +# Pull-request hosts and review state + +`src/Stampeded.Core/{PullRequests,GitHub,AzureDevOps,MergeQueue,Review}`. + +## Shape of the layer + +``` +PullRequestHosts.ForAsync(repoPath) <- decided once per workspace, from origin's URL + | + v +IPullRequestHost <- the only vocabulary above this line is GitHub's + |-- GitHubService(repoPath) over `gh` + \-- AzureDevOpsService(repoPath, org, project, repo) over `az` + the azure-devops extension + +PrCache <- what only the host knows, on disk, so a review opens offline +ReviewStateStore <- what the *reader* did, on disk, per review +CommentAnchor <- glue that survives a force-push +MergeQueueService <- a queue in a git ref, driven through IPullRequestHost +``` + +Everything outside the two implementations speaks GitHub: `APPROVE` / `REQUEST_CHANGES` / +`COMMENT`, `APPROVED` / `CHANGES_REQUESTED`, `LEFT` / `RIGHT`, `MERGEABLE` / `CONFLICTING`, `CLEAN` +/ `UNSTABLE` / `BLOCKED` / `BEHIND` / `DIRTY` / `DRAFT`. Azure DevOps translates into that inside +`AzureDevOpsService` and nowhere else. The only thing a pane may say out loud about which host +answered is `IPullRequestHost.Name`, surfaced as `ReviewWorkspace.HostName`. + +### Host selection + +`PullRequestHosts.ForAsync` (`PullRequestHosts.cs:16`): + +1. `git config --get remote.origin.url`, with **exit 1 accepted** (`:39`) - a clone that was never + pushed is an answer, not a failure. +2. `AzureDevOpsUrl.TryParse(origin, ...)` decides. Anything it does not parse is GitHub **on + purpose**: `gh` also serves GitHub Enterprise hosts, which cannot be enumerated here. +3. `STAMPEDED_PR_HOST=github|azdo` overrides (`:19`). Note the override still uses the org/project/ + repo the parse produced - forcing `azdo` on a GitHub clone constructs + `AzureDevOpsService("", "", "")` and every command will fail. + +## IPullRequestHost - the contract + +| Member | Meaning | GitHub | Azure DevOps | +| --- | --- | --- | --- | +| `Name` | "GitHub" / "Azure DevOps" | const | const | +| `AcceptsOwnApproval` | whether the host takes a verdict from the PR's own author | `false` | `true` | +| `PrHeadRefspecAsync(int)` | refspec fetching PR head into `refs/stampeded/pr/N` | `+refs/pull/N/head:refs/stampeded/pr/N` - synchronous, always works, fork or not | reads `az repos pr show`, **refuses forks**, uses `+refs/heads/:refs/stampeded/pr/N` | +| `PrUrlAsync` / `CommitUrlAsync` | browser URL, `null` when the repo is not on this host | `gh repo view --json nameWithOwner` | computed from org/project/repo, never null | +| `GetViewerLoginAsync()` | the account the CLI is signed in as | `gh api user --jq .login`, cached | `az devops invoke Location/ConnectionData` -> `authenticatedUser.properties.Account.$value` (a UPN), falling back to `az account show` | +| `GetDefaultBranchAsync()` | authority for the default branch | `gh repo view --json defaultBranchRef` | `az repos show` -> `defaultBranch`, `refs/heads/` stripped | +| `ListOpenPrsAsync()` | the open list | one `gh pr list` with 16 JSON fields | `az repos pr list --status active --top 50`; **no line totals, no check state** | +| `GetPrAsync(int)` | title, body, branches, state, author, draft | `gh pr view N --json ...` | `az repos pr show --id N` | +| `GetChecksAsync(int)` | check runs | `gh pr checks N --json name,state,link,bucket,workflow` | build **policy evaluations** only | +| `GetMergeStateAsync(int)` | would the host merge right now | `gh pr view N --json mergeable,mergeStateStatus,...` | `pr show` + `pr policy list`, folded | +| `GetIssueTitleAsync(int)` | title of `#N`, or `null` | `gh api repos/{owner}/{repo}/issues/N`; 404 is the answer | `az boards work-item show --id N` | +| `GetIssueUrlPrefixAsync()` | prefix for autolinking `#N` | `.../issues/` | `.../_workitems/edit/` | +| `GetMergeMethodsAsync()` | which merge strategies the repo allows | `gh repo view --json mergeCommitAllowed,...`, cached | the target branch's "Require a merge strategy" policy; **all three on when there is none** | +| `MergePrAsync(int, method, deleteBranch)` | `method` is a *gh flag name* - `merge`, `squash`, `rebase` | `gh pr merge N --{method} [--delete-branch]` | REST `PATCH` with `completionOptions.mergeStrategy` (`noFastForward`/`squash`/`rebase`) | +| `MarkReadyForReviewAsync(int)` | out of draft | `gh pr ready N` | `az repos pr update --id N --draft false` | +| `GetFailedLogAsync(long runId)` | log of the failed steps | `gh run view --log-failed` | build timeline + per-record logs, assembled into the same shape | +| `GetReviewCommentsAsync(int)` | posted line comments | `gh api .../pulls/N/comments --paginate` | PR threads with a `threadContext.filePath` | +| `GetReviewsAsync(int)` | submitted reviews, oldest first | `gh api .../pulls/N/reviews --paginate` | **synthesized from votes** | +| `GetThreadResolutionsAsync(int)` | thread resolution + the REST ids of each thread's comments | GraphQL `reviewThreads` | thread `status` | +| `SetThreadResolvedAsync(threadId, bool)` | resolve/unresolve | GraphQL mutation with an opaque node id | id is `"{prNumber}/{threadId}"`, split apart again | +| `UpdateBranchAsync(int)` | rebase PR branch onto target **on the server** | `gh api -X PUT .../pulls/N/update-branch -f update_method=rebase` | **throws `RefusedException`** - no such API | +| `SubmitReviewAsync(int, ReviewSubmission)` | a verdict plus its line comments | one POST to `.../pulls/N/reviews` | comments first, then a vote - there is no review object | +| `ReplyToCommentAsync(int, long, string)` | answer inside an existing thread | POST `.../pulls/N/comments/{id}/replies` | POST into the thread, `parentCommentId` | +| `HasMergeQueueWorkflowAsync()` | does something on the host drain the queue | looks for `stampeded-merge-queue.yml` among active workflows | always `false` | +| `DispatchMergeQueueAsync()` | wake that drainer | `repository_dispatch` with `event_type=stampeded-merge-queue` | no-op | + +### The error model + +Two exception types, both meaning "it did not happen, and the message says why": +`ToolFailedException` (a CLI said no; has exit code and stderr) and `RefusedException` (*this tool* +said no, before running anything). `ExternalTool.RunAsync` converts a `Win32Exception` (the binary +is not installed) into `ToolFailedException(exe, -1, ...)`, so a missing `gh` never escapes +unhandled and leaves a pane spinning. + +## The data model + +### Small records + +| Record | Meaning | +| --- | --- | +| `PrAuthor(Login)` | a user. On Azure DevOps the "login" is a UPN. | +| `PrLatestReview(Author?, State?)` | one reviewer's last word | +| `PrRepoOwner(Login)` | owner of the *head* repository - the fork test | +| `PrReviewRequest(Login?)` | `Login` is nullable **because a team has none**: `reviewRequests` holds both, and gh names the team, not its members | +| `PrDetail(Number, Title, Body, BaseRefName, HeadRefName, State, Author, IsDraft)` | `State` is GitHub's `OPEN`/`CLOSED`/`MERGED`; Azure DevOps puts `active`/`completed`/`abandoned` here and *nothing compares it* | +| `CheckRun(Name, State, Bucket, Link, Workflow, RunId?)` | `Bucket` is `gh pr checks`'s own word: `pass`/`fail`/`pending`/`skipping`/`cancel`. `RunId` is `null` for a check reported from somewhere neither CLI can read | +| `MergeMethods(...)` | `Allowed` returns the *gh flag names* `merge`/`squash`/`rebase`, in GitHub's own menu order | +| `ThreadResolution(ThreadId, IsResolved, CommentIds)` | `ThreadId` is opaque - a GraphQL node id on GitHub, `"pr/thread"` on Azure DevOps | +| `ReviewCommentDto(Path, Line, Side, Body)` | one line comment of a submission | +| `ReviewSubmission(Body, Event, Comments)` | `Event` is `APPROVE` / `REQUEST_CHANGES` / `COMMENT` | + +### PrSummary + +Positional fields map 1:1 onto `gh pr list --json`. Two are stamped on **after** the list is read +and are `init`-only: + +- `ViewerLogin` - without it, "approved" cannot be told from "approved by *me*". +- `OriginOwner` - without it, a head branch name means nothing: a PR names the branch as it is + called in the repository it lives in, which for a fork is not this one. + +```csharp +public bool HeadIsFork => OriginOwner is { Length: > 0 } origin + && HeadRepositoryOwner is { Login.Length: > 0 } head + && !string.Equals(head.Login, origin, StringComparison.OrdinalIgnoreCase); +``` + +Owner alone decides it - a fork cannot sit beside its original under the same account. + +`ApprovedByMe` is read from `LatestReviews`, **not** from `ReviewDecision`: a PR can be approved +without your vote and voted on without being approved. `ReviewRequestedFromMe` is by name only; a +team request is nobody's in particular. + +### CheckRollup - one folding, two panes + +A PR's status-check rollup arrives as one array mixing two kinds: modern check runs (`conclusion`) +and the older status contexts (`state`). It is read in exactly one place because the PR list and the +merge state fold it the same way, and two foldings that drift apart would show the same review green +in one pane and failing in another. + +```csharp +public static string Verdict(JsonElement item) +{ + string? conclusion = ...; string? state = ...; + return ((conclusion is { Length: > 0 } ? conclusion : state) ?? "").ToUpperInvariant() switch { + "FAILURE" or "ERROR" or "TIMED_OUT" or "STARTUP_FAILURE" or "CANCELLED" or "ACTION_REQUIRED" => "fail", + "" or "PENDING" or "IN_PROGRESS" or "QUEUED" or "EXPECTED" or "WAITING" or "REQUESTED" => "pending", + _ => "green", + }; +} +``` + +`conclusion` wins over `state`. **Anything not named is green** - `SUCCESS`, but also `SKIPPED` and +`NEUTRAL`, which are not a check saying no. `CANCELLED` deliberately counts as a failure. +`Bucket` is worst-first: any fail -> `"fail"`; else any pending -> `"pending"`; else `"green"` if +there was anything at all, otherwise `"none"`. + +### MergeState + +```csharp +public sealed record MergeState(string? Mergeable, string? MergeStateStatus, string? ReviewDecision = null, + bool IsDraft = false, string? BaseRefName = null, JsonElement? StatusCheckRollup = null, + string? State = null, string? HeadRefOid = null) +{ + public string Host { get; init; } = "GitHub"; + public bool CanMerge => Mergeable == "MERGEABLE" + && MergeStateStatus is "CLEAN" or "UNSTABLE" or "HAS_HOOKS"; +``` + +`UNSTABLE` is a failing or pending check on a PR GitHub *would still merge* - the reader's call, not +a refusal. `UNKNOWN` is what GitHub answers without push access, and offering a button that will be +rejected is worse than not offering one. `State` and `HeadRefOid` exist for the merge queue: whether +somebody merged or closed it since, and whether the branch still carries the queued revision. + +`Summary` gives at most **two** reasons, ordered by what the reader would do: fix first (conflicts, +draft), then wait (behind, failing checks, running checks), then what someone else owes (changes +requested, no approving review). `Explain` is the long form, one line per reason. + +**`Host` is why both read `{Host}` and not `"GitHub"`.** It is `init`-only and defaults to +`"GitHub"`; Azure DevOps sets it. A third host that forgot would silently say "GitHub". + +### PostedComment + +```csharp +public sealed record PostedComment(long Id, string Body, string Path, int? Line, string? Side, PostedUser? User, + [property: JsonPropertyName("original_line")] int? OriginalLine, + [property: JsonPropertyName("diff_hunk")] string? DiffHunk, + [property: JsonPropertyName("original_commit_id")] string? OriginalCommitId, + [property: JsonPropertyName("html_url")] string? HtmlUrl = null); +``` + +`Line` is `null` once GitHub has stopped tracking the comment (the diff moved past it) - that is the +trigger for the whole `CommentAnchor` path. `OriginalLine` + `DiffHunk` are what survive; +`OriginalCommitId` is the commit the comment was written against, still in the object database +whenever that head was ever fetched, which is what lets the member-relocation path work. + +`PrReview.CommitId` is what makes the overview's **stale-review** marker possible - a verdict given +on a head that is no longer current. Azure DevOps sets it `null`. + +## GitHubService - exactly what it runs + +Constructed with `repoPath`; every command runs with that working directory so `gh` resolves the +repo from origin and fills in the `{owner}`/`{repo}` placeholders itself. There is no token of its +own - auth, SSO and refresh ride on `gh auth`. + +Serialization is a source-generated context over `JsonSerializerDefaults.Web` + +`PropertyNameCaseInsensitive`, so gh's camelCase JSON lands on the PascalCase records without +attributes, and the snake_case REST fields carry explicit `[JsonPropertyName]`s. + +**Cached for the process lifetime** (they cannot change without gh being re-authenticated or the +repo settings edited underneath): `viewerLogin`, `defaultBranch`, `mergeMethods`, `ownerRepo`, +`hasMergeQueueWorkflow`, and the per-number `issueTitles` memo. + +### The two GraphQL calls + +REST does not expose thread resolution, so `GetThreadResolutionsAsync` issues: + +```graphql +query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100) { + nodes { id isResolved comments(first: 50) { nodes { databaseId } } } + } + } + } +} +``` + +via `gh api graphql -f query=... -f owner=... -f repo=... -F number=N` (`-F` for the typed Int). It +walks the result with hard `GetProperty` calls - **any shape change throws `KeyNotFoundException`, +not a `ToolFailedException`**. The `first: 100` / `first: 50` caps are silent: a PR with more than +100 threads loses resolution state on the rest. + +`SetThreadResolvedAsync` posts `resolveReviewThread` / `unresolveReviewThread`. + +### Notable details + +- **`gh pr checks` exits non-zero when checks failed or are pending.** So `GetChecksAsync` drops out + of `ExternalTool.RunAsync` and uses CliWrap directly with `CommandResultValidation.None`, taking + the JSON from stdout regardless of exit code, and only throwing when stdout is *empty* and the + exit code is non-zero. +- **Run ids come from the link, by regex**: `[GeneratedRegex(@"/actions/runs/(\d+)")]`. Only a + GitHub Actions link carries one; a check reported by anything else opens nothing. +- **Issue titles memoize the *task*, not the answer** - a description rebuilt twice in a row would + otherwise ask about every number twice before either reply arrived. The dictionary is unlocked + because the callers are all on the UI thread. A 404 is the *answer* (`#141414` is a colour), not a + failure. +- **Reply is its own request**: a review submission's comments take only a path and a line, so a + comment meant as an answer would start a *new* thread on that line. A reply cannot be batched into + a pending review. + +## AzureDevOpsService - mapping onto GitHub's vocabulary + +Every command names the organization explicitly rather than leaning on `az devops configure +--defaults`, which is a per-machine setting a reader may have pointed at another project entirely. + +```csharp +const string ApiVersion = "7.1"; +readonly string orgUrl = $"https://dev.azure.com/{org}"; +string[] OrgArgs => ["--organization", orgUrl, "--output", "json"]; +string[] ProjectArgs => [.. OrgArgs, "--project", project]; +``` + +`--project` is **not** universal: the commands addressed by pull-request id (`pr show`, `pr policy +list`, `pr set-vote`, `pr update`) and `az devops invoke` reject it. + +`InvokeAsync(area, resource, httpMethod, route, jsonBody, ct)` is the `gh api` analogue. A body can +only be handed to `az` **in a file**, so it is written to `%TEMP%/stampeded-.json` and deleted +in a `finally`. The log names the route, never the body - review prose can be a page long. An empty +response is normalized to `{}` before parsing. + +### The mappings + +**Votes -> GitHub review states.** Azure DevOps scores a reviewer from 10 to -10: + +| Vote | Meaning | Mapped to | +| --- | --- | --- | +| `10` | approved | `APPROVED` | +| `5` | approved with suggestions | `APPROVED` | +| `0` | no vote | *not a review* - becomes a `PrReviewRequest` instead | +| `-5` | waiting for the author | `CHANGES_REQUESTED` | +| `-10` | rejected | `CHANGES_REQUESTED` | + +**No review object -> votes as reviews.** `GetReviewsAsync` manufactures one `PrReview` per non-zero +vote with `CommitId: null` and `SubmittedAt: null`. `CommitId: null` is load-bearing: Azure DevOps +does not record which commit a vote was cast on - its "reset votes on push" policy is how a +repository makes a vote mean the head it was cast on - **so the overview's stale-review marker never +shows on Azure DevOps.** + +**No review decision -> counted votes.** `Decision(reviewers)`: any negative vote gives +`CHANGES_REQUESTED`; otherwise `APPROVED` when something voted and every *required* reviewer is at +5 or better; otherwise `null`. It never returns `"REVIEW_REQUIRED"`, so `MergeState.Summary`'s "no +approving review yet" branch is unreachable on this host. + +**Policies -> checks and merge state.** Only evaluations whose `configuration.type.displayName == +"Build"` are checks; the rest (reviewer counts, linked work items, resolved comments) are *why a +merge is blocked*. `Bucket(status)`: `rejected`/`broken` -> `fail`; `queued`/`running` -> `pending`; +`notApplicable` -> `skipping` (a policy that does not apply is not a check saying no); else `pass`. + +`GetMergeStateAsync` is the densest translation in the file: + +- `mergeStatus`: `succeeded` -> `MERGEABLE`, `conflicts` -> `CONFLICTING`, else `UNKNOWN`. +- `mergeStateStatus` is synthesized: `isDraft` -> `DRAFT`; else any **blocking** policy unsettled -> + `BLOCKED`; else any **build** policy unsettled -> `UNSTABLE`; else `CLEAN`. The blocking/optional + split is exactly GitHub's `BLOCKED` vs `UNSTABLE` distinction. +- The checks are re-serialized into a **fake GitHub rollup** so `CheckRollup` folds both hosts + identically. +- There is **no `BEHIND` and no `HAS_HOOKS`** on this host, so those branches are dead here. + +**Completion.** `az repos pr update` knows only `--squash`, so `MergePrAsync` PATCHes instead, naming +`lastMergeSourceCommit` - Azure DevOps refuses a completion that names a commit the branch has moved +past, which is the guard a merge wants. + +**Threads -> comments.** `GetReviewCommentsAsync` GETs `git/pullRequestThreads` and: + +- skips any thread with no `threadContext.filePath` - that is the PR's own conversation, which this + review does not show, as it does not show GitHub's issue comments; +- `rightFileStart` present -> `Side = "RIGHT"`, else `"LEFT"`; +- keeps only `commentType == "text"` and not `isDeleted`; +- sets `OriginalLine: line` and `DiffHunk: null` - **Azure DevOps tracks a thread's line across + iterations itself**, so the line is always current. This is why the `CommentAnchor` fallback path + is effectively GitHub-only. + +**Comment ids are packed.** Azure DevOps numbers a thread's comments from 1 again in every thread, +and `PostedComment.Id` is one `long`: + +```csharp +public static long PackId(int threadId, int commentId) => threadId * 1_000_000L + commentId; +public static (int Thread, int Comment) SplitId(long packed) + => ((int)(packed / 1_000_000L), (int)(packed % 1_000_000L)); +``` + +Pinned by `AzureDevOpsIdTests.cs`. The ceiling is 999 999 comments per thread; nothing checks it. + +**Submitting a review** is a sequence, because there is no review object: attribute, post each line +comment as its own thread (counting), post the body as a thread with no `threadContext`, and only +then cast the vote. Comments first so a failure never leaves a vote standing with no reasons written +down; on failure it logs how many were posted so the reader knows what to look for on the site. + +`REQUEST_CHANGES` maps to **`wait-for-author` (-5), not `reject` (-10)** - deliberately: "reject" +blocks completion outright, where GitHub's request-for-changes is a reviewer asking. + +## What is unsupported on Azure DevOps + +| Not supported | How it surfaces | +| --- | --- | +| **Pull requests from forks** | `RefusedException("Pull requests from forks are not supported on Azure DevOps yet.")` | +| **Server-side branch update** | `RefusedException("Azure DevOps has no server-side update-branch; rebase the branch locally and push.")` | +| **Line totals in the PR list** | `Additions`/`Deletions`/`ChangedFiles` left at `0`; both would be a call per row | +| **Check state in the PR list** | `StatusCheckRollup: null` -> `ChecksBucket == "none"` -> the dots hide | +| **"Last updated"** | the list carries no last-touched date; `UpdatedAt` is `creationDate`, so "most recently updated" really means "most recently opened" | +| **Stale-review marker** | `CommitId: null` - the marker never appears | +| **A ```suggestion``` block** | nothing host-specific; it posts as a plain code block, which Azure DevOps renders and nobody can apply | +| **The merge-queue drainer** | `HasMergeQueueWorkflowAsync` -> `false`; the queue is drained by whoever has the window open | + +## PrCache - reading a pull request without the host + +```csharp +public sealed record PrSnapshot(PrDetail Detail, string HeadSha, string BaseSha, DateTimeOffset TakenAt, + IReadOnlyList? Comments = null, IReadOnlyList? Checks = null); +``` + +Only what the host alone knows. **The change itself is never in here**: its commits are in the object +database from the fetch that first opened the review, and the diff is read from them. + +Layout: `$HOME/.cache/stampeded/prs/_pr.json`, written indented. The `repoKey` +is `Path.GetFileName(RepoPath)` - the **folder name** - so two clones of different repositories in +identically-named folders share cache files. + +`Load` returns `null` on `IOException` or `JsonException` and logs it - "an unreadable cache is a +cache miss, which is a working state". `Save` swallows `IOException` with a log line. **Neither is +ever fatal**: deleting the whole directory costs a reader nothing but the ability to open a review +offline. + +### The offline path + +1. Try `GetPrAsync` -> `FetchPrHeadAsync` -> `FetchBranchAsync` -> `GetMergeBaseAsync`. +2. On `ToolFailedException`, `OpenableSnapshotAsync` loads the snapshot **and rev-parses both its + SHAs**. A snapshot whose head has been garbage-collected describes a review that cannot be built, + so the original failure is reported instead - that is the honest answer. +3. On success: `Offline = true`, `OfflineSince = cached.TakenAt`. +4. The snapshot's checks go through `SetChecks` so the overview stops waiting for an answer that is + not coming. +5. Offline, `LoadIssueUrlPrefixAsync` and `LoadReviewersAsync` are **not** called. +6. `KeepSnapshot` writes back **only when not offline**, so an offline session never overwrites a + good snapshot with a degraded one. +7. An offline review refuses a verdict outright, naming the snapshot's age. + +## CommentAnchor + +```csharp +public sealed record CommentAnchor(string Path, bool OldSide, int Line, string LineText, + IReadOnlyList ContextBefore, IReadOnlyList ContextAfter); +const int FuzzRange = 20; +``` + +A position that survives a force-push: the line's own text plus a small window, re-attached **by +content** rather than by number. + +- **Drafts**: `Create(path, oldSide, line, fileLines, context = 2)` - two lines either side, taken + from the blob on screen when the draft is written. +- **Posted comments**: `FromDiffHunk(path, oldSide, originalLine, diffHunk)`. GitHub drops a + comment's line number once the diff has moved on but keeps the excerpt it was written against, and + **that excerpt ends at the commented line**. So: filter the hunk to the side the comment is on, + strip the marker column, and the last surviving line *is* the commented line. `ContextAfter` is + therefore always empty for a posted comment. + +`Reattach(fileLines)` returns a 1-based line or `null` (meaning **Outdated**): + +1. Collect every line whose text equals `LineText`. None -> `null`. +2. Score each candidate by surviving context, take the best (ties broken by nearness). **Exact + stage:** a full-window score returns immediately. +3. **Fuzzy stage:** otherwise the nearest text match within `FuzzRange` (20) lines of the original - + text matches farther than that, once their context is gone, are not the same location. + +`Approximate(fileLines)` is the best-effort answer when `Reattach` failed - **never null**. It slides +a ghost position over the file scoring only the surviving **non-blank** context lines (blank lines +match everywhere and would win by accident). + +**Gate at submission time:** a draft is only posted when it has a current line **and** that line is +in `ChangedLines.CommentableOld/New` **and** the file is not generated - a generated file has no +counterpart in the pull request, so the host would reject the whole review over it. + +## ReviewStateStore + +On disk: `$LOCALAPPDATA/stampeded/reviews/.json` (on Linux +`~/.local/share/stampeded/reviews`), written indented, **to `.tmp` then `File.Move(..., +overwrite: true)`** - the file is rewritten in full on every toggled flag, and a write interrupted +halfway would read back as a review nobody ever started, drafts and all. + +| Scope | File name | +| --- | --- | +| Pull request | `{repoKey}_pr{N}.json` | +| One commit | `{repoKey}_commit_{sha[..9]}.json` | +| Uncommitted work | `{repoKey}_worktree_{tipSha[..9]}.json` | +| Local `base..head` | `{repoKey}_local_{rangeKey}.json` | + +A commit scope is keyed **by the commit**, because having read a file in one commit says nothing +about the next commit's change to it. The working-tree scope is keyed by the commit it sits on +rather than by content: the checkout changes with every save, and a file read there has been read +*for the tip it was written against*. + +```csharp +public sealed record StoredComment(Guid Id, CommentAnchor Anchor, string Body, DateTimeOffset CreatedAt, + long? InReplyTo = null); +``` + +`InReplyTo` is the REST id of the posted comment being answered - that makes it a reply into a +thread rather than a new comment on the same line, and replies survive their line moving. + +### The head-move protocol - the subtle part + +`OpenFile` has four branches: + +1. **No file** -> write a fresh one **immediately**, before anything has been read. What the next + pass needs from this one is the head it was opened at. +2. **`HeadSha` differs** - the push happened: + - `Superseded = (oldHead, copy of Viewed)` - a one-shot signal, non-null only for the open that + discovered the move. + - `Viewed` is cleared; `PreviousHead`/`PreviousBase` take the outgoing values. + - `PreviousMarkedHead = MarkedHead ?? PreviousMarkedHead`, likewise for `Submitted` - **a pass + that neither ticked a file off nor submitted anything leaves the older marks standing**, which + is what keeps merely *opening* a review from counting as having read it. + - `Marked*`/`Submitted*` are reset, and the file is saved **now**: reopening before the next + `SetViewed` must not re-read the old head from disk and report a second supersede. +3. **Same head, different base** - the target branch moved under the same work; record the new base. +4. Same head and base -> nothing. + +`SetViewed`: ticking a file off is the act that makes this head a pass, so it stamps +`MarkedHead`/`MarkedBase`. **Unticking does not** - it says the reader wants to look again, not that +they never did. `RecordReviewSubmitted` stamps `SubmittedHead`/`SubmittedBase`. + +A corrupt state file is logged and treated as a fresh start - but said out loud, "because everything +the reader recorded here is about to look like a first pass that never happened". + +`ReviewStateFile.Depth` is **dead**: it carried a per-file review plan that no longer exists, and +the field stays only so older state files parse. + +### ReReview + +```csharp +public static IReadOnlyList CarryOverViewed( + IReadOnlyDictionary previousViewed, IReadOnlySet touchedSinceLastPass) + => previousViewed.Where(kv => kv.Value && !touchedSinceLastPass.Contains(kv.Key)).Select(kv => kv.Key).ToList(); +``` + +Re-review is a different process, not a repeat: prior conclusions stay valid except where the new +push touched them. + +## The rest of Review/ + +- **`ReviewVerdicts.Latest(reviews)`** - each reviewer's most recent review **that took a + position**. A `COMMENT` takes none and leaves an earlier verdict in force; `DISMISSED` sets the + entry to `null`. Ordered by name, because a list that reorders itself as people revisit a review + reads as new activity. Note it iterates `OrderBy(r => r.SubmittedAt ?? MinValue)` - on Azure + DevOps every `SubmittedAt` is null, harmless only because that host emits one entry per person. +- **`TriageEstimate`** prices a review by what the lines *are*: implementation 5 lines/min, tests 15, + generated 50, a dependency file a flat 2 minutes, 75 minutes to a sitting. `Compute` overrides the + filename guess with `file.IsGenerated` - output collected from a build is generated whatever it is + called; the filename hints exist only to guess at generated code that was *committed*. +- **`TestPaths.IsTestPath`** is `path.Contains("test", OrdinalIgnoreCase)`. Deliberately crude, and + it will call `src/Contest/...` a test. +- **`FolderOrder.ByFolder`** regroups a flat ordering by directory so it can be shown as a tree. The + changed-file list puts a directory where its first file would have been; an order that leaves a + directory and comes back to it ("tests first", "touched since the last pass first", "generated + output last") is an order that tree cannot show, and the keyboard keys that walk the list would + then walk it in an order the reader is not looking at. +- **`FixtureAssemblies`** is ILSpy-specific: sources under `ICSharpCode.Decompiler.Tests/TestCases/`, + with variant sources (`Name.opt.roslyn.il`) collapsed onto the base fixture name by truncating at + the first dot. +- **`IssueLinks.Autolink`** rewrites `#1234` as a markdown link. The regex puts the **skip + alternatives first** so a `#123` inside fenced code, an inline span, an existing link, an autolink + or a bare URL (whose fragment can look exactly like a reference) is consumed as part of that thing + and never rewritten. +- **`ReviewAttribution`** appends the mark **once**, to the first thing a reader will meet: the first + line comment if there is one (the file view, the thread and the mail notification all show those, + and none of them shows the summary the comments were batched into), otherwise the summary. An + approval with nothing written at all is left alone - the mark would be the entire review. + +## MergeQueueService + +### The idea + +Clients never talk to each other and have no server. The queue lives on the one thing they all +reach: **a ref on the remote**, `refs/stampeded/merge-queue` - outside `refs/heads` and `refs/tags` +on purpose, so no branch or tag list shows it, no clone fetches it by default, and no branch +protection rule applies to it. + +The ref points at a chain of commits with an **empty tree**, each carrying the whole queue JSON as +its message and each parented on the state it replaces. Writing is a plain `git push`, which git +refuses unless it fast-forwards - **that refusal is the compare-and-swap.** Because each state names +its predecessor, `git log` on the ref is the queue's own history. + +### The document + +```csharp +public sealed record MergeQueueEntry(int Pr, string Title, string HeadSha, string Method, string By, + DateTimeOffset At, bool DeleteBranch = false); +public sealed record MergeQueueLock(string Holder, string Client, DateTimeOffset At, int Pr); +public sealed record MergeQueueDocument(int Version, IReadOnlyList Entries, MergeQueueLock? Lock); +``` + +`HeadSha` is the revision that was cleared: a push after it makes the entry stale. `Method` and +`DeleteBranch` are the *enqueuer's* choice, carried so whichever client (or the drainer workflow) +drains the queue merges the way they meant. `Holder` is who to name in the UI; `Client` is a +per-process GUID prefix so **two Stampeded windows on one machine can still tell whose lock this +is**. + +### Read and write + +`ReadAsync`: `git ls-remote origin ` - an empty listing is an **empty queue, not an error**. +Then `git fetch origin +:` (mirrored, not merged) and `git cat-file commit `. **A +document that does not parse throws** after logging - "a queue nobody can read is worse than an empty +one only if it is silently emptied." + +`UpdateAsync(edit)` is the only writer. `edit` receives the current document and returns +`(replacement, subject)` or `null`. **It may be called more than once** - a push another client won +is not an error, it is the signal to re-apply. Up to `WriteAttempts = 5`. + +``` +git mktree # stdin empty and closed -> names AND stores the empty tree +git commit-tree [-p ] -m "\n\n" +git push origin :refs/stampeded/merge-queue # no --force, no lease +``` + +**No force and no lease is the whole design**: a queue state that does not descend from the one on +the remote is exactly what must not be published, and git already refuses it. + +`RaceLostAsync(expected)` decides whether a rejection was somebody else's write landing first by +**re-reading the ref** and comparing SHAs - reading git's refusal would mean matching on its wording, +and a push refused for want of access leaves the ref where we found it. + +### One turn - `DriveOnceAsync` + +``` +read; empty -> "The queue is empty." +someone else's unexpired lock -> "#N is being merged by ." +for each entry, in order: + GetMergeStateAsync + ToolFailedException -> Pass(reason); continue # one entry's problem, not the queue's + state.State is MERGED/CLOSED -> RemoveAsync; return "was already ; dropped it." + HeadRefOid != entry.HeadSha -> Pass("pushed to since it was queued; queue it again") + !state.CanMerge -> Pass(state.Summary) + !TryAcquireAsync -> return "Another client took #N first." + MergePrAsync -> drop the entry AND clear our lock in one write -> return "Merged #N (method)." + ToolFailedException -> ReleaseAsync; Pass(message); return "Merging #N failed: ..." +-> "Nothing in the queue can be merged right now (K waiting)." +``` + +Two decisions worth keeping: + +- **Entries that cannot be merged are passed over, not left to block the queue.** A failing check at + the front is one person's problem, not everybody's - and the reason is reported so the pane can + say why. +- **An entry is dropped when its pull request is seen merged or closed, not when *this client* + merged it.** That is what makes a client dying mid-merge harmless: its lock runs out, the next + driver finds the pull request already merged, and drops the entry. + +`BreakLockAsync` is safe because the lock was never what makes a merge exclusive - the host is; the +worst a broken lock can do is let a second client attempt a merge the host then refuses. + +`WhyGoneAsync(pr)` reads `git log --format=%s -100 ` and returns the first subject that mentions +`#pr` and is not an `enqueue `/`lock ` line. `null` when the history says nothing - an entry can also +vanish because somebody rewrote the ref, and inventing a reason would be worse. `Mentions` checks the +character after the token so `#14` never answers for `#142`. + +`LeaseTime` is 5 minutes, marked `ponytail:`: a fixed lease against unsynchronised clocks, sound only +because a wrong steal is harmless here. A queue that waited for CI would hold the lock for as long as +CI takes and would need a real heartbeat. + +**`refs/stampeded/*` triggers no `on: push` workflow** - GitHub Actions accepts only branches and +tags there. That is why the queue cannot be its own event and `repository_dispatch` exists. + +## Gotchas + +1. **GitHub's words are the model's words.** Adding a GitHub-shaped field to `MergeState` or + `PrSummary` obliges `AzureDevOpsService` to synthesize it. +2. **`MergeState.Host` defaults to `"GitHub"`.** Not setting it makes every explanation lie. +3. **`gh pr checks` exits non-zero on failing or pending checks.** Anything routed through + `ExternalTool.RunAsync` instead of the hand-rolled call would throw on a normal red build. +4. **A 404 from `gh api .../issues/N` is an answer.** `#141414` is a colour. +5. **The title memos cache the in-flight `Task`, not the result**, in a plain `Dictionary` - **UI + thread only**. Calling from a background thread is a data race. +6. **The GraphQL walker uses hard `GetProperty`.** A schema change throws `KeyNotFoundException`, + which nothing in the comment-loading path catches. +7. **`reviewThreads(first: 100)` / `comments(first: 50)` are unpaged.** +8. **`RefusedException` is not caught uniformly.** The PR list, the offline fallback and the queue + driver all catch only `ToolFailedException` - see the refactor notes. +9. **Azure DevOps comment ids are packed decimal**, 6 digits for the comment. `PackId`/`SplitId` must + stay each other's inverse. +10. **`PrCache` and `ReviewStateStore` key on the repository *folder name***, not the remote URL. +11. **The two stores use different base directories and different sanitizers.** +12. **`Superseded` is one-shot; `PreviousHead` persists.** +13. **Ticking a file off is what makes a head a pass.** Opening a review does not. +14. **The state file is rewritten in full on every flag toggle.** +15. **The merge-queue ref is pushed without `--force`, deliberately.** Adding a force or a lease + anywhere in `PublishAsync` destroys the only mutual exclusion in the design. +16. **`MergeQueueEntry.Method` is a `gh` flag name** even on Azure DevOps, where `MergePrAsync` + translates `merge` -> `noFastForward`. +17. **Azure DevOps `GetMergeMethodsAsync` reads the *default* branch's policy**, not the PR's target. +18. **`InvokeAsync` writes the request body to `Path.GetTempPath()`** - review prose transits the + system temp directory in plaintext. Deleted in a `finally`, but not on a hard kill. diff --git a/docs/review-session.md b/docs/review-session.md new file mode 100644 index 0000000..28355cb --- /dev/null +++ b/docs/review-session.md @@ -0,0 +1,243 @@ +# The review session (orchestration layer) + +`src/Stampeded/ReviewWorkspace.cs`, `ReviewScopes.cs`, `ReviewComments.cs`, `MainViewModel.cs`, +`Program.cs`. This is the layer that turns "a PR number" into "a window full of documents", and +it is where most cross-cutting behaviour of the app lives. + +## Startup + +`Program.Main` (`src/Stampeded/Program.cs:24`) does four things before any window exists, in this +order, and the order matters: + +1. `OPENSSL_ENABLE_SHA1_SIGNATURES=1` is set **process-wide**, not per invocation. Child processes + inherit it: `git`, `gh`, `dotnet`, and - the reason it cannot be per invocation - the MSBuild + build hosts that `MSBuildWorkspace` spawns for itself. +2. `MSBuildLocator.RegisterDefaults()`. Must run before any Roslyn workspace assembly loads, or + MSBuild resolves the wrong assemblies (see the `Microsoft.Build.Framework` + `ExcludeAssets="runtime"` note in `Directory.Build.props`, guarding MSBL001). +3. Argument parsing: `--pr N` sets `Program.AutoOpenPr`; the first non-option argument that is not + the value of `--pr` becomes `Program.RepoPath`. (The exclusion is a real bug fix: `--pr 4013 + /path/to/repo` used to take `4013` as the path.) +4. `Program.Host = PullRequestHosts.ForAsync(RepoPath)` - blocked on synchronously, deliberately: + there is no dispatcher to deadlock against yet and the first window is built from the answer. + +Then Avalonia starts. `BuildAvaloniaApp` maps Windows font family names (`Consolas`, `Segoe UI`, ...) +onto fontconfig aliases on non-Windows - third-party styles name those fonts outright and an +unresolvable family aborts the layout pass. On Windows the mapping must NOT be applied: there the +named fonts are real and the aliases are the unresolvable ones, and with them in place no window +ever appears. + +`Program.RepoPath` and `Program.Host` are mutable statics: "Open Repository" changes both at +runtime. They are a property of the repository, so they sit together. + +## MainViewModel + +`MainViewModel` (`src/Stampeded/MainViewModel.cs`) is the window's view model, and it is small on +purpose - it owns no review logic, only what the menu bar needs to grey items correctly. + +Construction order (`:133`) is load-bearing: + +- `ZoomState.Set(Zoom)` hands the remembered zoom to the popup transform (nothing else sets it). +- `RecentRepos.Record(Program.RepoPath)` runs **before** `Recent` is snapshotted, because both + views of the list (this menu and the start page) snapshot it during this constructor. +- `new ReviewWorkspace(Program.RepoPath, Program.Host)` -> `App.Workspace` (a static, which is how + panes and views reach the session). +- `new StampededDockFactory(workspace)` -> `CreateLayout()` / `InitLayout()`, then + `workspace.Factory` and `workspace.Documents` are wired back. The workspace cannot build its own + layout; it is handed one. +- `workspace.OpenStart()` puts the start page in front. + +`RefreshReviewState()` is the single place that reads review state into the menu's observable +flags, hooked to `ReviewChanged` and `Scopes.Changed`. The state of the *tab in front* is +deliberately absent: it has no event, so the menu reads it when it opens. + +## ReviewWorkspace: what it is + +One instance per repository, created by `MainViewModel` and abandoned (via `Shutdown()`) when the +app switches repositories. It owns: + +| Field | What it is | +| --- | --- | +| `RepoPath`, `Git`, `Blobs`, `Worktrees` | git access for this clone | +| `Host` / `HostName` | the pull-request host, decided once from origin's URL | +| `MergeQueue` | the shared merge queue (note: constructs a **second** `GitService`) | +| `Store` | `ReviewStateStore` - viewed flags, drafts, pass heads | +| `Busy` | the busy tracker the status bar shows | +| `Comments` / `Scopes` | lazily created collaborators, each handed `this` | +| `Factory` / `Documents` | set by `MainViewModel` once the dock layout exists | + +The review itself is the quadruple `BaseSha` / `HeadSha` / `Files` / `changed`, and those four +must move together - `SetScopeContent` (`:1327`) exists precisely to make that atomic, and is +`internal` so only `ReviewScopes` can call it. + +**`BaseSha` is not always a commit.** In the since-last-pass scope it is a *tree* built for that +scope. Reading a blob out of it works; blame, a worktree, `rev-parse ^` do not - git answers +"Non commit"/"invalid reference". Anything needing history or a checkout must first ask +`Scopes.InSinceLastPass`. `EnsureBaseWorktreeAsync` (`:655`) is the model for this: it refuses +with a sentence naming the reason rather than letting git refuse four callers deep. + +## Opening a review + +Two entry points, and they are near-parallel: + +- `OpenPrAsync(int number)` (`:452`) +- `OpenLocalRangeAsync(baseRef, headRef, prNumber = null)` (`:369`) + +Both follow the same nine-step shape: + +1. Cancel the previous session (`sessionCts`), take a new token. Everything background in a review + is tied to this token. +2. Resolve `headSha` / `baseSha`. For a PR: `Host.GetPrAsync`, `Git.FetchPrHeadAsync` (via + `Host.PrHeadRefspecAsync`), `Git.FetchBranchAsync(detail.BaseRefName)`, then + `GetMergeBaseAsync`. The base is **the PR's own target**, not the repository default branch - + a branch targeting a release branch is not a diff against master. +3. `Git.DiffAsync(base, head)`, or `DiffWorkingTreeAsync` when a dirty checkout holds the branch + (`FindDirtyCheckoutAsync`, `:1249`). `UncommittedFileCount` is the difference. +4. Reset session state: `Scopes.Reset()`, `Reviewers = null`, `Offline`, `snapshot`, `PrHeadSha`. +5. Set the review quadruple, fire `ReviewReset`. +6. `Store.Open` / `Store.OpenLocal` - keyed by repo name plus PR number or the *range text the + user typed* (`ReviewScopes.LocalRangeKey` must key it identically on scope exit, or the state + file is orphaned). +7. `ApplyReReviewCarryOverAsync`, `PinReviewHeadsAsync`, `ComputeChurnAsync`. +8. `history.Clear()`, close documents, fire `ReviewChanged`, `OpenOverview()`, `CloseStartPage()`. +9. Fire off the background loads, each `.HandleExceptions()`: scope-then-semantics, generated + sources, draft reattachment, posted comments, reviewers, issue-URL prefix. + +The overview opens rather than every file: files open one tab at a time as the Explorer list is +walked. + +### Offline + +`OpenPrAsync` catches `ToolFailedException` around the host calls and falls back to +`OpenableSnapshotAsync` (`:538`) - the `PrCache` snapshot, but **only if both its SHAs still +rev-parse**. A snapshot whose head was garbage-collected describes a review that cannot be built, +and rethrowing the original failure is then the honest answer. Offline: `Offline = true`, +`OfflineSince` set, cached checks pushed through `SetChecks` so the overview stops waiting, and +issue-prefix/reviewer loads are skipped entirely rather than asked and failed. + +`KeepSnapshot` writes through to `PrCache` only while online, and is updated as parts arrive +(`SetChecks`, `KeepComments`). + +### Semantics load order + +`LoadScopeThenSemanticsAsync` (`:568`): **scope first, then workspaces**. The scope decides what +the review *is* (seconds); the workspaces take as long as a solution load. Restoring the scope +after them would rearrange a review already being read. Because entering a scope overlays text +onto the workspaces and this load replaces them, the overlay is re-applied at the end. + +`LoadSemanticsAsync` (`:576`) disposes the old providers, creates the head worktree, loads C# +(in process, or over LSP when `STAMPEDED_SEMANTICS=lsp`), starts other-language servers, computes +the change map, prunes the worktree cache to `KeptWorktrees = 6`. + +The base-side C# workspace is **not** a second solution load: `baseSemantics.LoadFrom(headSemantics, +replaced, removed, added)` reuses the head compilation with the review's files reading as they did +before, from the object database (`BaseSideTextsAsync`, `:624`). A second checkout+restore+design- +time build would spend minutes arriving at the same answers. + +Python is different: a language server holds one text per file, so the base side must be a second +process on a checkout of the base revision (`TryStartPythonAsync`, `:2106`). + +### Provider lookup + +- `SemanticsFor(bool oldSide)` - the primary (C#) pair. +- `SemanticsFor(bool oldSide, string relPath)` (`:1948`) - by file extension, through the + `languages` list, falling back to the C# pair only for `.cs`/`.csx`. Everything else gets + `null`, and `NoProviderMessage` (`:1977`) says which nothing it is: "nothing reads .json files" + vs "nothing reads the base side of .py files in this review". Without that, a `.md` handed to + Roslyn produces the same silence as "still loading". + +## Scopes + +`ReviewScopes` owns *which part* of the review is on screen; the workspace owns what the review +is. Three states, one way out ("Whole change"): + +- **Whole change** - `Commit is null && !InSinceLastPass`. +- **Commit by commit** - `Commit`/`Series`/`CommitIndex`. The series is oldest-first (the order it + was written), with `CommitInfo.WorkingTree` appended when the checkout is dirty. Each step + re-keys the state store (`Store.OpenCommitScope`) so viewed flags are per commit. The parent + comes from `commit.FirstParent`, carried by the log, rather than a `rev-parse` per step. +- **Since last pass** - diffs against `SinceLastPassBase`, a **tree** produced by + `Git.ReplayTreeAsync(base, previousHead, previousBase)`: everything the reader already read, + replayed onto the current base. A tree and not a commit because after a rebase no commit's diff + to the head is the author's own edits. Unlike the commit scope, it shares the review's state + file: its head *is* the review's head, so a file read here has genuinely been read. + +Refusals are properties, not silent disabled controls: `CommitScopeRefusal` / `SinceLastPassRefusal` +return the same sentence the enter method would post, so the tooltip and the outcome cannot drift. + +`PassBaselineKind` picks what "last pass" means: `MarkedViewed` (default - opening a review is not +reading it), `SubmittedReview`, `Opened`. `ReviewWorkspace.ReadPassBaselinesAsync` (`:989`) only +offers those whose commit is still in the repository. + +`ScopeKey` (`"commit:"`, `"pass"`, or null) is recorded into every navigation history entry, +and `RestoreScopeAsync` puts the scope back before navigating - a line recorded in one commit's +diff is not the same line in the whole change. + +`fullRange` remembers the range to return to; `Reset()` clears everything scope-owned because a +freshly opened review is the whole change by definition. + +## Comments + +`ReviewComments` holds drafts (local, in the state store) and posted comments (from the host), +plus the placement of both in the code as it now stands. + +Placement is a three-step fallback, used identically for drafts (`ReattachDraftsAsync`, `:148`) +and posted comments (`LocateAsync`, `:261`): + +1. `CommentAnchor.Reattach(blobLines)` - find the line by content. Exact. +2. `MemberRelocation.Locate(oldText, oldLine, newText, lineText)` - read the member the comment was + written in out of the revision it was written against, and find that member now. Reported as + `MovedTo` ("moved with `Foo.Bar`", or "the exact line is gone; placed in `Foo.Bar`"). + C# only, and only when that old revision is still in the clone. +3. `CommentAnchor.Approximate(blobLines)` - best guess from surviving context, flagged + `IsApproximate`. + +Blob reads are memoised per `(rev, path)` in `blobs` for the whole pass; a ten-comment thread on +one file was ten `git show` calls. + +`SubmitCheckedAsync` (`:386`) is the single gate every submission goes through - both the Comments +pane and the review document - so a verdict cannot be refused in one and slip through the other. +It refuses, in order: offline; `LocalHead` (the branch is ahead of what the host has, so line +comments would land on lines the host does not have - replies are exempt, they name a thread); +`APPROVE` with an unread series; `APPROVE` blocked by the guide's `ApprovalGate`; +`APPROVE`/`REQUEST_CHANGES` on your own PR where the host refuses it (`Host.AcceptsOwnApproval`). +Then it **leaves the scope** rather than refusing, because drafts are matched against the files in +scope, and reports what it did in the result line. + +`SubmitAsync` (`:459`) splits drafts into line comments and replies. A reply goes as its own +request (it survives its line moving); a line comment is dropped ("kept local") when its line is +not in `Changed.CommentableNew/Old`, or when the file is generated - the host would reject the +whole review over it. An all-replies pass submits no review at all, and the first reply carries +the attribution mark the review body would have (`ReviewAttribution.AttributedReply`). + +## Navigation history + +`NavigationHistory`; `NavEntry` is `(DockableId, BlobLine, OldSide, Scope)`. + +`RecordCurrentPosition()` (`:2493`) **rewrites** the current entry rather than pushing when it is +already the current document in the same scope: an entry recorded at open says line 1, and coming +back to line 1 of a half-read file is not coming back. + +`NavigateToEntryAsync` (`:2526`) restores the scope first, then handles the id prefixes. The +prefix vocabulary is the app's document-identity scheme and appears in several places: + +| Prefix | Document | +| --- | --- | +| `diff:` | a file of the review (unified **or** side-by-side - same id, one tab) | +| `src:` / `srcbase:` | a file outside the change, head / base side | +| `source:` | a definition outside the tree (a package), read-only | +| `decomp:` | a decompiled type; only revisited while still open | +| `hist::` | one commit's change to one file | +| `show:` / `interdiff:` | a whole patch as text | +| `overview`, `start`, `review` | the three singleton documents | + +## Events + +`ReviewWorkspace` exposes 18 events. Two are easy to confuse and the distinction is documented in +the source: + +- `ReviewChanged` - fires whenever anything about the current review changed, **including while + one review is being read** (generated sources arriving, the issue prefix arriving). +- `ReviewReset` - a *different* review is in front, or none. This is the signal for a pane to + throw away derived state (references, call graph, file history). A pane that emptied itself on + `ReviewChanged` would go blank under the reader's hands. diff --git a/docs/semantics.md b/docs/semantics.md new file mode 100644 index 0000000..1490743 --- /dev/null +++ b/docs/semantics.md @@ -0,0 +1,896 @@ +# Semantics, language servers and testing + +`src/Stampeded.Core/{Semantics,Roslyn,Lsp,Decompilation,Testing}` and `src/Stampeded.RoslynLsp`. + +## The shape of the layer + +``` +UI (ReviewWorkspace, panes, diff views) + | + v +ISemanticProvider (Stampeded.Core/Semantics/ISemanticProvider.cs) + | | + | +-- LspSemanticProvider -> LspConnection -> child process + | (pyright / Stampeded.RoslynLsp) + +-- RoslynWorkspaceService (in-process MSBuild/Adhoc workspace) +``` + +Two rules decide everything else: + +1. **A symbol is a file plus a position** (`SymbolRef`), never a compiler object. That is the only + symbol identity a language server can accept back. +2. **One provider instance serves one side (head or base) of one review for one language.** The + head/base pairing is done above the interface, in `ReviewWorkspace` (`ReviewWorkspace.cs:1939`). + +`ReviewWorkspace.SemanticsFor(bool oldSide, string relPath)` (`:1948`) is the dispatch: extension -> +language -> head/base provider; `.cs`/`.csx` fall back to the primary C# pair; everything else gets +`null` and the caller prints "nothing here reads .json files" (`NoProviderMessage`, `:1977`). + +## `ISemanticProvider` - the contract + +`Semantics/ISemanticProvider.cs:13`, `IDisposable`. No default implementations. + +### State + +| Member | Semantics | +| --- | --- | +| `SemanticState State` (`:18`) | `NotLoaded / Restoring / Loading / Ready / SyntaxOnly / Failed`. Callers gate compilation-dependent commands on `Ready or SyntaxOnly` (`ReviewWorkspace.IsReady`, `:2152`). | +| `string StateDetail` (`:21`) | One status-bar line: solution name, failure message, or progress. | +| `string LoadLog` (`:24`) | Whole load transcript for the Log pane. `LspSemanticProvider` returns `""` (`:83`) - the server's transcript arrives via stderr into `CliLog` instead. | +| `event Action? StateChanged` (`:26`) | Every state transition. Raised from whatever thread the load runs on; UI subscribers marshal themselves. | + +### Path mapping + +```csharp +string? ToRelativePath(string absolutePath); // null when outside the served tree +string ToAbsolutePath(string repoRelativePath); +``` + +Repo-relative paths are always forward-slashed (git's spelling); absolute paths are the platform's. + +### Text overlay + +```csharp +void SetTextOverlay(IReadOnlyDictionary textByRelativePath); +void ClearTextOverlay(); +Task GetDocumentTextAsync(string relPath, CancellationToken ct); +``` + +The overlay is how "review one commit of a file that later commits change" works: positions are +offsets into a *specific* text, so the whole stack must agree which one. `GetDocumentTextAsync` +exists so a view can check the provider's text is the text on screen before applying positions. + +### Positions and tokens + +```csharp +Task GetPositionAsync(string relPath, int line, int column, CancellationToken ct); +Task> GetSemanticTokensAsync(string relPath, CancellationToken ct); +Task> GetSemanticTokensForTextAsync(string relPath, string text, CancellationToken ct); +``` + +`GetSemanticTokensForTextAsync` classifies text the provider does not hold (a historical revision). +Roslyn forks the document (`:443`); LSP *declines* unless the text equals what the server holds (`:369`). + +### Queries at a position + +```csharp +Task GetQuickInfoAsync(string relPath, int position, CancellationToken ct); // one line +Task GetHoverTextAsync(string relPath, int position, CancellationToken ct); // full, with docs +Task GetSymbolAtAsync(string relPath, int position, CancellationToken ct); +Task GetSymbolOnLineAsync(string relPath, int line, int preferredColumn, CancellationToken ct); +Task GetEnclosingMemberAsync(string relPath, int line, CancellationToken ct); +``` + +`GetSymbolOnLineAsync` falls back to any identifier on the line - a caret usually sits in the +indentation. `GetEnclosingMemberAsync` answers "which member is this line in", which on a body line +is *not* the token under the caret. + +### Queries about a symbol + +```csharp +Task GetDefinitionAsync(SymbolRef, CancellationToken); +Task> FindReferencesAsync(SymbolRef, CancellationToken); +Task> FindOccurrencesInFileAsync(SymbolRef, string relPath, CancellationToken); +Task> GetCallsAsync(SymbolRef, CallDirection, CancellationToken); +Task> FindDeclarationsAsync(string pattern, int max, CancellationToken); +``` + +### Queries about a file + +```csharp +Task> MapLinesToMembersAsync(string relPath, IReadOnlyCollection lines, CancellationToken); +Task> ListMemberDisplaysAsync(string relPath, CancellationToken); +Task> GetOutlineAsync(string relPath, string sideText, CancellationToken); +Task> GetFoldRegionsAsync(string relPath, string sideText, CancellationToken); +``` + +The `sideText` parameter on the last two is load-bearing (`:99`): one side of a diff is another +revision, and an outline drawn at the wrong lines is worse than none. A provider that only knows the +revision it holds must return `[]` when `sideText` differs. Roslyn sidesteps the problem by parsing +`sideText` itself - those two are pure functions (`RoslynWorkspaceService.cs:1112`), and the views +exploit it: `Task.IsCompletedSuccessfully` lets them keep a synchronous path. + +### Threading and async expectations + +- Every method is cancellable; **no method has an internal timeout**. `LspConnection.RequestAsync` + waits forever unless the caller's token fires (`LspConnection.cs:207`). Most UI call sites pass + `CancellationToken.None` (`ReviewWorkspace.cs:2176, 2183, 2200, 2302, 2336, 2354, 2400`). A wedged + server therefore wedges those awaits permanently. **This is the layer's biggest hazard.** +- Implementations are not documented as thread-safe, and `LspSemanticProvider` is not. +- `LspSemanticProvider.Dispose` disposes the underlying connection (`:726`), so the head and base + providers sharing one connection (the Roslyn-LSP case) must not both dispose it - in practice + `LspConnection.disposed` guards the second call (`:376`). + +### `IDecompileTargets` (`:115`) + +```csharp +Task GetDecompileTargetAsync(SymbolRef symbol, CancellationToken ct); +``` + +Deliberately *not* on `ISemanticProvider`: only a provider with real metadata behind it can answer. +Callers test `sem is IDecompileTargets` (`ReviewWorkspace.cs:2263`). + +## `SemanticTypes.cs` - the vocabulary + +All records, all 1-based lines and columns. + +| Type | Line | Carries | +| --- | --- | --- | +| `SemanticState` | 4 | the enum above | +| `SymbolLocation(FilePath, Line, Column, Length)` | 16 | absolute path as the provider knows it | +| `ReferenceHit(FilePath, Line, Column, Length, LineText)` | 19 | `LineText`, so the references pane needs no second read | +| `SemanticToken(Line, Column, Length, Classification)` | 23 | `Classification` is a **Roslyn classification name** (`"class name"`, `"method name"`), whoever produced the token - that is what the editor's colour table keys on | +| `ChangedMember(Display, Kind, FirstLine)` | 26 | symbol-level change map | +| `DeclarationHit(Name, Container, Kind, RelPath, Line)` | 29 | go-to-symbol-by-name | +| `CallDirection` | 31 | `Callers` / `Callees` | +| `CallSite(FilePath, Line, Preview)` | 41 | one actual call, not a signature | +| `CallNode(Display, ContainingType, FilePath?, Line, Column, Sites)` | 48 | `CanExpand => FilePath is {Length:>0}`; a metadata-only member is a leaf | +| `SymbolRef(RelPath, Line, Column, Display, Name, IsType, ContainingType?)` | 70 | see below | +| `OutlineNode(Kind, Title, StartLine, EndLine, Children)` | 80 | structure tree | +| `MemberFoldRegion(StartLine, EndLine, HeaderEndLine)` | 87 | `HeaderEndLine` = last line of the declaration itself (the one carrying `{` or `=>`) | +| `DecompileTarget(AssemblyPath, ReflectionName, MetadataToken, TypeName)` | 91 | | + +### Why `SymbolRef` is a position + +Documented at `:59`. Nothing else survives leaving a compiler's memory. A language server names a +symbol by "the position that resolves to it" and nothing more, so that is what every provider can +accept back. + +The consequence to preserve when touching `RoslynWorkspaceService`: **the position stored in a +`SymbolRef` has to re-resolve to the same symbol.** `MakeRef` stores the query position (`:995`); +`DeclarationRef` stores the declaration's own name-token position (`:1013`); `GetSymbolOnLineAsync` +prefers `DeclarationRef` precisely because the position that found the symbol is not reported back +(`:1057`). + +`Display` uses `SymbolDisplayFormat.CSharpShortErrorMessageFormat` (e.g. `Foo.Bar(int)`) and is +compared against the change map (`ReviewWorkspace.IsChangedMember`, `:2373`) - **it must stay +stable**. `ContainingType` is null for a type itself and for members whose type is metadata-only. + +## RoslynWorkspaceService - in-process C# semantics + +`Roslyn/RoslynWorkspaceService.cs`, 1131 lines, implements `ISemanticProvider` and `IDecompileTargets`. + +### Fields and lifecycle + +```csharp +Workspace? workspace; // MSBuildWorkspace or AdhocWorkspace +bool ownsWorkspace; // false for a derived (base-side) view +Solution? solution; // current, possibly overlaid +Solution? loadedSolution; // what was loaded, overlay-free +Dictionary? documentsByPath; // absolute path -> id, OrdinalIgnoreCase +string worktreePath = ""; +``` + +One instance per review session per side; dispose and reload on PR switch, never patch +incrementally (`:13`). `Dispose` (`:1121`) disposes the workspace **only if `ownsWorkspace`** - a +derived base view shares the head's workspace, and disposing it would take the head down. + +### Head load - `LoadAsync(worktree, chosenSolution, ct)` (`:118`) + +1. `SolutionTarget.ForSemantics(worktree, chosenSolution)` picks the solution (`.sln`, `.slnx`, and + for a `.slnf` filter the solution the filter names - Roslyn opens a solution, not a filter). +2. `Restoring` -> `RestoreAsync(sln, cleanRetry: false)`. +3. On `ToolFailedException`: log, `Restoring "clean retry"`, `RestoreAsync(sln, cleanRetry: true)` + (`:134`). Causes named in the comment: a stale `packages.lock.json` on the PR branch, a broken + `obj/` from an interrupted restore in the cached worktree. +4. `Loading` -> `MSBuildWorkspace.Create()` + `OpenSolutionAsync`; every `msbuild.Diagnostics` entry + goes into `LoadLog`. +5. `DropUnresolvedAnalyzers(loaded)`. +6. If any project has any document: adopt, `IndexDocuments()`, `Ready`. Otherwise dispose the + MSBuild workspace and fall through. +7. `LoadSyntaxOnly(worktree, ct)`. +8. `OperationCanceledException` rethrows; **any other exception degrades to syntax-only** (`:168`), + and if even that throws, `Failed`. + +**`RestoreAsync`** (`:210`): + +``` +normal: dotnet restore -p:RestoreEnablePackagePruning=false +clean retry: DeleteBuildArtifacts(worktree) first, then + dotnet restore -p:RestoreEnablePackagePruning=false \ + --force --force-evaluate -p:RestoreLockedMode=false +``` + +Environment: `OPENSSL_ENABLE_SHA1_SIGNATURES=1` plus `ExternalTool.StripMsBuildLocatorVariables`, so +the child does not inherit this process's pinned MSBuild. Pruning is disabled because it would +rewrite a committed `packages.lock.json` (ILSpy carries full lock files). NuGet reports errors on +**stdout**, so both streams are captured and tail-truncated to 4000 chars; when both are empty, +`LogHostDiagnosticsAsync` (`:251`) dumps `PATH`, `DOTNET_ROOT`, `DOTNET_HOST_PATH`, +`MSBUILD_EXE_PATH`, `MSBuildSDKsPath`, `MSBuildExtensionsPath` and `dotnet --version`. + +**`DropUnresolvedAnalyzers`** (`:192`) - subtle and important. An analyzer whose file is missing +stays in the project as an `UnresolvedAnalyzerReference`, and *checksumming* one throws. Every +`FindReferences` call checksums the solution, so one missing analyzer path kills Shift+F12 for the +whole session. The fix drops any reference whose `FullPath` does not exist. + +**`DeleteBuildArtifacts(root)`** (public static, `:278`) - deletes every `obj`/`bin` below `root`, +deepest first, `IOException` swallowed. `AttributesToSkip = FileAttributes.ReparsePoint` is +mandatory: review worktrees link their submodules back to the real clone, and a walk descending +through such a link once deleted committed fixture binaries in the user's own checkout. Covered by +`BuildArtifactCleanupTests.cs`. + +**`LoadSyntaxOnly`** (`:300`) - `AdhocWorkspace`, one project `"Worktree"`, every `*.cs` below the +worktree except paths containing `/obj/` or `/bin/`, metadata references from +`AppContext.GetData("TRUSTED_PLATFORM_ASSEMBLIES")` (`:321`). Ends in `SyntaxOnly`. + +**`IndexDocuments`** (`:330`) - `documentsByPath` keyed `OrdinalIgnoreCase`; `TryAdd`, so the first +project wins for linked or multi-targeted files. + +### Base load - `LoadFrom(head, replaced, removed, added)` (`:62`) + +The base side is **not** a second checkout, restore and design-time build. It is the head's own +immutable `Solution` with documents swapped: + +- shares `head.workspace` and does **not** own it; +- copies `head.documentsByPath`, so its own mutations do not leak back; +- `replaced`: `derived.WithDocumentText(id, SourceText.From(text))`; +- `removed`: `documentsByPath.Remove` + `derived.RemoveDocument(id)`; +- `added` (files only the base revision has - deletions and rename sources): a new `DocumentId` in + **the project whose directory reaches furthest down towards the file** (`:97`), because a file no + longer in the tree has no project of its own; +- sets both `solution` and `loadedSolution` to the derived one, so a later overlay starts from the + *base* revision rather than the head's; +- copies the head's `State`/`StateDetail`. + +The caller assembles the three maps in `ReviewWorkspace.BaseSideTextsAsync` (`:625`) from the object +database via `Blobs.ReadAsync(baseSha, ...)`. + +### Paths + +- `ToAbsolutePath` (`:382`) = `Path.GetFullPath(Path.Combine(...))`. The `GetFullPath` is required: + git speaks forward slashes everywhere, `Path.Combine` does not normalise the ones already there, + and on Windows the index is keyed on Roslyn's `src\Foo.cs`. +- `ToRelativePath` (`:391`) - `OrdinalIgnoreCase` on Windows only, and it explicitly checks that the + character after the root is a separator, so `/repo-other` is not "inside" `/repo`. Returns + forward-slashed. + +### Operation by operation + +| Operation | Implementation | +| --- | --- | +| semantic tokens | `ClassifyAsync` (`:450`): `Classifier.GetClassifiedSpansAsync` over the whole text, keep only `IsIdentifierClassification` (`:547` - 18 `ClassificationTypeNames` values), drop multi-line spans, emit 1-based tokens, `DistinctBy((Line,Column,Length))`, ordered. The for-text variant forks the document with `document.WithText(...)` so the project's references and other files still resolve (`:443`). | +| quick info | `QuickInfoService.GetQuickInfoAsync`, sections joined with `\n\n` (`:488`). | +| hover | A *different* path: resolve the symbol, `ToDisplayString(CSharpErrorMessageFormat)` plus the `` extracted from `GetDocumentationCommentXml` by two regexes (`ExtractSummary`, `:975`). | +| go to definition | `ResolveAsync` (re-resolve the `SymbolRef` at its position) -> `DefinitionLocationOf` (`:814`): the first in-source location of `symbol.OriginalDefinition`. Null when metadata-only, and the caller then tries decompilation. | +| find references | `SymbolFinder.FindReferencesAsync(symbol, solution, ct)` -> `ReferencesOfAsync` (`:841`). Each hit reads its tree's text for the line preview; dedup on `(FilePath, Line, Column)`, ordered by path then line. | +| occurrences in file | `OccurrencesOfAsync` (`:508`): `FindReferencesAsync` scoped to one document; classification is `"reference"` or `"definition"` (definitions matched by comparing `location.SourceTree` with the document's syntax tree). | +| call hierarchy | `GetCallersAsync` (`:880`) = `SymbolFinder.FindCallersAsync` over the solution, indirect callers kept and *not* marked apart. `GetCalleesAsync` (`:898`) walks the member's `DeclaringSyntaxReferences`, keeps `InvocationExpressionSyntax` and `ObjectCreationExpressionSyntax`, groups sites by `target.OriginalDefinition` with `SymbolEqualityComparer.Default`. Both end in `Order` (dedup, sort by containing type then display). | +| go to symbol by name | `FindDeclarationsAsync` (`:708`): `SymbolFinder.FindSourceDeclarationsWithPatternAsync(solution, pattern, SymbolFilter.TypeAndMember, ct)` - Roslyn's own matcher, so prefix, substring and camel-hump all work (`"RWS"` finds `RoslynWorkspaceService`). Source declarations only; `IsImplicitlyDeclared` skipped; capped at `max`. | +| outline / folds | Delegated to `DocumentOutline.Compute` / `MemberFolding.Compute` on `sideText` - pure, synchronous, `Task.FromResult` (`:1112`). | +| enclosing member | `FindEnclosingMemberAsync` (`:682`) -> `MemberAtPosition` -> `DeclarationRef`. | +| change map | `MapLinesToMembersAsync` (`:590`) per line: skip blank lines, take the position at the first non-whitespace character, `MemberAtPosition`, key by display, keep the smallest line. `ListMemberDisplaysAsync` (`:560`) walks `MemberDeclarationSyntax` nodes; a field declaration declares nothing itself, so each `Variable` is asked separately. | +| decompile target | `GetDecompileTargetAsync` (`:1091`): resolve -> `OriginalDefinition` -> climb to the **top-level** containing type -> `TryGetMetadataAssemblyPath` (`:826`, scanning *already realized* compilations only, via `project.TryGetCompilation` + `compilation.GetMetadataReference(assembly)`) -> `DecompileTarget(assemblyPath, ns+MetadataName, original.MetadataToken, topType.Name)`. | + +**`MemberAtPosition`** (`:641`) is the trickiest piece. Walking outward from +`root.FindToken(position).Parent`: + +- hitting a `BlockSyntax` or `ArrowExpressionClauseSyntax` first -> break out and use + `model.GetEnclosingSymbol(position)`, which correctly reports a local function or lambda; +- hitting a `MemberDeclarationSyntax`/`LocalFunctionStatementSyntax` first (the position is in a + *header* - a signature, a `class C`, an attribute) -> `GetDeclaredSymbol(node)`, because a + member's own header is not inside the scope it opens, and a signature is exactly what a diff of a + changed member touches. + +`WalkToMember` (`:665`) then climbs `ContainingSymbol` until it reaches a method, property, field, +event or named type, maps accessors to their `AssociatedSymbol`, and returns null for a namespace. +Covered by `EnclosingMemberTests.cs`. + +### DocumentOutline + +`CSharpSyntaxTree.ParseText` - syntax only, resilient to broken code, no workspace needed. +Namespaces are **flattened** (`:31`). Titles: `class Foo` for types, `Bar(int, string)` for +methods (parameters rendered as *types*, falling back to the identifier), `~Foo()`, +`operator +(...)`, `implicit operator Foo`, `this[int]`, plain identifiers for properties, events +and fields; field and event-field declarations emit one node per variable. Lines are 1-based +inclusive from `syntax.Span`. + +### MemberFolding + +Foldable node kinds (`:21`): `BaseTypeDeclarationSyntax`, `BaseMethodDeclarationSyntax`, +`BasePropertyDeclarationSyntax`, `EventFieldDeclarationSyntax`, `LocalFunctionStatementSyntax`. + +- `DeclarationStart` (`:101`) skips attribute lists - folding from the attribute would hide the one + thing a collapsed member cannot say for itself. A member that is one line once attributes are + excluded stops folding. +- `HeaderEnd` (`:79`) finds the token that opens the body: for a type the `OpenBraceToken`; for + anything else the first token of the first child that is a `BlockSyntax`, + `ArrowExpressionClauseSyntax` or `AccessorListSyntax`. Asking child *nodes* is what keeps an `=>` + in a parameter default or a base list from being mistaken for the body opener. No body means the + header ends where it starts. +- `AddRegionDirectives` (`:47`) folds `#region` to `#endregion`, starting the fold at the *end* of + the `#region` line so the label stays visible. Regions are not bound by syntax nesting, so a + region crossing a member fold is **dropped** (`Crosses`, `:66`) rather than allowed to corrupt the + set - no folding manager can represent crossing folds. + +### MemberRelocation + +Not part of `ISemanticProvider`. It backs comment anchoring (`src/Stampeded/ReviewComments.cs:294`): +when a comment's line no longer exists, text-and-context matching finds nothing and the remark is +falsely reported outdated although the member it is about is right there. + +`Locate(oldText, oldLine, newText, lineText)` -> `MemberMove(Line, Member, FoundTheLine)`: + +1. `PathTo(DocumentOutline.Compute(oldText), oldLine)` - the chain of outline nodes containing the + line, outermost first. **Empty means null** (a using directive or file-level comment has no + member to follow). +2. `Find` (`:88`) walks the same chain in the new outline via `Best` at each level; failing that, + `Anywhere` searches the whole new tree for the innermost member - a type rename, or a member + moving between types of one file, leaves the member intact. +3. `Best` (`:112`): an exact `(Kind, Title)` match wins; otherwise same kind and same `Name(title)` + (the name without its parameter list), tie-broken by `Shared` - the length of the common prefix + of the two signatures, which separates overloads without pretending to compare types. +4. Inside the found member, look for a line whose trimmed text equals `lineText`, preferring the + candidate closest to `oldLine` (which is what tells two identical lines of one member apart) + -> `FoundTheLine: true`. +5. Otherwise place it at the same *offset* into the member (`oldLine - oldPath[^1].StartLine`); if + that runs past the member's end, use its first line - the declaration is what the comment is + about once the statement it named is gone, and a closing brace says nothing. + +Nothing requires the two texts to be related: it works across a rebase or force-push as long as the +old blob is still readable. Tests: `MemberRelocationTests.cs`. + +## The LSP client + +### Framing - `LspStream` + +Shared by both ends (client and our own server), which is why it lives apart from either. + +- `WriteMessageAsync`: `Content-Length: N\r\n\r\n` plus N UTF-8 bytes, then `FlushAsync`. +- `ReadMessageAsync`: header, then a `while (read < length)` loop; `null` at end of stream. +- `ReadHeaderAsync` (`:40`) reads **one byte at a time** - deliberate: any buffering would eat into + the body. `\r` dropped, `\n` ends a line, an empty line ends the header block and returns the last + `Content-Length` seen (`-1` at EOF). + +### `LspConnection` + +`LspServerSpec(Name, Executable, IReadOnlyList Arguments)` (`:12`). + +**Start-up - `StartAsync(spec, rootPath, ct, initializationOptions, settings)`** (`:73`): + +- `ProcessStartInfo` with all three streams redirected, `UseShellExecute = false`, and + **`CreateNoWindow = true`** - on Windows a console child of a GUI process gets its own window, and + npx reaches the server through cmd, so without this the reader gets black windows in their face. +- Removes `MSBUILD_EXE_PATH`, `MSBuildSDKsPath` and `MSBuildExtensionsPath` from the child + environment: this process pins them, and a server that runs `dotnet` would get the wrong SDK. +- A start failure becomes `CliLog` plus `ToolFailedException(executable, -1, message)`. This is the + common case (a server nobody installed) and callers catch exactly this + (`ReviewWorkspace.cs:2059, 2135`). +- Starts `PumpStdErrAsync` and `ReadLoopAsync`, each wrapped in `HandleFailure` (`:439`), which logs + `connection FAILED: ...` on fault. +- Sends `initialize` with `processId`, `rootUri`, `capabilities`, `trace` (`"verbose"` when tracing, + else `"off"`), `initializationOptions` and `workspaceFolders`; stores `capabilities.Clone()`; logs + `ReportWhatItCanDo`; sends `initialized`. + +**Client capabilities** (`:185`) are deliberately small, with **no dynamic registration**: +`textDocument.{synchronization(didSave:false, willSave:false), definition(linkSupport:false), +references, hover(plaintext+markdown), documentHighlight, documentSymbol(hierarchical), +semanticTokens(full, relative), callHierarchy}`, `workspace.{symbol, workspaceFolders, +configuration}`, `window.workDoneProgress`. + +**`ReportWhatItCanDo`** (`:145`) logs the server's `serverInfo` (falling back to `spec.Name`, +because pyright does not name itself) and, crucially, the list of the eight capabilities the review +needs that the server does *not* advertise - a missing capability answers nothing forever, which is +otherwise indistinguishable from a broken setup. + +**Request/response correlation** (`:207`): + +```csharp +public async Task RequestAsync(string method, object? parameters, CancellationToken ct) +``` + +- `id = Interlocked.Increment(ref nextId)`; the first id is 1 - note that `shutdown` in `Dispose` + uses id `0`, so there is no collision. +- `pending[id] = (TaskCompletionSource(RunContinuationsAsynchronously), method)`; the + method name is stored so failures log as `textDocument/definition FAILED: ...` rather than + `request 7 failed`. +- `ct.Register(() => completion.TrySetCanceled(ct))`; on `OperationCanceledException` it sends + `$/cancelRequest` and rethrows. +- `finally { pending.TryRemove(id, out _); }` +- Logging: while tracing, every request with elapsed ms and `Summarize(result)` (kind plus array + length, or a 200-char truncation); otherwise only requests over **500 ms**. +- **If `disposed`, returns `default(JsonElement)` silently.** + +**Dispatch** (`:272`): + +- `id` and no `method` is a response. An unknown or removed id is dropped. An `error` member is + logged and the waiter completed with `default` - so **a server error is indistinguishable from + "found nothing" at the call site**, and the log is the only record. +- `method` with `id` is a server-to-client request, handled by `AnswerServerRequest`. +- `method` without `id` is a notification: `window/logMessage` is copied into `CliLog`, and every + notification is re-raised on the `Notification` event. + +**`workspace/configuration`** (`:327`) answers one entry per requested item, **in the requested +order**, using `settings(section)` or `new object()` for sections we know nothing about, and logs +the whole answer. The comment at `:308` explains why nothing is declined by silence: a server that +asked and heard nothing waits, and everything behind it waits too. Responses go through +`ResponseJson` (`:36`), which does **not** drop nulls - a JSON-RPC response with neither result nor +error is malformed. + +**Shutdown** (`:374`): `disposed = true`, cancel `stopping`, `shutdown` (wait 500 ms), `exit` +(200 ms), `WaitForExit(1000)`, then `Kill(entireProcessTree: true)`. The polite shutdown exists so a +server mid-write to its cache does not leave it broken. + +**Tracing** (`:136`): `STAMPEDED_LSP_TRACE` set and not `"0"`. Turns on per-request logging, asks the +server for verbose tracing at initialize, and raises pyright's `logLevel` to `Trace`. + +**`LspUri`** (`:407`): + +- `FromPath` = `new Uri(path).AbsoluteUri`. +- `ToPath` (`:421`) un-escapes `%3A`/`%3a` **before** parsing, because pyright and everything else + built on vscode-uri writes `file:///d%3A/src/app.py`, which `Uri` does not recognise as naming a + drive; then strips a leading separator in front of `X:`. Returns `null` for non-file URIs (a + decompiled or generated document the server invented). + +### Document URIs and `?side=base` + +`LspSemanticProvider.Uri(relPath)` (`:76`): + +```csharp +LspUri.FromPath(ToAbsolutePath(relPath)) + (UriSide.Length > 0 ? "?side=" + UriSide : "") +``` + +`UriSide` is `""` for every server we did not write - those are rooted at one revision - and +`"base"` for the second `LspSemanticProvider` built on the **same** Roslyn-LSP connection +(`ReviewWorkspace.cs:2055`). It is a query on a file URI rather than a scheme of its own, so +everything that merely wants the path still reads one. + +### `LspSemanticProvider` + +State: `openDocuments` (relPath -> `TextIndex`), `symbolsByPath` (relPath -> flattened document +symbols), `overlay` (relPath -> text), `tokenTypes` (the server's semantic-token legend). + +- **Initial state** (`:37`): `Loading` if the server advertises + `experimental.loadsAsynchronously`, else `Ready`. Our Roslyn server sets that flag and then pushes + `stampeded/state` notifications, handled in `OnNotification` (`:53`). +- **`Open(relPath)`** (`:106`) is the gate on everything: servers answer about what they were told, + not about what is on disk. First use reads the overlay or the file, builds a `TextIndex`, logs + `opened (, line(s))`, and sends `textDocument/didOpen`. `LanguageIdOf` + (`:143`) maps `.py`/`.pyi` to `python`, `.cs` to `csharp`, plus `.ts`, `.js`, `.go`, `.rs`, else + `plaintext`. +- **Overlay** (`:153`): `SetTextOverlay` replaces the map and re-sends every already-open document; + `Resend` swaps the whole text (`didChange` with a single full `contentChanges` entry, version + hard-coded to 2), rebuilds the `TextIndex` and **invalidates `symbolsByPath`**. Incremental sync + would save bytes on a keystroke; nothing here types. +- **Symbols are words** (`:194`): there is no request that answers "what is the token here called", + so `GetSymbolAtAsync`/`GetSymbolOnLineAsync` take the identifier under the position from + `TextIndex.WordAt`, falling back to `FirstWordOn` for a caret in the indentation. `Make` builds a + `SymbolRef` with `Display == Name == word`, `IsType: false`, no containing type. +- **Enclosing member** (`:214`) comes from document symbols instead: `Innermost` plus the nearest + containing type-kind symbol. +- **Positional requests** map 1:1 to `textDocument/definition`, `references` (with + `includeDeclaration: true`), `documentHighlight` (kind `3` = Write is `"definition"`, else + `"reference"`) and `hover`. `GetQuickInfoAsync` simply calls `GetHoverTextAsync` (`:303`) - the + full documentation is what a reader opened the tooltip for. +- **Semantic tokens** (`:333`) decode the protocol's relative 5-tuple encoding (`deltaLine, + deltaStartChar, length, tokenType, tokenModifiers`), where `deltaStartChar` is relative only when + `deltaLine == 0`. Types outside the legend are skipped; `LspSymbolKinds.ClassificationOf` returns + `null` for keywords, strings and comments, which the grammar already colours and which a second + opinion would only fight with. +- **`Holds(relPath, sideText)`** (`:528`) is the revision guard for outline, folds and for-text + tokens: it compares the server's text with the text on screen after + `ReplaceLineEndings("\n").TrimEnd('\n')`, because no server preserves line endings or a trailing + newline faithfully. +- **Folds from document symbols** (`:515`): `MemberFoldRegion(StartLine, EndLine, max(StartLine, + SelectionLine))` - a document symbol says nothing about where the brace or colon is, only where + the name is. +- **Outline** (`:482`, `ToOutline` `:495`) drops variable-kind children of callable parents: a + server reports every local in a function body, and an outline is for finding a place to jump to. +- **Call hierarchy** (`:427`): `prepareCallHierarchy` -> item `[0]` -> + `callHierarchy/incomingCalls` or `outgoingCalls`; the other end is `from`/`to` respectively. + `fromRanges` are sites in the *caller's* file for incoming calls and in the *asked-about* file for + outgoing (`:458`). The node display is re-derived from document symbols via `DisplayOfAsync` + (`:567`) so it matches how the change map names the same member (`Greeter.greet`, not `greet`) - + the two are compared to tint calls the review touches. +- **`FindDeclarationsAsync`** is `workspace/symbol`, capped at `max`. +- **`GetDecompileTargetAsync`** (`:618`) sends `stampeded/decompileTarget`, but only when the server + advertises `experimental.decompileTarget`. +- **`Flatten`** (`:575`) handles both reply shapes: hierarchical `DocumentSymbol` (has `range`, + `selectionRange`, `children`) and flat `SymbolInformation` (has `location.range`, no children). +- **`LineTextOf`** (`:695`) reads a file for its text only and caches the `TextIndex` **without** + telling the server about it - a references search would otherwise open half the repository. + +**Thread-safety caveat.** `openDocuments`, `symbolsByPath` and `overlay` are plain `Dictionary`s +mutated from `async` methods (`DocumentSymbolsAsync` writes at `:557` *after* an await). This is only +safe as long as every call originates on one thread. There is no lock and no `ConcurrentDictionary`; +treat it as a UI-thread-affine object. + +### TextIndex and LspSymbolKinds + +`TextIndex`: line starts computed once by scanning for `'\n'`; `OffsetOf` (clamped to line length), +`LineColumnOf` (binary search), `LineText` (drops a trailing `\r`), `WordAt` +(`char.IsLetterOrDigit || '_'`), `FirstWordOn` (first word character that is not a digit). All +1-based; LSP's 0-based pairs are converted at the request boundary. + +`LspSymbolKinds` translates the two protocol vocabularies into the ones the review already uses: +`Names[]` for `SymbolKind` (a number in the protocol and nothing else), the predicates `IsType`, +`IsCallable` and `IsVariable`, `OutlineKindOf` (icon keys - a Python `def` reads as a `method`; +`Module`, `Namespace` and `Package` read as `class`), and `ClassificationOf` (LSP token type to +Roslyn classification name, where `null` means "no colour of ours"). + +## LanguageServers + +`ExtensionsByLanguage` (`:17`) currently holds only `python: {.py, .pyi}`. It decides whether a +review pays for a server at all (`ReviewWorkspace.LoadOtherLanguagesAsync`, `:2072`). + +### Python lookup order - `Python()` (`:27`) + +1. **`STAMPEDED_PYTHON_LSP`** - a whole command line (`"pylsp"`, `"npx basedpyright-langserver + --stdio"`), split on spaces, the first token resolved through `OnPath` where possible. Logged, + with `" - WHICH DOES NOT EXIST"` appended when the resolved executable is not there + (`FromEnvironment`, `:148`). +2. **On PATH**, in order: `pyright-langserver --stdio`, `basedpyright-langserver --stdio`, + `jedi-language-server`, `pylsp`. +3. **This tool's own install**: `Installed()` (`:72`) - + `/python-lsp/{bin|Scripts}/basedpyright-langserver[.exe]`. +4. **npx**: `npx --yes --package pyright -- pyright-langserver --stdio`. The explicit `--package` is + required - npx cannot infer the package `pyright` from the binary `pyright-langserver` and fails + with a 404 otherwise. +5. `null`, with a log line saying one can be installed. + +`ReviewWorkspace` then falls through to installing one when the spec is null *or* fails to start +(`ReviewWorkspace.cs:2084`); if that also fails it posts a status message and opens the Log pane. + +### Bootstrap install - `InstallPythonAsync(repoPath, ct)` (`:92`) + +Cache root is `CachePath.For("python-lsp")` = `$XDG_CACHE_HOME/stampeded/python-lsp`, falling back +to `~/.cache` (non-Windows) or `LocalApplicationData` (Windows). + +``` +python -m venv /python-lsp +/python-lsp/bin/python -m pip install --disable-pip-version-check basedpyright +``` + +Both go through `ExternalTool.RunAsync`, so both appear in the log; if `Installed()` already exists +it returns immediately; a `ToolFailedException` is logged and returns null. The interpreter comes +from `PythonEnvironment.InterpreterFor(repoPath)`, and when there is none, nothing is installed. + +**basedpyright rather than pyright** because it ships as a wheel carrying its own node, so a machine +with Python and no node still gets a working server - the case npx cannot serve. It is pyright +underneath. Nothing is added to the reader's Python or PATH; deleting the directory undoes all of +it. Test: `PythonServerInstallTests.cs`. + +### `Roslyn()` (`:126`) + +`STAMPEDED_CSHARP_LSP` first; then `Stampeded.RoslynLsp[.exe]` beside `AppContext.BaseDirectory`; +then the source-build sibling +`../../../../Stampeded.RoslynLsp/bin/{Debug|Release}/net10.0/Stampeded.RoslynLsp[.exe]`, with the +configuration guessed from whether the current output path contains `/Release/`. + +### The Windows PATHEXT problem - `OnPath` / `ExecutableNames` (`:163`, `:190`) + +PATH is walked by hand, rather than left to `Process.Start`, so the log can say which executable was +picked. For each directory, `ExecutableNames` yields the candidates: + +- **not Windows, or the path already has an extension** - the path itself, and nothing else; +- **Windows, no extension** - `path + ext` for every entry of `PATHEXT` (default + `.COM;.EXE;.BAT;.CMD`), and **only** those. + +The bug this prevents: npm installs a command twice into one directory - a POSIX shell script under +the bare name, and the `.cmd` that Windows actually runs. A search that stops at the first existing +file finds the extension-less `npx`, hands it to `CreateProcess`, and gets *"The specified +executable is not a valid application for this OS platform"*. The platform and the extension list +are **parameters**, not ambient reads, so the machine this matters on need not be the machine +running the test (`LanguageServerLookupTests.cs`). + +## PythonEnvironment + +### Why an interpreter at all + +The files under review live in a detached worktree of one commit. A virtual environment is not +committed, so it is never in there. But an interpreter path is just a path: it can point into the +reader's own clone while the analysed files sit elsewhere. That is what makes a review see the +project's dependencies. + +### Resolution order - `Candidates(repoPath)` (`:50`) + +Asked of the **repository**, not the worktree: + +1. `STAMPEDED_PYTHON_PATH` +2. an active `VIRTUAL_ENV` -> `/{bin/python | Scripts/python.exe}` +3. an active `CONDA_PREFIX` -> the same layout +4. `.venv`, then `venv`, then `env` inside the repository +5. `python3`, then `python` on PATH (via `LanguageServers.OnPath`) + +`InterpreterFor` (`:23`) takes the first candidate that is non-null **and exists**, logs +`interpreter: ()` and - the part that matters on someone else's machine - logs every +candidate that lost and why (`not set`, ` does not exist`). The question is never "which did +it pick" but "why not mine". + +### Why the interpreter is offered twice + +Servers disagree about where the interpreter is named, and a client cannot know which a given server +reads, so it is supplied everywhere any server we might start looks. + +**At initialize** - `InitializationOptions(interpreter)` (`:98`): + +```csharp +new { python = new { pythonPath = interpreter, defaultInterpreterPath = interpreter }, + workspace = new { environmentPath = interpreter } } // jedi-language-server's name for it +``` + +**Via `workspace/configuration`** - `SettingsFor(section, interpreter)` (`:78`): + +- `"python"` -> `{ pythonPath, defaultInterpreterPath, analysis }` (the analysis block is nested here + *as well as* standing alone, because servers and versions differ on whether it is one section or + two) +- `"python.analysis"` / `"basedpyright.analysis"` -> `Analysis` +- anything else -> `{}` + +`Analysis` (`:91`) is `{ autoSearchPaths = true, useLibraryCodeForTypes = true, logLevel = Tracing ? +"Trace" : "Information" }`. Under trace, pyright reports the interpreter it settled on and every path +it searches for imports into the Log pane - the whole answer to "why is this import unresolved on +that machine". Both are passed together at every start site (`ReviewWorkspace.cs:2115, 2127`). + +### pyrightconfig / pyproject + +There is **no code** that reads `pyrightconfig.json` or `[tool.pyright]`. That is the design, not an +omission: the project's config is committed, so the worktree has it and the server reads it itself. +The hazard is that its relative `venvPath` resolves against the checkout, where there is no +environment. The contract relied on - and asserted by `PythonProjectConfigTests.cs` - is that **the +interpreter supplied via initialize and configuration wins over a project's `venvPath`/`venv`**. The +test builds a worktree with `[tool.pyright] venvPath="." venv=".venv"` (pointing at nothing) plus a +separate clone whose `.venv` holds `mylib`, and asserts that go-to-definition on `mylib.hello` still +lands in `mylib`. + +## Stampeded.RoslynLsp + +### Program + +```csharp +var protocol = Console.OpenStandardOutput(); +Console.SetOut(Console.Error); // stdout is the protocol from here on +MSBuildLocator.RegisterDefaults(); // BEFORE any Roslyn assembly loads +Environment.SetEnvironmentVariable("OPENSSL_ENABLE_SHA1_SIGNATURES", "1"); +``` + +Everything written for a human - including the workspace's own load log - goes to stderr, which the +client copies into its Log pane. `--version` prints a banner and exits 0. Exceptions out of +`RunAsync` are logged and exit 1. + +### RoslynLspServer + +One `RoslynWorkspaceService head`, one nullable `@base`, one `Task headLoad`. Requests are handled +**serially, in arrival order** (`RunAsync`, `:44`) - Roslyn answers are cheap once loaded, and a +review asks one question at a time. Any handler exception is logged as ` FAILED: ...` and +answered with a `null` result. + +| Method | Handler | +| --- | --- | +| `initialize` | `:131` | +| `shutdown` | `:121` (sets `shuttingDown`) | +| `textDocument/definition` | `:315` | +| `textDocument/references` | `:325` | +| `textDocument/hover` | `:336` - returns `{contents:{kind:"plaintext", value}}` | +| `textDocument/documentHighlight` | `:344` - kind 3 for definitions, 2 for references | +| `textDocument/documentSymbol` | `:420` | +| `textDocument/semanticTokens/full` | `:480` | +| `textDocument/prepareCallHierarchy` | `:360` | +| `callHierarchy/incomingCalls` / `outgoingCalls` | `:384` | +| `workspace/symbol` | `:532` (capped at 100, head only) | +| `stampeded/loadBase` | `:220` | +| `stampeded/decompileTarget` | `:567` | +| `stampeded/changedMembers` | `:544` | +| `stampeded/memberDisplays` | `:557` | +| notifications: `exit`, `textDocument/didOpen`, `textDocument/didChange`, `initialized`, `$/cancelRequest` | `:99` | + +Anything else returns `null`. + +**`initialize`** (`:131`) advertises `textDocumentSync: 1` (full text), the definition, references, +hover, documentHighlight, documentSymbol, workspaceSymbol and callHierarchy providers, a +`semanticTokensProvider` with the 12-name legend at `:509`, and `experimental: { decompileTarget, +derivedBaseSide, changedMembers, loadsAsynchronously }`. The solution comes from +`initializationOptions.solution`. **The load is started on a background task and `initialize` +answers immediately** - loading is minutes on a large solution - and the client learns readiness +through `stampeded/state` notifications pushed from `head.StateChanged` (`ReportState`, `:212`). + +**`WatchParent`** (`:182`) polls the client's `processId` every 3 seconds and calls +`Environment.Exit(0)` when it is gone. A client killed with a signal never sends `shutdown`, and an +invisible language server is a solution's worth of memory left behind. + +**Base-side derivation - `stampeded/loadBase`** (`:220`): + +```jsonc +{ "replaced": {relPath: text}, "removed": [relPath], "added": {relPath: text} } +``` + +It first awaits `headLoad` - deriving from a solution still being opened produces a workspace that +knows nothing - then `new RoslynWorkspaceService().LoadFrom(head, replaced, removed, added)`, +disposes any previous `@base`, and logs the counts. The client sends exactly the three maps +`BaseSideTextsAsync` produced. + +**Side routing - `Target(uri)`** (`:285`): `LspUri.ToPath`, then `parsed.Query.Contains("side=base")` +picks `@base` over `head`; `service.ToRelativePath(path)` gives the file. `UriOf` (`:607`) adds +`?side=base` back when the service *is* `@base` (reference equality). + +**`didOpen`/`didChange` -> `OverlayAsync`** (`:251`) takes `params.text` or +`contentChanges[0].text` and applies it as a `SetTextOverlay` **only if it differs** from what the +workspace already has (line endings normalised). Re-stating the file on disk would throw away the +compilation that already knows it. + +**Document symbols** (`:420`) are computed by `DocumentOutline.Compute` on whatever text the +workspace holds - a pure function, so it answers loaded solution or not. `NameOf` (`:451`) strips the +leading keyword from an outline title (`"class Greeter"` -> `"Greeter"`), because the protocol +carries the kind in its own field and the client joins names with dots to build the display the +change map is compared against; leaving the keyword in would poison that comparison. `SymbolKindOf` +(`:458`) maps both the outline's C# keywords *and* Roslyn's own `DeclarationHit` kind names into +`SymbolKind` numbers. + +**Semantic tokens** (`:480`) re-encode `SemanticToken`s into the relative 5-tuple stream, mapping +Roslyn classification names to the legend via `LegendNameOf` (`:516`); classifications with no legend +name are dropped. + +**`prepareCallHierarchy`** (`:360`) converts the offset back to (line, column) with `LineColumnAsync` +(`:586` - an O(n) character scan) and resolves via `GetSymbolOnLineAsync`, then reports the +**symbol's own position** as both `range` and `selectionRange`, because incoming and outgoing calls +re-resolve from that item and it must name the member, not the call site that led there. + +Tests: `RoslynLspServerTests.cs` drives a real server through `LspSemanticProvider`. + +## DecompilationService + +`Decompilation/DecompilationService.cs`. **Shells out to nothing** - it is an in-process ILSpy +(`ICSharpCode.Decompiler`) call, the one place in the tool where an external process is not involved. + +```csharp +public static DecompiledType DecompileType(string assemblyPath, string reflectionName, int targetMetadataToken) +``` + +- `DecompilerSettings(LanguageVersion.Latest) { ThrowOnAssemblyResolveErrors = false }` - a review's + assembly graph is rarely complete. +- `new CSharpDecompiler(assemblyPath, settings).DecompileType(new FullTypeName(reflectionName))`, + where `reflectionName` is e.g. ``System.Collections.Generic.List`1``. +- Output is written through `CSharpOutputVisitor` with Allman formatting into a `StringWriter`, + wrapped by `MemberLocatingTokenWriter` (`:43`). + +**`MemberLocatingTokenWriter`** watches the token stream and records the line of the declaration +whose `IEntity` has `MetadataTokens.GetToken(entity.MetadataToken) == targetToken`. +`WriteIdentifier` gives the exact line (fields and events put the name inside a +`VariableInitializer`, so it climbs one parent); `StartNode` on an `EntityDeclaration` is the +fallback for declarations that write no `Identifier` at all (indexers, operators), whose start may +point at leading documentation or attributes. `FoundLine => identifierLine ?? declarationLine`, and +the caller defaults to line 1. + +The line comes from `ILocatable.Location` - **the writer that produces the text** - because comments +and preprocessor directives end their own line without going through `NewLine()`, so counting +`NewLine` calls drifts further off with every doc comment above the member. + +### How a sourceless definition becomes a read-only document + +`ReviewWorkspace.NavigateToDefinitionAsync` (`:2194`): + +1. `GetDefinitionAsync` returns null -> `OpenDecompiledDefinitionAsync` (`:2259`): + `sem is IDecompileTargets` -> `GetDecompileTargetAsync` -> `DecompilationService.DecompileType` on + a background thread -> `DiffDocumentViewModel.ForSource(TypeName + ".cs", text)` with the title + ` [decompiled]`, document id `decomp:`, caret at `result.MemberLine`. + Failure surfaces as a status message *and* a log line. +2. `GetDefinitionAsync` returns a path **outside the tree** (`ToRelativePath` gives null) -> + `OpenDefinitionOutsideTheTree` (`:2225`): reads the file, opens it as `source:`, tab tooltip + is the full path. This is what makes F12 into a Python package in the environment work at all; it + used to do nothing. + +Both routes produce an ordinary source document with no diff behind it, which is what makes them +read-only in practice. Both `RecordOrigin` and `history.Record`, so Back works. + +## Testing + +### TestService + +```csharp +public sealed class TestService(string worktreePath) +public async Task<(int ExitCode, IReadOnlyList Results)> RunAsync( + string argsLine, Action onOutputLine, CancellationToken ct, string? coverageOutput = null) +``` + +- `started = UtcNow - 5s` is the cutoff for "a TRX this run produced". +- `argsLine.Split(' ')` - no quote handling, marked with an explicit `// ponytail:` comment at `:16` + acknowledging the ceiling (the args box is a developer-facing escape hatch). +- Without coverage: `dotnet `. With coverage: `dotnet-coverage collect --output + --output-format cobertura -- dotnet ` - Microsoft's dynamic engine wraps the whole run, so no + project changes are needed. +- Environment `OPENSSL_ENABLE_SHA1_SIGNATURES=1` plus `StripMsBuildLocatorVariables`; + `CommandResultValidation.None`; both stdout and stderr piped line by line into `onOutputLine`; + start and exit code logged to `CliLog`. +- Collection: every `*.trx` below the worktree whose `LastWriteTimeUtc >= started`, parsed; + `XmlException` swallowed (a truncated TRX from an aborted run is not worth failing over). +- `ParseDirectory(directory)` (`:56`) does the same without the timestamp filter, for downloaded CI + artifacts. + +Call sites: `Panes/TestsPaneViewModel.cs:212/217` (A/B) and `:291` (single run). Both **wait for the +semantic load to finish first** (`:201`, `:284`) - the semantic load runs `dotnet restore` and +design-time builds in the same worktree, and a concurrent test build trips over half-written `obj/` +state and dies with an opaque "Build failed" (the test platform hides MSBuild's errors). + +### TrxParser + +`TestOutcome { Passed, Failed, Skipped, Other }`. + +`TestResult(TestName, Outcome, Duration, ErrorMessage?, StackTrace?)` with `TryGetSourceLocation()` +(`:25`) - the first `" in :line "` frame, via +`[GeneratedRegex(@" in (?.+?):line (?\d+)")]`. That is what makes double-clicking a +failure jump to the frame. + +`Parse(trxContent)` (`:41`): `XDocument`, namespace +`http://microsoft.com/schemas/VisualStudio/TeamTest/2010`, every `UnitTestResult` descendant; +`outcome` mapped with `"NotExecuted"` and `"Skipped"` both to `Skipped` and anything unrecognised to +`Other`; `TimeSpan.TryParse` on `duration` (a failure silently leaves `default`); message and stack +from `Output/ErrorInfo/{Message,StackTrace}`. + +### CoberturaParser + +```csharp +public static IReadOnlyDictionary> Parse(string xml, string rootPath) +``` + +Every ``: the `filename` attribute, resolved by `Resolve` (`:38`) against, in order, the +absolute path itself, each `` element, or `rootPath`; the first candidate landing **strictly +inside** `root` wins and is returned root-relative with `/` separators. Line hits are merged **by +max** across class entries sharing a file (partial classes, multiple targets). The result is +consumed by `ReviewWorkspace.SetCoverage` for the gutter overlay. + +### TestRunComparison + +The question a text diff of two outputs cannot answer: *did this change introduce the failure, or +was it already broken at base?* + +```csharp +public sealed record TestRunComparison( + IReadOnlyList NewlyFailing, IReadOnlyList Fixed, IReadOnlyList StillFailing, + int BasePassed, int BaseFailed, int HeadPassed, int HeadFailed) +``` + +`Compare(baseResults, headResults)` (`:14`) keys on **test name** (ordinal), because one name can +appear once per target framework and one failing result marks the name failing: + +- head failures: in `baseFailed` -> `StillFailing`, else -> `NewlyFailing` (a newly written test + that fails counts as a new failure - deliberate, `:29`); +- head passes that were failing at base and are not also failing at head -> `Fixed`; +- the four counts are distinct-name counts, not result counts. + +### GeneratedSources + +Generated code is not in git, so a change that is entirely about what a generator emits is invisible +in the diff. This turns it into an ordinary before/after comparison. + +- `EmitProperty = "-p:EmitCompilerGeneratedFiles=true"` (`:20`); output lands under + `obj///generated/`. +- **`BuildAsync(worktreePath, chosenSolution, ct)`** (`:27`): `dotnet build [] + -p:EmitCompilerGeneratedFiles=true --nologo -v quiet -p:GenerateDocumentationFile=false`. The + solution is **named** via `SolutionTarget.ForRoot` - a root with several solutions is refused + outright by `dotnet` ("Specify which project or solution file to use"), which is exactly why + generated sources never arrived for repositories shipping an installer or extension solution + beside the product's own. The log line is written *before* the build, because a command is + normally logged when it finishes and this one runs for minutes. +- **`Collect(worktreePath)`** (`:56`): every directory named `generated`, keyed by `/generated/`. `RelativeKeyPrefix` (`:79`) accepts only the real layout - + `.../obj///generated`, i.e. `parts.Length - lastIndexOf("obj") == 4` - so a source + directory that happens to be called `generated` is ignored. The config and TFM are dropped from the + key so the two sides pair up even when built differently; the project stays in the key because two + projects can host the same generator. +- **`DiffAsync(baseWorktree, headWorktree, ct)`** (`:95`): the union of both key sets, ordered; kind + from which side has the file; hunks from `DiffFilesAsync`; identical files produce no hunks and are + omitted entirely. Each `FileDiff` carries `new GeneratedSource(oldFile, newFile)`. +- **`DiffFilesAsync`** (`:123`): `git diff -U3 --no-index -- ` run + from `Path.GetTempPath()`, with `okExitCodes: [1]` because `--no-index` reports "differences found" + as exit 1. Parsed with the review's own `GitDiffParser` so the output has the same shape as + everything else; the parsed paths are discarded (they would be absolute paths into two throwaway + worktrees). diff --git a/docs/ui.md b/docs/ui.md new file mode 100644 index 0000000..98bfc47 --- /dev/null +++ b/docs/ui.md @@ -0,0 +1,730 @@ +# The Avalonia UI layer + +`src/Stampeded/` - the app project. Roughly 24k lines across 158 files. + +| Directory | What lives there | +| --- | --- | +| `*.cs` (root) | startup (`Program`, `App`), shell (`MainWindow`, `MainViewModel`), the session hub (`ReviewWorkspace`, `ReviewScopes`, `ReviewComments`), dialogs, preference/state singletons, `ScreenshotWatcher`, `Images`, `ViewLocator` | +| `Docking/` | `StampededDockFactory` - the one place the layout is built | +| `Documents/` | Dock `Document` view models + views: diff (unified), side-by-side, overview, review verdict, start page, plain text; comment-thread rendering; the shared gesture table | +| `Panes/` | Dock `Tool` view models + views | +| `Editor/` | AvaloniaEdit extension points | +| `Diff/` | diff chrome: margins, row background renderer, overview scrollbar, context gaps, folding glue, classification colours | +| `Controls/` | small custom controls | +| `Controls/TreeView/` | **vendored from ILSpy** (MIT), kept close to upstream - read `src/Stampeded.Core/TreeView/README.md` first. The model half lives in `Stampeded.Core/TreeView/`. Used instead of Avalonia's `TreeView` because `TreeFlattener` projects the hierarchy into one virtualized `IList`: depth costs an indent value, not a nested container, which is what makes an unbounded call hierarchy survivable. | +| `Themes/` | `ThemeManager`, `SyntaxColor`, `SyntaxColorPalettes` | +| `Navigation/` | `NavigationHistory` (also vendored from ILSpy) | + +The five largest files: `ReviewWorkspace.cs` (2802), `DiffDocumentView.axaml.cs` (1270), +`StartDocumentViewModel.cs` (1049), `SideBySideDocumentView.axaml.cs` (684), `ReviewScopes.cs` (653). + +## Startup + +`Program.Main` and `MainViewModel`'s construction order are described in +[review-session.md](review-session.md). What belongs here is the rest of the shell. + +### App.axaml.cs + +`App.Workspace` is a **static mutable `ReviewWorkspace?`** - the single review session of this +process. Nearly every view reaches it as `App.Workspace?....`. + +`OnFrameworkInitializationCompleted` creates `MainWindow`, hooks `desktop.ShutdownRequested -> +Workspace?.Shutdown()`, and registers `PosixSignalRegistration` for SIGTERM/SIGINT/SIGHUP, each +calling `Workspace?.Shutdown()` **without cancelling the signal** - a language server is a child +process that outlives an unclean exit, and holding the process open to tidy is how a kill becomes a +`kill -9`. The registrations are kept in a static list so they stay alive. + +`OpenRepositoryAsync(path, prNumber)` validates `.git` exists, shuts the old workspace down, +rewrites `Program.RepoPath` / `Program.Host`, then **replaces `window.DataContext` with a new +`MainViewModel`** - which is what rebuilds the entire dock. + +`OpenFromUrlAsync(input)` parses Azure DevOps URLs **first** (GitHub's grammar accepts bare +`owner/repo` and would read `dev.azure.com/org/...` as a repo owned by `dev.azure.com`), then +GitHub. It searches `Program.RepoPath` + `RecentRepos` for a clone whose **any** remote matches, and +otherwise asks where to clone and makes a blobless partial clone (`--filter=blob:none`). + +`App.NextFolderAnswer` is a test seam: the folder picker is the desktop portal's own dialog and +nothing in-process can drive it, so the screenshot harness pre-answers the next question. + +### App.axaml - resources and styles + +- Theme dictionaries `Light`/`Dark` define `Stampeded.EditorBackground`, + `Stampeded.EditorSelectionBrush`, `Stampeded.ChromeBackground`, `Stampeded.TreeFocusFill/Border`, + **and restate the whole Simple-theme palette**. Only the *brushes* are overridden, not the colours + behind them: the Simple theme builds each brush from its colour with a `StaticResource`, resolved + once at parse time. +- `Button.tool` is the flat icon-button class used by every pane toolbar; `Border.toolsep` is the + hairline group separator. +- `DocumentTabStripItem` binds `ToolTip.Tip` to `TabTooltip` **by ReflectionBinding** - the strip's + item is typed as a dockable and only file documents carry the property. +- `OverlayPopupHost` gets `RenderTransform = {x:Static local:ZoomState.PopupScale}` - popups live in + the window's overlay layer, outside the `LayoutTransformControl` that scales the content, so they + are scaled separately. +- The empty `` suppresses the "About Avalonia" app + menu macOS would otherwise synthesize. + +### MainWindow + +`LayoutTransformControl` (ScaleTransform bound to `MainViewModel.Zoom`) -> `DockPanel` painted +`Stampeded.ChromeBackground` -> `NativeMenuBar` (top), busy bar (bottom), and a `Panel` holding +`dock:DockControl` plus the **preparation overlay** - a modal scrim bound to +`StartPage.State.IsPreparing` listing `StartPage.PrepareItems` with a `WaveSpinner` per pending item +and a "Continue now" button. + +`LayoutTransformControl` rather than a `RenderTransform` is deliberate: it re-runs layout at the new +scale, so text is laid out *and rendered* at that size instead of a fixed layout being magnified. + +## The docking model + +`StampededDockFactory` extends `Dock.Model.Mvvm.Factory`. There is **no registry indirection** - the +pane set is small and closed. + +``` +Root (IRootDock) ++- mainLayout: ProportionalDock, Horizontal + +- leftDock: ProportionalDock (Proportion 0.2, Vertical) + | \- filesDock: ToolDock "FilesDock", Alignment.Left + | \- Explorer*, Structure, Map (* active) + +- ProportionalDockSplitter + \- rightSide: ProportionalDock, Vertical + +- Documents: DocumentDock "Documents", IsCollapsable = false + +- ProportionalDockSplitter + \- bottomDock: ToolDock "BottomDock", Alignment.Bottom, Proportion 0.28 + \- References*, CallGraph, Comments, Commits, History, + Checks, MergeQueue, Tests, Run, Log +``` + +Pane ids: `Explorer`, `Map`, `Structure`, `References`, `CallGraph`, `Comments`, `Commits`, +`History`, `Checks`, `MergeQueue`, `Tests`, `Run`, `Log`. + +Each pane is recorded in `panes: Dictionary`: + +```csharp +public T? Pane(string id) where T : Tool +public void ShowPane(string id) +``` + +`ShowPane` re-adds the pane to its **home** dock when it is nowhere in the layout (`FindDockable` +walks `RootDock` and every floating `Window.Layout`), then activates and focuses it. +`workspace.Comments.Pane` is wired here so `ReviewComments.BeginComment` can activate the pane. + +### Documents + +Documents are **not** created by the factory. They are created on demand by `ReviewWorkspace` +through one private `ShowDocument(id, create)` helper. The id vocabulary is listed in +[review-session.md](review-session.md). + +**A file is one tab in either layout.** `ShowDiffDocument` keys both layouts on `diff:`; if +the existing document's type does not match `DiffLayoutPreference.SideBySide`, it is closed and +rebuilt. + +### Layout persistence - there is none + +`CreateLayout()` runs fresh on every `MainViewModel` construction. The only window-level state that +survives a session is the window geometry (`WindowPlacement`), the zoom (`ZoomPreference`), the tab +row mode (`TabRowsPreference`) and the diff layout (`DiffLayoutPreference`). Anyone adding layout +persistence would hook the factory's serializer around `MainViewModel.cs:146`. + +### ViewLocator + +An explicit `Dictionary>` mapping 22 view models to views, installed as +`Window.DataTemplates`. A missing entry renders `"No view registered for X"` rather than throwing. +**Any new document or pane needs an entry here or it renders as that text block.** + +## Documents + +### Common contracts + +```csharp +public interface IDiffDocument +{ + FileDiff File { get; } + string? Id { get; } + void RequestCaret(int blobLine, bool oldSide = false); +} + +[Flags] public enum ReviewCommands { + None, JumpToHunk, JumpToUncovered, ToggleBlame, CommentAtCaret, GoToDefinition, + FindReferences, HighlightOccurrences, ShowCallGraph, HistoryOfSelection, DebugHere } + +public interface IReviewDocumentView +{ + ReviewCommands Supported { get; } + string DocumentId { get; } + (int BlobLine, bool OldSide)? CaretOrigin { get; } + bool JumpToHunkCommand(int direction); + void JumpToEdgeHunk(int direction); + void JumpToUncoveredCommand(); + bool BlameVisible { get; } + void ToggleBlameCommand(); + void CommentAtCaretCommand(); + void GoToDefinitionCommand(); + // ... FindReferences, HighlightOccurrences, ShowCallGraph, HistoryOfSelection, DebugHere +} +``` + +`IReviewDocumentView` exists so the two layouts cannot drift silently: adding a command forces both +views to answer. `ReviewViews` is a `Dictionary` keyed by **dockable +id**, and `ReviewViews.Active` resolves through `Documents.ActiveDockable.Id`, **not through focus** - +clicking a tab header need not move focus, and a command landing on a document nobody is looking at +is worse than one that does nothing. + +`DiffDocumentView.Supported` is every flag. `SideBySideDocumentView.Supported` is +`JumpToHunk | GoToDefinition | FindReferences | CommentAtCaret` only; everything else routes to +`NotHere(string)`, which logs and posts a status line. + +### DiffDocumentViewModel + +Key members: `PristineModel` (the diff as built from blobs, the base every comment-thread re-splice +starts from), `Model` / `ReplaceModel`, `IsSourceView`, `Historical` + `HistoricalSha`, `IsPatch`, +`TabTooltip` / `TabTooltipOverride`, `RequestCaret` / `TakePendingCaret` (the pending-caret pair +exists because navigation may open a document and *then* say where to land, in either order), and +the static `ForSource(relPath, text)` factory which builds an identity diff. + +### DiffDocumentView - the unified diff + +Composition, in the constructor: + +| What is installed | Where | +| --- | --- | +| `SearchPanel.Install(Editor)` | AvaloniaEdit's find bar | +| `DiffLineBackgroundRenderer(() => model?.Tags)` | `TextView.BackgroundRenderers` | +| `TextMarkerService` | `TextView.BackgroundRenderers` (occurrence highlights) | +| `ThreadElementGenerator` + `CommentThreadBox` | `TextView.ElementGenerators` | +| `ReferenceElementGenerator` | `TextView.ElementGenerators` | +| `DiffLineNumberMargin` | `TextArea.LeftMargins.Insert(0, ...)` | +| `FoldViewportAnchor.Install(Editor)` | tunnelling pointer handler on `TextArea` | +| `ContextGapView(Editor)` | element generator + background renderer + `LayoutUpdated` | +| `PointerCrossHairRenderer` (DEBUG) | `TextView.BackgroundRenderers` | +| `BlameMargin` / `CoverageMargin` | inserted and removed on demand | + +Handler routing subtleties, each of which cost a debugging session: + +- editor gestures are handled **Tunnel** on `TextArea` because AvaloniaEdit has its own bindings for + Ctrl+Down/Up and a key it acts on never reaches the window; +- `CommentBox` keydown is `Bubble, handledEventsToo: true` because a `TextBox` with `AcceptsReturn` + handles Enter itself before any handler declared on it; +- pointer **release** is handled on `TextArea`, not `TextView`, because AvaloniaEdit captures the + pointer and captured releases are raised on the capturing control. + +`ActiveView` / `ActiveViewChanged` is the static view registry the Explorer, Structure and History +panes follow. `ViewFor(vm)` uses a `ConditionalWeakTable` +because `ActiveView` is stale the moment a tab is selected without the mouse. + +Rendering a model: + +``` +Editor.Text = model.Text +ApplySyntaxColors() // TextMate paint, sliced +margin.Columns = IsSourceView ? New : Both; margin.Tags = model.Tags +Overview.Attach(Editor, model.Tags) +InstallFoldsAndGaps(model) +ApplyMarginCursors() +referenceGenerator.References = null; markers.RemoveAll(_ => true) +QueueSemanticsRefresh() +``` + +**Folding vs context gaps.** Two *separate* mechanisms, deliberately not nested: + +- **Structural folds** (types, members, `#region`) come from `ISemanticProvider.GetFoldRegionsAsync` + over **one side's text**, mapped to document lines through `DiffFolding.Members(regions, + sideToDocLine)` and installed via `FoldingManager`. When the provider answers synchronously + (`Task.IsCompletedSuccessfully`) they are installed immediately; a language server's answer + re-installs later. +- **Context gaps** (unchanged runs) are hidden by `ContextGapView` using `TextView.CollapseLines` + directly. `RefreshFoldings` clips structural ranges to what is visible so the fold margin never + offers to collapse code the reader cannot see. + +**Blame** asks each side separately so one unblameable side does not kill the other; `oldRev` is +`null` in the since-last-pass scope, because that scope's base is a *tree*, not a commit, and `git +blame` answers a tree with "Non commit". + +**Hover** is a 400 ms `DispatcherTimer`, gated by `HoverPointer.PointsElsewhere` (text position, not +pixels). Every empty outcome is logged once per reason through `HoverLog` - "no tooltip" and "no +hover" are otherwise indistinguishable. + +**Semantic layer**: `RefreshSemanticsAsync` asks each side for tokens (falling back to +`GetSemanticTokensForTextAsync` when the loaded workspace holds a different revision than what is +displayed), builds a `RichTextModel` + `TextSegmentCollection`, and installs a +fresh `RichTextColorizer` as a `LineTransformer`. + +### SideBySideDocumentView and SideBySidePane + +Two `ReviewTextEditor`s with a `GridSplitter`, fed from `SideBySideModel`. **Equal line counts on +both sides** (Filler rows on the shorter one) are the invariant that makes everything else work. + +- **Scroll sync**: the `ScrollViewer`s only exist once templates are applied, so wiring retries up to + 20 times at `DispatcherPriority.Background`. `Sync` copies `Offset` with a re-entrancy guard + released **a dispatcher turn later** - a narrower pane clamps the offset it was given and reports + the clamp as a scroll of its own from inside the layout pass, and answering that echo makes the two + panes correct each other until layout gives up. +- **Fold mirroring**: both panes get the *same* ranges, so sections are matched by index. +- **One gap view drives both editors**: `new ContextGapView(Left, Right)`. +- **Thread rows**: a thread belongs to one side, so the other pane draws a `ThreadSpacer` of exactly + the same height. +- `SideBySidePane` carries the per-pane semantic/navigation layer. A pane holds exactly one blob, so + its `blobToDocLine` mapping is a dictionary lookup rather than the tag disambiguation the unified + view needs. + +### OverviewDocumentViewModel + +The review brief. `CanClose = false` - it is the review's home tab. + +Sections, each an `Expander` over a `ListBox` with `VerticalScrollBarVisibility="Disabled"` (one +scroll region for the page): Description (`MarkdownScrollViewer`), Commits, Changed files +(cost/churn), Changed members. + +Above them, docked and non-scrolling: title, scope toolbar, scope banner painted with `ScopePalette`, +warning lines (`LocalHeadLine`, `OfflineLine`, `WorkingTreeLine`, `ToolStatus`), estimate, CI header ++ failing checks, reviewers, coverage, tests, linked issues. + +Each rebuild method is bound to one workspace event. `RebuildLinkedIssuesAsync` puts every `#123` in +the body **to the host** and shows only the ones something answers to, with a pass counter to drop +overtaken runs. + +The view installs `MarkdownLinks.NewEngine()` so description links open, enables `MarkdownSelection`, +and posts `MarkdownEmphasis.Repair` after every markdown re-render. It is `Focusable = true` so `o` +has somewhere to land. + +### ReviewDocumentViewModel + +Every comment of the review, each quoted with ±3 lines of code read from the blob (with a +`(rev, path) -> string[]` cache), plus the verdict row and the merge block. + +`RefreshMergeAsync` reads the merge state fresh on every rebuild - it changes with every push and +every review. `MergeButtonTip` recomputes from `MergeExplanation` because a disabled button shows no +tooltip, so the explanation is also put on the status *line*. `Submit` keeps `Outcome` separate from +`Status` because posting a review reloads the comments, and the reload used to overwrite the answer. + +### StartDocumentViewModel + +Three columns: recent repositories, open pull requests, branches/stashes. Each column has a filter +toggle that takes over the column header, reachable by Ctrl+F or by just typing into the list. + +Branch annotation joins branches against PRs (excluding fork heads - a fork's `master` is a different +branch), computes merge state from `ListMergedBranchesAsync` plus a *cached, background* +patch-equivalence check (keyed by branch tip, invalidated whenever the default base moves), sync +state against the PR head, worktree ownership and ahead-counts. + +Also here: the `in progress` banner with Resolve / Continue / Skip / Abort, driven by +`Git.ListInProgressAsync`, re-asked on every window activation - a conflicted rebase is usually +finished in a terminal. + +The preparation checklist has eight fixed rows. **Only row 0 (the diff) gates the overlay**: +semantics, map, CI, churn and comments arrive into a window already in use. + +A deliberate warm-up: `Task.Run(() => SyntaxPainter.For("warm.cs")?.Paint(...))` pays the TextMate +registry and first-tokenizer cost (~¼ s) while the start page waits for a click. + +## AvaloniaEdit extension points + +### ReviewTextEditor + +Three things only, each load-bearing: + +```csharp +protected override Type StyleKeyOverride => typeof(TextEditor); +``` + +Without it Avalonia resolves the template by runtime type, AvaloniaEdit's template never applies, +**no `ScrollViewer` is installed**, scroll offsets stay 0 and `Copy` cannot reach the `TextArea`. + +Font `"Cascadia Code,Consolas,Menlo,DejaVu Sans Mono,monospace"` at 13; `SelectionCornerRadius = 0` +and a flat translucent `SelectionBrush` so **selected text keeps its syntax colours**; +`ThemeManager.ThemeChanged -> TextView.Redraw()` on attach/detach, because painted lines cache their +colour decisions. + +### Element generators + +| Generator | Replaces | Invalidated by | +| --- | --- | --- | +| `ReferenceElementGenerator` | spans in a `TextSegmentCollection` -> `VisualLineReferenceText` (clamped to the line; hyperlinks cannot span line breaks) | setting `.References` + `Redraw()` | +| `ThreadElementGenerator` | the `ThreadMarkerPrefix...Suffix` marker text on a synthetic line -> `InlineObjectElement(markerLength, control)` | document re-splice, or `Redraw()` when only the content changed | +| `ContextGapElementGenerator` | the whole run of hidden lines -> one `InlineObjectElement` carrying the reveal buttons | `ContextGapView.Apply()` | + +`ContextGapElementGenerator.ConstructElement` spans **every line the gap hides**, not just the one +the bar sits on: a visual line may cover several document lines only while an element accounts for +their text, and a collapsed line cannot start a visual line of its own. + +`VisualLineReferenceText.OnQueryCursor` deliberately **does not** set `e.Handled` - marking it +handled suppresses `PointerHoverLogic`'s tracking and hover events then fire with stale args. + +### Background renderers + +| Renderer | Layer | Draws | +| --- | --- | --- | +| `DiffLineBackgroundRenderer` | `Background` | full-width added/removed/filler row tints + intra-line word-diff spans from `tag.WordDiffs` | +| `ContextGapBackgroundRenderer` | `Background` | the gap row band under the bar | +| `TextMarkerService` | `Selection` (behind selection) | rounded background rects for occurrence highlights | +| `LineHighlightAdorner` | `Selection` | one-shot amber line flash, 800 ms | +| `CaretHighlightAdorner` | `Caret` | one-shot rectangle around the caret; rects kept in **document** coordinates and translated by live `ScrollOffset` each frame | +| `PointerCrossHairRenderer` (DEBUG) | `Caret` | crosshair + `(x, y) Ln/Col` readout | + +`CaretHighlightAdorner.InvalidateHostLayer` and `PointerCrossHairRenderer.UpdatePointer` both loop +over `textView.Layers` calling `InvalidateVisual()`: **`TextView.InvalidateLayer` only invalidates +the TextView's own measure in AvaloniaEdit 12** and never re-renders the per-layer child controls. + +### Margins + +| Margin | Width | Draws | +| --- | --- | --- | +| `DiffLineNumberMargin` | `ColumnCount * (digits*digitWidth + 8) + 3 + 4` | old and/or new blob line numbers, a 3 px added/removed strip at the right edge, and a **drawn vertical ellipsis** on a context-gap row (the gutter's mono font is not guaranteed to carry `⋮`) | +| `BlameMargin` | `charWidth * 25 + 8` | age-tinted `sha7 author age` rows, text only on the first row of a same-commit run | +| `CoverageMargin` | 5 px | 3 px green/red strip per measured head line | + +All three begin `Render` with `ContextGapChrome.DrawRows(...)` - the gap bar is an inline object and +can only cover the *text*, so each gutter paints its share of the band. +`ContextGapFoldingMargin` is a `FoldingMargin` subclass doing the same, installed **in place of** the +margin `FoldingManager.Install` added; otherwise the band has a notch in it. + +### Line transformers + +Two `RichTextColorizer`s are stacked per editor: **syntax** inserted at index 0, **semantic** +appended. Both are removed and rebuilt rather than mutated. `ClassificationColors` maps Roslyn +`ClassificationTypeNames` to a light/dark hex pair with a `(name, dark)` cache of frozen +`HighlightingColor`s. + +### Scroll and anchor helpers + +`FoldViewportAnchor`: a tunnelling `PointerPressed` on the `TextArea` captures `(topmost visible +line, its delta from the offset)` **before** the fold margin acts, then restores it at +`DispatcherPriority.Loaded`. `Preserving(editor, action)` is the programmatic form. + +Both it and `ContextGapView.RestoreBelow` note the same trap: **`TextEditor.ScrollToVerticalOffset` +is an empty method in AvaloniaEdit 12** (its whole body is a call to `ApplyTemplate`). Everything +that scrolls must reach the editor's `ScrollViewer` through `GetVisualDescendants()`. + +## Syntax highlighting + +### Why the document cannot simply be highlighted + +A grammar is a state machine over *consecutive* lines - a block comment, a `'''` string, a here-doc +all open on one line and close on a later one. A unified diff is consecutive on neither side: it +interleaves two blobs and splices comment rows between them. A removed line that opens a span and +the added line that closes it switch the state on and off in places where neither file does, and +everything below reads in the wrong colour. + +### SyntaxPainter + +```csharp +readonly record struct ColoredSpan(int Line, int Start, int Length, HighlightingColor Color); +abstract class SyntaxPainter +{ + public abstract IEnumerable Paint(string text); + public static SyntaxPainter? For(string path, Func content); + public static SyntaxPainter? For(string path); +} +``` + +Resolution order: TextMate grammar by extension -> content sniffing +(`GuessFileType.DetectTextType` -> `xml`/`json`) -> the editor's own `.xshd` definitions via +`HighlightingService` (which answers for ILAsm, registered from an embedded resource in its static +constructor). `content` is a lazy `Func` - it is only read when the extension said nothing. + +`XshdPainter` runs a `DocumentHighlighter` over a throwaway `TextDocument`. `TextMatePainter` +tokenizes line by line carrying `IStateStack`, with a **100 ms per-line budget** so a pathological +regex leaves a line uncoloured instead of stalling the view. + +`TextMateGrammars` holds the registry and theme behind a `Lock`, rebuilt whenever +`ThemeManager.Current.IsDarkTheme` flips (`DarkPlus` / `LightPlus`), with a per-scope painter cache. + +### DiffSyntaxColors - the transfer step + +Both methods return `IEnumerable` - **the work, not the result** - so the caller decides how +much of it runs before the view draws. + +```csharp +public static IEnumerable Build(SyntaxPainter painter, DiffDocumentModel model, + TextDocument document, RichTextModel rich); // unified +public static IEnumerable Whole(SyntaxPainter painter, TextDocument document, + RichTextModel rich); // one pane +``` + +`AddSide` paints one side's own text, then maps each span's line through +`model.GetSideText(oldSide).sideToDocLine` onto a document row, clamps the length to the row, and +calls `rich.ApplyHighlighting`. The old side is skipped on any row that is not `Removed` - a context +row shows the same text on both sides and is already painted by the new one. + +### SlicedPaint + +Drives that enumerator in 15 ms slices at `DispatcherPriority.Background`, checking the clock every +128 spans, calling `redraw()` after each slice. The first slice runs *before the view draws*, so the +rows on screen are already coloured; the rest follows between everything else the thread has to do. + +Deliberately **on** the UI thread, not off it: a grammar plus its cache is one state machine shared +by every document of that language, and running two at once is a data race. + +Both callers cancel the previous paint, remove the old colorizer, install a fresh +`RichTextColorizer` over an **empty** `RichTextModel` and start the paint against it - the model is +read as rows are drawn, so painting it further only needs a redraw. + +`QuickInfoView` reuses the same painter for tooltips. Its `Split` separates signature from +documentation, handling both the plain-text convention (blank line) and the markdown-fenced form some +servers return despite being asked for plain text. + +## Comment threads and suggestions + +```csharp +sealed record ThreadComment(bool IsDraft, string Author, string Body, Guid? DraftId, + string? ThreadId = null, bool Resolved = false, string? Url = null, long CommentId = 0); +sealed record ThreadData(bool OldSide, int BlobLine, List Comments, + string? OutdatedQuote = null, bool Approximate = false, string? MovedTo = null); + +public static Dictionary For(ReviewWorkspace workspace, FileDiff file); +public static List Anchors(Dictionary threads); +``` + +Keys are `"n"` / `"o"` for anchored threads and `"od"` for outdated ones (pinned at +the top of the file). Read in `Documents/`, not in a view, because a comment belongs to a line of a +blob - a fact about the review, not about how it is drawn. + +How a thread becomes a row: + +1. `CommentThreads.For` -> `Anchors` -> `model.WithThreadLines(anchors)` splices a **synthetic + marker line** per thread into a copy of `PristineModel`. +2. `ThreadElementGenerator` finds the marker text and replaces it with `ControlFactory(key)`. +3. `CommentThreadBox.Build(key, thread)` draws it: OUTDATED/MOVED banner, one header + rendered + markdown body per comment, then Reply / Resolve / Unresolve / Hide. A resolved thread with no + draft collapses to a one-line summary with a "Show" button. +4. The box's `Width` is bound to the view's bounds through an observable subscription disposed on + `DetachedFromVisualTree` - an inline object only sizes to content otherwise. + +The box sets `TextElement.FontFamilyProperty = FontFamily.Default` and `Cursor = Arrow` so prose is +not monospace and the editor's I-beam does not bleed over it. Markdown is rendered through the engine +**directly** rather than a `MarkdownScrollViewer`: a `ScrollViewer` inside an editor inline object +would nest scroll regions into every visual line. + +**Re-splicing**: when the target text equals the current one, only `Redraw` (the rows are right, only +the content changed). Otherwise the whole model is replaced, and **caret, expanded folds and open +gaps are carried by blob position, not by line number**, because a splice renumbers every line below +it. + +**The inline editor** is a `Popup` anchored to the editor. `IsLightDismissEnabled` is turned **off as +soon as the box holds text** so a click aimed at the code behind it cannot take the words with it. +Ctrl+Enter saves, Esc closes. `ScrollToMakeRoomBelow` scrolls the editor far enough that the 150 px +box fits under its anchor - replying to a tall thread otherwise puts the box over the pane below, +since a popup is an overlay and knows nothing of the editor's bounds. + +**Suggestions** write ` ```suggestion\n\n``` \n` and place the caret at the +end of the line. Refused on the base side (a suggestion replaces a line of the *new* file) and +refused a second time (the host applies one per comment). + +## Keyboard model + +Three layers, in the order a key meets them. + +### 1. View-level, tunnelling - ReviewGestures + +Handled **Tunnel** on the `TextArea` of both layouts because AvaloniaEdit binds Ctrl+Down/Up itself. +Both handlers first bail out if `e.Source` is inside a `TextBox` - the search panel lives inside the +text area. + +| Key | Action | +| --- | --- | +| `n` / `Ctrl+Down` | next hunk, **or the next file** when there is none | +| `p` / `Ctrl+Up` | previous hunk | +| `]` / `[` | next / previous file | +| `Ctrl+]` / `Ctrl+[` | next / previous commit in scope | +| `v` | mark viewed and advance | +| `o` | overview / back to file | +| `F12` / `Shift+F12` | definition / references | +| `u` | next uncovered added line | +| `b` | blame margin | +| `c` | comment at caret | +| `Alt+Left` / `Alt+Right` | back / forward | + +`StepFileAsync` marks the file being left viewed (forwards only), calls `FinishReadingAsync` off the +last file, and posts `JumpToEdgeHunk(direction)` at `DispatcherPriority.Loaded` so the next file is +entered at the edge the reader is travelling towards. + +`OnEditorKeyDown` additionally claims `Escape` (clears occurrence markers) **without marking it +handled** - it is not a review gesture and anything else listening should still hear it. + +### 2. Window-level fallback - MainWindow.OnKeyDown + +The same table again, plus `F5` (reload), `Ctrl+W` (close tab), `Ctrl++`/`Ctrl+-`/`Ctrl+0` (zoom, +accepting both `OemPlus/OemMinus/D0` and `Add/Subtract/NumPad0`), and `Ctrl+G` (Go to). It runs +**only on what came back unhandled**, so reading gestures work from the Explorer, the Commits pane +and the Tests pane too. Anything whose source has a `TextBox` ancestor is left alone - which is why +the single letters are not `InputGesture`s. + +### 3. The NativeMenu + +Declared once in `MainWindow.axaml`. On macOS the platform exports it to the system bar; everywhere +else `NativeMenuBar` draws it in the window and hides itself where the platform took it. Three +consequences the code works around: + +1. **A `NativeMenuItem` is a model object, not a control** - no `x:Name`, no generated field. The + items the code-behind needs carry a key in `CommandParameter` and are found once in the + constructor via `FindMenuItem`. +2. **There is no `ItemsSource`.** `FillRecentMenu` and `FillBuildSolutionMenu` rebuild their + submenus every time a menu opens. +3. **Gestures are written into the header text**, not set as `Gesture`. On macOS a `Gesture` becomes + a real AppKit key equivalent that fires *before* the focused text box sees the key - a `v` typed + into a comment would mark the file viewed. + +Menu refresh hooks both `top.Menu.NeedsUpdate` per top-level item **and** `MenuItem.SubmenuOpenedEvent` +bubbling, both landing in `RefreshMenus()`. On macOS the `Exit` item and the separator above it are +removed from the model entirely. + +`KeyboardShortcuts.Text` is one raw string constant opened from Help, because single letters never +appear as a gesture next to a command. **Editing a gesture means editing three places**: +`ReviewGestures.Handle`, `MainWindow.OnKeyDown`, and this string (plus the menu header text). + +## ScreenshotWatcher - the harness protocol + +A 1 s `DispatcherTimer` polling two files: `/tmp/stampeded-screenshot-request` (any instance) and +`/tmp/stampeded-screenshot-request.` (this instance only, reported in the log at startup). The +own file wins. + +**Line 1 is the target PNG path.** Every later line is a command run before the capture. The file is +deleted on pickup. The capture is of the newest visible window, so a modal dialog photographs itself. + +| Command | Effect | +| --- | --- | +| `goto::` | `NavigateToFileLineAsync` | +| `open-file:` | `OpenFileAsync` without navigating in | +| `open-range::` | `OpenLocalRangeAsync` | +| `open-url:` | `App.OpenFromUrlAsync` | +| `close-review` | `CloseReviewAsync` | +| `pane:` | `Factory.ShowPane(id)` | +| `overview`, `since-last-pass`, `commit-scope`, `commit-next`, `commit-exit` | scope commands | +| `sbs` | toggles `DiffLayoutPreference` | +| `comment`, `callgraph`, `vscode`, `ilspy-fixtures`, `impacted` | the corresponding commands | +| `highlight::` | `HighlightAtCommand` | +| `expand::` | sets `IsExpanded` on the flattened tree's row | +| `check:`, `changed-only` | toggles a named `ToggleButton` | +| `menu:
` | `RefreshMenus()` then raises `Clicked` on that `NativeMenuItem` | +| `click:` | presses a `Button`; name wins over content, newest window first | +| `press:,[:]`, `move:...`, `release:...` | one pointer gesture, **driven in written order**; each part carries its own modifiers | +| `mouse-back:,` / `mouse-forward:,` | XButton1/2 press+release | +| `wheel:,:` | wheel over a point, positive up | +| `tooltip:,` | walks up from the hit element until something carries a tip, then opens it | +| `context:,` | raises `ContextRequestedEventArgs` (a synthesized right button does *not*) | +| `type:` / `key:` | text input / `KeyGesture.Parse` on the focused element | +| `select::` | sets `SelectedIndex` + `ScrollRowIntoView` | +| `folder:` \| `folder:cancel` | pre-answers the next clone-location question | +| `caret`, `stranded` | diagnostics; `stranded` reports virtualizing-panel children that are unrealized or arranged at their own width (the ghost-row bug) | + +One shared `Avalonia.Input.Pointer TestPointer` (id 9001) - a press and its release must arrive on +the same pointer or nothing that tracks a press (drag distance, click count, capture) sees it. + +Everything asynchronous a command does runs *before* the capture but does not *finish* before it: a +second, plain screenshot request is needed to see it. + +## Theme, zoom, preferences, on-disk state + +### ThemeManager + +Singleton `ThemeManager.Current`. `UpdateTheme(name)` sets `Application.RequestedThemeVariant`, +**re-themes every registered highlighting definition first**, then raises `ThemeChanged`. + +`ApplyHighlightingColors` writes colours onto the definition's named `HighlightingColor` instances +**in place**, because the `RichTextModel` holds references to those same instances. A snapshot of the +original (light, `.xshd`-default) colours is kept per definition so Light restores exactly. Dark uses +the hand-authored `SyntaxColorPalettes.CSharpDark` where one exists, else an algorithmic HSL +conversion: invert lightness with a 1.2 curve, then desaturate colours above 0.75 saturation so they +do not glow. + +**Note:** nothing in the current UI calls `UpdateTheme` - there is no theme menu item. `Theme` is +`null` at startup, so the app runs Light. The machinery is complete and wired; only the command is +missing. + +### Zoom + +Two cooperating pieces: `MainViewModel.Zoom` -> the `LayoutTransformControl`'s `ScaleTransform` +(real relayout); and `ZoomState.PopupScale` -> a shared mutable `ScaleTransform` that `App.axaml`'s +`OverlayPopupHost` style points at. Popups are hosted by the window's overlay layer, *outside* the +scaled content, so they must be scaled where they are; a style has no data context, hence the static. + +### ScopePalette + +Two shared mutable brushes (`Accent`, `Tint`) referenced by `{x:Static}` from `App.axaml` and several +pane XAMLs. `Set(workspace)` picks blue `#3794FF` (whole change, tint opacity 0), purple `#A371F7` +(commit-by-commit, 0.10) or orange `#F0883E` (since last pass, 0.10). + +### On-disk user data + +`UserData` writes one small file per setting under `%LocalAppData%/stampeded` (on Linux +`~/.local/share/stampeded`). All reads and writes swallow `IOException` - a preference that cannot be +read is a preference that was never set. + +| File | Owner | Content | +| --- | --- | --- | +| `zoom.txt` | `ZoomPreference` | the zoom, clamped on load | +| `window.txt` | `WindowPlacement` | `x y w h normal\|maximized` | +| `recent-repos.txt` | `RecentRepos` | up to 10 paths, existing directories only | +| `tab-rows.txt` | `TabRowsPreference` | `multi` / `single` | +| `diff-layout.txt` | `DiffLayoutPreference` | `side-by-side` / `unified` | +| `merge-method.txt` | `MergeMethodPreference` | gh flag name; default `merge` (not squash - the series a review was read as is worth keeping) | +| `delete-branch.txt` | `DeleteBranchPreference` | `true` / `false` | +| `build-solutions.txt` | `BuildSolutionPreference` | tab-separated `repoPath\tsolution` lines | +| `scope-mode.txt` | `ReviewScopes` | `commit` / `whole` | + +Also written: `%LocalAppData%/stampeded/logs/test-*.log` (Tests pane), and the three +`~/.cache/stampeded/` directories owned by `Stampeded.Core`. + +`WindowPlacement.Attach(window)` only records geometry while `WindowState == Normal`, and checks a +restored rectangle against the screens that *exist now* with an 80 px corner requirement, because a +window nobody can see cannot be dragged back. + +### BusyTracker + +Thread-safe reference-counted activity tracker; `Begin(label)` returns an `IDisposable`. Publishes +`Text` (labels joined with `·`), `IsBusy` and a braille `SpinnerFrame` on an 80 ms timer, always +marshalled to the UI thread. The Merge Queue pane deliberately reuses `SpinnerFrame` for its own row +spinners rather than running a second clock. + +## Every pane + +| Pane (id) | Shows | Data source | Commands | +| --- | --- | --- | --- | +| **Explorer** (`Explorer`) | scope header + **hosts `PrFilesPaneView` and `FileBrowserPaneView`** in a 2:3 split | `workspace.Scopes`, `ReviewChanged` | enter/step/exit commit scope, since-last-pass + baseline dropdown, VS Code, open PR, open Review doc, reload, close review | +| **PR files** (in Explorer) | tree of changed files, compacted single-child directory runs; per row: change marker, name, +/- counts, comment badge (amber open / green settled), `new!` since-last-pass, coverage badge, viewed checkbox | `workspace.ReadingOrder`, `Store.IsViewed`, `Comments`, `Coverage` | selection opens the file; Toggle Viewed; `TestsFirst` checkbox | +| **File browser** (in Explorer) | `SharpTreeView` over the head worktree, lazily enumerated, skipping `bin/obj/.git/.vs/node_modules` | `workspace.WorktreePath`, rebuilt on `SemanticsChanged` | double-click opens a source view; `RevealAsync` follows the active document | +| **Change map** (`Map`) | project -> file -> member, coloured by kind | `workspace.ChangeMap` | click jumps | +| **Structure** (`Structure`) | outline of the **active** diff, members tinted by how much of their range the change touches | `GetOutlineAsync(relPath, sideText)` - the *text on screen* | double-click jumps | +| **References** (`References`) | find-references hits, `*` marking hits on changed lines; also narrates semantic load state | `ReferencesAvailable`, `SemanticsChanged` | double-click opens | +| **Call graph** (`CallGraph`) | member -> `Incoming calls` / `Outgoing calls` buckets (lazily loaded, placeholder row holds the expander open) + per-call-site rows | `GetCallsAsync`, rooted by `CallGraphRequested` | "Only members this review changes" checkbox re-roots the tree, since children are fetched once per node | +| **Comments** (`Comments`) | drafts + posted comments, preview capped at 220 chars (long bodies made the layout pass crawl) | `Comments.Changed` | Add Draft, Refresh, Delete Draft, Mark Resolved/Unresolved/All, Open on host, Approve / Request Changes / Comment | +| **Checks** (`Checks`) | CI check runs sorted fail -> pending -> rest | `Host.GetChecksAsync`; publishes back via `workspace.SetChecks` | Refresh; double-click a failed run opens its failed-step log | +| **Merge queue** (`MergeQueue`) | the shared queue: position, PR, method, who queued it, a live note column; departed entries kept below with the reason read out of the ref's history | `MergeQueueService` | Add current PR, Remove, Move up/down, Empty, Clear errors, Clear lock, **Drive** (30 s poll - the only polling in the app) | +| **Tests** (`Tests`) | failure list + live output; spinner in the pane **and in the dock tab title** | `TestService` over the head worktree | Run/Cancel, Run+Coverage (-> `SetCoverage`), Run A/B (base then head, `TestRunComparison`, opens a side-by-side output document), Impacted filter, Clear | +| **Commits** (`Commits`) | commits of the review range │ files of the selected commit │ full message | `Scopes.GetCommitsAsync`, `Git.DiffNameStatusAsync` | double-click a file opens `OpenHistoricalDiffAsync` | +| **History** (`History`) | `git log --follow` of the active file (50), or a pickaxe search over selected text | `ActiveViewChanged`, `PickaxeRequested` | double-click opens that commit's diff | +| **Run** (`Run`) | project combo (executables first) + arguments + live output | `*.csproj` filtered by `Exe`/`WinExe` | Run/Stop (`dotnet run --project`), Clear | +| **Log** (`Log`) | the `CliLog` ring buffer, newest at the bottom, file:line references turned into links | `CliLog.Sink` - **setting it replays what was written before the pane existed** | Clear, Copy Selected, Copy All | +| **PR list** | open PRs sorted: review-requested-from-me, unreviewed, reviewed, approved-by-me, drafts. Not in the default layout - instantiated by `StartDocumentViewModel` | `Host.ListOpenPrsAsync` + viewer login | double-click opens a review; `base..head` range box | + +`PassBaselineFlyout.ShowFor(sender)` builds the three-item `MenuFlyout` used by both the Explorer and +the Overview, marking the one in use with `•` and enabling only the available ones. + +## Things that will bite a maintainer + +1. **`TextEditor.ScrollToVerticalOffset` does nothing in AvaloniaEdit 12.** Always go through the + editor's `ScrollViewer`. +2. **`TextView.InvalidateLayer` does not repaint layer children.** Loop over `textView.Layers` and + `InvalidateVisual()` each. +3. **A subclassed `TextEditor` needs `StyleKeyOverride`** or it gets no template and no + `ScrollViewer`. +4. **Captured pointer releases are raised on the capturing control.** Handle `PointerReleased` on + `TextArea`, not `TextView`. +5. **A `TextBox` with `AcceptsReturn` handles Enter before your handler.** Use + `handledEventsToo: true`. +6. **Never add an `InputGesture`/`Gesture` for a single letter** - on macOS it becomes a real key + equivalent that beats the focused text box. +7. **A `NativeMenuItem` has no name, no `ItemsSource` and no tooltip flag of its own.** +8. **A disabled control answers no hit test in Avalonia**, so a tooltip on one is unreadable - wrap + it in a transparent `Border` carrying the tip. +9. **`ItemsControl.ScrollIntoView` strands containers.** Use `ListScrolling.ScrollRowIntoView`; the + `stranded` screenshot command detects the failure. +10. **Clear-then-await-then-add doubles a bound list.** Use `Collections.Replace` - the events these + lists refill from fire more than once per review. +11. **Document line numbers are not stable.** Comment-thread splices renumber everything below; carry + caret, folds and gaps as *blob* positions. +12. **The since-last-pass scope's base is a tree, not a commit.** Anything wanting history or a + checkout must check `Scopes.InSinceLastPass` first. +13. **Adding a document or pane type means three edits**: the class, an entry in `ViewLocator`, and - + for a pane - registration in `StampededDockFactory.CreateLayout` plus a `View > Panes` menu item. +14. **Adding a document command means four edits**: the `ReviewCommands` flag, both + `IReviewDocumentView` implementations, `MainWindow.RefreshMenus`, and the menu item. From 4b4b3e347a6497f5ab7ec8fa864ce64df1b904fd Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:20:53 +0200 Subject: [PATCH 2/8] Remove what nothing calls 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 --- .../Roslyn/RoslynWorkspaceService.cs | 16 ---------------- src/Stampeded.Core/Testing/TestRunComparison.cs | 1 - 2 files changed, 17 deletions(-) diff --git a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs index 29a1da6..6683fa0 100644 --- a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs +++ b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs @@ -406,22 +406,6 @@ public string ToAbsolutePath(string repoRelativePath) return full[(root.Length + 1)..].Replace('\\', '/'); } - /// Spans of identifier-like classified tokens, for clickable reference segments. - public async Task> GetIdentifierSpansAsync(string repoRelativePath, CancellationToken ct) - { - var document = GetDocument(ToAbsolutePath(repoRelativePath)); - if (document is null) - return []; - var text = await document.GetTextAsync(ct); - var classified = await Classifier.GetClassifiedSpansAsync(document, new TextSpan(0, text.Length), ct); - return classified - .Where(c => IsIdentifierClassification(c.ClassificationType)) - .Select(c => c.TextSpan) - .Distinct() - .OrderBy(s => s.Start) - .ToList(); - } - /// /// Identifier-like classified tokens as (1-based line, column, length, classification). /// Feeds both semantic colouring and the clickable reference segments; identifiers never diff --git a/src/Stampeded.Core/Testing/TestRunComparison.cs b/src/Stampeded.Core/Testing/TestRunComparison.cs index 718b995..2adabcc 100644 --- a/src/Stampeded.Core/Testing/TestRunComparison.cs +++ b/src/Stampeded.Core/Testing/TestRunComparison.cs @@ -16,7 +16,6 @@ public static TestRunComparison Compare(IReadOnlyList baseResults, I // A test name can appear once per target framework; one failing result marks // the name failing. var baseFailed = Names(baseResults, TestOutcome.Failed); - var basePresent = baseResults.Select(r => r.TestName).ToHashSet(StringComparer.Ordinal); var headFailedNames = Names(headResults, TestOutcome.Failed); var newlyFailing = new List(); From 12ba4870fd9c6a64794954e32f59b27a173af8f1 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:22:43 +0200 Subject: [PATCH 3/8] Put two doc comments back on the members they describe 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 --- src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs index 6683fa0..ebfe592 100644 --- a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs +++ b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs @@ -455,7 +455,6 @@ static async Task> ClassifyAsync(Document document, .ToList(); } - /// IDE-style quick info (signature, docs, ...) as plain text sections. /// /// This workspace's copy of a file. Token positions only mean anything against the /// exact text they were computed from, so a caller displaying some other revision has @@ -469,6 +468,7 @@ static async Task> ClassifyAsync(Document document, return (await document.GetTextAsync(ct)).ToString(); } + /// IDE-style quick info (signature, docs, ...) as plain text sections. public async Task GetQuickInfoAsync(string repoRelativePath, int position, CancellationToken ct) { var document = GetDocument(ToAbsolutePath(repoRelativePath)); @@ -610,9 +610,6 @@ public async Task> MapLinesToMembersAsync( return members.Values.OrderBy(m => m.FirstLine).ToList(); } - /// Walks up to the member users think in: method/property/field/event/ctor, - /// falling back to the containing type for lines outside any member. Null when the walk - /// leaves the type system (a line in a namespace declaration, or nothing resolvable). /// /// The member a text position belongs to. /// @@ -646,6 +643,9 @@ public async Task> MapLinesToMembersAsync( return WalkToMember(model.GetEnclosingSymbol(position, ct)); } + /// Walks up to the member users think in: method/property/field/event/ctor, + /// falling back to the containing type for lines outside any member. Null when the walk + /// leaves the type system (a line in a namespace declaration, or nothing resolvable). static ISymbol? WalkToMember(ISymbol? symbol) { while (symbol is not null From 3239f42740a2ac0135785ab7b93bbc3d0a02ef4f Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:23:05 +0200 Subject: [PATCH 4/8] Answer nothing for a path outside the tree, in both providers 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 --- src/Stampeded.Core/Infra/WorkspacePaths.cs | 47 ++++++++++++++ src/Stampeded.Core/Lsp/LspSemanticProvider.cs | 10 +-- .../Roslyn/RoslynWorkspaceService.cs | 32 +--------- .../WorkspacePathHelperTests.cs | 63 +++++++++++++++++++ 4 files changed, 114 insertions(+), 38 deletions(-) create mode 100644 src/Stampeded.Core/Infra/WorkspacePaths.cs create mode 100644 tests/Stampeded.Core.Tests/WorkspacePathHelperTests.cs diff --git a/src/Stampeded.Core/Infra/WorkspacePaths.cs b/src/Stampeded.Core/Infra/WorkspacePaths.cs new file mode 100644 index 0000000..58927ce --- /dev/null +++ b/src/Stampeded.Core/Infra/WorkspacePaths.cs @@ -0,0 +1,47 @@ +namespace Stampeded.Core.Infra; + +/// +/// The mapping between the repo-relative paths git speaks and the absolute ones a semantic +/// provider reports. Every provider needs both directions and they have to agree exactly: +/// a path that fails to map reads as "outside the tree", which silently drops every reference +/// hit and navigation target pointing at it. +/// +public static class WorkspacePaths +{ + /// + /// Absolute path of a repo-relative one. Git speaks forward slashes on every platform and + /// Path.Combine only inserts a separator without touching the ones already there, so on + /// Windows the result would keep "src/Foo.cs" while a document index is keyed on what the + /// provider reports, "src\Foo.cs" - and every lookup would miss, taking the whole semantic + /// layer down with it. GetFullPath normalises; elsewhere it changes nothing. + /// + public static string ToAbsolute(string root, string repoRelativePath) + => Path.GetFullPath(Path.Combine(root, repoRelativePath.Replace('/', Path.DirectorySeparatorChar))); + + /// + /// The root-relative form of an absolute path, or null for a path outside the root. + /// + /// Compared the way the filesystem does: on Windows a provider's spelling of a path need + /// not match how the root was spelled, and treating that as "outside" drops the answer. + /// Case still matters elsewhere, because two files of one tree may differ only in it. + /// + /// The character after the root has to be a separator, or a sibling directory whose name + /// merely starts with the root's counts as being inside it: "/repo-other/x.cs" under + /// "/repo" would answer "-other/x.cs", a path nothing in the review has. + /// + public static string? ToRelative(string root, string absolutePath) + { + string full = Path.GetFullPath(absolutePath); + string trimmed = Path.TrimEndingDirectorySeparator(Path.GetFullPath(root)); + var comparison = OperatingSystem.IsWindows() + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; + if (full.Length <= trimmed.Length || !full.StartsWith(trimmed, comparison) + || (full[trimmed.Length] != Path.DirectorySeparatorChar + && full[trimmed.Length] != Path.AltDirectorySeparatorChar)) + { + return null; + } + return full[(trimmed.Length + 1)..].Replace('\\', '/'); + } +} diff --git a/src/Stampeded.Core/Lsp/LspSemanticProvider.cs b/src/Stampeded.Core/Lsp/LspSemanticProvider.cs index 4f0de9f..d91a413 100644 --- a/src/Stampeded.Core/Lsp/LspSemanticProvider.cs +++ b/src/Stampeded.Core/Lsp/LspSemanticProvider.cs @@ -85,16 +85,10 @@ string Uri(string relPath) public event Action? StateChanged; public string ToAbsolutePath(string repoRelativePath) - => Path.GetFullPath(Path.Combine(rootPath, repoRelativePath.Replace('/', Path.DirectorySeparatorChar))); + => WorkspacePaths.ToAbsolute(rootPath, repoRelativePath); public string? ToRelativePath(string absolutePath) - { - string full = Path.GetFullPath(absolutePath); - string root = Path.GetFullPath(rootPath); - if (!full.StartsWith(root, StringComparison.OrdinalIgnoreCase)) - return null; - return full[root.Length..].TrimStart(Path.DirectorySeparatorChar, '/').Replace(Path.DirectorySeparatorChar, '/'); - } + => WorkspacePaths.ToRelative(rootPath, absolutePath); #region Documents diff --git a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs index ebfe592..0f65acb 100644 --- a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs +++ b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs @@ -372,39 +372,11 @@ public void SetTextOverlay(IReadOnlyDictionary textByRelativePat return documentsByPath.TryGetValue(absolutePath, out var id) ? solution.GetDocument(id) : null; } - /// - /// Absolute path of a repo-relative one. Git speaks forward slashes on every platform and - /// Path.Combine only inserts a separator without touching the ones already there, so on - /// Windows the result would keep "src/Foo.cs" while the document index is keyed on what - /// Roslyn reports, "src\Foo.cs" - and every lookup would miss, taking the whole semantic - /// layer down with it. GetFullPath normalises; elsewhere it changes nothing. - /// public string ToAbsolutePath(string repoRelativePath) - => Path.GetFullPath(Path.Combine(worktreePath, repoRelativePath)); + => WorkspacePaths.ToAbsolute(worktreePath, repoRelativePath); - /// - /// The worktree-relative form of an absolute path, or null for a path outside the - /// worktree. Compared the way the filesystem does: on Windows, Roslyn's spelling of a - /// path need not match how the worktree path was spelled, and treating that as "outside" - /// silently drops every reference hit and navigation target. - /// public string? ToRelativePath(string absolutePath) - { - string full = Path.GetFullPath(absolutePath); - string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(worktreePath)); - var comparison = OperatingSystem.IsWindows() - ? StringComparison.OrdinalIgnoreCase - : StringComparison.Ordinal; - // The character after the root has to be the separator, or "/repo-other" counts as - // being inside "/repo". - if (full.Length <= root.Length || !full.StartsWith(root, comparison) - || (full[root.Length] != Path.DirectorySeparatorChar - && full[root.Length] != Path.AltDirectorySeparatorChar)) - { - return null; - } - return full[(root.Length + 1)..].Replace('\\', '/'); - } + => WorkspacePaths.ToRelative(worktreePath, absolutePath); /// /// Identifier-like classified tokens as (1-based line, column, length, classification). diff --git a/tests/Stampeded.Core.Tests/WorkspacePathHelperTests.cs b/tests/Stampeded.Core.Tests/WorkspacePathHelperTests.cs new file mode 100644 index 0000000..b28b03d --- /dev/null +++ b/tests/Stampeded.Core.Tests/WorkspacePathHelperTests.cs @@ -0,0 +1,63 @@ +using NUnit.Framework; + +using Stampeded.Core.Infra; + +namespace Stampeded.Core.Tests; + +/// +/// The path mapping every semantic provider shares. Each provider used to carry its own copy, +/// and they disagreed: the one without the separator check answered a sibling directory whose +/// name merely starts with the root's, which is a path nothing in the review has. +/// +public class WorkspacePathHelperTests +{ + static string Root => OperatingSystem.IsWindows() ? @"C:\src\repo" : "/src/repo"; + + static string Under(params string[] parts) => Path.Combine([Root, .. parts]); + + [Test] + public void MapsBothWaysWithTheSeparatorsGitUses() + { + string absolute = WorkspacePaths.ToAbsolute(Root, "src/Lib/C.cs"); + + Assert.That(absolute, Is.EqualTo(Under("src", "Lib", "C.cs")), + "the platform's separators, or a document index cannot be keyed by it"); + Assert.That(WorkspacePaths.ToRelative(Root, absolute), Is.EqualTo("src/Lib/C.cs"), + "git's separators, on the way back"); + } + + [Test] + public void RejectsASiblingWhoseNameStartsWithTheRoot() + { + string sibling = Path.Combine(Root + "-other", "C.cs"); + + Assert.That(WorkspacePaths.ToRelative(Root, sibling), Is.Null, + "a path outside the tree has no repository-relative form, and answering '-other/C.cs' " + + "names a file the review does not have"); + } + + [Test] + public void RejectsTheRootItselfAndWhatIsAboveIt() + { + Assert.That(WorkspacePaths.ToRelative(Root, Root), Is.Null, "the root is not a file in itself"); + Assert.That(WorkspacePaths.ToRelative(Root, Path.GetDirectoryName(Root)!), Is.Null); + } + + [Test] + public void AcceptsARootSpelledWithATrailingSeparator() + { + // Where a root comes from decides how it is spelled, and a worktree path assembled by + // hand can carry one. + string absolute = Under("C.cs"); + + Assert.That(WorkspacePaths.ToRelative(Root + Path.DirectorySeparatorChar, absolute), + Is.EqualTo("C.cs")); + } + + [Test] + [Platform("Win", Reason = "only Windows compares paths without regard to case")] + public void IgnoresCaseWhereTheFilesystemDoes() + { + Assert.That(WorkspacePaths.ToRelative(Root, Under("SRC", "C.cs")), Is.EqualTo("SRC/C.cs")); + } +} From 4d883c08bd343f7f1b22a470cf22b95ea27dc98b Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:23:14 +0200 Subject: [PATCH 5/8] Answer a refusal wherever a failure is answered 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 --- src/Stampeded.Core/Infra/ExternalTool.cs | 42 +++++++++++++++---- .../Documents/StartDocumentViewModel.cs | 13 +++--- 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/Stampeded.Core/Infra/ExternalTool.cs b/src/Stampeded.Core/Infra/ExternalTool.cs index 5771758..6efb50d 100644 --- a/src/Stampeded.Core/Infra/ExternalTool.cs +++ b/src/Stampeded.Core/Infra/ExternalTool.cs @@ -3,18 +3,42 @@ namespace Stampeded.Core.Infra; -public sealed class ToolFailedException(string tool, int exitCode, string stdErr) - : Exception($"{tool} exited with code {exitCode}: {stdErr.Trim()}") +public class ToolFailedException : Exception { - public string Tool { get; } = tool; - public int ExitCode { get; } = exitCode; - public string StdErr { get; } = stdErr; + public ToolFailedException(string tool, int exitCode, string stdErr) + : this(tool, exitCode, stdErr, $"{tool} exited with code {exitCode}: {stdErr.Trim()}") + { + } + + protected ToolFailedException(string tool, int exitCode, string stdErr, string message) + : base(message) + { + Tool = tool; + ExitCode = exitCode; + StdErr = stdErr; + } + + public string Tool { get; } + public int ExitCode { get; } + public string StdErr { get; } } -/// An operation this tool refuses to perform, as opposed to one a CLI rejected. -/// Callers treat it like : it did not happen, and the -/// message says why. -public sealed class RefusedException(string message) : Exception(message); +/// +/// An operation this tool refuses to perform, as opposed to one a CLI rejected. +/// +/// It IS a , because every caller means the same thing by +/// both: it did not happen, and the message says why. Left as a type of its own, a refusal +/// walked past the catch clause written for the failure next to it and reached the reader as +/// a crash dialog - which is what a fork on Azure DevOps, or a worktree with uncommitted work +/// in it, did. A caller that wants to tell them apart still can, by catching this first. +/// +public sealed class RefusedException(string message) + : ToolFailedException("stampeded", Refused, message, message) +{ + /// The exit code a refusal reports. No process ran, so it is not one of anyone's: + /// -1 already means "the tool never started", and this is "the tool was never asked". + public const int Refused = -2; +} public static class ExternalTool { diff --git a/src/Stampeded/Documents/StartDocumentViewModel.cs b/src/Stampeded/Documents/StartDocumentViewModel.cs index bbf03ea..66f65e0 100644 --- a/src/Stampeded/Documents/StartDocumentViewModel.cs +++ b/src/Stampeded/Documents/StartDocumentViewModel.cs @@ -577,16 +577,17 @@ async Task RebaseAsync() : $"Rebased {row.Info.Name} onto {defaultBase}. Previous head was {result.Before[..9]} " + $"(recover with: {result.RecoveryCommand(row.Info.Name)})."; } - catch (ToolFailedException ex) - { - State.Status = $"Rebase of {row.Info.Name} failed, branch left unchanged: {ExternalTool.Explain(ex)}"; - } catch (RefusedException ex) { // Refused rather than attempted: something is already half-finished, and the - // banner below is the way out of it. + // banner below is the way out of it. Caught before the failure it is a kind of, + // because the two say different things to a reader. State.Status = ex.Message; } + catch (ToolFailedException ex) + { + State.Status = $"Rebase of {row.Info.Name} failed, branch left unchanged: {ExternalTool.Explain(ex)}"; + } finally { // Whatever happened, ask again what git is in the middle of. A rebase that @@ -931,7 +932,7 @@ async Task DeleteAsync() { deletion = await workspace.Git.DeleteBranchAsync(branch); } - catch (Exception ex) when (ex is ToolFailedException or RefusedException) + catch (ToolFailedException ex) { // Only the deletion itself is caught here. Reporting a failure for anything // that goes wrong afterwards would claim the branch is still there when it From 0ed9ee98864cc0fab14e6b2f259d7f57b7fb4304 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:23:25 +0200 Subject: [PATCH 6/8] Give a language-server request a deadline, and answer the ones left waiting 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 --- src/Stampeded.Core/Lsp/LspConnection.cs | 95 ++++++++++++++++++++++--- src/Stampeded/ReviewWorkspace.cs | 6 +- 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/Stampeded.Core/Lsp/LspConnection.cs b/src/Stampeded.Core/Lsp/LspConnection.cs index e4cb2bd..184f70e 100644 --- a/src/Stampeded.Core/Lsp/LspConnection.cs +++ b/src/Stampeded.Core/Lsp/LspConnection.cs @@ -112,6 +112,10 @@ public static async Task StartAsync( connection.PumpStdErrAsync().HandleFailure(spec.Name); connection.ReadLoopAsync().HandleFailure(spec.Name); + // A longer deadline than a question about code gets: a server installed through npx + // downloads itself on first use, and the handshake is what waits for that. Still a + // deadline, because a server that never finishes starting leaves the review saying it + // is starting one forever. var initialize = await connection.RequestAsync("initialize", new { processId = Environment.ProcessId, rootUri = LspUri.FromPath(rootPath), @@ -119,7 +123,7 @@ public static async Task StartAsync( trace = Tracing ? "verbose" : "off", initializationOptions, workspaceFolders = new[] { new { uri = LspUri.FromPath(rootPath), name = Path.GetFileName(rootPath) } }, - }, ct); + }, HandshakeTimeout, ct); connection.Capabilities = initialize.TryGetProperty("capabilities", out var capabilities) ? capabilities.Clone() : default; @@ -202,9 +206,38 @@ bool Advertises(string capability) window = new { workDoneProgress = true }, }; - /// Sends a request and waits for its answer. A server that never answers stops - /// the caller's cancellation token, not the connection. - public async Task RequestAsync(string method, object? parameters, CancellationToken ct) + /// + /// How long a request waits for its answer before it is given up on. Most callers ask from + /// the UI thread with no cancellation token of their own, so without a deadline a server + /// that stops answering takes go-to-definition and everything like it down for as long as + /// the review is open, silently and with no way back but restarting. + /// + /// Generous, because the cost of being wrong in one direction is a command that answers + /// nothing and in the other a review that cannot be read. + /// + public static readonly TimeSpan DefaultTimeout = TimeSpan.FromSeconds(30); + + /// How long the handshake may take. See where it is used for why it is not the + /// deadline a question about code gets. + public static readonly TimeSpan HandshakeTimeout = TimeSpan.FromMinutes(2); + + /// Sends a request and waits for its answer, giving up after + /// . + public Task RequestAsync(string method, object? parameters, CancellationToken ct) + => RequestAsync(method, parameters, DefaultTimeout, ct); + + /// + /// Sends a request and waits for its answer. A null waits for as + /// long as it takes, which is for the requests whose work IS the waiting - loading a + /// solution takes as long as a solution takes, and a deadline there would abandon a review + /// that was going to be fine. + /// + /// Giving up answers with nothing rather than throwing: that is what a server error already + /// does here, and it is what every caller is written for. The log is where the difference + /// between "found nothing" and "never answered" is recorded. + /// + public async Task RequestAsync( + string method, object? parameters, TimeSpan? timeout, CancellationToken ct) { if (disposed) return default; @@ -212,10 +245,15 @@ public async Task RequestAsync(string method, object? parameters, C var completion = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); pending[id] = (completion, method); var watch = Stopwatch.StartNew(); + using var deadline = timeout is { } limit + ? CancellationTokenSource.CreateLinkedTokenSource(ct) + : null; + deadline?.CancelAfter(timeout!.Value); + var waiting = deadline?.Token ?? ct; try { await SendAsync(new { jsonrpc = "2.0", id, method, @params = parameters }, ct); - using var registration = ct.Register(() => completion.TrySetCanceled(ct)); + using var registration = waiting.Register(() => completion.TrySetCanceled(waiting)); var result = await completion.Task; // Normally only what a reader would want explained - a request nobody noticed is // noise - and everything, with what came back, while tracing. @@ -225,6 +263,15 @@ public async Task RequestAsync(string method, object? parameters, C CliLog.Write(spec.Name, $"{method} -> {watch.ElapsedMilliseconds} ms"); return result; } + catch (OperationCanceledException) when (!ct.IsCancellationRequested) + { + // The deadline, not the caller: the server was asked and said nothing. Worth a line + // of its own, because from the caller's side this is indistinguishable from an + // answer of "nothing", and the two need different things done about them. + Notify("$/cancelRequest", new { id }); + CliLog.Write(spec.Name, $"{method} gave up after {watch.ElapsedMilliseconds} ms: no answer"); + return default; + } catch (OperationCanceledException) { Notify("$/cancelRequest", new { id }); @@ -260,12 +307,40 @@ async Task SendAsync(object message, CancellationToken ct) async Task ReadLoopAsync() { - var stream = process.StandardOutput.BaseStream; - while (!stopping.IsCancellationRequested) + try + { + var stream = process.StandardOutput.BaseStream; + while (!stopping.IsCancellationRequested) + { + if (await LspStream.ReadMessageAsync(stream, stopping.Token) is not { } payload) + break; + // Parsed into a document that is given back rather than left to the pool: one + // buffer per message arrives, and a review asks thousands of questions. + using var document = JsonDocument.Parse(payload); + Dispatch(document.RootElement.Clone()); + } + } + finally + { + // Nothing else will answer these. A server that ended - crashed, was killed, closed + // its output - leaves every request made of it waiting forever otherwise, and the + // panes behind them wait with it. + FailPending(); + } + } + + /// Answers every outstanding request with nothing, because the connection that was + /// going to answer them is gone. + void FailPending() + { + foreach (int id in pending.Keys) { - if (await LspStream.ReadMessageAsync(stream, stopping.Token) is not { } payload) - break; - Dispatch(JsonDocument.Parse(payload).RootElement.Clone()); + if (!pending.TryRemove(id, out var waiting) || !waiting.Completion.TrySetResult(default)) + continue; + // Shutting down is the expected end and says nothing; ending underneath a review + // that is still being read is the thing to report. + if (!disposed) + CliLog.Write(spec.Name, $"{waiting.Method} unanswered: the server's output ended"); } } diff --git a/src/Stampeded/ReviewWorkspace.cs b/src/Stampeded/ReviewWorkspace.cs index 7870b6a..a5153a2 100644 --- a/src/Stampeded/ReviewWorkspace.cs +++ b/src/Stampeded/ReviewWorkspace.cs @@ -2051,7 +2051,11 @@ async Task LoadCSharpOverLspAsync(string baseSha, CancellationToken ct) head.StateChanged += () => SemanticsChanged?.Invoke(); Semantics = head; var (replaced, removed, added) = await BaseSideTextsAsync(baseSha, ct); - await connection.RequestAsync("stampeded/loadBase", new { replaced, removed, added }, ct); + // No deadline: the server answers this once its own solution has loaded, and that + // takes as long as a solution takes. Giving up on it would abandon a base side that + // was going to arrive. + await connection.RequestAsync("stampeded/loadBase", new { replaced, removed, added }, + timeout: null, ct); BaseSemantics = new LspSemanticProvider(connection, WorktreePath!, spec.Name + " (base)") { UriSide = "base", }; From 3a5d56fe4837313150ddba14b6ee1282fbf90b53 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:23:34 +0200 Subject: [PATCH 7/8] Take three sharp edges off the paths that already handle the odd case 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 --- src/Stampeded.Core/Diff/DiffDocumentModel.cs | 5 +++- src/Stampeded.Core/Git/WorktreeManager.cs | 12 +++++++--- .../Testing/GeneratedSources.cs | 6 ++++- tests/Stampeded.Core.Tests/ThreadLineTests.cs | 23 +++++++++++++++++++ 4 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/Stampeded.Core/Diff/DiffDocumentModel.cs b/src/Stampeded.Core/Diff/DiffDocumentModel.cs index c279af1..e779469 100644 --- a/src/Stampeded.Core/Diff/DiffDocumentModel.cs +++ b/src/Stampeded.Core/Diff/DiffDocumentModel.cs @@ -231,7 +231,10 @@ public DiffDocumentModel WithThreadLines(IReadOnlyList anchors) } for (int i = 0; i < Tags.Count; i++) { - newLines.Add(sourceLines[i]); + // Tags and lines are built together and match, except where a trailing newline + // leaves one more tag than there is text; the side-by-side splice has always + // allowed for that, and a comment on the last line of such a file threw here. + newLines.Add(i < sourceLines.Length ? sourceLines[i] : ""); newTags.Add(Tags[i]); if (insertAfter.TryGetValue(i + 1, out var keys)) { diff --git a/src/Stampeded.Core/Git/WorktreeManager.cs b/src/Stampeded.Core/Git/WorktreeManager.cs index 406cb6a..ee5d258 100644 --- a/src/Stampeded.Core/Git/WorktreeManager.cs +++ b/src/Stampeded.Core/Git/WorktreeManager.cs @@ -10,6 +10,12 @@ public sealed class WorktreeManager(string repoPath) { static string CacheRoot => CachePath.For("worktrees"); + /// What a revision's worktree is called under the cache: the first nine characters + /// of whatever resolved it. Abbreviated revisions reach this too - a scope carries whatever + /// it was given - and slicing one of those blindly throws where the answer is the revision + /// itself. + static string DirectoryName(string sha) => sha.Length > 9 ? sha[..9] : sha; + /// Deletes cached worktrees of this repo except those for the given SHAs, /// then prunes git's registrations. Returns the number of directories removed. public async Task PruneAsync(IReadOnlyCollection keepShas, CancellationToken ct = default) @@ -18,7 +24,7 @@ public async Task PruneAsync(IReadOnlyCollection keepShas, Cancella int removed = 0; if (Directory.Exists(repoDir)) { - var keep = keepShas.Select(s => s.Length > 9 ? s[..9] : s).ToHashSet(); + var keep = keepShas.Select(DirectoryName).ToHashSet(); foreach (var dir in Directory.EnumerateDirectories(repoDir)) { if (keep.Contains(Path.GetFileName(dir))) @@ -48,7 +54,7 @@ public async Task PruneToRecentAsync( string repoDir = Path.Combine(CacheRoot, Path.GetFileName(repoPath)); if (!Directory.Exists(repoDir)) return 0; - var pinned = keepShas.Select(s => s[..9]).ToHashSet(); + var pinned = keepShas.Select(DirectoryName).ToHashSet(); var survivors = Directory.EnumerateDirectories(repoDir) .Where(d => !pinned.Contains(Path.GetFileName(d))) .OrderByDescending(Directory.GetLastWriteTimeUtc) @@ -60,7 +66,7 @@ public async Task PruneToRecentAsync( public async Task GetOrCreateAsync(string sha, CancellationToken ct = default) { - string dir = Path.GetFullPath(Path.Combine(CacheRoot, Path.GetFileName(repoPath), sha[..9])); + string dir = Path.GetFullPath(Path.Combine(CacheRoot, Path.GetFileName(repoPath), DirectoryName(sha))); if (Directory.Exists(dir) && File.Exists(Path.Combine(dir, ".git"))) { // Reuse counts as use: what the cache keeps is what a reader comes back to, not diff --git a/src/Stampeded.Core/Testing/GeneratedSources.cs b/src/Stampeded.Core/Testing/GeneratedSources.cs index 1f435aa..73f66d9 100644 --- a/src/Stampeded.Core/Testing/GeneratedSources.cs +++ b/src/Stampeded.Core/Testing/GeneratedSources.cs @@ -123,9 +123,13 @@ public static async Task> DiffAsync( static async Task> DiffFilesAsync(string? oldFile, string? newFile, CancellationToken ct) { // --no-index answers "differences found" with exit 1, which is the interesting case. + // A side the other does not have is diffed against the platform's empty file: naming + // the POSIX one on Windows diffs against a path that is not there, and a generator + // whose output a change adds or removes shows up as nothing at all. + string nothing = OperatingSystem.IsWindows() ? "NUL" : "/dev/null"; string diff = await ExternalTool.RunAsync( "git", - ["diff", "-U3", "--no-index", "--", oldFile ?? "/dev/null", newFile ?? "/dev/null"], + ["diff", "-U3", "--no-index", "--", oldFile ?? nothing, newFile ?? nothing], Path.GetTempPath(), ct, okExitCodes: [1]); var parsed = GitDiffParser.Parse(diff); return parsed.Count > 0 ? parsed[0].Hunks : []; diff --git a/tests/Stampeded.Core.Tests/ThreadLineTests.cs b/tests/Stampeded.Core.Tests/ThreadLineTests.cs index f7da481..f8be238 100644 --- a/tests/Stampeded.Core.Tests/ThreadLineTests.cs +++ b/tests/Stampeded.Core.Tests/ThreadLineTests.cs @@ -80,4 +80,27 @@ public void BlobLineZeroPinsThreadAtTopOfDocument() Assert.That(withThreads.DocLineFromNewLine(3), Is.EqualTo(model.DocLineFromNewLine(3) + 1)); Assert.That(lines[withThreads.DocLineFromNewLine(3)!.Value], Is.EqualTo("@@thread:n3@@")); } + + /// + /// A model carrying more tags than its text has lines. The side-by-side splice has always + /// allowed for it and the unified one threw, so a file whose text and tags disagree by the + /// trailing newline took the document down as soon as anything was said about it. + /// + [Test] + public void SplicesAModelWithMoreTagsThanLines() + { + var model = new Stampeded.Core.Diff.DiffDocumentModel { + Text = "a", + Tags = [ + new Stampeded.Core.Diff.DiffLineTag(Stampeded.Core.Diff.DiffLineKind.Context, 1, 1, null), + new Stampeded.Core.Diff.DiffLineTag(Stampeded.Core.Diff.DiffLineKind.Context, 2, 2, null), + ], + Hunks = [], + }; + + var withThreads = model.WithThreadLines([new Stampeded.Core.Diff.ThreadAnchor(false, 1, "n1")]); + + Assert.That(withThreads.Text.Split('\n'), Is.EqualTo(new[] { "a", "@@thread:n1@@", "" }), + "the missing line reads as empty rather than ending the splice"); + } } From a44814bf4328d35f96172fbfcd6276636872317f Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 16 Sep 2026 08:24:01 +0200 Subject: [PATCH 8/8] Resolve a symbol only where there is a workspace to resolve it against 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 --- src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs index 0f65acb..6e20be5 100644 --- a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs +++ b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs @@ -725,9 +725,12 @@ public async Task> FindDeclarationsAsync(string pa if (document is null) return null; var semanticModel = await document.GetSemanticModelAsync(ct); - if (semanticModel is null) + // A workspace is what SymbolFinder resolves against, and a load that failed part-way + // can leave a solution behind without one. Answering nothing is what every caller + // already handles; dereferencing it would take the pane down instead. + if (semanticModel is null || workspace is not { } host) return null; - var symbol = await SymbolFinder.FindSymbolAtPositionAsync(semanticModel, position, workspace!, ct); + var symbol = await SymbolFinder.FindSymbolAtPositionAsync(semanticModel, position, host, ct); return symbol; } @@ -747,7 +750,7 @@ public async Task> FindDeclarationsAsync(string pa return null; var semanticModel = await document.GetSemanticModelAsync(ct); var root = await document.GetSyntaxRootAsync(ct); - if (semanticModel is null || root is null) + if (semanticModel is null || root is null || workspace is not { } host) return null; var textLine = text.Lines[line - 1]; var positions = new List(); @@ -761,7 +764,7 @@ public async Task> FindDeclarationsAsync(string pa } foreach (int position in positions) { - if (await SymbolFinder.FindSymbolAtPositionAsync(semanticModel, position, workspace!, ct) is { } symbol) + if (await SymbolFinder.FindSymbolAtPositionAsync(semanticModel, position, host, ct) is { } symbol) return symbol; } return null;