refactor(782): consolidate the PR #778 post-merge review residuals into one delivery

[drmoisan]

Suggested title

refactor(782): consolidate the PR #778 post-merge review residuals into one delivery

Summary

• Consolidates every actionable finding from the three-phase post-merge code review of PR fix(UtilitiesCS): guard UiThread dispatcher null-race in ProgressTrackerAsync #778 into a single Refactor delivery, so that none is lost when the Bug: uithread-dispatcher-null-race-progresstrackerasync #584 feature folder is archived. The review returned zero Blocking findings; this closes the 7 Should-fix items, 25 Nits, and 2 optional cleanups it left behind.
• Makes UiThread.Dispatcher read its backing field exactly once, documents the deliberate non-lazy contract in XML comments, and routes both not-initialized throw sites through one shared internal const message that names only the public Init() entry point and states the STA requirement.
• Reduces the UtilitiesCS.Test manipulation of the UiThread._dispatcher static from four independent reflection sites to a single disposable install scope with one reflection acquisition, and moves QuickFiler.Test onto the existing UiThreadDispatcherFixture.Current accessor.
• Removes the last unshut dispatcher on a pooled MTA worker thread, splits the one test file that exceeded the repository's 500-line limit, and corrects comments and reason strings that still described the pre-fix(UtilitiesCS): guard UiThread dispatcher null-race in ProgressTrackerAsync #778 NullReferenceException mechanism.
• Repairs nine internal inconsistencies in the Bug: uithread-dispatcher-null-race-progresstrackerasync #584 feature folder's audits and evidence, including a misstated formatter command, an evidence count that was wrong by four, and fifteen EXIT_CODE: fields that did not parse as integers.
• Withdraws one finding, C03, after execution measured a reproducible regression that it caused; the withdrawal is recorded with its bisect and promoted forward rather than dropped.

Why

PR #778 changed UiThread.Dispatcher from a null-returning accessor to one that throws InvalidOperationException. The subsequent three-phase review confirmed the fix and found no functional regression, but produced 26 code findings plus a group of documentation and evidence defects. Individually none justified its own delivery. Collectively they were about to be archived along with the #584 feature folder and lost.

Two of the findings were latent defects in the new code rather than hygiene: a test that created a dispatcher on a pooled MTA thread and never shut it down, and a double read of the non-volatile static inside the new getter, where the guard and the return value could observe different values.

What Changed

Production, 5 files, +52/-15

• UtilitiesCS/Threading/UiThread.cs — single-read getter (C02); internal const string DispatcherNotInitializedMessage shared with the sibling throw site (C20); message names only Init() (C06) and states the STA/UI-thread requirement (C09, message half); <summary>, <remarks> and <exception> XML documentation on a file that previously carried none (C08); a comment recording why this accessor deliberately does not self-heal where its two siblings do (C05).
• UtilitiesCS/OutlookObjects/Folder/WpfDispatcherYield.cs — throw routed through the shared constant; the false comment corrected (C20).
• UtilitiesCS/Threading/ProgressTracker.cs, ProgressTrackerAsync.cs — the InvokeAsync lambdas use the already-captured dispatcher instead of re-reading the static (C23).
• TaskMaster/Ribbon/RibbonViewer.EngineCommands.cs — the two now-dead dispatcher != null comparisons removed (C01).

Tests, 10 files plus the test project, +690/-385

• New UtilitiesCS.Test/TestHelpers/UiThreadDispatcherScope.cs, one disposable install scope holding the only reflection acquisition; the four previous sites migrate to it and it restores the prior value even when that value was null (C12, C13).
• UtilitiesCS.Test/Threading/ProgressTracker_Tests.cs split into a partial class pair, following the four-file TimeOutTask_Tests precedent, so both halves sit under the 500-line limit and every fully-qualified test name is preserved (C16, C15). The new file is registered as a <Compile Include>.
• UiThread_Tests.cs obtains its sentinel dispatcher on a dedicated STA thread inside a using, so BeginInvokeShutdown and the thread join run on every exit path including a failing assertion (C10, C11).
• New regression tests for the WpfDispatcherYield production fallback and for both ProgressTrackerAsync.InitializeAsync and the synchronous ProgressTracker.Initialize (C21, C26).
• EmailMoveMonitorTests.cs reads through UiThreadDispatcherFixture.Current instead of its own FieldInfo, so the order-independence guard fails rather than silently degrading if the field is renamed (C18, C25).
• Cleanup added to IdleActionQueue_Tests, with the class serialized because the cleanup mutates process-global Application.Idle state shared with two sibling classes (C14).
• Stale NullReferenceException prose corrected in IdleAsyncQueue_Tests.cs and QfcItemController.InitializationTests.Part2.cs (C19, S2-1).

