fix(#505): synchronous getPressed via toggle-state coordinator, awaited toggles, and guarded engine dereferences

[drmoisan]

Summary

Fixes three coupled defects in the Spam Config and Triage Config ribbon submenu callbacks, delivered as one change because all three land in TaskMaster/Ribbon/RibbonViewer.EngineCommands.cs and #518's call sites overlap the exact methods #505 and #506 rewrite.

• Bug: ribbon-async-getpressed-signature #505 — SpamBayesEnabled_GetPressed and TriageEnabled_GetPressed were declared async Task<bool>, but Office's getPressed contract requires a synchronous bool GetPressed(Office.IRibbonControl control). VSTO ignores a signature mismatch silently, so both toggle buttons never reflected real engine activation state.
• Bug: ribbon-toggle-engine-fire-and-forget #506 — SpamBayesEnabled_Click and TriageEnabled_Click were void methods whose body was an unawaited ToggleEngineAsync(...). The returned Task was discarded, so the toggle had no ordering guarantee and any fault vanished into an unobserved task.
• Bug: Bug: ribbon-engines-callers-unguarded-null-deref #518 — Ten Controller.Engines.<member> dereferences had no null guard. Bug: ribbon-controller-engines-null-unsafe #507 (merged) made the property Globals?.Engines, which returns null instead of throwing — relocating the NullReferenceException to the call site rather than eliminating it.

These are causally coupled: a synchronous GetPressed needs cached state, that cache is only correct if the toggle is awaited and refreshes it in a defined order, and both must respect the #507 null contract. Fixing them separately would have meant three passes over one file with three fan-in conflicts.

Closes #505
Closes #506
Closes #518

Design: two guard shapes, because there are two different semantics

The single most important decision in this change, and the one worth reviewing first.

The obvious approach — route all ten sites through the existing RunEngineCommandAsync / EngineReadinessGate from #503 — is wrong for four of them. That gate is keyed on InboxEngines, and AppItemEngines.InitAsync filters a disabled engine out of InboxEngines entirely. Gating the enable/disable toggle on it would mean a disabled engine could never be re-enabled: the gate would refuse the only command that could turn it back on.

So the ten sites split by what actually backs them:

Sites	Backed by	Routed through
4 toggle sites (SpamBayesEnabled_Click/_GetPressed, TriageEnabled_Click/_GetPressed)	configuration (ToggleEngineAsync / EngineActiveAsync read Globals.AF.Manager.Configuration)	new EngineToggleStateCoordinator
6 save/info sites (ShowDiskDialog x4, ShowSaveInfo x2)	InboxEngines (they no-op without the key)	existing RunEngineCommandAsync gate

For the six command sites the readiness gate is semantically exact, so they reuse the #503 mechanism plus six new EngineCommandCatalog entries and six matching getEnabled attributes in RibbonExplorer.xml (an existing set-equality test forces those two to change atomically). The two toggle controls are checkBox elements and deliberately stay out of EngineCommandCatalog, which an existing test requires to contain only buttons.

The ordering invariant

EngineToggleStateCoordinator.ExecuteToggleAsync performs, in exactly this order:

await ToggleEngineAsync  ->  await EngineActiveAsync  ->  write cache  ->  invalidate control

Updating the cache before invalidating is load-bearing: Office answers an invalidation by re-querying getPressed, so invalidating first would be answered from stale state. The test pins this by probing GetPressed from inside the invalidation sink — the exact instant Office would re-query — so the invariant is verified as observable behavior rather than as a mock-call sequence.

GetPressed itself is a lock-free ConcurrentDictionary read that defaults to false and starts a background prime on first miss. It never blocks the STA: .Result / .Wait() / GetAwaiter().GetResult() are prohibited in the new code and their absence is gated, because blocking would deadlock against the captured WindowsFormsSynchronizationContext and freeze the menu during the configuration disk load.

The async void boundary

Both *_Click handlers are now single awaited expressions into HandleEngineToggleClickAsync, whose returned task cannot fault: the type's only catch wraps ExecuteToggleAsync and routes to logError without rethrowing. The prime path is observed by a continuation that reads Task.Exception, so no UnobservedTaskException remains. This matches the shape the sibling *SaveNetwork_Click / *SaveLocal_Click handlers in the same regions already used.

Verified counts

RibbonViewer.EngineCommands.cs held 11 Engines. references at the merge base. TestSpam_Click was already gated (its dereference sits inside a lambda passed to RunEngineCommandAsync), leaving 10 unguarded at lines 120, 123, 126, 129, 132, 189, 192, 195, 198, 201. This was derived independently and matches the restated table in the #518 comment exactly.

At head: 10 newly guarded, 1 pre-existing gate, 0 unguarded production dereferences. TestSpam_Click is byte-identical. RibbonController.Intelligence.cs has a zero-line diff, so #507's Globals?.Engines is intact and was not reverted.

Two corrections to the generated PR context

artifacts/pr_context.summary.txt contains two lines that are false for this branch; recording them so a reader does not rely on them:

1. "GitHub CLI unavailable: gh is not installed" — false. gh is at /c/Program Files/GitHub CLI/gh, authenticated as drmoisan. The autoclose section consequently reports "None (GitHub CLI unavailable)" and lists stale author-asserted candidates (Bug: quickfiler-high-confidence-queue-init-stall #424, Bug: ribbon-engine-readiness-guard #503, Bug: ribbon-dead-callback-names #504, Bug: ribbon-controller-engines-null-unsafe #507, Bug: wpf-dispatcher-yield-test-order-dependent #508, Bug: winformspumphost-tests-load-flaky-visible-window #511) inherited from earlier branches. The actual closing set for this PR is exactly Bug: ribbon-async-getpressed-signature #505, Bug: ribbon-toggle-engine-fire-and-forget #506, Bug: Bug: ribbon-engines-callers-unguarded-null-deref #518.
2. "Core logic changes: 0 files / Docs...: 61 files" — false. The change touches 13 core-logic files (+1650/-21): 7 production .cs/.csproj/.xml and 6 test files. The classifier bucketed the entire C# diff as docs.

Testing

Red-first per the CLAUDE.md bugfix workflow: regression tests R1-R5 were written and demonstrated failing against pre-fix source before any fix landed, each with a captured red-run artifact.

• EngineToggleStateCoordinatorTests.cs (459 lines) — cached-read semantics, the ordering invariant, fault observation at the boundary, prime lifecycle, null-engines degradation.
• EngineToggleCatalogTests.cs, RibbonViewerEngineCallbackShapeTests.cs — the callback signatures are pinned by reflection, including an AsyncStateMachineAttribute check, so a regression to async Task<bool> fails the build instead of failing silently in Office.

MSTest + Moq (strict) + FluentAssertions. Determinism comes from TaskCompletionSource, not sleeps: no Thread.Sleep, Task.Delay, wall-clock reads, temp files, Form, MessageBox, BackgroundWorker, or message pump in any new test.

Toolchain

Independently re-verified by the orchestrator against the committed tree, in addition to the executor's own fingerprint-proven single pass:

Stage	Result
csharpier check .	EXIT 0, 1517 files, 0 unformatted
Analyzers (/t:Rebuild)	EXIT 0, 6 warnings, 0 errors, 18 csc.exe invocations
Type-check (/t:Rebuild)	EXIT 0, 6 warnings, 0 errors
Tests + coverage	6435 passed, 1 skipped, 0 failures across all 9 assemblies

The 6 warnings are byte-identical to the merge base (2x pre-existing CS2002 in UtilitiesCS.Test, 4 System.Reactive packages.config advisories). Zero new diagnostics.

Two notes on how those numbers were obtained, both of which produced misleading results first:

• The analyzer gate uses /t:Rebuild, not /t:Build. MSBuild's legacy up-to-date check is timestamp-based and does not invalidate on a /p: change, so a /t:Build analyzer run following any earlier build skips CoreCompile on all 18 projects and returns EXIT 0 having compiled nothing (measured: 18 skips, 0 csc.exe). The gate now asserts a non-zero csc.exe count as a non-vacuity proof.
• The aggregate single-process test run aborted twice with Test host process crashed at differing points (1476 and 1840 tests in), reporting Total tests: Unknown. Per-assembly /InIsolation runs resolved it — every assembly green. This is pre-existing load-driven instability in the QuickFiler.Test WinFormsPumpHost family (Bug: winformspumphost-tests-load-flaky-visible-window #511); QuickFiler.csproj does not reference TaskMaster, so this change cannot reach it. No test was weakened and no retry or sleep was added.

The type-check step deliberately uses CI's command and omits /p:Nullable=enable, which CLAUDE.md prescribes. That variant is defective and separately tracked as #522: this repo uses per-file #nullable enable opt-in, and forcing the flag reports 200-414 errors that are red on main regardless of any change. .github/workflows/ci.yml omits it for the same reason. The deviation is documented in the feature's spec.md.

Coverage

Repo-wide line 85.89% -> 85.92%, branch 79.34% -> 79.36%. New non-exempt types: EngineToggleStateCoordinator.cs 99.15% (133/135) and EngineToggleCatalog.cs 100.00%, against the 90% new-code floor.

RibbonViewer and RibbonController carry [ExcludeFromCodeCoverage] under the ratified VSTO/COM ribbon-handler exemption, so the modified handlers add little coverage surface and a nearly flat repo-wide figure is the expected outcome, not a regression. The exemption was neither removed nor widened; it was empirically confirmed still honored (both files are absent from the Cobertura document). All extracted logic is host-neutral and tested.

Review artifacts

policy-audit, code-review, and feature-audit (all 2026-08-08T21-59) are committed under the feature folder. All three: PASS, 0 blocking findings.

Twenty-two of 23 acceptance criteria are delivered and checked off. AC-22 is PENDING-MANUAL by design: VSTO callback binding cannot be observed outside a live Outlook process — which is precisely why #505 went undetected — so a maintainer checklist is committed at evidence/manual-verification/ac22-checklist.2026-08-08T21-44.md rather than the criterion being checked off on the strength of unit tests.

Known limitation shipped deliberately

The review raised one Major, non-blocking finding, tracked as issue #525 rather than fixed here: in EngineToggleStateCoordinator.ApplyPrimeAsync, an in-flight prime that read engine state before a toggle flipped it can land afterwards and overwrite the fresher toggle-written value, and the retained prime marker then prevents a re-prime — so a stale toggle display can persist until the next click.

Stated plainly rather than buried: this ships a known display-only race in newly added code. It was promoted rather than fixed because it is display-only (the underlying configuration is always correct), the window is narrow, it self-corrects on the next click, it violates no acceptance criterion, and it is strictly better than the merge-base behavior in which the toggles never reflected engine state at all. The fix is one line (TryAdd instead of the indexer, invalidating only on a successful add) plus tests at an already-established seam; pull it forward into this PR if you would rather not merge with it open.

Out of scope

Held to three issues. Everything else found was promoted, not fixed:

• Issue Bug: ribbon-controller-intelligence-unguarded-globals-deref #524 (new) — unguarded Globals dereferences in RibbonController.Intelligence.cs, the same defect class as Bug: Bug: ribbon-engines-callers-unguarded-null-deref #518 but at different call sites.
• Issue Bug: engine-toggle-prime-last-writer-race #525 (new) — the coordinator race above, plus a silently-ignored canceled prime and two uncovered defensive-guard lines.
• Issue Bug: ribbon-dead-callback-names #504 (pre-existing) — orphan onAction callbacks in RibbonExplorer.xml.
• Issue Bug: winformspumphost-tests-load-flaky-visible-window #511 (pre-existing) — WinFormsPumpHost load flakiness.

Issue #522 (the defective nullable type-check command) is documentation-only and separately tracked; it is not addressed here.

User-visible change to call out

The six Spam/Triage save-options buttons (Network, Local, Current Location) now render disabled until their engine finishes loading, instead of being always enabled and silently doing nothing. They re-enable automatically after the post-load refresh.

🤖 Generated with Claude Code