Skip to content

Latest commit

 

History

History
61 lines (32 loc) · 16.3 KB

File metadata and controls

61 lines (32 loc) · 16.3 KB

DiffEngine bug audit — todo

Audit of the working tree at 9d39435c (2026-08-21). Eight area reviews, each candidate then re-verified against the code. Items tagged ran were reproduced by executing code; read items were confirmed by tracing the cited code paths and their callers; plausible items rest on documented platform behaviour that could not be executed here (no Mac or Linux box). Of the 80 original items 79 have been addressed and are listed under Fixed since the audit rather than in the sections below - merged, or in flight and marked as such. One was investigated and rejected and one was deliberately left; both say so where they sit, and they are all that is below.

Fixed since the audit

Recorded so they are not re-audited. Each was verified by a test that fails without the change, except where the entry says otherwise.

Inline patcher — trailing comment swallowed into the argument (#783); a retire ignoring its anchor and deleting the nearest call (#784); a stale line hint reaching into the next member (#785); C# having no chain terminators, so an appended Snapshot did not compile — and threw at run time where it did, since CurrentSettings rejects a converted SettingsTask (#786); an anchor never matching across mixed line endings (#787); an F# triple-quoted literal carrying no layout being unpatchable (#771); the C# lexer reading a verbatim string as a raw string and losing every call after it (#772).

Inline queue and staging — a glibc viewer resolving against musl (#788); a staging clear that could not be scoped to the framework that settled (#789).

Wire protocol and tray clientTrySendAsync with no deadline, so an unresponsive owner hung the failing test forever (#779); the listener giving up the queue because one accept failed (#790); a refused inline indistinguishable from an absent owner (#791); moves and deletes going nowhere once the tray exited (#792); tracked keys folding case on Linux, where two such paths are two files (#793); an inaccessible tray mutex becoming a permanent TypeInitializationException (#794); .NET Framework cancellation surfacing as the wrong exception (#795).

Core runner and tool definitions — P4Merge opening text diffs with the files on the wrong sides (#769); DiffTool and Definitions disagreeing about order behind an order-insensitive assertion (#796); WOW64 command lines unreadable, so no 32-bit tool was ever detected or killed (#798); AddToolBasedOn defaulting useShellExecute to true and dropping the inherited CreateNoWindow (#799); Launch and LaunchAsync resolving different tools (#800); extensions compared case sensitively (#801); a relaunch spending an instance slot after killing its own window (#802); a repeated tool name reported as "not installed", permanently (#803); $HOME never expanded, hiding a globally installed viewer (#804); a missing ps crashing every launch and kill (#805); the ps parser truncating commands and throwing on long PIDs (#806); Guard.FileExists validating the parameter name instead of the path, and an unset PATH dereferenced (#807); Cursor and Sublime Merge searched in the wrong places (#808, #809 — both additive, neither verified against an install); ExamDiff pane titles unquoted (#810); duplicate extensions, and a hardcoded Neovim path that concatenated to binnvim.exe and so could never match (#811).

DiffEngineTray — the menu and hotkey delete paths deleting unguarded, so one bad delete threw out of the click handler and skipped every remaining delete, move and snapshot (#813); a tracked process held only by id, so an accept could kill whatever later owned that pid (#814); startup registering a guessed path rather than the running one (#815); a remote discard given 500 ms against an owner that can be ten seconds inside the applier mutex, and its result dropped (#816); a plain listing taking the window command meant for a surface that has a window (#817); an unguarded Enum.Parse on a hand edited key name, which took the tray down at every login, and the same parse reading "1" as the left mouse button (#818); the max instances spinner stopping at 100 while the setting takes any ushort, so a higher one made Options unopenable (#819); settings.json deleted and then rewritten, so an interrupted save left nothing or a truncated file (#820); the issue title unencoded, so a # in a path dropped the body (#821); the menu telling its own items from the tracked ones by text, which left a group header named after one of them forever and made the disposal dead (#822).

macOS viewer — the window never ordered front on a normal launch (#774), with rebuilt dylibs (#770). Not executed, and not executable here: no Mac, and no CI job exercises the visible-window path, since PixelTests capture with hidden: true — the one path that deliberately builds no window. Still unconfirmed in the only way that counts, which is opening the viewer on a Mac and seeing a window.

Viewer core and headsDiffRows ignoring whitespace-only differences (#773); the Windows viewer blocking shutdown (#778); the tray leaving a disposed Process on a failed accept (#776); "Purge verified files" killing the tray (#775).

Docs — CLAUDE.md documented a --filter "FullyQualifiedName~ClassName" that matches nothing under this tree's runner, and a --nologo that makes any run report "Zero tests ran"; both are corrected there, along with the path to global.json.

DiffEngineTray, second pass — hot keys re-registered one at a time, so a collision on the second left the first live on its new combination while settings.json still held the old (#823).

Core — a bundled viewer path stamped on another machine taken and then found wanting, rather than passed over (#824); DiffRunner.Disabled captured at type initialisation, so a build server or AI CLI reported after that was inert (#825); the second NoWarn replacing the first, and net9.0 and net10.0 listed twice (#826); a symlinked source replaced by a regular file, and a file's permissions replaced by the umask's, because the swap is a rename (#845, in flight).

Viewer core, second pass — an F# entry showing as a new snapshot with an empty pane (#827); a removal moving the reader off what they were reading (#828); PreviousChange stepping over the block just above the viewport (#829); selecting the entry already selected throwing the reader to the top, which a right click does (#830); an identical re-send doing the same, every few seconds under a continuous runner (#831); a refusal left on a conflicted entry holding every pending delete on every later sweep (#833, in flight); an error reply from the owner closing the attached window (#835, in flight); revealing a move pointing at a file that does not exist yet (#836, in flight); two tracked files with one name never told apart (#837, in flight); a post-move cleanup reporting a move that succeeded as failed (#840, in flight).

Windows head — a tooltip left registered over the diff panes (#832); sub-notch wheel deltas dropped, so a touchpad scrolled nothing (#834, in flight); the character cell not re-measured when the display scaling changed (#841, in flight); a rewritten image at the same size leaving the previous picture on screen (#842, in flight); a focus leaving a minimised window minimised (#843, in flight); an ampersand in a solution name eaten as a mnemonic (#844, in flight).

Native heads (#846, in flight) — Escape and an outside click failing to dismiss the Linux menu, the first of them quitting the viewer instead; a minimised Linux window stopping the managed loop, since WindowShouldClose waits on events; the vertex offset ignored, so a draw list past 65535 vertices drew scrambled panes; a failed row coloured but unmarked; sub-notch scrolling dropped on Linux and points read as notches on macOS; macOS tooltips rebuilt every frame, which restarts the delay that would have shown them; and "Pending (N)" derived from the visible slice, which needed a field on the ABI.

CI and hygiene — the committed linux-x64 binary loaded by no job, which is the RID most people run (#838, in flight); an empty ViewerLaunchTests.cs at the repository root (#839, in flight).

Test-quality fixes found along the way

None of these were product bugs. All were tests that could not fail, or could fail for reasons unrelated to what they tested.

  • LaunchAndKill asserted only that the tool was gone, which FakeDiffTool self-exiting after five seconds made true regardless — it passed with ProcessCleanup.Kill short-circuited to a bare return. Now asserts the exit code (#780). The sync-named tests in that file also all awaited LaunchAsync, leaving DiffRunner.Launch uncovered (#781).
  • ContentIsNeverObservedHalfWritten cancelled its own reader through Task.Run(delegate, token), which cancels the scheduling rather than the delegate. Main was red on it for two days; fixed directly on main.
  • ASlowExchangeDoesNotBlockTheNext, the two TryTerminateProcess tests (#797) and the listener drain in Wait (#812) all set fixed timeouts sized for an idle machine. CI runs six assemblies at once on two cores.

Five separate flaky tests in one day, all the same shape. If more appear, the durable fix is one shared under-load budget for the suite rather than a constant per site. ViewerProtocolTests is now internally consistent; the tray and viewer suites still have their own numbers.

What is left

Two, and both are here on purpose: one remedy was investigated and rejected, and one costs more than it is worth today. Everything else the audit found is merged or in flight, with the pull request named beside it above.

Core runner and tool definitions (src/DiffEngine)

  • Low · read (remedy rejected)ProcessCleanup.cs:27: commands is written in exactly one place, Refresh(), and nothing in the library calls it after the static constructor. LaunchProcess (DiffRunner.cs:263) returns process.Id and drops the rest, and Kill terminates without removing the entry, so the index stays the startup snapshot for the life of the process in both directions: a tool launched later is invisible, and a tool killed earlier still reads as running. Originally filed as Medium with "refresh in Kill and TryGetProcessInfo"; that was wrong on both severity and remedy. In the intended flow a pair is launched in one process and killed in a later one, where the startup snapshot does have it, so the gap needs the same pair twice in one process (a retry attribute, a reused test host, a long lived embedding host) — and a re-scan costs ~14 ms on every launch and every kill to close it. If it is ever worth closing, record the launch and drop the entry on kill rather than re-indexing.

Native heads (native/, src/DiffEngineViewer/Native)

  • Low · read (left deliberately)deview.cpp:797,814,976,768: Selectable, TableSetupColumn and the menu labels pass user text straight through ImGui's ## ID convention, so a test name containing ## renders truncated on Linux only. The remedy - an empty-ID selectable plus TextUnformatted - changes how every queue row is drawn, and the Linux pixel baselines can only be regenerated on CI, so a Low severity truncation that needs a ## in a test name would be paid for by rewriting every Linux snapshot in the repository. Worth doing next time those baselines are being regenerated anyway.

Checked and found sound

Not bugs, recorded so they are not re-audited: protocol framing and base64 escaping on both ends; InlineApplier mutex, BOM/EOL/encoding round trip and atomic swap; InlineQueue fold/settle/conflict invariants; InlinePatchFile parsing; PiperServer resilience and PiperClient JSON escaping; Tracker dictionary races and AsyncTimer re-entrancy; tray shutdown order and single-instance mutex; ViewerSession clamping, folding, menu invalidation and the Rebuild/Sync split; ImageHeader sniffing for every format; CommandLine rejections; C# raw-string delimiter widening and escapes; F# nested comments and char literals; patcher index arithmetic; native struct layouts, enum numbering, DEVIEW_VERSION parity and buffer lifetimes; PaneScroll arithmetic; WinForms GDI disposal; the CI workflow scripts apart from the coverage gap above.