Bug: Bug: ribbon-engines-callers-unguarded-null-deref

[drmoisan]

• Work Mode: minor-audit

Summary

All 11 production call sites of RibbonController.Engines dereference the result with no null
guard. Issue #507 changed Engines from Globals.Engines to Globals?.Engines so the property
returns null instead of throwing when Globals is unassigned, matching the sibling SB
precedent. That fix is correct and is the behavior #507 specified, but on its own it relocates the
NullReferenceException rather than eliminating it: the same ribbon click now throws one frame
later, at the call site, instead of inside get_Engines().

Discovered during the feature review of #507 (bug/ribbon-controller-engines-null-unsafe-507).

Environment

• OS/version: Windows 11, Outlook desktop (VSTO add-in host)
• Runtime: .NET Framework 4.8.1, TaskMaster VSTO add-in
• Data source or fixture: Live Outlook profile during add-in startup

Steps to Reproduce

1. Reload the TaskMaster add-in so the ribbon is constructed before the controller's Globals is
   assigned.
2. Invoke any of the callbacks listed above.
3. Observe a NullReferenceException raised at the call site rather than inside get_Engines().

Expected Behavior

Each callback guards the Engines result and degrades gracefully when the engines are not yet
available, rather than dereferencing null.

Actual Behavior

Every call site dereferences Controller.Engines immediately with no guard.

Logs / Screenshots

(not provided in potential file)

Impact / Severity

• Blocker
• High
• Medium
• Low

Same narrow reachable window as #507: the callback must run before SetGlobals. The affected
callbacks are configuration submenu items rather than primary commands.

Source

From: docs/features/potential/2026-08-08-ribbon-engines-callers-unguarded-null-deref.md

[drmoisan]

Restated after #503 merged (PR #515)

The original report was written against RibbonViewer.cs at merge base 003c5715. #503 has since merged and relocated this code, so the finding needs restating. Verified against main at 2fe930f5:

The call sites moved. They now live in TaskMaster/Ribbon/RibbonViewer.EngineCommands.cs, not RibbonViewer.cs.

The count is now 10, not 11. TestSpam_Click (line 100) is now wrapped in Controller.RunEngineCommandAsync(...), so its Controller.Engines dereference only executes after EngineReadinessGate reports ready. It is no longer unguarded and is excluded from this issue.

Remaining unguarded sites (10), all in RibbonViewer.EngineCommands.cs:

Line	Callback
120	SpamBayesEnabled_Click
123	SpamBayesEnabled_GetPressed
126	SpamSaveNetwork_Click
129	SpamSaveLocal_Click
132	GetSaveLocation_Click
189	TriageEnabled_Click
192	TriageEnabled_GetPressed
195	TriageSaveNetwork_Click
198	TriageSaveLocal_Click
201	TriageGetSaveLocation_Click

These are exactly the Spam Config and Triage config submenu callbacks. Each dereferences Controller.Engines.<member> directly.

Sequencing constraint is now satisfied. This issue was blocked on #503, which has merged. It is ready to be worked.

Suggested approach. #503 already established the mechanism: route these ten through the existing RunEngineCommandAsync / EngineGatedCommandRunner gate rather than adding ten ad-hoc null checks. Note that four of them (SpamBayesEnabled_Click/_GetPressed, TriageEnabled_Click/_GetPressed) are also the subject of #505 (async GetPressed signature) and #506 (fire-and-forget toggle), so all three issues touch the same methods and should be addressed together.

Related: #507 (PR #519) makes RibbonController.Engines itself null-safe, which is what surfaced this. That PR does not and cannot fix these call sites.