fix(#503): guard engine-backed ribbon commands against the InboxEngines initialization race

[drmoisan]

Summary

• Guards the eight engine-backed Outlook ribbon commands against the AppItemEngines.InitAsync() initialization race, so a click during add-in startup no longer throws.
• Introduces a per-engine readiness signal computed from the existing InboxEngines dictionary, implemented in four new host-neutral types that are unit-testable and not coverage-exempt.
• Wires the Office getEnabled callback to eight <button> controls and invalidates them once initialization completes, so the buttons report disabled until their backing engine exists.
• Adds a defense-in-depth click guard that defers the engine dereference into a lambda, so a blocked command is a no-op with a user-facing notification rather than an exception.
• AppItemEngines.cs and IAppItemEngines.cs are unchanged (zero-line diff), preserving the async engine construction, config loading, and dictionary population order exactly.
• Four new types at 100% line coverage; repo-wide line coverage 85.86% and branch coverage 79.27%, both up from baseline.

Why

RibbonController.SB resolves the SpamBayes engine by looking up the "Spam" key in Globals.Engines.InboxEngines. That ConcurrentDictionary is created empty at field-initializer time and is only populated at the very end of AppItemEngines.InitAsync(), which first awaits Globals.AF.Manager.Configuration and then asynchronously constructs each engine.

Clicking a ribbon button inside that window — most easily reproduced immediately after an add-in reload — makes SB return null, and RibbonViewer.TrainSpam_Click then executes await Controller.SB.TrainAsync(...), throwing NullReferenceException. Because the handler is async void, the exception surfaces on the message-pump synchronization context rather than at the call site.

Investigation established the defect surface is broader in one direction and narrower in another than the original report:

• TestSpam_Click fails with KeyNotFoundException, not NullReferenceException, because it indexes the dictionary directly rather than going through SB.
• The affected set is exactly eight handlers — Spam x3 (TrainSpam_Click, TrainHam_Click, TestSpam_Click) and Triage x5 (TriageSetA/B/C_Click, ClearTriage_Click, FilterViewer_Click). The original report's claim that the Project, Context, and Actionable classifiers are affected was checked against the call graph and is not correct: no ribbon callback dereferences those engines.

A coarse IsInitialized flag or InitTask on AppItemEngines/IAppItemEngines was considered and rejected on two grounds. First, it is wrong on the merits: InitAsync filters on config.Value.Engine, so an engine that is configured off never enters the dictionary at all, and a global "initialized" flag would report ready for a command that will never work. Second, .NET Framework 4.8.1 has no default interface members, so a new IAppItemEngines member could only be bodied inside AppItemEngines, which carries [ExcludeFromCodeCoverage] — the readiness logic would have been untestable by construction.

What Changed

Core fix — four new host-neutral decision types

All are internal sealed under TaskMaster\Ribbon\, contain zero Microsoft.Office.* references, and deliberately carry no [ExcludeFromCodeCoverage] attribute.

• EngineCommandCatalog.cs — the single source of truth binding the eight ribbon control ids to their engine keys. Centralizing it is what allows one test to assert the ribbon XML and the code agree.
• EngineReadinessGate.cs — the readiness predicate. Returns false for a null engines accessor, a null InboxEngines, a missing key, a null value, or a null/whitespace engine name. Readiness is recomputed on every query, so it tracks both initialization and RestartEngineAsync.
• EngineGatedCommandRunner.cs — the click guard and the getEnabled decision. Callers pass a lambda, so the engine is dereferenced only inside that lambda and never evaluated when the gate is closed. This is what converts the exception into a no-op without scattering ?. across the viewer.
• EngineCommandRefreshPlanner.cs — decides which controls to invalidate, keeping that decision covered while the IRibbonUI call stays in the uncovered shim.

Thin Office-typed shims

• RibbonController.EngineCommands.cs (new partial) — builds the gate as () => Globals?.Engines, which is null-safe before SetGlobals runs.
• RibbonViewer.EngineCommands.cs (new partial) — hosts EngineCommand_GetEnabled, the only new Office-typed member introduced by this change, plus the relocated Spam and Triage callbacks rewritten to route through the gated runner.
• RibbonViewer.cs — becomes partial; the #region Spam Manager and #region Triage blocks move into the new partial. This is required, not cosmetic: the file was at 487 of the 500-line cap, so the new callbacks could not be added in place. Net −99 lines.
• ThisAddIn.cs — one refresh call after await _globals.LoadAsync(false), already on the STA via the idle queue.
• RibbonExplorer.xml — getEnabled="EngineCommand_GetEnabled" on the eight engine-backed buttons.
• Both .csproj files gain explicit <Compile Include> entries; these are legacy non-SDK packages.config projects, so new files are not auto-included.

Tests

Four new MSTest classes plus an extension to RibbonExplorerXmlTests.cs, using MSTest + Moq + FluentAssertions. No temporary files, no Thread.Sleep/Task.Delay, no Form, MessageBox, message pump, or live COM.

Architecture / How It Fits Together

Office calls EngineCommand_GetEnabled(IRibbonControl) on the COM-visible RibbonViewer shim. That forwards to RibbonController.IsEngineCommandEnabled(controlId), which asks EngineGatedCommandRunner, which maps the control id to an engine key through EngineCommandCatalog and asks EngineReadinessGate whether that key is present and non-null in the live InboxEngines dictionary.

The same runner backs the click path: each of the eight handlers now calls RunEngineCommandAsync(controlId, () => <original expression>). The original expression is unchanged and is evaluated only when the gate is open.

When InitAsync() completes, ThisAddIn calls RefreshEngineCommands(), which marshals to the UI dispatcher and invalidates the eight control ids so Office re-queries getEnabled.

All decision logic is host-neutral and would port unchanged if the ribbon is later replaced by an Office.js command surface. The only Office-typed surface added is one method on the pre-existing [ComVisible(true)] [ExcludeFromCodeCoverage] RibbonViewer.

Verification

Completed

• CSharpier: csharpier check . — exit 0, 1498 files, empty unformatted set.
• .NET analyzers: msbuild TaskMaster.sln /t:Build /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true — exit 0, 0 errors.
• Nullable: msbuild TaskMaster.sln /t:Build /p:Nullable=enable /p:TreatWarningsAsErrors=true — exit 0.
• Tests: 6338 total, 6338 passed, 0 failed, 0 skipped.
• Coverage: repo-wide line 85.8561% (floor 85), branch 79.2702% (floor 75), both improved against the merge-base baseline. All four new types at 100% line coverage.
• The TaskMaster package coverage counter gained exactly 186 covered lines — the sum of the four new types (48+48+72+18) — with missed byte-identical, which independently establishes both the new-code floor and no regression on changed lines.
• Zero-line diff verified on AppItemEngines.cs, IAppItemEngines.cs, and ApplicationGlobals.cs.
• The ribbon-XML getEnabled assertion was proven non-vacuous by removing one attribute from the embedded resource, confirming the mutation reached the built assembly, observing the test fail, and restoring.

Recommended

• Manual live-Outlook verification is required before merge and is tracked in the feature folder checklist. Three acceptance criteria cannot be satisfied by automated tests: clicking the eight commands during the initialization window, confirming unchanged behavior after initialization completes, and confirming Office visually greys and re-enables the controls. The last of these is the only check that can prove the getEnabled callback is actually bound, since VSTO silently ignores a signature mismatch.

Backward Compatibility / Migration Notes

No breaking changes. No public API is altered, no member is added to IAppItemEngines, and no engine construction, configuration loading, or dictionary population order is modified. On the ready path the eight handlers evaluate expressions identical to the previous implementation.

Risks and Mitigations

• The getEnabled callback may not bind at runtime. VSTO ignores a signature mismatch silently, and no local test can prove binding. Mitigated by a reflection test pinning the signature, and more importantly by the click guard, which makes the original defect unreachable even if the enabled-state callback never fires. Rollback is limited to reverting the eight XML attributes.
• Notification presentation. The repository has no non-modal notification surface, so a blocked click logs and shows a MessageBox, following the existing pattern in RibbonViewer/RibbonController. Tests assert against an injected sink, so no dialog is constructed under test.
• RibbonExplorer.xml is 539 lines, over the 500-line cap. The file was already at 519 before this change; the growth is 8 required attributes plus 12 lines that CSharpier mandates, because a collapsed <button> carrying the new attribute is 116 characters against a default print width of 100. Splitting the resource is tracked separately rather than bundled here.

Review Guide

Suggested order:

1. TaskMaster/Ribbon/EngineReadinessGate.cs and EngineCommandCatalog.cs — the readiness contract and the id/key binding.
2. TaskMaster/Ribbon/EngineGatedCommandRunner.cs — the lambda-deferral mechanism that is the actual fix.
3. TaskMaster/Ribbon/RibbonViewer.EngineCommands.cs — confirm the eight rewritten handlers preserve their original expressions.
4. TaskMaster/Ribbon/RibbonExplorer.xml and RibbonExplorerXmlTests.cs — the wiring and the tests that pin it.
5. TaskMaster/ThisAddIn.cs — the single refresh call.

TaskMaster/Ribbon/RibbonViewer.cs shows 1 insertion and 100 deletions; this is almost entirely the mechanical region move into the new partial plus the partial keyword. Reviewing it against RibbonViewer.EngineCommands.cs side by side is the fastest path.

Follow-ups

Defects found during this work were promoted to their own issues rather than fixed here:

• Bug: ribbon-dead-callback-names #504 — five ribbon onAction callback names resolve to no method, leaving four Quick Filer settings check boxes inert.
• Bug: ribbon-async-getpressed-signature #505 — SpamBayesEnabled_GetPressed and TriageEnabled_GetPressed are async Task<bool> against the required synchronous getPressed contract.
• Bug: ribbon-toggle-engine-fire-and-forget #506 — SpamBayesEnabled_Click and TriageEnabled_Click discard the task returned by ToggleEngineAsync.
• Bug: ribbon-controller-engines-null-unsafe #507 — RibbonController.Engines lacks the null propagation its sibling properties use.
• Bug: wpf-dispatcher-yield-test-order-dependent #508 — YieldAsync_WithoutDispatcher_RemainsStrict is order-dependent and flakes under parallel execution.
• Bug: csharpier-documented-command-incompatible-with-pinned-version #509, Bug: utilitiescs-test-duplicate-percentageformattertests-compile-entry #510, Bug: winformspumphost-tests-load-flaky-visible-window #511 — CSharpier command/version mismatch, a duplicate compile entry in UtilitiesCS.Test.csproj, and a flaky WinForms pump-host test.
• Bug: nullable-gate-cannot-fail-incremental-build #512 — the documented nullable type-check gate cannot fail, because MSBuild skips recompilation when only /p: values change. A forced rebuild surfaces substantial pre-existing nullable debt, none of it in this change's files.

GitHub Auto-close

• Closes Bug: ribbon-engine-readiness-guard #503