diff --git a/.claude/agent-memory/atomic-executor/MEMORY.md b/.claude/agent-memory/atomic-executor/MEMORY.md index 81524fe7..109d6c71 100644 --- a/.claude/agent-memory/atomic-executor/MEMORY.md +++ b/.claude/agent-memory/atomic-executor/MEMORY.md @@ -3,24 +3,24 @@ ## Plan validation & gates - [Verify line citations with numbered output](feedback_verify_line_citations_with_numbered_output.md) — never hand-count from a `sed` window; a wrong #438 advisory got applied and corrupted 3 correct citations in plan + spec - [Self-derived gate thresholds are blind](project_preflight_selfderived_gate_thresholds_are_blind.md) — a "count >= floor" gate whose floor comes from the runs it validates is deflation-blind + scope-incommensurable; use git-enumeration + `/ListTests` existence proofs -- [Merge-base diff gates need a commit cadence](project_preflight_mergebase_diff_gates_need_commit_cadence.md) — `..HEAD` gates are vacuous while HEAD == merge-base and unsatisfiable once HEAD is ahead; on a later cycle scope-audit via `git show --numstat --format= HEAD` +- [Merge-base diff gates need a commit cadence](project_preflight_mergebase_diff_gates_need_commit_cadence.md) — `..HEAD` gates are vacuous while HEAD == merge-base; plan an explicit commit task - [Inserted plan tasks force renumbering](project_plan_task_ids_digit_only_forces_renumbering.md) — suffixed IDs (`P3-T5a`) fail validation; say "insert + renumber downstream", then verify defs-vs-mentions mechanically - [Plan rationale clauses are evidence](project_418_plan_rationale_clauses_are_evidence.md) — #418 needed 3 preflight passes; all blockers were unmeasured world-state claims in prose, never in the fix - [Tracked agent-memory breaks unscoped git gates](project_agent_memory_tracked_breaks_unscoped_git_gates.md) — `.claude/agent-memory/**` is tracked + dirty at branch head; every git diff/status/grep gate needs an explicit pathspec or it is unsatisfiable / false-positive - [#418 500-line gate vs mandated plan content](project_418_500line_gate_vs_plan_content.md) — P1-T19 unsatisfiable (193 new lines into 146 headroom); per-block logging clauses block centralizing; delta = extract helpers to a new file -- [#376 capstone scope-expansion layers](project_376_capstone_scope_expansion_layers.md) — 5 escalated layers past P2-T17 resolved via the 3 authorized patterns; stop-condition never triggered -- [#207 Hook() redesign breaks AppEventsTests](project_207_hook_redesign_breaks_appeventstests.md) — readiness-gate Hook() fails an out-of-scope test asserting superseded ordering; needs a plan revision, not a test weakening -- [ApplicationGlobalsTests.cs at 500-line ceiling](project_appglobalstests_at_500_line_ceiling.md) — file is exactly 500 lines; any plan adding a Testable* override must extract first -- [Swordfish F5 test misclassification](project_swordfish_f5_test_misclassification.md) — #308 mislabeled two clean-base tests "direct-Swordfish"; verify using/namespace before treating a removal as Swordfish-only +- [#207 Hook() redesign breaks AppEventsTests](project_207_hook_redesign_breaks_appeventstests.md) — needs a plan revision, not a test weakening +- [ApplicationGlobalsTests.cs at 500-line ceiling](project_appglobalstests_at_500_line_ceiling.md) — exactly 500 lines; adding a Testable* override requires extraction first +- [#376 capstone scope-expansion layers](project_376_capstone_scope_expansion_layers.md) — 5 escalated layers resolved via the 3 authorized patterns +- [Swordfish F5 test misclassification](project_swordfish_f5_test_misclassification.md) — verify using/namespace before treating a removal as Swordfish-only ## Build / toolchain environment -- [Project Build/Test Env](project_build_test_env.md) — git-bash quirks: MSBuild dash-switches, MSYS_NO_PATHCONV, csharpier v1 syntax, forced-nullable Rebuild + Debug-restore, legacy csproj Compile includes, IVT for Moq, C# 7.3 in QuickFiler.Test +- [Project Build/Test Env](project_build_test_env.md) — git-bash quirks (MSBuild switches, MSYS_NO_PATHCONV), csharpier v1 syntax, legacy csproj Compile includes, IVT for Moq - [VS18 build/test toolchain paths](project_vs18_build_toolchain_paths.md) — use VS **18** full-framework msbuild.exe (not .dotnet-sdk, dies on binary resx MSB3822); nuget.exe restore; dotnet-coverage needs `--` separator - [Repo-local SDK install + nullable Rebuild](project_repo_sdk_and_nullable_rebuild.md) — .dotnet-sdk install needs pwsh7; csharpier check/format subcommands; nullable debt scope NOT stable across sessions — re-verify which csproj errors come from -- [vstest TestCaseFilter OR-vs-pipe + fresh-worktree bootstrap](project_vstest_testcasefilter_or_operator_and_env_setup.md) — vstest rejects `OR`, needs `|`; fresh worktree needs Install-RepoDotNetSdk.ps1 + `dotnet tool restore` + global `dotnet-coverage` + Invoke-Restore.ps1 +- [vstest TestCaseFilter OR-vs-pipe + fresh-worktree bootstrap](project_vstest_testcasefilter_or_operator_and_env_setup.md) — vstest rejects `OR`, needs `|`; fresh worktree needs restore + global `dotnet-coverage` - [Analyzer version skew on fresh worktree](project_analyzer_version_skew_fresh_worktree.md) — analyzer build can fail CS0006 when csproj `` HintPaths diverge from packages.config; compare the two before blaming restore - [SecurityCodeScan incompatible with Roslyn 5.6](project_securitycodescan_roslyn56_incompat.md) — SecurityCodeScan.VS2019 5.6.7 throws CS8032/YamlDotNet under VS18, breaking TWAE; Meziantou/Roslynator need roslyn-version subfolders -- [Missing VSTO runtime breaks baseline gates](project_missing_vsto_runtime_breaks_baseline_gates.md) — absent Office Tools v4.0.Utilities => 4x CS0234 in ThisAddIn.Designer.cs; TaskMaster.Test/UtilitiesCS.Test never build so repo coverage reads ~25% not ~71% +- [Missing VSTO runtime breaks baseline gates](project_missing_vsto_runtime_breaks_baseline_gates.md) — HISTORICAL, not reproducing 2026-08-08; build before citing - [New sln member surfaces MSB3277](project_new_sln_member_surfaces_msb3277_pin_divergence.md) — wiring an unbuilt legacy test project into the sln emits a new MSB3277 when packages.config pins diverge; fix in packages.config/.csproj, never a .cs file - [Legacy csproj: no transitive compile refs](project_legacy_csproj_no_transitive_compile_refs.md) — non-SDK ProjectReference doesn't flow package types to csc (CS0012 despite copy-local DLL); tests need their own `` + packages.config entry - [sln/csproj edits: preserve CRLF](project_sln_csproj_edit_crlf_preserve.md) — git-bash `sed -i` strips CRLF from TaskMaster.sln (churn + BOM loss); use Edit or `perl -0777` w/ explicit `\r\n` @@ -36,12 +36,12 @@ - [Compile-time red needs body-level refs](project_compile_red_needs_body_level_references.md) — a missing type in a method SIGNATURE suppresses body binding, so an `[expect-fail]` task requiring N named CS0246s reports only 1; construct the types inline in test bodies ## Test execution & isolation -- [Tests must mock GUI; no visible window](feedback_tests_must_mock_gui_no_visible_window.md) — user saw a window pop up during a QuickFiler.Test run; new tests must use headless seams (Mock, injected show/focus delegates, host-neutral types), never Form.Show/Application.Run +- [Tests must mock GUI; no visible window](feedback_tests_must_mock_gui_no_visible_window.md) — use headless seams (mocked viewers, injected show/focus delegates), never Form.Show/Application.Run - [WinFormsPumpHost tests are load-flaky](project_winformspumphost_tests_load_flaky.md) — QfcItemController_InitializationTests fail with "window handle has been created"/60s timeouts when the box is CPU-saturated; re-run when load drops, don't treat as a red baseline - [vstest /InIsolation + FilePathHelper serialization](project_vstest_isolation_and_filepathhelper_serialization.md) — Moq assemblies need /InIsolation (else STTE Setup FileNotFound); FilePathHelper.FilePath is "" default but null after JSON deserialize - [Invoke-MSTest.ps1 dies on a single test assembly](project_418_invoke_mstest_single_assembly_bug.md) — StrictMode + `.Count` on a scalar String throws before vstest runs; call vstest.console.exe directly with the script's arg list - [Timed-out MSTest leaves detached runner](project_timedout_mstest_leaves_detached_runner.md) — leaves a pwsh runner respawning testhosts → user.config hangs; kill the pwsh runner too, verify 0, rerun >=8min -- [Sibling-worktree shared-tooling hazard](project_sibling_worktree_shared_tooling_hazard.md) — a concurrent agent in another worktree crashes your testhost + clobbers /tmp logs; use session scratchpad, trust bash `$?`, never touch others' processes +- [Sibling-worktree shared-tooling hazard](project_sibling_worktree_shared_tooling_hazard.md) — a concurrent agent elsewhere crashes your testhost + clobbers /tmp logs; use the session scratchpad - [Concurrent executor in same worktree](project_concurrent_executor_same_worktree.md) — two executors on one worktree corrupt shared files; detect via mtime progression during your own turn; STOP, don't stash/race - [UtilitiesCS.Test parallelism flakiness](project_utilitiescs_test_parallelism_flakiness.md) — timing tests time out (~22s) under default parallelism + coverage; lower MSTest Workers to 4 via /Settings for a deterministic gate - [MSTest [DoNotParallelize] overlaps the parallel bucket](project_mstest_donotparallelize_overlaps_parallel_bucket.md) — a [DoNotParallelize] null-baseline reader still sees parallel-bucket writers; mark every writer too @@ -55,40 +55,32 @@ - [Coverage delta: reproduce the baseline's counting method](project_coverage_delta_reproduce_baseline_counting_method.md) — Cobertura repeats lines under `` AND class ``; deduped vs all-descendant give ~2x denominators → false escalation - [First-party coverage denominator method (#197)](project_coverage_firstparty_denominator_method.md) — production-only rate = per-`` count across ALL deduped packages INCLUDING vendored Swordfish/SVGControl; reproduces 71.73% - [dotnet-coverage denominator nondeterminism](project_dotnet_coverage_denominator_nondeterminism.md) — repo line-rate swings (47% vs 81%) from double-counted denominator; re-baseline via git-stash, trust per-class rates -- [C# canonical coverage artifact conversion](project_csharp_canonical_coverage_artifact_conversion.md) — hook reads artifacts/csharp/coverage.xml as JaCoCo; convert feature Cobertura; first-party aggregate under-counts from uninstrumented assemblies — defer repo-wide to PR CI +- [Koverage Cobertura post-processing shape](project_koverage_cobertura_postprocessing_shape.md) — Invoke-MSTestWithCoverage rewrites filenames with `\`, pre-merges per-file `` nodes, strips test packages, recomputes root attrs; forward-slash queries match nothing +- [C# canonical coverage artifact conversion](project_csharp_canonical_coverage_artifact_conversion.md) — hook reads artifacts/csharp/coverage.xml as JaCoCo (85% floor); defer repo-wide to PR CI - [Cobertura runsettings `` override](project_cobertura_runsettings_attributes_override.md) — a custom `` block replaces the default `` excludes, silently disabling [ExcludeFromCodeCoverage]; re-add it - [Changed-line coverage: Cobertura hits vs MS-coverage partial](project_changed_line_coverage_cobertura_vs_mscoverage_partial.md) — null-guard throws read "partially covered" in MS.CodeCoverage XML but hits=1 in Cobertura; use Cobertura for >=90% proofs - [QFC #227 coverage tooling](project_qfc227_coverage_tooling.md) — vstest + Cobertura runsettings (Format under Configuration + attribute-exclude) is the reliable per-class path; .coverage not offline-convertible here - [#398 test-split gate gotchas](project_398_test_split_gate_gotchas.md) — pre-existing CS2002 duplicate Compile (latent, out of scope); /EnableCodeCoverage has no branch% + .coverage merges to empty cobertura → use Cobertura-runsettings variant - [ExcludeFromCodeCoverage on partial class = CS0579](project_excludefromcodecoverage_partial_class_cs0579.md) — annotate a partial type ONCE, not both parts, or the build breaks with duplicate-attribute CS0579 -- [#400 CompleteOpenAsync unreachable recovery catch](project_400_completeopenasync_unreachable_recovery_catch.md) — d__16 153-156 is dead code (dispatcher.Report swallows sink exceptions), can't reach >=90%; escalated; 17-class gate deadlocks testhost, retry after clearing runners -- [Swordfish-removal epic: incidental vendored-coverage side effect](project_swordfish_removal_epic_incidental_coverage_sideeffect.md) — deleting a ScoXxx wrapper drops incidental UtilitiesSwordfish coverage; non-blocking, expect it on every F-slice -- [TaskVisualization #298 ScoCollection + live-bridge exemptions](project_taskvis_scocollection_and_livebridge_exemptions.md) — ScoCollection forces a Swordfish ProjectReference on test assemblies; a default-factory live-form bridge must be method-level exempt +- Closed one-offs (low reuse): [#400 dead recovery catch](project_400_completeopenasync_unreachable_recovery_catch.md), [Swordfish vendored-coverage side effect](project_swordfish_removal_epic_incidental_coverage_sideeffect.md), [#298 ScoCollection/live-bridge exemptions](project_taskvis_scocollection_and_livebridge_exemptions.md), [#328 OlObjectsProxy](project_328_rebuild_threading_olobjectsproxy_conflict.md) ## Nullable / C# language -- [Nullable per-file pragma gate mechanics](project_nullable_pragma_gate_mechanics.md) — HISTORICAL: solution-wide TWAE once aborted on vendored SVGControl CS0649 + UtilitiesCS CS0618/CS0168, needing an isolated `UtilitiesCS.csproj -t:Rebuild -p:BuildProjectReferences=false` + grep CS86xx. Re-verified 2026-08-08 (#507) with CI's own command (`/t:Rebuild`, TWAE, NO `/p:Nullable=enable`): genuinely EXIT 0 / 0 errors, so the enforced gate really does pass. The 195 UtilitiesCS + 219 TaskMaster error population appears only when `/p:Nullable=enable` is added, which no gate does — see [[project-507-nullconditional-return-triggers-cs8603-under-genuine-nullable-check]]. Still use `/t:Rebuild` (not `/t:Build`) so the pass is not vacuous. Supersedes the #364, epic, and net481-mechanics variants. +- [Nullable per-file pragma gate mechanics](project_nullable_pragma_gate_mechanics.md) — CI's command (`/t:Rebuild`, TWAE, NO `/p:Nullable=enable`) genuinely passes EXIT 0; the 195+219 error population appears only under the forced flag. Use `/t:Rebuild` so the pass is not vacuous - [CLAUDE.md nullable command != the CI gate](project_507_nullconditional_return_triggers_cs8603_under_genuine_nullable_check.md) — ci.yml omits `/p:Nullable=enable` and relies on per-file `#nullable` pragmas; forced-flag CS86xx in an unannotated file is NOT a blocker. Repro the CI command before failing an AC - [Nullable remediation annotation patterns](project_nullable_remediation_annotation_patterns.md) — net481 has no post-condition attrs; struct `= default!`; `.ToString()!` for string cells; IsNullOrEmpty overload gotcha; `x!.M()` for defensive flow-state -- [Nullable annotation CS8632 scoping](project_nullable_annotation_cs8632_scoping.md) — `Type?` in nullable-disabled projects emits CS8632; wrap in `#nullable enable annotations`/`restore annotations`, not whole-file -- [init/record struct fails CS0518 on net48](project_record_struct_isexternalinit_netfx.md) — ANY init accessor needs IsExternalInit (absent on net48, no polyfill); use a constructor-initialized readonly struct with get-only props -- [CS8714 does not fire on net481](project_nullable_cs8714_not_on_net481.md) — ConcurrentDictionary-derived unconstrained-TKey types emit 0 CS8714 (net481 BCL lacks notnull); `where TKey : notnull` is forward-looking -- [#366 notnull cascades past WrapperScoDictionary](project_366_notnull_cascades_beyond_wrapperscodictionary.md) — the ratified `where TKey : notnull` also forces the constraint onto ScoDictionaryConverter; fix = the same one-line constraint there -- [#366 ScDictionary constraint cascades to a 4th file](project_366_scdictionary_constraint_cascades_to_fourth_file.md) — cascades CS8714 into ScDictionaryConverter.cs beyond the 3-file waiver; STOP + re-escalate, don't widen -- [#366 Batch7 T? return triggers CS8766](project_366_batch7_tnullable_return_cs8766.md) — `T?` on a class-constrained generic implementing a null-oblivious interface member emits CS8766; conform to `T` + justified `!`, don't edit the interface -- [#372 email-classifier nullable patterns](project_372_email_classifier_nullable_patterns.md) — engine props post-ctor `null!`; factory returns `T?`; `.Class!` cascade; `(await Deserialize())!`; DTO `= null!` adds a coverage line -- [#371 OutlookObjects nullable lessons](project_371_outlookobjects_nullable_lessons.md) — a public-signature nullable change regresses OTHER nullable files in the same assembly; keep public tuples non-null w/ `!`; lazy-field CS8618 → `Lazy?` -- [#375 residuals nullable gotchas](project_375_residuals_nullable_gotchas.md) — CS8644 inherited-interface mismatch fixed with a `#nullable disable` island on the class-decl line; full-solution Rebuild cleans SVGControl.dll → isolated CS0006 +- [Nullable annotation CS8632 scoping](project_nullable_annotation_cs8632_scoping.md) — `Type?` in nullable-disabled projects emits CS8632; wrap in `#nullable enable annotations`, not whole-file +- [init/record struct fails CS0518 on net48](project_record_struct_isexternalinit_netfx.md) — no IsExternalInit polyfill; use a ctor-initialized readonly struct with get-only props +- [CS8714 does not fire on net481](project_nullable_cs8714_not_on_net481.md) — net481 BCL lacks notnull, so `where TKey : notnull` is forward-looking only +- Nullable-epic per-issue notes (closed epic, low reuse): [#366 notnull cascade](project_366_notnull_cascades_beyond_wrapperscodictionary.md), [#366 4th file](project_366_scdictionary_constraint_cascades_to_fourth_file.md), [#366 CS8766](project_366_batch7_tnullable_return_cs8766.md), [#372](project_372_email_classifier_nullable_patterns.md), [#371](project_371_outlookobjects_nullable_lessons.md), [#375](project_375_residuals_nullable_gotchas.md) - [Outlook `Action`/`Exception` ambiguity](project_outlook_action_ambiguity.md) — bare `Action` AND bare `Exception` are CS0104-ambiguous in Outlook-interop files; use `System.Action`/`System.Exception` (surfaces only at analyzer/type-check build) ## Component-specific gotchas -- [#349 breadcrumb WebView2 gotchas](project_349_breadcrumb_webview2_gotchas.md) — a retyped Designer field breaks reflection-injected tests (inject a router); aggregate async d__ classes for >=90% proofs; QuickFiler.Test is Newtonsoft-free -- [#227 cycle-4 ToggleFocus genuine-execution gotchas](project_qfc227_cycle4_toggle_focus_genuine_test_gotchas.md) — QuickFiler.Test lacks refs to ObjectListView/WebView2.WinForms (use Activator.CreateInstance(field.FieldType)); ToggleFocus wraps a nested Invoke -- [Theme/FolderPredictor seam retrofit gotchas (#227 cycle-3)](project_theme_folderpredictor_seam_retrofit_gotchas.md) — new required field + shared parameterless-ctor builder = silent regression (inject a non-executing dispatcher); InitAsync FromField is COM-bound -- [ObjectListView TreeListView headless selection](project_objectlistview_treelistview_headless_selection.md) — SelectedObject/SelectedIndex need a native handle; headless can't select — cache the node via SelectionChanged; ListBox→TreeListView retype breaks `new ListBox()` -- [QfcDatamodel BackgroundWorker async-void IsBusy race](project_qfc_backgroundworker_async_void_race.md) — Worker_DoWork is async void so IsBusy flips false instantly; assert WorkerSupportsCancellation instead -- [QfcItemController pump harness needs SaveParameters](project_qfcitemcontroller_pump_harness_needs_saveparameters.md) — SetField-only injection leaves the `??=` factory defaults null (NRE in LoadFolderHandlerAsync); InitializeAsync also needs AF.CtfMap/UseLcppnPredictor/FolderPredictor/RecentsList mocks -- [TaskController (#297) unit-test gotchas](project_taskvisualization_taskcontroller_test_gotchas.md) — ApplyChanges hangs over Moq (mark exempt); get-only MailItem.TaskSubject throws MissingMethodException; STA harness needs TableLayoutPanel parenting + NavTips warmup -- [#328 Rebuild-threading breaks OlObjectsProxy](project_328_rebuild_threading_olobjectsproxy_conflict.md) — threading Parent.Ol.StoresWrapper into AppToDoObjects.Rebuild fails LoadProjInfoAsync_Rebuilds; fix = return null for get_StoresWrapper +- [#349 breadcrumb WebView2 gotchas](project_349_breadcrumb_webview2_gotchas.md) — retyped Designer field breaks reflection-injected tests; aggregate async d__ classes for >=90% +- QuickFiler #227 cycle notes: [cycle-4 ToggleFocus](project_qfc227_cycle4_toggle_focus_genuine_test_gotchas.md), [cycle-3 Theme/FolderPredictor seam](project_theme_folderpredictor_seam_retrofit_gotchas.md) +- [ObjectListView TreeListView headless selection](project_objectlistview_treelistview_headless_selection.md) — selection needs a native handle; cache the node via SelectionChanged +- [QfcDatamodel BackgroundWorker async-void IsBusy race](project_qfc_backgroundworker_async_void_race.md) — IsBusy flips false instantly; assert WorkerSupportsCancellation +- [QfcItemController pump harness needs SaveParameters](project_qfcitemcontroller_pump_harness_needs_saveparameters.md) — SetField-only injection leaves `??=` factory defaults null (NRE in LoadFolderHandlerAsync) +- [TaskController (#297) unit-test gotchas](project_taskvisualization_taskcontroller_test_gotchas.md) — ApplyChanges hangs over Moq; get-only MailItem.TaskSubject throws; STA harness needs TableLayoutPanel parenting - [ProjectEntry setter raw MessageBox](project_projectentry_setter_raw_messagebox.md) — the ProjectID setter uses RAW un-seamed MessageBox.Show and hangs STA tests; CompareTo tie-break needs a Moq IProjectEntry with shifting ProjectID - [IApplicationGlobals member forces implementers](project_iapplicationglobals_member_forces_implementers.md) — adding a member breaks 7 hand-written test-double stubs beyond scope lock; Moq mocks auto-implement - [TimeProvider seam gotchas](project_timeprovider_seam_gotchas.md) — Moq can't mock non-virtual GetLocalNow (use FakeTimeProvider); an optional TimeProvider param forces a Bcl.TimeProvider `` on every consumer (CS0012) diff --git a/.claude/agent-memory/atomic-executor/project_incremental_build_vacuous_baseline.md b/.claude/agent-memory/atomic-executor/project_incremental_build_vacuous_baseline.md index 6c0d21e6..c5a57b63 100644 --- a/.claude/agent-memory/atomic-executor/project_incremental_build_vacuous_baseline.md +++ b/.claude/agent-memory/atomic-executor/project_incremental_build_vacuous_baseline.md @@ -29,5 +29,21 @@ leaves outputs cleaned, so follow it with a passing analyzer Rebuild to restore any coverage run; and `Invoke-VSBuild.ps1` also runs `Sync-PackageReferences.ps1` on every invocation, so verify `git status` afterwards. +**This is NOT specific to `Invoke-VSBuild.ps1`.** Re-measured 2026-08-08 (#505 preflight) with a +direct `MSBuild.exe` call — the exact `CLAUDE.md` analyzer command +(`TaskMaster.sln /t:Build /p:Configuration=Debug "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true +/p:EnforceCodeStyleInBuild=true`) run immediately after a plain `/t:Build` returned `EXIT_CODE=0` +with **18 `Skipping target "CoreCompile"` notices and 0 `csc.exe` invocations**. The analyzer gate +therefore analyzed nothing. Measure it with +`/fl /flp:"logfile=;verbosity=normal"` then count `Skipping target "CoreCompile"` and +`csc.exe` occurrences — `csc.exe = 0` is the unambiguous vacuity signal. + +**Plan-review consequence:** in any final-QC phase the analyzer step almost always follows an +earlier build of the same tree (and, on a loop restart, follows the type-check `/t:Rebuild`), so a +`/t:Build` analyzer gate is vacuous by construction. Require `/t:Rebuild` for the analyzer gate, or +require the acceptance artifact to record a non-zero `csc.exe`/`CoreCompile` count for +`TaskMaster` and `TaskMaster.Test`. CI does not hit this because it always starts from a clean +checkout; `ci.yml` states the same rationale in a comment on its type-check step. + Related: [[project_repo_sdk_and_nullable_rebuild]], [[project_364_nullable_gate_preexisting_blockers]], -[[project_vs18_build_toolchain_paths]]. +[[project_vs18_build_toolchain_paths]], [[project_nullable_build_gate_is_vacuous_incremental]]. diff --git a/.claude/agent-memory/atomic-executor/project_koverage_cobertura_postprocessing_shape.md b/.claude/agent-memory/atomic-executor/project_koverage_cobertura_postprocessing_shape.md new file mode 100644 index 00000000..d2d89ff2 --- /dev/null +++ b/.claude/agent-memory/atomic-executor/project_koverage_cobertura_postprocessing_shape.md @@ -0,0 +1,39 @@ +--- +name: koverage-cobertura-postprocessing-shape +description: Invoke-MSTestWithCoverage.ps1 post-processes the Cobertura dump - backslash filenames, classes already merged per file, test packages stripped, root attributes recomputed - so per-file coverage queries pinned to forward slashes or to summing sibling nodes silently match nothing +metadata: + type: project +--- + +`scripts/vscode/Invoke-MSTestWithCoverage.ps1` does not emit a raw `dotnet-coverage` Cobertura +dump. Before writing `-CoverageOutput` it calls `ConvertTo-KoverageCoberturaXml` +(`Invoke-MSTestWithCoverage.Helpers.ps1`) with **no `-PathSeparator` argument**, so four +transformations apply: + +1. **`filename` uses `\`, not `/`.** `-PathSeparator` defaults to + `[System.IO.Path]::DirectorySeparatorChar`, which is `\` on Windows. Attributes read + `TaskMaster\Ribbon\EngineToggleStateCoordinator.cs`. A query pinned to + `TaskMaster/Ribbon/...` matches **zero** rows and a coverage gate built on it reports 0 or + nothing while appearing to run. +2. **`` nodes are already merged per file.** `Merge-CoberturaClassesByFilename` collapses + the `d__N` async state-machine classes and `<>c` closure classes into one `` per + filename and rewrites its `line-rate`/`branch-rate`. Read that attribute directly; the advice to + "sum `lines-covered`/`lines-valid` across all `` elements sharing the filename" applies + only to an unprocessed raw dump. +3. **Test packages are stripped.** `Get-KoverageProjectAllowlist` enumerates every non-`*.Test` + `*.csproj`/`*.vbproj`/`*.fsproj` assembly name under the repo root; packages outside that set are + removed from ``. First-party production projects (including `TaskMaster`) are in. +4. **Root `` attributes are recomputed** after stripping (`line-rate`, `branch-rate`, + `lines-covered`, `lines-valid`, `branches-covered`, `branches-valid`), so the headline figure is + already a production-only denominator — do not re-derive it. + +Also: `$repoRoot` is `Resolve-Path "$PSScriptRoot\..\.."`, i.e. the **worktree** root, and the +`*.Test.dll` glob applies no `\.claude\` filter, so the script is safe to run from an agent +worktree rooted under `.claude/worktrees/`. It hard-fails if `vswhere.exe`, the vswhere-resolved +`Common7\IDE\Extensions\TestPlatform\vstest.console.exe`, or a global `dotnet-coverage` is missing — +note that vstest path differs from the `CommonExtensions\Microsoft\TestWindow` one used for direct +`vstest.console.exe` calls. + +Related: [[project_coverage_delta_reproduce_baseline_counting_method]], +[[project_csharp_canonical_coverage_artifact_conversion]], +[[project_dotnet_coverage_denominator_nondeterminism]]. diff --git a/.claude/agent-memory/atomic-executor/project_missing_vsto_runtime_breaks_baseline_gates.md b/.claude/agent-memory/atomic-executor/project_missing_vsto_runtime_breaks_baseline_gates.md index a67f7054..b9a94213 100644 --- a/.claude/agent-memory/atomic-executor/project_missing_vsto_runtime_breaks_baseline_gates.md +++ b/.claude/agent-memory/atomic-executor/project_missing_vsto_runtime_breaks_baseline_gates.md @@ -1,10 +1,23 @@ --- name: missing-vsto-runtime-breaks-baseline-gates -description: On this host the VSTO Office Tools runtime assemblies are absent, so the analyzer and nullable solution builds fail with 4x CS0234 in TaskMaster/ThisAddIn.Designer.cs and UtilitiesCS.Test/TaskMaster.Test never build - collapsing repo-wide coverage +description: HISTORICAL (2026-08-04) - absent VSTO Office Tools assemblies once broke the solution build and collapsed repo-wide coverage; NOT reproducing as of 2026-08-08, so always re-verify by building before citing this metadata: type: project --- +> **RE-VERIFIED 2026-08-08 (issue #505 worktree) — DOES NOT REPRODUCE.** The two named +> assemblies (`Microsoft.Office.Tools.Common.v4.0.Utilities`, +> `Microsoft.Office.Tools.Outlook.v4.0.Utilities`) are still absent from the GAC, the VS +> install, and `Common Files\Microsoft Shared\VSTO` — yet +> `msbuild TaskMaster.sln /t:Build /p:Configuration=Debug "/p:Platform=Any CPU"` after +> `nuget restore TaskMaster.sln` completed **EXIT 0 with zero errors** and produced all +> 18 project outputs, including `TaskMaster.dll` and all **9** `*.Test.dll`. The +> `Microsoft.Office.Tools.*` (non-`v4.0.Utilities`) assemblies present in +> `C:\Windows\Microsoft.NET\assembly\GAC_MSIL` are apparently sufficient for +> `ThisAddIn.Designer.cs` to bind. **Never assert the CS0234/6-assembly failure below from +> memory — build first.** Treat the text that follows as a record of one historical +> environment state. + `msbuild TaskMaster.sln` (both the analyzer gate and the nullable gate) fails on this machine with 4 `CS0234` errors in `TaskMaster/ThisAddIn.Designer.cs`, naming `Microsoft.Office.Tools.Outlook.OutlookAddInBase`, diff --git a/.claude/agent-memory/atomic-executor/project_vs18_build_toolchain_paths.md b/.claude/agent-memory/atomic-executor/project_vs18_build_toolchain_paths.md index 0afb6a2f..752e241e 100644 --- a/.claude/agent-memory/atomic-executor/project_vs18_build_toolchain_paths.md +++ b/.claude/agent-memory/atomic-executor/project_vs18_build_toolchain_paths.md @@ -17,4 +17,5 @@ Building/testing the TaskMaster net48 VSTO solution in a fresh worktree on this - Format: csharpier is a local tool v1.x → `dotnet csharpier check .` / `dotnet csharpier format ` (subcommands; bare `dotnet csharpier .` is v0 syntax). Needs `DOTNET_ROOT=.dotnet-sdk` on PATH. - Coverage: `dotnet-coverage` global exe. It re-parses the command string and splits on spaces, so a quoted `"C:\Program Files\...vstest.console.exe"` breaks with `'C:\Program' ... parameter is incorrect`. Use the `--` separator form: `dotnet-coverage collect --output OUT --output-format cobertura -- "$VSTEST" "UtilitiesCS.Test\bin\Debug\UtilitiesCS.Test.dll" /InIsolation`. Cobertura ``/`` elements put `line-rate`/`branch-rate` BEFORE `name`. - Nullable gate (`/p:Nullable=enable /p:TreatWarningsAsErrors=true`): incremental `/t:Build` after an analyzer build is a no-op (0/0). A genuine `/t:Rebuild` surfaces ~34 distinct pre-existing nullable errors confined to vendored `SVGControl.csproj` (see [[project_repo_sdk_and_nullable_rebuild]]); first-party UtilitiesCS/UtilitiesCS.Test are 0. Rebuild-with-dependencies double-reports (34→68) under parallel; use `-m:1` for distinct counts. +- `pwsh -NoProfile -Command` tokenization for MSBuild switches (measured 2026-08-08): `/p:Platform='Any CPU'` and `/flp:'logfile=.log;verbosity=normal'` each collapse to ONE argument with the quotes stripped, and the embedded `;` does NOT split the pwsh statement — the plan-style quoting is safe as written. `/fl /flp:logfile=...` needs the target directory to already exist. Inside a pwsh `-Command` string use `| Out-Null`, never `> /dev/null` (pwsh resolves it as the literal path `C:\dev\null` and the whole statement dies). - Under coverage instrumentation ~17 Deedle/DataFrame/ETL tests (FromDefaultFolder_*, FromArray2D_*, DeedleDoodles, etc.) flake; they pass with 0 failures when instrumentation is off. Pre-existing, unrelated to Folder scoring. diff --git a/.claude/agent-memory/atomic-planner/MEMORY.md b/.claude/agent-memory/atomic-planner/MEMORY.md index adc263ac..469faa2a 100644 --- a/.claude/agent-memory/atomic-planner/MEMORY.md +++ b/.claude/agent-memory/atomic-planner/MEMORY.md @@ -17,6 +17,7 @@ - [Repo-wide csharpier format breaks zero-diff ACs](csharpier-repowide-format-breaks-zero-diff-acs.md) — scope the mutating pass to the plan's own path list; keep `check .` read-only; re-verify the zero-line diff AFTER formatting - [Embedded-resource fail-proof needs a rebuild gate](embedded-resource-failproof-rebuild-gate.md) — edit → rebuild → assert embedded bytes → `[expect-fail]` run; skipping the assert makes the fail-proof itself vacuous - [#503 ribbon readiness plan seams](project_503_ribbon_readiness_plan_seams.md) — RibbonViewer 487/500 forces a 26-member region move; 6+4 Compile entries; compile-time red + dossier; #504-#508 already promoted +- [#505 toggle-state guards plan seams](project_505_toggle_state_guards_plan_seams.md) — runtime red (no dossier); R4 red between catalog and XML tasks; raw cobertura to gitignored coverage/; #522 CI type-check cmd; manual-verification kind accepted - [CSharpier gate: format not pipe-files](csharpier-format-not-pipe-files-gate.md) — formatting tasks must use `csharpier format` + scoped `csharpier check` exit 0; `pipe-files` is stdout-only/non-enforcing and masked a 500-line overflow in #400 - [#400 partial-class headroom placement](project_400_partial_class_headroom_placement.md) — put new coverage cases in existing `.Part2.cs` `[TestClass] partial` files to keep the 17-class filter/count assertions stable - [Manager AsyncLazy shared seam](project_manager_asynclazy_shared_seam.md) — Globals.AF.Manager is shared across all classifier subsystems; use a key-specific accessor, never retype the dictionary value for one key diff --git a/.claude/agent-memory/atomic-planner/project_505_toggle_state_guards_plan_seams.md b/.claude/agent-memory/atomic-planner/project_505_toggle_state_guards_plan_seams.md new file mode 100644 index 00000000..f616cfb6 --- /dev/null +++ b/.claude/agent-memory/atomic-planner/project_505_toggle_state_guards_plan_seams.md @@ -0,0 +1,22 @@ +--- +name: project-505-toggle-state-guards-plan-seams +description: "#505/#506/#518 bundled full-bug plan facts: runtime red (not compile red), R4 red captured between catalog and XML tasks, coordinator+EngineToggleCatalog seam, raw cobertura to gitignored coverage/, manual-verification evidence kind accepted" +metadata: + type: project +--- + +Plan seams for the #505/#506/#518 bundled `full-bug` delivery (plan.2026-08-08T19-22.md in feature 2026-08-08-ribbon-engine-toggle-state-guards-505). + +**Why:** three causally coupled defects in `RibbonViewer.EngineCommands.cs`; the spec pins a two-guard-shape split (4 toggle sites -> new `EngineToggleStateCoordinator`, 6 command sites -> existing `RunEngineCommandAsync` gate + 6 new `EngineCommandCatalog` entries + XML `getEnabled`). + +**How to apply:** +- Unlike #503 (compile-time red + exception dossier), #505's red is a RUNTIME red: reflection shape-pin tests compile against pre-fix code and fail on execution. No fail-before exception dossier needed when a genuine failing run exists. Reflection-invoke keeps R2 compiling across the signature change. +- R4 (existing catalog-derived XML set-equality tests) has no test-code change; its red is captured as a dedicated `[expect-fail]` run task BETWEEN the catalog-extension task and the XML-edit task. "Land atomically" means same commit, not same task — a captured red between the two edits satisfies AC-15 and the atomicity risk. +- `EngineCommandCatalogTests.ControlIds_ContainsExactlyTheEightEngineBackedControlIds` hard-codes the 8-entry set; extending the catalog requires renaming/extending it (research §8 predicted this). +- The `RibbonViewer(RibbonController)` public ctor is field-assignment-only (`Controller.Try` is inside an uninvoked lambda) — safe to construct in a unit test; `RibbonController()` parameterless ctor exists and #507 tests already use it. +- Raw Cobertura goes to the gitignored `coverage\` dir (`.gitignore` `coverage/*`), never under docs/features (81 MB incident) and never `artifacts/csharp/coverage.xml` (SubagentStop hook hard-codes an 85% floor there vs the real 80% policy). +- `evidence/manual-verification/` is an accepted evidence kind (spec AC-22 requires it verbatim; #503 executed with it; not on the forbidden list). +- Type-check gate is CI's `msbuild /t:Rebuild /m ... /p:TreatWarningsAsErrors=true` WITHOUT `/p:Nullable=enable` (issue #522, known-defective); cite #522 in every type-check task so the executor does not "correct" it back to the CLAUDE.md form. +- `Invoke-MSTestWithCoverage.ps1` applies no `\.claude\` filter and derives repoRoot internally, so it is safe in an agent worktree rooted under `.claude\worktrees\`; expected assembly count 9, and 0 discovered = filter bug, never an empty suite. + +Related: [[project-503-ribbon-readiness-plan-seams]], [[async-state-machine-coverage-aggregation]], [[csharpier-format-not-pipe-files-gate]]. diff --git a/.claude/agent-memory/feature-review/MEMORY.md b/.claude/agent-memory/feature-review/MEMORY.md index 2a8586df..ba250343 100644 --- a/.claude/agent-memory/feature-review/MEMORY.md +++ b/.claude/agent-memory/feature-review/MEMORY.md @@ -61,3 +61,4 @@ - [mandated nullable solution gate is vacuous](project_nullable_build_gate_is_vacuous.md) — #503: /t:Build with only /p: changes skips CoreCompile so the gate cannot fail; force /t:Rebuild on the changed project and attribute the errors by file - [null-conditional fix relocates NRE, check callers](project_null-conditional-fix-relocates-nre-check-callers.md) — #507: `Globals.Engines`->`Globals?.Engines` matched sibling `SB` precedent and passed full evidence, but all 11 real `RibbonViewer.cs` callers are unguarded, so the NRE just moves one frame later; grep every call site before crediting a throw->null fix with resolving the reachable crash - [coverage hook needs label+coverage+PASS/FAIL on one line](project_coverage-hook-label-plus-verdict-same-line-507.md) — #507 R1: `Test-LanguageCoverageRow` requires the language label, a coverage keyword, and PASS/FAIL all on the SAME line, and rejects any label+coverage line carrying a banned narrowing word anywhere; dot-source and simulate before finalizing, don't trust a wrapped narrative paragraph +- [505 coordinator prime/toggle race (CR-1)](project_505-coordinator-prime-toggle-race.md) — EngineToggleStateCoordinator lazy prime can overwrite a fresher toggle write and stick stale (no re-prime); Major non-blocking, TryAdd fix + promotion recommended — check status in later ribbon reviews diff --git a/.claude/agent-memory/feature-review/project_505-coordinator-prime-toggle-race.md b/.claude/agent-memory/feature-review/project_505-coordinator-prime-toggle-race.md new file mode 100644 index 00000000..bf8bd577 --- /dev/null +++ b/.claude/agent-memory/feature-review/project_505-coordinator-prime-toggle-race.md @@ -0,0 +1,12 @@ +--- +name: 505-coordinator-prime-toggle-race +description: 'EngineToggleStateCoordinator (#505 review CR-1): lazy prime can overwrite a fresher toggle-written cache value and persist stale for the session; recommended TryAdd fix, promotion recommended — check status in any later ribbon/coordinator review' +metadata: + type: project +--- + +The #505/#506/#518 review (2026-08-08, PASS with 0 blocking) recorded one Major non-blocking finding, CR-1: in `TaskMaster/Ribbon/EngineToggleStateCoordinator.cs`, `ApplyPrimeAsync` writes `_pressedState[engineName] = active` unconditionally. An in-flight prime that read `EngineActiveAsync` before a toggle flipped the setting can write its stale value AFTER `ExecuteToggleAsync` wrote the fresh one; the successful prime task stays registered in `_primeTasks` (`ContainsKey` guard), so no re-prime ever occurs and the stale display persists until the next click. The spec's "every cache write is followed by an invalidation, so the UI converges" assumption fails in this interleaving. + +**Why:** display-only, narrow window (prime resolving concurrently with a click during initial config load), strictly better than merge-base behavior, so dispositioned non-blocking with a recommended fix (`_pressedState.TryAdd` in the prime; residual toggle-vs-toggle double-click case needs write versioning) and a promotion recommendation. Also recorded: CR-2 canceled prime ignored by `CompletePrime` (`Task.Exception` null for Canceled -> marker never removed, key never re-primes, nothing logged); CR-3 the 2 uncovered defensive-guard lines are trivially testable with the existing harness. + +**How to apply:** in any later review touching `EngineToggleStateCoordinator`, the ribbon toggle surface, or a follow-up issue citing this race, check whether CR-1/CR-2 were promoted or fixed before re-deriving the interleaving. Full analysis: `docs/features/active/2026-08-08-ribbon-engine-toggle-state-guards-505/code-review.2026-08-08T21-59.md`. diff --git a/.claude/agent-memory/orchestrator/MEMORY.md b/.claude/agent-memory/orchestrator/MEMORY.md index e4d82eb2..a7d1941e 100644 --- a/.claude/agent-memory/orchestrator/MEMORY.md +++ b/.claude/agent-memory/orchestrator/MEMORY.md @@ -68,3 +68,6 @@ - [C# coverage has two denominators](csharp-coverage-denominator-two-figures.md) — filtered first-party ~85.9% clears the gate, unfiltered ~70.4% doesn't; measure before trusting the "85% floor trap" note, and project Cobertura to JaCoCo - [Preflight catches vacuous gates](preflight-catches-vacuous-gates.md) — MCP plan validator ok:true is not enough; executor preflight found 6 gates that passed while verifying nothing (empty diffs, unsatisfiable acceptance, uncompilable assertions) - [Bash tool mangles MSBuild switches](bash-tool-mangles-msbuild-switches.md) — /m becomes M:/ (MSB1008); run C# tools via pwsh -NoProfile with absolute paths, after nuget restore; carries verified VS18 tool locations +- [Analyzer gate is vacuous without /t:Rebuild](msbuild-analyzer-gate-vacuous-without-rebuild.md) — /t:Build after any earlier build skips CoreCompile on all 18 projects and returns EXIT 0 having compiled NOTHING; demand /t:Rebuild + an /fl log + a csc.exe-count > 0 acceptance +- [Aggregate vstest crash: isolate per assembly](vstest-aggregate-crash-isolate-per-assembly.md) — "Test host process crashed / Total tests: Unknown" is environmental (QuickFiler WinFormsPumpHost, #511); re-run the 9 assemblies with /InIsolation for the real verdict, and check the csproj graph before owning the blame +- [One executor per worktree](one-executor-per-worktree.md) — a stale checkpoint is NOT a dead delegation (executors don't own it); never launch a second executor into a live worktree; recover by re-verifying the committed tree, not by adjudicating artifact provenance diff --git a/.claude/agent-memory/orchestrator/msbuild-analyzer-gate-vacuous-without-rebuild.md b/.claude/agent-memory/orchestrator/msbuild-analyzer-gate-vacuous-without-rebuild.md new file mode 100644 index 00000000..a5fbf49e --- /dev/null +++ b/.claude/agent-memory/orchestrator/msbuild-analyzer-gate-vacuous-without-rebuild.md @@ -0,0 +1,43 @@ +--- +name: msbuild-analyzer-gate-vacuous-without-rebuild +description: An msbuild /t:Build analyzer gate that follows any earlier build of the same tree compiles NOTHING and returns EXIT 0 — always use /t:Rebuild plus a csc.exe-count non-vacuity proof +metadata: + type: project +--- + +MSBuild's legacy non-SDK up-to-date check is **timestamp-based and does not invalidate on a `/p:` +change**. So an analyzer gate written as + +``` +msbuild TaskMaster.sln /t:Build ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true +``` + +that runs after any earlier build of the same tree skips `CoreCompile` for every project and reports +`EXIT 0` having analyzed **nothing**. Measured on #505: **18 `Skipping target "CoreCompile"`, 0 +`csc.exe` invocations, EXIT 0.** The same command as `/t:Rebuild` gives 18 `csc.exe` invocations. + +This is vacuous *by construction* in any plan where the analyzer step follows an implementation-phase +build or a previous QC-loop iteration — which is essentially every plan. CI escapes it only because a +runner starts from a clean checkout; `.github/workflows/ci.yml:106-112` states this exact rationale +for its own `/t:Rebuild` type-check step. + +**How to apply.** In every plan's analyzer and baseline-analyzer task: + +1. Use `/t:Rebuild`, never `/t:Build`. +2. Add a file log and read a count back out of it as a mandatory acceptance condition: + `/fl "/flp:logfile=\coverage\analyzer.log;verbosity=normal"`, then count + `PathToTool=.*csc\.exe` matches. **The count must be > 0 and must include the projects you + touched.** `EXIT_CODE: 0` with a zero `csc.exe` count is a FAILED gate, not a passing one. +3. Apply the same treatment to the *baseline* task, or the final comparison silently depends on + execution order. +4. `/nodeReuse:false` is worth adding: `/m` parallel rebuilds leave ~17 resident MSBuild worker + processes that saturate the box and destabilize the subsequent test run. + +**Why this matters beyond analyzers:** it is the general shape of a vacuous gate — a command whose +exit code is green because it did no work. Pair it with +[[preflight-catches-vacuous-gates]]: the structural MCP plan validator returns `ok:true` on a plan +full of these; only the `atomic-executor` preflight, *running the command and measuring*, catches +them. On #505 the preflight caught this empirically and it was the single highest-value finding of +the run. + +Tool paths and the Bash-mangling caveat: [[bash-tool-mangles-msbuild-switches]]. diff --git a/.claude/agent-memory/orchestrator/one-executor-per-worktree.md b/.claude/agent-memory/orchestrator/one-executor-per-worktree.md new file mode 100644 index 00000000..93eea1de --- /dev/null +++ b/.claude/agent-memory/orchestrator/one-executor-per-worktree.md @@ -0,0 +1,36 @@ +--- +name: one-executor-per-worktree +description: Never launch a second executor against a worktree that already has one running; a stale checkpoint is NOT evidence of a dead delegation, and the cure is re-verifying the committed tree +metadata: + type: feedback +--- + +Do not launch a second `atomic-executor` (or any second writing agent) against a worktree that +already has one in flight. If you suspect a delegation died, prove it before acting. + +**Why:** on the #505/#506/#518 delivery a second executor was launched against +`.claude/worktrees/agent-a406ae4b7a2ce151f` while the original was mid-Phase-5. The stated reason was +"the S8 delegation terminated without returning; the checkpoint was never advanced" — a **false +premise**. An `atomic-executor` does not own `artifacts/orchestration/orchestrator-state.json`; only +the orchestrator that launched it advances the checkpoint. A stale checkpoint is therefore the +*expected* state during a long execution, not a death certificate. The two executors interleaved +writes into the same feature folder and the same coverage output for ~4 minutes, producing two +parallel evidence series and a plan checkbox state neither agent fully owned. + +**How to tell a live delegation from a dead one** (in order of strength): +1. Live processes: `Get-Process MSBuild, vstest.console, testhost, dotnet-coverage` with recent + `StartTime`. Fresh test processes mean the executor is alive. +2. File mtimes advancing under the feature folder. +3. Interleaved timestamp series in `evidence/qa-gates/` is the *signature of the incident*, not of + progress — two series minutes apart with different task coverage means two writers. + +**How to recover** (this worked and is the pattern to reuse): do not kill processes, delete +artifacts, stash, or commit from the duplicate. Halt it, let the incumbent finish, then **re-verify +the committed tree yourself** rather than adjudicating whose artifact is whose. Running +`csharpier check` + analyzer `/t:Rebuild` + type-check + per-assembly tests against the final commit +reproduced every recorded figure exactly (18 `csc.exe`, 0 errors, 6435 passed), which made the +provenance question moot for delivery. Record the incident in the checkpoint honestly instead of +erasing it; also keep the superseded evidence series, explicitly marked superseded. + +Related: [[preflight-catches-vacuous-gates]], [[vstest-aggregate-crash-isolate-per-assembly]], +[[feedback_reverify_ground_truth_after_user_midcycle_commit]]. diff --git a/.claude/agent-memory/orchestrator/vstest-aggregate-crash-isolate-per-assembly.md b/.claude/agent-memory/orchestrator/vstest-aggregate-crash-isolate-per-assembly.md new file mode 100644 index 00000000..c64148a1 --- /dev/null +++ b/.claude/agent-memory/orchestrator/vstest-aggregate-crash-isolate-per-assembly.md @@ -0,0 +1,52 @@ +--- +name: vstest-aggregate-crash-isolate-per-assembly +description: A "Test host process crashed" abort in the aggregate 9-assembly vstest run is environmental, not a failure — re-run per assembly with /InIsolation to get the real verdict +metadata: + type: project +--- + +The repo's aggregate test command (`scripts/vscode/Invoke-MSTestWithCoverage.ps1`, all 9 `*.Test.dll` +in one vstest process) intermittently aborts with: + +``` +The active test run was aborted. Reason: Test host process crashed +Test Run Aborted. Total tests: Unknown +``` + +On #505 this happened **twice at different points** (1476 and 1840 tests in). It is not a test +failure and `Total tests: Unknown` means you cannot read a verdict out of it at all. + +**The decisive check is per-assembly isolation.** Loop the 9 assemblies through +`vstest.console.exe /InIsolation`. On #505 every one passed: + +| assembly | result | +|---|---| +| QuickFiler.Test | 903/903 | +| SVGControl.Test | 75/75 | +| Tags.Test | 65/65 | +| TaskMaster.Test | 367 passed + 1 skipped of 368 | +| TaskTree.Test | 51/51 | +| TaskVisualization.Test | 163/163 | +| ToDoModel.Test | 122/122 | +| UtilitiesCS.Test | 4688/4688 | +| VBFunctions.Test | 1/1 | + +Total **6435 passed, 1 skipped, 0 failures** — a clean green that the aggregate run could not report. + +**Why:** the instability is load-driven and concentrated in the `QuickFiler.Test` +`WinFormsPumpHost` message-pump family, which drives a real WinForms pump against a real +`ItemViewer` (tracked as **#511**). Resident MSBuild `/m` node-reuse workers from the preceding +rebuilds saturate the box; killing them makes the same isolated tests pass 4/4. A sibling symptom is +`InvalidOperationException: Invoke or BeginInvoke cannot be called on a control until the window +handle has been created`. + +**How to apply.** +- Do not report an aggregate-run crash as a blocking test failure. Isolate first, then judge. +- Reach for `Get-Process MSBuild,vstest.console,testhost,dotnet-coverage | Stop-Process -Force` and + `/nodeReuse:false` on the preceding builds to remove the cause rather than adding retries or + sleeps to tests. +- Scope the blame properly before accepting it as yours: `QuickFiler.csproj` does **not** reference + `TaskMaster`, so a `TaskMaster`-only change cannot reach those tests. Check the csproj reference + graph before treating a cross-assembly failure as a regression. +- Discovery filter caveat still applies — filter on the path **relative** to the worktree root; see + [[project_agent_worktree_discovery_and_evidence_hygiene]]. 9 assemblies is correct; 0 is a filter bug. diff --git a/.claude/agent-memory/prd-feature/MEMORY.md b/.claude/agent-memory/prd-feature/MEMORY.md index d20dd343..c35afb5a 100644 --- a/.claude/agent-memory/prd-feature/MEMORY.md +++ b/.claude/agent-memory/prd-feature/MEMORY.md @@ -3,3 +3,4 @@ - [Test disposition: grep for old-overload pins](feedback_test_disposition_overload_pins.md) — grep test project for Setup/Verify of retired overloads before marking any test file "unchanged"; loose mocks fail at run time - [AC gates: verify satisfiability + fresh reads](feedback_ac_gates_verify_satisfiability.md) — check baseline evidence before encoding repo-wide coverage floors as blocking AC; re-read spec from disk before reporting tallies - [full-bug means spec.md only](feedback_full_bug_spec_only.md) — never create user-story.md in full-bug mode; the agent's Expected Outputs header conflicts with the AC-tracking skill +- [#522 nullable type-check deviation](project_522_nullable_typecheck_deviation.md) — CLAUDE.md's /p:Nullable=enable gate is defective; bind spec verification to CI's msbuild command and cite #522 diff --git a/.claude/agent-memory/prd-feature/project_522_nullable_typecheck_deviation.md b/.claude/agent-memory/prd-feature/project_522_nullable_typecheck_deviation.md new file mode 100644 index 00000000..d8bd547d --- /dev/null +++ b/.claude/agent-memory/prd-feature/project_522_nullable_typecheck_deviation.md @@ -0,0 +1,12 @@ +--- +name: nullable-typecheck-deviation-522 +description: CLAUDE.md's type-check command with /p:Nullable=enable is known-defective (issue #522); specs must bind verification to CI's command and record the deviation explicitly +metadata: + type: project +--- + +The `CLAUDE.md` type-check command `msbuild ... /p:Nullable=enable /p:TreatWarningsAsErrors=true` is known-defective and tracked as issue #522. Nullable is per-file opt-in in this solution (no `TaskMaster/Ribbon/` file carries a `#nullable` pragma; only five `AppGlobals` files do), so forcing `/p:Nullable=enable` solution-wide reports 200-414 errors that are red on `main` regardless of any change. CI (`.github/workflows/ci.yml`) deliberately omits the flag. + +**Why:** Encoding the CLAUDE.md command verbatim into a spec's toolchain AC creates an unsatisfiable dead gate (same failure class as [[ac-gates-verify-satisfiability]]). First recorded in the #505/#506/#518 spec (2026-08-08). + +**How to apply:** In any spec's Verification section and toolchain AC, use CI's actual command — `msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true` — and record it as a deliberate, documented deviation from CLAUDE.md with the #522 citation, so reviewers do not flag it as non-compliance. Re-verify against CLAUDE.md before relying on this: once #522 is fixed, this memory is stale and should be removed. diff --git a/.claude/agent-memory/task-researcher/MEMORY.md b/.claude/agent-memory/task-researcher/MEMORY.md index f6e6c9aa..4c6abdfa 100644 --- a/.claude/agent-memory/task-researcher/MEMORY.md +++ b/.claude/agent-memory/task-researcher/MEMORY.md @@ -27,3 +27,4 @@ - [winforms-pump-seam-230](project_winforms_pump_seam_230.md) — #230: WinFormsPumpHost design decided; CreateAsync factory-seam gap; InitializeWebViewAsync stays exempt; 19 -> 11 max (2026-08-07) - [qfc438-search-focus-steal](project_qfc438_search_focus_steal.md) — #438: TWO focus-steal mechanisms (open _focusPending + close _focusAnchor via per-keystroke Clear); CancelSelector emits no SelectionChanged -> stale _selectedFolder (2026-08-08) - [ribbon-engine-readiness-503](project_ribbon_engine_readiness_503.md) — #503: whole TaskMaster Ribbon layer is coverage-excluded; net481 blocks default interface members; 5 orphan onAction callbacks in RibbonExplorer.xml (2026-08-08) +- [ribbon-toggle-state-guards-505](project_ribbon_toggle_state_guards_505.md) — #505/#506/#518: toggle(config) vs command(InboxEngines) guard asymmetry; catalog<->XML set-equality tests; MessageBox in notify sink blocks viewer-level tests (2026-08-08) diff --git a/.claude/agent-memory/task-researcher/project_ribbon_toggle_state_guards_505.md b/.claude/agent-memory/task-researcher/project_ribbon_toggle_state_guards_505.md new file mode 100644 index 00000000..1ac44e31 --- /dev/null +++ b/.claude/agent-memory/task-researcher/project_ribbon_toggle_state_guards_505.md @@ -0,0 +1,21 @@ +--- +name: ribbon-toggle-state-guards-505 +description: "#505/#506/#518 bundle research: toggle vs command guard asymmetry (config vs InboxEngines), EngineCommandCatalog<->XML test coupling forces atomic changes, MessageBox in NotifyEngineCommandNotReady blocks viewer-level tests" +metadata: + type: project +--- + +Bundled bug research (#505 async getPressed, #506 fire-and-forget toggle, #518 ten unguarded `Controller.Engines.` sites), 2026-08-08. Recommended: new host-neutral `EngineToggleStateCoordinator` (last-known-state cache, lazy prime-on-read, update-cache-then-invalidate) for the 4 toggle/getPressed sites; route the 6 ShowDiskDialog/ShowSaveInfo sites through the existing #503 `RunEngineCommandAsync` gate by adding 6 button ids to `EngineCommandCatalog`. + +**Why (non-obvious facts, expensive to rediscover):** + +1. **Guard asymmetry is semantic, not stylistic.** `ToggleEngineAsync`/`EngineActiveAsync` operate on `Globals.AF.Manager.Configuration` (`AppItemEngines.cs:92-109`), while `InitAsync` filters `config.Value.Engine` before populating `InboxEngines` — so a readiness gate keyed on `InboxEngines` would permanently block re-enabling a disabled engine. `ShowSaveInfo`/`ShowDiskDialog` DO require the `InboxEngines` key (no-op otherwise), so the readiness gate is exactly right for them. +2. **`EngineCommandCatalog` membership is load-bearing in three `RibbonExplorerXmlTests`:** every catalog id must declare `getEnabled="EngineCommand_GetEnabled"` in the XML (set-EQUALITY both ways) and must be a `button` element. Adding catalog ids forces XML edits in the same task; `checkBox` ids (the two enable toggles) can never be added. +3. **`NotifyEngineCommandNotReady` calls `MessageBox.Show`** (`RibbonController.EngineCommands.cs:100`), so any unit test that drives a gate-closed path through viewer/controller glue hangs vstest. Behavioral tests must sit at the seam with injected sinks; viewer-level red tests are limited to reflection pins and the getPressed no-throw repro. +4. **Signature pins must compare parameter types by `Type.FullName == "Microsoft.Office.Core.IRibbonControl"`** — TaskMaster.Test has no Office PIA compile reference (`RibbonExplorerXmlTests.cs:280-287`). +5. `AsyncLazy` (`UtilitiesCS/ReusableTypeClasses/AsyncLazy/AsyncLazy.cs`) has NO non-triggering completed-value probe, so "read config synchronously when materialized" is not implementable without new `IAppItemEngines` surface (which bodies in the excluded `AppItemEngines` — the net481 no-DIM trap from [[ribbon-engine-readiness-503]]). +6. XML control ids diverge from method names: the "current location" buttons are `GetSaveState`/`TriageGetSaveState`, not `*SaveLocation*`; toggles are `SpamBayesEnabledToggle`/`TriageEnabledToggle` (checkBox). `SpamBayes.GroupName == "Spam"` (SpamBayes.cs:328). + +**How to apply:** for any future ribbon guard work, first ask whether the operation needs the engine *instance* (InboxEngines-keyed gate) or the engine *configuration* (availability-only guard + cached read); and check whether the proposed test path can reach a MessageBox before writing viewer-level tests. + +Research artifact: `docs/features/active/2026-08-08-ribbon-engine-toggle-state-guards-505/research/2026-08-08T19-30-ribbon-engine-toggle-state-guards-research.md` diff --git a/TaskMaster.Test/Ribbon/EngineCommandCatalogTests.cs b/TaskMaster.Test/Ribbon/EngineCommandCatalogTests.cs index 96cd4530..3811de96 100644 --- a/TaskMaster.Test/Ribbon/EngineCommandCatalogTests.cs +++ b/TaskMaster.Test/Ribbon/EngineCommandCatalogTests.cs @@ -21,6 +21,12 @@ public class EngineCommandCatalogTests [DataRow("TriageSetC", "Triage")] [DataRow("ClearTriage", "Triage")] [DataRow("FilterTriageGroup", "Triage")] + [DataRow("SpamSaveNetwork", "Spam")] + [DataRow("SpamSaveLocal", "Spam")] + [DataRow("GetSaveState", "Spam")] + [DataRow("TriageSaveNetwork", "Triage")] + [DataRow("TriageSaveLocal", "Triage")] + [DataRow("TriageGetSaveState", "Triage")] public void TryGetEngineName_ForEachEngineBackedControlId_ReturnsExpectedEngineName( string controlId, string expectedEngineName @@ -71,7 +77,7 @@ public void TryGetEngineName_WithEmptyControlId_ReturnsFalse() } [TestMethod] - public void ControlIds_ContainsExactlyTheEightEngineBackedControlIds() + public void ControlIds_ContainsExactlyTheFourteenEngineBackedControlIds() { // Arrange var expected = new[] @@ -84,6 +90,12 @@ public void ControlIds_ContainsExactlyTheEightEngineBackedControlIds() "TriageSetC", "ClearTriage", "FilterTriageGroup", + "SpamSaveNetwork", + "SpamSaveLocal", + "GetSaveState", + "TriageSaveNetwork", + "TriageSaveLocal", + "TriageGetSaveState", }; // Act @@ -94,7 +106,7 @@ public void ControlIds_ContainsExactlyTheEightEngineBackedControlIds() .Should() .BeEquivalentTo( expected, - "the verified defect surface is exactly these eight controls" + "the verified defect surface is the eight #503 controls plus the six #518 save/info commands" ); } diff --git a/TaskMaster.Test/Ribbon/EngineToggleCatalogTests.cs b/TaskMaster.Test/Ribbon/EngineToggleCatalogTests.cs new file mode 100644 index 00000000..0862a16b --- /dev/null +++ b/TaskMaster.Test/Ribbon/EngineToggleCatalogTests.cs @@ -0,0 +1,101 @@ +using System.Linq; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace TaskMaster.Test.Ribbon +{ + /// + /// Unit tests for the issue #505 engine-key to toggle-control-id map. The map is the single + /// source of truth shared by EngineToggleStateCoordinator and the ribbon XML, and is + /// deliberately separate from EngineCommandCatalog so the two toggle checkboxes never + /// acquire readiness-gated getEnabled semantics. + /// + [TestClass] + public class EngineToggleCatalogTests + { + [DataTestMethod] + [DataRow("Spam", "SpamBayesEnabledToggle")] + [DataRow("Triage", "TriageEnabledToggle")] + public void TryGetControlId_ForEachToggleEngineKey_ReturnsExpectedControlId( + string engineName, + string expectedControlId + ) + { + // Act + var mapped = EngineToggleCatalog.TryGetControlId(engineName, out var controlId); + + // Assert + mapped.Should().BeTrue("'{0}' has a toggle checkbox", engineName); + controlId.Should().Be(expectedControlId); + } + + [TestMethod] + public void TryGetControlId_ForUnknownEngineName_ReturnsFalse() + { + // Act + var mapped = EngineToggleCatalog.TryGetControlId( + "NotAToggleBackedEngine", + out var controlId + ); + + // Assert + mapped.Should().BeFalse("the catalog must not claim an engine it does not own"); + controlId.Should().BeNull(); + } + + [TestMethod] + public void TryGetControlId_WithNullEngineName_ReturnsFalse() + { + // Act: the coordinator forwards whatever key the ribbon callback supplied. + var mapped = EngineToggleCatalog.TryGetControlId(null, out var controlId); + + // Assert + mapped.Should().BeFalse("a null engine key must be rejected, not thrown on"); + controlId.Should().BeNull(); + } + + [TestMethod] + public void TryGetControlId_WithEmptyEngineName_ReturnsFalse() + { + // Act + var mapped = EngineToggleCatalog.TryGetControlId(string.Empty, out var controlId); + + // Assert + mapped.Should().BeFalse("an empty engine key must be rejected, not thrown on"); + controlId.Should().BeNull(); + } + + [TestMethod] + public void EngineNames_ContainsExactlyTheTwoToggleEngineKeys() + { + // Arrange + var expected = new[] { "Spam", "Triage" }; + + // Act + var actual = EngineToggleCatalog.EngineNames; + + // Assert: set equality, because iteration order carries no meaning. + actual + .Should() + .BeEquivalentTo( + expected, + "exactly two engines expose an enable/disable toggle checkbox" + ); + } + + [TestMethod] + public void EngineNames_ContainsNoDuplicates() + { + // Act + var engineNames = EngineToggleCatalog.EngineNames; + + // Assert: a duplicate would invalidate the same control twice on every refresh. + engineNames + .Should() + .HaveCount( + engineNames.Distinct().Count(), + "each toggle engine key must appear exactly once" + ); + } + } +} diff --git a/TaskMaster.Test/Ribbon/EngineToggleStateCoordinatorTests.cs b/TaskMaster.Test/Ribbon/EngineToggleStateCoordinatorTests.cs new file mode 100644 index 00000000..52edb742 --- /dev/null +++ b/TaskMaster.Test/Ribbon/EngineToggleStateCoordinatorTests.cs @@ -0,0 +1,459 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using UtilitiesCS; + +namespace TaskMaster.Test.Ribbon +{ + /// + /// Unit tests for the issue #505/#506/#518 toggle-state coordinator: the synchronous + /// getPressed cache, its lazy asynchronous prime, the update-before-invalidate ordering + /// of the toggle path, and the observed-and-logged click boundary. + /// + /// + /// Every asynchronous outcome is driven by a and + /// awaited through the coordinator's own prime handle, so no test sleeps, polls, reads the + /// wall clock, touches the filesystem, or starts a message pump. No test drives a path that + /// reaches NotifyEngineCommandNotReady: the notification sink is an injected delegate. + /// + [TestClass] + public class EngineToggleStateCoordinatorTests + { + private const string SpamEngine = "Spam"; + private const string SpamToggleControlId = "SpamBayesEnabledToggle"; + + #region Constructor contracts + + [TestMethod] + public void Constructor_WithNullEnginesAccessor_ThrowsArgumentNullException() + { + // Act + Action act = () => + new EngineToggleStateCoordinator(null, _ => { }, _ => { }, (_, _) => { }); + + // Assert + act.Should() + .Throw("the accessor is required to reach the engines") + .WithParameterName("enginesAccessor"); + } + + [TestMethod] + public void Constructor_WithNullInvalidateDelegate_ThrowsArgumentNullException() + { + // Act + Action act = () => + new EngineToggleStateCoordinator( + () => new Mock().Object, + null, + _ => { }, + (_, _) => { } + ); + + // Assert + act.Should() + .Throw("without invalidation the checkbox never corrects") + .WithParameterName("invalidateControl"); + } + + [TestMethod] + public void Constructor_WithNullNotifyDelegate_ThrowsArgumentNullException() + { + // Act + Action act = () => + new EngineToggleStateCoordinator( + () => new Mock().Object, + _ => { }, + null, + (_, _) => { } + ); + + // Assert + act.Should() + .Throw("a blocked click must always be able to notify") + .WithParameterName("notifyUnavailable"); + } + + [TestMethod] + public void Constructor_WithNullLogErrorDelegate_ThrowsArgumentNullException() + { + // Act + Action act = () => + new EngineToggleStateCoordinator( + () => new Mock().Object, + _ => { }, + _ => { }, + null + ); + + // Assert + act.Should() + .Throw("an observed fault must always be reportable") + .WithParameterName("logError"); + } + + #endregion Constructor contracts + + #region GetPressed — cached read semantics + + [DataTestMethod] + [DataRow(null)] + [DataRow("")] + [DataRow(" ")] + public void GetPressed_WithNullOrWhitespaceKey_ReturnsFalseWithoutPrimeOrInvalidate( + string engineName + ) + { + // Arrange + var harness = new Harness(); + + // Act + var pressed = harness.Coordinator.GetPressed(engineName); + + // Assert + pressed.Should().BeFalse("an unusable engine key must report the toggle as unchecked"); + harness.Engines.Verify(x => x.EngineActiveAsync(It.IsAny()), Times.Never); + harness + .Invalidations.Should() + .BeEmpty("nothing changed, so nothing may be invalidated"); + harness + .Coordinator.GetPrimeTask(engineName) + .IsCompleted.Should() + .BeTrue("no prime may be registered for an unusable key"); + } + + [TestMethod] + public void GetPressed_WithUnmappedKey_ReturnsFalseWithoutPrime() + { + // Arrange + var harness = new Harness(); + + // Act + var pressed = harness.Coordinator.GetPressed("NotAToggleBackedEngine"); + + // Assert + pressed.Should().BeFalse("an engine with no toggle checkbox reports unchecked"); + harness.Engines.Verify(x => x.EngineActiveAsync(It.IsAny()), Times.Never); + harness.Invalidations.Should().BeEmpty(); + } + + [TestMethod] + public void GetPressed_WhenEnginesAccessorReturnsNull_ReturnsFalseAndStartsNothing() + { + // Arrange: the pre-SetGlobals window, where RibbonController.Engines yields null. + var harness = new Harness { EnginesAvailable = false }; + + // Act + var pressed = harness.Coordinator.GetPressed(SpamEngine); + + // Assert + pressed + .Should() + .BeFalse("an unknown state must degrade to unchecked rather than throwing"); + harness.Engines.Verify(x => x.EngineActiveAsync(It.IsAny()), Times.Never); + harness.Invalidations.Should().BeEmpty(); + } + + [TestMethod] + public async Task GetPressed_OnCacheMissWithEnginesAvailable_StartsExactlyOnePrime() + { + // Arrange: the prime is held open, so the second read observes an in-flight prime. + var harness = new Harness(); + var probe = new TaskCompletionSource(); + harness.Engines.Setup(x => x.EngineActiveAsync(SpamEngine)).Returns(probe.Task); + + // Act + var first = harness.Coordinator.GetPressed(SpamEngine); + var second = harness.Coordinator.GetPressed(SpamEngine); + + // Assert + first.Should().BeFalse("the state is not known until the prime completes"); + second.Should().BeFalse("an in-flight prime does not change the cached answer"); + harness.Engines.Verify( + x => x.EngineActiveAsync(SpamEngine), + Times.Once, + "a second read during an in-flight prime must not start a second prime" + ); + harness.Invalidations.Should().BeEmpty("the prime has not completed yet"); + + // Cleanup: complete the prime deterministically so no work is left in flight. + probe.SetResult(false); + await harness.Coordinator.GetPrimeTask(SpamEngine); + } + + [TestMethod] + public async Task GetPressed_AfterPrimeCompletes_ReturnsPrimedValueAndInvalidatesMappedControl() + { + // Arrange + var harness = new Harness(); + var probe = new TaskCompletionSource(); + harness.Engines.Setup(x => x.EngineActiveAsync(SpamEngine)).Returns(probe.Task); + harness.Coordinator.GetPressed(SpamEngine); + + // Act + probe.SetResult(true); + await harness.Coordinator.GetPrimeTask(SpamEngine); + + // Assert + harness + .Coordinator.GetPressed(SpamEngine) + .Should() + .BeTrue("the completed prime is the new cached state"); + harness + .Invalidations.Should() + .Equal( + new[] { SpamToggleControlId }, + "Office must be told to re-query getPressed for the mapped control" + ); + } + + [TestMethod] + public async Task GetPressed_WhenPrimeFaults_LogsErrorAndStillReturnsFalse() + { + // Arrange + var harness = new Harness(); + var probe = new TaskCompletionSource(); + var failure = new InvalidOperationException("configuration load failed"); + harness.Engines.Setup(x => x.EngineActiveAsync(SpamEngine)).Returns(probe.Task); + harness.Coordinator.GetPressed(SpamEngine); + + // Act + probe.SetException(failure); + await harness.Coordinator.GetPrimeTask(SpamEngine); + + // Assert + harness + .Errors.Should() + .ContainSingle( + "a prime fault must be observed exactly once, never left unobserved" + ); + harness.Errors[0].Message.Should().Contain(SpamEngine); + harness.Errors[0].Exception.Should().BeSameAs(failure); + harness.Invalidations.Should().BeEmpty("a failed prime changed no state to display"); + + harness + .Coordinator.GetPressed(SpamEngine) + .Should() + .BeFalse("a failed prime leaves the toggle reporting unchecked"); + + // Cleanup: the failed prime clears its marker, so the read above re-primed. + await harness.Coordinator.GetPrimeTask(SpamEngine); + } + + #endregion GetPressed — cached read semantics + + #region ExecuteToggleAsync — ordering and fault propagation + + [TestMethod] + public async Task ExecuteToggleAsync_PerformsToggleThenRefreshThenCacheThenInvalidate_InOrder() + { + // Arrange: every step appends to one sequence, including a cache probe performed from + // inside the invalidation sink, which is where Office would re-query getPressed. + var harness = new Harness(); + var sequence = new List(); + harness + .Engines.Setup(x => x.ToggleEngineAsync(SpamEngine)) + .Returns(() => + { + sequence.Add("ToggleEngineAsync"); + return Task.CompletedTask; + }); + harness + .Engines.Setup(x => x.EngineActiveAsync(SpamEngine)) + .Returns(() => + { + sequence.Add("EngineActiveAsync"); + return Task.FromResult(true); + }); + harness.OnInvalidate = controlId => + { + sequence.Add( + "CacheVisible:" + harness.Coordinator.GetPressed(SpamEngine).ToString() + ); + sequence.Add("Invalidate:" + controlId); + }; + + // Act + await harness.Coordinator.ExecuteToggleAsync(SpamEngine); + + // Assert: update-before-invalidate is the invariant that prevents Office answering an + // invalidation from stale state. + sequence + .Should() + .Equal( + new[] + { + "ToggleEngineAsync", + "EngineActiveAsync", + "CacheVisible:True", + "Invalidate:" + SpamToggleControlId, + } + ); + } + + [TestMethod] + public async Task ExecuteToggleAsync_WhenToggleFaults_PropagatesUnchanged() + { + // Arrange + var harness = new Harness(); + var failure = new InvalidOperationException("toggle failed"); + harness.Engines.Setup(x => x.ToggleEngineAsync(SpamEngine)).ThrowsAsync(failure); + + // Act + Func act = () => harness.Coordinator.ExecuteToggleAsync(SpamEngine); + + // Assert: the testable core never catches, so the fault reaches the boundary intact. + (await act.Should().ThrowAsync()) + .Which.Should() + .BeSameAs(failure); + harness.Invalidations.Should().BeEmpty("a failed toggle changed no state to display"); + harness.Errors.Should().BeEmpty("only the click boundary reports faults"); + } + + [TestMethod] + public async Task ExecuteToggleAsync_WithUnmappedKey_ThrowsArgumentException() + { + // Arrange + var harness = new Harness(); + + // Act + Func act = () => harness.Coordinator.ExecuteToggleAsync("NotAToggleBackedEngine"); + + // Assert: fail fast — an unmapped key has no control to invalidate. + (await act.Should().ThrowAsync()).WithParameterName("engineName"); + harness.Engines.Verify(x => x.ToggleEngineAsync(It.IsAny()), Times.Never); + } + + #endregion ExecuteToggleAsync — ordering and fault propagation + + #region HandleToggleClickAsync — the observed boundary + + [TestMethod] + public async Task HandleToggleClickAsync_WhenToggleFaults_LogsErrorDoesNotThrowDoesNotInvalidate() + { + // Arrange + var harness = new Harness(); + var failure = new InvalidOperationException("toggle failed"); + harness.Engines.Setup(x => x.ToggleEngineAsync(SpamEngine)).ThrowsAsync(failure); + + // Act + Func act = () => harness.Coordinator.HandleToggleClickAsync(SpamEngine); + + // Assert + await act.Should() + .NotThrowAsync("an async void Office handler must never see an unobserved fault"); + harness.Errors.Should().ContainSingle(); + harness.Errors[0].Message.Should().Contain(SpamEngine); + harness.Errors[0].Exception.Should().BeSameAs(failure); + harness.Invalidations.Should().BeEmpty("a failed toggle changed no state to display"); + harness.Notifications.Should().BeEmpty("a fault is logged, not surfaced as a notice"); + } + + [TestMethod] + public async Task HandleToggleClickAsync_WithNullEngines_NotifiesOnceAndInvokesNothing() + { + // Arrange: the pre-SetGlobals window. + var harness = new Harness { EnginesAvailable = false }; + + // Act + Func act = () => harness.Coordinator.HandleToggleClickAsync(SpamEngine); + + // Assert + await act.Should().NotThrowAsync("a click before initialization must degrade quietly"); + harness + .Notifications.Should() + .ContainSingle("exactly one notice per blocked toggle click"); + harness.Notifications[0].Should().Contain(SpamEngine); + harness.Engines.Verify(x => x.ToggleEngineAsync(It.IsAny()), Times.Never); + harness.Invalidations.Should().BeEmpty(); + harness.Errors.Should().BeEmpty("a refused click is not a fault"); + } + + [TestMethod] + public async Task HandleToggleClickAsync_WhenEnginesAvailable_TogglesAndInvalidates() + { + // Arrange + var harness = new Harness(); + harness.Engines.Setup(x => x.ToggleEngineAsync(SpamEngine)).Returns(Task.CompletedTask); + harness.Engines.Setup(x => x.EngineActiveAsync(SpamEngine)).ReturnsAsync(true); + + // Act + await harness.Coordinator.HandleToggleClickAsync(SpamEngine); + + // Assert + harness.Engines.Verify(x => x.ToggleEngineAsync(SpamEngine), Times.Once); + harness.Invalidations.Should().Equal(new[] { SpamToggleControlId }); + harness + .Coordinator.GetPressed(SpamEngine) + .Should() + .BeTrue("the refreshed state is cached before the control is invalidated"); + harness.Notifications.Should().BeEmpty(); + harness.Errors.Should().BeEmpty(); + } + + #endregion HandleToggleClickAsync — the observed boundary + + /// + /// A coordinator wired to a strict mock and three recording + /// sinks, so every test asserts on what the coordinator decided rather than on + /// presentation. + /// + private sealed class Harness + { + internal Harness() + { + Coordinator = new EngineToggleStateCoordinator( + () => EnginesAvailable ? Engines.Object : null, + controlId => + { + Invalidations.Add(controlId); + OnInvalidate?.Invoke(controlId); + }, + message => Notifications.Add(message), + (message, exception) => Errors.Add(new LoggedError(message, exception)) + ); + } + + internal Mock Engines { get; } = + new Mock(MockBehavior.Strict); + + internal EngineToggleStateCoordinator Coordinator { get; } + + /// + /// When false the engines accessor yields null, modelling the pre-SetGlobals + /// window. + /// + internal bool EnginesAvailable { get; set; } = true; + + /// + /// An optional extra observer invoked from inside the invalidation sink, used by the + /// ordering test to probe the cache at the exact moment Office would re-query. + /// + internal Action OnInvalidate { get; set; } + + internal List Invalidations { get; } = new List(); + + internal List Notifications { get; } = new List(); + + internal List Errors { get; } = new List(); + } + + /// + /// One observed fault, as delivered to the injected error-log delegate. + /// + private sealed class LoggedError + { + internal LoggedError(string message, Exception exception) + { + Message = message; + Exception = exception; + } + + internal string Message { get; } + + internal Exception Exception { get; } + } + } +} diff --git a/TaskMaster.Test/Ribbon/RibbonExplorerXmlTests.cs b/TaskMaster.Test/Ribbon/RibbonExplorerXmlTests.cs index 7d422ef3..6f354da5 100644 --- a/TaskMaster.Test/Ribbon/RibbonExplorerXmlTests.cs +++ b/TaskMaster.Test/Ribbon/RibbonExplorerXmlTests.cs @@ -216,9 +216,14 @@ public void RibbonExplorerXml_EveryEngineBackedControlDeclaresGetEnabledCallback } /// - /// No element other than the engine-backed controls may declare the callback. Putting it - /// on a containing menu would disable save-location, folder-settings, and enable-toggle - /// commands that remain safe and useful during initialization. + /// No element other than the engine-backed controls may declare the callback. The hazard + /// this guards against is disabling via a containing menu, which would sweep up + /// folder-settings and the enable-toggle checkboxes along with the commands. Per-control + /// gating is different: as of issue #518 the save-location and save-info buttons are + /// themselves engine-backed catalog members, so they are disabled exactly while they would + /// otherwise no-op. The two enable-toggle checkboxes remain outside the catalog by design — + /// they are backed by engine configuration rather than readiness, and a readiness-gated + /// toggle could never re-enable a disabled engine. /// [TestMethod] public void RibbonExplorerXml_GetEnabledIsDeclaredOnlyOnEngineBackedControls() diff --git a/TaskMaster.Test/Ribbon/RibbonViewerEngineCallbackShapeTests.cs b/TaskMaster.Test/Ribbon/RibbonViewerEngineCallbackShapeTests.cs new file mode 100644 index 00000000..3c53f35b --- /dev/null +++ b/TaskMaster.Test/Ribbon/RibbonViewerEngineCallbackShapeTests.cs @@ -0,0 +1,365 @@ +using System; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; +using System.Xml.Linq; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace TaskMaster.Test.Ribbon +{ + /// + /// Regression tests for issues #505, #506, and #518: the Office callback signatures of the + /// Spam Config and Triage Config toggle checkboxes, and the pre-SetGlobals degradation + /// of the two getPressed callbacks. + /// + /// + /// + /// VSTO binds ribbon callbacks by name and signature and silently ignores a mismatch: the code + /// compiles, Office queries nothing, and no error is reported. A signature defect is therefore + /// invisible at runtime, which is why these reflection pins exist — they convert a silent + /// binding failure into a failing build. + /// + /// + /// Every parameter type is compared by rather than with + /// typeof. TaskMaster.Test.csproj carries no reference to the Office + /// (Microsoft.Office.Core) primary interop assembly, and a legacy non-SDK + /// ProjectReference does not flow that reference to the compiler. office.dll is + /// present in the test output directory and in the GAC, so runtime reflection resolves the + /// type at test time. + /// + /// + /// The callback names are read from the embedded ribbon document rather than hard-coded, so a + /// rename in either the XML or the viewer is caught as a resolution failure. + /// + /// + [TestClass] + public class RibbonViewerEngineCallbackShapeTests + { + private const string ResourceName = "TaskMaster.Ribbon.RibbonExplorer.xml"; + + private const string RibbonControlTypeName = "Microsoft.Office.Core.IRibbonControl"; + + /// + /// The two Office checkBox ids whose callbacks this fixture pins. They are + /// deliberately outside EngineCommandCatalog: catalog membership implies + /// readiness-gated getEnabled semantics, which is the wrong predicate for a + /// configuration toggle. + /// + private static readonly string[] ToggleControlIds = + { + "SpamBayesEnabledToggle", + "TriageEnabledToggle", + }; + + /// + /// The two save-info command handlers whose shape changes with this fix. Their method + /// names are pinned by onAction in the ribbon XML and do not match their control + /// ids (GetSaveState / TriageGetSaveState). + /// + private static readonly string[] ShowSaveInfoHandlerNames = + { + "GetSaveLocation_Click", + "TriageGetSaveLocation_Click", + }; + + /// + /// The getPressed callback of each toggle checkbox must expose the exact Office + /// contract: a public instance method returning with a single + /// IRibbonControl parameter. An async Task<bool> declaration is the + /// #505 defect and never binds. + /// + [TestMethod] + public void ToggleGetPressedCallbacks_MatchOfficeCheckBoxGetPressedSignature() + { + // Arrange + var document = LoadRibbonDocument(); + + foreach (var controlId in ToggleControlIds) + { + // Act + var callbackName = ResolveCallbackName(document, controlId, "getPressed"); + var callback = GetPublicInstanceMethod(callbackName); + + // Assert + callback + .Should() + .NotBeNull( + "'{0}' is declared as the getPressed callback of '{1}' and must be a public instance method on RibbonViewer", + callbackName, + controlId + ); + callback! + .ReturnType.Should() + .Be( + "Office requires a synchronous bool getPressed callback for '{0}'; an async Task is silently ignored", + controlId + ); + + var parameters = callback.GetParameters(); + parameters + .Should() + .ContainSingle( + "the Office getPressed callback for '{0}' takes exactly one parameter", + controlId + ); + parameters[0] + .ParameterType.FullName.Should() + .Be( + RibbonControlTypeName, + "the single parameter of '{0}' must be the Office IRibbonControl", + callbackName + ); + } + } + + /// + /// The onAction callback of each toggle checkbox must expose the Office + /// checkBox contract: void (IRibbonControl, bool). This shape is already + /// correct before the fix and is pinned so the #506 rewrite cannot regress it. + /// + [TestMethod] + public void ToggleOnActionCallbacks_MatchOfficeCheckBoxOnActionSignature() + { + // Arrange + var document = LoadRibbonDocument(); + + foreach (var controlId in ToggleControlIds) + { + // Act + var callbackName = ResolveCallbackName(document, controlId, "onAction"); + var callback = GetPublicInstanceMethod(callbackName); + + // Assert + callback + .Should() + .NotBeNull( + "'{0}' is declared as the onAction callback of '{1}' and must be a public instance method on RibbonViewer", + callbackName, + controlId + ); + AssertCheckBoxOnActionParameters(callback!, callbackName); + } + } + + /// + /// Invoked before SetGlobals has assigned Globals, each getPressed + /// callback must degrade to an unchecked box rather than raising + /// from a null Engines dereference (#518). + /// + /// + /// The callback is invoked by reflection so this test compiles unchanged across the + /// signature change. While the pre-fix declaration returns Task<bool> the + /// fault is captured inside the returned task, so the task must be awaited for the defect + /// to surface; once the signature is synchronous the returned value is asserted directly. + /// + [TestMethod] + public async Task GetPressedCallbacks_BeforeSetGlobals_ReturnFalseWithoutThrowing() + { + // Arrange + var document = LoadRibbonDocument(); + + foreach (var controlId in ToggleControlIds) + { + var callbackName = ResolveCallbackName(document, controlId, "getPressed"); + var callback = GetPublicInstanceMethod(callbackName); + callback.Should().NotBeNull("'{0}' must exist to be invoked", callbackName); + + // A bare RibbonController leaves Globals unassigned, so RibbonController.Engines + // yields null — exactly the pre-SetGlobals state the ribbon can be polled in. + var viewer = new RibbonViewer(new RibbonController()); + object invocationResult = null; + + // Act + Func act = async () => + { + invocationResult = callback!.Invoke(viewer, new object[] { null }); + if (invocationResult is Task pending) + { + await pending.ConfigureAwait(false); + } + }; + + // Assert + await act.Should() + .NotThrowAsync( + "'{0}' must degrade to an unchecked box before SetGlobals rather than dereferencing a null Engines", + callbackName + ); + + if (callback!.ReturnType == typeof(bool)) + { + invocationResult + .Should() + .Be( + false, + "a never-primed engine key must report the toggle as unchecked for '{0}'", + callbackName + ); + } + } + } + + /// + /// Both toggle click handlers must be awaited async void handlers rather than + /// fire-and-forget void methods that discard the toggle task (#506). The + /// compiler-emitted is the observable proof that + /// the handler awaits. + /// + [TestMethod] + public void ToggleClickHandlers_AreAsyncVoidAwaitedShape() + { + // Arrange + var document = LoadRibbonDocument(); + + foreach (var controlId in ToggleControlIds) + { + // Act + var handlerName = ResolveCallbackName(document, controlId, "onAction"); + var handler = GetPublicInstanceMethod(handlerName); + + // Assert + handler.Should().NotBeNull("'{0}' must exist on RibbonViewer", handlerName); + AssertCheckBoxOnActionParameters(handler!, handlerName); + AssertAwaitedAsyncVoidShape(handler!, handlerName); + } + } + + /// + /// The two ShowSaveInfo command handlers must also become awaited async void + /// handlers, matching the sibling *SaveNetwork_Click / *SaveLocal_Click + /// shape, because their engine work is now deferred into a gated lambda (#518). + /// + [TestMethod] + public void ShowSaveInfoHandlers_AreAsyncVoidAwaitedShape() + { + foreach (var handlerName in ShowSaveInfoHandlerNames) + { + // Act + var handler = GetPublicInstanceMethod(handlerName); + + // Assert + handler.Should().NotBeNull("'{0}' must exist on RibbonViewer", handlerName); + + var parameters = handler!.GetParameters(); + parameters.Should().ContainSingle("'{0}' takes exactly one parameter", handlerName); + parameters[0] + .ParameterType.FullName.Should() + .Be( + RibbonControlTypeName, + "the single parameter of '{0}' must be the Office IRibbonControl", + handlerName + ); + AssertAwaitedAsyncVoidShape(handler, handlerName); + } + } + + /// + /// Asserts the Office checkBox onAction parameter list: + /// (IRibbonControl, bool), with a void return. + /// + private static void AssertCheckBoxOnActionParameters(MethodInfo handler, string handlerName) + { + handler + .ReturnType.Should() + .Be( + typeof(void), + "Office requires a void onAction callback for a checkBox; '{0}' must not return a value", + handlerName + ); + + var parameters = handler.GetParameters(); + parameters + .Should() + .HaveCount( + 2, + "the Office checkBox onAction callback '{0}' takes (IRibbonControl, bool)", + handlerName + ); + parameters[0] + .ParameterType.FullName.Should() + .Be( + RibbonControlTypeName, + "the first parameter of '{0}' must be the Office IRibbonControl", + handlerName + ); + parameters[1] + .ParameterType.Should() + .Be("the second parameter of '{0}' must be the pressed state", handlerName); + } + + /// + /// Asserts the awaited async void shape: a void return plus the + /// compiler-emitted , which is present only when + /// the method body actually contains an await. + /// + private static void AssertAwaitedAsyncVoidShape(MethodInfo handler, string handlerName) + { + handler + .ReturnType.Should() + .Be(typeof(void), "'{0}' is an Office callback and must return void", handlerName); + handler + .GetCustomAttribute() + .Should() + .NotBeNull( + "'{0}' must await its work rather than discarding the returned Task; the compiler emits AsyncStateMachineAttribute only for an async body", + handlerName + ); + } + + /// + /// Resolves a callback name from a named control's attribute in the embedded ribbon + /// document, failing with a specific message when the control or the attribute is absent. + /// + private static string ResolveCallbackName( + XDocument document, + string controlId, + string attributeName + ) + { + var element = document + .Descendants() + .SingleOrDefault(candidate => candidate.Attribute("id")?.Value == controlId); + element.Should().NotBeNull("control '{0}' must exist in the ribbon XML", controlId); + + var attribute = element!.Attribute(attributeName); + attribute + .Should() + .NotBeNull("control '{0}' must declare a '{1}' callback", controlId, attributeName); + + return attribute!.Value; + } + + /// + /// Looks up a public instance method on RibbonViewer by the exact name Office would + /// bind against. + /// + private static MethodInfo GetPublicInstanceMethod(string methodName) + { + return typeof(RibbonViewer).GetMethod( + methodName, + BindingFlags.Public | BindingFlags.Instance + ); + } + + /// + /// Loads the embedded Explorer ribbon document from the production assembly, so the tests + /// read the same bytes Outlook does. + /// + private static XDocument LoadRibbonDocument() + { + var assembly = typeof(RibbonController).Assembly; + using var stream = assembly.GetManifestResourceStream(ResourceName); + stream + .Should() + .NotBeNull( + "the Explorer ribbon must be embedded as '{0}' for Outlook to load it", + ResourceName + ); + using var reader = new StreamReader(stream!); + return XDocument.Parse(reader.ReadToEnd()); + } + } +} diff --git a/TaskMaster.Test/TaskMaster.Test.csproj b/TaskMaster.Test/TaskMaster.Test.csproj index 41d1f714..a03c8433 100644 --- a/TaskMaster.Test/TaskMaster.Test.csproj +++ b/TaskMaster.Test/TaskMaster.Test.csproj @@ -312,8 +312,11 @@ + + + diff --git a/TaskMaster/Ribbon/EngineCommandCatalog.cs b/TaskMaster/Ribbon/EngineCommandCatalog.cs index 2dc793c9..4676a450 100644 --- a/TaskMaster/Ribbon/EngineCommandCatalog.cs +++ b/TaskMaster/Ribbon/EngineCommandCatalog.cs @@ -46,6 +46,19 @@ internal static class EngineCommandCatalog ["TriageSetC"] = "Triage", ["ClearTriage"] = "Triage", ["FilterTriageGroup"] = "Triage", + // Issue #518: the six save/info commands. Their engine work reduces to an + // InboxEngines lookup that no-ops when the key is absent, so engine readiness is the + // semantically exact predicate for them — unlike the two activation toggles, which are + // backed by configuration and live in EngineToggleCatalog instead. Note the id and + // handler names diverge for the "current location" buttons: the control ids are + // GetSaveState / TriageGetSaveState while their onAction handlers remain + // GetSaveLocation_Click / TriageGetSaveLocation_Click, as pinned by the ribbon XML. + ["SpamSaveNetwork"] = "Spam", + ["SpamSaveLocal"] = "Spam", + ["GetSaveState"] = "Spam", + ["TriageSaveNetwork"] = "Triage", + ["TriageSaveLocal"] = "Triage", + ["TriageGetSaveState"] = "Triage", }; private static readonly IReadOnlyCollection ControlIdList = diff --git a/TaskMaster/Ribbon/EngineToggleCatalog.cs b/TaskMaster/Ribbon/EngineToggleCatalog.cs new file mode 100644 index 00000000..837bb330 --- /dev/null +++ b/TaskMaster/Ribbon/EngineToggleCatalog.cs @@ -0,0 +1,92 @@ +using System; +using System.Collections.Generic; +using System.Collections.ObjectModel; + +namespace TaskMaster +{ + /// + /// The single source of truth binding an engine key to the id of the Explorer-ribbon + /// checkBox that displays and flips that engine's activation setting. + /// + /// + /// + /// This map is deliberately kept separate from , and + /// the two toggle checkboxes must never join that catalog. Two independent reasons: + /// + /// + /// First, semantics. EngineCommandCatalog membership means "gated on engine readiness", + /// and readiness is computed from InboxEngines. An engine that is configured off never + /// enters InboxEngines, so a readiness-gated toggle could never be used to re-enable a + /// disabled engine — the gate would be permanently closed on exactly the control that exists + /// to open it. Toggle state is backed by engine configuration, not by readiness. + /// + /// + /// Second, schema. Catalog membership drives the getEnabled set-equality assertions and + /// the "every catalog id resolves to a button element" assertion in + /// RibbonExplorerXmlTests. The toggles are checkBox elements, so adding them to + /// the command catalog would fail that test by construction. + /// + /// + /// This type is deliberately NOT marked [ExcludeFromCodeCoverage]: it is host-neutral + /// data with no COM and no Microsoft.Office.* reference, and is fully unit-tested. It + /// follows the precedent. + /// + /// + internal static class EngineToggleCatalog + { + /// + /// Engine key to toggle control id. Ordinal comparison, matching the + /// default + /// used by AppItemEngines.InboxEngines, so "spam" is not "Spam". + /// + /// + /// The "Spam" key is SpamBayes.GroupName; the "Triage" key is the + /// literal used by the Triage engine's own registration. + /// + private static readonly IReadOnlyDictionary Map = new Dictionary< + string, + string + >(StringComparer.Ordinal) + { + ["Spam"] = "SpamBayesEnabledToggle", + ["Triage"] = "TriageEnabledToggle", + }; + + private static readonly IReadOnlyCollection EngineNameList = + new ReadOnlyCollection(new List(Map.Keys)); + + /// + /// The engine keys that have a toggle checkbox, without duplicates. + /// + internal static IReadOnlyCollection EngineNames => EngineNameList; + + /// + /// Resolves the ribbon control id of the toggle checkbox that displays the supplied + /// engine's activation state. + /// + /// + /// The engine key, compared ordinally. May be null, empty, or unrecognized. + /// + /// + /// The toggle control id when the lookup succeeds; otherwise . + /// + /// + /// when has a toggle checkbox; + /// for a null, empty, or unrecognized key. Callers must not read + /// when the result is . + /// + internal static bool TryGetControlId(string engineName, out string controlId) + { + if (string.IsNullOrEmpty(engineName)) + { + // null-forgiving: the pinned signature uses a non-nullable `out string`, and the + // documented contract is that callers must not read the out value when the method + // returns false. This matches EngineCommandCatalog.TryGetEngineName. + controlId = null!; + return false; + } + + return Map.TryGetValue(engineName, out controlId); + } + } +} diff --git a/TaskMaster/Ribbon/EngineToggleStateCoordinator.cs b/TaskMaster/Ribbon/EngineToggleStateCoordinator.cs new file mode 100644 index 00000000..4c11904d --- /dev/null +++ b/TaskMaster/Ribbon/EngineToggleStateCoordinator.cs @@ -0,0 +1,389 @@ +using System; +using System.Collections.Concurrent; +using System.Globalization; +using System.Threading; +using System.Threading.Tasks; +using UtilitiesCS; + +namespace TaskMaster +{ + /// + /// The issue #505/#506/#518 state coordinator for the Spam and Triage engine-activation + /// toggle checkboxes: a synchronous last-known-state cache answering Office's + /// getPressed poll, a lazy asynchronous prime that corrects that cache, and the awaited + /// toggle path whose ordering guarantees Office never re-queries stale state. + /// + /// + /// + /// Office's checkBox getPressed contract is a synchronous + /// bool-returning callback polled on the Outlook STA, but the truth + /// () sits behind an awaited configuration + /// load. Blocking the STA to bridge that gap is prohibited: ribbon controller paths install a + /// WindowsFormsSynchronizationContext on that thread, so a continuation posted back to + /// a blocked STA is a deterministic deadlock, and the first configuration await triggers a + /// full classifier-configuration disk load that would freeze menu-open. This type resolves the + /// mismatch with a cache instead: the read is a dictionary lookup, and correctness is restored + /// asynchronously by invalidating the control once the real value is known. + /// + /// + /// Engine readiness is deliberately not consulted. + /// probes InboxEngines, from which an engine configured off is filtered out, so a + /// readiness-gated toggle could never re-enable a disabled engine. Toggle state is backed by + /// configuration, which is why these four call sites do not route through + /// RunEngineCommandAsync. + /// + /// + /// This type is deliberately NOT marked [ExcludeFromCodeCoverage]: it is host-neutral + /// decision logic with no COM, no Microsoft.Office.* reference, no MessageBox, + /// no WinForms type, and no logger reference — logging is an injected delegate. It follows the + /// precedent and is fully unit-tested. The only + /// STA-affine operation, IRibbonUI.InvalidateControl, stays behind the injected + /// invalidateControl delegate whose production implementation marshals through + /// UtilitiesCS.UiThread.Dispatcher. + /// + /// + internal sealed class EngineToggleStateCoordinator + { + /// + /// Rendered in place of an engine key when the caller supplied null or empty, so a message + /// is never ambiguous about which key was seen. + /// + private const string NullEngineNameToken = "(null)"; + + private readonly Func _enginesAccessor; + private readonly Action _invalidateControl; + private readonly Action _notifyUnavailable; + private readonly Action _logError; + + /// + /// Serializes the at-most-one-prime decision. Held only across a dictionary probe and a + /// task start; no await occurs inside it. + /// + private readonly object _primeGate = new object(); + + /// + /// Last-known activation state per engine key. A key absent from this map has never been + /// primed successfully and reports as unchecked. + /// + private readonly ConcurrentDictionary _pressedState = + new ConcurrentDictionary(StringComparer.Ordinal); + + /// + /// The in-flight — or most recently completed — prime per engine key. Its presence is the + /// at-most-one-prime guard; its value is the test-observable handle returned by + /// . + /// + private readonly ConcurrentDictionary _primeTasks = new ConcurrentDictionary< + string, + Task + >(StringComparer.Ordinal); + + /// + /// Creates a coordinator over an engines accessor and three injected sinks. + /// + /// + /// Supplies the current engines container. Must not be null, but is expected to return + /// null before the ribbon controller has been given its globals, which this type treats as + /// "state unknown" rather than as an error. + /// + /// + /// Receives a ribbon control id whenever the cached state behind that control changes, so + /// Office re-queries getPressed. Must not be null. + /// + /// + /// Receives exactly one message per toggle click refused because the engines are not + /// available. Presentation is the sink's concern. Must not be null. + /// + /// + /// Receives an observed prime or toggle fault as a message plus the exception. Must not be + /// null. + /// + /// Any argument is null. + internal EngineToggleStateCoordinator( + Func enginesAccessor, + Action invalidateControl, + Action notifyUnavailable, + Action logError + ) + { + _enginesAccessor = + enginesAccessor ?? throw new ArgumentNullException(nameof(enginesAccessor)); + _invalidateControl = + invalidateControl ?? throw new ArgumentNullException(nameof(invalidateControl)); + _notifyUnavailable = + notifyUnavailable ?? throw new ArgumentNullException(nameof(notifyUnavailable)); + _logError = logError ?? throw new ArgumentNullException(nameof(logError)); + } + + /// + /// The synchronous getPressed answer for an engine toggle, plus a lazy prime when + /// the state is not yet known. + /// + /// The engine key; ordinal, case-sensitive. + /// + /// The cached activation state, or when the key is null, + /// whitespace, unmapped, or has never been primed. This method performs a dictionary read + /// only: it never awaits, never blocks, and never throws. + /// + /// + /// On a cache miss with the engines available, at most one prime per key is started; a + /// second read while a prime is in flight starts no second prime. When the prime succeeds + /// it stores the value and invalidates the mapped control, so Office re-queries and the + /// checkbox corrects itself. With the engines unavailable nothing is started, which is the + /// correct pre-SetGlobals degradation. + /// + internal bool GetPressed(string engineName) + { + if (!EngineToggleCatalog.TryGetControlId(engineName, out var controlId)) + { + return false; + } + + if (_pressedState.TryGetValue(engineName, out var cached)) + { + return cached; + } + + StartPrimeIfNeeded(engineName, controlId); + return false; + } + + /// + /// The toggle-click boundary: the only place in this type that observes a fault with a + /// catch clause. + /// + /// The engine key whose activation setting is being flipped. + /// + /// A task that completes when the toggle path has completed or its fault has been + /// observed. + /// + /// + /// When the engines are not available the click is refused with exactly one + /// notifyUnavailable message and nothing else is invoked. Otherwise + /// runs inside a single boundary try/catch: + /// a fault is reported through logError, is not rethrown, and does not invalidate. + /// This method never throws, because its caller is an async void Office handler + /// whose faults would otherwise become unobserved. + /// + internal async Task HandleToggleClickAsync(string engineName) + { + if (_enginesAccessor() is null) + { + _notifyUnavailable(BuildUnavailableMessage(engineName)); + return; + } + + try + { + await ExecuteToggleAsync(engineName).ConfigureAwait(false); + } + catch (Exception ex) + { + _logError(BuildToggleFailedMessage(engineName), ex); + } + } + + /// + /// The testable core of the toggle path: flip the setting, re-read the truth, update the + /// cache, then invalidate the control — in exactly that order. + /// + /// The engine key; must be a mapped toggle key. + /// A task that completes once the control has been invalidated. + /// + /// is null, whitespace, or not a mapped toggle key. + /// + /// + /// The engines are not available. Callers reach this method through + /// , which refuses that case first; the guard exists so + /// a direct caller fails explicitly rather than with a null dereference. + /// + /// + /// This method contains no catch of any kind, so it can never degenerate into a + /// swallow-all: an engine fault propagates unchanged to the boundary. Updating the cache + /// before invalidating is the load-bearing invariant — Office answers an + /// invalidation by re-querying getPressed, so invalidating first would be answered + /// from stale state. + /// + internal async Task ExecuteToggleAsync(string engineName) + { + if (!EngineToggleCatalog.TryGetControlId(engineName, out var controlId)) + { + throw new ArgumentException( + BuildUnmappedKeyMessage(engineName), + nameof(engineName) + ); + } + + var engines = _enginesAccessor(); + if (engines is null) + { + throw new InvalidOperationException(BuildUnavailableMessage(engineName)); + } + + await engines.ToggleEngineAsync(engineName).ConfigureAwait(false); + var active = await engines.EngineActiveAsync(engineName).ConfigureAwait(false); + + _pressedState[engineName] = active; + _invalidateControl(controlId); + } + + /// + /// The in-flight — or most recently completed — prime for an engine key, exposed so tests + /// can await the prime deterministically instead of polling or sleeping. + /// + /// The engine key; ordinal, case-sensitive. + /// + /// The prime task, or when no prime has been started for + /// the key. The returned task never faults: a prime fault is observed inside the prime + /// itself and reported through logError. + /// + internal Task GetPrimeTask(string engineName) + { + if (string.IsNullOrEmpty(engineName)) + { + return Task.CompletedTask; + } + + return _primeTasks.TryGetValue(engineName, out var prime) ? prime : Task.CompletedTask; + } + + /// + /// Starts the single prime for an engine key, unless one is already registered or the + /// engines are not yet available. + /// + private void StartPrimeIfNeeded(string engineName, string controlId) + { + var engines = _enginesAccessor(); + if (engines is null) + { + return; + } + + lock (_primeGate) + { + if (_primeTasks.ContainsKey(engineName)) + { + return; + } + + _primeTasks[engineName] = StartObservedPrime(engines, engineName, controlId); + } + } + + /// + /// Runs and attaches the fault observer. + /// + /// + /// The observer is a continuation rather than a catch clause, so this type keeps + /// exactly one catch — the click boundary. Reading + /// inside marks the fault + /// observed, so no unobserved task remains. The returned continuation task always + /// completes successfully, which is what makes it safe for a test to await. + /// + private Task StartObservedPrime( + IAppItemEngines engines, + string engineName, + string controlId + ) + { + return ApplyPrimeAsync(engines, engineName, controlId) + .ContinueWith( + completed => CompletePrime(completed, engineName), + CancellationToken.None, + TaskContinuationOptions.None, + TaskScheduler.Default + ); + } + + /// + /// Reads the real activation state once, stores it, and invalidates the mapped control. + /// Contains no catch: a fault propagates into the returned task, where + /// observes it. + /// + private async Task ApplyPrimeAsync( + IAppItemEngines engines, + string engineName, + string controlId + ) + { + var active = await engines.EngineActiveAsync(engineName).ConfigureAwait(false); + _pressedState[engineName] = active; + _invalidateControl(controlId); + } + + /// + /// Observes the outcome of a prime. On failure the cache is left unset — so the key still + /// reports unchecked — the in-flight marker is cleared so a later read may re-prime, and + /// the fault is reported through logError. + /// + private void CompletePrime(Task completed, string engineName) + { + var failure = completed.Exception; + if (failure is null) + { + return; + } + + _primeTasks.TryRemove(engineName, out _); + _logError(BuildPrimeFailedMessage(engineName), failure.GetBaseException()); + } + + /// + /// Renders an engine key for inclusion in a message, so a null key is never ambiguous. + /// + private static string RenderEngineName(string engineName) + { + return string.IsNullOrEmpty(engineName) ? NullEngineNameToken : engineName; + } + + /// + /// The message emitted when a toggle click is refused because the engines are unavailable. + /// + private static string BuildUnavailableMessage(string engineName) + { + return string.Format( + CultureInfo.CurrentCulture, + "The engine '{0}' is not available yet, so its enable/disable setting cannot be " + + "changed. Please try again once initialization completes.", + RenderEngineName(engineName) + ); + } + + /// + /// The message logged when the toggle path faults. + /// + private static string BuildToggleFailedMessage(string engineName) + { + return string.Format( + CultureInfo.CurrentCulture, + "Toggling the enable/disable setting for engine '{0}' failed.", + RenderEngineName(engineName) + ); + } + + /// + /// The message logged when the state prime faults. + /// + private static string BuildPrimeFailedMessage(string engineName) + { + return string.Format( + CultureInfo.CurrentCulture, + "Reading the activation state for engine '{0}' failed; its toggle continues to " + + "report unchecked.", + RenderEngineName(engineName) + ); + } + + /// + /// The message carried by the for an unmapped engine key. + /// + private static string BuildUnmappedKeyMessage(string engineName) + { + return string.Format( + CultureInfo.CurrentCulture, + "The engine key '{0}' has no toggle checkbox in EngineToggleCatalog.", + RenderEngineName(engineName) + ); + } + } +} diff --git a/TaskMaster/Ribbon/RibbonController.EngineCommands.cs b/TaskMaster/Ribbon/RibbonController.EngineCommands.cs index 66e3359d..6736c009 100644 --- a/TaskMaster/Ribbon/RibbonController.EngineCommands.cs +++ b/TaskMaster/Ribbon/RibbonController.EngineCommands.cs @@ -19,6 +19,7 @@ namespace TaskMaster public partial class RibbonController { private EngineGatedCommandRunner _engineCommandRunner; + private EngineToggleStateCoordinator _engineToggleCoordinator; /// /// The lazily-built gated runner for engine-backed ribbon commands. @@ -44,6 +45,66 @@ public partial class RibbonController NotifyEngineCommandNotReady ); + /// + /// The lazily-built state coordinator for the two engine-activation toggle checkboxes. + /// + /// + /// + /// Deliberately separate from . The gated runner is keyed on + /// engine readiness (InboxEngines membership), which is the wrong predicate + /// for a configuration toggle: an engine that is configured off never enters + /// InboxEngines, so a readiness-gated toggle could never be used to re-enable it. + /// + /// + /// The engines accessor reads Globals directly rather than going through the + /// RibbonController.Engines property, matching the + /// decoupling, and is never routed through SB / Triage / TriageAsync, + /// whose getters install a real WindowsFormsSynchronizationContext on the calling + /// thread as a side effect. The blocked-click notice reuses + /// , so presentation stays in this exempt shim. + /// + /// + private EngineToggleStateCoordinator EngineToggles => + _engineToggleCoordinator ??= new EngineToggleStateCoordinator( + // The null-forgiving operator records that this accessor may legitimately return + // null before SetGlobals has run. EngineToggleStateCoordinator treats a null + // result as "state unknown" by contract, so null is a supported value rather than + // a defect. + () => Globals?.Engines!, + controlId => _viewer?.InvalidateEngineToggle(controlId), + NotifyEngineCommandNotReady, + (message, exception) => logger.Error(message, exception) + ); + + /// + /// The getPressed decision for an engine-activation toggle checkbox. + /// + /// The engine key backing the toggle. + /// + /// The last known activation state, or when it is not yet known. + /// A synchronous dictionary read: Office polls this on the STA, so it must never await or + /// block. + /// + internal bool IsEngineToggleActive(string engineName) + { + return EngineToggles.GetPressed(engineName); + } + + /// + /// Handles a click on an engine-activation toggle checkbox: flips the setting, refreshes + /// the cached state, and invalidates the control, observing any fault. + /// + /// The engine key backing the toggle. + /// + /// A task that completes when the toggle has completed or its fault has been observed. + /// The returned task never faults, so the async void Office handler that awaits it + /// cannot raise an unobserved exception. + /// + internal Task HandleEngineToggleClickAsync(string engineName) + { + return EngineToggles.HandleToggleClickAsync(engineName); + } + /// /// The getEnabled decision for an engine-backed ribbon control. /// diff --git a/TaskMaster/Ribbon/RibbonExplorer.xml b/TaskMaster/Ribbon/RibbonExplorer.xml index 9d8403ee..672dc334 100644 --- a/TaskMaster/Ribbon/RibbonExplorer.xml +++ b/TaskMaster/Ribbon/RibbonExplorer.xml @@ -116,18 +116,21 @@ id="SpamSaveNetwork" imageMso="ServerConnection" onAction="SpamSaveNetwork_Click" + getEnabled="EngineCommand_GetEnabled" label="Network" />