fix(#507): guard Globals in RibbonController.Engines

[drmoisan]

fix(#507): guard Globals in RibbonController.Engines

Summary

• RibbonController.Engines dereferenced Globals with no null guard, so any ribbon callback reaching it before SetGlobals had run threw NullReferenceException. One-line fix: Globals.Engines becomes Globals?.Engines, matching the sibling precedent already in the same file.
• Adds two MSTest regression tests: one reproducing the defect (verified failing pre-fix), one pinning the assigned-Globals path by reference identity so it proves forwarding rather than a null-to-null coincidence.
• The tests live in a new partial-class file RibbonControllerTests.Engines.cs because appending them inline pushed RibbonControllerTests.cs to 513 lines, past the repository's 500-line cap. Feature review flagged this as blocking; the split restores 452 lines and adds a 73-line sibling.
• Corrects a doc comment in RibbonController.EngineCommands.cs that this fix falsifies. That file arrived with Bug: ribbon-engine-readiness-guard #503 and asserts the Engines property "is not null-safe on Globals" — true before this PR, false after. Comment-only; the readiness gate's behavior is unchanged.
• Full toolchain passes in a single clean pass on the rebased head: 6397/6397 tests, 0 failures, 0 analyzer errors, 0 nullable errors, 0 formatting diffs.

Why

TaskMaster/Ribbon/RibbonController.Intelligence.cs declared:

internal IAppItemEngines Engines => Globals.Engines;

Globals is protected internal ApplicationGlobals Globals { get; set; } and stays null until SetGlobals runs. Every sibling accessor in the same file already guards it — the SB property uses Globals?.Engines?.InboxEngines?.... Engines was the lone unguarded outlier, so it threw where its siblings returned null:

System.NullReferenceException: Object reference not set to an instance of an object.
   at TaskMaster.RibbonController.get_Engines()

Severity is Low: the reachable window requires the callback to run before SetGlobals, and the affected callbacks are configuration submenu items rather than primary commands. It is nevertheless a real inconsistency and an avoidable throw.

What Changed

Core fix — TaskMaster/Ribbon/RibbonController.Intelligence.cs (+1/-1)

- internal IAppItemEngines Engines => Globals.Engines;
+ internal IAppItemEngines Engines => Globals?.Engines;

Tests — TaskMaster.Test/Ribbon/RibbonControllerTests.Engines.cs (new, +73)

• Engines_WhenGlobalsNotAssigned_ReturnsNullInsteadOfThrowing — constructs a bare new RibbonController() without setting Globals, asserts reading Engines does not throw and yields null.
• Engines_WhenGlobalsAssigned_ReturnsGlobalsEngines — assigns a Mock<IAppItemEngines> instance onto Globals.Engines via reflection and asserts BeSameAs, so the test fails if the property ever stops forwarding.

Supporting — RibbonControllerTests.cs (+1/-1, class marked partial), TaskMaster.Test.csproj (+1, registers the new file; this project is legacy non-SDK and does not glob *.cs).

Comment correction — TaskMaster/Ribbon/RibbonController.EngineCommands.cs (+8/-5). This file arrived on main with #503 while this PR was open. Its XML remarks explained that the readiness accessor reads Globals?.Engines directly because "The existing RibbonController.Engines property is deliberately NOT used as the accessor because it is not null-safe on Globals." That was accurate against main at the time and is falsified by this PR. The remark now describes the direct read as a deliberate decoupling and records that the property is null-safe as of #507. No code path changes — the accessor, the gate, and the separate SB/Triage synchronization-context rationale are all untouched.

Documentation — the remaining 38 changed files are the feature folder (issue, plan, remediation plan, both audit rounds, evidence artifacts), two agent-memory notes, and the promoted-issue document for #518.

Architecture / How It Fits Together

RibbonController is the VSTO ribbon's controller half; RibbonViewer holds the Office callback surface and calls through to it. Engines is one of several accessors projecting ApplicationGlobals state to those callbacks. The fix makes that projection uniform with its siblings: during the pre-SetGlobals window every accessor now yields null rather than one of them throwing.

Verification

Completed (run against the final head, single clean pass, in policy order):

Stage	Command	Exit	Result
Format	csharpier check .	0	1512 files, 0 reformatted
Analyzers	msbuild TaskMaster.sln /t:Build /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true	0	0 errors
Nullable	msbuild TaskMaster.sln /t:Rebuild /m /p:Configuration=Debug "/p:Platform=Any CPU" /p:TreatWarningsAsErrors=true	0	0 errors
Tests	vstest.console.exe <9 assemblies> /EnableCodeCoverage /InIsolation /TestCaseFilter:"TestCategory!=LiveOutlook"	0	6397 total, 6397 passed, 0 failed

This run is against the rebased head. After this PR opened, main advanced (#515 merged bug/ribbon-engine-readiness-guard-503, #514 merged the QuickFiler keystroke fix) and the PR went CONFLICTING. The branch was rebased onto 2fe930f5; the only conflict was the shared .claude/agent-memory/feature-review/MEMORY.md index, resolved by union. The toolchain was re-run from scratch on the rebased head rather than carrying the pre-rebase result forward. The test total rose from 6295 to 6397 because #503 and #514 brought their own tests onto main.

Both new tests pass by name. The regression test was verified failing against the pre-fix source with NullReferenceException before the fix was applied.

A note on the nullable stage. CLAUDE.md documents this stage with /p:Nullable=enable, but .github/workflows/ci.yml deliberately omits that flag and relies on each file's own #nullable enable pragma. Adding the flag force-enables nullable analysis across thousands of never-annotated files and yields ~414 errors that are red on main too. The changed file carries no #nullable pragma, so the CS8603 that the forced flag surfaces never reaches CI; the command CI actually runs returns 0 errors with this change applied. Full reasoning, including why a ! or IAppItemEngines? annotation was rejected, is in evidence/qa-gates/phase2-orchestrator-ci-gate-reconciliation.md. The CLAUDE.md-vs-ci.yml divergence is a real documentation defect, reported separately.

Recommended for a reviewer reproducing locally: run the four commands above in order from the repository root.

Backward Compatibility / Migration Notes

No breaking changes. Engines keeps its signature and its behavior whenever Globals is assigned; only the previously-throwing unassigned path changes, and it now returns null in line with its siblings. No public API, no renamed paths, no removals.

Risks and Mitigations

• The null return moves the failure rather than removing it. Ten production call sites still dereference Controller.Engines.<member> without a guard, so a click in the pre-SetGlobals window still throws — one frame later, at the call site. This is tracked as Bug: Bug: ribbon-engines-callers-unguarded-null-deref #518 and is deliberately not fixed here. Post-rebase the picture improved slightly: Bug: ribbon-engine-readiness-guard #503 relocated these to RibbonViewer.EngineCommands.cs and routed TestSpam_Click through its new RunEngineCommandAsync gate, so that one is now safe; the ten remaining are the Spam Config and Triage config submenu callbacks. Bug: Bug: ribbon-engines-callers-unguarded-null-deref #518 has been updated with the corrected file, count, and line numbers. This is not a regression — the sibling SB property already exhibits the identical unguarded-caller pattern on main, and Bug: ribbon-engine-readiness-guard #503's EngineGatedCommandRunner is the established mechanism for fixing them.
• Coverage. RibbonController is [ExcludeFromCodeCoverage] under the ratified VSTO/COM ribbon-handler exemption, so this change adds no coverage surface. The exemption is neither removed nor widened. Test pass counts strictly improved (+2, 0 failures).
• Rollback is a one-line revert of the production change plus removal of the new test file and its csproj entry.

Review Guide

1. TaskMaster/Ribbon/RibbonController.Intelligence.cs — the entire behavioral change, one line.
2. TaskMaster.Test/Ribbon/RibbonControllerTests.Engines.cs — the two new tests.
3. TaskMaster/Ribbon/RibbonController.EngineCommands.cs — comment-only correction of a rationale that this PR falsifies.
4. TaskMaster.Test/Ribbon/RibbonControllerTests.cs and TaskMaster.Test.csproj — mechanical: one partial keyword and one <Compile Include> line.
5. Everything else is documentation and evidence.

Two housekeeping notes for reviewers. The branch was rewritten before this PR opened to drop two raw Cobertura dumps (37 MB + 44 MB, ~1.42M lines) that had been committed as evidence, following the convention set by d0955dc4; insertions dropped from 1,419,897 to 2,254 and the numeric coverage headlines are retained in the markdown artifacts. The remediation-cycle narrative therefore lives in the committed audit artifacts rather than in commit granularity.

Follow-ups

• Bug: Bug: ribbon-engines-callers-unguarded-null-deref #518 — the 10 remaining unguarded Controller.Engines call sites in RibbonViewer.EngineCommands.cs. Its blocking dependency on bug/ribbon-engine-readiness-guard-503 is now satisfied (fix(#503): guard engine-backed ribbon commands against the InboxEngines initialization race #515 merged), so it is ready to be worked. Consider bundling with Bug: ribbon-async-getpressed-signature #505 and Bug: ribbon-toggle-engine-fire-and-forget #506, which touch four of the same methods.
• The CLAUDE.md vs ci.yml nullable-command divergence, reported separately for triage.

GitHub Auto-close

• Closes Bug: ribbon-controller-engines-null-unsafe #507