refactor(quickfiler): remove unused stackMovedItems parameter from MoveEmailsAsync (#629)

QuickFiler.Test/Controllers/QfcCollectionControllerDefects468MoveTests.cs

@@ -162,7 +162,7 @@ public async Task MoveEmailsAsync_WhenMoveIsCancelled_PropagatesOperationCancele
             );
 
             // Act
-            Func<Task> act = () => controller.MoveEmailsAsync(null);
+            Func<Task> act = () => controller.MoveEmailsAsync();
 
             // Assert
             await act.Should()
@@ -213,7 +213,7 @@ public async Task MoveEmailsAsync_AfterFirstFailure_DoesNotReadSubjectASecondTim
             );
 
             // Act
-            Func<Task> act = () => controller.MoveEmailsAsync(null);
+            Func<Task> act = () => controller.MoveEmailsAsync();
 
             // Assert
             await act.Should()
@@ -260,7 +260,7 @@ public async Task MoveEmailsAsync_WithNullGroupFromIndexLookup_DoesNotThrow()
             );
 
             // Act
-            Func<Task> act = () => controller.MoveEmailsAsync(null);
+            Func<Task> act = () => controller.MoveEmailsAsync();
 
             // Assert
             await act.Should()
@@ -460,15 +460,13 @@ private static string[] InvokeMoveDiagnostics(QfcCollectionController controller
         }
 
         /// <summary>
-        /// Issue #469 defect 4. The retained <c>stackMovedItems</c> parameter must not influence
-        /// the outcome. With an empty cached move collection, passing <see langword="null"/> and
-        /// passing an in-memory stack are observationally identical: neither throws and neither
-        /// leaves a record on the supplied stack, because undo records reach the stack through the
-        /// email filer's push path onto the same globals instance, never through this argument.
-        /// The stack is constructed in memory and is never serialized, so no file is touched.
+        /// Issue #629: <c>MoveEmailsAsync</c> no longer takes a <c>stackMovedItems</c> argument, so
+        /// this test (formerly <c>MoveEmailsAsync_WithNullStack_BehavesIdenticallyToAnEmptyStack</c>)
+        /// no longer has two argument shapes to compare. Rewritten to keep the one behavior it was
+        /// actually pinning: an empty cached move collection is a no-op early return, not a throw.
         /// </summary>
         [TestMethod]
-        public async Task MoveEmailsAsync_WithNullStack_BehavesIdenticallyToAnEmptyStack()
+        public async Task MoveEmailsAsync_WithEmptyItemGroupsToMove_DoesNotThrow()
         {
             // Arrange
             QfcCollectionController controller =
@@ -478,20 +476,15 @@ public async Task MoveEmailsAsync_WithNullStack_BehavesIdenticallyToAnEmptyStack
                 "_itemGroupsToMove",
                 new List<QfcItemGroup>()
             );
-            var stack = new SloStack<IMovedMailInfo>();
 
             // Act
-            Func<Task> withNullStack = () => controller.MoveEmailsAsync(null);
-            Func<Task> withSuppliedStack = () => controller.MoveEmailsAsync(stack);
+            Func<Task> act = () => controller.MoveEmailsAsync();
 
             // Assert
-            await withNullStack.Should().NotThrowAsync(because: NoStackEffect);
-            await withSuppliedStack.Should().NotThrowAsync(because: NoStackEffect);
-            stack.Count.Should().Be(0, because: NoStackEffect);
+            await act.Should()
+                .NotThrowAsync(
+                    because: "an empty cached move collection is the early-return no-op branch, not a failure"
+                );
         }
-
-        private const string NoStackEffect =
-            "the stackMovedItems parameter is retained for source compatibility only, so a null "
-            + "argument and a supplied argument must produce the same observable outcome";
     }
 }

QuickFiler.Test/Controllers/QfcFormControllerUndoHandoffTests.cs

@@ -71,9 +71,7 @@ public void Setup()
             _mockQfcQueue = new Mock<IQfcQueue>();
 
             _mockGroups = new Mock<IQfcCollectionController>();