Documentation and evidence, 23 files in the #584 folder

Ordering prose the timestamps contradict (S3-1); the formatter-command cells, which claimed a whole-tree run where the plan ran CSharpier over six explicit paths (S3-2); the evidence count corrected from 34 to the tracked figure of 38 (S3-3); a filename/Timestamp: mismatch noted in place rather than by renaming a committed file (S3-4); fifteen non-integer EXIT_CODE: fields normalized (S3-5); spec.md status and file lists reconciled (S3-6); call-site counts corrected to 49 live reads across 25 production files, with the review body's 26-file figure recorded as a divergence rather than silently adopted (S3-7); six evaluative spans replaced with neutral phrasing (S3-8); and the ProgressTrackerAsync synchronization follow-up recorded as discharged by the shared install scope rather than by the new test (S3-9).

Architecture / How It Fits Together

UiThread remains a static holder for UI-thread state captured once at host startup. The change tightens two things around it without altering the initialized path.

First, the not-initialized precondition now has one message with one owner. UiThread.Dispatcher and WpfDispatcherYield's local guard both throw InvalidOperationException(UiThread.DispatcherNotInitializedMessage). The constant is internal, both throw sites are in the same assembly, and UtilitiesCS.Test reaches it through the existing InternalsVisibleTo grant. The WpfDispatcherYield message loses its caller-specific tail as a direct consequence, which is intended and is asserted by the tests rather than left to inspection.

Second, test manipulation of the process-global _dispatcher static is funnelled through UiThreadDispatcherScope, which is the only place in UtilitiesCS.Test that acquires the FieldInfo. It is IDisposable, restores the captured prior value on dispose including when that value was null, and offers an explicit InstallNull() so a test can establish a known prior rather than depending on whatever a sibling assembly left behind. That matters because QuickFiler.Test and UtilitiesCS.Test share a test host in the full-suite run.

UiThread.Init() itself is byte-identical to its base form. See Follow-ups.

Verification

Completed

The full C# toolchain passed in a single uninterrupted pass, in the order CLAUDE.md requires:

• dotnet tool run csharpier format . — exit 0, Formatted 1583 files, before and after tree images byte-identical.
• dotnet tool run csharpier check . — exit 0, Checked 1583 files.
• msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true — exit 0, 0 Warning(s), 0 Error(s).
• msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true — exit 0, 0 Warning(s), 0 Error(s), all 19 projects recompiled so the gate was not vacuous.
• Nine test assemblies under dotnet-coverage collect — Total tests: 7000, Passed: 7000, Failed: 0.

Coverage did not regress. First-party line coverage is 112351/132961 and branch 26498/33480, byte-equal to the baseline on all four counters with equal denominators. Changed-line coverage over the implementation phases was 7 of 7 executable lines.

Two review cycles were run against the branch. Both returned PASS with zero Blocking findings. The second re-executed every gate independently rather than reading the reported figures, re-aggregated four Cobertura documents under two selections, and read the TRX counters directly.

UtilitiesCS/Threading/UiThread.cs reads 77.11% to 76.83% on modified-file coverage. That is arithmetic, not a regression: the baseline and post-change uncovered line sets are the identical 19 lines, and the movement comes from a covered three-line wrapped throw collapsing to one line against a fixed residue in untouched host-bound members. No line moved from covered to uncovered.

Recommended

• Re-run the four toolchain commands above.
• vstest.console.exe over the nine test assemblies with /InIsolation.

Note for local reproduction: this repository pins its SDK through global.json, so dotnet must be invoked with the repo-local .dotnet-sdk on PATH and DOTNET_ROOT set. Local runs also exclude four UtilitiesCS.Test shell-icon classes that stall SHGetFileInfo on some workstations; CI covers them, so the CI total exceeds the 7000 figure above.

