fix(quickfiler): resolve seven QfcCollectionController defects and remove twelve unreachable members

[drmoisan]

Fix seven QuickFiler collection-controller defects and remove twelve unreachable members

Summary

• Fixes the re-entrancy counter leak on the exceptional exit path of RemoveSpecificControlGroupAsync (Bug: qfc-collectioncontroller-removespecificcontrolgroup-counter-leak #286), which previously left the guard elevated after a throw and blocked all subsequent group removals for the life of the controller.
• Removes twelve unreachable members from QfcCollectionController (Bug: qfc-collection-controller-unreachable-load-paths #468) after proving no caller exists, including no reflective caller.
• Repairs four move-path defects (Bug: qfc-collection-move-diagnostics-defects #469): a null item controller crashing diagnostics, a diagnostics array sized from the wrong collection, an unordered move collection read by index, and an undocumented undo-stack contract.
• Repairs three conversation-path defects (Bug: qfc-collection-conversation-index-defects #470): negative-index subscripting in two paths, an unreconciled insertion count above the reserved allocation, and unguarded SetVisualDigits.
• Corrects the sign error in the item-panel height arithmetic on conversation collapse (Bug: qfc-collection-eliminate-space-sign-error #471), so eliminating space now shrinks the panel instead of growing it.
• Closes the background-task drain window (Bug: qfc-collection-background-task-and-catch-defects #473) with an atomic bag swap, and stops a second Subject read after a failed move.
• Replaces a runtime downcast with a compile-time interface binding and makes move readiness inspectable without presenting a modal dialog (Bug: qfc-collection-controller-coupling-and-modal-getter #474).
• Adds 28 MSTest methods across five new test files; the full suite is 6581/6581 green.

Why

Seven issues filed against QuickFiler/Controllers/QfcCollectionController.cs shared one root cause pattern: the class had accumulated logic that could not be exercised in isolation, so defects in it were found by reading rather than by testing. Two of the seven are latent rather than observed, and this PR states that plainly rather than overclaiming impact:

• Bug: qfc-collection-background-task-and-catch-defects #473 defect 1 is latent under the current call graph. Both Add pairs occur in the same method body strictly before their WhenAll; no other member adds to the bag; and each of the three production construction sites (QfcFormController.Actions.cs:49, :83, :139) creates a fresh controller that is awaited. The fix closes the window for a future caller. It does not repair an observed production failure, and nothing here should be read as fixing an intermittent hang.
• Bug: qfc-collection-controller-coupling-and-modal-getter #474 is latent in the current single-implementation configuration. QfcFormController is the only production implementation of the parent role, so the runtime downcast could not throw today. The value of the change is that a second implementation cannot reintroduce the hazard.

The remaining five are real, reachable defects.

What Changed

Core logic (2 production files)

QuickFiler/Controllers/QfcCollectionController.cs (+493 / -405) and QuickFiler/Interfaces/IQfcCollectionController.cs (+13 / -0, XML documentation only).

Dead-code removal (#468). Twelve unreachable members were deleted. Absence of any caller was verified three ways: the solution compiles without them; a reflective-caller search covered 398 build-input files and all 42 GetMethod( call sites; and the full suite stayed green across the removal commit. This removal is not offered as a coverage improvement — QfcCollectionController carries [ExcludeFromCodeCoverage] at line 21, so every line of the type sits outside both the numerator and the denominator of the coverage metric and removing lines from it cannot move any coverage number in either direction. Removing that attribute was explicitly out of scope.

Interface retype (#474 defect 1). QuickFiler.Controllers.IQfcFormController derives from IFilerFormController (QuickFiler/Controllers/IQfcFormController.cs:13) and is a strict superset of it. There were no parallel interfaces to consolidate. The fix is therefore a narrow field and constructor-parameter retype from the base interface to the derived one, which is why the diff for this defect is small. The call site now binds to IQfcFormController.SkipGroupAsync() at compile time instead of downcasting at runtime.

Three extracted seams, each landed in its own commit with the suite passed count identical before and after:

• DrainBackgroundLoadingTasksAsync — behaviour-preserving extraction of two byte-identical drain sites, then replaced with an Interlocked.Exchange atomic-swap drain loop.
• TryGetMoveReadiness(out string notifications) with an injectable _notifyNotReady delegate — separates the readiness predicate from its modal notification, so readiness became assertable without presenting a dialog. MessageBox.Show now appears exactly once in the file, inside the delegate's default.
• ShrinkByRows — isolates the height arithmetic corrected for Bug: qfc-collection-eliminate-space-sign-error #471.

Tests (5 new files, 2 changed, 1 csproj)

QfcCollectionControllerDefects468Tests.cs, ...Defects468MoveTests.cs, ...Defects468ConversationTests.cs, ...TestSupport.cs, and QfcCollectionControllerLayout.StaTests.cs. QfcCollectionControllerTests.cs and QfcCollectionControllerDarkModeTests.cs received small changes; QuickFiler.Test.csproj gained five consecutive Compile Include entries. All tests use MSTest, Moq and FluentAssertions, create no temporary file, and require no live Outlook.

Documentation and evidence

The full evidence tree under docs/features/active/qfc-collection-controller-defects-468/evidence/, plus the feature audit, policy audit and code review.

Architecture / How It Fits Together

QfcCollectionController owns a collection of item groups and mediates between QfcFormController (its parent) and the per-item controllers. Three of the seven issues came from that mediation layer reading a collection by index that carried no ordering guarantee, or subscripting with a sentinel -1 returned by a lookup that found nothing.

The shape of every fix in this PR is the same: extract the decision from the presentation or the framework dependency, assert the decision directly, and leave the wiring untouched. That is what makes the defects testable without a live Outlook process or a shown WinForms form. QfcCollectionController.cs is not split into partial classes here; that decomposition is tracked separately by #623.

Verification

Completed

Final QA loop, single clean pass, no restart, no file rewritten:

Stage	Command	Result
Format	dotnet tool run csharpier format <10 owned paths>	EXIT 0, 0 files rewritten (SHA-256 identical before and after on all 10)
Format verify	dotnet tool run csharpier check .	EXIT 0, 1530 files checked, zero unformatted
Analyzers	msbuild /t:Rebuild "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true	EXIT 0, 18 projects executed CoreCompile, 0 Skipping target "CoreCompile", 0 analyzer diagnostics
Nullable	msbuild /t:Rebuild "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true	EXIT 0, 18 projects compiled, 0 CS86xx
Tests	9 assemblies via the repository coverage runner (dotnet-coverage wrapping vstest with /InIsolation)	6581 total, 6581 passed, 0 failed, 0 skipped

/t:Rebuild is load-bearing: a warm /t:Build returns exit 0 having skipped CoreCompile on every project, so the analyzer and nullable gates would pass while compiling nothing. The zero skip count is recorded as the non-vacuity proof for both gates.

Repository line coverage moved from 84.7703% to 84.9435% and branch coverage from 78.6876% to 78.9377% — both up. No part of that delta is attributable to this feature's tests, for the [ExcludeFromCodeCoverage] reason given above. Changed-line coverage is therefore undefined rather than unmeasured: zero changed production lines lie in the coverage denominator.

Per-defect evidence

Because a coverage delta cannot serve as evidence for an excluded type, verification is cited as named tests:

Defect	Test name(s)
#286	RemoveSpecificControlGroupAsync_ThrowAtFirstStatement_RestoresReentrancyCounter, RemoveSpecificControlGroupAsync_ThrowLaterInBody_RestoresReentrancyCounter
#468	No test, by construction (a removal). Proven by compilation, the dead-identifier sweep, the live-member non-regression check, the reflective-caller search, and a green suite.
#469 defect 1	GetMoveDiagnostics_WithNullItemController_ReturnsUnknownLineWithoutThrowing
#469 defect 2	GetMoveDiagnostics_WithOneGroup_ReturnsExactlyOneLine, GetMoveDiagnostics_WithThreeGroups_ReturnsThreeLinesAndNoNulls
#469 defect 3	ItemGroupsToMoveFieldDeclaresAnOrderedContract, TryGetItemGroupByIndexResolvesInsertionOrderAfterMutation
#469 defect 4	MoveEmailsAsync_WithNullStack_BehavesIdenticallyToAnEmptyStack
#470 defect 1	PromoteFirstChild_WithNoMatchingChild_ReturnsMinusOneWithoutSubscripting, ToggleGroupConv_WithNoMatchingOriginal_DoesNotSubscriptWithMinusOne
#470 defect 2	ResolveConversationInsertions_ExcludesBaseEntryAndOrdersBySentOnDescending, ReconcileInsertionCount_AboveReservation_ReturnsInsertionsCountAndWarnsOnce, ReconcileInsertionCount_EqualToReservation_ReturnsInsertionsCountAndDoesNotWarn, ReconcileInsertionCount_BelowReservation_ReturnsInsertionsCountAndWarnsOnce, EnumerateConversationMembers_WithNoInsertions_DoesNotThrow
#470 defect 3	SetVisualDigits_WithNullItemController_SkipsTheGroupWithoutThrowing
#471	ShrinkByRows_WithPositiveRemovalCount_ReducesHeight, ShrinkByRows_WithNegativeRemovalCount_IncreasesHeight, EliminateSpaceForItems_ReducesMinimumHeightByTemplateHeightTimesRemovalCount, MakeSpaceThenEliminateSpace_IsMinimumHeightNeutral
#473 defect 1	DrainBackgroundLoadingTasksAsync_AwaitsATaskAddedDuringTheDrainWindow
#473 defect 2	MoveEmailsAsync_WhenMoveIsCancelled_PropagatesOperationCanceledException, MoveEmailsAsync_AfterFirstFailure_DoesNotReadSubjectASecondTime, MoveEmailsAsync_WithNullGroupFromIndexLookup_DoesNotThrow
#474 defect 1	ParentFieldAndConstructorParameterAreTypedIQfcFormController
#474 defect 2	TryGetMoveReadiness_WithUnassignedDestination_ReturnsFalseAndProducesNotificationText, TryGetMoveReadiness_WithAllDestinationsAssigned_ReturnsTrueAndEmptyNotification

Recommended

dotnet tool run csharpier check .
msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true
msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true

Backward Compatibility / Migration Notes

• No public API removal that has a caller. The twelve members removed under Bug: qfc-collection-controller-unreachable-load-paths #468 were unreachable; the live member set of the type is unchanged.
• IQfcCollectionController member set is unchanged. TryGetMoveReadiness is deliberately internal and was not added to the interface. The move-emails member keeps its stackMovedItems parameter for source compatibility; it is now documented and consumed rather than silently ignored, and its removal is deferred to Refactor: Remove the stackMovedItems parameter from MoveEmailsAsync #629.
• Behaviour change users will observe: collapsing a conversation now shrinks the item panel rather than growing it (Bug: qfc-collection-eliminate-space-sign-error #471). This is the corrected behaviour.
• No packages.config entry and no csproj PackageReference was added.

Risks and Mitigations

Risk	Mitigation
A reflective caller of a removed #468 member exists outside the searched set	Search covered 398 build-input files and all 42 GetMethod( call sites; the residual repository-wide sweep is filed as #635
The atomic-swap drain changes timing under a future concurrent caller	The drain test registers its continuation with TaskContinuationOptions.ExecuteSynchronously so the late add lands deterministically inside the drain window; no Thread.Sleep, Task.Delay, or wall-clock wait appears in any new test
QfcCollectionController.cs grew by 88 lines, to 2,437	Pre-existing cap violation (2,349 at the base commit) owned by #623; the only lawful remedy in this branch, splitting into partial classes, was prohibited by the feature's own acceptance criteria. #623's recorded baseline should be updated from 2,349 to 2,437

Rollback is a single revert of the merge commit; no data migration or schema change is involved.

Review Guide

1. QuickFiler/Controllers/QfcCollectionController.cs — the substance. Read the three seam commits first (refactor(473), refactor(474), refactor(471)); each is behaviour-preserving and isolated, which makes the following fix commits small.
2. QuickFiler/Interfaces/IQfcCollectionController.cs — XML documentation only, no signature change.
3. The five new test files — largest single review surface, but mechanical and independent.
4. docs/features/active/qfc-collection-controller-defects-468/ — evidence and audits; skim unless a specific claim above needs sourcing.
5. Skip evidence/baseline/coverage-baseline.cobertura.xml and evidence/qa-gates/coverage-final.cobertura.xml; they are generated coverage reports retained as evidence.

Commits are ordered to match the planned fix order, with the dead-code removal isolated in its own commit.

Follow-ups

Nine follow-up candidates were identified and all nine now own an open issue:

• Feature: quickfiler-500-line-cap-violations #623 — decompose QfcCollectionController.cs below the 500-line cap (baseline needs updating to 2,437)
• Refactor: Remove the stackMovedItems parameter from MoveEmailsAsync #629 — remove the stackMovedItems parameter entirely
• Refactor: Relocate the ReadyForMove presentation to the caller #630 — relocate the ReadyForMove presentation to the caller
• Refactor: Consolidate IFilerFormController and IQfcFormController #631 — consolidate IFilerFormController and IQfcFormController
• Refactor: Remove the orphan QuickFiler.Interfaces.IQfcFormController #632 — remove the orphan QuickFiler.Interfaces.IQfcFormController
• Bug: kbdactions-enumerable-ctor-bypasses-duplicate-guard #444 — harden KbdActions(IEnumerable<UClass>) with the duplicate check both Add overloads perform
• Bug: qfc-unsynchronized-undo-handoff-after-batch-move #633 — the unsynchronized undo handoff after a batch move
• Bug: qfc-unsynchronized-plain-read-reentrancy-counter #634 — the unsynchronized plain read of the re-entrancy counter
• Bug: Settle the issue #468 residual reflective-caller risk repository-wide #635 — settle the Bug: qfc-collection-controller-unreachable-load-paths #468 residual reflective-caller risk repository-wide

Two acceptance criteria remain open by design: AC-28 (issue closure) cannot be satisfied at this merge, because this PR targets the epic integration branch and GitHub registers closing references only for pull requests targeting the default branch. The seven issues below close when the integration branch merges to the default branch.

GitHub Auto-close

• Closes Bug: qfc-collectioncontroller-removespecificcontrolgroup-counter-leak #286
• Closes Bug: qfc-collection-controller-unreachable-load-paths #468
• Closes Bug: qfc-collection-move-diagnostics-defects #469
• Closes Bug: qfc-collection-conversation-index-defects #470
• Closes Bug: qfc-collection-eliminate-space-sign-error #471
• Closes Bug: qfc-collection-background-task-and-catch-defects #473
• Closes Bug: qfc-collection-controller-coupling-and-modal-getter #474