From dfbebb13fdc9ce2e9240376be2214dddf56ee5d0 Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 6 Jul 2026 07:29:04 -0400 Subject: [PATCH 1/5] fix(store-wrapper): prevent NullReferenceException when stores model is unavailable - Add EvaluateLaunchReadiness() guard to detect null or incomplete store model before Launch() opens dialog - Handle transient post-deserialize state where Globals.Ol.StoresWrapper is null or its Stores list is null - Show user-facing message when model is not ready, preventing unhandled exception - Add deterministic MSTest regression tests with Moq for both null-model and null-stores-list paths - Include feature-folder lifecycle artifacts and QA gate evidence Refs: #240 --- .../Store/StoreWrapperController_Tests.cs | 199 ++++++++++++++++++ .../Store/StoreWrapperController.cs | 101 ++++++++- .../baseline/ac-source-confirmation.md | 16 ++ .../evidence/baseline/analyzer-baseline.md | 10 + .../evidence/baseline/csharpier-baseline.md | 8 + .../evidence/baseline/git-baseline.md | 13 ++ .../evidence/baseline/nullable-baseline.md | 10 + .../baseline/phase0-instructions-read.md | 15 ++ .../baseline/test-coverage-baseline.md | 12 ++ .../issue-240.2026-07-06T07-58.md | 16 ++ .../evidence/other/ac6-deferral.md | 7 + .../evidence/other/plan-status-summary.md | 44 ++++ .../other/scope-budget-confirmation.md | 20 ++ .../evidence/qa-gates/qa-01-format.md | 14 ++ .../evidence/qa-gates/qa-02-analyzers.md | 9 + .../evidence/qa-gates/qa-03-nullable.md | 25 +++ .../evidence/qa-gates/qa-04-test-coverage.md | 21 ++ .../evidence/qa-gates/qa-05-coverage-delta.md | 19 ++ .../regression-testing/fail-before-240.md | 14 ++ .../regression-testing/pass-after-240.md | 22 ++ .../issue.md | 85 ++++++++ .../plan.2026-07-06T06-41.md | 83 ++++++++ ...0-store-wrapper-launch-npe-240-research.md | 170 +++++++++++++++ 23 files changed, 929 insertions(+), 4 deletions(-) create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/ac-source-confirmation.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/analyzer-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/csharpier-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/git-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/nullable-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/phase0-instructions-read.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/test-coverage-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/issue-updates/issue-240.2026-07-06T07-58.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/ac6-deferral.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/plan-status-summary.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/scope-budget-confirmation.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/fail-before-240.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/pass-after-240.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/plan.2026-07-06T06-41.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/research/2026-07-06T00-00-store-wrapper-launch-npe-240-research.md diff --git a/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs index 31e2f4398..95e535cde 100644 --- a/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs +++ b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs @@ -559,6 +559,205 @@ public void SelectFolder_WhenPickFolderThrows_ReturnsNull() #endregion + #region Launch (issue #240 regression) + + /// + /// Reproduces issue #240: when the Outlook store-wrapper model has not yet been + /// loaded (Globals.Ol.StoresWrapper is null), Launch() must not throw + /// an unhandled . It must show a user-facing + /// message via the dialog seam and leave Viewer null + /// rather than opening a broken dialog (AC1). + /// + [TestMethod] + public void Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + var originalInvoker = MyBox.DialogInvoker; + var invocationCount = 0; + + try + { + MyBox.DialogInvoker = _ => + { + invocationCount++; + return DialogResult.OK; + }; + + // Act + Action act = () => controller.Launch(); + + // Assert + act.Should().NotThrow(); + invocationCount.Should().Be(1); + controller.Viewer.Should().BeNull(); + } + finally + { + MyBox.DialogInvoker = originalInvoker; + } + } + + /// + /// Reproduces issue #240 for the secondary root cause: a non-null + /// StoresWrapper whose Stores list is transiently null (post-deserialize + /// state before the async rewire completes). Launch() must not throw and must + /// leave Viewer null instead of opening a broken dialog (AC2). + /// + [TestMethod] + public void Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null }); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + var originalInvoker = MyBox.DialogInvoker; + var invocationCount = 0; + + try + { + MyBox.DialogInvoker = _ => + { + invocationCount++; + return DialogResult.OK; + }; + + // Act + Action act = () => controller.Launch(); + + // Assert + act.Should().NotThrow(); + invocationCount.Should().Be(1); + controller.Viewer.Should().BeNull(); + } + finally + { + MyBox.DialogInvoker = originalInvoker; + } + } + + #endregion + + #region EvaluateLaunchReadiness (issue #240) + + /// + /// When is null, readiness cannot be + /// determined and the evaluation must report ModelUnavailable rather than + /// throwing. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable() + { + // Arrange + var controller = new StoreWrapperController(null!); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); + } + + /// + /// When Globals.Ol is null, readiness cannot be determined and the evaluation + /// must report ModelUnavailable rather than throwing. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable() + { + // Arrange + var mockGlobals = new Mock(); + mockGlobals.SetupGet(g => g.Ol).Returns((IOlObjects)null); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); + } + + /// + /// When Globals.Ol.StoresWrapper is null (store load has not completed), + /// the evaluation must report ModelUnavailable. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); + } + + /// + /// When the model is present but its Stores list is transiently null + /// (post-deserialize, before the async rewire populates it), the evaluation must + /// report StoresUnavailable. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null }); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.StoresUnavailable); + } + + /// + /// When the model and its Stores list are both populated, the evaluation must + /// report Ready with the model and the display names of every seeded store. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames() + { + // Arrange + var storeA = new StoreWrapper(null) { DisplayName = "Mailbox A" }; + var storeB = new StoreWrapper(null) { DisplayName = "Mailbox B" }; + var model = new StoresWrapper + { + Stores = new List { storeA, storeB }, + }; + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns(model); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.Ready); + readiness.DisplayNames.Should().Equal("Mailbox A", "Mailbox B"); + } + + #endregion + #region Stub helpers private sealed class StubSelectFolderController : StoreWrapperController diff --git a/UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs b/UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs index a70180b58..e05fbb426 100644 --- a/UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs +++ b/UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs @@ -11,6 +11,58 @@ namespace UtilitiesCS.OutlookObjects.Store { + /// + /// Describes whether the store-wrapper model is ready for + /// to open the settings dialog. + /// + internal enum StoreLaunchReadinessState + { + Ready, + ModelUnavailable, + StoresUnavailable, + } + + /// + /// Result of : the readiness + /// state plus, when ready, the model and store display names needed to populate the + /// settings dialog. + /// + internal readonly struct StoreLaunchReadiness + { + private StoreLaunchReadiness( + StoreLaunchReadinessState state, + StoresWrapper model, + IList displayNames + ) + { + State = state; + Model = model; + DisplayNames = displayNames; + } + + internal StoreLaunchReadinessState State { get; } + + internal StoresWrapper Model { get; } + + internal IList DisplayNames { get; } + + internal static StoreLaunchReadiness NotReady(StoreLaunchReadinessState state) + { + // why: this project has no #nullable annotation context, so Model/DisplayNames + // are declared non-nullable; the "not ready" sentinel legitimately has neither. + // Suppress narrowly rather than adding '?' annotations, which would produce new + // CS8632 warnings on this file during normal (non-forced-nullable) builds. +#pragma warning disable CS8625 // Cannot convert null literal to non-nullable reference type. + return new(state, null, null); +#pragma warning restore CS8625 + } + + internal static StoreLaunchReadiness Ready( + StoresWrapper model, + IList displayNames + ) => new(StoreLaunchReadinessState.Ready, model, displayNames); + } + public class StoreWrapperController { internal static bool RunFolderSelectionDialog(Func selector) @@ -41,17 +93,58 @@ public StoreWrapperController(IApplicationGlobals globals) internal FolderMinimalWrapper JunkPotential { get; set; } internal Func FsConverter { get; set; } + /// + /// Determines whether the store-wrapper model has finished loading and is safe to + /// bind into the settings dialog. Addresses issue #240: Globals.Ol.StoresWrapper + /// is populated asynchronously during startup and can be null (load not yet complete), + /// or non-null with a transiently null Stores list (post-deserialize, before the + /// async rewire populates it). Callers must not dereference the model until this + /// reports . + /// + /// + /// A describing the readiness state and, when ready, + /// the model and the display names of every store it contains. + /// + internal StoreLaunchReadiness EvaluateLaunchReadiness() + { + var model = Globals?.Ol?.StoresWrapper; + if (model is null) + { + return StoreLaunchReadiness.NotReady(StoreLaunchReadinessState.ModelUnavailable); + } + + if (model.Stores is null) + { + return StoreLaunchReadiness.NotReady(StoreLaunchReadinessState.StoresUnavailable); + } + + return StoreLaunchReadiness.Ready( + model, + model.Stores.Select(store => store.DisplayName).ToList() + ); + } + #region Events [ExcludeFromCodeCoverage] public void Launch() { + var readiness = EvaluateLaunchReadiness(); + if (readiness.State != StoreLaunchReadinessState.Ready) + { + MyBox.ShowDialog( + "Store settings are not available yet. Please try again after startup completes.", + "Store Settings Unavailable", + MessageBoxButtons.OK, + MessageBoxIcon.Warning + ); + return; + } + FsConverter = new FilePathHelperConverter(Globals.FS).GetSerializablePath; - Model = Globals.Ol.StoresWrapper; + Model = readiness.Model; Viewer = new StoreWrapperViewer(this); - Viewer.DisplayName.DataSource = Model - .Stores.Select(store => store.DisplayName) - .ToList(); + Viewer.DisplayName.DataSource = readiness.DisplayNames; Viewer.ShowDialog(); } diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/ac-source-confirmation.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/ac-source-confirmation.md new file mode 100644 index 000000000..198f784cb --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/ac-source-confirmation.md @@ -0,0 +1,16 @@ +# AC Source Confirmation (Issue #240) + +Timestamp: 2026-07-06T07-01 + +Confirmation: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md` contains an explicit `## Acceptance Criteria` heading. AC1-AC6 count = 6. + +AC1-AC6 verbatim (as found under `## Acceptance Criteria`): + +- AC1: `StoreWrapperController.Launch()` does not throw an unhandled `NullReferenceException` when `Globals.Ol.StoresWrapper` (`Model`) is null. It fails gracefully with a clear user-facing message and returns without opening a broken dialog. +- AC2: `Launch()` also handles a non-null `Model` whose `Stores` list is null (transient post-deserialize state) without throwing. +- AC3: A deterministic MSTest regression test reproduces the pre-fix crash path (fails before the fix, passes after) using Moq for `IApplicationGlobals`/`IOlObjects`; no live Outlook, no temporary files. +- AC4: The underlying readiness/initialization gap identified by root-cause research is addressed so that invoking the store-settings command when store state is unavailable produces deterministic, non-crashing behavior rather than an unhandled exception. +- AC5: The full C# toolchain passes in order (csharpier -> .NET analyzers -> nullable/TreatWarningsAsErrors -> MSTest with coverage); coverage on changed lines meets the >= 90% new-code target and repository line coverage remains >= 80% for the testable denominator. +- AC6: All required PR CI checks are green against the PR head SHA. + +This section (only) is treated as the AC source for this minor-audit plan. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/analyzer-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/analyzer-baseline.md new file mode 100644 index 000000000..181c5c8a1 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/analyzer-baseline.md @@ -0,0 +1,10 @@ +# Analyzer Build Baseline (Issue #240) + +Timestamp: 2026-07-06T07-10 + +Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` +(Invoked as: `MSBuild.exe TaskMaster.sln -t:Build -p:Configuration=Debug -p:Platform="Any CPU" -p:EnableNETAnalyzers=true -p:EnforceCodeStyleInBuild=true -nologo -clp:Summary`, dash-switch form required by the git-bash shell.) + +EXIT_CODE: 0 + +Output Summary: Build succeeded. 72 warning(s), 0 error(s). Pre-existing warnings include MSTEST0032 (QuickFiler.Test) and multiple CS8632 nullable-annotation-context warnings in TaskMaster.Test. No errors in `UtilitiesCS` or `UtilitiesCS.Test`. A NuGet restore (`scripts/vscode/Invoke-Restore.ps1`, 169 packages) was required first because the repo `packages/` folder was absent. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/csharpier-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/csharpier-baseline.md new file mode 100644 index 000000000..93c429ee4 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/csharpier-baseline.md @@ -0,0 +1,8 @@ +# CSharpier Format Baseline (Issue #240) + +Timestamp: 2026-07-06T07-05 + +Command: `dotnet tool run csharpier check .` +EXIT_CODE: 0 + +Output Summary: Checked 1269 files in 2378ms. All files pass formatting; 0 unformatted files at baseline. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/git-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/git-baseline.md new file mode 100644 index 000000000..c86eb9a3d --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/git-baseline.md @@ -0,0 +1,13 @@ +# Git Baseline (Issue #240) + +Timestamp: 2026-07-06T07-02 + +Command: `git rev-parse HEAD` +EXIT_CODE: 0 +Output: `4022fe7c9b07119224ca5aaa880b0a4003ef08db` + +Command: `git branch --show-current` +EXIT_CODE: 0 +Output: `TaskMaster-wt-2026-07-06-06-35` + +Output Summary: Baseline branch `TaskMaster-wt-2026-07-06-06-35` at short SHA `4022fe7c` (full SHA `4022fe7c9b07119224ca5aaa880b0a4003ef08db`). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/nullable-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/nullable-baseline.md new file mode 100644 index 000000000..998ab74c9 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/nullable-baseline.md @@ -0,0 +1,10 @@ +# Nullable / TreatWarningsAsErrors Baseline (Issue #240) + +Timestamp: 2026-07-06T07-15 + +Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` +(Invoked as `-t:Rebuild` because an incremental `-t:Build` skipped `CoreCompile` for up-to-date outputs and did not exercise the forced-nullable flags; `-t:Rebuild` forces recompilation. Dash-switch form required by the git-bash shell.) + +EXIT_CODE: 1 + +Output Summary: Build FAILED with 84 pre-existing error(s), 0 warning(s). All 84 errors are confined to two vendored/legacy projects that are not nullable-annotated: `SVGControl.csproj` (CS8600/CS8601/CS8602/CS8603/CS8618/CS8625/CS0649) and `UtilitiesSwordfish.NET.General.csproj` (CS8600/CS8601/CS8602/CS8603/CS8604/CS8618/CS8619/CS8625). `UtilitiesCS.csproj` and `UtilitiesCS.Test.csproj` (the in-scope projects for this fix) contribute zero errors. This is a pre-existing baseline condition when `Nullable=enable` is forced globally across the solution; it is not caused by this issue's change and is unaffected by the planned `StoreWrapperController.cs` fix. Final QA (Phase 3) will confirm zero new warnings/errors on the touched files. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/phase0-instructions-read.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/phase0-instructions-read.md new file mode 100644 index 000000000..e5cf19262 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/phase0-instructions-read.md @@ -0,0 +1,15 @@ +# Phase 0 — Policy Instructions Read (Issue #240) + +Timestamp: 2026-07-06T07-00 + +Policy Order: +1. `CLAUDE.md` (standing instructions) +2. `.claude/rules/general-code-change.md` (cross-language code change policy) +3. `.claude/rules/general-unit-test.md` (cross-language unit test policy) +4. `.claude/rules/csharp.md` (C#-specific toolchain and coding standards) + +Files read (start-to-end, no section skipped): +- `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\CLAUDE.md` +- `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\general-code-change.md` +- `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\general-unit-test.md` +- `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\csharp.md` diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/test-coverage-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/test-coverage-baseline.md new file mode 100644 index 000000000..b2530e13d --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/test-coverage-baseline.md @@ -0,0 +1,12 @@ +# Pre-Change Test + Coverage Baseline (Issue #240) + +Timestamp: 2026-07-06T07-20 + +Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation` +(`/InIsolation` added because Moq test assemblies in this repo require it to avoid an STTE 4.2.0.1 Setup FileNotFound failure; dash-free forward-slash dll path required because git-bash mangles backslash-separated relative paths.) + +EXIT_CODE: 0 + +Coverage extraction command: `dotnet-coverage merge .coverage -f xml -o TestResults/baseline-coverage.xml` + +Output Summary: Test Run Successful. Total tests: 4163, Passed: 4163, Failed: 0. Total time 43.12s. No `Launch()`-calling tests exist yet in `StoreWrapperController_Tests.cs` at this baseline. Converted `.coverage` module report for `UtilitiesCS.dll` (the production assembly containing `StoreWrapperController`): line_coverage = **85.87%** (lines_covered=36873, lines_partially_covered=984, lines_not_covered=5085), block_coverage = 86.68%. This is the testable-denominator repository line-coverage percentage referenced by AC5/P3-T4/P3-T5 for this issue's scope (UtilitiesCS project). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/issue-updates/issue-240.2026-07-06T07-58.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/issue-updates/issue-240.2026-07-06T07-58.md new file mode 100644 index 000000000..e4a5f3edc --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/issue-updates/issue-240.2026-07-06T07-58.md @@ -0,0 +1,16 @@ +Timestamp: 2026-07-06T07-58 + +PostedAs: unknown (local mirror only; not posted to GitHub by this executor run) + +Exact text applied to `## Acceptance Criteria` in `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md`: + +``` +## Acceptance Criteria + +- [x] AC1: `StoreWrapperController.Launch()` does not throw an unhandled `NullReferenceException` when `Globals.Ol.StoresWrapper` (`Model`) is null. It fails gracefully with a clear user-facing message and returns without opening a broken dialog. (Evidence: `evidence/regression-testing/pass-after-240.md`, fix in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` P2-T3.) +- [x] AC2: `Launch()` also handles a non-null `Model` whose `Stores` list is null (transient post-deserialize state) without throwing. (Evidence: `evidence/regression-testing/pass-after-240.md`, fix in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` P2-T3.) +- [x] AC3: A deterministic MSTest regression test reproduces the pre-fix crash path (fails before the fix, passes after) using Moq for `IApplicationGlobals`/`IOlObjects`; no live Outlook, no temporary files. (Evidence: `evidence/regression-testing/fail-before-240.md` and `evidence/regression-testing/pass-after-240.md`, P1-T3/P2-T5.) +- [x] AC4: The underlying readiness/initialization gap identified by root-cause research is addressed so that invoking the store-settings command when store state is unavailable produces deterministic, non-crashing behavior rather than an unhandled exception. (Evidence: `EvaluateLaunchReadiness()` in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, P2-T1/P2-T2/P2-T3.) +- [x] AC5: The full C# toolchain passes in order (csharpier -> .NET analyzers -> nullable/TreatWarningsAsErrors -> MSTest with coverage); coverage on changed lines meets the >= 90% new-code target and repository line coverage remains >= 80% for the testable denominator. (Evidence: `evidence/qa-gates/qa-01-format.md` through `evidence/qa-gates/qa-05-coverage-delta.md`, P3-T1 through P3-T5. Note: the solution-wide nullable gate's raw `EXIT_CODE` is 1 due to a pre-existing, unrelated condition documented in `evidence/qa-gates/qa-03-nullable.md`; the touched files themselves introduce zero new nullable diagnostics.) +- [ ] AC6: All required PR CI checks are green against the PR head SHA. +``` diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/ac6-deferral.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/ac6-deferral.md new file mode 100644 index 000000000..8d6c41da3 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/ac6-deferral.md @@ -0,0 +1,7 @@ +# AC6 Deferral (Issue #240) + +Timestamp: 2026-07-06T08-00 + +AC6 ("All required PR CI checks are green against the PR head SHA") is deferred to post-PR-creation. Verification of AC6 requires a PR to exist with a head SHA and a completed CI run against that SHA; neither exists during local plan execution. AC6 is out of scope for this executor run and will be verified once a PR is opened and CI completes against the PR head SHA. + +The AC6 checkbox in `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md` remains unchecked (`- [ ] AC6: ...`) pending that CI evidence. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/plan-status-summary.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/plan-status-summary.md new file mode 100644 index 000000000..4a2b4b3b4 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/plan-status-summary.md @@ -0,0 +1,44 @@ +# Plan Status Summary (Issue #240) + +Timestamp: 2026-07-06T08-02 + +## Phase 0 — Policy Read & Baseline Capture (complete) + +- P0-T1–T4: policy files read in order — evidenced by `evidence/baseline/phase0-instructions-read.md` +- P0-T5: `evidence/baseline/phase0-instructions-read.md` +- P0-T6: `evidence/baseline/ac-source-confirmation.md` +- P0-T7: `evidence/baseline/git-baseline.md` +- P0-T8: `evidence/baseline/csharpier-baseline.md` +- P0-T9: `evidence/baseline/analyzer-baseline.md` +- P0-T10: `evidence/baseline/nullable-baseline.md` +- P0-T11: `evidence/baseline/test-coverage-baseline.md` + +## Phase 1 — Regression Test First (Red) (complete) + +- P1-T1/P1-T2: two `[expect-fail]` tests added to `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` +- P1-T3: `evidence/regression-testing/fail-before-240.md` (2 failed, 0 passed against pre-fix code) + +## Phase 2 — Minimal Fix (Green) (complete) + +- P2-T1/P2-T2/P2-T3: fix implemented in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (`StoreLaunchReadinessState`, `StoreLaunchReadiness`, `EvaluateLaunchReadiness()`, modified `Launch()`) +- P2-T4: 5 new unit tests added to `StoreWrapperController_Tests.cs` +- P2-T5: `evidence/regression-testing/pass-after-240.md` (4170 passed, 0 failed) + +## Phase 3 — Toolchain & Coverage Verification (complete, one documented deviation) + +- P3-T1: `evidence/qa-gates/qa-01-format.md` (csharpier clean; loop restarted once after an auto-fix pass) +- P3-T2: `evidence/qa-gates/qa-02-analyzers.md` (70 warnings, 0 errors; no new diagnostics on touched files) +- P3-T3: `evidence/qa-gates/qa-03-nullable.md` — **deviation**: the plan's literal "EXIT_CODE 0" acceptance is not achievable at solution scope because of a pre-existing, unrelated nullable-debt condition in vendored/legacy projects (documented in the P0-T10 baseline). The touched files were verified in isolation (scoped rebuild) to introduce zero new nullable diagnostics; a genuine 2-diagnostic regression from this issue's first fix attempt was found and corrected with a narrowly-scoped, documented `#pragma warning disable/restore CS8625`. +- P3-T4: `evidence/qa-gates/qa-04-test-coverage.md` (4170 passed; `EvaluateLaunchReadiness()` 100% coverage) +- P3-T5: `evidence/qa-gates/qa-05-coverage-delta.md` (all three delta checks PASS) + +## Phase 4 — Acceptance Criteria Reconciliation & Documentation (complete) + +- P4-T1/P4-T2: `evidence/other/scope-budget-confirmation.md` (1 production file changed; 396 lines; RibbonController.cs/AppOlObjects.cs untouched) — **second documented deviation**: the test file `StoreWrapperController_Tests.cs` was already 582 lines (over the 500-line policy limit) before this issue; the plan's scope lock required all new tests to land in that single file, growing it to 778 lines. Not resolved unilaterally (would require an unauthorized new outcome — splitting the file); flagged for remediation. +- P4-T3: `issue.md` AC1–AC5 checked with evidence annotations; mirrored to `evidence/issue-updates/issue-240.2026-07-06T07-58.md` +- P4-T4: `evidence/other/ac6-deferral.md` (AC6 deferred to post-PR-creation; left unchecked) +- P4-T5: this file + +## Outcome + +All plan tasks P0-T1 through P4-T5 are checked off in `plan.2026-07-06T06-41.md`. Two deviations are documented above (nullable gate's solution-wide exit code; pre-existing test-file line-count overage) and are surfaced in the executor's completion report rather than resolved by widening scope beyond the plan's authorization. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/scope-budget-confirmation.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/scope-budget-confirmation.md new file mode 100644 index 000000000..f272f7083 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/scope-budget-confirmation.md @@ -0,0 +1,20 @@ +# Scope Budget Confirmation (Issue #240) + +Timestamp: 2026-07-06T07-55 + +## P4-T1 — Small-path budget + +Command: `git diff --name-only 4022fe7c9b07119224ca5aaa880b0a4003ef08db -- '*.cs'` (baseline commit from `evidence/baseline/git-baseline.md`) + +Changed `.cs` files: +- `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (production, 1 file) +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (test, 1 file) + +Exactly one production `.cs` file was changed. `git diff --name-only` against the baseline commit, filtered for `RibbonController|AppOlObjects`, returned no matches — `TaskMaster/Ribbon/RibbonController.cs` and `TaskMaster/AppGlobals/AppOlObjects.cs` were not touched. The small-path budget (1 production file, confined test file) was honored. + +## P4-T2 — File-size limit + +Command: `wc -l UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` +Result: 396 lines. This is `<= 500`, satisfying the repo file-size limit. + +Note (recorded for transparency, not part of this task's pass/fail acceptance): `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` is 778 lines after this change. The pre-existing file was already 582 lines before this issue's edits (a pre-existing violation of the 500-line policy predating issue #240), and the plan's explicit scope lock ("Test changes are confined to `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`") required all 7 new test methods to be added to this single file. This is flagged as a policy-conflict finding in the executor's completion report rather than resolved unilaterally (splitting the test file would be a new, plan-unauthorized outcome). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.md new file mode 100644 index 000000000..eb73a5098 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.md @@ -0,0 +1,14 @@ +# QA Gate 01 — CSharpier Format (Issue #240) + +Timestamp: 2026-07-06T07-35 + +Command (mutation pass): `dotnet tool run csharpier format .` +EXIT_CODE: 0 +Result: Formatted 1269 files (scanned); reformatted the two touched files (`StoreWrapperController.cs`, `StoreWrapperController_Tests.cs`) to normalize indentation/wrapping. No other tracked files were changed (confirmed via `git status --porcelain`). + +Because this mutation step changed files, the toolchain loop was restarted from step 1 per the plan's loop rule. + +Command (verification pass): `dotnet tool run csharpier check .` +EXIT_CODE: 0 + +Output Summary: Checked 1269 files in 3296ms. 0 files require reformatting after the mutation pass. No residual diff on either touched file (confirmed via a targeted `csharpier check` on both files individually: "Checked 2 files in 533ms"). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.md new file mode 100644 index 000000000..9d425f85e --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.md @@ -0,0 +1,9 @@ +# QA Gate 02 — .NET Analyzers (Issue #240) + +Timestamp: 2026-07-06T07-38 + +Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` + +EXIT_CODE: 0 + +Output Summary: Build succeeded. 70 warning(s), 0 error(s) (baseline was 72 warning(s), 0 error(s) — no increase). No files were changed by this step (verification-only build). Neither touched file (`StoreWrapperController.cs`, `StoreWrapperController_Tests.cs`) produced any analyzer diagnostic. The single `StoreWrapperController`-related warning present (`CS0067` in `StoreWrapperControllerTests.cs`, a pre-existing, unrelated file with no underscore in its name) is a pre-existing baseline warning, unaffected by this change. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.md new file mode 100644 index 000000000..3cc62d789 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.md @@ -0,0 +1,25 @@ +# QA Gate 03 — Nullable / TreatWarningsAsErrors (Issue #240) + +Timestamp: 2026-07-06T07-45 + +## Solution-wide command (as specified by the plan) + +Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` +(Invoked as `-t:Rebuild` for the reason recorded in the P0-T10 baseline: an incremental `-t:Build` skips `CoreCompile` for up-to-date outputs.) + +EXIT_CODE: 1 + +This non-zero exit code reproduces the exact pre-existing condition recorded in the P0-T10 baseline (`evidence/baseline/nullable-baseline.md`): forcing `Nullable=enable` across the whole solution fails on the vendored `SVGControl.csproj` and `UtilitiesSwordfish.NET.General.csproj` projects, which are dependencies of `UtilitiesCS.csproj` and therefore block `UtilitiesCS`/`UtilitiesCS.Test` from even reaching `CoreCompile` in a full-solution run. This is unrelated to and pre-dates issue #240. + +## Scoped verification (touched-file diagnostics) + +To satisfy the acceptance clause "confirm 0 warnings/errors on the touched files," `UtilitiesCS.csproj` and `UtilitiesCS.Test.csproj` were rebuilt directly with `-p:BuildProjectReferences=false` (using the already-built vendored-project outputs) so `CoreCompile` actually runs against `StoreWrapperController.cs` and `StoreWrapperController_Tests.cs`. + +Command: `msbuild TaskMaster.sln -t:UtilitiesCS:Rebuild,UtilitiesCS_Test:Rebuild -p:Configuration=Debug -p:Platform="Any CPU" -p:Nullable=enable -p:TreatWarningsAsErrors=true -p:BuildProjectReferences=false` +EXIT_CODE: 1 (2089 pre-existing errors across the wider `UtilitiesCS.csproj`, unrelated to this issue — the project has no `` project setting and was never nullable-annotated; forcing the flag surfaces the whole project's latent debt, not a regression from this change) + +Output Summary: `StoreWrapperController_Tests.cs` contributes zero diagnostics. `StoreWrapperController.cs` contributes diagnostics only on pre-existing, unmodified code (constructor property initialization at line 77 — `CS8618` x8, unchanged since before this fix; and `SelectFolder`/`SelectFsFolder`'s pre-existing `return null;` statements at lines 345/352/371 — `CS8603` x3); these lines are unmodified original code, merely shifted by the new type declarations inserted above the class. A first attempt at this fix introduced 2 new `CS8625` diagnostics (`StoreLaunchReadiness.NotReady` passing `null` literals to non-nullable-typed constructor parameters, at line 50). This was fixed with a narrowly-scoped `#pragma warning disable CS8625` / `#pragma warning restore CS8625` around the two-argument `null, null` construction, documented in-code with a `why:` comment, per repo policy C#7 ("keep suppression as narrow as possible and document the rationale in-code"). A `?`-nullable-annotation approach was rejected because this project has no `` setting (implicit disable for normal builds), so declaring `StoresWrapper?`/`IList?` would introduce new `CS8632` warnings during ordinary (non-forced) builds — confirmed empirically not to occur with the pragma approach (re-verified against the P3-T2 analyzer gate: 70 warnings, 0 errors, no new diagnostics on either touched file). After the fix, the scoped rebuild's total error count for `UtilitiesCS.csproj` dropped from 2091 to 2089 — exactly the 2 diagnostics eliminated, confirming the fix is isolated and introduces zero new nullable diagnostics. + +## Verdict + +No warnings or errors on `StoreWrapperController.cs` or `StoreWrapperController_Tests.cs` are attributable to the code introduced or modified by this issue. Both the solution-wide `EXIT_CODE: 1` and the scoped rebuild's `EXIT_CODE: 1` are pre-existing, unrelated conditions (vendored-project nullable debt and whole-project nullable debt, respectively), identical in kind to the P0-T10 baseline. This is recorded transparently rather than reported as a false `EXIT_CODE: 0`, per the fail-closed evidence rule. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.md new file mode 100644 index 000000000..20dd7e5db --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.md @@ -0,0 +1,21 @@ +# QA Gate 04 — Test + Coverage (Issue #240, post-change) + +Timestamp: 2026-07-06T07-50 + +Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation` + +EXIT_CODE: 0 + +Coverage extraction command: `dotnet-coverage merge .coverage -f xml -o TestResults/final-coverage.xml` + +Output Summary: Test Run Successful. Total tests: 4170, Passed: 4170, Failed: 0. Total time 40.59s. + +Post-change repository (testable-denominator) line coverage for `UtilitiesCS.dll`: **85.88%** (lines_covered=36897, lines_partially_covered=985, lines_not_covered=5082; block_coverage=86.69%). Baseline (P0-T11) was 85.87% — no regression. + +New/changed-code coverage for `StoreWrapperController.cs`: +- `EvaluateLaunchReadiness()` (new, non-`[ExcludeFromCodeCoverage]` decision method): **100.00%** line coverage (13/13 lines covered, 0 not covered), 100.00% block coverage. Exceeds the >= 90% new-code target. +- `StoreLaunchReadiness.NotReady(...)` (new factory): 100.00% line coverage (3/3 lines). +- `StoreLaunchReadiness.Ready(...)` (new factory): 100.00% line coverage (1/1 line; block_coverage 100.00%). +- `Launch()` (modified guard branch): reported as `skipped_function reason="attribute_excluded"` — correctly excluded from the coverage denominator via its retained `[ExcludeFromCodeCoverage]` attribute, per the plan's fix design (WinForms/dialog-construction shell stays exempt; the extracted readiness decision is the non-exempt, tested unit). + +Acceptance verdict: EXIT_CODE 0; new/changed-line coverage on `EvaluateLaunchReadiness()` = 100.00% (>= 90% required); repository line coverage for the testable denominator = 85.88% (>= 80% required). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.md new file mode 100644 index 000000000..73eb6828c --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.md @@ -0,0 +1,19 @@ +# QA Gate 05 — Coverage Delta Verification (Issue #240) + +Timestamp: 2026-07-06T07-52 + +Baseline coverage (P0-T11, `UtilitiesCS.dll`): line_coverage = 85.87% (lines_covered=36873, lines_partially_covered=984, lines_not_covered=5085); test pass count = 4163/4163. + +Post-change coverage (P3-T4, `UtilitiesCS.dll`): line_coverage = 85.88% (lines_covered=36897, lines_partially_covered=985, lines_not_covered=5082); test pass count = 4170/4170. + +New-code coverage (P3-T4): `EvaluateLaunchReadiness()` = 100.00% line coverage (13/13 lines); `StoreLaunchReadiness.NotReady`/`Ready` factories = 100.00% line coverage. + +## Verdict + +| Check | Result | Verdict | +|---|---|---| +| (a) No regression on previously-covered lines | Line coverage moved from 85.87% to 85.88% (+0.01 pp); all 4163 baseline-passing tests still pass (now 4170 total, 0 failed) | PASS | +| (b) New-code coverage on `EvaluateLaunchReadiness()` >= 90% | 100.00% | PASS | +| (c) Repository line coverage remains >= 80% for the testable denominator | 85.88% | PASS | + +All three checks PASS. No regression was introduced; the two new regression tests (P1-T1/P1-T2) and five new unit tests (P2-T4) add coverage without displacing any previously-covered line. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/fail-before-240.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/fail-before-240.md new file mode 100644 index 000000000..aab478a01 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/fail-before-240.md @@ -0,0 +1,14 @@ +# Fail-Before Evidence (Issue #240, Phase 1 Red) + +Timestamp: 2026-07-06T07-25 + +Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /TestCaseFilter:"FullyQualifiedName~Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer|FullyQualifiedName~Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer" /InIsolation` + +EXIT_CODE: 1 + +Output Summary: Test Run Failed. Total tests: 2, Failed: 2, Passed: 0. Both regression tests fail against the pre-fix production code, reproducing the issue #240 crash path: + +- `Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` fails with an unhandled `System.NullReferenceException` at `StoreWrapperController.cs:line 52` (`Model.Stores.Select(...)` on a null `Model`), matching the exact stack trace reported in `issue.md`. +- `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` fails with an unhandled `System.ArgumentNullException` ("Value cannot be null. Parameter name: source") at the same line 52, because `Enumerable.Select` on a null `IEnumerable` (`Model.Stores` is null while `Model` itself is non-null) throws `ArgumentNullException` rather than `NullReferenceException`. This is a more precise characterization than the plan text's blanket "both fail with NullReferenceException": both scenarios reproduce an unhandled, uncaught exception from the same unguarded `Launch()` code path, satisfying AC2's "does not throw" requirement equally; only the concrete CLR exception type differs for the null-list case. + +Both failures confirm the pre-fix defect and establish the fail-before baseline required by AC3. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/pass-after-240.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/pass-after-240.md new file mode 100644 index 000000000..e897b149e --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/pass-after-240.md @@ -0,0 +1,22 @@ +# Pass-After Evidence (Issue #240, Phase 2 Green) + +Timestamp: 2026-07-06T07-30 + +Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation` + +EXIT_CODE: 0 + +Output Summary: Test Run Successful. Total tests: 4170, Passed: 4170, Failed: 0. Total time 21.99s. This is the P0-T11 baseline pass count (4163) plus the 7 new methods introduced by P1-T1, P1-T2, and P2-T4, with zero regressions against the baseline. Both P1 regression tests now pass: + +- `Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` — Passed [67 ms] +- `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` — Passed [68 ms] + +The 5 `EvaluateLaunchReadiness()` unit tests from P2-T4 also pass: + +- `EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable` — Passed +- `EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable` — Passed +- `EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable` — Passed +- `EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable` — Passed +- `EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames` — Passed + +All 20+ pre-existing tests in `StoreWrapperController_Tests.cs` that do not call `Launch()` remain unmodified and continue to pass. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md new file mode 100644 index 000000000..db6eed89d --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md @@ -0,0 +1,85 @@ +# store-wrapper-launch-npe (Issue #240) + +- Date captured: 2026-07-06 +- Author: Dan Moisan +- Status: Promoted -> docs/features/active/store-wrapper-launch-npe/ (Issue #240) + +> Automation note: Keep the section headings below unchanged; the promotion tooling maps each of them into the GitHub bug issue template. + +- Issue: #240 +- Issue URL: https://github.com/drmoisan/TaskMaster/issues/240 +- Last Updated: 2026-07-06 +- Work Mode: minor-audit + +## Summary + +`StoreWrapperController.Launch()` throws an unhandled `System.NullReferenceException` when the Outlook store settings dialog is opened before the store-wrapper model has been initialized. The immediate crash is a missing null guard; the underlying issue is that the ribbon entry point invokes the dialog with no gating on whether async store initialization completed or succeeded. + +## Environment + +- OS/version: Windows, Outlook desktop (VSTO add-in) +- Assembly: UtilitiesCS +- Command/flags used: Ribbon action -> RibbonController.FolderStoresSettings() -> StoreWrapperController.Launch() +- Data source or fixture: Globals.Ol.StoresWrapper (populated by AppOlObjects.LoadStoresAsync during startup) + +## Steps to Reproduce + +1. Start Outlook with the TaskMaster add-in; startup queues `_globals.LoadAsync(false)` on the `IdleAsyncQueue`. +2. Invoke the store/junk-folder settings ribbon action before `LoadStoresAsync()` completes, or in a session where `LoadStoresAsync()` did not populate `StoresWrapper` (config-missing branch logs an error and leaves it null, or deserialization returned null). +3. `StoreWrapperController.Launch()` executes. + +## Expected Behavior + +The dialog either opens with a valid store model, or the command fails gracefully with a clear, user-facing message (and no unhandled exception) when store state is not yet available. + +## Actual Behavior + +Unhandled exception reaches the user: + +``` +System.NullReferenceException + HResult=0x80004003 + Message=Object reference not set to an instance of an object. + Source=UtilitiesCS + StackTrace: + at UtilitiesCS.OutlookObjects.Store.StoreWrapperController.Launch() in ...\UtilitiesCS\OutlookObjects\Store\StoreWrapperController.cs:line 52 +``` + +## Logs / Screenshots + +- [x] Attached minimal logs or screenshot +- Snippet: debugger cause analysis shows `this.Model == null`, `Globals != null`, `Globals.Ol != null`, `Globals.Ol.StoresWrapper == null` at the point of failure. + +## Impact / Severity + +- [ ] Blocker +- [x] High +- [ ] Medium +- [ ] Low + +## Suspected Cause / Notes + +- Immediate defect: `Launch()` (StoreWrapperController.cs lines 50-54) assigns `Model = Globals.Ol.StoresWrapper` and dereferences `Model.Stores.Select(...)` with no null guard. `DisplayName_SelectedValueChanged` (line 89) and `SaveChanges` (line 213) share the same unguarded `Model` dependency. +- Secondary latent risk: even a non-null `StoresWrapper` can transiently have a null `Stores` list. `[OnDeserialized] RewireOlObjects` fires-and-forgets the rewire (`_ = RewireAfterDeserializeWithLoggingAsync()`), and `Stores ??= []` runs only inside that async path. `Launch()` guards neither `Model` nor `Model.Stores`. +- Underlying bug: `StoresWrapper` is populated only by `AppOlObjects.LoadStoresAsync()` during async startup (`ThisAddIn` queues `_globals.LoadAsync(false)` on `IdleAsyncQueue`). `RibbonController.FolderStoresSettings()` invokes `Launch()` with no readiness gate and no recovery when the load has not completed or failed. +- Files to inspect: `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, `TaskMaster/Ribbon/RibbonController.cs`, `TaskMaster/AppGlobals/AppOlObjects.cs`, `UtilitiesCS/OutlookObjects/Store/StoresWrapper.cs`. + +## Proposed Fix / Validation Ideas + +- [x] Unit coverage areas: `Launch()` null-model and null-stores behavior; graceful-failure path. +- [x] Integration scenario to retest: open store settings dialog before/after store load completes. +- [x] Manual verification notes: confirm no unhandled exception when store state is unavailable; confirm normal open when populated. + +## Acceptance Criteria + +- [x] AC1: `StoreWrapperController.Launch()` does not throw an unhandled `NullReferenceException` when `Globals.Ol.StoresWrapper` (`Model`) is null. It fails gracefully with a clear user-facing message and returns without opening a broken dialog. (Evidence: `evidence/regression-testing/pass-after-240.md`, fix in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` P2-T3.) +- [x] AC2: `Launch()` also handles a non-null `Model` whose `Stores` list is null (transient post-deserialize state) without throwing. (Evidence: `evidence/regression-testing/pass-after-240.md`, fix in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` P2-T3.) +- [x] AC3: A deterministic MSTest regression test reproduces the pre-fix crash path (fails before the fix, passes after) using Moq for `IApplicationGlobals`/`IOlObjects`; no live Outlook, no temporary files. (Evidence: `evidence/regression-testing/fail-before-240.md` and `evidence/regression-testing/pass-after-240.md`, P1-T3/P2-T5.) +- [x] AC4: The underlying readiness/initialization gap identified by root-cause research is addressed so that invoking the store-settings command when store state is unavailable produces deterministic, non-crashing behavior rather than an unhandled exception. (Evidence: `EvaluateLaunchReadiness()` in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, P2-T1/P2-T2/P2-T3.) +- [x] AC5: The full C# toolchain passes in order (csharpier -> .NET analyzers -> nullable/TreatWarningsAsErrors -> MSTest with coverage); coverage on changed lines meets the >= 90% new-code target and repository line coverage remains >= 80% for the testable denominator. (Evidence: `evidence/qa-gates/qa-01-format.md` through `evidence/qa-gates/qa-05-coverage-delta.md`, P3-T1 through P3-T5. Note: the solution-wide nullable gate's raw `EXIT_CODE` is 1 due to a pre-existing, unrelated condition documented in `evidence/qa-gates/qa-03-nullable.md`; the touched files themselves introduce zero new nullable diagnostics.) +- [ ] AC6: All required PR CI checks are green against the PR head SHA. + +## Next Step + +- [x] Promote to GitHub issue (bug-report template) +- [x] Move to active fix folder / branch diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/plan.2026-07-06T06-41.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/plan.2026-07-06T06-41.md new file mode 100644 index 000000000..80f946090 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/plan.2026-07-06T06-41.md @@ -0,0 +1,83 @@ +# store-wrapper-launch-npe (Plan) + +- **Issue:** #240 +- **Parent (optional):** none +- **Owner:** drmoisan +- **Last Updated:** 2026-07-06T06-41 +- **Status:** Draft +- **Version:** 0.2 +- **Work Mode:** minor-audit (bugfix) +- **Feature folder:** `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/` + +**Fail-closed evidence rule:** Every baseline, regression, and final-QA command task below writes its own evidence artifact under `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence//`. If any required artifact is missing or incomplete (missing `Timestamp:`, `Command:`, `EXIT_CODE:`, or `Output Summary:`), the corresponding checklist item MUST remain unchecked and the plan outcome MUST be reported as remediation-required, never PASS. + +**Evidence location invariant:** All evidence in this plan resolves to `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence//` (canonical sub-paths: `baseline/`, `regression-testing/`, `qa-gates/`, `issue-updates/`, `other/`). No task in this plan writes to `artifacts/baselines/`, `artifacts/qa/`, `artifacts/coverage/`, or any other non-canonical path. + +**Scope lock (small-path budget):** Production changes are confined to `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (1 production file). Test changes are confined to `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`. No change to `TaskMaster/Ribbon/RibbonController.cs` or `TaskMaster/AppGlobals/AppOlObjects.cs` is in scope for this issue (see research §4 "Rejected Alternatives"). + +**Fix design (binding on Phase 2):** Add an internal readiness-result type and an internal, non-`[ExcludeFromCodeCoverage]` decision method `EvaluateLaunchReadiness()` to `StoreWrapperController`, computed from `Globals?.Ol?.StoresWrapper`. `Launch()` remains the thin `[ExcludeFromCodeCoverage]` WinForms shell: it calls `EvaluateLaunchReadiness()`, shows a `MyBox` user-facing message and returns without constructing the viewer when the result is not `Ready`, and otherwise proceeds with the existing `Viewer`/`DataSource` binding using the readiness result's model and display names. + +--- + +### Phase 0 — Policy Read & Baseline Capture + +- [x] [P0-T1] Read `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\CLAUDE.md` in full (policy order position 1). Acceptance: file read start-to-end; no section skipped. +- [x] [P0-T2] Read `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\general-code-change.md` (policy order position 2). Acceptance: file read start-to-end. +- [x] [P0-T3] Read `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\general-unit-test.md` (policy order position 3). Acceptance: file read start-to-end. +- [x] [P0-T4] Read `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\csharp.md` (policy order position 4, C#-specific). Acceptance: file read start-to-end. +- [x] [P0-T5] Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/phase0-instructions-read.md` containing `Timestamp:` (ISO-8601 `yyyy-MM-ddTHH-mm`), `Policy Order:` (the 4-item ordered list from P0-T1-T4), and an explicit list of the 4 file paths read. Acceptance: artifact exists with all three fields populated. +- [x] [P0-T6] Confirm `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md` contains an explicit `## Acceptance Criteria` heading with AC1-AC6 listed beneath it, and treat only that section as the AC source for this minor-audit plan. Record the confirmation (heading present, AC1-AC6 count = 6) in `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/ac-source-confirmation.md`. Acceptance: artifact records `## Acceptance Criteria` found at issue.md and lists AC1-AC6 verbatim. +- [x] [P0-T7] Capture the git baseline by running `git rev-parse HEAD` and `git branch --show-current` from the repo root. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/git-baseline.md` with `Timestamp:`, `Command:` (both commands), `EXIT_CODE:`, and `Output Summary:` (branch name and short SHA). Acceptance: artifact contains both command outputs. +- [x] [P0-T8] Capture the formatting baseline by running `dotnet tool run csharpier --check .` from the repo root (check-only, no mutation, to establish pre-change formatting state). Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/csharpier-baseline.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` (pass, or count of unformatted files). Acceptance: artifact exists with all four fields; EXIT_CODE is not `SKIPPED`. +- [x] [P0-T9] Capture the analyzer-build baseline by running `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` from the repo root. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/analyzer-baseline.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` (build result, warning/error counts). Acceptance: artifact exists with all four fields. +- [x] [P0-T10] Capture the nullable/type-check baseline by running `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` from the repo root. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/nullable-baseline.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` (build result, warning/error counts). Acceptance: artifact exists with all four fields. +- [x] [P0-T11] Capture the pre-change test + coverage baseline by running `vstest.console.exe UtilitiesCS.Test\bin\Debug\UtilitiesCS.Test.dll /EnableCodeCoverage` (existing suite, no `Launch()` calls exist yet). Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/baseline/test-coverage-baseline.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` including the numeric pass/fail count and the overall repository line-coverage percentage. Acceptance: artifact records a numeric coverage percentage (not `UNVERIFIED`). + +### Phase 1 — Regression Test First (Red) + +- [x] [P1-T1] [expect-fail] Add MSTest method `Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` to `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`: arrange `Mock` + `Mock` with `mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null)` and `mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object)` (do not set up `LoadAsync`); save/restore `MyBox.DialogInvoker` in a `try`/`finally`, stubbing it to a non-modal delegate that records invocation count; act via `Action act = () => controller.Launch();`; assert with FluentAssertions that `act` does not throw, that the dialog-invoker stub was called exactly once, and that `controller.Viewer` is `null` after the call (AC1). Acceptance: method compiles and is discoverable by the test runner. +- [x] [P1-T2] [expect-fail] Add MSTest method `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` to `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`: identical arrangement to P1-T1 except `mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null })`; same act/assert shape (AC2). Acceptance: method compiles and is discoverable by the test runner. +- [x] [P1-T3] [expect-fail] Run `vstest.console.exe UtilitiesCS.Test\bin\Debug\UtilitiesCS.Test.dll /TestCaseFilter:"FullyQualifiedName~Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer|FullyQualifiedName~Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer"` against the pre-fix production code and confirm both new tests FAIL with an unhandled `NullReferenceException` (reproducing the issue #240 crash). Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/fail-before-240.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` (both test names + `NullReferenceException` failure reason). Acceptance: artifact records 2 failed, 0 passed for the filtered run; EXIT_CODE reflects the failing run and is not `SKIPPED`. + +### Phase 2 — Minimal Fix (Green) + +- [x] [P2-T1] In `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, add an `internal enum StoreLaunchReadinessState { Ready, ModelUnavailable, StoresUnavailable }` and an `internal readonly struct StoreLaunchReadiness` exposing `State`, `Model` (`StoresWrapper`), and `DisplayNames` (`IList`), with private constructor plus `internal static StoreLaunchReadiness NotReady(StoreLaunchReadinessState state)` and `internal static StoreLaunchReadiness Ready(StoresWrapper model, IList displayNames)` factory methods; add a one-line XML doc comment on each new type. Acceptance: both types compile inside the `StoreWrapperController` class or file scope with no other member signatures changed. +- [x] [P2-T2] In `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, add `internal StoreLaunchReadiness EvaluateLaunchReadiness()` (no `[ExcludeFromCodeCoverage]` attribute) that reads `Globals?.Ol?.StoresWrapper`; returns `StoreLaunchReadiness.NotReady(StoreLaunchReadinessState.ModelUnavailable)` when the model is `null`; returns `StoreLaunchReadiness.NotReady(StoreLaunchReadinessState.StoresUnavailable)` when the model's `Stores` is `null`; otherwise returns `StoreLaunchReadiness.Ready(model, model.Stores.Select(store => store.DisplayName).ToList())`. Acceptance: method compiles, is `internal` (not `public`), and carries no `[ExcludeFromCodeCoverage]` attribute. +- [x] [P2-T3] In `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, modify `Launch()` (keep its existing `[ExcludeFromCodeCoverage]` attribute) to: call `var readiness = EvaluateLaunchReadiness();`; when `readiness.State != StoreLaunchReadinessState.Ready`, call `MyBox.ShowDialog("Store settings are not available yet. Please try again after startup completes.", "Store Settings Unavailable", MessageBoxButtons.OK, MessageBoxIcon.Warning);` and `return;` without constructing `Viewer`; otherwise set `FsConverter`, set `Model = readiness.Model;`, construct `Viewer = new StoreWrapperViewer(this);`, set `Viewer.DisplayName.DataSource = readiness.DisplayNames;`, and call `Viewer.ShowDialog();` as before. Acceptance: `Launch()` no longer dereferences `Model.Stores` without a prior readiness check; the existing 20+ tests in `StoreWrapperController_Tests.cs` that do not call `Launch()` remain unmodified and continue to compile. +- [x] [P2-T4] Add 5 MSTest methods to `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` targeting `EvaluateLaunchReadiness()` directly (no `MyBox`/WinForms involvement): `EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable` (construct controller with `null!` globals), `EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable` (`mockGlobals.SetupGet(g => g.Ol).Returns((IOlObjects)null)`), `EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable`, `EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable`, and `EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames` (assert `State == Ready` and `DisplayNames` matches the seeded `Stores` display names via FluentAssertions `Should().Equal(...)`). Acceptance: all 5 methods compile and are discoverable by the test runner; together with P1-T1/P1-T2 this is 7 new test methods. +- [x] [P2-T5] Run `vstest.console.exe UtilitiesCS.Test\bin\Debug\UtilitiesCS.Test.dll /EnableCodeCoverage` for the full `UtilitiesCS.Test` assembly (existing suite + 7 new methods from P1-T1, P1-T2, P2-T4) and confirm 100% pass with zero regressions against the P0-T11 baseline pass count. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/regression-testing/pass-after-240.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` (pass/fail counts, confirmation the two P1 regression tests now pass). Acceptance: artifact records 0 failed for the full assembly run; EXIT_CODE is `0`. + +### Phase 3 — Toolchain & Coverage Verification (Final QA Loop) + +**Loop rule:** If any of P3-T1 through P3-T4 fails or changes/auto-fixes any file, restart the loop from P3-T1. Do not proceed to P3-T5 until P3-T1 through P3-T4 complete cleanly in a single pass. + +- [x] [P3-T1] Run `dotnet tool run csharpier .` (formatting, repo root) and confirm exit code `0` with no residual diff on `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` or `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` after the run. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:`. Acceptance: EXIT_CODE `0`; artifact records no files reformatted (or restart the loop if files were reformatted). +- [x] [P3-T2] Run `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` and confirm 0 new analyzer diagnostics relative to the P0-T9 baseline. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:`. Acceptance: EXIT_CODE `0`; artifact records diagnostic count and confirms no increase over baseline. +- [x] [P3-T3] Run `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` and confirm 0 warnings/errors on the touched files. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:`. Acceptance: EXIT_CODE `0`. +- [x] [P3-T4] Run `vstest.console.exe UtilitiesCS.Test\bin\Debug\UtilitiesCS.Test.dll /EnableCodeCoverage` (post-change, full assembly) and record the numeric post-change repository line-coverage percentage and the changed-line coverage for `StoreWrapperController.cs` (the new `EvaluateLaunchReadiness()` method and the modified `Launch()` guard branch). Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.md` with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` including both numeric values. Acceptance: EXIT_CODE `0`; new/changed-line coverage on `EvaluateLaunchReadiness()` >= 90%; repository line coverage for the testable denominator >= 80%. +- [x] [P3-T5] Compare the P0-T11 baseline coverage and pass count against the P3-T4 post-change values and confirm (a) no regression on previously-covered lines, (b) new-code coverage on `EvaluateLaunchReadiness()` >= 90%, and (c) repository line coverage remains >= 80% for the testable denominator. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.md` with `Timestamp:`, baseline coverage value, post-change coverage value, new-code coverage value, and a pass/fail verdict for each of the three checks. Acceptance: all three checks recorded as PASS; if any check is not PASS, the plan outcome is remediation-required, not PASS. + +### Phase 4 — Acceptance Criteria Reconciliation & Documentation + +- [x] [P4-T1] Confirm the small-path budget was honored: exactly one production file (`UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`) was modified for this change, with no edits to `TaskMaster/Ribbon/RibbonController.cs` or `TaskMaster/AppGlobals/AppOlObjects.cs`. Write `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/scope-budget-confirmation.md` with `Timestamp:` and the list of changed production files (via `git diff --name-only` against the P0-T7 baseline commit). Acceptance: artifact lists exactly one production `.cs` file changed. +- [x] [P4-T2] Confirm `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` is `<= 500` lines after the Phase 2 edits (repo file-size limit). Record the line count in `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/scope-budget-confirmation.md` (append to the P4-T1 artifact). Acceptance: recorded line count is `<= 500`. +- [x] [P4-T3] Update `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md` to check AC1, AC2, AC3, AC4, and AC5 under `## Acceptance Criteria`, each annotated with the evidence artifact path that satisfies it (P1-T3/P2-T5 for AC3, P2-T3 for AC1/AC2/AC4, P3-T1 through P3-T5 for AC5). Mirror the exact updated section to `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/issue-updates/issue-240..md` with `Timestamp:`, the exact text posted, and `PostedAs:`. Acceptance: both `issue.md` and the mirror artifact show AC1-AC5 checked with evidence-path annotations. +- [x] [P4-T4] Record AC6 (required PR CI checks green against the PR head SHA) as deferred-to-PR in `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/ac6-deferral.md`, stating that AC6 is verified after PR creation via CI on the PR head SHA and is out of scope for local plan execution; leave AC6 unchecked in `issue.md` until that CI evidence exists. Acceptance: artifact explicitly states the deferral reason and the checkbox for AC6 remains unchecked. +- [x] [P4-T5] Write a final plan-status summary to `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/plan-status-summary.md` listing each phase's completion state and the evidence artifact path backing it. Acceptance: summary references every evidence artifact produced in P0-T5 through P4-T4 by path. + +--- + +## Acceptance Criteria Traceability + +| AC | Satisfied by | +|----|--------------| +| AC1 (null `Model` guarded, graceful message, no throw) | P2-T3 (fix), P1-T1/P1-T3 (regression), P2-T5 (pass-after) | +| AC2 (null `Model.Stores` guarded) | P2-T3 (fix), P1-T2/P1-T3 (regression), P2-T5 (pass-after) | +| AC3 (deterministic MSTest regression, fail-before/pass-after, Moq, no live Outlook, no temp files) | P1-T1, P1-T2, P1-T3 (fail-before), P2-T5 (pass-after) | +| AC4 (underlying readiness gap addressed deterministically for all root causes a-d) | P2-T1, P2-T2, P2-T3 (readiness decision covers null-Model and null-Stores paths for causes a/b/c/d) | +| AC5 (full toolchain passes in order; >=90% new-code coverage; >=80% repo coverage) | P3-T1 through P3-T5 | +| AC6 (PR CI checks green against PR head SHA) | P4-T4 (deferred to post-PR-creation) | + +## Preflight + +`DIRECTIVE: PREFLIGHT VALIDATION ONLY` diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/research/2026-07-06T00-00-store-wrapper-launch-npe-240-research.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/research/2026-07-06T00-00-store-wrapper-launch-npe-240-research.md new file mode 100644 index 000000000..2bbc00494 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/research/2026-07-06T00-00-store-wrapper-launch-npe-240-research.md @@ -0,0 +1,170 @@ +# Root-Cause Research: StoreWrapperController.Launch NullReferenceException (Issue #240) + +- Timestamp: 2026-07-06T00-00 +- Feature: docs/features/active/2026-07-06-store-wrapper-launch-npe-240 +- Scope: research only, no production code modified +- Canonical issue number: 240 + +## 1. Current-State Analysis (verified) + +### Crash site + +`UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` lines 47-57: + +```csharp +[ExcludeFromCodeCoverage] +public void Launch() +{ + FsConverter = new FilePathHelperConverter(Globals.FS).GetSerializablePath; + Model = Globals.Ol.StoresWrapper; // line 50 + Viewer = new StoreWrapperViewer(this); // line 51 + Viewer.DisplayName.DataSource = Model // line 52 -> NRE when Model is null + .Stores.Select(store => store.DisplayName) + .ToList(); + Viewer.ShowDialog(); +} +``` + +Verified facts: +- `Model` is assigned directly from `Globals.Ol.StoresWrapper` (line 50) with no guard. +- Line 52 dereferences `Model.Stores` and enumerates it. Two independent null dereferences are possible here: `Model == null` (the observed failure) and `Model.Stores == null`. +- `Launch()` carries `[ExcludeFromCodeCoverage]` (line 46). This is material to the fix design (see Section 6): any guard placed inline in `Launch()` is excluded from the coverage denominator and cannot satisfy the AC5 >= 90% changed-line target. +- The same unguarded `Model` dependency exists in `DisplayName_SelectedValueChanged` (line 89: `Model.Stores.Find(...)`) and `SaveChanges` (line 213: `Model.Serialize()`). Both are reachable only after a dialog is already open, so they are not the crash entry point but share the latent dependency. + +### Population path for `StoresWrapper` + +`TaskMaster/AppGlobals/AppOlObjects.cs`: +- `StoresWrapper` is a plain auto-property (line 244), default null. +- `LoadStoresAsync()` (lines 251-265) is the only populator: + - Config-present branch: deserializes into `StoresWrapper` (lines 255-258), then `await AwaitStoreRewireAsync(StoresWrapper)` (line 259). + - Config-missing branch (lines 261-264): logs `"StoresWrapper config not found."` and leaves `StoresWrapper` null permanently. +- `AwaitStoreRewireAsync` (lines 246-249) guards null (`storesWrapper is null ? Task.CompletedTask : storesWrapper.RewireAfterDeserializeAsync()`), so a null deserialize result yields no exception and leaves `StoresWrapper` null. +- `LoadAsync()` (lines 34-38) awaits `LoadStoresAsync()`. + +### Startup queueing + +`TaskMaster/ThisAddIn.cs` `Application_Startup()` (lines 58-69): +- Enqueues `await _globals.LoadAsync(false)` on `IdleAsyncQueue` (line 64). The call is asynchronous and not complete when `Application_Startup` returns. +- After the awaited load it sets `_currentStartupStageLabel = StartupStageLabels.PostLoad` and `_startupPostLoadReached = true` (lines 66-67). `_startupPostLoadReached` is a `private bool` field of `ThisAddIn` (line 117); it is not exposed on any interface reachable from the ribbon path. + +### Ribbon entry point (no gating) + +`TaskMaster/Ribbon/RibbonController.cs` lines 259-263: + +```csharp +internal void FolderStoresSettings() +{ + var wrapper = new StoreWrapperController(Globals); + wrapper.Launch(); +} +``` + +There is no readiness check, no null check, and no try/catch. The ribbon can fire at any time after add-in load, including before the `IdleAsyncQueue` entry that runs `LoadStoresAsync()` has drained. + +### `StoresWrapper.Stores` lifecycle + +`UtilitiesCS/OutlookObjects/Store/StoresWrapper.cs`: +- `Stores` is a public settable `List` (line 317), default null. +- `[OnDeserialized] RewireOlObjects` (lines 60-64) fire-and-forgets: `_ = RewireAfterDeserializeWithLoggingAsync();`. +- `Stores ??= [];` occurs only inside `RewireOlObjectsAsync` (line 85), which runs on the async rewire path. +- Along the `LoadStoresAsync` awaited path, `AwaitStoreRewireAsync` -> `RewireAfterDeserializeAsync` -> `RewireOlObjectsAsync` sets `Stores ??= []`. So once `LoadStoresAsync` completes, `Stores` is non-null. The transient window is: `StoresWrapper` assigned (AppOlObjects line 255) but the awaited rewire (line 259) has not yet run and the `[OnDeserialized]` fire-and-forget has not yet completed. A read in that window observes non-null `Model` with null `Stores`. + +## 2. Root-Cause Ranking (evidence-based) + +The debugger snapshot at failure (`Model == null`, `Globals != null`, `Globals.Ol != null`, `Globals.Ol.StoresWrapper == null`) matches every cause that leaves `StoresWrapper` null. Ranking: + +1. **(a) Ribbon invoked before `IdleAsyncQueue` `LoadStoresAsync` completes — most likely, operative.** + Evidence: `LoadAsync` is queued asynchronously on `IdleAsyncQueue` (ThisAddIn line 64); the ribbon (`FolderStoresSettings`) has no gating. `StoresWrapper` is null until `LoadStoresAsync` assigns it (AppOlObjects line 255). The exposure window is the entire interval between add-in load and idle-queue drain, which on cold start can be long (issue #211 diagnostics reference a multi-second-to-minute startup). This exactly reproduces the observed `StoresWrapper == null` snapshot. + +2. **(b) `LoadStoresAsync` config-missing branch leaves `StoresWrapper` null — likely in misconfigured sessions, deterministic (not a race).** + Evidence: AppOlObjects lines 261-264 only log and return; `StoresWrapper` stays null for the entire session. Produces the identical `Model == null` crash and is permanent, so the dialog can never open in such a session. This is a distinct, non-timing root cause with the same crash signature. + +3. **(c) Deserialization returning null — possible edge, reachable.** + Evidence: config-present branch assigns the deserialize result directly (lines 255-258); a null result is not rejected, and `AwaitStoreRewireAsync` tolerates null (lines 246-249). `StoresWrapper` remains null. Lower likelihood absent evidence of corrupt config, but structurally reachable and indistinguishable from (a)/(b) at the crash site. + +4. **(d) Non-null `StoresWrapper` with transiently-null `Stores` — latent secondary, narrower race.** + Evidence: `Stores` defaults null (line 317); `Stores ??= []` runs only in the async rewire (line 85); `[OnDeserialized]` fires-and-forgets (line 63). A read between assignment (AppOlObjects line 255) and rewire completion observes non-null `Model`, null `Stores`, which throws at `Model.Stores.Select` (controller line 52). This does not match the specific debugger snapshot (which showed `StoresWrapper == null`) but is a real defect and is the AC2 concern. + +Conclusion: the observed crash is (a)/(b)/(c) (`Model == null`); (d) is a latent second null-dereference on the same line. A correct fix must guard both `Model` and `Model.Stores`. + +## 3. Existing Readiness-Gating Pattern — Reuse Assessment + +Verified that a readiness abstraction exists but is semantically wrong for this defect: + +- `IOutlookReadinessGate` / `OutlookReadinessGate` (`UtilitiesCS/OutlookObjects/`) and `HookReadinessCoordinator` (`TaskMaster/AppGlobals/`) were introduced for issue #207. `OutlookReadinessGate.IsReady()` (lines 55-66) probes whether the Outlook **default store's default inbox folder** is reachable over COM. It says nothing about whether the deserialized `StoresWrapper` model has been populated. `IsReady()` can return true (COM store reachable) while `StoresWrapper` is still null (the `IdleAsyncQueue` load has not run, or hit the config-missing branch). These are different concerns loaded by different paths, so this gate is not a valid proxy for "store model ready." +- `_startupPostLoadReached` (ThisAddIn line 117) is a private field, not surfaced on `IApplicationGlobals` or `IOlObjects`. The ribbon path holds only `Globals` (`RibbonController` line 261) and cannot read it. +- `IApplicationGlobals` (line 11 exposes `IOlObjects Ol`) and `IOlObjects` expose no load-completion flag; `IOlObjects` exposes `StoresWrapper { get; set; }` and `LoadAsync()` only (IOlObjects.cs lines 24, 37). + +Finding: there is no existing readiness signal the `Launch()` path can consult that means "StoresWrapper is populated." The only direct, correct readiness signal reachable from the controller is the presence of `Globals?.Ol?.StoresWrapper` and its `Stores`. A new external gating mechanism is not warranted; the guard should test the model state directly. + +## 4. Recommended Fix Posture (bugfix workflow) + +Distinguish two layers, both satisfied by one change in the controller: + +- **Immediate guard (AC1, AC2):** In the store-launch flow, detect `Model == null` or `Model.Stores == null` before constructing/binding the viewer. On detection, present a clear user-facing message via the `MyBox` surface (e.g., "Store settings are not available yet. Please try again after startup completes.") and return without opening a broken dialog. +- **Underlying-bug remediation (AC4):** Because causes (a), (b), (c), and (d) all converge on the same two null states observable at the controller, guarding the model state at the controller produces deterministic, non-crashing behavior for every identified root cause. This satisfies AC4's requirement that invoking the command when store state is unavailable yields deterministic, non-crashing behavior. No timing/coordination mechanism is required. + +Where to fix, and why: +- **Primary: `StoreWrapperController.Launch()` (the controller).** This is the crash site and the correct cohesion boundary: it covers all callers of the controller regardless of entry point and keeps the guard next to the state it validates. Recommended. +- **Not `RibbonController.FolderStoresSettings()`.** It is a thin pass-through (3 lines). Gating there would cover only the ribbon path, would duplicate the null logic, and would leave the controller itself crash-prone if invoked elsewhere. Avoid. +- **Not `AppOlObjects` for issue #240.** The config-missing branch (cause b) leaving `StoresWrapper` permanently null is a separate latent defect. Changing it (e.g., initializing an empty `StoresWrapper`) would alter startup semantics and widen scope beyond the bugfix. Record as an optional follow-up; it is not required to close #240 because the controller guard already delivers deterministic behavior in the config-missing session. + +Minimal-fix conclusion: one production file (`StoreWrapperController.cs`). No change to `RibbonController.cs` or `AppOlObjects.cs` is needed to satisfy AC1-AC4. + +### Coverage-driven design detail (interacts with AC5) + +`Launch()` is `[ExcludeFromCodeCoverage]` (controller line 46) and also constructs `StoreWrapperViewer` (WinForms) and calls `ShowDialog()`. A guard written inline in `Launch()` would be excluded from coverage, so it cannot meet the AC5 >= 90% changed-line target. Recommended structure: + +- Extract the readiness decision into a small non-exempt, testable member — for example an `internal bool TryGetStoreDisplayNames(out IList names)` or an `internal StoreLaunchReadiness EvaluateModel()` that returns a result and the display-name list, computed from `Globals?.Ol?.StoresWrapper`. +- Keep `Launch()` as the thin, coverage-exempt shell that calls the extracted method, shows the `MyBox` message on the not-ready result, or constructs the viewer and binds `DataSource` on the ready result. +- This mirrors the repository's established seam pattern in the same class (click handlers route through the `Viewer` seam; `MyBox` routes through `DialogInvoker`) and keeps the covered logic out of the WinForms shell. + +## 5. Deterministic MSTest Seams (AC3) + +Verified seams in `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`: + +- **Constructor injection:** `new StoreWrapperController(IApplicationGlobals)` (controller lines 25-28). Tests already use `Mock` + `Mock` and `SetupGet(g => g.Ol)` / `SetupGet(o => o.NamespaceMAPI)` (test lines 379-386, 527-538), so this pattern is proven in-repo. +- **Reproduce cause (a)/(b)/(c) — null Model:** `mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null);` then assert the extracted decision method reports not-ready (or that binding is skipped). `StoresWrapper` is a settable property on `IOlObjects` (IOlObjects.cs line 24), so `SetupGet` is valid. +- **Reproduce cause (d) — non-null Model, null Stores (AC2):** `mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null });`. `Stores` is public settable (StoresWrapper.cs line 317). Assert the decision method reports not-ready without throwing. +- **Positive path:** `new StoresWrapper { Stores = new List { new StoreWrapper(null) { DisplayName = "X" } } }` — the existing test at lines 480-484 already constructs `StoresWrapper` with a `Stores` list, confirming this is achievable without live Outlook. +- **User-message surface (`MyBox`):** `MyBox.DialogInvoker` is an injectable `AsyncLocal` seam (MyBox.cs lines 28-43); the existing test at lines 488-497 sets `MyBox.DialogInvoker = _ => DialogResult.Yes` and restores it in a `finally`. If the guard shows a `MyBox` message, a test can set this seam to avoid a modal. The lower-risk approach is to target the extracted decision method directly so the test asserts readiness state without invoking `MyBox` or WinForms at all. +- **Moq caveat (documented in-repo):** test comment at lines 336-343 notes that `Mock` of Task-bearing interfaces can throw `TypeInitializationException` (missing `System.Threading.Tasks.Extensions 4.2.0.1`) when Moq's `AwaitableFactory` initializes. `IOlObjects` extends `INotifyPropertyChanged` and exposes `LoadAsync()` (Task-returning). The existing passing tests create `Mock` successfully because they only set non-Task members (`NamespaceMAPI`, `StoresWrapper`, `Ol`). The regression test must follow the same practice: set only `Ol` and `StoresWrapper`, never force setup of `LoadAsync`. + +Preserving existing tests: the extract-and-guard approach adds a new internal method and leaves the signatures of `ButtonOk_Click`, `ButtonCancel_Click`, `SaveChanges`, `PairwiseEquals`, `GetRelativeFsPath`, `PopulateWithCurrent`, the click handlers, and `SelectFolder` unchanged, so the 20+ existing tests continue to compile and pass without modification. + +## 6. Change-Budget Confirmation (AC of small-path budget) + +- Production files touched: **1** — `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`. +- No production change required in `RibbonController.cs` or `AppOlObjects.cs` to satisfy AC1-AC4. +- Test files touched: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (added regression tests). Test files do not count against the production small-path budget. + +The change stays within the 1-3 production-file small-path budget with margin. It does not need to expand. The optional `AppOlObjects` config-missing follow-up, if pursued, would be a separate scoped change and should not be bundled into #240. + +## Automation Feasibility + +The fix is fully implementable and verifiable in-repo with the standard C# toolchain and MSTest, with no third-party UI, portal, or human-interaction dependency: + +- **Format:** `dotnet tool run csharpier .` — file-based, no external service. +- **Analyze:** `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true`. +- **Type-check:** `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true`. +- **Test:** `vstest.console.exe /EnableCodeCoverage` — MSTest + Moq + FluentAssertions. +- The regression test reproduces the null-`Model` and null-`Stores` crash paths using `Mock` / `Mock` with no live Outlook process. No temporary files are used (compliant with the repo prohibition). The user-facing message is exercised through the `MyBox.DialogInvoker` `AsyncLocal` seam or avoided entirely by targeting the extracted decision method. +- No network, filesystem, portal, or human step is required at any stage. The full four-stage toolchain runs locally and in CI deterministically. + +## Rejected Alternatives (brief) + +- **Reuse `IOutlookReadinessGate` / `HookReadinessCoordinator` to gate the ribbon action.** Rejected: `IsReady()` probes COM store/inbox reachability, not `StoresWrapper` population; it can report ready while `StoresWrapper` is null. Semantically incorrect signal and adds cross-assembly wiring for no benefit. +- **Gate in `RibbonController.FolderStoresSettings()`.** Rejected: covers only the ribbon path, duplicates null logic, and leaves the controller crash-prone from other call sites. +- **Fix `AppOlObjects.LoadStoresAsync` config-missing branch as part of #240.** Rejected for this issue: changes startup semantics and widens scope; the controller guard already yields deterministic behavior in that session. Record as optional follow-up. + +## File References + +- `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (crash site lines 47-57; shared `Model` deps lines 89, 213; `[ExcludeFromCodeCoverage]` line 46) +- `TaskMaster/AppGlobals/AppOlObjects.cs` (`StoresWrapper` line 244; `LoadStoresAsync` lines 251-265; `AwaitStoreRewireAsync` lines 246-249) +- `TaskMaster/ThisAddIn.cs` (`Application_Startup` queueing lines 58-69; `_startupPostLoadReached` line 117) +- `TaskMaster/Ribbon/RibbonController.cs` (`FolderStoresSettings` lines 259-263) +- `UtilitiesCS/OutlookObjects/Store/StoresWrapper.cs` (`Stores` line 317; `[OnDeserialized]` lines 60-64; `Stores ??= []` line 85) +- `UtilitiesCS/OutlookObjects/IOutlookReadinessGate.cs`, `UtilitiesCS/OutlookObjects/OutlookReadinessGate.cs` (readiness gate, IsReady lines 55-66) +- `UtilitiesCS/Interfaces/IGlobals/IOlObjects.cs` (line 24 `StoresWrapper`, line 37 `LoadAsync`) +- `UtilitiesCS/Dialogs/MyBox.cs` (`DialogInvoker` seam lines 28-43; `ShowDialog` overloads) +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (Moq patterns lines 379-386, 527-538; `MyBox.DialogInvoker` usage lines 488-497; Moq Task-interface caveat lines 336-343) From 9e3615b9dd369e66338b4ad333fb7c5371ece0dd Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 6 Jul 2026 08:14:42 -0400 Subject: [PATCH 2/5] refactor(test): split StoreWrapperController_Tests into partial classes - Split 781-line test file into three partial-class files, each under the 500-line limit - Trimmed original StoreWrapperController_Tests.cs to 181 lines - New ButtonAndPopulate partial: 396 lines; new Launch partial: 234 lines - Added two Compile Include entries to UtilitiesCS.Test.csproj - All 39 test methods preserved; no behavior change; production code unchanged - Includes feature-review audit artifacts and remediation evidence Refs: #240 --- .claude/agent-memory/feature-review/MEMORY.md | 1 + ...results-coverage-xml-cross-module-check.md | 14 + ...apperController_Tests.ButtonAndPopulate.cs | 396 ++++++++++++ .../StoreWrapperController_Tests.Launch.cs | 234 +++++++ .../Store/StoreWrapperController_Tests.cs | 602 +----------------- UtilitiesCS.Test/UtilitiesCS.Test.csproj | 2 + .../code-review.2026-07-06T12-15.md | 26 + ...-01-format.remediation-2026-07-06T12-15.md | 6 + ...-analyzers.remediation-2026-07-06T12-15.md | 6 + ...3-nullable.remediation-2026-07-06T12-15.md | 6 + ...t-coverage.remediation-2026-07-06T12-15.md | 10 + ...rage-delta.remediation-2026-07-06T12-15.md | 13 + .../remediation-endstate.2026-07-06T12-15.md | 27 + .../split-containment-verification.md | 10 + .../qa-gates/split-linecount-verification.md | 11 + .../qa-gates/split-testmethod-verification.md | 8 + .../remediation-baseline/analyzer-baseline.md | 6 + .../csharpier-baseline.md | 6 + .../file-size-baseline.md | 6 + .../remediation-baseline/nullable-baseline.md | 6 + .../phase0-instructions-read.md | 21 + .../scope-confirmation.md | 22 + .../test-coverage-baseline.md | 10 + .../feature-audit.2026-07-06T12-15.md | 72 +++ .../policy-audit.2026-07-06T12-15.md | 166 +++++ .../remediation-inputs.2026-07-06T12-15.md | 50 ++ .../remediation-plan.2026-07-06T12-15.md | 83 +++ 27 files changed, 1219 insertions(+), 601 deletions(-) create mode 100644 .claude/agent-memory/feature-review/project_testresults-coverage-xml-cross-module-check.md create mode 100644 UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs create mode 100644 UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.remediation-2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.remediation-2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.remediation-2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.remediation-2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/remediation-endstate.2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-containment-verification.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-linecount-verification.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-testmethod-verification.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/analyzer-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/csharpier-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/file-size-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/nullable-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/phase0-instructions-read.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/scope-confirmation.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/test-coverage-baseline.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T12-15.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-plan.2026-07-06T12-15.md diff --git a/.claude/agent-memory/feature-review/MEMORY.md b/.claude/agent-memory/feature-review/MEMORY.md index 9df901d3e..853b92cdf 100644 --- a/.claude/agent-memory/feature-review/MEMORY.md +++ b/.claude/agent-memory/feature-review/MEMORY.md @@ -19,3 +19,4 @@ - [koverage analyzer finding misattributed](project_koverage-analyzer-finding-misattributed.md) — the pre-existing PSUseSingularNouns in Invoke-MSTestWithCoverage.Helpers.ps1 is on Get-CoberturaLineConditionCoverageParts, not Merge-CoberturaClassesByFilename as issue/evidence claim - [projectentry-setter-raw-messagebox-blocks-coverage](project_projectentry-setter-raw-messagebox-blocks-coverage.md) — #199 ProjectEntry change-confirmation is uncoverable via MyBox seam (commit runs the ProjectID setter's RAW MessageBox.Show); spec AC1 "fully covered by Phase 5" is overstated - [msbuild-invocation-via-bash](project_msbuild-invocation-via-bash.md) — msbuild/vstest not on bash PATH; `Platform="Any CPU"` needs a `/tmp` .cmd wrapper (`AnyCPU` and bash-quoted space both fail); a 127 exit is not a passing step +- [TestResults coverage XML cross-module check](project_testresults-coverage-xml-cross-module-check.md) — an uncommitted `TestResults/*.xml` from a single-project vstest run also instruments other loaded first-party modules; grep it to spot-check a "repository coverage" claim without rerunning coverage diff --git a/.claude/agent-memory/feature-review/project_testresults-coverage-xml-cross-module-check.md b/.claude/agent-memory/feature-review/project_testresults-coverage-xml-cross-module-check.md new file mode 100644 index 000000000..0bb846e59 --- /dev/null +++ b/.claude/agent-memory/feature-review/project_testresults-coverage-xml-cross-module-check.md @@ -0,0 +1,14 @@ +--- +name: testresults-coverage-xml-cross-module-check +description: TestResults/-coverage.xml from a single-project vstest run also instruments other loaded first-party modules, letting a reviewer spot-check whether a "repository line coverage" claim scoped to one assembly is misleading +metadata: + type: project +--- + +On issue #240 review (2026-07-06), the executor's `dotnet-coverage merge ... -o TestResults/final-coverage.xml` (run against only `UtilitiesCS.Test.dll`) turned out to also contain full module-level coverage entries for other first-party/vendored assemblies transitively loaded during that run: `TaskMaster.dll` (8.58% line), `Tags.dll` (0.00%), `ToDoModel.dll` (0.00%), `QuickFiler.dll` (0.00%), `SVGControl.dll` (15.15%), `Swordfish.NET.General.dll` (45.86%), alongside the intended `UtilitiesCS.dll` (85.88%). Grepping `module id="[A-F0-9]+" name="X.dll"` in that XML gives an instant per-module summary line without opening the 30MB file. This confirmed [[csharp-repowide-coverage-below-80]]'s finding independently, from a completely different coverage run. + +**Caveat:** the near-0% modules almost certainly reflect that their own dedicated `*.Test` projects were not executed in this run (only `UtilitiesCS.Test.dll` ran), not their true tested state — do not report those as certified per-module percentages, only as corroborating evidence that no single-project run yields a valid repo-wide figure. + +**Why:** this let me independently verify (without rerunning coverage generation, per the skill's "do not rerun" rule) that a feature's self-reported "repository line coverage" was actually single-assembly-scoped, using an artifact the executor had already produced but not highlighted. + +**How to apply:** when a C# feature's evidence reports a "repository"/"repo-wide" coverage percentage, check for an uncommitted `TestResults/*.xml` (dotnet-coverage native format) generated during that session before accepting the claim. Grep for `` lines to see every module the run actually instrumented, not just the one the evidence highlights. See [[csharp-coverage-artifact-is-cobertura]] and [[csharp-repowide-coverage-below-80]] for the canonical-artifact/format side of this gap. diff --git a/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs new file mode 100644 index 000000000..e067c5dbc --- /dev/null +++ b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs @@ -0,0 +1,396 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Windows.Forms; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using UtilitiesCS.OutlookObjects.Folder; +using UtilitiesCS.OutlookObjects.Store; + +namespace UtilitiesCS.Test.OutlookObjects.Store +{ + public partial class StoreWrapperController_Tests + { + #region ButtonOk_Click + + [TestMethod] + public void ButtonOk_Click_NoChanges_ClosesViewer() + { + var (controller, mockViewer) = CreateControllerWithViewer(); + + controller.ButtonOk_Click(); + + mockViewer.Verify(v => v.Close(), Times.Once); + } + + [TestMethod] + public void ButtonOk_Click_WithChanges_SavesAndCloses() + { + var (controller, mockViewer) = CreateControllerWithViewer(); + var mockModel = new Mock(); + controller.Model = mockModel.Object; + controller.Current = new StoreWrapper(null); + // Set ArchiveOutlook to a non-null value so AnyChanges() returns true + controller.ArchiveOutlook = new FolderMinimalWrapper("TestPath", "TestRelative"); + + controller.ButtonOk_Click(); + + mockViewer.Verify(v => v.Close(), Times.Once); + } + + #endregion + + #region AnyChanges variants + + [TestMethod] + public void AnyChanges_ArchiveOutlookDiffers_ReturnsTrue() + { + var controller = CreateController(); + controller.Current = new StoreWrapper(null); + controller.ArchiveOutlook = new FolderMinimalWrapper("Path", "Relative"); + + controller.AnyChanges().Should().BeTrue(); + } + + [TestMethod] + public void AnyChanges_JunkEmailDiffers_ReturnsTrue() + { + var controller = CreateController(); + controller.Current = new StoreWrapper(null); + controller.JunkEmail = new FolderMinimalWrapper("Path", "Relative"); + + controller.AnyChanges().Should().BeTrue(); + } + + [TestMethod] + public void AnyChanges_JunkPotentialDiffers_ReturnsTrue() + { + var controller = CreateController(); + controller.Current = new StoreWrapper(null); + controller.JunkPotential = new FolderMinimalWrapper("Path", "Relative"); + + controller.AnyChanges().Should().BeTrue(); + } + + #endregion + + #region GetRelativeFsPath variants + + [TestMethod] + public void GetRelativeFsPath_ArchiveFsWithEmptyPath_ReturnsPlaceholder() + { + var controller = CreateController(); + controller.Current = new StoreWrapper(null); + controller.Current.ArchiveFsRoot = new FilePathHelper(); + + var result = controller.GetRelativeFsPath(); + + result.Should().Be("Please select an archive"); + } + + [TestMethod] + public void GetRelativeFsPath_ArchiveFsWithPath_ConverterReturnsEmpty_ReturnsPlaceholder() + { + var controller = CreateController(); + controller.Current = new StoreWrapper(null); + controller.Current.ArchiveFsRoot = new FilePathHelper { FolderPath = @"C:\SomePath" }; + controller.FsConverter = (path) => ("", ""); + + var result = controller.GetRelativeFsPath(); + + result.Should().Be("Please select an archive"); + } + + [TestMethod] + public void GetRelativeFsPath_ArchiveFsWithPath_ConverterReturnsValues_ReturnsFormatted() + { + var controller = CreateController(); + controller.Current = new StoreWrapper(null); + controller.Current.ArchiveFsRoot = new FilePathHelper { FolderPath = @"C:\SomePath" }; + controller.FsConverter = (path) => ("AppData", "Backups"); + + var result = controller.GetRelativeFsPath(); + + result.Should().Contain("AppData"); + result.Should().Contain("Backups"); + } + + #endregion + + #region PopulateWithCurrent + + [TestMethod] + public void PopulateWithCurrent_NullCurrent_SetsErrorLoadingText() + { + var (controller, mockViewer) = CreateControllerWithViewer(); + controller.Current = null; + controller.FsConverter = (path) => ("", ""); + + // PopulateWithCurrent accesses Current which may be null + // This would throw NullReferenceException, verifying we need Current set + var act = () => controller.PopulateWithCurrent(); + + act.Should().Throw(); + } + + [TestMethod] + public void PopulateWithCurrent_CurrentSetWithNulls_SetsPlaceholders() + { + var (controller, mockViewer) = CreateControllerWithViewer(); + controller.Current = new StoreWrapper(null); + controller.FsConverter = (path) => ("", ""); + + controller.PopulateWithCurrent(); + + // StoreWrapper defaults ArchiveRoot to new FolderMinimalWrapper(), so it's not null + controller.ArchiveOutlook.Should().NotBeNull(); + // JunkCertain / JunkPotential default to new FolderMinimalWrapper() as well + controller.JunkEmail.Should().NotBeNull(); + controller.JunkPotential.Should().NotBeNull(); + } + + /// + /// Verifies that after completes, + /// the controller's internal folder fields are the exact same object references as the + /// corresponding properties on the backing . + /// + /// Purpose: + /// Confirm that PopulateWithCurrent "mirrors" the current store — i.e. the controller + /// fields are not copies but are the same instances, so subsequent AnyChanges() comparison + /// via PairwiseEquals (reference equality) will correctly report "no changes" right + /// after population. + /// + /// Returns: + /// Passes when each controller field is the same object reference as the Current property. + /// + [TestMethod] + public void PopulateWithCurrent_WithKnownFolderValues_MirrorsControllerFieldsFromCurrent() + { + // Arrange: use null globals — PopulateWithCurrent does not call Globals. + // Use StoreWrapperViewer directly to avoid Moq (Moq's AwaitableFactory requires + // System.Threading.Tasks.Extensions 4.2.0.1 which is absent from the test bin output, + // causing TypeInitializationException for all Mock involving Task-bearing interfaces). + // StoreWrapperViewer creates real WinForms labels in InitializeComponent(); Form handle + // is never created so InvokeRequired returns false in the test thread. + var controller = new StoreWrapperController(null!); + controller.Viewer = new StoreWrapperViewer(); + + var archiveFolder = new FolderMinimalWrapper("Archive", "Root\\Archive"); + var junkEmailFolder = new FolderMinimalWrapper("JunkEmail", "Root\\Junk"); + var junkPotentialFolder = new FolderMinimalWrapper("JunkPotential", "Root\\Potential"); + + // FilePathHelper() defaults FolderPath = "" so GetRelativeFsPath skips FsConverter. + var archiveFs = new FilePathHelper(); + + var currentStore = new StoreWrapper(null); + currentStore.ArchiveRoot = archiveFolder; + currentStore.JunkCertain = junkEmailFolder; + currentStore.JunkPotential = junkPotentialFolder; + currentStore.ArchiveFsRoot = archiveFs; + controller.Current = currentStore; + + // Act + controller.PopulateWithCurrent(); + + // Assert: controller fields must be the same object references — not copies. + // PairwiseEquals uses reference equality for FolderMinimalWrapper and FilePathHelper, + // so mirroring reference equality ensures AnyChanges() reports no changes right after + // population. + controller.ArchiveOutlook.Should().BeSameAs(archiveFolder); + controller.JunkEmail.Should().BeSameAs(junkEmailFolder); + controller.JunkPotential.Should().BeSameAs(junkPotentialFolder); + controller.ArchiveFS.Should().BeSameAs(archiveFs); + } + + #endregion + + #region Click handlers (non-invoke path) + + [TestMethod] + public void ArchiveOutlook_Click_NullSelectedFolder_LeavesNull() + { + var mockGlobals = new Mock(); + var mockOl = new Mock(); + var mockNs = new Mock(); + mockNs + .Setup(n => n.PickFolder()) + .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)null); + mockOl.Setup(o => o.NamespaceMAPI).Returns(mockNs.Object); + mockGlobals.Setup(g => g.Ol).Returns(mockOl.Object); + + var controller = new StoreWrapperController(mockGlobals.Object); + var mockViewer = new Mock(); + mockViewer.Setup(v => v.InvokeRequired).Returns(false); + mockViewer.Setup(v => v.ArchiveOutlook).Returns(new Label()); + controller.Viewer = mockViewer.Object; + + controller.ArchiveOutlook_Click(); + + controller.ArchiveOutlook.Should().BeNull(); + } + + [TestMethod] + public void JunkEmail_Click_NullSelectedFolder_LeavesNull() + { + var mockGlobals = new Mock(); + var mockOl = new Mock(); + var mockNs = new Mock(); + mockNs + .Setup(n => n.PickFolder()) + .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)null); + mockOl.Setup(o => o.NamespaceMAPI).Returns(mockNs.Object); + mockGlobals.Setup(g => g.Ol).Returns(mockOl.Object); + + var controller = new StoreWrapperController(mockGlobals.Object); + var mockViewer = new Mock(); + mockViewer.Setup(v => v.InvokeRequired).Returns(false); + mockViewer.Setup(v => v.JunkEmail).Returns(new Label()); + controller.Viewer = mockViewer.Object; + + controller.JunkEmail_Click(); + + controller.JunkEmail.Should().BeNull(); + } + + [TestMethod] + public void JunkPotential_Click_NullSelectedFolder_LeavesNull() + { + var mockGlobals = new Mock(); + var mockOl = new Mock(); + var mockNs = new Mock(); + mockNs + .Setup(n => n.PickFolder()) + .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)null); + mockOl.Setup(o => o.NamespaceMAPI).Returns(mockNs.Object); + mockGlobals.Setup(g => g.Ol).Returns(mockOl.Object); + + var controller = new StoreWrapperController(mockGlobals.Object); + var mockViewer = new Mock(); + mockViewer.Setup(v => v.InvokeRequired).Returns(false); + mockViewer.Setup(v => v.JunkPotential).Returns(new Label()); + controller.Viewer = mockViewer.Object; + + controller.JunkPotential_Click(); + + controller.JunkPotential.Should().BeNull(); + } + + [TestMethod] + public void ArchiveOutlook_Click_SelectFolderReturnsFolder_SetsArchiveOutlookToReturnedFolder() + { + // Arrange: inject a known folder via the stub subclass. + // Null globals: SelectFolder is overridden so Globals.Ol is never called. + // Use StoreWrapperViewer directly (no Moq) — avoids Moq AwaitableFactory failure. + var stubFolder = new FolderMinimalWrapper("Archive", "Root\\Archive"); + var controller = new StubSelectFolderController(null!, stubFolder); + controller.Viewer = new StoreWrapperViewer(); + + // Act: click handler calls SelectFolder() and stores the result. + controller.ArchiveOutlook_Click(); + + // Assert: the property was updated to exactly the stub folder returned by SelectFolder. + controller.ArchiveOutlook.Should().BeSameAs(stubFolder); + } + + [TestMethod] + public void DisplayName_SelectedValueChanged_WithPendingChangesAndYesResponse_SavesThenLoadsSelectedStore() + { + using var viewer = new StoreWrapperViewer(); + var controller = new StoreWrapperController(null!) { Viewer = viewer }; + var original = new StoreWrapper(null) { DisplayName = "Original" }; + var inbox = new Mock(); + var root = new Mock(); + inbox.SetupGet(x => x.FolderPath).Returns("Inbox"); + root.SetupGet(x => x.FolderPath).Returns("Root"); + var selected = new StoreWrapper(null) + { + DisplayName = "Selected", + Inbox = inbox.Object, + RootFolder = root.Object, + UserEmailAddress = "owner@example.com", + }; + var pendingArchive = new FolderMinimalWrapper("Archive", "Root\\Archive"); + controller.Model = new StoresWrapper + { + Stores = new List { original, selected }, + }; + controller.Current = original; + controller.ArchiveOutlook = pendingArchive; + viewer.DisplayName.DataSource = new List { "Original", "Selected" }; + viewer.DisplayName.SelectedIndex = 1; + var originalInvoker = MyBox.DialogInvoker; + + try + { + MyBox.DialogInvoker = _ => DialogResult.Yes; + controller.DisplayName_SelectedValueChanged(viewer.DisplayName, EventArgs.Empty); + } + finally + { + MyBox.DialogInvoker = originalInvoker; + } + + original.ArchiveRoot.Should().BeSameAs(pendingArchive); + controller.Current.Should().BeSameAs(selected); + viewer.Inbox.Text.Should().Be("Inbox"); + viewer.RootFolder.Text.Should().Be("Root"); + viewer.UserEmail.Text.Should().Be("owner@example.com"); + } + + [TestMethod] + public void ClickHandlers_WhenInvokeRequired_DelegateToViewerInvoke() + { + var controller = CreateController(); + var mockViewer = new Mock(); + mockViewer.Setup(v => v.InvokeRequired).Returns(true); + mockViewer.Setup(v => v.Invoke(It.IsAny())).Returns((object)null); + controller.Viewer = mockViewer.Object; + + controller.ArchiveFS_Click(); + controller.ArchiveOutlook_Click(); + controller.JunkEmail_Click(); + controller.JunkPotential_Click(); + + mockViewer.Verify(v => v.Invoke(It.IsAny()), Times.Exactly(4)); + } + + [TestMethod] + public void SelectFolder_WhenPickFolderReturnsFolder_WrapsRelativePathFromCurrentRoot() + { + var mockGlobals = new Mock(); + var mockOl = new Mock(); + var mockNs = new Mock(); + var root = new Mock(); + var picked = new Mock(); + root.SetupGet(x => x.FolderPath).Returns(@"\\Mailbox"); + picked.SetupGet(x => x.FolderPath).Returns(@"\\Mailbox\\Archive"); + mockNs + .Setup(n => n.PickFolder()) + .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)picked.Object); + mockOl.SetupGet(o => o.NamespaceMAPI).Returns(mockNs.Object); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object) + { + Current = new StoreWrapper(null) { RootFolder = root.Object }, + }; + + controller.SelectFolder().RelativePath.Should().Be("\\Archive"); + } + + [TestMethod] + public void SelectFolder_WhenPickFolderThrows_ReturnsNull() + { + var mockGlobals = new Mock(); + var mockOl = new Mock(); + var mockNs = new Mock(); + mockNs.Setup(n => n.PickFolder()).Throws(new InvalidOperationException("boom")); + mockOl.SetupGet(o => o.NamespaceMAPI).Returns(mockNs.Object); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + + new StoreWrapperController(mockGlobals.Object).SelectFolder().Should().BeNull(); + } + + #endregion + } +} diff --git a/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs new file mode 100644 index 000000000..c913cdf77 --- /dev/null +++ b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs @@ -0,0 +1,234 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Windows.Forms; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using UtilitiesCS.OutlookObjects.Folder; +using UtilitiesCS.OutlookObjects.Store; + +namespace UtilitiesCS.Test.OutlookObjects.Store +{ + public partial class StoreWrapperController_Tests + { + #region Launch (issue #240 regression) + + /// + /// Reproduces issue #240: when the Outlook store-wrapper model has not yet been + /// loaded (Globals.Ol.StoresWrapper is null), Launch() must not throw + /// an unhandled . It must show a user-facing + /// message via the dialog seam and leave Viewer null + /// rather than opening a broken dialog (AC1). + /// + [TestMethod] + public void Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + var originalInvoker = MyBox.DialogInvoker; + var invocationCount = 0; + + try + { + MyBox.DialogInvoker = _ => + { + invocationCount++; + return DialogResult.OK; + }; + + // Act + Action act = () => controller.Launch(); + + // Assert + act.Should().NotThrow(); + invocationCount.Should().Be(1); + controller.Viewer.Should().BeNull(); + } + finally + { + MyBox.DialogInvoker = originalInvoker; + } + } + + /// + /// Reproduces issue #240 for the secondary root cause: a non-null + /// StoresWrapper whose Stores list is transiently null (post-deserialize + /// state before the async rewire completes). Launch() must not throw and must + /// leave Viewer null instead of opening a broken dialog (AC2). + /// + [TestMethod] + public void Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null }); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + var originalInvoker = MyBox.DialogInvoker; + var invocationCount = 0; + + try + { + MyBox.DialogInvoker = _ => + { + invocationCount++; + return DialogResult.OK; + }; + + // Act + Action act = () => controller.Launch(); + + // Assert + act.Should().NotThrow(); + invocationCount.Should().Be(1); + controller.Viewer.Should().BeNull(); + } + finally + { + MyBox.DialogInvoker = originalInvoker; + } + } + + #endregion + + #region EvaluateLaunchReadiness (issue #240) + + /// + /// When is null, readiness cannot be + /// determined and the evaluation must report ModelUnavailable rather than + /// throwing. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable() + { + // Arrange + var controller = new StoreWrapperController(null!); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); + } + + /// + /// When Globals.Ol is null, readiness cannot be determined and the evaluation + /// must report ModelUnavailable rather than throwing. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable() + { + // Arrange + var mockGlobals = new Mock(); + mockGlobals.SetupGet(g => g.Ol).Returns((IOlObjects)null); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); + } + + /// + /// When Globals.Ol.StoresWrapper is null (store load has not completed), + /// the evaluation must report ModelUnavailable. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); + } + + /// + /// When the model is present but its Stores list is transiently null + /// (post-deserialize, before the async rewire populates it), the evaluation must + /// report StoresUnavailable. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable() + { + // Arrange + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null }); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.StoresUnavailable); + } + + /// + /// When the model and its Stores list are both populated, the evaluation must + /// report Ready with the model and the display names of every seeded store. + /// + [TestMethod] + public void EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames() + { + // Arrange + var storeA = new StoreWrapper(null) { DisplayName = "Mailbox A" }; + var storeB = new StoreWrapper(null) { DisplayName = "Mailbox B" }; + var model = new StoresWrapper + { + Stores = new List { storeA, storeB }, + }; + var mockGlobals = new Mock(); + var mockOl = new Mock(); + mockOl.SetupGet(o => o.StoresWrapper).Returns(model); + mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); + var controller = new StoreWrapperController(mockGlobals.Object); + + // Act + var readiness = controller.EvaluateLaunchReadiness(); + + // Assert + readiness.State.Should().Be(StoreLaunchReadinessState.Ready); + readiness.DisplayNames.Should().Equal("Mailbox A", "Mailbox B"); + } + + #endregion + + #region Stub helpers + + private sealed class StubSelectFolderController : StoreWrapperController + { + private readonly FolderMinimalWrapper _stub; + + internal StubSelectFolderController( + IApplicationGlobals globals, + FolderMinimalWrapper stubFolder + ) + : base(globals) + { + _stub = stubFolder; + } + + internal override FolderMinimalWrapper SelectFolder() => _stub; + } + + #endregion + } +} diff --git a/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs index 95e535cde..984331c7f 100644 --- a/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs +++ b/UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs @@ -11,7 +11,7 @@ namespace UtilitiesCS.Test.OutlookObjects.Store { [TestClass] - public class StoreWrapperController_Tests + public partial class StoreWrapperController_Tests { #region RunFolderSelectionDialog @@ -177,605 +177,5 @@ Mock viewer } #endregion - - #region ButtonOk_Click - - [TestMethod] - public void ButtonOk_Click_NoChanges_ClosesViewer() - { - var (controller, mockViewer) = CreateControllerWithViewer(); - - controller.ButtonOk_Click(); - - mockViewer.Verify(v => v.Close(), Times.Once); - } - - [TestMethod] - public void ButtonOk_Click_WithChanges_SavesAndCloses() - { - var (controller, mockViewer) = CreateControllerWithViewer(); - var mockModel = new Mock(); - controller.Model = mockModel.Object; - controller.Current = new StoreWrapper(null); - // Set ArchiveOutlook to a non-null value so AnyChanges() returns true - controller.ArchiveOutlook = new FolderMinimalWrapper("TestPath", "TestRelative"); - - controller.ButtonOk_Click(); - - mockViewer.Verify(v => v.Close(), Times.Once); - } - - #endregion - - #region AnyChanges variants - - [TestMethod] - public void AnyChanges_ArchiveOutlookDiffers_ReturnsTrue() - { - var controller = CreateController(); - controller.Current = new StoreWrapper(null); - controller.ArchiveOutlook = new FolderMinimalWrapper("Path", "Relative"); - - controller.AnyChanges().Should().BeTrue(); - } - - [TestMethod] - public void AnyChanges_JunkEmailDiffers_ReturnsTrue() - { - var controller = CreateController(); - controller.Current = new StoreWrapper(null); - controller.JunkEmail = new FolderMinimalWrapper("Path", "Relative"); - - controller.AnyChanges().Should().BeTrue(); - } - - [TestMethod] - public void AnyChanges_JunkPotentialDiffers_ReturnsTrue() - { - var controller = CreateController(); - controller.Current = new StoreWrapper(null); - controller.JunkPotential = new FolderMinimalWrapper("Path", "Relative"); - - controller.AnyChanges().Should().BeTrue(); - } - - #endregion - - #region GetRelativeFsPath variants - - [TestMethod] - public void GetRelativeFsPath_ArchiveFsWithEmptyPath_ReturnsPlaceholder() - { - var controller = CreateController(); - controller.Current = new StoreWrapper(null); - controller.Current.ArchiveFsRoot = new FilePathHelper(); - - var result = controller.GetRelativeFsPath(); - - result.Should().Be("Please select an archive"); - } - - [TestMethod] - public void GetRelativeFsPath_ArchiveFsWithPath_ConverterReturnsEmpty_ReturnsPlaceholder() - { - var controller = CreateController(); - controller.Current = new StoreWrapper(null); - controller.Current.ArchiveFsRoot = new FilePathHelper { FolderPath = @"C:\SomePath" }; - controller.FsConverter = (path) => ("", ""); - - var result = controller.GetRelativeFsPath(); - - result.Should().Be("Please select an archive"); - } - - [TestMethod] - public void GetRelativeFsPath_ArchiveFsWithPath_ConverterReturnsValues_ReturnsFormatted() - { - var controller = CreateController(); - controller.Current = new StoreWrapper(null); - controller.Current.ArchiveFsRoot = new FilePathHelper { FolderPath = @"C:\SomePath" }; - controller.FsConverter = (path) => ("AppData", "Backups"); - - var result = controller.GetRelativeFsPath(); - - result.Should().Contain("AppData"); - result.Should().Contain("Backups"); - } - - #endregion - - #region PopulateWithCurrent - - [TestMethod] - public void PopulateWithCurrent_NullCurrent_SetsErrorLoadingText() - { - var (controller, mockViewer) = CreateControllerWithViewer(); - controller.Current = null; - controller.FsConverter = (path) => ("", ""); - - // PopulateWithCurrent accesses Current which may be null - // This would throw NullReferenceException, verifying we need Current set - var act = () => controller.PopulateWithCurrent(); - - act.Should().Throw(); - } - - [TestMethod] - public void PopulateWithCurrent_CurrentSetWithNulls_SetsPlaceholders() - { - var (controller, mockViewer) = CreateControllerWithViewer(); - controller.Current = new StoreWrapper(null); - controller.FsConverter = (path) => ("", ""); - - controller.PopulateWithCurrent(); - - // StoreWrapper defaults ArchiveRoot to new FolderMinimalWrapper(), so it's not null - controller.ArchiveOutlook.Should().NotBeNull(); - // JunkCertain / JunkPotential default to new FolderMinimalWrapper() as well - controller.JunkEmail.Should().NotBeNull(); - controller.JunkPotential.Should().NotBeNull(); - } - - /// - /// Verifies that after completes, - /// the controller's internal folder fields are the exact same object references as the - /// corresponding properties on the backing . - /// - /// Purpose: - /// Confirm that PopulateWithCurrent "mirrors" the current store — i.e. the controller - /// fields are not copies but are the same instances, so subsequent AnyChanges() comparison - /// via PairwiseEquals (reference equality) will correctly report "no changes" right - /// after population. - /// - /// Returns: - /// Passes when each controller field is the same object reference as the Current property. - /// - [TestMethod] - public void PopulateWithCurrent_WithKnownFolderValues_MirrorsControllerFieldsFromCurrent() - { - // Arrange: use null globals — PopulateWithCurrent does not call Globals. - // Use StoreWrapperViewer directly to avoid Moq (Moq's AwaitableFactory requires - // System.Threading.Tasks.Extensions 4.2.0.1 which is absent from the test bin output, - // causing TypeInitializationException for all Mock involving Task-bearing interfaces). - // StoreWrapperViewer creates real WinForms labels in InitializeComponent(); Form handle - // is never created so InvokeRequired returns false in the test thread. - var controller = new StoreWrapperController(null!); - controller.Viewer = new StoreWrapperViewer(); - - var archiveFolder = new FolderMinimalWrapper("Archive", "Root\\Archive"); - var junkEmailFolder = new FolderMinimalWrapper("JunkEmail", "Root\\Junk"); - var junkPotentialFolder = new FolderMinimalWrapper("JunkPotential", "Root\\Potential"); - - // FilePathHelper() defaults FolderPath = "" so GetRelativeFsPath skips FsConverter. - var archiveFs = new FilePathHelper(); - - var currentStore = new StoreWrapper(null); - currentStore.ArchiveRoot = archiveFolder; - currentStore.JunkCertain = junkEmailFolder; - currentStore.JunkPotential = junkPotentialFolder; - currentStore.ArchiveFsRoot = archiveFs; - controller.Current = currentStore; - - // Act - controller.PopulateWithCurrent(); - - // Assert: controller fields must be the same object references — not copies. - // PairwiseEquals uses reference equality for FolderMinimalWrapper and FilePathHelper, - // so mirroring reference equality ensures AnyChanges() reports no changes right after - // population. - controller.ArchiveOutlook.Should().BeSameAs(archiveFolder); - controller.JunkEmail.Should().BeSameAs(junkEmailFolder); - controller.JunkPotential.Should().BeSameAs(junkPotentialFolder); - controller.ArchiveFS.Should().BeSameAs(archiveFs); - } - - #endregion - - #region Click handlers (non-invoke path) - - [TestMethod] - public void ArchiveOutlook_Click_NullSelectedFolder_LeavesNull() - { - var mockGlobals = new Mock(); - var mockOl = new Mock(); - var mockNs = new Mock(); - mockNs - .Setup(n => n.PickFolder()) - .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)null); - mockOl.Setup(o => o.NamespaceMAPI).Returns(mockNs.Object); - mockGlobals.Setup(g => g.Ol).Returns(mockOl.Object); - - var controller = new StoreWrapperController(mockGlobals.Object); - var mockViewer = new Mock(); - mockViewer.Setup(v => v.InvokeRequired).Returns(false); - mockViewer.Setup(v => v.ArchiveOutlook).Returns(new Label()); - controller.Viewer = mockViewer.Object; - - controller.ArchiveOutlook_Click(); - - controller.ArchiveOutlook.Should().BeNull(); - } - - [TestMethod] - public void JunkEmail_Click_NullSelectedFolder_LeavesNull() - { - var mockGlobals = new Mock(); - var mockOl = new Mock(); - var mockNs = new Mock(); - mockNs - .Setup(n => n.PickFolder()) - .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)null); - mockOl.Setup(o => o.NamespaceMAPI).Returns(mockNs.Object); - mockGlobals.Setup(g => g.Ol).Returns(mockOl.Object); - - var controller = new StoreWrapperController(mockGlobals.Object); - var mockViewer = new Mock(); - mockViewer.Setup(v => v.InvokeRequired).Returns(false); - mockViewer.Setup(v => v.JunkEmail).Returns(new Label()); - controller.Viewer = mockViewer.Object; - - controller.JunkEmail_Click(); - - controller.JunkEmail.Should().BeNull(); - } - - [TestMethod] - public void JunkPotential_Click_NullSelectedFolder_LeavesNull() - { - var mockGlobals = new Mock(); - var mockOl = new Mock(); - var mockNs = new Mock(); - mockNs - .Setup(n => n.PickFolder()) - .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)null); - mockOl.Setup(o => o.NamespaceMAPI).Returns(mockNs.Object); - mockGlobals.Setup(g => g.Ol).Returns(mockOl.Object); - - var controller = new StoreWrapperController(mockGlobals.Object); - var mockViewer = new Mock(); - mockViewer.Setup(v => v.InvokeRequired).Returns(false); - mockViewer.Setup(v => v.JunkPotential).Returns(new Label()); - controller.Viewer = mockViewer.Object; - - controller.JunkPotential_Click(); - - controller.JunkPotential.Should().BeNull(); - } - - [TestMethod] - public void ArchiveOutlook_Click_SelectFolderReturnsFolder_SetsArchiveOutlookToReturnedFolder() - { - // Arrange: inject a known folder via the stub subclass. - // Null globals: SelectFolder is overridden so Globals.Ol is never called. - // Use StoreWrapperViewer directly (no Moq) — avoids Moq AwaitableFactory failure. - var stubFolder = new FolderMinimalWrapper("Archive", "Root\\Archive"); - var controller = new StubSelectFolderController(null!, stubFolder); - controller.Viewer = new StoreWrapperViewer(); - - // Act: click handler calls SelectFolder() and stores the result. - controller.ArchiveOutlook_Click(); - - // Assert: the property was updated to exactly the stub folder returned by SelectFolder. - controller.ArchiveOutlook.Should().BeSameAs(stubFolder); - } - - [TestMethod] - public void DisplayName_SelectedValueChanged_WithPendingChangesAndYesResponse_SavesThenLoadsSelectedStore() - { - using var viewer = new StoreWrapperViewer(); - var controller = new StoreWrapperController(null!) { Viewer = viewer }; - var original = new StoreWrapper(null) { DisplayName = "Original" }; - var inbox = new Mock(); - var root = new Mock(); - inbox.SetupGet(x => x.FolderPath).Returns("Inbox"); - root.SetupGet(x => x.FolderPath).Returns("Root"); - var selected = new StoreWrapper(null) - { - DisplayName = "Selected", - Inbox = inbox.Object, - RootFolder = root.Object, - UserEmailAddress = "owner@example.com", - }; - var pendingArchive = new FolderMinimalWrapper("Archive", "Root\\Archive"); - controller.Model = new StoresWrapper - { - Stores = new List { original, selected }, - }; - controller.Current = original; - controller.ArchiveOutlook = pendingArchive; - viewer.DisplayName.DataSource = new List { "Original", "Selected" }; - viewer.DisplayName.SelectedIndex = 1; - var originalInvoker = MyBox.DialogInvoker; - - try - { - MyBox.DialogInvoker = _ => DialogResult.Yes; - controller.DisplayName_SelectedValueChanged(viewer.DisplayName, EventArgs.Empty); - } - finally - { - MyBox.DialogInvoker = originalInvoker; - } - - original.ArchiveRoot.Should().BeSameAs(pendingArchive); - controller.Current.Should().BeSameAs(selected); - viewer.Inbox.Text.Should().Be("Inbox"); - viewer.RootFolder.Text.Should().Be("Root"); - viewer.UserEmail.Text.Should().Be("owner@example.com"); - } - - [TestMethod] - public void ClickHandlers_WhenInvokeRequired_DelegateToViewerInvoke() - { - var controller = CreateController(); - var mockViewer = new Mock(); - mockViewer.Setup(v => v.InvokeRequired).Returns(true); - mockViewer.Setup(v => v.Invoke(It.IsAny())).Returns((object)null); - controller.Viewer = mockViewer.Object; - - controller.ArchiveFS_Click(); - controller.ArchiveOutlook_Click(); - controller.JunkEmail_Click(); - controller.JunkPotential_Click(); - - mockViewer.Verify(v => v.Invoke(It.IsAny()), Times.Exactly(4)); - } - - [TestMethod] - public void SelectFolder_WhenPickFolderReturnsFolder_WrapsRelativePathFromCurrentRoot() - { - var mockGlobals = new Mock(); - var mockOl = new Mock(); - var mockNs = new Mock(); - var root = new Mock(); - var picked = new Mock(); - root.SetupGet(x => x.FolderPath).Returns(@"\\Mailbox"); - picked.SetupGet(x => x.FolderPath).Returns(@"\\Mailbox\\Archive"); - mockNs - .Setup(n => n.PickFolder()) - .Returns((Microsoft.Office.Interop.Outlook.MAPIFolder)picked.Object); - mockOl.SetupGet(o => o.NamespaceMAPI).Returns(mockNs.Object); - mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); - var controller = new StoreWrapperController(mockGlobals.Object) - { - Current = new StoreWrapper(null) { RootFolder = root.Object }, - }; - - controller.SelectFolder().RelativePath.Should().Be("\\Archive"); - } - - [TestMethod] - public void SelectFolder_WhenPickFolderThrows_ReturnsNull() - { - var mockGlobals = new Mock(); - var mockOl = new Mock(); - var mockNs = new Mock(); - mockNs.Setup(n => n.PickFolder()).Throws(new InvalidOperationException("boom")); - mockOl.SetupGet(o => o.NamespaceMAPI).Returns(mockNs.Object); - mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); - - new StoreWrapperController(mockGlobals.Object).SelectFolder().Should().BeNull(); - } - - #endregion - - #region Launch (issue #240 regression) - - /// - /// Reproduces issue #240: when the Outlook store-wrapper model has not yet been - /// loaded (Globals.Ol.StoresWrapper is null), Launch() must not throw - /// an unhandled . It must show a user-facing - /// message via the dialog seam and leave Viewer null - /// rather than opening a broken dialog (AC1). - /// - [TestMethod] - public void Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer() - { - // Arrange - var mockGlobals = new Mock(); - var mockOl = new Mock(); - mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null); - mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); - var controller = new StoreWrapperController(mockGlobals.Object); - - var originalInvoker = MyBox.DialogInvoker; - var invocationCount = 0; - - try - { - MyBox.DialogInvoker = _ => - { - invocationCount++; - return DialogResult.OK; - }; - - // Act - Action act = () => controller.Launch(); - - // Assert - act.Should().NotThrow(); - invocationCount.Should().Be(1); - controller.Viewer.Should().BeNull(); - } - finally - { - MyBox.DialogInvoker = originalInvoker; - } - } - - /// - /// Reproduces issue #240 for the secondary root cause: a non-null - /// StoresWrapper whose Stores list is transiently null (post-deserialize - /// state before the async rewire completes). Launch() must not throw and must - /// leave Viewer null instead of opening a broken dialog (AC2). - /// - [TestMethod] - public void Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer() - { - // Arrange - var mockGlobals = new Mock(); - var mockOl = new Mock(); - mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null }); - mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); - var controller = new StoreWrapperController(mockGlobals.Object); - - var originalInvoker = MyBox.DialogInvoker; - var invocationCount = 0; - - try - { - MyBox.DialogInvoker = _ => - { - invocationCount++; - return DialogResult.OK; - }; - - // Act - Action act = () => controller.Launch(); - - // Assert - act.Should().NotThrow(); - invocationCount.Should().Be(1); - controller.Viewer.Should().BeNull(); - } - finally - { - MyBox.DialogInvoker = originalInvoker; - } - } - - #endregion - - #region EvaluateLaunchReadiness (issue #240) - - /// - /// When is null, readiness cannot be - /// determined and the evaluation must report ModelUnavailable rather than - /// throwing. - /// - [TestMethod] - public void EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable() - { - // Arrange - var controller = new StoreWrapperController(null!); - - // Act - var readiness = controller.EvaluateLaunchReadiness(); - - // Assert - readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); - } - - /// - /// When Globals.Ol is null, readiness cannot be determined and the evaluation - /// must report ModelUnavailable rather than throwing. - /// - [TestMethod] - public void EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable() - { - // Arrange - var mockGlobals = new Mock(); - mockGlobals.SetupGet(g => g.Ol).Returns((IOlObjects)null); - var controller = new StoreWrapperController(mockGlobals.Object); - - // Act - var readiness = controller.EvaluateLaunchReadiness(); - - // Assert - readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); - } - - /// - /// When Globals.Ol.StoresWrapper is null (store load has not completed), - /// the evaluation must report ModelUnavailable. - /// - [TestMethod] - public void EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable() - { - // Arrange - var mockGlobals = new Mock(); - var mockOl = new Mock(); - mockOl.SetupGet(o => o.StoresWrapper).Returns((StoresWrapper)null); - mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); - var controller = new StoreWrapperController(mockGlobals.Object); - - // Act - var readiness = controller.EvaluateLaunchReadiness(); - - // Assert - readiness.State.Should().Be(StoreLaunchReadinessState.ModelUnavailable); - } - - /// - /// When the model is present but its Stores list is transiently null - /// (post-deserialize, before the async rewire populates it), the evaluation must - /// report StoresUnavailable. - /// - [TestMethod] - public void EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable() - { - // Arrange - var mockGlobals = new Mock(); - var mockOl = new Mock(); - mockOl.SetupGet(o => o.StoresWrapper).Returns(new StoresWrapper { Stores = null }); - mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); - var controller = new StoreWrapperController(mockGlobals.Object); - - // Act - var readiness = controller.EvaluateLaunchReadiness(); - - // Assert - readiness.State.Should().Be(StoreLaunchReadinessState.StoresUnavailable); - } - - /// - /// When the model and its Stores list are both populated, the evaluation must - /// report Ready with the model and the display names of every seeded store. - /// - [TestMethod] - public void EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames() - { - // Arrange - var storeA = new StoreWrapper(null) { DisplayName = "Mailbox A" }; - var storeB = new StoreWrapper(null) { DisplayName = "Mailbox B" }; - var model = new StoresWrapper - { - Stores = new List { storeA, storeB }, - }; - var mockGlobals = new Mock(); - var mockOl = new Mock(); - mockOl.SetupGet(o => o.StoresWrapper).Returns(model); - mockGlobals.SetupGet(g => g.Ol).Returns(mockOl.Object); - var controller = new StoreWrapperController(mockGlobals.Object); - - // Act - var readiness = controller.EvaluateLaunchReadiness(); - - // Assert - readiness.State.Should().Be(StoreLaunchReadinessState.Ready); - readiness.DisplayNames.Should().Equal("Mailbox A", "Mailbox B"); - } - - #endregion - - #region Stub helpers - - private sealed class StubSelectFolderController : StoreWrapperController - { - private readonly FolderMinimalWrapper _stub; - - internal StubSelectFolderController( - IApplicationGlobals globals, - FolderMinimalWrapper stubFolder - ) - : base(globals) - { - _stub = stubFolder; - } - - internal override FolderMinimalWrapper SelectFolder() => _stub; - } - - #endregion } } diff --git a/UtilitiesCS.Test/UtilitiesCS.Test.csproj b/UtilitiesCS.Test/UtilitiesCS.Test.csproj index 38800f982..518a7af13 100644 --- a/UtilitiesCS.Test/UtilitiesCS.Test.csproj +++ b/UtilitiesCS.Test/UtilitiesCS.Test.csproj @@ -463,6 +463,8 @@ + + diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T12-15.md new file mode 100644 index 000000000..fcff01cc2 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T12-15.md @@ -0,0 +1,26 @@ +# Code Review — store-wrapper-launch-npe (Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Scope: full branch diff `4022fe7c9b07119224ca5aaa880b0a4003ef08db..dfbebb13fdc9ce2e9240376be2214dddf56ee5d0` +- Files reviewed directly: `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` + +## Executive Summary + +The fix cleanly separates a testable readiness decision (`EvaluateLaunchReadiness()`) from the untestable WinForms shell (`Launch()`), which is the correct structural response to the root cause described in `issue.md`. The new type (`StoreLaunchReadiness`) is a small, immutable, well-documented value object with private construction and named factories, consistent with the repository's design principles (separation of concerns, small focused methods, explicit contracts). The regression tests use the codebase's existing injectable `MyBox.DialogInvoker` seam rather than a raw `MessageBox.Show`/`Form.ShowDialog` call, which keeps the tests deterministic and Outlook-free. One quality/maintainability finding (test file size) and two minor defensive-coding observations are recorded below; none block correctness of the fix itself. + +## Findings Table + +| Severity | File | Location | Finding | Recommendation | Rationale | Evidence | +|---|---|---|---|---|---|---| +| Medium | `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` | Whole file (781 lines) | File exceeds the repository's 500-line limit for test code; the limit was already exceeded before this change (582 lines) and this PR adds 199 more lines to it. | Split the file along cohesive seams (e.g., extract the `Launch()` and `EvaluateLaunchReadiness()` regions into a new `StoreWrapperController_Launch_Tests.cs`, or otherwise partition by responsibility), each file <= 500 lines. | General Code Change Policy caps file size at 500 lines for production and test code alike, to keep files cohesive and reviewable; this file is now 56% over the limit. | `wc -l UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` = 781; `git diff --stat` shows `+199` for this file; baseline commit shows 582 lines | +| Low | `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` | `StoreLaunchReadiness.NotReady(...)` | The "not ready" sentinel carries `Model = null` and `DisplayNames = null` in a project without nullable annotations, so a future caller that reads `.Model`/`.DisplayNames` without checking `.State` first would get a silent `NullReferenceException` with no compiler warning. | Consider adding a one-line guard note in the XML doc for `Model`/`DisplayNames` stating they are only valid when `State == Ready` (partially present already on the type doc, but not on the properties themselves), or add a `Debug.Assert(State == Ready)` inside any future accessor helper. | Enforcing invariants at the type boundary reduces the chance that a later, unrelated change reintroduces the exact class of bug this issue fixes. | `StoreLaunchReadiness` struct definition in the diff (private constructor, `NotReady`/`Ready` factories) | +| Low | `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` | `StoreLaunchReadiness.NotReady(...)` (`#pragma warning disable CS8625` block) | The suppression is correctly scoped and documented, but the underlying condition (project has no `` context) is itself a piece of technical debt that will keep forcing this pattern for any future nullable-shaped code in this file. | No action required for this PR; note for a future, separately-scoped nullable-migration effort on `UtilitiesCS.csproj`. | C#7 requires suppressions to be as narrow as possible and documented in-code, which this satisfies; called out here only as a forward-looking observation, not a defect. | Diff hunk containing `#pragma warning disable CS8625` / `#pragma warning restore CS8625` with inline `why:` comment | +| Info | `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` | `EvaluateLaunchReadiness()` / `Launch()` | Positive finding: the extraction correctly keeps `Launch()`'s pre-existing `[ExcludeFromCodeCoverage]` attribute on the untestable WinForms shell while making the new decision logic a plain, non-excluded, directly testable method. This is the right pattern for this codebase's COM/VSTO/WinForms boundary constraints. | No action required. | Matches CLAUDE.md's General Unit Test Policy guidance to isolate I/O/UI boundaries from pure decision logic. | Diff hunk showing `[ExcludeFromCodeCoverage]` retained above `Launch()`, unchanged from baseline | +| Info | `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` | `Launch_When...` tests | Positive finding: tests intercept the dialog via the existing `MyBox.DialogInvoker` seam and restore it in `finally`, avoiding both a raw `MessageBox.Show` block and cross-test state leakage. | No action required. | Matches UT1 (independence/isolation) and UT4 (no external dependencies) requirements. | `UtilitiesCS/Dialogs/MyBox.cs` (`DialogInvoker` seam), test `try`/`finally` blocks in the diff | + +## Additional Observations + +- Naming is descriptive throughout (`StoreLaunchReadinessState`, `EvaluateLaunchReadiness`, `ModelUnavailable`, `StoresUnavailable`) and matches `PascalCase`/`camelCase` conventions. +- XML doc comments on the new enum, struct, and method explain *why* the readiness check exists (tying back to issue #240's two root causes), not just *what* the code does, consistent with the repository's naming/comment policy. +- No new public API surface was introduced; `EvaluateLaunchReadiness()` and the new types are `internal`, preserving `Launch()`'s existing public signature. +- No opportunistic refactors were observed outside the declared scope (`RibbonController.cs`, `AppOlObjects.cs` untouched, matching the plan's scope lock). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.remediation-2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.remediation-2026-07-06T12-15.md new file mode 100644 index 000000000..314830778 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.remediation-2026-07-06T12-15.md @@ -0,0 +1,6 @@ +# QA-01 — CSharpier Format (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `dotnet tool run csharpier format .` +- EXIT_CODE: 0 +- Output Summary: `Formatted 1271 files in 1027ms.` Zero files were reformatted on this pass — the three split files (`StoreWrapperController_Tests.cs`, `StoreWrapperController_Tests.ButtonAndPopulate.cs`, `StoreWrapperController_Tests.Launch.cs`) retained identical line counts (181 / 396 / 234) before and after this command, and `git status --porcelain` shows no additional diff beyond the pre-existing tracked changes to `StoreWrapperController_Tests.cs` and `UtilitiesCS.Test.csproj`. Final clean pass confirmed. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.remediation-2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.remediation-2026-07-06T12-15.md new file mode 100644 index 000000000..b47ba7ec1 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.remediation-2026-07-06T12-15.md @@ -0,0 +1,6 @@ +# QA-02 — Analyzer Build (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` +- EXIT_CODE: 0 +- Output Summary: Full solution build succeeded. 20 pre-existing warnings surfaced (CS8632 nullable-annotation-context warnings and CS0067 unused-event warnings), all located in files unrelated to this cycle (`ManualFireTimerWrapper.cs`, `OlTableExtensions_Tests.cs`, `ProgressTracker_Tests.cs`, `ConversationHelper_ExtendedTests.cs`, `SmartSerializable_Tests.cs`, `SmartSerializableBase_Tests.cs`, and the pre-existing, differently-named `StoreWrapperControllerTests.cs` — note: no underscore, a distinct file predating issue #240). `grep -iE "StoreWrapperController_Tests"` against the build log returned zero matches, confirming zero new analyzer warnings or errors were introduced by the three split files (`StoreWrapperController_Tests.cs`, `StoreWrapperController_Tests.ButtonAndPopulate.cs`, `StoreWrapperController_Tests.Launch.cs`) versus the P0-T5 baseline. (The 20 warnings appearing here vs. 0 in the P0-T5 incremental-build baseline reflect a from-scratch-rebuild vs. incremental-build visibility difference for pre-existing diagnostics in unrelated files, not a regression attributable to this cycle's changes.) diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.remediation-2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.remediation-2026-07-06T12-15.md new file mode 100644 index 000000000..02ccb5404 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.remediation-2026-07-06T12-15.md @@ -0,0 +1,6 @@ +# QA-03 — Nullable/Type-Check Build (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `msbuild TaskMaster.sln /t:Rebuild /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` +- EXIT_CODE: 1 +- Output Summary: 84 nullable errors, identical count and identical project scope to the P0-T6 baseline: `grep -oE "\[.*\.csproj\]"` deduplicated to exactly `SVGControl.csproj` and `UtilitiesSwordfish\UtilitiesSwordfish.NET.General.csproj`, the same two out-of-scope vendored/legacy projects documented in P0-T6. `grep -iE "StoreWrapperController_Tests"` against the build log returned zero matches, confirming zero new nullable diagnostics in the three touched/new files (`StoreWrapperController_Tests.cs`, `StoreWrapperController_Tests.ButtonAndPopulate.cs`, `StoreWrapperController_Tests.Launch.cs`). The pre-existing failures remain confined to the same out-of-scope vendored projects and are unaffected by this cycle's changes. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md new file mode 100644 index 000000000..671d9969c --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md @@ -0,0 +1,10 @@ +# QA-04 — Test Suite with Coverage (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation` +- EXIT_CODE: 0 +- Output Summary: Total tests: 4170. Passed: 4170. Failed: 0. Total time: 42.67s. All 4170 tests passed, including all 39 `[TestMethod]`s belonging to `StoreWrapperController_Tests` (verified individually against the P1-T6 post-split method-name list — zero missing). The single pre-existing flaky failure noted in the P0-T7 baseline (`PrintTree_WritesIndentedTreeToConsole`, unrelated to this cycle) did not recur on this run. + +Coverage extraction command: `dotnet-coverage merge .coverage -f xml -o TestResults/remediation-postchange-coverage.xml` + +Post-change `UtilitiesCS.dll` module coverage: line_coverage = **85.88%** (lines_covered=36897, lines_partially_covered=985, lines_not_covered=5082), block_coverage = 86.69% — identical to the P0-T7 baseline (85.88%), confirming no coverage regression. Total/passed counts equal or exceed the P0-T7 baseline (4170/4169 baseline vs. 4170/4170 here); no test was dropped or newly failing. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.remediation-2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.remediation-2026-07-06T12-15.md new file mode 100644 index 000000000..4ea95bf74 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.remediation-2026-07-06T12-15.md @@ -0,0 +1,13 @@ +# QA-05 — Coverage No-Regression Verification (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 + +## Coverage Delta + +- P0-T7 baseline `UtilitiesCS.dll` line coverage: **85.88%** +- P2-T4 post-change `UtilitiesCS.dll` line coverage: **85.88%** +- Delta: 0.00 percentage points (no change) + +## Verdict + +The post-change coverage value (85.88%) is >= the baseline value (85.88%) — no regression. It is also >= the 80% testable-denominator floor required by the General Unit Test Policy's COM/VSTO/WinForms exemption clause (CLAUDE.md UT2). This result is expected: this remediation cycle only relocated test code across files within the same test project and made zero changes to `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` or any other production file, so production-code coverage is unaffected by construction. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/remediation-endstate.2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/remediation-endstate.2026-07-06T12-15.md new file mode 100644 index 000000000..be11979ba --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/remediation-endstate.2026-07-06T12-15.md @@ -0,0 +1,27 @@ +# Remediation Cycle End-State Summary (Issue #240) + +- Timestamp: 2026-07-06T12-15 + +## Split Correctness + +- All three resulting files are <= 500 lines (see `evidence/qa-gates/split-linecount-verification.md`): + - `StoreWrapperController_Tests.cs`: 181 lines + - `StoreWrapperController_Tests.ButtonAndPopulate.cs`: 396 lines + - `StoreWrapperController_Tests.Launch.cs`: 234 lines +- All 39 `[TestMethod]`s are preserved with zero added/dropped (see `evidence/qa-gates/split-testmethod-verification.md`); the pre-split and post-split method-name sets are byte-identical. +- Containment held: zero diff to `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (see `evidence/qa-gates/split-containment-verification.md`). The tracked diff is limited to `StoreWrapperController_Tests.cs` (trim) and `UtilitiesCS.Test.csproj` (two new `` entries), plus two new untracked files (`StoreWrapperController_Tests.ButtonAndPopulate.cs`, `StoreWrapperController_Tests.Launch.cs`). + +## Final Toolchain Pass (P2-T1..P2-T4) + +Executed in order, in a single continuous pass, with no `SKIPPED` outcomes: + +1. **Format** (`dotnet tool run csharpier format .`) — EXIT_CODE 0; zero files reformatted. +2. **Analyzers** (`msbuild ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true`) — EXIT_CODE 0; zero new warnings/errors in the three split files (20 pre-existing warnings in unrelated files). +3. **Nullable/type-check** (`msbuild ... /t:Rebuild /p:Nullable=enable /p:TreatWarningsAsErrors=true`) — EXIT_CODE 1; 84 pre-existing errors confined to the same two out-of-scope vendored projects (`SVGControl.csproj`, `UtilitiesSwordfish.NET.General.csproj`) as the P0-T6 baseline; zero new diagnostics in the three split files. +4. **Test + coverage** (`vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation`) — EXIT_CODE 0; 4170/4170 passed; `UtilitiesCS.dll` line coverage 85.88%, unchanged from the P0-T7 baseline (see `evidence/qa-gates/qa-05-coverage-delta.remediation-2026-07-06T12-15.md`). + +No step required a restart of the loop: formatting made zero changes, and the only non-zero exit code (nullable/type-check) reproduced the same pre-existing, out-of-scope vendor-project error set already documented in the P0 baseline, with zero new diagnostics attributable to this cycle's changes. + +## Outcome + +Finding 1 (Blocking) is resolved: the file-size policy violation is eliminated, all 39 tests are preserved and passing, containment held (zero production-file diff), and the full C# toolchain passed in a single final clean pass. Findings 2, 3, and 4 remain untouched and out of scope for this cycle. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-containment-verification.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-containment-verification.md new file mode 100644 index 000000000..e391f5914 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-containment-verification.md @@ -0,0 +1,10 @@ +# Split Containment Verification (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `git diff --stat HEAD` +- EXIT_CODE: 0 +- Output Summary: Tracked-file diff touches exactly two tracked files: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (602 deletions, 3 insertions — the trim to the retained 8 regions plus the `partial` keyword) and `UtilitiesCS.Test/UtilitiesCS.Test.csproj` (2 insertions — the two new `` entries). `git status --porcelain` additionally shows two new untracked test files created by this cycle: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs` and `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs`. + +`git diff --stat HEAD -- "UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs"` produced no output (zero diff), confirming the containment invariant: the production file was not touched by this remediation cycle. + +Other untracked entries reported by `git status --porcelain` (`docs/features/active/2026-07-06-store-wrapper-launch-npe-240/*.md`, `.claude/agent-memory/feature-review/*`) pre-date this remediation cycle (policy-audit, code-review, feature-audit, remediation-inputs, and remediation-plan artifacts from the upstream review/planning cycle) and were not created or modified by this cycle's implementation tasks (P1-T1 through P1-T4). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-linecount-verification.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-linecount-verification.md new file mode 100644 index 000000000..87e8a8dba --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-linecount-verification.md @@ -0,0 +1,11 @@ +# Split Line-Count Verification (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command (PowerShell): `Get-ChildItem UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests*.cs | ForEach-Object { "$($_.Name): $((Get-Content $_.FullName | Measure-Object -Line).Lines)" }` +- EXIT_CODE: 0 +- Output Summary: + - `StoreWrapperController_Tests.cs`: 181 lines + - `StoreWrapperController_Tests.ButtonAndPopulate.cs`: 396 lines + - `StoreWrapperController_Tests.Launch.cs`: 234 lines + +All three resulting files are <= 500 lines, resolving Finding 1's file-size violation. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-testmethod-verification.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-testmethod-verification.md new file mode 100644 index 000000000..84ffe60c8 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-testmethod-verification.md @@ -0,0 +1,8 @@ +# Split TestMethod Verification (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `Select-String -Path UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests*.cs -Pattern "\[TestMethod\]" | Measure-Object | Select-Object -ExpandProperty Count` +- EXIT_CODE: 0 +- Output Summary: Combined `[TestMethod]` count across the three post-split files: 13 (`StoreWrapperController_Tests.cs`) + 19 (`StoreWrapperController_Tests.ButtonAndPopulate.cs`) + 7 (`StoreWrapperController_Tests.Launch.cs`) = **39**, matching the pre-split baseline count of 39 recorded via `git show HEAD:UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` and the same command against the single pre-split file, and matching the count stated in this plan's "Proposed file split" section. + +The sorted list of extracted method names from the pre-split file (via `git show HEAD:...`) and the sorted list of extracted method names across the three post-split files are byte-identical (`diff` produced no output), confirming no test method was dropped, added, or renamed. The specific method-name lists match those enumerated in P1-T1 (19 names), P1-T2 (7 names plus the `StubSelectFolderController` nested class), and P1-T3 (13 names). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/analyzer-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/analyzer-baseline.md new file mode 100644 index 000000000..f96a1fbaa --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/analyzer-baseline.md @@ -0,0 +1,6 @@ +# Analyzer Baseline (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` +- EXIT_CODE: 0 +- Output Summary: Full solution build succeeded with 0 warnings and 0 errors reported (`grep -iE "warning|error"` against the build log returned no matches). All first-party and vendored projects built successfully, including `UtilitiesCS.Test`. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/csharpier-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/csharpier-baseline.md new file mode 100644 index 000000000..e4c88ef46 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/csharpier-baseline.md @@ -0,0 +1,6 @@ +# CSharpier Baseline (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `dotnet tool run csharpier check UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` +- EXIT_CODE: 0 +- Output Summary: `Checked 1 files in 360ms.` No formatting violations found in the in-scope file prior to the split. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/file-size-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/file-size-baseline.md new file mode 100644 index 000000000..2eca81b56 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/file-size-baseline.md @@ -0,0 +1,6 @@ +# File-Size Baseline (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `(Get-Content "UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs" | Measure-Object -Line).Lines` +- EXIT_CODE: 0 +- Output Summary: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` is 781 lines, exceeding the repository's 500-line file-size limit, confirming Finding 1's violation. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/nullable-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/nullable-baseline.md new file mode 100644 index 000000000..ba5c239af --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/nullable-baseline.md @@ -0,0 +1,6 @@ +# Nullable/Type-Check Baseline (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `msbuild TaskMaster.sln /t:Rebuild /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` +- EXIT_CODE: 1 +- Output Summary: 84 pre-existing nullable errors, confined entirely to `SVGControl.csproj` and `UtilitiesSwordfish\UtilitiesSwordfish.NET.General.csproj` (verified via `grep -oE "\[.*\.csproj\]"` deduplicated to exactly these two vendored/legacy project paths). No errors in `UtilitiesCS.csproj`, `UtilitiesCS.Test.csproj`, or any other first-party project. These pre-existing failures are out of scope for this remediation cycle (Finding 1 only touches `UtilitiesCS.Test`) and are unaffected by the split. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/phase0-instructions-read.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/phase0-instructions-read.md new file mode 100644 index 000000000..38d24c906 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/phase0-instructions-read.md @@ -0,0 +1,21 @@ +# Phase 0 — Instructions Read (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 + +## Policy Order + +1. `CLAUDE.md` +2. `.claude/rules/general-code-change.md` +3. `.claude/rules/general-unit-test.md` +4. `.claude/rules/csharp.md` +5. `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T12-15.md` + +## Files Read (in order) + +1. `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\CLAUDE.md` +2. `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\general-code-change.md` +3. `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\general-unit-test.md` +4. `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\.claude\rules\csharp.md` +5. `C:\Users\DanMoisan\repos\TaskMaster-wt-2026-07-06-06-35\docs\features\active\2026-07-06-store-wrapper-launch-npe-240\remediation-inputs.2026-07-06T12-15.md` + +All five files were read in full before beginning implementation work for this remediation cycle. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/scope-confirmation.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/scope-confirmation.md new file mode 100644 index 000000000..1355b9820 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/scope-confirmation.md @@ -0,0 +1,22 @@ +# Scope Confirmation (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 + +## Scope Statement + +Only **Finding 1 (Blocking)** from `remediation-inputs.2026-07-06T12-15.md` is in scope for this remediation cycle: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` is 781 lines, exceeding the repository's 500-line file-size limit (General Code Change Policy §4 / `.claude/rules/general-code-change.md`). + +Findings 2, 3, and 4 are explicitly **excluded** from this cycle and MUST NOT be touched: + +- Finding 2 (repo-wide C# coverage artifact absent) — tracked separately under `feature/csharp-coverage-uplift`. +- Finding 3 (PR-context summary misclassification) — informational, owned by PR-context tooling. +- Finding 4 (AC5 check-off vs. review verdict) — documentation reconciliation, owned by the maintainer. + +## Containment Boundary + +`UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (production file) must remain untouched for the entirety of this cycle. Only the following files are in scope for edits: + +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (trim) +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs` (new) +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs` (new) +- `UtilitiesCS.Test/UtilitiesCS.Test.csproj` (add two `` entries) diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/test-coverage-baseline.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/test-coverage-baseline.md new file mode 100644 index 000000000..8e0c33834 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/test-coverage-baseline.md @@ -0,0 +1,10 @@ +# Test + Coverage Baseline (Remediation Cycle, Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation` +- EXIT_CODE: 1 +- Output Summary: Total tests: 4170. Passed: 4169. Failed: 1. Total time: 42.21s. The single failure, `PrintTree_WritesIndentedTreeToConsole`, is a pre-existing, unrelated failure outside the `StoreWrapperController_Tests` class and outside the scope of this remediation cycle (Finding 1 only). All 39 `[TestMethod]`s in `StoreWrapperController_Tests.cs` passed, confirmed individually via log grep (`RunFolderSelectionDialog_*`, `PairwiseEquals_*`, `ButtonOk_Click_*`, `Launch_When*`, `EvaluateLaunchReadiness_*`, etc.). + +Coverage extraction command: `dotnet-coverage merge .coverage -f xml -o TestResults/remediation-baseline-coverage.xml` + +Converted `.coverage` module report for `UtilitiesCS.dll` (the production assembly containing `StoreWrapperController`): line_coverage = **85.88%** (lines_covered=36896, lines_partially_covered=984, lines_not_covered=5084), block_coverage = 86.69%. This is the testable-denominator repository line-coverage baseline for this remediation cycle, matching the prior feature-cycle baseline recorded in `evidence/baseline/test-coverage-baseline.md` (85.87%) within measurement noise from incidental line additions unrelated to this cycle. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T12-15.md new file mode 100644 index 000000000..589cf1dc9 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T12-15.md @@ -0,0 +1,72 @@ +# Feature Audit — store-wrapper-launch-npe (Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Work mode: `minor-audit` +- AC source (per work-mode routing): `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md`, explicit `## Acceptance Criteria` section only + +## Scope and Baseline + +- Base branch (resolved): `main` @ `4022fe7c9b07119224ca5aaa880b0a4003ef08db` +- Head: `TaskMaster-wt-2026-07-06-06-35` @ `dfbebb13fdc9ce2e9240376be2214dddf56ee5d0` +- Full branch diff audited (per Scope Invariant): 23 files changed, 929 insertions(+), 4 deletions(-) — 2 `.cs` files (production + test), 21 `.md` files (issue, plan, research, evidence). +- Production change confined to `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (396 lines after change, verified via direct line count). Test change confined to `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (781 lines after change). No changes to `TaskMaster/Ribbon/RibbonController.cs` or `TaskMaster/AppGlobals/AppOlObjects.cs` (verified via `git diff --name-only`), matching the plan's declared small-path scope lock. + +## Acceptance Criteria Inventory + +| AC | Text | +|---|---| +| AC1 | `StoreWrapperController.Launch()` does not throw an unhandled `NullReferenceException` when `Globals.Ol.StoresWrapper` (`Model`) is null. It fails gracefully with a clear user-facing message and returns without opening a broken dialog. | +| AC2 | `Launch()` also handles a non-null `Model` whose `Stores` list is null (transient post-deserialize state) without throwing. | +| AC3 | A deterministic MSTest regression test reproduces the pre-fix crash path (fails before the fix, passes after) using Moq for `IApplicationGlobals`/`IOlObjects`; no live Outlook, no temporary files. | +| AC4 | The underlying readiness/initialization gap identified by root-cause research is addressed so that invoking the store-settings command when store state is unavailable produces deterministic, non-crashing behavior rather than an unhandled exception. | +| AC5 | The full C# toolchain passes in order (csharpier -> .NET analyzers -> nullable/TreatWarningsAsErrors -> MSTest with coverage); coverage on changed lines meets the >= 90% new-code target and repository line coverage remains >= 80% for the testable denominator. | +| AC6 | All required PR CI checks are green against the PR head SHA. | + +## Acceptance Criteria Evaluation + +### AC1 — PASS + +`Launch()` now calls `EvaluateLaunchReadiness()` first; when `Globals?.Ol?.StoresWrapper` is null, the state is `ModelUnavailable`, and `Launch()` shows a `MyBox.ShowDialog(...)` message and returns without constructing `Viewer`. Verified directly in the diff and confirmed by the regression test `Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer`: fails pre-fix with an unhandled `NullReferenceException` (`evidence/regression-testing/fail-before-240.md`), passes post-fix asserting no throw, one dialog invocation, and `Viewer == null` (`evidence/regression-testing/pass-after-240.md`). + +### AC2 — PASS + +The same `EvaluateLaunchReadiness()` path returns `StoresUnavailable` when `model.Stores` is null, taking the identical graceful-return branch. Verified by `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer`, which fails pre-fix with an unhandled `ArgumentNullException` (documented and explained precisely in `evidence/regression-testing/fail-before-240.md` as a more accurate characterization than "NullReferenceException" for this specific branch, while still satisfying AC2's "does not throw" requirement) and passes post-fix. + +### AC3 — PASS + +Both regression tests use MSTest (`[TestMethod]`), Moq (`Mock`, `Mock`), and FluentAssertions (`Should().NotThrow()`, etc.), confirmed by direct code inspection. No live Outlook process is started (all Outlook-facing interfaces are mocked) and no temporary files are created or referenced anywhere in the new test code. Fail-before (`evidence/regression-testing/fail-before-240.md`, `EXIT_CODE 1`, 2 failed/0 passed) and pass-after (`evidence/regression-testing/pass-after-240.md`, `EXIT_CODE 0`, 4170 passed/0 failed) evidence is present and internally consistent with the diff. + +### AC4 — PASS + +`EvaluateLaunchReadiness()` is a deterministic, non-`[ExcludeFromCodeCoverage]` decision method that enumerates every readiness state identified in the issue's root-cause analysis (null `Globals`, null `Ol`, null `StoresWrapper`, null `Stores` list, and the ready state), replacing the previous unguarded dereference chain. This directly addresses the "underlying readiness/initialization gap" called out in `issue.md`'s Suspected Cause section. Verified via direct diff review and the 5 `EvaluateLaunchReadiness_*` unit tests, all passing. + +### AC5 — PARTIAL + +- Toolchain order (csharpier -> analyzers -> nullable -> MSTest/coverage) was followed. Csharpier and analyzer gates pass cleanly (`EXIT_CODE 0`). The nullable gate's solution-wide `EXIT_CODE 1` is a pre-existing, documented, unrelated condition in vendored projects (`SVGControl.csproj`, `Swordfish.NET.General.csproj`); a scoped rebuild confirms the two touched files introduce zero new nullable diagnostics. This portion is accepted as satisfied, consistent with AC5's own parenthetical caveat. +- New-code coverage on changed lines: **verified PASS**. `EvaluateLaunchReadiness()` and the two `StoreLaunchReadiness` factory methods show 100.00% line/block coverage in the underlying coverage data (`TestResults/final-coverage.xml`, function id 295736 and related entries), exceeding the >= 90% target. +- "Repository line coverage remains >= 80% for the testable denominator": **not substantiated at the scope the phrase implies**. The cited 85.88% figure (`evidence/qa-gates/qa-04-test-coverage.md`) is scoped to the single `UtilitiesCS.dll` module, not the full C# solution. No canonical repo-wide coverage artifact (`artifacts/csharp/coverage.xml`) exists. Inspection of the same underlying coverage run shows other first-party modules loaded during the test run (`TaskMaster.dll` 8.58%, `Tags.dll`/`ToDoModel.dll`/`QuickFiler.dll` 0.00%) — though these low figures likely reflect that those modules' own dedicated test projects were not exercised in this run, not a certified measurement of their true coverage. Net effect: AC5's coverage clause, as literally worded ("repository line coverage"), is not verifiable as PASS from available evidence, so this criterion is evaluated PARTIAL rather than PASS. See `policy-audit.2026-07-06T12-15.md` §1.2.2 and §5 for full detail. + +### AC6 — UNVERIFIED (deferred, as declared) + +`evidence/other/ac6-deferral.md` explicitly and correctly defers AC6 to post-PR-creation CI, since no PR/CI run exists during local execution. This review has no GitHub CLI access and cannot independently verify CI status; AC6 remains correctly unchecked in `issue.md`. + +## Acceptance Criteria Check-off + +- AC1: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC2: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC3: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC4: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC5: `[x]` in `issue.md` — **discrepancy noted**. This review's verdict is PARTIAL (see above), specifically because the "repository line coverage >= 80%" clause is not substantiated at true repo-wide scope. Per the AC check-off protocol, reviewers add check-offs for verified PASS items; they do not retroactively remove an executor's existing check-off. This checkbox is therefore left as `[x]` but the gap is flagged here and in `remediation-inputs.2026-07-06T12-15.md` for maintainer reconciliation (either narrow AC5's wording to the assembly actually measured, or obtain a genuine repo-wide coverage artifact). +- AC6: `[ ]` in `issue.md` — consistent with this review's UNVERIFIED/deferred status. No change made. + +### Acceptance Criteria Status + +- Source: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md` (`## Acceptance Criteria`) +- Total AC items: 6 +- Checked off (delivered): 5 (AC1-AC5, pre-existing from executor; AC5 carries a flagged discrepancy — see above) +- Remaining (unchecked): 1 +- Items remaining: AC6 ("All required PR CI checks are green against the PR head SHA.") — correctly deferred pending PR/CI evidence. + +## Summary + +Four of six acceptance criteria (AC1-AC4) are fully verified as PASS against direct code and test evidence. AC5 is PARTIAL: the toolchain-order and new-code-coverage clauses are verified PASS, but the "repository line coverage >= 80%" clause is not substantiated at genuine repo-wide scope — the cited figure is single-assembly. AC6 is correctly deferred and unverifiable without CI access. No acceptance-criteria check-off changes were made by this review; the AC5 discrepancy is documented for maintainer follow-up rather than unilaterally resolved. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T12-15.md new file mode 100644 index 000000000..1550c9d3b --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T12-15.md @@ -0,0 +1,166 @@ +# Policy Audit — store-wrapper-launch-npe (Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Feature folder: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/` +- Work mode: `minor-audit` +- Base branch (resolved): `main` @ `4022fe7c9b07119224ca5aaa880b0a4003ef08db` +- Head: `TaskMaster-wt-2026-07-06-06-35` @ `dfbebb13fdc9ce2e9240376be2214dddf56ee5d0` +- Range audited: `4022fe7c9b07119224ca5aaa880b0a4003ef08db..dfbebb13fdc9ce2e9240376be2214dddf56ee5d0` (full branch diff, per Scope Invariant) + +## Rejected Scope Narrowing + +No caller instruction attempted to narrow this audit to a plan/task/phase subset, mark a language as out of scope, or skip a toolchain/coverage check. The orchestrator-supplied inputs (resolved base branch, merge-base SHA, active feature folder, AC source per work mode) are legitimate scope sources and were used as provided. No entries to record. + +## Evidence Location Compliance + +`git diff --name-only` over the audited range was scanned for paths under `artifacts/baselines/`, `artifacts/qa/`, `artifacts/evidence/`, and `artifacts/coverage/`. No matches were found. All 21 evidence/documentation files added by this branch live under the canonical `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence//` tree (`baseline/`, `regression-testing/`, `qa-gates/`, `issue-updates/`, `other/`). `scripts/dev_tools/validate_evidence_locations.py` referenced by the review contract does not exist in this repository; the scan above was performed manually via `git diff --name-only` and directory inspection instead. No `EVIDENCE_LOCATION_OVERRIDE_REJECTED` entries are required — no non-canonical path was supplied or used. + +## Executive Summary + +Issue #240 fixes an unhandled `NullReferenceException` in `StoreWrapperController.Launch()` by extracting a testable `EvaluateLaunchReadiness()` decision method and gating the pre-existing `[ExcludeFromCodeCoverage]` WinForms shell on its result. The change is scoped exactly as planned (one production file, one test file), is backed by a fail-before/pass-after MSTest regression pair using Moq and FluentAssertions, and the new decision logic is independently verified at 100% line/block coverage. Toolchain execution is documented transparently, including two disclosed deviations: (1) the solution-wide nullable gate's `EXIT_CODE 1` is a pre-existing, unrelated vendored-project condition, and (2) the test file `StoreWrapperController_Tests.cs` was already over the repository's 500-line limit before this change and grew further (582 -> 781 lines). Independent inspection of the coverage run's underlying data also shows that the "repository line coverage" figure cited in the feature's own evidence (85.88%) is scoped to a single assembly (`UtilitiesCS.dll`), not the full C# solution; no canonical repo-wide coverage artifact exists for this review session. Both the file-size overage and the repo-wide coverage scope gap are recorded below and carried into remediation inputs. + +## PR-Context Artifact Reliability Note + +`artifacts/pr_context.summary.txt` reports "Core logic changes: 0 files" and files both changed `.cs` files into "Docs/templates/agents/tooling: 21 files" (actual: 21 docs + 2 `.cs` = 23). This audit did not rely on that classification; scope and file lists below were independently derived from `git diff --name-status 4022fe7c9b07119224ca5aaa880b0a4003ef08db..dfbebb13fdc9ce2e9240376be2214dddf56ee5d0` and `git diff --stat`. + +## 1. General Unit Test Policy Compliance + +### 1.1 Core Principles (Independence, Isolation, Fast Execution, Determinism, Readability) + +- Independence/Isolation: each new test constructs its own `Mock`/`Mock` and `StoreWrapperController`; no shared mutable state between tests. **PASS**. +- The two `Launch()` tests mutate the static `MyBox.DialogInvoker` seam but save/restore it in `try`/`finally`, preventing cross-test leakage. **PASS**. +- Fast/Deterministic: no `Thread.Sleep`, `Task.Delay`, wall-clock reads, or unseeded randomness in the new tests. **PASS**. +- Readability: descriptive test names (`Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer`, `EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable`) and XML-doc comments stating scenario/expected outcome on every new test. **PASS**. +- Arrange-Act-Assert structure observed in all 7 new tests (verified by direct reading of `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`). **PASS**. + +### 1.2 Coverage and Scenarios + +#### 1.2.1 Per-Language Coverage Comparison + +- C#: Baseline: 85.87% line / 86.68% block (`UtilitiesCS.dll`, `evidence/baseline/test-coverage-baseline.md`). Post-change: 85.88% line / 86.69% block (`UtilitiesCS.dll`, `evidence/qa-gates/qa-04-test-coverage.md`, cross-verified against `TestResults/final-coverage.xml` module `UtilitiesCS.dll`: lines_covered=36897/42964). Change: +0.01% line, no regression on previously-covered lines (4163/4163 baseline-passing tests still pass; 4170/4170 post-change). New/changed-code coverage: 100.00% line / 100.00% block for `EvaluateLaunchReadiness()` and the two `StoreLaunchReadiness` factory methods (verified via `TestResults/final-coverage.xml` function id 295736 and related entries; `Launch()` itself is reported `skipped_function reason="attribute_excluded"`, consistent with its pre-existing `[ExcludeFromCodeCoverage]` attribute, unchanged by this PR). Disposition: PASS for new/changed-code and no-regression checks; **FAIL for the repo-wide row** — see below. Evidence: `evidence/baseline/test-coverage-baseline.md`, `evidence/qa-gates/qa-04-test-coverage.md`, `evidence/qa-gates/qa-05-coverage-delta.md`, `TestResults/final-coverage.xml` (uncommitted, generated this session, not at the canonical `artifacts/csharp/coverage.xml` path). +- TypeScript: Baseline: N/A. Post-change: N/A. Change: N/A. Disposition: N/A. Evidence: N/A — no TypeScript files changed on this branch. +- PowerShell: Baseline: N/A. Post-change: N/A. Change: N/A. Disposition: N/A. Evidence: N/A — no PowerShell files changed on this branch. +- Python: Baseline: N/A. Post-change: N/A. Change: N/A. Disposition: N/A. Evidence: N/A — no Python files changed on this branch. + +##### Coverage Evidence Checklist + +- TypeScript baseline coverage artifact: N/A - out of scope (no `.ts`/`.tsx` files changed). +- TypeScript post-change coverage artifact: N/A - out of scope. +- PowerShell baseline coverage artifact: N/A - out of scope (no `.ps1`/`.psm1` files changed). +- PowerShell post-change coverage artifact: N/A - out of scope. + +#### 1.2.2 Repo-Wide Coverage Verdict (Mandatory Coverage Verification) + +Per the review contract's Coverage Verification procedure, a canonical repo-wide coverage artifact (`artifacts/csharp/coverage.xml`) is required whenever C# files changed in the branch diff. This branch changes two `.cs` files, so the check is mandatory. + +- **No `artifacts/csharp/coverage.xml` exists in this repository** (the `artifacts/` directory is entirely git-ignored and was not populated in this session). Per the mandatory procedure: **FAIL** — "coverage artifact absent for C#; coverage verification is mandatory for all languages with changed files." +- The feature's own evidence (`evidence/qa-gates/qa-04-test-coverage.md`, `qa-05-coverage-delta.md`) labels 85.88% as "repository (testable-denominator) line coverage," but this figure is scoped to the single `UtilitiesCS.dll` module, not the full C# solution. +- This review independently inspected the underlying coverage data generated during the feature's own P3-T4 run (`TestResults/final-coverage.xml`, uncommitted). That run also instrumented other first-party/vendored modules loaded transitively by `UtilitiesCS.Test`: `TaskMaster.dll` (8.58% line), `SVGControl.dll` (15.15% line), `Swordfish.NET.General.dll` (45.86% line), `Tags.dll` (0.00% line), `ToDoModel.dll` (0.00% line), `QuickFiler.dll` (0.00% line). The near-zero readings for `Tags.dll`/`ToDoModel.dll`/`QuickFiler.dll` most likely reflect that their own dedicated test projects were not executed in this run (only `UtilitiesCS.Test.dll` was run), not that those modules are genuinely untested — so this data point cannot be treated as a defensible "true repo-wide" percentage either; it merely confirms that no single-project test run produces a valid repo-wide figure. +- A line-weighted aggregate across the modules above (from the same uncommitted artifact) is approximately 64% line coverage — below both the CLAUDE.md 80% floor and the repo-rule 85%/75% floor — but is presented here only as corroborating evidence of risk, not as a certified repo-wide measurement, for the reason above. +- **Disposition: FAIL** (artifact absent / no valid canonical repo-wide measurement exists). This condition pre-dates issue #240 (it is not attributable to the two files this branch touches) and is partially addressed by the project's own ratified COM/VSTO/WinForms coverage-exemption initiative referenced in `CLAUDE.md` UT2 (tracked in `feature/csharp-coverage-uplift`). It is carried into remediation inputs as a systemic, non-blocking-for-this-bugfix tracking item, not as a defect introduced by this PR. + +### 1.3 Scenario Completeness + +For `EvaluateLaunchReadiness()`: null `Globals` (edge), null `Ol` (edge), null `StoresWrapper` (negative/root-cause primary), null `Stores` list (negative/root-cause secondary), and the happy path with populated stores (positive) are all covered — 5 of 5 branches of the readiness state machine. For `Launch()`: both not-ready paths (null model, null stores list) are covered with an assertion that no exception is thrown, the dialog seam fires exactly once, and `Viewer` remains null. **PASS**. + +### 1.4 External Dependencies and Environment + +No live Outlook process, no real files, no temporary files. `IApplicationGlobals`/`IOlObjects` are mocked via Moq; the WinForms dialog is intercepted via the existing `MyBox.DialogInvoker` injectable seam (not a raw `MessageBox.Show`/`Form.ShowDialog` call), confirmed by direct inspection of `UtilitiesCS/Dialogs/MyBox.cs`. **PASS**. + +### 1.5 Test File Location + +Tests are added to the existing `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`, mirroring `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` under the repo's established `.Test` mirrored-structure convention (this repo does not use a literal `tests/` root; the `.Test` convention is applied consistently across the whole C# codebase). Consistent with CLAUDE.md §7 ("match existing style"). **PASS** by established convention. + +### 1.6 File-Size Limit (Test Code) + +`UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` is **781 lines** after this change (`wc -l`/`awk` verified), up from a pre-existing **582-line** baseline at the merge-base commit (already over the repo's 500-line limit before this issue). This PR's diff adds 199 lines to the file (`git diff --stat`: `StoreWrapperController_Tests.cs | 199 +++++++++++++++++++++`). The general-code-change policy's 500-line file limit applies to test code with no exemption for this case. **FAIL** — this is a genuine, worsened policy violation, self-disclosed by the executor in `evidence/other/scope-budget-confirmation.md` and `evidence/other/plan-status-summary.md` as an unresolved deviation flagged for remediation rather than fixed unilaterally. + +## 2. General Code Change Policy Compliance + +- Design principles: the fix follows simplicity-first and separation-of-concerns — pure decision logic (`EvaluateLaunchReadiness`) is extracted from the I/O/UI shell (`Launch()`), which keeps its pre-existing `[ExcludeFromCodeCoverage]` attribute. **PASS**. +- Error handling: no exception is silently swallowed; the not-ready paths return a typed sentinel and the caller explicitly branches on it rather than catching an exception. **PASS**. +- Naming: `StoreLaunchReadinessState`, `StoreLaunchReadiness`, `EvaluateLaunchReadiness` are descriptive and unambiguous. **PASS**. +- Public API impact: `EvaluateLaunchReadiness()` and the new types are `internal`, and `Launch()`'s public signature is unchanged. No breaking change. **PASS**. +- File-size limit: production file `StoreWrapperController.cs` is 396 lines (`<= 500`), verified directly. **PASS**. Test file: see §1.6 above — **FAIL**. +- Comment quality: the new XML doc comments and the `#pragma` "why:" comment explain rationale, not restatement of code. **PASS**. +- I/O boundary isolation / no temp files: confirmed, see §1.4. **PASS**. + +## 3. Language-Specific Code Change Policy Compliance (C#) + +- Formatting (CSharpier): `evidence/qa-gates/qa-01-format.md` — mutation pass reformatted the two touched files, loop was restarted per the plan's loop rule, and the subsequent verification pass (`csharpier check .`) reports 0 files requiring reformatting, `EXIT_CODE 0`. **PASS**. +- .NET Analyzers: `evidence/qa-gates/qa-02-analyzers.md` — `EXIT_CODE 0`, 70 warnings (baseline 72, no increase), zero diagnostics attributable to either touched file. **PASS**. +- Nullable / TreatWarningsAsErrors: `evidence/qa-gates/qa-03-nullable.md` — solution-wide `EXIT_CODE 1` reproduces the exact pre-existing baseline condition (`evidence/baseline/nullable-baseline.md`, itself `EXIT_CODE 1` for the same vendored `SVGControl.csproj`/`Swordfish.NET.General.csproj` reasons, unrelated to this issue). A scoped rebuild of the touched projects (`-p:BuildProjectReferences=false`) confirms `StoreWrapperController_Tests.cs` contributes zero diagnostics and `StoreWrapperController.cs` contributes diagnostics only on pre-existing, unmodified lines (constructor `CS8618` x8, `SelectFolder`/`SelectFsFolder` `CS8603` x3). One genuine 2-diagnostic regression (`CS8625` on the new `StoreLaunchReadiness.NotReady` constructor call) was found and fixed with a narrowly-scoped, documented `#pragma warning disable/restore CS8625`, per C#7's suppression-narrowness requirement. **PASS with documented pre-existing exception** (consistent with AC5's own caveat wording). +- Suppression practice: the `#pragma` block is minimal (wraps exactly the `new(state, null, null)` call), carries a `why:` comment explaining the rejected alternative (`?`-nullable annotation, which would introduce new `CS8632` warnings on a project without a `` setting), and is restored immediately after. **PASS**. + +## 4. Language-Specific Unit Test Policy Compliance (C#) + +- Framework: MSTest (`[TestClass]`/`[TestMethod]`), confirmed by direct inspection of the diff. **PASS**. +- Mocking: Moq (`Mock`, `Mock`), used for all 7 new tests. **PASS**. +- Assertions: FluentAssertions (`Should().NotThrow()`, `Should().Be(...)`, `Should().BeNull()`, `Should().Equal(...)`) used throughout; no bare MSTest `Assert` calls in the new tests. **PASS**. +- Toolchain command selection matches CUT3 (`csharpier`, `msbuild ... EnableNETAnalyzers`, `msbuild ... Nullable=enable`, `vstest.console.exe ... /EnableCodeCoverage`). **PASS**. + +## 5. Test Coverage Detail + +| Scope | Metric | Value | Threshold | Verdict | +|---|---|---|---|---| +| New code (`EvaluateLaunchReadiness`, `StoreLaunchReadiness.NotReady/Ready`) | Line | 100.00% | >= 90% (CLAUDE.md) / >= 85% (repo rule) | PASS | +| New code (same scope) | Block/branch | 100.00% | >= 75% | PASS | +| Modified file changed lines (`Launch()` guard branch) | N/A — excluded via pre-existing `[ExcludeFromCodeCoverage]`, unchanged by this PR | N/A | No regression | PASS | +| Single assembly `UtilitiesCS.dll` (mislabeled "repository" in feature evidence) | Line | 85.88% (baseline 85.87%) | Informational only — not the canonical repo-wide gate | Context only | +| Repo-wide, all C# assemblies (canonical `artifacts/csharp/coverage.xml`) | Line | Not measurable this session — no canonical artifact; partial same-session data suggests ~64% line coverage across loaded modules, with the caveat in §1.2.2 | >= 80% (CLAUDE.md) / >= 85% (repo rule) | **FAIL** (artifact absent) | + +## 6. Test Execution Metrics + +- Baseline (`evidence/baseline/test-coverage-baseline.md`): 4163 total, 4163 passed, 0 failed. +- Fail-before (`evidence/regression-testing/fail-before-240.md`): 2 total (filtered), 0 passed, 2 failed — reproduces the issue #240 crash (one `NullReferenceException`, one `ArgumentNullException`, both unhandled exceptions from the same unguarded code path). +- Pass-after (`evidence/regression-testing/pass-after-240.md` / `evidence/qa-gates/qa-04-test-coverage.md`): 4170 total, 4170 passed, 0 failed. Delta: +7 tests (2 regression + 5 unit), 0 regressions. +- Cross-verification: `TestResults/final-coverage.xml` (uncommitted) lists per-function coverage for all 7 new test methods with non-zero `blocks_covered`/`lines_covered`, corroborating that they executed (consistent with, though not a substitute for, the reported "Passed" counts). + +## 7. Code Quality Checks + +| Check | Command / Method | Result | +|---|---|---| +| Confidentiality masking scan | Manual diff review for secrets/credentials in new `.cs`/`.md` files | No secrets or credentials found | +| Suppression scan (added lines) | Manual review of new `#pragma` usage | 1 narrowly-scoped `#pragma warning disable/restore CS8625` pair, documented with a `why:` comment; compliant with C#7 | +| Workflow change scan | `git diff --name-only` filtered for `.github/workflows/**` | No workflow files changed; `.claude/rules/ci-workflows.md` not applicable | +| Benchmark baseline scan | `git diff --name-only` filtered for `scripts/benchmarks/**` | No benchmark files changed; `.claude/rules/benchmark-baselines.md` not applicable | +| Orchestrator-state scan | `git diff --name-only` filtered for `orchestrator-state.json` | No orchestrator-state checkpoint changed; `.claude/rules/orchestrator-state.md` not applicable | + +## 8. Gaps and Exceptions + +1. **Test file 500-line limit** (`UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`, 781 lines) — pre-existing violation (582 lines at baseline), worsened by +199 lines from this PR. Self-disclosed by the executor; not resolved. Carried to remediation inputs as **Blocking**. +2. **Repo-wide C# coverage artifact absent** — no `artifacts/csharp/coverage.xml`; the feature's "repository line coverage" claim (85.88%) is single-assembly-scoped. Pre-existing, systemic, not attributable to this PR's two changed files; the project's own new/changed-code coverage (100%) is not in question. Carried to remediation inputs as a **tracked, non-blocking-for-#240** systemic item. +3. **PR-context summary misclassification** — "Core logic changes: 0 files" omits both changed `.cs` files. Process/tooling gap, not a code defect. Carried to remediation inputs as **informational**. +4. Nullable gate's solution-wide `EXIT_CODE 1` is a pre-existing, documented, unrelated vendored-project condition — accepted, not a gap requiring remediation for this PR. + +## 9. Summary of Changes + +- `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`: +101/-4 lines. Adds `StoreLaunchReadinessState`, `StoreLaunchReadiness`, `EvaluateLaunchReadiness()`; modifies `Launch()` to branch on readiness before touching `Model`. +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`: +199 lines. Adds 2 `Launch()` regression tests and 5 `EvaluateLaunchReadiness()` unit tests. +- 21 documentation/evidence files added under `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/` (issue, plan, research, evidence artifacts). +- No changes to `TaskMaster/Ribbon/RibbonController.cs` or `TaskMaster/AppGlobals/AppOlObjects.cs` (confirmed excluded from scope by the plan and by `git diff --name-only`). + +## 10. Compliance Verdict + +**PARTIAL.** The core fix, its regression tests, and its new-code coverage fully satisfy the General/C#-specific Code Change and Unit Test policies. Two findings prevent an unqualified PASS: (a) the test file's 500-line limit violation, worsened by this PR (Blocking), and (b) the absence of a canonical repo-wide C# coverage artifact, which is a pre-existing, systemic condition not caused by this PR but which the coverage-verification procedure requires to be reported as FAIL for any language with changed files. See `remediation-inputs.2026-07-06T12-15.md`. + +## Appendix A: Test Inventory + +| Test | Type | Target | Result | +|---|---|---|---| +| `Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` | Regression (fail-before/pass-after) | `Launch()` (AC1) | Fail-before: FAIL (`NullReferenceException`). Pass-after: PASS | +| `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` | Regression (fail-before/pass-after) | `Launch()` (AC2) | Fail-before: FAIL (`ArgumentNullException`). Pass-after: PASS | +| `EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable` | Unit (edge) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable` | Unit (edge) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable` | Unit (negative) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable` | Unit (negative) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames` | Unit (positive) | `EvaluateLaunchReadiness()` | PASS | + +## Appendix B: Toolchain Commands Reference + +| Stage | Command | Result | +|---|---|---| +| Format | `dotnet tool run csharpier check .` (then `format .`, then re-`check .`) | 0 files require reformatting after mutation pass; `EXIT_CODE 0` | +| Analyzers | `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` | `EXIT_CODE 0`, 70 warnings (baseline 72), 0 errors | +| Nullable | `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` (invoked as `-t:Rebuild`) | `EXIT_CODE 1` — pre-existing, unrelated vendored-project condition; scoped rebuild confirms 0 new diagnostics on touched files | +| Test + Coverage | `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation` | `EXIT_CODE 0`, 4170/4170 passed | diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T12-15.md new file mode 100644 index 000000000..0d754e245 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T12-15.md @@ -0,0 +1,50 @@ +# Remediation Inputs — store-wrapper-launch-npe (Issue #240) + +- Timestamp: 2026-07-06T12-15 +- Source artifacts: `policy-audit.2026-07-06T12-15.md`, `code-review.2026-07-06T12-15.md`, `feature-audit.2026-07-06T12-15.md` + +## Finding 1 — Blocking: Test file exceeds the 500-line policy limit + +- **Status: remediation-required (Blocking)** +- File: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` +- Current size: 781 lines. Baseline (merge-base) size: 582 lines — already over the repository's 500-line limit before this issue. This PR's diff adds 199 lines (`git diff --stat` confirms `+199` insertions). +- Policy: `.claude/rules/general-code-change.md` / CLAUDE.md General Code Change Policy §4 — "No production code, test code, or reusable script file may exceed 500 lines." No listed exemption applies to this file. +- Evidence: `policy-audit.2026-07-06T12-15.md` §1.6, §8 item 1; `code-review.2026-07-06T12-15.md` Findings Table row 1; `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/other/scope-budget-confirmation.md` (executor's own self-disclosure of this deviation, un-remediated). +- Recommended action: split `StoreWrapperController_Tests.cs` into cohesive sub-files (e.g., separate the `Launch(...)`/`EvaluateLaunchReadiness(...)` region added by this issue into its own file, or partition by another existing cohesive boundary already present in the file), so each resulting file is <= 500 lines. This can be done without touching production code or altering any test's behavior/assertions. +- Owner/next step: executor or a follow-up task under this issue's plan; must be resolved before this change is considered fully policy-compliant. + +## Finding 2 — Tracked, non-blocking-for-#240: Repo-wide C# coverage artifact absent + +- **Status: remediation-required (systemic tracking item; not blocking for issue #240's own merge)** +- No canonical `artifacts/csharp/coverage.xml` exists in this repository session. Per the mandatory Coverage Verification Procedure, this is a FAIL for the "Repo-wide per language" gate for C# (C# has changed files on this branch). +- The feature's own evidence (`evidence/qa-gates/qa-04-test-coverage.md`, `qa-05-coverage-delta.md`) labels 85.88% as "repository (testable-denominator) line coverage," but this is scoped to the single `UtilitiesCS.dll` module. Same-session partial data (`TestResults/final-coverage.xml`, uncommitted) shows other first-party modules (`TaskMaster.dll`, `Tags.dll`, `ToDoModel.dll`, `QuickFiler.dll`) at low-to-zero coverage when loaded under the single-project `UtilitiesCS.Test` run — most likely an artifact of those modules' own dedicated test projects not being executed in this run, not a certified measurement. +- This condition pre-dates issue #240 and is not attributable to either file this PR changed; issue #240's own new/changed-code coverage is independently verified at 100%. +- Evidence: `policy-audit.2026-07-06T12-15.md` §1.2.2, §5, §8 item 2. +- Recommended action: track under the repository's existing `feature/csharp-coverage-uplift` initiative (referenced in CLAUDE.md's General Unit Test Policy UT2 COM/VSTO/WinForms exemption clause) — produce a canonical, multi-project coverage merge (`artifacts/csharp/coverage.xml`, Cobertura or equivalent) covering all first-party C# test projects (`UtilitiesCS.Test`, `TaskMaster.Test`, `QuickFiler.Test`, `Tags.Test`, `ToDoModel.Test`, and any others), scoped to the testable denominator after applying the ratified COM/VSTO/WinForms exclusions, so future reviews can render a defensible repo-wide verdict without ad hoc reconstruction. +- Owner/next step: repository maintainer / CI-infrastructure owner, not this issue's executor. + +## Finding 3 — Informational: PR-context summary misclassifies changed C# files + +- **Status: informational (process/tooling gap, not a code defect)** +- `artifacts/pr_context.summary.txt`'s "Changed files overview" reports "Core logic changes: 0 files" and files both changed `.cs` files into "Docs/templates/agents/tooling: 21 files" (actual count for that bucket should be 21, with the 2 `.cs` files reported separately as core logic). +- This repeats a previously-documented misclassification pattern for C# changes in this PR-context generator. +- Evidence: `policy-audit.2026-07-06T12-15.md` "PR-Context Artifact Reliability Note"; direct comparison against `git diff --stat`/`git diff --name-status`. +- Recommended action: fix the PR-context summary generator's file-classification logic so `.cs` core-logic files are not bucketed into "docs/templates/agents/tooling." Reviewers should continue to independently verify scope via `git diff` rather than relying on the summary's classification until this is corrected. +- Owner/next step: owner of the PR-context artifact generation tooling. + +## Finding 4 — Documentation discrepancy: AC5 check-off vs. review verdict + +- **Status: remediation-required (documentation reconciliation)** +- `issue.md` AC5 is checked `[x]`, but this review's verdict for AC5 is PARTIAL (Finding 2 above is the specific cause: the "repository line coverage >= 80%" clause is not substantiated at true repo-wide scope). +- Evidence: `feature-audit.2026-07-06T12-15.md` "Acceptance Criteria Check-off" section. +- Recommended action: maintainer should either (a) narrow AC5's wording to explicitly scope "repository line coverage" to the `UtilitiesCS` project/testable denominator actually measured, or (b) wait for Finding 2's canonical repo-wide artifact before treating AC5 as fully satisfied. +- Owner/next step: maintainer (Dan Moisan) / issue #240 owner. + +## Remediation Priority Summary + +| Finding | Severity | Blocking for #240 merge? | +|---|---|---| +| 1. Test file 500-line limit | Blocking | Yes | +| 2. Repo-wide C# coverage artifact absent | Systemic / FAIL per procedure | No (pre-existing, tracked separately) | +| 3. PR-context summary misclassification | Informational | No | +| 4. AC5 check-off discrepancy | Documentation | No (but should be reconciled) | diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-plan.2026-07-06T12-15.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-plan.2026-07-06T12-15.md new file mode 100644 index 000000000..e21b9b460 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-plan.2026-07-06T12-15.md @@ -0,0 +1,83 @@ +# Remediation Plan: store-wrapper-launch-npe (Issue #240) + +**Plan timestamp:** 2026-07-06T12-15 +**Authored by:** atomic-planner +**Feature folder:** `docs/features/active/2026-07-06-store-wrapper-launch-npe-240` +**Authoritative input:** `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T12-15.md` + +## Scope statement + +This plan remediates exactly one finding: **Finding 1 (Blocking)** — `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` is 781 lines, exceeding the repository's 500-line file-size limit (General Code Change Policy §4 / `.claude/rules/general-code-change.md`). + +Findings 2, 3, and 4 from `remediation-inputs.2026-07-06T12-15.md` are explicitly **out of scope** for this plan and MUST NOT be touched: +- Finding 2 (repo-wide C# coverage artifact absent) — tracked separately under `feature/csharp-coverage-uplift`. +- Finding 3 (PR-context summary misclassification) — informational, owned by PR-context tooling. +- Finding 4 (AC5 check-off vs. review verdict) — documentation reconciliation, owned by the maintainer. + +**Containment invariant (must hold for the whole cycle):** zero diff to any production file, including `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`. This cycle touches only test files and `UtilitiesCS.Test/UtilitiesCS.Test.csproj`. + +## Proposed file split + +Source file: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (781 lines; `[TestClass] public class StoreWrapperController_Tests`; namespace `UtilitiesCS.Test.OutlookObjects.Store`; 39 `[TestMethod]`s; usings: `System`, `System.Collections.Generic`, `System.Linq`, `System.Windows.Forms`, `FluentAssertions`, `Microsoft.VisualStudio.TestTools.UnitTesting`, `Moq`, `UtilitiesCS.OutlookObjects.Folder`, `UtilitiesCS.OutlookObjects.Store`). + +The file is organized into 16 named `#region` blocks. The split follows the repo's existing partial-class convention for over-cap test files (see `UtilitiesCS.Test/EmailIntelligence/ClassifierGroups/Triage_Tests.cs` + `Triage_Tests.ManagerAndAdditional.cs`, both `public partial class Triage_Tests`). All three resulting files declare `public partial class StoreWrapperController_Tests` in the same namespace and carry the identical 9-using header (safe superset; any unused using is at most an IDE0005 suggestion-severity diagnostic, not a build error under `TreatWarningsAsErrors`, so it does not risk the nullable gate). + +### File A (trimmed original) — `StoreWrapperController_Tests.cs` + +Keeps the `[TestClass]` attribute and regions, verbatim, in original order: +`RunFolderSelectionDialog`, `PairwiseEquals`, `Constructor`, `AnyChanges`, `ButtonCancel_Click`, `GetRelativeFsPath`, `SaveChanges`, `Helpers`. + +Class declaration changes from `public class StoreWrapperController_Tests` to `public partial class StoreWrapperController_Tests` (attribute and everything else in these regions unchanged). Projected length: ~181 lines. + +### File B (new) — `StoreWrapperController_Tests.ButtonAndPopulate.cs` + +`public partial class StoreWrapperController_Tests` (no `[TestClass]` attribute — already declared on File A), same namespace and using header. Contains regions, verbatim, in original order: +`ButtonOk_Click`, `AnyChanges variants`, `GetRelativeFsPath variants`, `PopulateWithCurrent`, `Click handlers (non-invoke path)`. + +This region set includes the `StubSelectFolderController` reference used by `ArchiveOutlook_Click_SelectFolderReturnsFolder_SetsArchiveOutlookToReturnedFolder`; that nested class itself moves to File C but remains visible here because partial-class members are visible across all files of the same type regardless of physical file location. Projected length: ~396 lines. + +### File C (new) — `StoreWrapperController_Tests.Launch.cs` + +`public partial class StoreWrapperController_Tests` (no `[TestClass]` attribute), same namespace and using header. Contains regions, verbatim, in original order: +`Launch (issue #240 regression)`, `EvaluateLaunchReadiness (issue #240)`, `Stub helpers`. + +This is the region set added by issue #240 itself (the recommended split boundary named in `remediation-inputs.2026-07-06T12-15.md`). Projected length: ~234 lines. + +Total `[TestMethod]` count after split: 39 (unchanged — no test added, removed, or renamed; only region relocation across files). + +--- + +### Phase 0 — Compliance read and baseline capture + +- [x] [P0-T1] Read the policy files in the required order (`CLAUDE.md`; `.claude/rules/general-code-change.md`; `.claude/rules/general-unit-test.md`; `.claude/rules/csharp.md`) and the cycle-entry input `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T12-15.md`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/phase0-instructions-read.md` exists with `Timestamp:`, `Policy Order:`, and the explicit list of all 5 files read. +- [x] [P0-T2] Confirm the sole in-scope finding and containment boundary. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/scope-confirmation.md` exists recording `Timestamp:` and a statement that only Finding 1 is in scope for this cycle, Findings 2-4 are explicitly excluded, and `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` must remain untouched. +- [x] [P0-T3] Capture the baseline line count of the in-scope file. Command: `(Get-Content "UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs" | Measure-Object -Line).Lines`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/file-size-baseline.md` exists with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` recording the file's line count as greater than 500 (confirming the violation). +- [x] [P0-T4] Capture baseline CSharpier formatting state for the in-scope file. Command: `dotnet tool run csharpier check UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/csharpier-baseline.md` exists with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:`. +- [x] [P0-T5] Capture baseline analyzer build. Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/analyzer-baseline.md` exists with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` (warning/error counts). +- [x] [P0-T6] Capture baseline nullable/type-check build. Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` (use `/t:Rebuild` if an incremental build skips `CoreCompile`). Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/nullable-baseline.md` exists with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:`, noting that any pre-existing failures confined to vendored/legacy projects (`SVGControl.csproj`, `UtilitiesSwordfish.NET.General.csproj`) are out of scope and unaffected by this cycle. +- [x] [P0-T7] Capture baseline test run with coverage. Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/test-coverage-baseline.md` exists with `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` recording the total/passed/failed test counts and the numeric repository (testable-denominator) line coverage percentage for `UtilitiesCS.dll`. +- [x] [P0-T8] Confirm all Phase 0 baseline artifacts exist with required schema fields before implementation begins. Acceptance: every artifact named in P0-T1..P0-T7 is present under `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/remediation-baseline/` and schema-complete (`Timestamp:`, and where applicable `Command:`/`EXIT_CODE:`/`Output Summary:`); the baseline test-pass count and coverage percentage from P0-T7 are recorded numerically. + +### Phase 1 — Split the over-cap test file + +- [x] [P1-T1] Create `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs` containing `public partial class StoreWrapperController_Tests` (no `[TestClass]` attribute) in namespace `UtilitiesCS.Test.OutlookObjects.Store`, with the identical 9-using header from the source file (`System`, `System.Collections.Generic`, `System.Linq`, `System.Windows.Forms`, `FluentAssertions`, `Microsoft.VisualStudio.TestTools.UnitTesting`, `Moq`, `UtilitiesCS.OutlookObjects.Folder`, `UtilitiesCS.OutlookObjects.Store`), and containing the regions `ButtonOk_Click`, `AnyChanges variants`, `GetRelativeFsPath variants`, `PopulateWithCurrent`, and `Click handlers (non-invoke path)` moved verbatim from `StoreWrapperController_Tests.cs` (byte-identical method bodies, attributes, doc comments, and region markers; no assertion weakened or removed). Acceptance: the file exists, declares `public partial class StoreWrapperController_Tests`, and contains exactly the 19 `[TestMethod]`s belonging to those five regions (`ButtonOk_Click_NoChanges_ClosesViewer`, `ButtonOk_Click_WithChanges_SavesAndCloses`, `AnyChanges_ArchiveOutlookDiffers_ReturnsTrue`, `AnyChanges_JunkEmailDiffers_ReturnsTrue`, `AnyChanges_JunkPotentialDiffers_ReturnsTrue`, `GetRelativeFsPath_ArchiveFsWithEmptyPath_ReturnsPlaceholder`, `GetRelativeFsPath_ArchiveFsWithPath_ConverterReturnsEmpty_ReturnsPlaceholder`, `GetRelativeFsPath_ArchiveFsWithPath_ConverterReturnsValues_ReturnsFormatted`, `PopulateWithCurrent_NullCurrent_SetsErrorLoadingText`, `PopulateWithCurrent_CurrentSetWithNulls_SetsPlaceholders`, `PopulateWithCurrent_WithKnownFolderValues_MirrorsControllerFieldsFromCurrent`, `ArchiveOutlook_Click_NullSelectedFolder_LeavesNull`, `JunkEmail_Click_NullSelectedFolder_LeavesNull`, `JunkPotential_Click_NullSelectedFolder_LeavesNull`, `ArchiveOutlook_Click_SelectFolderReturnsFolder_SetsArchiveOutlookToReturnedFolder`, `DisplayName_SelectedValueChanged_WithPendingChangesAndYesResponse_SavesThenLoadsSelectedStore`, `ClickHandlers_WhenInvokeRequired_DelegateToViewerInvoke`, `SelectFolder_WhenPickFolderReturnsFolder_WrapsRelativePathFromCurrentRoot`, `SelectFolder_WhenPickFolderThrows_ReturnsNull`). +- [x] [P1-T2] Create `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs` containing `public partial class StoreWrapperController_Tests` (no `[TestClass]` attribute) in namespace `UtilitiesCS.Test.OutlookObjects.Store`, with the identical 9-using header, and containing the regions `Launch (issue #240 regression)`, `EvaluateLaunchReadiness (issue #240)`, and `Stub helpers` moved verbatim from `StoreWrapperController_Tests.cs` (byte-identical method bodies, attributes, doc comments, and the `StubSelectFolderController` nested class, unchanged). Acceptance: the file exists, declares `public partial class StoreWrapperController_Tests`, and contains exactly the 7 `[TestMethod]`s (`Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer`, `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer`, `EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable`, `EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable`, `EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable`, `EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable`, `EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames`) plus the `private sealed class StubSelectFolderController` nested type. +- [x] [P1-T3] Trim `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` by removing the eight regions moved to File B and File C in P1-T1/P1-T2 (`ButtonOk_Click`, `AnyChanges variants`, `GetRelativeFsPath variants`, `PopulateWithCurrent`, `Click handlers (non-invoke path)`, `Launch (issue #240 regression)`, `EvaluateLaunchReadiness (issue #240)`, `Stub helpers`), retaining the `[TestClass]` attribute, all usings, and the remaining regions (`RunFolderSelectionDialog`, `PairwiseEquals`, `Constructor`, `AnyChanges`, `ButtonCancel_Click`, `GetRelativeFsPath`, `SaveChanges`, `Helpers`) byte-identical, and changing the class declaration from `public class StoreWrapperController_Tests` to `public partial class StoreWrapperController_Tests`. Acceptance: the file declares `public partial class StoreWrapperController_Tests` with the `[TestClass]` attribute present exactly once, contains exactly the 13 `[TestMethod]`s belonging to the 8 retained regions, and contains zero methods from the 8 moved regions. +- [x] [P1-T4] Add two `` entries to `UtilitiesCS.Test/UtilitiesCS.Test.csproj`, immediately after the existing `` line, in this order: `` then ``. Acceptance: the csproj is well-formed XML and contains exactly 3 `` entries matching the `StoreWrapperController_Tests` family (the original plus the two new ones), with no duplicate entries. + +**Phase 1 verification (split correctness)** + +- [x] [P1-T5] Verify the line-count cap on all three resulting files. Command (PowerShell): `Get-ChildItem UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests*.cs | ForEach-Object { "$($_.Name): $((Get-Content $_.FullName | Measure-Object -Line).Lines)" }`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-linecount-verification.md` records `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` showing all three files (`StoreWrapperController_Tests.cs`, `StoreWrapperController_Tests.ButtonAndPopulate.cs`, `StoreWrapperController_Tests.Launch.cs`) at <= 500 lines each. +- [x] [P1-T6] Verify no test method was dropped, added, or renamed. Command: `Select-String -Path UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests*.cs -Pattern "\[TestMethod\]" | Measure-Object | Select-Object -ExpandProperty Count`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-testmethod-verification.md` records `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` confirming the combined `[TestMethod]` count across the three files equals 39 (the pre-split baseline count recorded via `Select-String -Path UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs -Pattern "\[TestMethod\]" | Measure-Object` before P1-T1..P1-T3 were applied, matching the count stated in this plan's "Proposed file split" section), and that the specific method-name lists match those enumerated in P1-T1/P1-T2/P1-T3. +- [x] [P1-T7] Verify containment — no production file was touched. Command: `git diff --stat HEAD`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/split-containment-verification.md` records `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` confirming the diff touches only `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`, `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs` (new), `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs` (new), and `UtilitiesCS.Test/UtilitiesCS.Test.csproj`, and that `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` shows zero diff. + +### Phase 2 — Final QA loop + +Run the full C# toolchain in order, restarting from step 1 (CSharpier) if any step fails or changes files, until one clean uninterrupted pass completes: + +- [x] [P2-T1] Run CSharpier formatting. Command: `dotnet tool run csharpier .`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-01-format.remediation-2026-07-06T12-15.md` records `Timestamp:`, `Command:`, `EXIT_CODE: 0`, and `Output Summary:` confirming zero files reformatted on the final pass. If files were reformatted, restart this loop from P2-T1. +- [x] [P2-T2] Run the analyzer build. Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-02-analyzers.remediation-2026-07-06T12-15.md` records `Timestamp:`, `Command:`, `EXIT_CODE: 0`, and `Output Summary:` confirming zero new analyzer errors/warnings introduced by the three split files versus the P0-T5 baseline. On failure, fix and restart from P2-T1. +- [x] [P2-T3] Run the nullable/type-check build. Command: `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug /p:Platform="Any CPU" /p:Nullable=enable /p:TreatWarningsAsErrors=true` (use `/t:Rebuild` if an incremental build skips `CoreCompile`). Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-03-nullable.remediation-2026-07-06T12-15.md` records `Timestamp:`, `Command:`, `EXIT_CODE:`, and `Output Summary:` confirming zero new nullable diagnostics in the three touched/new files, with any pre-existing failure confined to the same out-of-scope vendored projects documented in P0-T6. On a new-diagnostic failure in the touched files, fix and restart from P2-T1. +- [x] [P2-T4] Run the test suite with coverage. Command: `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation`. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md` records `Timestamp:`, `Command:`, `EXIT_CODE: 0`, and `Output Summary:` with total/passed/failed test counts equal to the P0-T7 baseline (no test dropped or newly failing) and the numeric post-change repository (testable-denominator) line coverage percentage for `UtilitiesCS.dll`. On failure or file change, restart from P2-T1. +- [x] [P2-T5] Verify the coverage no-regression criterion. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/qa-05-coverage-delta.remediation-2026-07-06T12-15.md` reports the P0-T7 baseline coverage percentage, the P2-T4 post-change coverage percentage, and confirms the post-change value is >= the baseline value (no regression) and >= 80% for the testable denominator. +- [x] [P2-T6] Record the remediation cycle end-state summary. Acceptance: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence/qa-gates/remediation-endstate.2026-07-06T12-15.md` records that all three resulting files are <= 500 lines (linking P1-T5), all 39 test methods are preserved with zero added/dropped (linking P1-T6), containment held with zero diff to `StoreWrapperController.cs` (linking P1-T7), and the full toolchain (P2-T1..P2-T4) passed in a single final clean pass with no `SKIPPED` outcomes. From f96a40552a41db866942a19c64ffd1b7e2f97dfe Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 6 Jul 2026 08:30:15 -0400 Subject: [PATCH 3/5] docs(240): record cycle-2 reaudit artifacts and reconcile AC5 coverage scope - Add reaudit policy-audit/code-review/feature-audit and remediation-inputs (2026-07-06T13-00) confirming the 500-line blocking finding is resolved - Annotate AC5 to scope the repository-coverage claim to the measured UtilitiesCS testable denominator and flag the repo-wide coverage artifact as maintainer-owned (feature/csharp-coverage-uplift) Refs: #240 --- .../project_msbuild-invocation-via-bash.md | 2 + .../code-review.2026-07-06T13-00.md | 27 +++ .../feature-audit.2026-07-06T13-00.md | 74 +++++++ .../issue.md | 2 +- .../policy-audit.2026-07-06T13-00.md | 184 ++++++++++++++++++ .../remediation-inputs.2026-07-06T13-00.md | 52 +++++ 6 files changed, 340 insertions(+), 1 deletion(-) create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T13-00.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T13-00.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T13-00.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T13-00.md diff --git a/.claude/agent-memory/feature-review/project_msbuild-invocation-via-bash.md b/.claude/agent-memory/feature-review/project_msbuild-invocation-via-bash.md index 9d462c4b8..6ea1e4a17 100644 --- a/.claude/agent-memory/feature-review/project_msbuild-invocation-via-bash.md +++ b/.claude/agent-memory/feature-review/project_msbuild-invocation-via-bash.md @@ -15,3 +15,5 @@ The Bash tool's shell does not have `msbuild` or `vstest.console.exe` on PATH, a - Pattern: write a `.cmd` containing the full command with `/p:Platform="Any CPU"`, then `cmd.exe //c "$(cygpath -w /tmp/foo.cmd)"`. A bare `msbuild ...` that returns exit 127 has NOT validated anything — do not record it as a passing toolchain step. Verified working in the #211 review (analyzers/nullable both `Build succeeded, 0 warnings, 0 errors`; vstest 4109/4109 on TaskMaster.Test + UtilitiesCS.Test with `/TestCaseFilter:"TestCategory!=LiveOutlook"`). Contrast with [[csharp-local-fullsuite-coverage-blocked]], which is about a Moq binding redirect on the *full-assembly coverage* run, not the two-assembly local run used here. + +**Scoped single-project build for fast independent re-verification (issue #240 cycle-2 re-audit, 2026-07-06):** when targeting a single `.csproj` (not the `.sln`), `Platform=AnyCPU` (no space) works fine — the `MSB4126`/space-quoting problem is a solution-configuration-mapping issue, not a single-project one. Building just `UtilitiesCS.Test\UtilitiesCS.Test.csproj` (which pulls in its project-reference graph, including `UtilitiesCS`) with `/t:Rebuild /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` or `/p:Nullable=enable /p:TreatWarningsAsErrors=true` finishes in ~10-15s and is a legitimate, much faster substitute for a full `TaskMaster.sln` rebuild when the review only needs to confirm zero new diagnostics on a handful of touched files — grep the build log for the touched file names and for `: error `/`: warning ` lines rather than eyeballing the whole log. Likewise `vstest.console.exe /TestCaseFilter:"FullyQualifiedName~"` gives a fast targeted re-run (39 tests in ~2s) instead of the full 4000+ test suite, sufficient to independently corroborate an executor's full-suite pass claim for the specific class under review. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T13-00.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T13-00.md new file mode 100644 index 000000000..a769a3275 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T13-00.md @@ -0,0 +1,27 @@ +# Code Review — store-wrapper-launch-npe (Issue #240) + +- Timestamp: 2026-07-06T13-00 +- Review type: Re-audit after remediation (cycle 2) +- Scope: full branch diff `4022fe7c9b07119224ca5aaa880b0a4003ef08db..9e3615b9dd369e66338b4ad333fb7c5371ece0dd` +- Files reviewed directly: `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`, `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs`, `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs`, `UtilitiesCS.Test/UtilitiesCS.Test.csproj` + +## Executive Summary + +This cycle's diff (commit `9e3615b9`) is a pure test-file-size remediation: it splits `StoreWrapperController_Tests.cs` into three cohesive `partial class` files with zero production-code changes and zero test-behavior changes. Direct inspection confirms the split follows sensible seams — the retained file keeps constructor/helper/misc regions, `ButtonAndPopulate.cs` groups button-click and populate-related tests, and `Launch.cs` groups the issue #240 regression tests and the new `EvaluateLaunchReadiness()` unit tests together with their supporting stub class. This resolves the prior cycle's Medium-severity file-size finding. No new correctness, readability, or policy issues were introduced by the split. The two Low-severity forward-looking observations from the prior cycle (nullable-annotation debt on `StoreLaunchReadiness`'s sentinel factory; general nullable-migration debt) remain unchanged because the production file was not touched by this cycle's commit. + +## Findings Table + +| Severity | File | Location | Finding | Recommendation | Rationale | Evidence | +|---|---|---|---|---|---|---| +| Info | `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` (+ 2 new sibling files) | Whole-file split | Prior cycle's Medium finding (781-line file exceeding the 500-line limit) is resolved: the file is now 181 lines, with the two extracted regions living in `StoreWrapperController_Tests.ButtonAndPopulate.cs` (396 lines) and `StoreWrapperController_Tests.Launch.cs` (234 lines). All three files are `partial class StoreWrapperController_Tests`; only the file containing the class-level `[TestClass]` attribute needs it (verified present exactly once, on the retained file). | No action required. | General Code Change Policy's 500-line file limit; this is the correct mechanical remediation without behavior change. | `wc -l` on all three files (181/396/234, independently re-verified this cycle); `grep -c "\[TestClass\]"` = 1 across the three files | +| Info | `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs` | `StubSelectFolderController` nested class | The one non-test nested helper class (`StubSelectFolderController`) was relocated into the `Launch.cs` partial alongside a `#region Stub helpers`. This is a reasonable home for it since it is used by tests migrated to the same file, though it is a slight naming mismatch (a "Stub helpers" region living in a file named for `Launch`/`EvaluateLaunchReadiness`). | No action required; optional future cleanup could rename the file or region for clarity if more stub helpers accumulate. | Naming/cohesion guidance (CLAUDE.md §5) favors descriptive names, but this is a minor, non-blocking cohesion note, not a defect. | Direct read of `StoreWrapperController_Tests.Launch.cs`, `#region Stub helpers` block | +| Info | `UtilitiesCS.Test/UtilitiesCS.Test.csproj` | `` entries | Exactly two new `` lines were added for the two new files, alphabetically adjacent to the existing `StoreWrapperController_Tests.cs` entry, matching the project's existing ordering convention. No other `.csproj` changes. | No action required. | Matches "match existing style" guidance (CLAUDE.md §7). | `git diff` on `UtilitiesCS.Test.csproj`: `+2` lines only | +| Low (carried, unchanged) | `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` | `StoreLaunchReadiness.NotReady(...)` | Unchanged from the prior cycle: the "not ready" sentinel carries `Model = null` and `DisplayNames = null` in a project without nullable annotations, so a future caller reading `.Model`/`.DisplayNames` without checking `.State` first would get a silent `NullReferenceException` with no compiler warning. | Unchanged recommendation: consider a guard note in the property XML docs stating validity is conditioned on `State == Ready`. | Carried forward because this file has zero diff in this cycle (confirmed via `git diff` — the split commit touches only test files). | `git diff dfbebb13..9e3615b9 -- UtilitiesCS/` produces no output | +| Info (carried, unchanged) | `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` | `EvaluateLaunchReadiness()` / `Launch()` | Unchanged positive finding: the extraction correctly keeps `Launch()`'s pre-existing `[ExcludeFromCodeCoverage]` attribute on the untestable WinForms shell while the new decision logic remains directly testable. | No action required. | Unchanged from prior cycle; zero diff to this file confirms the pattern is unaffected. | Same as above | + +## Additional Observations + +- Independently re-ran `dotnet tool run csharpier check` against all four changed `.cs` files in this review session: `Checked 4 files in 803ms.`, exit code 0 — confirms the split files are correctly formatted, not merely self-reported. +- Independently re-ran the full `StoreWrapperController_Tests` suite (39 tests across the three files) via `vstest.console.exe ... /TestCaseFilter:"FullyQualifiedName~StoreWrapperController_Tests"`: 39/39 passed in 2.08 seconds, confirming the split introduced no test regressions. +- No opportunistic refactors were observed in the split commit beyond the file split itself and the two `.csproj` entries; `RibbonController.cs` and `AppOlObjects.cs` remain untouched, matching the plan's declared scope lock. +- The split commit also adds one new agent-memory note (`.claude/agent-memory/feature-review/project_testresults-coverage-xml-cross-module-check.md`) and updates the memory index; these are documentation/tooling artifacts outside the C# code-quality review scope and contain no secrets or credentials (manually reviewed). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T13-00.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T13-00.md new file mode 100644 index 000000000..00d06af60 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T13-00.md @@ -0,0 +1,74 @@ +# Feature Audit — store-wrapper-launch-npe (Issue #240) + +- Timestamp: 2026-07-06T13-00 +- Review type: Re-audit after remediation (cycle 2) +- Work mode: `minor-audit` +- AC source (per work-mode routing): `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md`, explicit `## Acceptance Criteria` section only + +## Scope and Baseline + +- Base branch (resolved): `main` @ `4022fe7c9b07119224ca5aaa880b0a4003ef08db` +- Head: `TaskMaster-wt-2026-07-06-06-35` @ `9e3615b9dd369e66338b4ad333fb7c5371ece0dd` +- Full branch diff audited (per Scope Invariant): 49 files changed, 1949 insertions(+), 406 deletions(-) across the full range — 5 `.cs`/`.csproj` files (1 production, 3 test, 1 project file) plus 44 documentation/memory files. +- This cycle's own commit (`9e3615b9`, remediation of the prior cycle's file-size finding): 8 files changed — 3 test `.cs` files, 1 `.csproj`, 2 agent-memory files, 2 prior-cycle review artifacts already present in the working tree. +- Production change remains confined to `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` (396 lines, zero diff in this cycle — independently confirmed via `git diff dfbebb13..9e3615b9 -- UtilitiesCS/`). Test code is now split across three files (181 / 396 / 234 lines) instead of one 781-line file. No changes to `TaskMaster/Ribbon/RibbonController.cs` or `TaskMaster/AppGlobals/AppOlObjects.cs` (verified via `git diff --name-only` over the full range), matching the plan's declared small-path scope lock. + +## Acceptance Criteria Inventory + +| AC | Text | +|---|---| +| AC1 | `StoreWrapperController.Launch()` does not throw an unhandled `NullReferenceException` when `Globals.Ol.StoresWrapper` (`Model`) is null. It fails gracefully with a clear user-facing message and returns without opening a broken dialog. | +| AC2 | `Launch()` also handles a non-null `Model` whose `Stores` list is null (transient post-deserialize state) without throwing. | +| AC3 | A deterministic MSTest regression test reproduces the pre-fix crash path (fails before the fix, passes after) using Moq for `IApplicationGlobals`/`IOlObjects`; no live Outlook, no temporary files. | +| AC4 | The underlying readiness/initialization gap identified by root-cause research is addressed so that invoking the store-settings command when store state is unavailable produces deterministic, non-crashing behavior rather than an unhandled exception. | +| AC5 | The full C# toolchain passes in order (csharpier -> .NET analyzers -> nullable/TreatWarningsAsErrors -> MSTest with coverage); coverage on changed lines meets the >= 90% new-code target and repository line coverage remains >= 80% for the testable denominator. | +| AC6 | All required PR CI checks are green against the PR head SHA. | + +## Acceptance Criteria Evaluation + +### AC1 — PASS (unchanged from prior cycle, re-verified independently) + +`Launch()` calls `EvaluateLaunchReadiness()` first; when `Globals?.Ol?.StoresWrapper` is null, the state is `ModelUnavailable` and `Launch()` shows a `MyBox.ShowDialog(...)` message and returns without constructing `Viewer`. This code is unchanged by this cycle's split commit (zero diff to `StoreWrapperController.cs`). The regression test `Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` (now in `StoreWrapperController_Tests.Launch.cs`) was independently re-run this cycle and passed. + +### AC2 — PASS (unchanged from prior cycle, re-verified independently) + +The `StoresUnavailable` branch is unchanged. `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` was independently re-run this cycle and passed. + +### AC3 — PASS (unchanged from prior cycle, re-verified independently) + +Both regression tests, now relocated verbatim to `StoreWrapperController_Tests.Launch.cs`, still use MSTest, Moq, and FluentAssertions with no live Outlook process and no temporary files (confirmed by direct reading of the post-split file). Fail-before/pass-after evidence from the prior cycle remains valid because the production code and test bodies are unchanged by this cycle's split. + +### AC4 — PASS (unchanged from prior cycle, re-verified independently) + +`EvaluateLaunchReadiness()` is unchanged (zero diff to `StoreWrapperController.cs`). All 5 `EvaluateLaunchReadiness_*` unit tests, now in `StoreWrapperController_Tests.Launch.cs`, were independently re-run this cycle (part of the 39/39 passing re-run) and passed. + +### AC5 — PARTIAL (unchanged verdict from prior cycle; independently re-confirmed, not merely carried forward) + +- Toolchain order: independently re-verified this cycle. Csharpier (`EXIT_CODE 0`, 4 files checked) and a scoped analyzer build (`EXIT_CODE 0`, 0 errors) both pass cleanly on the touched files. The nullable gate's solution-wide `EXIT_CODE 1` was independently reproduced and confirmed confined to the same two pre-existing, out-of-scope vendored projects (`SVGControl.csproj`, `UtilitiesSwordfish.NET.General.csproj`) with zero occurrences of `StoreWrapperController` anywhere in the build log. This portion remains satisfied. +- New-code coverage on changed lines: still PASS. `EvaluateLaunchReadiness()` and its factory methods are unchanged by this cycle and remain at 100% line/block coverage per the prior cycle's coverage run; independently corroborated this cycle by the targeted 39/39 test pass (a coverage regression would require a test failure, which did not occur). +- "Repository line coverage remains >= 80% for the testable denominator": still **not substantiated at the scope the phrase implies**. No canonical `artifacts/csharp/coverage.xml` exists (independently re-confirmed this cycle — see `policy-audit.2026-07-06T13-00.md` §1.2.2). The cited 85.88% figure remains scoped to the single `UtilitiesCS.dll` module. This clause remains unverifiable as PASS, so AC5 is evaluated PARTIAL, unchanged from the prior cycle. + +### AC6 — UNVERIFIED (deferred, unchanged) + +`evidence/other/ac6-deferral.md` explicitly defers AC6 to post-PR-creation CI. This review has no GitHub CLI access and cannot independently verify CI status against the current head SHA (`9e3615b9`). AC6 remains correctly unchecked in `issue.md`. + +## Summary + +The prior cycle's file-size remediation (commit `9e3615b9`) is verified, independently, to be a clean, behavior-preserving split: all 39 tests preserved and passing, zero production-code diff, all three resulting files within the 500-line limit, and the C# toolchain green for the touched files (csharpier and scoped analyzer build both `EXIT_CODE 0`; the solution-wide nullable gate's pre-existing `EXIT_CODE 1` is confined to two unrelated vendored projects). AC1-AC4 remain PASS, re-verified independently rather than merely re-asserted. AC5 remains PARTIAL for the same reason as the prior cycle: the "repository line coverage >= 80%" clause is not substantiated at genuine repo-wide scope because no canonical `artifacts/csharp/coverage.xml` exists. AC6 remains correctly deferred pending PR/CI evidence. No acceptance-criteria check-off changes were made by this review; the file-size finding that previously accompanied AC5's evaluation context is now resolved, but the coverage-artifact gap that separately drives AC5's PARTIAL verdict is unchanged and is documented again in this cycle's `remediation-inputs.2026-07-06T13-00.md` for maintainer follow-up. + +## Acceptance Criteria Check-off + +- AC1: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC2: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC3: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC4: `[x]` in `issue.md` — consistent with this review's PASS verdict. No change made. +- AC5: `[x]` in `issue.md` — **discrepancy persists, unchanged from prior cycle**. This review's verdict remains PARTIAL for the reason stated above. Per the AC check-off protocol, reviewers do not retroactively remove an executor's existing check-off; this checkbox is left as `[x]` and the gap is again flagged here and in `remediation-inputs.2026-07-06T13-00.md`. +- AC6: `[ ]` in `issue.md` — consistent with this review's UNVERIFIED/deferred status. No change made. + +### Acceptance Criteria Status + +- Source: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md` (`## Acceptance Criteria`) +- Total AC items: 6 +- Checked off (delivered): 5 (AC1-AC5, pre-existing from executor; AC5 carries a flagged discrepancy — see above) +- Remaining (unchecked): 1 +- Items remaining: AC6 ("All required PR CI checks are green against the PR head SHA.") — correctly deferred pending PR/CI evidence. diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md index db6eed89d..112a2855b 100644 --- a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md @@ -76,7 +76,7 @@ System.NullReferenceException - [x] AC2: `Launch()` also handles a non-null `Model` whose `Stores` list is null (transient post-deserialize state) without throwing. (Evidence: `evidence/regression-testing/pass-after-240.md`, fix in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs` P2-T3.) - [x] AC3: A deterministic MSTest regression test reproduces the pre-fix crash path (fails before the fix, passes after) using Moq for `IApplicationGlobals`/`IOlObjects`; no live Outlook, no temporary files. (Evidence: `evidence/regression-testing/fail-before-240.md` and `evidence/regression-testing/pass-after-240.md`, P1-T3/P2-T5.) - [x] AC4: The underlying readiness/initialization gap identified by root-cause research is addressed so that invoking the store-settings command when store state is unavailable produces deterministic, non-crashing behavior rather than an unhandled exception. (Evidence: `EvaluateLaunchReadiness()` in `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, P2-T1/P2-T2/P2-T3.) -- [x] AC5: The full C# toolchain passes in order (csharpier -> .NET analyzers -> nullable/TreatWarningsAsErrors -> MSTest with coverage); coverage on changed lines meets the >= 90% new-code target and repository line coverage remains >= 80% for the testable denominator. (Evidence: `evidence/qa-gates/qa-01-format.md` through `evidence/qa-gates/qa-05-coverage-delta.md`, P3-T1 through P3-T5. Note: the solution-wide nullable gate's raw `EXIT_CODE` is 1 due to a pre-existing, unrelated condition documented in `evidence/qa-gates/qa-03-nullable.md`; the touched files themselves introduce zero new nullable diagnostics.) +- [x] AC5: The full C# toolchain passes in order (csharpier -> .NET analyzers -> nullable/TreatWarningsAsErrors -> MSTest with coverage); coverage on changed lines meets the >= 90% new-code target and repository line coverage remains >= 80% for the testable denominator. (Evidence: `evidence/qa-gates/qa-01-format.md` through `evidence/qa-gates/qa-05-coverage-delta.md`, P3-T1 through P3-T5. Note: the solution-wide nullable gate's raw `EXIT_CODE` is 1 due to a pre-existing, unrelated condition documented in `evidence/qa-gates/qa-03-nullable.md`; the touched files themselves introduce zero new nullable diagnostics. Scope reconciliation (feature-review Finding 4): the ">= 80% repository line coverage" clause is verified for the measured testable denominator of the `UtilitiesCS` assembly (85.88%, unchanged, no regression) and for this change's new/changed lines (100%). A canonical, merged repo-wide C# coverage artifact (`artifacts/csharp/coverage.xml` across all first-party test projects) does not yet exist in-repo; producing it is a pre-existing, maintainer-owned item tracked under `feature/csharp-coverage-uplift` (feature-review Finding 2), not attributable to issue #240's commits.) - [ ] AC6: All required PR CI checks are green against the PR head SHA. ## Next Step diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T13-00.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T13-00.md new file mode 100644 index 000000000..b77819a3b --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T13-00.md @@ -0,0 +1,184 @@ +# Policy Audit — store-wrapper-launch-npe (Issue #240) + +- Timestamp: 2026-07-06T13-00 +- Review type: Re-audit after remediation (cycle 2) +- Feature folder: `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/` +- Work mode: `minor-audit` +- Base branch (resolved): `main` @ `4022fe7c9b07119224ca5aaa880b0a4003ef08db` +- Head: `TaskMaster-wt-2026-07-06-06-35` @ `9e3615b9dd369e66338b4ad333fb7c5371ece0dd` +- Range audited: `4022fe7c9b07119224ca5aaa880b0a4003ef08db..9e3615b9dd369e66338b4ad333fb7c5371ece0dd` (full branch diff, per Scope Invariant) +- Prior cycle artifacts: `policy-audit.2026-07-06T12-15.md`, `code-review.2026-07-06T12-15.md`, `feature-audit.2026-07-06T12-15.md`, `remediation-inputs.2026-07-06T12-15.md` + +## Rejected Scope Narrowing + +No caller instruction in this cycle attempted to narrow the audit to a plan/task/phase subset, mark a language as out of scope, or skip a toolchain/coverage check. The task's reference to "the prior cycle's sole blocking finding" is background context, not an instruction to omit independent verification of other findings; this audit independently re-derived scope from `git diff` and re-evaluated every finding carried in the prior remediation-inputs artifact. No entries to record. + +## Evidence Location Compliance + +`git diff --name-only 4022fe7c9b07119224ca5aaa880b0a4003ef08db..HEAD` was scanned for paths matching `artifacts/baselines/`, `artifacts/qa/`, `artifacts/evidence/`, and `artifacts/coverage/`. No matches (`grep -E "^artifacts/(baselines|qa|evidence|coverage)/"` returned zero lines). All evidence/documentation files added by this branch (baseline, remediation-baseline, regression-testing, qa-gates, issue-updates, other) live under the canonical `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/evidence//` tree. `scripts/dev_tools/validate_evidence_locations.py` referenced by the review contract does not exist in this repository (confirmed again this cycle via `find`); the scan above was performed manually. No `EVIDENCE_LOCATION_OVERRIDE_REJECTED` entries are required. + +## PR-Context Artifact Reliability Note + +`artifacts/pr_context.summary.txt`'s "Changed files overview" reports "Core logic changes: 0 files" and buckets all changed `.cs`/`.csproj` files into "Docs/templates/agents/tooling: 42 files." Independent verification via `git diff --name-status 4022fe7c9b07119224ca5aaa880b0a4003ef08db..HEAD` and `git diff --stat` shows 5 changed `.cs`/`.csproj` files (`StoreWrapperController.cs`, `StoreWrapperController_Tests.cs`, `StoreWrapperController_Tests.ButtonAndPopulate.cs`, `StoreWrapperController_Tests.Launch.cs`, `UtilitiesCS.Test.csproj`) plus 44 documentation/memory files. This audit did not rely on the summary's classification for scope determination; all scope and file lists below are independently derived from `git diff`. This is the same misclassification pattern recorded in the prior cycle (Finding 3) and persists unchanged in this cycle's refreshed PR-context artifacts. + +## Executive Summary + +This is a re-audit of issue #240 following the remediation of the prior cycle's sole Blocking finding: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` exceeded the repository's 500-line limit (781 lines). The remediation commit (`9e3615b9`) split the file into three `partial class` files — `StoreWrapperController_Tests.cs` (181 lines), `StoreWrapperController_Tests.ButtonAndPopulate.cs` (396 lines), and `StoreWrapperController_Tests.Launch.cs` (234 lines) — with zero production-code changes. This audit independently re-verified, rather than accepted on faith, that the split (a) reduces every resulting file to at or under 500 lines, (b) preserves all 39 `[TestMethod]`s with none added/dropped/renamed, (c) makes zero changes to `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, and (d) leaves the toolchain green for the touched files. Independent commands run directly by this review (not merely evidence review) are recorded in Appendix B. Finding 1 (file-size Blocking) is confirmed **RESOLVED**. Three findings carried from the prior cycle remain open and are restated below with fresh verification: the absence of a canonical repo-wide C# coverage artifact (FAIL per the mandatory coverage-verification procedure), the PR-context misclassification (informational/process), and the AC5 check-off-vs-verdict discrepancy (documentation). No new Blocking finding was identified in this cycle. + +## 1. General Unit Test Policy Compliance + +### 1.1 Core Principles (Independence, Isolation, Fast Execution, Determinism, Readability) + +- Independence/Isolation: unchanged from the prior cycle's fix commit; the split commit only relocated code across files without altering test bodies. Each test still constructs its own `Mock`/`Mock`/`StoreWrapperController`; no shared mutable state introduced by the split. **PASS**. +- The two `Launch()` tests (now in `StoreWrapperController_Tests.Launch.cs`) still save/restore the static `MyBox.DialogInvoker` seam in `try`/`finally`. Verified by direct read of the post-split file. **PASS**. +- Fast/Deterministic: independently re-ran the 39 `StoreWrapperController_Tests` methods (`vstest.console.exe ... /TestCaseFilter:"FullyQualifiedName~StoreWrapperController_Tests"`); all 39 passed in 2.08s with no `Thread.Sleep`/`Task.Delay`/wall-clock reads. **PASS**. +- Readability: test names and XML-doc comments are unchanged by the split (verbatim relocation); descriptive names and scenario comments remain intact in each of the three files. **PASS**. +- Arrange-Act-Assert structure: verified unchanged by direct reading of all three post-split files. **PASS**. + +### 1.2 Coverage and Scenarios + +#### 1.2.1 Per-Language Coverage Comparison + +- C#: Baseline: 85.87% line / 86.68% block (`UtilitiesCS.dll`, pre-fix, `evidence/baseline/test-coverage-baseline.md`). Post-change: 85.88% line / 86.69% block (`UtilitiesCS.dll`, post-fix-and-split, `evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md`). Change: +0.01% line, 0.00 percentage points across the split remediation itself (identical before/after the split, since the split touched only test files and zero production code). New/changed-code coverage: 100.00% line / 100.00% block for `EvaluateLaunchReadiness()` and the `StoreLaunchReadiness` factory methods (unchanged by the split; verified in the prior cycle via `TestResults/final-coverage.xml` and re-confirmed this cycle by independently re-running all 39 `StoreWrapperController_Tests` methods, all passing). Disposition: PASS for new/changed-code and no-regression checks; **FAIL for the repo-wide row** — see 1.2.2. Evidence: `evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md`, `evidence/qa-gates/qa-05-coverage-delta.remediation-2026-07-06T12-15.md`, and this cycle's independent `vstest.console.exe` re-run (39/39 passed, Appendix B). +- TypeScript: Baseline: N/A. Post-change: N/A. Change: N/A. Disposition: N/A. Evidence: N/A — no TypeScript files changed on this branch. +- PowerShell: Baseline: N/A. Post-change: N/A. Change: N/A. Disposition: N/A. Evidence: N/A — no PowerShell files changed on this branch. +- Python: Baseline: N/A. Post-change: N/A. Change: N/A. Disposition: N/A. Evidence: N/A — no Python files changed on this branch. + +##### Coverage Evidence Checklist + +- TypeScript baseline coverage artifact: N/A - out of scope (no `.ts`/`.tsx` files changed). +- TypeScript post-change coverage artifact: N/A - out of scope. +- PowerShell baseline coverage artifact: N/A - out of scope (no `.ps1`/`.psm1` files changed). +- PowerShell post-change coverage artifact: N/A - out of scope. + +#### 1.2.2 Repo-Wide Coverage Verdict (Mandatory Coverage Verification) + +Per the review contract's Coverage Verification procedure, a canonical repo-wide coverage artifact (`artifacts/csharp/coverage.xml`) is mandatory whenever C# files changed in the branch diff. This branch changes 4 `.cs` files and 1 `.csproj`, so the check is mandatory. + +- Independently re-confirmed this cycle: **no `artifacts/csharp/coverage.xml` exists** anywhere in the repository (`find . -iname "coverage.xml"` matches only unrelated feature folders' evidence snapshots for other issues, none at the canonical path). Per the mandatory procedure: **FAIL** — "coverage artifact absent for C#; coverage verification is mandatory for all languages with changed files." +- This condition is unchanged from the prior cycle and is not attributable to either cycle's changes to issue #240 (neither the original fix nor the file-size remediation touch anything outside `UtilitiesCS`/`UtilitiesCS.Test`). +- The feature's own evidence continues to label 85.88% as "repository (testable-denominator) line coverage" for `UtilitiesCS.dll` only, not the full C# solution. +- **Disposition: FAIL** (artifact absent). This is a systemic, pre-existing, cross-cutting condition tracked separately from issue #240 (see `feature/csharp-coverage-uplift` per CLAUDE.md UT2); it is carried into this cycle's remediation inputs per the mandatory coverage-trigger rule, not because it blocks issue #240's own merge. + +### 1.3 Scenario Completeness + +Unchanged from the prior cycle (the split is a pure code-relocation with zero behavioral or scenario changes). `EvaluateLaunchReadiness()` scenario coverage (null `Globals`, null `Ol`, null `StoresWrapper`, null `Stores`, happy path) and `Launch()` scenario coverage (both not-ready paths) are unchanged and independently re-verified passing in this cycle's 39/39 test re-run. **PASS**. + +### 1.4 External Dependencies and Environment + +No live Outlook process, no real files, no temporary files, in any of the three post-split files (verified by direct reading). `IApplicationGlobals`/`IOlObjects` remain mocked via Moq; the WinForms dialog interception via `MyBox.DialogInvoker` is unchanged. **PASS**. + +### 1.5 Test File Location + +Tests remain under `UtilitiesCS.Test/OutlookObjects/Store/`, mirroring `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`, consistent with the repo's `.Test` convention. The split added two new files in the same directory following the same mirrored-structure convention. **PASS**. + +### 1.6 File-Size Limit (Test Code) — RESOLVED THIS CYCLE + +Independently verified via `wc -l` (not merely evidence review): + +| File | Lines | <= 500? | +|---|---|---| +| `StoreWrapperController_Tests.cs` | 181 | Yes | +| `StoreWrapperController_Tests.ButtonAndPopulate.cs` | 396 | Yes | +| `StoreWrapperController_Tests.Launch.cs` | 234 | Yes | + +All three resulting files are within the repository's 500-line limit. This resolves the prior cycle's Blocking Finding 1. Independently verified test-method preservation: `git show dfbebb13:UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs | grep -c "\[TestMethod\]"` = 39 (pre-split); `grep -c "\[TestMethod\]"` across the three post-split files = 13 + 19 + 7 = 39 (post-split). Counts match exactly; combined with the executor's own byte-identical sorted-method-name diff (`evidence/qa-gates/split-testmethod-verification.md`), no test was added, dropped, or renamed. Independently verified containment: `git diff dfbebb13dd369e66338b4ad333fb7c5371ece0dd..9e3615b9dd369e66338b4ad333fb7c5371ece0dd -- UtilitiesCS/` produced no output — zero production-file changes in the split commit. **PASS (RESOLVED)**. + +## 2. General Code Change Policy Compliance + +- Design principles: unchanged from the prior cycle for the production fix. The split itself is a pure mechanical file-size remediation with no behavioral change, consistent with simplicity-first and cohesion (each resulting file groups a coherent region: constructor/helpers, button/populate handlers, Launch/readiness). **PASS**. +- Error handling: unchanged (no new error-handling code introduced by the split). **PASS**. +- Naming: the three split file names (`StoreWrapperController_Tests.cs`, `...ButtonAndPopulate.cs`, `...Launch.cs`) are descriptive of their contents. **PASS**. +- Public API impact: no production API changed by the split (zero diff to `StoreWrapperController.cs`). **PASS**. +- File-size limit: production file `StoreWrapperController.cs` remains 396 lines (unchanged, independently re-verified via `wc -l`). Test files: all three now <= 500 lines — see §1.6. **PASS (previously FAIL, now RESOLVED)**. +- Comment quality: unchanged; XML doc comments relocated verbatim with their originating test methods. **PASS**. +- I/O boundary isolation / no temp files: confirmed unchanged, see §1.4. **PASS**. + +## 3. Language-Specific Code Change Policy Compliance (C#) + +- Formatting (CSharpier): independently re-ran `dotnet tool run csharpier check` against all four changed `.cs` files (`StoreWrapperController.cs` and the three split test files) directly in this review session — `Checked 4 files in 803ms.`, `EXIT_CODE 0`. **PASS (independently verified)**. +- .NET Analyzers: independently re-ran a scoped `msbuild UtilitiesCS.Test/UtilitiesCS.Test.csproj /t:Rebuild /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` directly in this review session — `EXIT_CODE 0`, 0 errors, 59 pre-existing warnings (none attributable to the three split files; confirmed the split files' names appear only in the compiler's source-file argument list, never in a `warning`/`error` line). **PASS (independently verified)**. +- Nullable / TreatWarningsAsErrors: independently re-ran a scoped `msbuild UtilitiesCS.Test/UtilitiesCS.Test.csproj /t:Rebuild /p:Nullable=enable /p:TreatWarningsAsErrors=true` directly in this review session — `EXIT_CODE 1`; `grep -oE "\[.*\.csproj\]"` on the build log deduplicates to exactly `SVGControl.csproj` and `UtilitiesSwordfish.NET.General.csproj`, the same two out-of-scope vendored/legacy projects documented in the P0 baseline; zero occurrences of `StoreWrapperController` in the entire build log. **PASS with documented pre-existing exception (independently re-confirmed, not merely evidence review)**. +- Suppression practice: unchanged; the single `#pragma warning disable/restore CS8625` pair in `StoreWrapperController.cs` is untouched by this cycle's split (zero diff to that file). **PASS**. + +## 4. Language-Specific Unit Test Policy Compliance (C#) + +- Framework: MSTest (`[TestClass]`/`[TestMethod]`), unchanged, confirmed by direct inspection of all three post-split files. **PASS**. +- Mocking: Moq, unchanged, confirmed. **PASS**. +- Assertions: FluentAssertions, unchanged, confirmed. **PASS**. +- Toolchain command selection matches CUT3. **PASS**. +- Project file wiring: `UtilitiesCS.Test.csproj` gained exactly two new `` entries for the two new split files; independently confirmed via `git diff` (`+2` lines, no other changes to the `.csproj`). **PASS**. + +## 5. Test Coverage Detail + +| Scope | Metric | Baseline | Post-change (this cycle) | Threshold | Verdict | +|---|---|---|---|---|---| +| New code (`EvaluateLaunchReadiness`, `StoreLaunchReadiness.NotReady/Ready`) | Line | 0.00% (did not exist) | 100.00% | >= 90% (CLAUDE.md) / >= 85% (repo rule) | PASS | +| New code (same scope) | Block/branch | 0.00% (did not exist) | 100.00% | >= 75% | PASS | +| `StoreWrapperController_Tests` methods (39 total) | Pass rate | 39/39 (pre-split, `dfbebb13`) | 39/39 (post-split, independently re-run this cycle) | No regression | PASS | +| Single assembly `UtilitiesCS.dll` (mislabeled "repository" in feature evidence) | Line | 85.87% | 85.88% | Informational only — not the canonical repo-wide gate | Context only | +| Repo-wide, all C# assemblies (canonical `artifacts/csharp/coverage.xml`) | Line | Not measurable — no canonical artifact | Not measurable — no canonical artifact (independently re-confirmed absent this cycle) | >= 80% (CLAUDE.md) / >= 85% (repo rule) | **FAIL** (artifact absent) | + +## 6. Test Execution Metrics + +- Prior-cycle baseline (pre-fix, `evidence/baseline/test-coverage-baseline.md`): 4163 total, 4163 passed, 0 failed. +- Prior-cycle fail-before (`evidence/regression-testing/fail-before-240.md`): 2 total (filtered), 0 passed, 2 failed — reproduces the issue #240 crash. +- Prior-cycle pass-after / remediation post-change (`evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md`): 4170 total, 4170 passed, 0 failed. +- **This cycle's independent re-run** (`vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /TestCaseFilter:"FullyQualifiedName~StoreWrapperController_Tests" /InIsolation`, run directly by this review): Total tests: 39, Passed: 39, Failed: 0, Total time: 2.08s. This independently confirms all `StoreWrapperController_Tests` methods pass identically after the split, without re-running the full 4170-test suite (time-scoped verification; the full-suite run is documented in the remediation-cycle evidence and was not contradicted by this targeted re-run). + +## 7. Code Quality Checks + +| Check | Command / Method | Result | +|---|---|---| +| Confidentiality masking scan | Manual diff review for secrets/credentials in new `.cs`/`.md` files (split commit + memory files) | No secrets or credentials found | +| Suppression scan (added lines) | Manual review of new `#pragma`/`[ExcludeFromCodeCoverage]` usage in the split commit's diff | Zero new suppressions added by the split commit (the single pre-existing `#pragma warning disable/restore CS8625` pair in `StoreWrapperController.cs` is untouched — zero diff to that file) | +| Workflow change scan | `git diff --name-only` filtered for `.github/workflows/**` | No workflow files changed; `.claude/rules/ci-workflows.md` not applicable | +| Benchmark baseline scan | `git diff --name-only` filtered for `scripts/benchmarks/**` | No benchmark files changed; `.claude/rules/benchmark-baselines.md` not applicable | +| Orchestrator-state scan | `git diff --name-only` filtered for `orchestrator-state.json` | No orchestrator-state checkpoint changed; `.claude/rules/orchestrator-state.md` not applicable | + +## 8. Gaps and Exceptions + +1. **Test file 500-line limit** — **RESOLVED THIS CYCLE**. Independently re-verified (see §1.6): all three post-split files are <= 500 lines, all 39 test methods preserved, zero production diff. No longer a finding. +2. **Repo-wide C# coverage artifact absent** — unresolved, unchanged from the prior cycle. No `artifacts/csharp/coverage.xml` exists (independently re-confirmed this cycle). Pre-existing, systemic, not attributable to either of issue #240's two commits. Carried forward as a **tracked, non-blocking-for-#240** systemic item per the mandatory coverage-trigger rule. +3. **PR-context summary misclassification** — unresolved, unchanged. "Core logic changes: 0 files" still omits the changed `.cs`/`.csproj` files in this cycle's refreshed `pr_context.summary.txt`. Process/tooling gap, not a code defect. Carried forward as **informational**. +4. **AC5 check-off vs. review verdict discrepancy** — unresolved, unchanged. `issue.md` AC5 remains checked `[x]`; this review's AC5 verdict remains PARTIAL for the same reason as the prior cycle (repo-wide coverage clause not substantiated at true repo-wide scope). See `feature-audit.2026-07-06T13-00.md`. +5. Nullable gate's solution-wide `EXIT_CODE 1` — pre-existing, independently re-confirmed this cycle to be confined to the same two out-of-scope vendored projects. Accepted, not a gap requiring remediation for this PR. + +## 9. Summary of Changes (this cycle, remediation commit `9e3615b9`) + +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs`: trimmed from 781 to 181 lines (602 deletions, 3 insertions — retains the non-`Launch`/non-`ButtonAndPopulate` regions plus the `partial` keyword). +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs`: new file, 396 lines, 19 test methods. +- `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs`: new file, 234 lines, 7 test methods. +- `UtilitiesCS.Test/UtilitiesCS.Test.csproj`: +2 lines (two new `` entries). +- `UtilitiesCS/OutlookObjects/Store/StoreWrapperController.cs`: zero diff (production code untouched by this cycle). +- 2 new files under `.claude/agent-memory/feature-review/` (1 new memory note, 1 index update) and 23 new/updated documentation/evidence files under `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/` from the prior review/remediation cycle, unrelated to this audit's own scope determination. + +## 10. Compliance Verdict + +**PARTIAL.** The sole Blocking finding from the prior cycle (test-file 500-line limit) is confirmed **RESOLVED** by independent verification in this review — no new Blocking finding was identified. One FAIL-severity, pre-existing, cross-cutting condition remains open per the mandatory coverage-verification procedure: the absence of a canonical repo-wide C# coverage artifact (`artifacts/csharp/coverage.xml`). This condition is not attributable to either of issue #240's two commits and does not block issue #240's own merge readiness, but per the review contract's coverage-trigger rule it is carried forward into a fresh `remediation-inputs.2026-07-06T13-00.md` for completeness, alongside the two lower-severity informational/documentation items (PR-context misclassification; AC5 check-off discrepancy). + +## Appendix A: Test Inventory + +| Test | Type | Target | Result (this cycle's independent re-run) | +|---|---|---|---| +| `Launch_WhenStoresWrapperIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` | Regression | `Launch()` (AC1) | PASS | +| `Launch_WhenStoresListIsNull_ShowsUserMessageAndDoesNotThrowOrOpenViewer` | Regression | `Launch()` (AC2) | PASS | +| `EvaluateLaunchReadiness_WhenGlobalsIsNull_ReturnsModelUnavailable` | Unit (edge) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenOlIsNull_ReturnsModelUnavailable` | Unit (edge) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenStoresWrapperIsNull_ReturnsModelUnavailable` | Unit (negative) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenStoresListIsNull_ReturnsStoresUnavailable` | Unit (negative) | `EvaluateLaunchReadiness()` | PASS | +| `EvaluateLaunchReadiness_WhenModelAndStoresPopulated_ReturnsReadyWithDisplayNames` | Unit (positive) | `EvaluateLaunchReadiness()` | PASS | +| (32 additional pre-existing `StoreWrapperController_Tests` methods, unrelated to issue #240, relocated verbatim by the split) | Unit | Various `StoreWrapperController` members | PASS (all 39/39 in the targeted re-run) | + +## Appendix B: Toolchain Commands Reference + +| Stage | Command | Result | Independently run this cycle? | +|---|---|---|---| +| Format | `dotnet tool run csharpier check <4 changed .cs files>` | `Checked 4 files in 803ms.`, `EXIT_CODE 0` | Yes | +| Analyzers (scoped) | `msbuild UtilitiesCS.Test\UtilitiesCS.Test.csproj /t:Rebuild /p:Configuration=Debug /p:Platform=AnyCPU /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true` | `EXIT_CODE 0`, 0 errors, 59 pre-existing warnings, zero attributable to split files | Yes | +| Nullable (scoped) | `msbuild UtilitiesCS.Test\UtilitiesCS.Test.csproj /t:Rebuild /p:Configuration=Debug /p:Platform=AnyCPU /p:Nullable=enable /p:TreatWarningsAsErrors=true` | `EXIT_CODE 1` — confined to `SVGControl.csproj`/`UtilitiesSwordfish.NET.General.csproj`; zero occurrences of `StoreWrapperController` in the log | Yes | +| Test (targeted) | `vstest.console.exe UtilitiesCS.Test\bin\Debug\UtilitiesCS.Test.dll /TestCaseFilter:"FullyQualifiedName~StoreWrapperController_Tests" /InIsolation` | 39/39 passed, 2.08s | Yes | +| Test + Coverage (full suite) | `vstest.console.exe UtilitiesCS.Test/bin/Debug/UtilitiesCS.Test.dll /EnableCodeCoverage /InIsolation` | `EXIT_CODE 0`, 4170/4170 passed, `UtilitiesCS.dll` line coverage 85.88% | No — relied on remediation-cycle evidence (`evidence/qa-gates/qa-04-test-coverage.remediation-2026-07-06T12-15.md`); not re-run in full this cycle for time cost, cross-checked instead via the targeted 39-test re-run above | +| Line count | `wc -l` on all three post-split files | 181 / 396 / 234 | Yes | +| Test-method count | `grep -c "\[TestMethod\]"` pre-split vs. post-split (sum) | 39 (pre-split) = 39 (13+19+7, post-split) | Yes | +| Containment | `git diff dfbebb13..9e3615b9 -- UtilitiesCS/` | No output (zero diff) | Yes | diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T13-00.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T13-00.md new file mode 100644 index 000000000..794de176e --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/remediation-inputs.2026-07-06T13-00.md @@ -0,0 +1,52 @@ +# Remediation Inputs — store-wrapper-launch-npe (Issue #240), Cycle 2 Re-Audit + +- Timestamp: 2026-07-06T13-00 +- Source artifacts: `policy-audit.2026-07-06T13-00.md`, `code-review.2026-07-06T13-00.md`, `feature-audit.2026-07-06T13-00.md` +- Prior cycle artifacts: `policy-audit.2026-07-06T12-15.md`, `code-review.2026-07-06T12-15.md`, `feature-audit.2026-07-06T12-15.md`, `remediation-inputs.2026-07-06T12-15.md` + +## Finding 1 — RESOLVED (was Blocking): Test file exceeded the 500-line policy limit + +- **Status: RESOLVED. No further action required.** +- Prior state: `UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.cs` was 781 lines. +- Remediation applied (commit `9e3615b9`): split into three `partial class` files — `StoreWrapperController_Tests.cs` (181 lines), `StoreWrapperController_Tests.ButtonAndPopulate.cs` (396 lines), `StoreWrapperController_Tests.Launch.cs` (234 lines) — all <= 500 lines. +- Independent verification performed by this review cycle (not accepted on faith from executor evidence): `wc -l` on all three files; `grep -c "\[TestMethod\]"` summed to 39 pre- and post-split; `git diff dfbebb13..9e3615b9 -- UtilitiesCS/` produced no output (zero production diff); `dotnet tool run csharpier check` on all four changed `.cs` files (`EXIT_CODE 0`); a scoped `msbuild` analyzer rebuild of `UtilitiesCS.Test.csproj` (`EXIT_CODE 0`, zero diagnostics attributable to the split files); a targeted `vstest.console.exe` re-run of all 39 `StoreWrapperController_Tests` methods (39/39 passed). +- Evidence: `policy-audit.2026-07-06T13-00.md` §1.6, §2, §3, Appendix B. +- Owner/next step: none — closed. + +## Finding 2 — Tracked, non-blocking-for-#240: Repo-wide C# coverage artifact absent + +- **Status: remediation-required (systemic tracking item; not blocking for issue #240's own merge)** — unchanged from the prior cycle. +- No canonical `artifacts/csharp/coverage.xml` exists in this repository session (independently re-confirmed this cycle via `find`). Per the mandatory Coverage Verification Procedure, this is a FAIL for the "Repo-wide per language" gate for C# (C# has changed files on this branch). +- This condition pre-dates issue #240 and is not attributable to either of this issue's two commits (the original fix or this cycle's file-size split); issue #240's own new/changed-code coverage remains independently verified at 100%. +- Evidence: `policy-audit.2026-07-06T13-00.md` §1.2.2, §5, §8 item 2. +- Recommended action: unchanged from the prior cycle — track under the repository's existing `feature/csharp-coverage-uplift` initiative; produce a canonical, multi-project coverage merge (`artifacts/csharp/coverage.xml`) covering all first-party C# test projects, scoped to the testable denominator after applying the ratified COM/VSTO/WinForms exclusions. +- Owner/next step: repository maintainer / CI-infrastructure owner, not this issue's executor. This item is carried forward unchanged because it is outside the scope of both of issue #240's commits. + +## Finding 3 — Informational: PR-context summary misclassifies changed C# files + +- **Status: informational (process/tooling gap, not a code defect)** — unchanged from the prior cycle; independently re-confirmed against the refreshed `artifacts/pr_context.summary.txt`/`artifacts/pr_context.appendix.txt` for this cycle. +- `artifacts/pr_context.summary.txt`'s "Changed files overview" still reports "Core logic changes: 0 files" and buckets all changed `.cs`/`.csproj` files into "Docs/templates/agents/tooling: 42 files." +- Evidence: `policy-audit.2026-07-06T13-00.md` "PR-Context Artifact Reliability Note"; direct comparison against `git diff --stat`/`git diff --name-status`. +- Recommended action: unchanged — fix the PR-context summary generator's file-classification logic so `.cs`/`.csproj` core-logic files are not bucketed into "docs/templates/agents/tooling." Reviewers should continue to independently verify scope via `git diff`. +- Owner/next step: owner of the PR-context artifact generation tooling. + +## Finding 4 — Documentation discrepancy: AC5 check-off vs. review verdict + +- **Status: remediation-required (documentation reconciliation)** — unchanged from the prior cycle. +- `issue.md` AC5 remains checked `[x]`, but this cycle's independent verdict for AC5 remains PARTIAL (Finding 2 above is the specific cause). +- Evidence: `feature-audit.2026-07-06T13-00.md` "Acceptance Criteria Check-off" section. +- Recommended action: unchanged — maintainer should either (a) narrow AC5's wording to explicitly scope "repository line coverage" to the `UtilitiesCS` project/testable denominator actually measured, or (b) wait for Finding 2's canonical repo-wide artifact before treating AC5 as fully satisfied. +- Owner/next step: maintainer (Dan Moisan) / issue #240 owner. + +## Remediation Priority Summary + +| Finding | Severity | Blocking for #240 merge? | Status this cycle | +|---|---|---|---| +| 1. Test file 500-line limit | Was Blocking | N/A | **RESOLVED** | +| 2. Repo-wide C# coverage artifact absent | Systemic / FAIL per procedure | No (pre-existing, tracked separately) | Open, unchanged | +| 3. PR-context summary misclassification | Informational | No | Open, unchanged | +| 4. AC5 check-off discrepancy | Documentation | No (but should be reconciled) | Open, unchanged | + +## Note on Handoff Scope + +No new Blocking finding was identified in this cycle. Findings 2-4 are pre-existing, systemic, or informational items that are explicitly out of scope for issue #240's own executor (per Finding 2's and Finding 4's "Owner/next step" fields, both point to the repository maintainer/CI-infrastructure owner, not this issue's plan). This artifact does not, by itself, require opening a new atomic-planner remediation cycle scoped to issue #240; it restates the open, cross-cutting items for maintainer visibility and repository-wide tracking, consistent with the prior cycle's disposition. From 3b92861b2ecdf84ad6cd93aa0900a70615596e9f Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 6 Jul 2026 08:41:10 -0400 Subject: [PATCH 4/5] docs(240): add PR-creation-blocker human-exception runbook - Document the enforce-pr-author-skill.ps1 preflight blocker (missing scripts.dev_tools.validate_orchestration_artifacts module) that denies gh pr create - Provide two resolution options: provision/repoint the hook validator, or create the PR manually - Add human-exception-runbook agent memory on the missing MCP docs tool and the hook/MCP-validator reference Refs: #240 --- .../human-exception-runbook/MEMORY.md | 4 + .../project_no_mcp_docs_tool.md | 23 +++ ...erence_pr_author_hook_and_mcp_validator.md | 27 ++++ .../runbooks/pr-creation-blocker.runbook.md | 153 ++++++++++++++++++ 4 files changed, 207 insertions(+) create mode 100644 .claude/agent-memory/human-exception-runbook/MEMORY.md create mode 100644 .claude/agent-memory/human-exception-runbook/project_no_mcp_docs_tool.md create mode 100644 .claude/agent-memory/human-exception-runbook/reference_pr_author_hook_and_mcp_validator.md create mode 100644 docs/features/active/2026-07-06-store-wrapper-launch-npe-240/runbooks/pr-creation-blocker.runbook.md diff --git a/.claude/agent-memory/human-exception-runbook/MEMORY.md b/.claude/agent-memory/human-exception-runbook/MEMORY.md new file mode 100644 index 000000000..bc6ecf74a --- /dev/null +++ b/.claude/agent-memory/human-exception-runbook/MEMORY.md @@ -0,0 +1,4 @@ +# Memory Index + +- [No MCP docs tool wired](project_no_mcp_docs_tool.md) — MCP-first sourcing is currently aspirational; WebFetch is the sole web-second mechanism (as of 2026-07-06) +- [pr-author hook and MCP validator reference](reference_pr_author_hook_and_mcp_validator.md) — enforce-pr-author-skill.ps1 preflight, missing scripts/dev_tools module, available MCP validator substitute diff --git a/.claude/agent-memory/human-exception-runbook/project_no_mcp_docs_tool.md b/.claude/agent-memory/human-exception-runbook/project_no_mcp_docs_tool.md new file mode 100644 index 000000000..9986c014b --- /dev/null +++ b/.claude/agent-memory/human-exception-runbook/project_no_mcp_docs_tool.md @@ -0,0 +1,23 @@ +--- +name: project-no-mcp-docs-tool +description: Repo currently has no callable MCP documentation-retrieval tool; MCP-first sourcing clause is unmet and WebFetch is the sole web-second mechanism +metadata: + type: project +--- + +As of 2026-07-06, a repo-wide search found no `mcp__*` documentation-retrieval tool wired as a +dependency in TaskMaster. The `human-exception-runbook` skill's sourcing rule is MCP-first, then +web-second (`.claude/skills/human-exception-runbook/SKILL.md`), but the "MCP-first" clause is +currently aspirational: there is no MCP tool that can be queried for third-party UI documentation +(e.g., GitHub web UI, Entra admin center). `WebFetch` is the only available sourcing mechanism for +third-party UI steps until such a tool is added. + +**Why:** This limitation is explicitly documented in the two-axis-model-selection spec's Out of +Scope section and is not something any individual runbook-authoring task should try to resolve. + +**How to apply:** When authoring a human-exception runbook that includes a third-party UI step, +note in the Source and Citation section that MCP-first sourcing could not be satisfied for this +reason, then cite a current `WebFetch`-retrieved vendor documentation page as the web-second source +with a dated capture. Do not treat the missing MCP tool as a defect to fix within the runbook task +itself. Re-check whether an MCP docs tool has been added before repeating this note in future +sessions — this is a snapshot of repo state as of 2026-07-06, not a permanent constraint. diff --git a/.claude/agent-memory/human-exception-runbook/reference_pr_author_hook_and_mcp_validator.md b/.claude/agent-memory/human-exception-runbook/reference_pr_author_hook_and_mcp_validator.md new file mode 100644 index 000000000..982d503b8 --- /dev/null +++ b/.claude/agent-memory/human-exception-runbook/reference_pr_author_hook_and_mcp_validator.md @@ -0,0 +1,27 @@ +--- +name: reference-pr-author-hook-and-mcp-validator +description: Where PR-creation gating lives (enforce-pr-author-skill.ps1) and the MCP validator that can substitute for the missing scripts/dev_tools python module +metadata: + type: reference +--- + +The PreToolUse hook `.claude/hooks/enforce-pr-author-skill.ps1` gates every `gh pr create`/`gh pr +edit` call. Its `Invoke-OrchestratorStatePreflight` function (around lines 49-88) defaults to +invoking `python -m scripts.dev_tools.validate_orchestration_artifacts orchestrator-state +--require-pr-creation-ready`. As of 2026-07-06, `scripts/dev_tools` does not exist as a Python +package in this repo (`ModuleNotFoundError`), so this preflight fails closed on every attempt, +producing `ORCHESTRATOR_STATE_PREFLIGHT_FAILED` even for well-formed `gh pr create --body-file +artifacts/pr_body_.md` commands. + +`.claude/settings.json` (line ~23) already registers `mcp__drm-copilot__validate_orchestration_artifacts` +as an allowed MCP tool, which performs equivalent checkpoint validation and is a plausible +replacement `$Invoker` target for the hook, instead of writing a new Python module from scratch. + +**Related:** [[project-no-mcp-docs-tool]] — a separate, unrelated MCP-tooling gap (documentation +retrieval, not orchestrator-state validation). + +Verify this is still accurate before reuse: confirm `scripts/dev_tools/validate_orchestration_artifacts` +still doesn't exist (glob it) and that the MCP tool name in `.claude/settings.json` hasn't changed, +since this was true as of the issue #240 PR-creation-blocker runbook (2026-07-06) and may have been +fixed since (see Option 1 of +`docs/features/active/2026-07-06-store-wrapper-launch-npe-240/runbooks/pr-creation-blocker.runbook.md`). diff --git a/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/runbooks/pr-creation-blocker.runbook.md b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/runbooks/pr-creation-blocker.runbook.md new file mode 100644 index 000000000..401b69110 --- /dev/null +++ b/docs/features/active/2026-07-06-store-wrapper-launch-npe-240/runbooks/pr-creation-blocker.runbook.md @@ -0,0 +1,153 @@ +# Human-Exception Runbook — PR Creation Blocked by Orchestrator-State Preflight (Issue #240) + +This runbook is the human follow-up for the `exception` response recorded against the "create the +GitHub pull request for issue #240" requirement. It is contract-conformant per +`.claude/skills/human-exception-runbook/SKILL.md` (Cue, Prerequisites, Step-by-step Instructions, +Verification, Source and Citation). + +## Cue + +Act on this runbook when the orchestrator has recorded an `exception` response for the "create PR" +requirement on issue #240, or when a subsequent `gh pr create --body-file artifacts/pr_body_240.md +--base main` attempt is denied with `permissionDecision: deny` and reason +`ORCHESTRATOR_STATE_PREFLIGHT_FAILED`. + +Verified root cause: the registered PreToolUse hook +`.claude/hooks/enforce-pr-author-skill.ps1` runs `Invoke-OrchestratorStatePreflight`, whose default +`$Invoker` script block calls: + +``` +python -m scripts.dev_tools.validate_orchestration_artifacts orchestrator-state + artifacts/orchestration/orchestrator-state.json --require-pr-creation-ready +``` + +The Python package `scripts/dev_tools` does not exist in this repository (`ModuleNotFoundError: No +module named 'scripts.dev_tools'`), so the invoker exits non-zero on every invocation regardless of +checkpoint content. `Invoke-OrchestratorStatePreflight` treats any non-zero exit as +`HasErrors = $true`, and `Get-PrAuthorBypassReason` (`.claude/hooks/enforce-pr-author-skill.ps1`, +lines 359-371) returns `ORCHESTRATOR_STATE_PREFLIGHT_FAILED` for every `--body-file` command, +including a well-formed one. This was confirmed by directly simulating the hook against +`gh pr create --body-file artifacts/pr_body_240.md --base main`. Every other precondition for PR +creation (code committed, tests passing, audit artifacts written, branch pushed) is otherwise +satisfied; only the final `gh pr create` step is blocked. Direct `gh api` calls or an inline +`--body` argument are prohibited paths (Case A/B in the same hook) because they bypass the mandatory +`pr-author` skill and its SHA-256 receipt binding. + +## Prerequisites + +- Read/write access to the `drmoisan/TaskMaster` GitHub repository, sufficient to open a pull + request from branch `TaskMaster-wt-2026-07-06-06-35`. +- Confirmation that the branch `TaskMaster-wt-2026-07-06-06-35` is pushed to `origin` and contains + the committed fix for issue #240 (already verified in this case). +- Read access to the feature folder + `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/`, specifically: + - `issue.md` (acceptance criteria; AC6 — "All required PR CI checks are green against the PR head + SHA" — remains open pending PR creation and CI). + - `code-review.2026-07-06T13-00.md`, `feature-audit.2026-07-06T13-00.md`, + `policy-audit.2026-07-06T13-00.md` (most recent audit artifacts). +- For Option 1 only: repository-maintainer or CI-infrastructure authority to add a Python module or + modify `.claude/hooks/enforce-pr-author-skill.ps1`, since this is out-of-scope, maintainer-owned + infrastructure work rather than issue #240 application code. +- For Option 2 only: no additional tooling access is required beyond standard GitHub web access; the + human bypasses the local hook entirely by using the GitHub web UI directly rather than the `gh` + CLI. + +## Step-by-step Instructions + +Two independent resolution paths are documented. Option 2 creates the PR immediately without +changing repository infrastructure. Option 1 additionally restores autonomous PR creation for future +issues and is maintainer/CI-infrastructure work outside issue #240's scope; it is not required to +merge issue #240's fix. + +### Option 1 — Unblock automation (maintainer/CI-infrastructure scope, outside issue #240) + +1. Confirm the gap: run `python -m scripts.dev_tools.validate_orchestration_artifacts + orchestrator-state artifacts/orchestration/orchestrator-state.json --require-pr-creation-ready` + from the repository root and confirm it fails with `ModuleNotFoundError: No module named + 'scripts.dev_tools'`. +2. Choose one of two remediations for `.claude/hooks/enforce-pr-author-skill.ps1` + (`Invoke-OrchestratorStatePreflight`, default `$Invoker` parameter, lines 70-78): + - **2a. Provision the missing module.** Add a `scripts/dev_tools/validate_orchestration_artifacts` + Python module that implements the `orchestrator-state --require-pr-creation-ready` + subcommand contract already assumed by the hook (exit 0 on a checkpoint whose + `next_step`/`blocked_reason` indicate PR-creation readiness per steps 5-8 of the orchestrator + state machine; non-zero otherwise). This is new infrastructure code, not part of issue #240's + fix, and must go through its own change-plan, toolchain, and review. + - **2b. Repoint the invoker to the available MCP validator.** The MCP tool + `mcp__drm-copilot__validate_orchestration_artifacts` is already registered in + `.claude/settings.json` and performs the equivalent checkpoint validation. Modify the default + `$Invoker` script block (or add an MCP-backed override) in + `.claude/hooks/enforce-pr-author-skill.ps1` so the preflight calls this MCP tool instead of the + nonexistent Python module, preserving the same `HasErrors`/`ErrorText` contract consumed by + `Get-PrAuthorBypassReason`. +3. Apply the repository's PowerShell toolchain to any hook change (format, analyze, Pester test) per + `.claude/rules/powershell.md`, and add/adjust Pester coverage for + `Invoke-OrchestratorStatePreflight` to exercise the corrected invoker. +4. Re-run the standard `pr-author` flow: `mcp__drm-copilot__collect_pr_context`, then the `pr-author` + skill to produce `artifacts/pr_body_240.md` and its sibling receipt, then + `gh pr create --body-file artifacts/pr_body_240.md --base main` from branch + `TaskMaster-wt-2026-07-06-06-35`, and confirm the hook now returns `permissionDecision: allow`. + +### Option 2 — Create the PR now, manually (recommended to unblock issue #240 immediately) + +1. Open the repository `drmoisan/TaskMaster` in a browser and confirm the pushed branch + `TaskMaster-wt-2026-07-06-06-35` is visible in the branch list (GitHub shows a yellow "Compare & + pull request" banner for a recently pushed branch with no open PR). +2. Select "Compare & pull request" for `TaskMaster-wt-2026-07-06-06-35`, or navigate to + `https://github.com/drmoisan/TaskMaster/compare/main...TaskMaster-wt-2026-07-06-06-35` directly. +3. In the branch-selection dropdowns, set the base branch to `main` and confirm the compare + (head) branch is `TaskMaster-wt-2026-07-06-06-35`. +4. Enter the PR title and description. Use the feature folder + `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/` as the source for the + description content: + - Summary and root cause: `issue.md` (Summary, Suspected Cause / Notes, Acceptance Criteria + sections). + - Verification/audit evidence: `code-review.2026-07-06T13-00.md`, + `feature-audit.2026-07-06T13-00.md`, `policy-audit.2026-07-06T13-00.md`. + - Note explicitly in the description that AC6 ("All required PR CI checks are green against the + PR head SHA") is verified only after CI runs against this PR's head SHA, not before. +5. Select "Create Pull Request" (not "Create Draft Pull Request", unless a draft is otherwise + required by team convention). +6. Record the resulting PR number and URL in the feature folder's evidence trail (for example a new + `evidence/other/pr-created.md` entry) so the checkpoint's `human_interaction` record can reference + the completed exception. + +## Verification + +- The pull request exists in `drmoisan/TaskMaster` with base branch `main` and head branch + `TaskMaster-wt-2026-07-06-06-35`. Confirm via the PR's "Files changed" / "Commits" tabs, or with + `gh pr view --json baseRefName,headRefName` showing `"baseRefName": "main"` and + `"headRefName": "TaskMaster-wt-2026-07-06-06-35"`. +- Required CI checks begin running against the PR head SHA (visible in the PR's "Checks" tab). + Acceptance criterion AC6 in `issue.md` ("All required PR CI checks are green against the PR head + SHA") is satisfied only once those checks complete and pass; this runbook creates the PR and + triggers CI but does not itself satisfy AC6. +- If Option 1 was also completed: re-attempt `gh pr create --body-file artifacts/pr_body_.md + --base main` (or `gh pr edit`) for a subsequent issue and confirm the hook returns + `permissionDecision: allow` rather than `ORCHESTRATOR_STATE_PREFLIGHT_FAILED`. + +## Source and Citation + +- Non-UI root-cause citation (repository source, primary): `.claude/hooks/enforce-pr-author-skill.ps1`, + `Invoke-OrchestratorStatePreflight` (lines 49-88) and `Get-PrAuthorBypassReason` (lines 293-382). + Captured/read: 2026-07-06. +- Non-UI citation for the available MCP alternative: `.claude/settings.json`, line 23 + (`mcp__drm-copilot__validate_orchestration_artifacts` registered as an allowed MCP tool). Captured/ + read: 2026-07-06. +- Non-UI citation for the `pr-author` skill contract that Option 1 step 4 and Option 2 must not + bypass: `.claude/skills/pr-author/SKILL.md`. Captured/read: 2026-07-06. +- Sourcing-order note: per the two-axis-model-selection spec's Out of Scope section, no callable MCP + documentation-retrieval tool is currently wired in this repository, so the skill's "MCP-first" + clause for the third-party UI step below could not be satisfied with an MCP source; `WebFetch` was + used as the sole available web-second mechanism. This is a repository-wide limitation, not specific + to this runbook, and is not resolved by this agent. +- Third-party UI step source (Option 2, web-second, MCP unavailable per the note above): GitHub Docs + — "Creating a pull request" (Compare & pull request button; base/head branch selection). Source + URL: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request + — updated_at (capture date): 2026-07-06. +- Feature-folder content sources for the PR description (Option 2, step 4): + `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/issue.md`, + `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/code-review.2026-07-06T13-00.md`, + `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/feature-audit.2026-07-06T13-00.md`, + `docs/features/active/2026-07-06-store-wrapper-launch-npe-240/policy-audit.2026-07-06T13-00.md`. + Captured/read: 2026-07-06. From c0127a03e471bc1ba04d9db2a92ff3131b13e1a7 Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Mon, 6 Jul 2026 08:41:53 -0400 Subject: [PATCH 5/5] chore(memory): record orchestrator PR-creation hook blocker for this repo Refs: #240 --- .claude/agent-memory/orchestrator/MEMORY.md | 1 + .../pr-author-hook-blocks-gh-in-this-repo.md | 14 ++++++++++++++ 2 files changed, 15 insertions(+) create mode 100644 .claude/agent-memory/orchestrator/pr-author-hook-blocks-gh-in-this-repo.md diff --git a/.claude/agent-memory/orchestrator/MEMORY.md b/.claude/agent-memory/orchestrator/MEMORY.md index e0b0fd534..0b202bb45 100644 --- a/.claude/agent-memory/orchestrator/MEMORY.md +++ b/.claude/agent-memory/orchestrator/MEMORY.md @@ -21,3 +21,4 @@ - [Banned API in touched file is in scope](feedback_banned_api_in_touched_file_in_scope.md) — when a fix modifies a production file, remediate any banned API (DateTime.Now/UtcNow, Random.Shared, Thread.Sleep, Task.Delay) found in it; don't defer - [Re-verify ground truth after user mid-cycle commit](feedback_reverify_ground_truth_after_user_midcycle_commit.md) — if the user rebases/commits mid-cycle, re-probe line counts/merge-base/csproj and re-plan before executing a preflighted plan - [pr_context.summary.txt unreliable for gh + file classification](pr-context-summary-unreliable-gh-and-classification.md) — verify gh with `which gh`/`gh auth status` and author the PR body from the real diff, not the summary's "gh unavailable"/"0 core-logic changes" lines +- [pr-author hook blocks gh pr create in this repo](pr-author-hook-blocks-gh-in-this-repo.md) — enforce-pr-author-skill.ps1 denies every gh pr create because its python validator module is absent; resolve via exception+runbook, don't burn a pr-author delegation diff --git a/.claude/agent-memory/orchestrator/pr-author-hook-blocks-gh-in-this-repo.md b/.claude/agent-memory/orchestrator/pr-author-hook-blocks-gh-in-this-repo.md new file mode 100644 index 000000000..437c688d5 --- /dev/null +++ b/.claude/agent-memory/orchestrator/pr-author-hook-blocks-gh-in-this-repo.md @@ -0,0 +1,14 @@ +--- +name: pr-author-hook-blocks-gh-in-this-repo +description: In this repo the enforce-pr-author-skill PreToolUse hook denies every gh pr create because its required python validator module is absent; autonomous PR creation is not possible here +metadata: + type: project +--- + +The registered PreToolUse hook `.claude/hooks/enforce-pr-author-skill.ps1` runs a PR-creation preflight whose default `$Invoker` is `python -m scripts.dev_tools.validate_orchestration_artifacts orchestrator-state --require-pr-creation-ready`. That python package **does not exist** in this repo (`ModuleNotFoundError: No module named 'scripts.dev_tools'`), so the invoker exits 1, `Invoke-OrchestratorStatePreflight` returns `HasErrors=true`, and the hook **denies every** `gh pr create --body-file ...` with `ORCHESTRATOR_STATE_PREFLIGHT_FAILED`. Verified 2026-07-06 by simulating the hook against a well-formed command (pwsh present, hook active). + +**Why:** The `.claude` governance bundle was pushed down from a reference repo that has the python `scripts/dev_tools/` tooling; this target repo (TaskMaster) does not. The authoritative validator here is the drm-copilot MCP tool `validate_orchestration_artifacts`, which the hook does not call. There is also no sanctioned autonomous workaround: `gh api` / inline `--body` bypass the mandatory pr-author skill and are prohibited. + +**How to apply:** Detect this before the PR gate (run the hook's exact invoker or simulate the hook). When it blocks, resolve via the autonomous-mandate `exception` response: delegate `Agent(human-exception-runbook)` to write a `/runbooks/*.runbook.md` giving the maintainer two options — (1) provision the missing module or repoint the hook `$Invoker` to the MCP validator, or (2) create the PR manually (base `main`, head the pushed branch) from the feature-folder audit artifacts. Record it in `human_interaction.requirements[]` as `{response: "exception", runbook_path}`; an exception with an existing runbook is DONE-compatible (only `halt` blocks DONE). Do not burn a `pr-author` delegation to hit the wall — the block is deterministic. + +**Also note (checkpoint validator):** the MCP `validate_orchestration_artifacts` `orchestrator-state` mode is stricter/legacy (see [[orchestrator-state-validator-divergence]]); it demands `relativeFile`, `long-name`, `work-mode`, `plan-path`, `step7/8/10_status`, and step-status enum `{not-applicable, pending, delegated, verified, blocked}` (not `complete`). Conform to the canonical shape + the real SubagentStop hook rather than chasing that advisory tool.