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. 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/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.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/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.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 29a1da6..6e20be5 100644 --- a/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs +++ b/src/Stampeded.Core/Roslyn/RoslynWorkspaceService.cs @@ -372,55 +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('\\', '/'); - } - - /// 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(); - } + => WorkspacePaths.ToRelative(worktreePath, absolutePath); /// /// Identifier-like classified tokens as (1-based line, column, length, classification). @@ -471,7 +427,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 @@ -485,6 +440,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)); @@ -626,9 +582,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. /// @@ -662,6 +615,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 @@ -769,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; } @@ -791,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(); @@ -805,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; 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/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(); 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 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", }; 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"); + } } 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")); + } +}