-            _mockGroups
-                .Setup(g => g.MoveEmailsAsync(It.IsAny<SloStack<IMovedMailInfo>>()))
-                .Returns(Task.CompletedTask);
+            _mockGroups.Setup(g => g.MoveEmailsAsync()).Returns(Task.CompletedTask);
             _mockGroups.Setup(g => g.CleanupBackground()).Callback(() => Record(CleanupToken));
 
             _gate = new TaskCompletionSource<bool>(
@@ -393,10 +391,7 @@ public async Task BackGroundMoveAsync_WhenParentIsNull_ReturnsWithoutThrowing()
                 .IsFaulted.Should()
                 .BeFalse("a null parent is an early return, not an exception");
             moveTask.IsCompleted.Should().BeTrue("the method returns rather than hanging");
-            _mockGroups.Verify(
-                g => g.MoveEmailsAsync(It.IsAny<SloStack<IMovedMailInfo>>()),
-                Times.Never
-            );
+            _mockGroups.Verify(g => g.MoveEmailsAsync(), Times.Never);
         }
 
         /// <summary>

QuickFiler/Controllers/QfcCollectionController.cs

@@ -2140,23 +2140,14 @@ public void Cleanup()
         /// <summary>
         /// Moves every cached item group's message to its assigned destination folder.
         /// </summary>
-        /// <param name="stackMovedItems">
-        /// The undo stack. This parameter does not carry the undo records: the stack is populated
-        /// by the email filer's push-to-undo-stack path, which pushes onto
-        /// <c>Globals.AF.MovedMails</c>. That is the same instance the caller passes here, because
-        /// the caller reads it from the same globals object. Passing a different instance would not
-        /// redirect the undo records, and passing <c>null</c> does not suppress them. The parameter
-        /// is retained only for source compatibility with existing callers; removing it is a
-        /// follow-up candidate, not part of this change.
-        /// </param>
-        public async Task MoveEmailsAsync(SloStack<IMovedMailInfo> stackMovedItems)
+        /// <remarks>
+        /// The undo stack is populated by the email filer's push-to-undo-stack path, which pushes
+        /// onto <c>Globals.AF.MovedMails</c> directly; this method takes no undo-stack argument
+        /// because none is needed to reach it (issue #629).
+        /// </remarks>
+        public async Task MoveEmailsAsync()
         {
-            //TraceUtility.LogMethodCall(stackMovedItems);
-
-            // The parameter is deliberately discarded rather than left untouched. The undo records
-            // reach the stack through the email filer, not through this argument, and the discard
-            // states that at the point of use so the parameter cannot be read as an oversight.
-            _ = stackMovedItems;
+            //TraceUtility.LogMethodCall();
 
             var count = _itemGroupsToMove?.Count() ?? 0;
             if (count <= 0)

QuickFiler/Controllers/QfcFormController.EventHandlers.cs

@@ -225,7 +225,7 @@ _groups is null
             }
 
             // Move emails
-            await _groups.MoveEmailsAsync(_movedItems);
+            await _groups.MoveEmailsAsync();
 
             // Wait for the batch's filing work, including its undo pushes, to finish before either
             // downstream step runs. The await at the line above completes as soon as the last item has

QuickFiler/Interfaces/IQfcCollectionController.cs

