Repository navigation
Address full code review findings: release workflow, diff memory, comparison races and reliability #29
Description
Track the findings from the full code review of revision 7e450fc (PR #28). The review below refers to that snapshot; check for intervening fixes before implementation.
Fix checklist
- F1 — Fix tagged-release version extraction (P1)
- F2 — Bound line-diff memory/work (P1)
- F3 — Preserve comparison identity across Explorer file reads (P2)
- F4 — Prevent confirmation from restoring automatically cleared text (P2)
- F5 — Coordinate shared resources across sessions (P2)
- F6 — Accept Explorer commands without waiting for previous file processing (P2)
- F7 — Handle dotnet-hosted startup commands (P3)
Full review
ClipboardDiff code review
Date: 6 October 2026
Repository: https://github.com/stuartd/ClipboardDiff
Reviewed revision: 7e450fcacd8d713d453c4eee73e7cb305ba53874 — PR #28, including start at login, menu simplification, and dropdown styling.
Base main revision: 1cad67438586b29fef826a4b63e3475471934248.
This report describes the reviewed snapshot, not subsequent repository changes. No application code was changed during this review.
Assessment
There are two high-priority issues to address before a release: the tagged-release workflow loses its version, and the line diff algorithm has unbounded quadratic memory growth for dissimilar inputs. There are also asynchronous comparison and privacy problems that are not covered by the existing tests.
| ID | Priority | Finding |
|---|---|---|
| F1 | P1 | Tagged releases use an empty release version |
| F2 | P1 | Dissimilar files can require gigabytes of diff working memory |
| F3 | P2 | Pending Explorer reads can compare the wrong capture or replace a newer comparison |
| F4 | P2 | External-viewer confirmation can resurrect automatically cleared text |
| F5 | P2 | Concurrent sessions interfere with live temporary files and Explorer registration |
| F6 | P2 | A slow Explorer request prevents subsequent single-file commands from connecting |
| F7 | P3 | Start at login registers only dotnet.exe when launched through the dotnet host |
P1 means fix before release; P2 means a functional defect under the stated conditions; P3 means a narrower compatibility issue. Findings are supported by source inspection. Windows reproduction scenarios below are proposed verification steps, not claims that those desktop scenarios were executed.
F1 — Tagged releases use an empty release version
Priority: P1. Already present on main.
Location: .github/workflows/release.yml, lines 29–49.
The “Validate release tag” step assigns $tag but then exports $Matches.version. There is no regex match left in this step to populate that capture. Consequently RELEASE_VERSION is empty.
The packaging script treats an empty version as a request to use the project version, currently 1.0.0. The later release step still constructs archive paths from the empty environment variable, such as ClipDiff--win-x64-net10.zip. Those paths do not match the produced files, so publishing the tagged release fails. Even without that filename failure, the build would not reliably carry the tag's version.
Fix: Explicitly derive the version from the tag and validate it against the packaging script's accepted format. If strict tag validation is deliberately unwanted, stripping the leading v still needs to be explicit. Export the same resolved value used for both publishing and asset paths.
Verification: Exercise the version-extraction step with v1.2.3 and a prerelease tag; assert the exported value, assembly version, and archive names agree. Ordinary branch CI does not exercise this tag workflow.
Reference: PowerShell automatic variables: Matches. The capture variable is populated by regex matching; assigning $tag does not populate it.
F2 — Dissimilar files can require gigabytes of diff working memory
Priority: P1. Already present on main.
Location: DiffEngine.cs, lines 130–175.
ShortestEditScript clones the entire frontier array at every edit distance and retains every clone for backtracking. The 4096 used to initialize the list is a capacity hint, not a limit.
For N unrelated lines on each side, edit distance is 2N. The retained integer-array payload alone is:
4 × (4N + 3) × (2N + 1) bytes
| Lines per side | Retained trace-array payload |
|---|---|
| 1,000 | 32,040,012 bytes |
| 5,000 | 800,200,012 bytes |
| 10,000 | 3,200,400,012 bytes |
These values were calculated from the allocation pattern; they are not measured process-memory benchmarks. They exclude the original frontier, array headers, source strings, edits, rows, and highlighting.
Small text files can contain 10,000 lines while remaining far below the 16 MiB input limit. Inline highlighting has budgets, but they apply only after this unbounded line comparison. Cancellation also does not impose a resource limit on an otherwise current request.
Impact: Severe memory pressure or a failed comparison. The current comparison error handler does not handle allocation failure, and the caller discards the returned task; a useful user-facing failure is not assured.
Fix: Use a bounded-memory diff implementation or impose an explicit work/memory budget with a documented fallback. Avoid attempting to solve this solely by catching OutOfMemoryException after allocations have exhausted memory.
Verification: Add tests for thousands of unrelated short lines, wholly inserted/deleted inputs, and cancellation during expensive work. Measure allocation and execution time under a stated budget.
F3 — Explorer reads can compare the wrong capture or replace newer work
Priority: P2. Already present on main.
Location: AppController.cs, lines 416–535.
The single-file command checks only that a current capture exists before reading. It does not retain the capture identity. After the read completes, AcceptDirectText promotes whatever is current at that later instant.
For example:
- Capture A and invoke “Compare with current ClipDiff capture” for slow file B.
- Copy C while B is loading.
- B completes and the app compares C against B, although the action was initiated against A.
The two-file command has a related ordering problem: separate reads use only the shutdown token. A slow pair requested first can finish after a fast pair requested second, replace the history, and open the stale comparison. LatestComparison protects diff computation after reading; it does not protect this earlier file-loading stage.
Fix: Give comparison requests an identity spanning both file loading and diff publication. Snapshot the single-file source capture, and either compare that explicit pair or reject completion if the source was invalidated. Cancel or ignore stale file-read completions.
Verification: Use a controllable asynchronous file reader to complete two requests in reverse order. Also change the clipboard during a single-file read. Assert that neither case silently substitutes an unintended pair.
F4 — Confirmation can resurrect automatically cleared text
Priority: P2, privacy-related. Already present on main.
Location: AppController.cs, lines 343–365, automatic clearing, lines 221–228, and ConfirmExternalDiffRisk.
ShowDiff captures references to both entries before displaying the native modal external-viewer warning. While the dialog processes messages, an automatic clipboard clear can remove the current entry and call ClearComparison. The local references in ShowDiff remain valid.
When the dialog returns, there is no invalidation check. Accepting it can write the cleared text to the external-viewer files. Cancelling can instead reopen it in the built-in viewer via the fallback path.
Fix: Track a privacy invalidation generation across confirmation. If clearing invalidated the captured request while the warning was open, return without either external launch or built-in fallback. Apply the same guard to any other nested-dialog path that can publish a saved comparison.
Verification: With warning acknowledgement unset, create a comparison containing a value eligible for recent-clear handling, open the warning, and clear the clipboard while it remains open. Check both OK and Cancel: neither should restore or export the removed value. This needs a Windows message-loop integration test.
F5 — Sessions share resources that their cleanup treats as exclusive
Priority: P2, conditional on the same user running ClipDiff in concurrent sessions. Already present on main.
Location: App.xaml.cs, ExternalDiffWorkspace.cs, lines 86–119, and ExplorerContextMenuRegistration.cs.
The Local\ClipDiff mutex permits one instance per session. However, the temporary root and HKCU Explorer keys are shared by the user's sessions.
Every new launcher calls CleanupStaleDirectories, which deletes all comparison directories without checking whether another session still owns them. An external viewer in session A can lose its backing files when ClipDiff starts in session B, where file sharing permits deletion.
Similarly, quitting one instance removes the shared Explorer registrations, even if another session still needs them. The remaining instance caches registration state, so an ordinary subsequent update need not recreate the missing keys.
Fix: Choose and implement a consistent ownership model. Use live-owner/session information for temporary-directory cleanup and coordinate shared registry registration across sessions. Do not infer that a directory is stale merely because this instance did not create it.
Verification: On a Windows Server setup allowing concurrent sessions for the same user, open an external comparison in A, start ClipDiff in B, then quit B. A's backing files and Explorer commands should remain usable. No cross-session desktop test was executed here.
F6 — Slow single-file commands block subsequent Explorer requests
Priority: P2. Already present on main.
Location: ExplorerCommandServer.cs, lines 53–73 and ExplorerCommandClient.cs, lines 9–31.
The server allows one pipe instance and awaits the entire file handler before disposing the connection and accepting another. A slow network-file read therefore leaves no listener available for the next request. The next client gives up after two seconds; the application startup path ignores its false return value, so the action disappears without useful feedback.
A connected client that never finishes its message can also monopolize the listener because reads use only the application-shutdown token. This is primarily a reliability issue, not a claimed privilege-escalation vulnerability.
Fix: Separate bounded message receipt from comparison work, promptly accept the next connection, and apply a per-message deadline. Coordinate the resulting requests with the ordering policy in F3. Surface failed delivery rather than silently exiting.
Verification: Hold a first handler pending beyond two seconds, issue another command, and verify it is accepted or explicitly rejected. Test a partial message followed by a healthy client.
F7 — Dotnet-hosted startup registers the host without the application
Priority: P3. Introduced in PR #28.
Location: StartupRegistration.cs, lines 35–51.
When launched using dotnet ClipDiff.dll, Environment.ProcessPath is the dotnet host. Enabling startup writes only that quoted path. Signing in launches dotnet without the application assembly, so ClipDiff does not start.
The Explorer command builder already handles the equivalent host/entry-assembly case, but startup registration does not. Packaged self-contained and apphost-based launches are not affected.
Fix: Share a tested application-command builder with Explorer integration, or explicitly disable startup registration for unsupported host launches.
Verification: Test direct executable launch and dotnet-hosted launch, including spaces in both host and assembly paths.
Further quality and performance observations
These are improvements or validation gaps, not additional confirmed release blockers.
- Controller integration coverage: Existing tests cover history policy, privacy inspection, hotkey transactions, diff cancellation, filename handling, settings, and native Shell integration. There are no equivalent controller-level tests covering the complete asynchronous read/confirmation/publication sequences in F3–F4. Add injectable file-reading and confirmation boundaries rather than trying to test these races through the whole desktop.
- Large clipboard values: Raw Unicode clipboard reads allow allocations up to the native size check near 2 GiB. Preview generation replaces and trims the whole string to display at most 120 text elements, and it is called again during presentation updates. Rejecting excessive input or scanning only the required preview prefix would make memory and UI latency more predictable.
- Large viewer rows: Virtualization reduces the number of rows materialized, but each visible row contains RichTextBox/FlowDocument objects. A single very long wrapped line can still be expensive. Profile large minified JSON and long-line inputs on Windows.
- External-viewer lifetime: Cleanup follows the launched process plus a three-second grace period. Programs that hand work to another process need real integration tests; argument-array tests alone do not establish that the files live long enough. The README already acknowledges this limitation, so it is not presented here as a newly confirmed defect.
- Settings feedback: External viewer selection and privacy acknowledgement ignore
TrySavefailure, unlike shortcut and startup handling. A failed preference save should receive restrained feedback rather than appearing persistent. - UI verification: The new ComboBox templates compile in CI, but that does not establish correct popup placement, contrast, focus, mouse selection, or keyboard behaviour. Check both Windows themes, high contrast, DPI scaling, and keyboard navigation. Other button styles still rely on default Windows templates and deserve the same visual check.
- Maintainability: AppController combines lifecycle, history updates, Explorer I/O, settings, modal confirmation, and presentation. Extract the comparison request coordination first; it directly enables tests for the real defects. A large framework rewrite is unnecessary.
- Formatting/documentation: The repository still mixes tabs and spaces and retains legacy pause-related implementation/test language after the controls were removed. Normalize touched code to AGENTS.md and distinguish internal policy from exposed UI. README also says the Windows policy tests run on macOS even though their project now targets Windows and references WPF; update those build instructions.
What is working well
The core comparison/history code is separated from Windows I/O. The privacy inspector checks advisory exclusion markers before requesting text or file paths. Copied-file conversion has a byte limit and encoding/binary handling. Process arguments use ArgumentList rather than a shell-concatenated command. Inline highlighting has grapheme-aware ranges and explicit work budgets. The latest-diff publication guard and hotkey replacement transaction have useful tests. Native Shell tests go beyond compilation and exercise real Shell selection/menu behaviour.
The review found no intentional clipboard upload, telemetry, or hidden clipboard database in the inspected application paths. This is a source-review observation, not a security certification.
Validation and limitations
- Inspected core algorithms, clipboard/native interop, Explorer IPC and COM/native extension, external diff discovery/launch/cleanup, settings/startup, hotkeys, tray/viewer code, test coverage, and build/release configuration.
- Verified the local reviewed tree matches remote PR revision
7e450fc. - Confirmed both existing Windows CI runs for that revision succeeded: push build and PR build. These run the repository's build/test/package script; they do not exercise the broken tag-version extraction.
- Parsed all XAML files as XML and calculated the diff trace allocation sizes from the source.
- No .NET SDK, PowerShell runtime, or Windows desktop was available locally. No new C# tests, actual login tests, modal-dialog race reproductions, cross-session tests, or visual smoke tests were executed during this review.
- Reproduction scenarios above are specifically intended to convert source-confirmed defects into regression tests. Existing green CI is not evidence that those scenarios pass.
Recommended order: fix F1 and F2 before tagging a release; address F3 and F4 together with controller-level regression tests; then handle session/IPC reliability and the dotnet-hosted startup edge case.
Activity