fix(store): state-specific readiness dialog copy (#287) - #716
Merged
drmoisan merged 14 commits intoSep 1, 2026
Conversation
Adds the active feature folder for GitHub issue #287 with the work-mode marker full-bug, the seeded spec, and the canonical plan file. Preparation output only; no production code is touched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Records the verified mapping of StoreLaunchReadinessState onto the store load pipeline. ModelUnavailable, not StoresUnavailable, is the state that carries the terminal case: AppOlObjects.LoadStoresAsync swallows a load exception and leaves the model null with no retry path, while StoresWrapper.RewireOlObjectsAsync assigns an empty list before its first await so a null Stores list is transient. Adds 16 numbered acceptance criteria, mirrors them into issue.md, and fixes the scope to both readiness call sites behind one pure helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds the research artifact establishing that the permanent case belongs to ModelUnavailable, not StoresUnavailable, corroborated by the existing regression test LoadStoresAsync_WhenDeserializeThrows_AbsorbsExceptionAnd LeavesStoresWrapperNull. Revises the spec to place the two message-selection methods on the existing StoreLaunchReadinessEvaluator rather than a new file. The non-SDK projects require an explicit Compile Include per source file and StoreWrapperController.cs has only 22 lines of headroom under the 500 line cap, so the chosen placement needs neither a new file nor a project file edit. Selects the dialog copy from the tone-constrained option set and restates the 16 acceptance criteria in both spec.md and issue.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Six phases, 54 tasks, all 16 acceptance criteria traced. Passes the MCP plan validator with no warnings. Held for orchestrator mirror review before executor preflight. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds decision D16 and rescopes the AC9 viewer-construction assertion to the two test files this change edits. A repository-wide zero-hit gate for that token was unsatisfiable: the token already occurs nine times elsewhere in UtilitiesCS.Test. Also removes bracketed and interpolation markers from asserted literals so the acceptance-gate checkable-literal guard does not skip them, and repairs malformed command text in the baseline coverage task. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Adds the mandatory SELF-REVIEW: RE-DERIVED THIS PASS declaration with its citation enumeration, which the previous planning pass never emitted. Repairs three acceptance conditions that could not fail or could not hold. P4-T2 and P5-T19 required an empty porcelain status while the tasks themselves write uncommitted evidence artifacts. P5-T6 asserted a control count of four that no task determined; the re-derivation found the literal occurs twice today, both production and both removed by Phase 2, so the bound is lowered to one occurrence that P1-T1 creates. P3-T6 required that no file change after the formatting pass while later gates write evidence by design. Restates D7, whose claimed StrictMode defect the re-derivation did not support, as a conservative choice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
StoreWrapperController.cs is 478 lines against the 500-line cap, so the headroom is 22 lines, not 21. Found during preflight round 1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Applies all ten defects enumerated by executor preflight. Three were blocking. The material one: Invoke-MSTestWithCoverage.ps1 throws before its post-processing step, so a non-green suite leaves a raw Cobertura file whose denominator differs from a processed one. The plan had tolerated a non-empty baseline failure set, which would have made the baseline and post-change line rates incomparable. Baseline is now required green with an explicit halt clause. Also adds the scoped formatter fallback to the final check gate, asserts the title literal left both controllers and that each evaluator call is present in each, adds a test-discovery count control so an absent test can no longer pass an absence-based gate, records rather than asserts the execution branch name, relaxes the class coverage clause to match AC14, asserts branch coverage, probes all four analyzer package trees, and corrects a false character-count justification. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
… path Captures the remainder of the planner's revision, which landed after the previous commit snapshotted a partial write. Corrects one relayed delta rather than applying it literally. The delta named .config/dotnet-tools.json as the CSharpier pin; that path does not exist in this repository. The manifest is dotnet-tools.json at the root, which is what CLAUDE.md names and what git tracks. Asserting a match against a non-existent file would have made the P0-T5 acceptance unverifiable. Adds three citations, extends the self-review enumeration to 22 entries, and records that the two test runners build identical inner vstest argument tails, which is what makes the cross-runner test-count control in P3-T5 a like-for-like comparison. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Round 2 found three defects that round 1's own fixes had created or left unreached. P5-T15 demanded an uncovered-line list its command never printed, and counted lines with a descendant traversal. This repository's Cobertura output emits each line twice per class, once under methods and once at class level: for the evaluator class the descendant form returns 25 while the class-level form returns 13. The command now prints both a count and the line numbers, scoped with SelectNodes to the class-level element. P0-T13 was the sibling the round-1 baseline fix did not reach. It gated on neither exit code nor an empty failed set, so a non-green instrumented run could leave a raw Cobertura file whose four root attributes still parse, and the later comparison would then difference two denominators. It now requires exit zero, the two post-processing stdout literals, an empty failed set, and a test total matching the baseline. Corrects the character-count justification, which asserted 87 characters for both declarations; the title overload is 85. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Resumes the interrupted preparation run. The prior attempt recorded its clearance only in the untracked checkpoint, so it did not survive; a search of this feature folder for the signal returned nothing. An adversarial self-review over a transitive citation scope found five gaps in files the mandated edits force the toolchain to touch but the plan did not name. P0-T4 checked four analyzer package trees, while both edited projects also wire SonarAnalyzer 10.33.0.1635 and the solution build needs MSTest.Analyzers 4.3.3; a missing Analyzer Include path is CS0006, and EnsureNuGetPackageBuildImports guards Import targets only, so the task could report four passing checks and still fail the next gate. The 169-character message drives MyBoxViewer.GrowTextbox through the TextChanged subscription, a runtime path in an uncited file, now recorded as D17. Added the LangVersion citation admitting the switch expression, the BannedSymbols wiring behind the test constraint, and a qualification that the no-viewer-construction claim is token-level only. Preflight round 1 then found five more. Three are the same class: an acceptance condition that cannot fail or cannot pass. The artifacts demanded four vstest summary counts, but a green run prints only Total tests and Passed. The viewer-construction absence assertion had no positive control. The QC restart clause fired on iteration one by construction, because the first formatter pass over hand-written edits always rewrites and the discard arm is 193 columns against a 100-column width. Round 2 confirmed all ten closed with no new findings. Two figures supplied to the planner were wrong and were corrected against measurement: the message is 169 characters, not 163, and the literal it replaces is 79, not 76. Preparation only. No plan phase executed and no product code changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Add policy-audit, code-review, and feature-audit artifacts (all PASS, zero blocking findings) and check off the plan's P5-T19 commit task, per the plan's own documented check-off-fixpoint residual. Also commits feature-review's own agent-memory updates from this run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggested title
fix(store): state-specific readiness dialog copy (#287)
Summary
StoreWrapperController.Launch()andDisabledStoresController.Launch()showed one hardcoded dialog for every non-ready readiness state, asserting unconditionally that "retrying after startup completes" would resolve the condition — untrue for theModelUnavailablestate, which is permanent for the rest of the session once the store load has failed.internal staticmethods onStoreLaunchReadinessEvaluator(BuildUnavailableMessage,BuildUnavailableTitle) that mapStoreLaunchReadinessStateto state-specific copy, and rewires both call sites to use them instead of inline literals.ModelUnavailablenow reads: "Store settings are not available. Retry once startup has completed; if the message persists, the store settings failed to load and the application log records the cause."StoresUnavailablenow reads: "The store list has not finished loading. Please try again shortly." with title "Store Settings Loading".DisabledStoresController.LaunchXML doc claim that it "shows the same warning as the single-store editor" — it no longer does, by design.[ExcludeFromCodeCoverage], gate condition,MessageBoxButtons.OK/MessageBoxIcon.Warning, no dialog on ready) is unchanged..csproj/.props/.targets/packages.configchange.Why
StoreLaunchReadinessStatehas three members (Ready,ModelUnavailable,StoresUnavailable), but both non-ready states collapsed onto identical dialog copy that told the user to retry once startup completed. That is correct advice forStoresUnavailable(a transient, still-loading condition) but incorrect forModelUnavailable, which — perTaskMaster/AppGlobals/AppOlObjects.StoreLoading.cs:66-72— is the terminal state for the remainder of the Outlook session once the store load has failed inside its catch block. A user seeingModelUnavailablewas told to do something (wait and retry) that provably cannot fix their situation, which is the defect this PR corrects: distinct, state-accurate copy for each non-ready state.What Changed
Core logic (
UtilitiesCS)OutlookObjects/Store/StoreLaunchReadinessEvaluator.cs: addsBuildUnavailableMessage(StoreLaunchReadinessState)andBuildUnavailableTitle(StoreLaunchReadinessState), both pure, both throwingArgumentOutOfRangeExceptionforReady, both returning theModelUnavailablecopy for any undefined enum value (conservative default). Extends the class XML doc to documentModelUnavailablepermanence and cites the terminal catch block.OutlookObjects/Store/StoreWrapperController.cs:Launch()now obtains both dialog strings from the evaluator instead of inline literals.OutlookObjects/Store/DisabledStoresController.cs: same wiring change; XML summary corrected to drop the stale "same warning as the single-store editor" claim.Tests (
UtilitiesCS.Test)OutlookObjects/Store/StoreWrapperController_Tests.Launch.cs: 9 new tests for the two pure evaluator methods (message/title selection per state, exception onReady, undefined-cast fallback, non-ready messages differ), plus 2 extended existing wiring tests and 1 new wiring test asserting the two non-ready dialogs show different copy.OutlookObjects/Store/DisabledStoresControllerTests.cs: 2 new wiring tests confirmingLaunch()shows the state-specific copy and leavesViewernull for both non-ready states.Docs / evidence
issue.md,spec.md(authoritative AC source, v2.0), research artifact, atomic plan (all 47 tasks complete), and Phase 0/regression/QA-gate evidence artifacts, pluspolicy-audit,code-review, andfeature-auditreview artifacts (all PASS).Architecture / How It Fits Together
StoreLaunchReadinessEvaluatoris aninternal staticclass already shared between the two controllers viaEvaluate(...). This change adds two more pure, stateless static methods to the same class rather than introducing a new type or touchingEvaluateitself (Evaluate's return values are unchanged and covered by AC12). BothLaunch()methods keep their existing shape — gate onreadiness.State != Ready, callMyBox.ShowDialog(...)withMessageBoxButtons.OK/MessageBoxIcon.Warning, return without constructing a viewer — and only the two literal arguments passed toMyBox.ShowDialogchange, now sourced from the evaluator. No new dependency, no new file, no change to theMyBox/MyBoxViewerdialog machinery itself.Verification
Completed (independently verified by feature-review, not merely self-reported by the implementing agent):
dotnet tool run csharpier check ., 1565 files, exit 0)./t:Rebuild /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true): exit 0, 0 errors, 5 pre-existingSystem.Reactivepackages.config advisory warnings (unrelated, pre-existing)./t:Rebuild /p:TreatWarningsAsErrors=true): exit 0, 0 errors, same 5 pre-existing warnings.Invoke-MSTestWithCoverage.ps1, 9 test assemblies): 6912/6912 passed, 0 failed.BuildUnavailableMessage/BuildUnavailableTitle) at 100% line-rate.evidence/regression-testing/fail-before-wiring-tests.md,pass-after-wiring.md).origin/mainmerge-base: exactly the 5 files listed above changed outside the feature folder; zero change to.claude/rules/or.github/instructions/.spec.md(v2.0) verified and checked off.Recommended (for reviewer, not run as part of this change):
Backward Compatibility / Migration Notes
No breaking changes. No public API surface changes outside the two new
internal staticmethods (internal, not part of any external contract). No.csproj/.props/.targets/packages.configedits. No behavior change to the dialog-wiring contract other than the copy shown.Risks and Mitigations
ModelUnavailablemessage (169 characters) is substantially longer than the literal it replaces (79 characters) and drivesMyBoxViewer.GrowTextbox's text-measurement and resize path at runtime. Mitigated: the two full-suite MSTest runs in this change (fail-before and pass-after) both exercise that path via the realMyBoxViewerconstruction insideMyBox.ShowDialog, and both completed without hang or exception.spec.md:297-300. If the maintainer prefers different guidance, only the two literals and their corresponding test assertions change — no structural rework needed.Review Guide
Suggested order:
UtilitiesCS/OutlookObjects/Store/StoreLaunchReadinessEvaluator.cs— the two new pure methods (small, easy to verify in isolation).UtilitiesCS/OutlookObjects/Store/StoreWrapperController.csandDisabledStoresController.cs— the two call-site rewires (each a 1-2 line diff).UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.Launch.csandDisabledStoresControllerTests.cs— the new/extended tests.spec.md,plan.2026-08-31T20-56.md, review artifacts) for full traceability, if a deeper audit trail is wanted.The large file count in this PR is almost entirely feature-folder documentation and evidence (spec, plan, research, and per-acceptance-criterion evidence artifacts); the code diff itself is small (5 files, ~400 lines, mostly new tests).
Follow-ups
Not promoted to a new issue from this branch, because this PR carries a footprint acceptance criterion (AC16) restricting the diff to the five files above plus the feature folder. Recorded here for a maintainer to consolidate into a follow-up issue after merge:
StoreWrapperControlleris entirely absent from the Cobertura coverage report in both the baseline and post-change XML, despite only 2 of its members carrying[ExcludeFromCodeCoverage]. This is pre-existing and not introduced or fixed by this branch.spec.mdby thenet48/ no-IsExternalInitconstraint (norecord/const-friendly shared literal type is available).spec.md's own already-documented open item:ModelUnavailablestill conflates two distinct root causes (a failed load vs. an undefined/future readiness state); resolving that would require a new readiness state and crosses theUtilitiesCS/TaskMasterassembly boundary, and is intentionally out of scope for this bug fix.spec.md:297-300's open item on whether pointing the user at the application log is the maintainer's preferred support-path guidance (see Risks above).Also open, unrelated to this PR's scope but noted by review for maintainer awareness: this repository's coverage-floor policy is internally inconsistent —
CLAUDE.mdstates an 80%/90% floor with COM/VSTO exemptions, while.claude/rules/quality-tiers.mdand.claude/rules/general-unit-test.mdstate a uniform 85% line / 75% branch floor with no tier relief. This PR's measured coverage (85.297%/79.293% repo-wide, 100%/100% new code) clears both readings, so the conflict does not affect this PR's verdict, but it may bite a future borderline change.GitHub Auto-close