fix(breadcrumb): enforce close, lifetime, broadcast and lease invariants (#501, #462, #500, #502) - #659
Merged
drmoisan merged 5 commits intoAug 28, 2026
Conversation
Committed by the epic-orchestrator parent, not by the feature's own child, to remove a data-loss risk: the child was killed by an API spend limit leaving 16 uncommitted entries, including two NEW files (BreadcrumbBridgeCoordinator.Suggestions.cs and BreadcrumbBridgeCoordinatorSupersessionTests.cs) that were one checkout away from being lost. No review, no toolchain pass, and no acceptance-criteria verification has been performed on this state. The resuming child owns validating, completing and if necessary amending it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ration' into bug/breadcrumb-coordinator-hub-defects-501
Completes the breadcrumb coordinator and messenger hub correctness work for issues #501, #462, #500 and #502 on top of the merged integration tip. - #462: replace the single _closePending flag with _closeInFlight and _closeCompleted so a reopened host can close again, without weakening the repeated-close suppression the two must-pass tests encode. - #500: narrow the TryRunCurrent lock scope so the guarded action, and any PostJson it reaches, no longer runs while the lifetime lock is held. - #501: broadcast to every attached surface before caching, and log a per-surface delivery failure through log4net instead of propagating it. - #502: make RunSynchronous return its skip verdict and have both callers consume it, so a superseded population settles its lease and replaces a stale SuggestionsUpgrade handle. Adds an internal AddItemsCore seam mirroring SetSuggestionsCore so the superseded-AddItems skip path is reachable from a deterministic test; this restores the SR-1 split pair to 100% line coverage and new/changed-line coverage to 100%. Repository line coverage moves from 85.1380% to 85.1448%. All four toolchain steps pass in one uninterrupted pass on the merged tree, using /t:Rebuild with zero skipped CoreCompile targets. Full suite: 6730 passed, 0 failed, 0 skipped. All 32 acceptance criteria are checked off against named evidence artifacts. Records two follow-ups as real issues rather than prose: #655 (non-re-entrant upgrade-lifetime guard, research 6.2 option C) and #656 (SR-4 _closeCompleted residual, owned by feature 488). Refs #501, #462, #500, #502
Feature review returned 0 Blocking findings across policy-audit, code-review and feature-audit (all 2026-08-27T23-48): 29 of 32 acceptance criteria PASS, 3 PARTIAL, 0 FAIL. No remediation cycle was opened. Disposes all five non-blocking findings: - NB-4 fixed in place: adds a committed run ledger transcribing the three full-suite runs verbatim, so the pass/fail counts and the pump-timeout flake argument rest on captured output rather than prose. - NB-5 fixed in place: the handoff index cited five artifact paths that did not exist; corrected, and all 93 cited paths verified present. - NB-1, NB-2 and NB-3 filed as issue #657 rather than left as folder prose, covering the source-level log assertion, the inert superseded-AddItems branch whose test does not discriminate it, and an uncovered CloseCore line. Refs #501
drmoisan
merged commit Aug 28, 2026
4cb709d
into
epic/quickfiler-bug-family-integration
5 checks passed
This was referenced Aug 28, 2026
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
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.
Summary
Fixes four ordering and lifetime defects in the QuickFiler breadcrumb coordinator and messenger hub. Each defect is a case of an invariant the code stated but did not enforce.
CloseCorenever cleared_closePendingon the successful-close path, so a laterRequestOpensilently returned the closed sentinel_closeInFlightand_closeCompleted, cleared on the paths that legitimately reopen the hostTryRunCurrentinvoked the guarded action inside_sync, so a WebView2 post ran under nested re-entrant locks and the currency check was not atomic with the actionPostJsonit reaches, runs outside the lifetime lockPostJsonwrote the replay cache before broadcasting and wrapped the broadcast in notry/catch, so one throwing surface starved later attachments while the cache recorded deliveryRunSynchronousdiscardedTryRunCurrent'sbool, so a superseded lease silently skipped the guarded action andSuggestionsUpgraderetained a stale handleRunSynchronousnow returns its skip verdict and both callers consume it; a superseded population settles its lease and replaces the stale handleBase
Targets
epic/quickfiler-bug-family-integration, notmain. The integration tip69e83171(which added merged siblings #493 and #444) is merged in; the branch is 0 behind.Changed files
Twelve files, all within this feature's
Viewers\Breadcrumb*ownership region:BreadcrumbBridgeCoordinator.cs,BreadcrumbBridgeCoordinator.Suggestions.cs,BreadcrumbCoordinatorUpgradeLifetime.cs,BreadcrumbDropDownOpenCoordinator.cs,BreadcrumbMessengerHub.csBreadcrumbBridgeCoordinatorSupersessionTests.cs,BreadcrumbCoordinatorUpgradeLifetimeTests.cs,BreadcrumbDropDownOpenCoordinatorTests.Part2.cs,BreadcrumbMessengerHubTests.cs,BreadcrumbSelectorCoordinatorTests.cs<Compile Include>line added to each, both underViewers\Breadcrumb*. Sibling Bug: uithread-dispatcher-static-swap-no-restore #493's two entries and sibling Bug: kbdactions-enumerable-ctor-bypasses-duplicate-guard #444's ten entries are preserved verbatim and unmoved.BreadcrumbBridgeCoordinator.csshows as a pure 50-line deletion. That is the SR-1 partial split, not lost content: the base never touched anyViewers/Breadcrumb*file, and the split pair retains 295/295 covered lines.Toolchain
All four steps passed in one uninterrupted pass on the merged tree, using
/t:Rebuild(never/t:Build) with 0Skipping target "CoreCompile"lines, so the analyzer and nullable gates were live rather than skipped.dotnet tool run csharpier format ./check .msbuild TaskMaster.sln /t:Rebuild /m ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=truemsbuild TaskMaster.sln /t:Rebuild /m ... /p:TreatWarningsAsErrors=trueInvoke-MSTestWithCoverage.ps1full suiteCoverage
All four per-file deltas are at or above 0.00 pp. Coverage moved up, not down.
Review
Feature review returned 0 Blocking findings: 29 of 32 acceptance criteria PASS, 3 PARTIAL, 0 FAIL. All 32 criteria in
spec.mdare checked off against named evidence artifacts, and all 115 plan tasks are complete. 93 evidence artifacts are indexed in the handoff index, every one verified present.Five non-blocking findings were dispositioned. Two were documentation defects fixed in place (a dangling-path table, and full-suite run counts that rested on prose rather than captured output — now backed by a committed run ledger). Three were filed as #657 rather than left as folder prose.
Deviations worth a reviewer's attention
log4netreference to justify verifying AC-11's logging half at source level. That premise is false — the reference is atQuickFiler.Test.csproj:209-210. The plan was amended to record the true reason (file budget) rather than write a false claim into the audit trail. The review judged the replacement reason still somewhat overstated; that is tracked as Refactor: breadcrumb-501-review-residuals #657 R-1.AddItemsCoreseam was added mid-run. The coverage gate found the superseded-AddItemsskip path was unreachable by any deterministic test, putting the split pair 1.03 pp below its floor. A seam mirroring the already-ratifiedSetSuggestionsCorepattern was extracted. The review noted the branch it covers is inert, so the new test raises coverage without fully discriminating that branch; tracked as Refactor: breadcrumb-501-review-residuals #657 R-2.timed out after 60000mspump-host signature in sibling-owned files, on a host running three concurrent agents. Both runs are transcribed in the committed run ledger. A re-run that passes proves the failure is non-deterministic, not that it was environmental; the residual risk belongs to those files' owners.Follow-ups filed
_closeCompletedresidual, owned by feature #488CloseCorelineCI
ci.ymltriggerspull_requestonly on[main, development], so this PR into the epic integration branch receives no automatic checks. CI was dispatched explicitly against the branch head and is recorded in the checkpoint'sci_gate.Closes #501
Closes #462
Closes #500
Closes #502