@@ -51,16 +51,12 @@ ItemViewer LoadItemViewer_03(
         /// <summary>
         /// Moves every cached item group's message to its assigned destination folder.
         /// </summary>
-        /// <param name="StackMovedItems">
-        /// The undo stack. This parameter does not carry the undo records: the stack is populated
-        /// by the email filer's push-to-undo-stack path, which pushes onto
-        /// <c>Globals.AF.MovedMails</c>. That is the same instance the caller passes here, because
-        /// the caller reads it from the same globals object. Passing a different instance would not
-        /// redirect the undo records, and passing <c>null</c> does not suppress them. The parameter
-        /// is retained only for source compatibility with existing callers; removing it is a
-        /// follow-up candidate, not part of this change.
-        /// </param>
-        Task MoveEmailsAsync(SloStack<IMovedMailInfo> StackMovedItems);
+        /// <remarks>
+        /// The undo stack is populated by the email filer's push-to-undo-stack path, which pushes
+        /// onto <c>Globals.AF.MovedMails</c> directly; this method takes no undo-stack argument
+        /// because none is needed to reach it (issue #629).
+        /// </remarks>
+        Task MoveEmailsAsync();
         void AddItemGroup(MailItem mailItem);
 
         /// <summary>

docs/features/active/2026-08-26-qfc-remove-stackmoveditems-parameter-629/code-review.2026-09-02T12-00.md

@@ -0,0 +1,143 @@
+# Code Review: Remove `stackMovedItems` parameter from `MoveEmailsAsync` (#629)
+
+**Review Date:** 2026-09-02
+**Reviewer:** Claude (fork, orchestration session)
+**Feature Folder:** `docs/features/active/2026-08-26-qfc-remove-stackmoveditems-parameter-629`
+**Feature Folder Selection Rule:** Pre-existing promoted folder for issue #629; only `issue.md` had real
+content at the start of this run, `spec.md`/`user-story.md`/`plan.md` were empty scaffolds authored
+during this run.
+**Base Branch:** `main`
+**Head Branch:** `feature/qfc-remove-stackmoveditems-parameter-629`
+**Review Type:** Initial review
+
+---
+
+## Executive Summary
+
+The change removes an unused `stackMovedItems` parameter from `IQfcCollectionController.MoveEmailsAsync`
+and its implementation, updates the single production call site, and updates four tests across two test
+files. Total footprint: 5 files, no new files, no dependency changes.
+
+**What changed:**
+`IQfcCollectionController.cs` and `QfcCollectionController.cs` drop the parameter and its `<param>` doc
+comment (replaced with an explanatory `<remarks>` block); `QfcCollectionController.cs` also drops the
+`_ = stackMovedItems;` discard statement that issue #468 had added as an interim marker. The sole call
+site in `QfcFormController.EventHandlers.cs` drops its argument. Four tests in two files are updated:
+three call-shape fixes with no behavioral change, and one test rewritten to preserve early-return-branch
+coverage that the old test's null-vs-supplied-stack comparison happened to also provide.
+
+**Top 3 risks:**
+1. A missed mock/setup site elsewhere in the test suite would fail to compile — mitigated by a
+   full-repo grep (`evidence/baseline/p0-t8-mock-sweep.md`) that found all 6 call sites (1 more than the
+   1 named in `issue.md`) before editing began.
+2. Deleting rather than rewriting the null-stack test could silently drop branch coverage on the
+   early-return path — mitigated by confirming via grep that this was the only test setting
+   `_itemGroupsToMove` to an empty collection, and rewriting instead of deleting.
+3. Cobertura's root rate attributes appear to show a coverage regression — this is a known, already-filed
+   tooling defect (#529/#530), not a real regression; the true per-line sums show a marginal improvement.
+
+**PR readiness recommendation:** **Go** — clean toolchain pass, no test-count change, no behavioral
+change outside the documented scope, footprint matches the prediction in `spec.md`.
+
+---
+
+## Findings Table
+
+No Blockers or Major findings.
+
+| Severity | File | Location | Finding | Recommendation | Rationale | Evidence |
+|---|---|---|---|---|---|---|
+| Info | `QuickFiler/Controllers/QfcCollectionController.cs` | doc comment | The removed `<param>` element is replaced by a `<remarks>` block explaining the undo-stack invariant rather than simply deleted. | None — this is the correct choice. | Preserves the "why" (issue #629's rationale) for future readers instead of leaving a bare signature. | Diff inspection. |
+| Nit | `QuickFiler.Test/Controllers/QfcCollectionControllerDefects468MoveTests.cs` | rewritten test | Test renamed from `MoveEmailsAsync_WithNullStack_BehavesIdenticallyToAnEmptyStack` to `MoveEmailsAsync_WithEmptyItemGroupsToMove_DoesNotThrow`. | None — rename is accurate to the new scope. | The old name described a comparison that no longer exists after the parameter removal. | `evidence/other/p1-t5-test-disposition.md` |
+
+---
+
+## Implementation Audit
+
+### C# implementation audit
+
+#### What changed well
+
+- The scope stayed minimal: exactly the files predicted in `spec.md`'s Implementation Strategy section
+  were touched, confirmed post-hoc by `evidence/other/p1-t6-footprint-check.md`.
+- The doc-comment rewrite on `IQfcCollectionController.MoveEmailsAsync` explains *why* no argument is
+  needed (citing where the undo stack is actually populated), following the repository's "comment why,
+  not what" convention rather than just deleting the stale `<param>` block.
+- The P0-T8 mock/setup sweep caught a call-site count (4 direct calls, plus 2 mock setup/verify sites)
+  larger than the 1 the source issue anticipated, preventing a compile break that would only have
+  surfaced later in the toolchain.
+
+#### Type safety and API notes
+
+- `IQfcCollectionController.MoveEmailsAsync()` remains a `Task`-returning public contract; removing a
+  parameter that was never read by any implementation cannot introduce a nullable-flow change. The
+  nullable build (`evidence/qa-gates/p2-t4-nullable-build.md`) confirms 0 new warnings.
+- No public API outside `QuickFiler` is affected — `IQfcCollectionController` has a single production
+  implementation and a single production call site, both first-party and in-repo.
+
+#### Error handling and logging
+
+- The only logging-adjacent line touched was a comment (`//TraceUtility.LogMethodCall(stackMovedItems);`),
+  already dead code before this change; updating it to drop the removed argument keeps the comment
+  internally consistent but changes no runtime behavior.
+
+---
+
+## Test Quality Audit
+
+Four tests across two files were updated. Three (`MoveEmailsAsync_WhenMoveIsCancelled_...`,
+`MoveEmailsAsync_AfterFirstFailure_...`, `MoveEmailsAsync_WithNullGroupFromIndexLookup_...`) required
+only a call-shape fix (`MoveEmailsAsync(null)` → `MoveEmailsAsync()`); none of them asserted on the
+removed argument's value, so no behavioral change resulted. The fourth was rewritten in place, and two
+Moq sites in a fifth file's test class were updated to the new zero-argument signature.
+
+### Reviewed test and QA artifacts
+
+- `QuickFiler.Test/Controllers/QfcCollectionControllerDefects468MoveTests.cs` — 4 call sites, 1 test
+  body rewritten; verified no orphaned `SloStack`/`IMovedMailInfo` usage in the file after the change.
+- `QuickFiler.Test/Controllers/QfcFormControllerUndoHandoffTests.cs` — 1 `Setup`, 1 `Verify` updated;
+  confirmed `SloStack<IMovedMailInfo>` is still legitimately referenced once elsewhere in the file
+  (`_mockAF.SetupGet(a => a.MovedMails)...`), so no unused-using cleanup was needed.
+- `evidence/baseline/p0-t7-baseline-coverage.md` / `evidence/qa-gates/p2-t5-final-coverage.md` — full
+  suite run, 6949/6949 passing both before and after.
+- `evidence/qa-gates/p2-t6-coverage-delta.md` — coverage delta computed from reliable Cobertura sums,
+  disposition: no regression.
+
+### Quality assessment prompts
+
+- **Determinism:** no randomness, timers, or wall-clock reads in any touched test.
+- **Isolation:** each test still targets one behavior of `MoveEmailsAsync`; the rewrite narrowed one
+  test's assertion surface rather than broadening it.
+- **Speed:** no new I/O or waits introduced; full suite (6949 tests) ran in both baseline and final
+  passes with no observed slowdown.
+- **Diagnostics:** the rewritten test's `NotThrowAsync(because: ...)` carries an explicit rationale
+  string identifying the early-return branch it pins.
+
+---
+
+## Security / Correctness Checks
+
+| Check | Status | Evidence |
+|---|---|---|
+| No secrets in code | PASS | Diff inspection — no new literals of any kind. |
+| No unsafe subprocess or command construction | N/A | No process/command code touched. |
+| Input validation at boundaries | N/A | No external input boundary touched; internal signature change only. |
+| Error handling remains explicit | PASS | No `catch`/`throw` logic touched; nullable build clean. |
+| Configuration / path handling is safe | N/A | No configuration or path code touched. |
+
+---
+
+## Research Log
+
+No external research was required. All context came from the pre-existing `issue.md` (which documents
+the issue #468/#469 history and the actual undo-stack population path in `EmailFiler.cs`) and direct
+repository inspection (grep sweeps, file reads).
+
+---
+
+## Verdict
+
+The change is a minimal, well-scoped signature simplification with a clean toolchain pass and no
+test-count regression. The one test requiring more than a mechanical fix (the null-stack comparison
+test) was correctly disposed of by rewrite rather than deletion, preserving its early-return-branch
+coverage. Ready for normal PR flow.

docs/features/active/2026-08-26-qfc-remove-stackmoveditems-parameter-629/evidence/baseline/base-ref.md

@@ -0,0 +1 @@
+Base ref: fe91e965adf66112f1e489f00f04ccd8a3ab1437

docs/features/active/2026-08-26-qfc-remove-stackmoveditems-parameter-629/evidence/baseline/p0-t3-sdk-bootstrap.md

@@ -0,0 +1 @@
+SDK 8.0.205 installed and NuGet restore succeeded, 172 packages, 0 errors, 2026-09-02

docs/features/active/2026-08-26-qfc-remove-stackmoveditems-parameter-629/evidence/baseline/p0-t4-csharpier-check.md

@@ -0,0 +1,2 @@
+Checked 1575 files in 6648ms.
+EXIT_CODE: 0