Backward Compatibility / Migration Notes

No public API signature changed. UiThread.Dispatcher keeps its non-nullable declared type and its private setter.

One observable change: the InvalidOperationException message text. It now names only the public Init() rather than the private Initialize(), states the STA/UI-thread requirement, and is shared by both throw sites. WpfDispatcherYield's previous message carried a caller-specific tail, "before yielding folder tree work", which the shared constant necessarily drops. Both assertions on that message were converted from a wildcard to an exact comparison against the constant, and the conversion was verified by mutation: appending the removed tail at the throw site makes the test fail, and reverting makes it pass.

No file was renamed and no evidence file's Timestamp: was altered.

Risks and Mitigations

• Shared message constant reduces diagnostic specificity at the WpfDispatcherYield site. Intended per the review finding. Mitigated by the exact-match assertions, which now detect a caller-specific tail reappearing at either site.
• The new install scope serializes test writers of a process-global static. The scope restores the prior value on every exit path including the null case, and the affected classes carry [DoNotParallelize]. The full nine-assembly suite passes.
• The ProgressTracker_Tests split could have dropped a test. Mitigated by asserting all 24 original fully-qualified test names are present and passing after the split; the partial class shape preserves every name.
• Rollback is a plain revert of this branch. No migration, no data change, no configuration change.

Review Guide

Suggested order:

1. UtilitiesCS/Threading/UiThread.cs — the shared constant, the single-read getter, and the XML documentation.
2. UtilitiesCS/OutlookObjects/Folder/WpfDispatcherYield.cs — the second throw site.
3. UtilitiesCS.Test/TestHelpers/UiThreadDispatcherScope.cs — the new seam, and in particular its dispose semantics.
4. The four migrated reflection sites and EmailMoveMonitorTests.cs.
5. The ProgressTracker_Tests split, which is the largest diff and is a mechanical move.
6. The Bug: uithread-dispatcher-null-race-progresstrackerasync #584 documentation corrections, which are independent of the code and can be read separately.

The ProgressTracker_Tests.cs / ProgressTracker_ReportAndViewerTests.cs pair is the noisy part of the diff: 288 added and 249 removed lines that are almost entirely a move.

The delivery's own audit trail is under docs/features/active/2026-09-05-pr-778-post-merge-review-residuals-782/, including the plan, the research record, the evidence tree, and both review cycles' artifacts.

Follow-ups

• Issue Bug: uithread-init-accepts-non-sta-callers #787 carries the behavioural half of finding C09: UiThread.Init() accepts a call from any thread and installs a non-pumping dispatcher into set-once process-global state. Deliberately out of scope here, because it is a production behavior change that breaks one existing test call site.
• Issue Bug: uithread-init-latch-not-rearmed-after-failed-initialize #788 carries finding C03, which this delivery withdrew. C03 asked that the single-shot latch be re-armed when Initialize() throws so a failed initialization could be retried. Implementing it caused DictionaryExtensions_Tests.TryAddValuesAsync_UpdatesExistingValue to fail reproducibly, bisected to the single re-arm line: with it, UtilitiesCS.Test plus TaskMaster.Test returned 5179/5180 at a 21-second duration; without it, 5180/5180. The mechanism is that the UiSyncContext and AutoScaleFactor getters call Init() lazily, so re-arming turns a one-time expensive WinForms failure into a repeated one on every accessor read, starving the thread pool. The entry records the measurement and three candidate approaches so a future attempt does not repeat the naive form.
• Two items belong upstream in the drm-copilot repository rather than here, and are recorded in the delivery's evidence: finding S4-1, stale notes under .claude/agent-memory/task-researcher/, and the S3-1 request to define Timestamp: semantics in the evidence-and-timestamp-conventions skill. Nothing under .claude/ is modified by this branch.
• Two of this delivery's own documents, the plan and the research record, embed an absolute host path in prose describing where a command ran. Left in place deliberately: both are completed historical records, 827 documents already on main carry the same pattern, and no repository rule codifies the prohibition. Disclosed rather than silently dropped.
• Findings C04, C07, C17, C22, C24 and S4-2 were refuted by the review and need no action.

GitHub Auto-close

• Closes Refactor: pr-778-post-merge-review-residuals #782