Skip to content

fix(breadcrumb): stop suppressing a close while the host reports open (#656) - #720

Merged
drmoisan merged 15 commits into
mainfrom
bug/breadcrumb-closecompleted-residual-outside-requestopen-invalidate-656
Sep 1, 2026
Merged

fix(breadcrumb): stop suppressing a close while the host reports open (#656)#720
drmoisan merged 15 commits into
mainfrom
bug/breadcrumb-closecompleted-residual-outside-requestopen-invalidate-656

Conversation

@drmoisan

@drmoisan drmoisan commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Stop suppressing a breadcrumb close while the host reports open

Closes #656

Summary

BreadcrumbDropDownOpenCoordinator._closeCompleted is cleared only on the RequestOpen and
Invalidate paths. A host that becomes open again by any other route left the flag set, so the next
close request was suppressed even though the host was genuinely open.

CloseCore now reads the host's open state into a local declared before the lock (_sync) and
narrows the completed-close guard from if (_closeCompleted) to if (_closeCompleted && !hostOpen).

What changed

File Change
QuickFiler/Viewers/BreadcrumbDropDownOpenCoordinator.cs Hoists bool hostOpen = _host.IsOpen; above the critical section; narrows the guard; adds two XML doc remarks recording the new condition and why the read sits outside _sync.
QuickFiler.Test/Viewers/BreadcrumbDropDownOpenCoordinatorTests.Part3.cs Adds CloseCore_AfterSuccessfulCloseAndHostReopen_ReachesHostCloseAgain.

No other production, test, project, or package file is touched.

Why the host read is outside the lock

SR-4 of issue 501 declined this same refinement written as a read taken inside lock (_sync),
because that adds a foreign call made while the coordinator lock is held. Hoisting the read leaves
the count of such calls unchanged, so the design objection recorded there is respected rather than
reopened. The one pre-existing in-lock _host call at RequestOpen is untouched, and no new one is
added.

Reachability

This is latent-correctness hardening of a seam contract, not a repair of an observed user-facing
failure. The severity recorded on the issue (Medium, latent) is unchanged.

The picture is more precise than the issue text suggests, and the two analyses produced during this
work each covered only part of it. Both were re-derived against source before merge:

  • BreadcrumbDropDownHost.Close returns true immediately after scheduling CompleteClose, and
    OpenState = false lives inside that scheduled callback. The scheduling chain ends at
    BreadcrumbPopupUiOperations.PostAsync, which delegates directly to BreadcrumbUiDispatcher.Dispatch.
  • Dispatch runs the action inline and returns Task.CompletedTask when IsCurrentBoundary()
    is true. On the owner boundary — the normal path — CompleteClose therefore completes
    synchronously, OpenState is already false when Close returns, and the new guard behaves
    identically to the old one. On that path the change is a no-op.
  • When IsCurrentBoundary() is false, Dispatch posts asynchronously, so Close can return true
    while OpenState is still true. The new branch is reachable there on the shipped host, with no
    substituted seam.

An exhaustive enumeration confirmed that no production path reopens the drop-down while bypassing
both RequestOpen and Invalidate: the only OpenState = true assignment is in
BreadcrumbDropDownOpenLifetime.ShowCurrentSurface, reachable only via RequestOpen, which clears
the flag immediately beforehand.

Risk direction: the narrowed guard can only let CloseCore proceed to _host.Close in the window
where the host genuinely reports open. It never suppresses a close that previously succeeded. Not
merge-method dependent.

Verification

Full C# toolchain, clean in a single pass:

Gate Result
dotnet tool run csharpier check . exit 0, 1566 files
msbuild TaskMaster.sln /t:Rebuild ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true 0 Error(s); warnings 5, unchanged from baseline
msbuild TaskMaster.sln /t:Rebuild ... /p:TreatWarningsAsErrors=true 0 Error(s)
Full MSTest suite with coverage 6926 / 6926 passed

Analyzer non-vacuity confirmed: zero Skipping target "CoreCompile" lines, so the changed files were
actually compiled by the gate.

Red-to-green: the new regression test was recorded failing before the production edit (with the
production file confirmed unmodified at that moment) and passing after it.

Coverage: repo line rate 0.853792 to 0.853732; the coordinator class 0.983122 to 0.983193, and both
changed lines carry hit counts of at least 1. This clears the CLAUDE.md floors (80 percent repo-wide,
90 percent new code) and also the 85/75 figures in .claude/rules.

Acceptance criteria

All 20 criteria in the feature spec.md are checked off and were independently re-verified from
primary evidence during review, not from the executor's summaries.

The three footprint criteria were re-measured with git diff --name-only origin/main...HEAD: under
QuickFiler/ and QuickFiler.Test/ the diff contains exactly the two files listed above, and it
contains no .csproj, .props, .targets, or packages.config path.

Review outcome

Policy audit, code review, and feature audit are committed in the feature folder. Blocking findings:
0 (0 FAIL, 0 blocking-PARTIAL). Seven non-blocking follow-ups are recorded as text in the feature
audit — chiefly amending the spec's reachability wording to match the analysis above, correcting a
description of the not-open Close branch, and adding a test harness variant that defers IsOpen
so the asynchronous timing is represented. They are deliberately not filed as issues from this
branch, which carries footprint acceptance criteria; the maintainer files one consolidated issue
after merge.

Note on the PR context bundle

The generated PR-context bundle in this run is unreliable and its output was not used for the
close list:

  • It reports "GitHub CLI unavailable: GitHub CLI (gh) is not installed". This is false; gh is
    installed and working, and every issue number below was verified with it directly.
  • Its author-asserted auto-close list contained four unrelated issues (462, 488, 500, 501) and
    twenty non-issue tokens of the form #AC-1 through #AC-20, scraped from the acceptance-criteria
    identifiers in spec.md.

Verified with gh: issues 462, 488, 500, and 501 are all already closed, so the scrape was
benign in this instance. Issue 656 is open and is this item's own issue. It is the only issue
this pull request closes.

drmoisan and others added 15 commits August 31, 2026 20:14
Create the active feature folder for issue 656 (breadcrumb _closeCompleted
residual outside RequestOpen/Invalidate) in full-bug work mode. The potential
entry was promoted previously and issue 656 already exists, so promotion was
not re-run.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Enumerate every production path that makes the breadcrumb drop-down host
open and establish that none bypasses both RequestOpen and Invalidate. The
single open transition is BreadcrumbDropDownOpenLifetime.cs:268, reachable
only through RequestOpen, which clears _closeCompleted immediately before.

Issue 656 is therefore latent-correctness hardening rather than an observed
user-facing failure. Records the SR-4 rationale, the four standing test
guards, the existing ControlledHost.SetOpen seam, and the option space.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Record the reopen-path enumeration result, the SR-4 reconciliation, and the
recommended remedy: hoist the host open-state read above the CloseCore
critical section and qualify the completed-close suppression with it, so no
new IBreadcrumbDropDownHost call is made while _sync is held.

Pins the production footprint to BreadcrumbDropDownOpenCoordinator.cs and the
test footprint to BreadcrumbDropDownOpenCoordinatorTests.Part3.cs, and leaves
the four standing close-suppression guards unedited.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Six phases: policy reads plus worktree bootstrap and baseline capture, the
expect-fail regression test, the CloseCore fix, pass-after verification, the
full C# QC toolchain with footprint and coverage gates, and acceptance
criteria check-off.

Passes mcp validate_orchestration_artifacts with artifact_type plan.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Select-String has no -Recurse parameter, so the test-name absence check now
produces its file set with Get-ChildItem and pipes it in. Records that every
count stated as an acceptance value must use the array subexpression form so
a zero-match result is 0 rather than absent.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Nine deltas from the executor preflight. Blocking: read the coordinator
coverage figures from the class-relative ./lines rollup rather than from
lines-covered and lines-valid attributes, which exist only on the root
coverage node; add a Phase 0 task creating the TestResults directories the
msbuild file logger and Tee-Object write into, since neither creates
intermediate directories; give the standing-guard vstest run an explicit
command with the LiveOutlook conjunct applied to a parenthesised group.

Also bounds the per-file coverage comparison against known measurement
nondeterminism, scopes the test gate to QuickFiler.Test as AC-18 states
rather than to every assembly, and records the AC-4 evidence-location
reconciliation. Phase 0 renumbered to T1 through T12.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
A non-zero wrapper exit throws before the post-processing that rewrites
class filenames and sets the root lines-covered and lines-valid, so a failed
run leaves the raw all-modules document on disk.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
…ompleted-residual-outside-requestopen-invalidate-656

# Conflicts:
#	.claude/agent-memory/atomic-planner/MEMORY.md
#	.claude/agent-memory/task-researcher/MEMORY.md
… artifact

Marks the last plan task complete, records the close-out evidence artifact, and appends the stale-pinned-base finding to the executor memory note.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Policy audit, code review and feature audit for the #656 breadcrumb
CloseCore completed-close guard change, at 2026-09-01T15-03.

20 of 20 acceptance criteria verified PASS. Zero blocking findings.
All toolchain and coverage figures re-derived independently from git,
the retained msbuild/vstest logs and the Cobertura document, including
one live re-execution of the CSharpier check.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
Written during the #656 feature review, so it is a branch mutation made by
this run rather than pre-existing content.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL
@drmoisan
drmoisan merged commit 43dcc80 into main Sep 1, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: breadcrumb-closecompleted-residual-outside-requestopen-invalidate

1 participant