fix(panel): don't let a dismissed approval silence later requests (#309) - #2
fix(panel): don't let a dismissed approval silence later requests (#309)#2halindrome wants to merge 5 commits into
Conversation
Dismissing an approval hides it but deliberately leaves it queued, so the CLI stays blocked and the prompt stays recoverable. The card-and-sound trigger in handlePermissionRequest was gated on `permissionQueue.count == 1`, which a dismissed entry keeps false forever — so every later permission request, from every session, arrived with no card and no sound until the dismissed one was resolved some other way. Queue size was standing in for "is a card already showing", and dismissal is exactly the state where those two stop agreeing. The gate now asks the same predicate the display path uses, nextVisiblePermissionIndex(), which already skips dismissed sessions. Showing the card via showNextPending() rather than pointing it at this session by hand matters: with a dismissed entry still leading the queue, the approval card renders the head, so a hand-set surface would show the dismissed request's content under the new session's name. The dismissed session's own next request stays hidden — dismissal is per-session and clears when that session's request resolves. That behaviour is unchanged and now pinned by a test so this fix isn't read as altering it. Fixes wxtsky#309. 689 tests, 0 failures; the new regression test was confirmed to fail against the old gate. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
QA found the first attempt incomplete. `nextVisiblePermissionIndex() != nil`
is "a non-dismissed request is queued", which is still not "a card is on
screen": handlePermissionRequest un-dismisses the session on entry ("session
needs user decision again"), so a dismissed session's NEXT request makes its
own still-queued earlier request count as visible while nothing is displayed.
The silencing then resumed exactly as wxtsky#309 described, one step later.
The gate now asks the surface directly. That also settles what the previous
commit's pinning test got backwards: a dismissal is cleared by the session's
next request arriving, not by the dismissed request resolving, so that
session's next request must bring its card back. The old test asserted the
broken state was correct and its doc comment stated an invariant the code does
not have; both are corrected.
Also strengthened, per QA:
- the cross-session regression test's anti-hang guard checked only the surface;
an implementation that points the card at the arriving session by hand passes
that while the dismissed request still leads the queue, so approve resolves
the wrong one and the await hangs. It now checks the queue head too.
- the new "asking again doesn't silence others" test passed under the broken
gate as originally written (resolving A's requests surfaces B either way).
It now asserts a card is on screen at the moment B arrives, which is the
state that actually differs.
Known limitation, unchanged from main and noted in the code: a card suppressed
by Smart Suppress also leaves a visible request undisplayed, so a second
session's request waits behind it. Closing that needs showNextPending to skip
un-openable entries; tracked separately.
690 tests, 0 failures; both new guards verified red against the incomplete gate.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
QA Round 13-lens panel ( The round's headline question — the new Contract VerificationSynthesized contract — no ticket linked; upstream defect report is wxtsky#309.
The ordering / reentrancy question — no defectThe specific concern (enqueue now reorders
Finding 1 —
|
| Severity | Confirmed | Blocking |
|---|---|---|
| critical | 0 | 0 |
| major | 1 | 1 |
| minor | 2 | 2 |
| Total | 3 | 3 |
Observations (non-blocking): 3.
Observations
- Pre-existing — hotkey collapse leaves the same false state.
executeShortcut(.togglePanel)(AppDelegate.swift:222-224) setssurface = .collapsed
for any expanded surface including.approvalCard, without registering a dismissal —
leaving a non-dismissed request queued with nothing on screen, after which the new gate
suppresses every later card and sound.PanelWindowController.swift:269-272explicitly
guards the click-outside path; the hotkey path is not guarded. Not a regression (the
old gate was equally silent here) and the call sites are untouched by this MR. A
surface-based gate would close this and Finding 1 together, so fixing it separately is
probably wasted effort. - "Don't steal the card" is asserted but not pinned (mutation gap). Deleting
!wasShowingPermission,fromAppState.swift:1322still passes the assertion at
AppStatePermissionFlowTests.swift:70-71, becauseshowNextPending()promotes the first
visible entry, which in that scenario is already at index 0 — an unguarded call is a no-op
onsurface. Closing it means adding a sound-emission seam toAppState, i.e. more new
unreviewed code than the MR contains. Recorded so the guard is not mistaken for tested. - Sound emission and the reorder/dedup interaction are unasserted. Deleting
SoundManager.shared.handleEvent("PermissionRequest")(AppState.swift:1328) fails no
test — the "and play the sound" half of criterion 1 is verified by neither new test — and
no test enqueues a duplicatetoolUseIdor drives a cache drain against a reordered queue.
Both are safe by construction (see the ordering section) and both would need an injected
seam around theSoundManager.sharedsingleton to assert. Cost exceeds the risk; recorded
so the criteria are not silently treated as tested.
SAST review not applicable
Target default has security_stage: false in base-branches.json. No CI
security stage is wired for this target, so no SAST/SCA delta is computed.
No security-scan claim is made for this HEAD (e984ad6); none was run.
Security posture of the change itself: nothing here can auto-approve, resolve, or drop a
permission request. The only new call is showNextPending(), and the guard immediately
preceding it forces its reorder-only branch; the continuation-resuming branches are
unreachable from this call site. Dismissal still leaves the CLI blocked.
⚠ QA agent token unavailable for this round.
QA performed by Claude Code (claude-opus-5), manager + 3-lens panel
Round 1 disposition (main loop)
All three findings fixed in 29e1456064204251fe0f4b9bc57793845d4124fb.
F1 was right and the fix was incomplete. Verified at AppState.swift:1291: dismissedPermissionSessionIds.remove(sessionId) runs on entry, before the visibility probe, so a dismissed session's next request un-dismisses it and its still-queued earlier request counts as visible while nothing is displayed — resuming the exact silencing wxtsky#309 reports. The gate now reads the surface directly.
F3 was right in a way that mattered more than a comment. Dismissal is cleared by the next request arriving, not by resolution — so the previous commit's pinning test asserted the broken state as correct. It is inverted, not just re-worded. The synthesized contract's third criterion carried the same error and should be read as corrected.
F2 fixed, and while re-checking I found the same weakness in my own new test: it passed under the broken gate because resolving A's requests surfaces B either way. It now asserts a card is on screen at the moment B arrives.
The round's headline question — whether the mid-enqueue showNextPending() reorder breaks the dedup path or the tool-use cache — came back clean from all three lenses, which is what the round was run for.
Known limitation, unchanged from main and now marked in code: a Smart-Suppress-suppressed card also leaves a visible request undisplayed, so a second session waits behind it. Needs showNextPending to skip un-openable entries; separate change.
Verification: swift test — 690 tests, 2 skipped, 0 failures. Both new guards confirmed red against the incomplete gate.
QA-Fix-Commit: 29e1456
Round 1's surface-only gate had its own hole, found by two lenses independently: drainPermissions (process exit, or a question arriving for the session) empties the queue without clearing `surface`, and the card renders nothing when there is no head request. A bare `.approvalCard` check therefore blocked on a phantom card and swallowed the next request from every session — the same wxtsky#309 defect, third variant. The gate now requires both an .approvalCard surface and a non-empty queue. Card and sound also stopped sharing a gate. They answer different questions: the card asks "is one already on screen", the sound asks "does this request start a new burst" — which is what `count == 1` used to approximate. Sharing the surface-derived gate made the sound fire per request instead of per burst whenever no card was open, e.g. under Smart Suppress. The sound now keys off whether a visible request was already queued, which keeps burst behaviour and still fixes the dismissed-request silence. Tests, per QA: - new: a request arriving under a stale .approvalCard surface must still raise a card (verified red against round 1's gate). - the cross-session test asserted only "not collapsed", the weakest thing available; it now names the card it expects. - the re-raise test could not tell showNextPending's promotion from a hand-pointed surface, both requests being from one session; it now asserts the card shows the earlier queued request, not the arriving one. 691 tests, 0 failures. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
QA Round 2Panel: 3 lenses (contract-security, regression-edges, test-quality), all completed. Test suite was run by the orchestrator on this HEAD: 690 tests, 2 skipped, 0 failures, Contract VerificationSource: synthesized (no ticket linked; upstream defect report is wxtsky#309).
FindingsFinding 1 — Surface-derived gate blocks on a stale
|
| Severity | Contract | Regression | Total |
|---|---|---|---|
| critical | 0 | 0 | 0 |
| major | 0 | 1 | 1 |
| minor | 0 | 3 | 3 |
| Blocking (critical + major) | 1 |
| Relevance | Count |
|---|---|
| contract | 0 |
| regression | 4 |
| observation | 6 |
Lens navigation: contract-security ctx, regression-edges ctx, test-quality ctx.
No failed lenses. Working tree verified unchanged across the panel (HEAD, branch and
porcelain status snapshotted before and after).
SAST review not applicable
Target default has security_stage: false in base-branches.json. No CI
security stage is wired for this target, so no SAST/SCA delta is computed.
No security-scan claim is made for 29e1456 — none was computed.
⚠ Posted with dev credentials — QA agent token unavailable.
QA performed by Claude Code (claude-opus-5), manager + 3-lens panel
Round 2 disposition (main loop)
All four findings fixed in f3642dda20d57463f20344fa6c16e3c34a8df1c9.
The major was real and mine: round 1 replaced a queue-derived gate with a surface-derived one, and traded one hole for another. The gate now requires both halves — an .approvalCard surface and a non-empty queue — and the phantom-card case is pinned by a test verified red against round 1's gate.
On the dead-conjunct finding: the right fix was not to delete it but to notice that card and sound were answering different questions. They no longer share a gate — the sound keys off whether a burst was already in progress, which restores count == 1's burst semantics that round 1 had silently changed to per-request under Smart Suppress. That behaviour change was not in any finding; it surfaced from the finding's reasoning.
qa_introduced_blocking: 1 and all four findings blamed to 29e1456 — this cycle is now reviewing its own fixes. The panel was right to raise diminishing returns while also saying the major should still be fixed; both were correct.
Verification: swift test — 691 tests, 2 skipped, 0 failures.
QA-Fix-Commit: f3642dd
Round 3 finding. The assertion checks that the card renders the queue head, so the user is asked about the earlier request — it pins queue order, not which mechanism set the surface. With both requests from one session no assertion at that level can tell showNextPending's promotion from a hand-pointed surface; the cross-session test's head check is what covers that. Comment only. 691 tests, 0 failures. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
QA Round 3
Panel: 3 lenses (contract-security, regression-edges, test-quality), run concurrently against Verification note: Navigation note: all three lenses loaded the code-navigation tools successfully, but this repo has no CMM index ( Contract Verification
Finding 1 — Sound predicate
|
| Severity | Contract | Regression | Total |
|---|---|---|---|
| critical | 0 | 0 | 0 |
| major | 0 | 0 | 0 |
| minor | 1 | 2 | 3 |
No blocking (critical/major) findings. All three minor findings carry an explicit cost note arguing against acting on them in this round, and all three sit on code a previous round of this QA cycle wrote.
State-space sweep (checked, no finding):
(.approvalCard, queue empty)— the phantom surface afterdrainPermissions(:1955, which never touchessurface). Gate correctly returns false. Covered bytestRequestArrivingUnderAStaleApprovalSurfaceStillRaisesACard, and the guarded state is genuinely reachable:handleProcessExit(:570) drains without callingshowNextPending().(.approvalCard, queue non-empty, all entries dismissed)— unreachable.dismissPermissionPrompt(:1587-1594) collapses the surface whenevernextVisiblePermissionIndex()is nil.(.approvalCard for session X, queue holds only other sessions' entries)— gate true and a card genuinely is on screen; the arrival queues behind it and surfaces on resolve. Asserted bytestDismissedSessionAskingAgainDoesNotSilenceOtherSessions.(.sessionList, any queue)— gate false,showNextPendingruns, its:2013guard keeps the list open, matching the old code.- Queue reordering is a no-op whenever the head is visible —
remove(at:0)/insert(at:0)is identity. The head moves only when it is dismissed. - Concurrency —
AppStateis@MainActor; there is no suspension point between the gate at:1333andshowNextPending()at:1354, so no resolve or dismissal can interleave. - Same-session silent re-raise — a dismissed session asking again re-raises the card with no sound. Deliberate:
mainis silent and card-less here, and the contract requires only the card.
Security posture: clean. The change adds no path that can auto-approve, silently drop, or misattribute a request. The added block contains no continuation.resume and no queue removal. Every resume site (approvePermission:1405, denyPermission:1563, drainPermissions:1961, AppState+ToolUseCache.swift:167, :118) is untouched and still requires explicit user action or an explicit drain trigger. No tokens or secrets are handled on these paths.
Round-2 assessment upheld: the ponytail: note at AppState.swift:1324-1327 — a Smart-Suppress-suppressed card leaving a visible request undisplayed so a second session waits behind it — is pre-existing on main and is not worsened by this MR. Round 3 does not contest it.
Observations
Non-blocking; not defects this MR introduces.
- Approval card can show one session's command under another session's identity chrome (severity major as a standalone defect, but pre-existing and identical on
main; reported independently by contract-security and regression-edges). The card takes its body from the queue head but its identity chrome (project name, session label, terminal badge) from the surface's associated session id.showNextPending(AppState.swift:2007-2015) promotes a new head and setsactiveSessionId, but assignssurfaceonly whenshouldAutoOpenPendingSurface(for: sid)(:930-939) is true — with Smart Suppress on and the session's terminal frontmost,surfacekeeps the stalesid.NotchPanelView.swift:206-221then rendersappState.sessions[sid]abovependingPermission's tool and input, andonAllowresolves the head. Reached identically onmainvia the pre-existingshowNextPendingcallers (:1577,:1981,AppState+ToolUseCache.swift:70-77). Distinct from theponytail:note above (that one is a hidden card; this is a wrong-identity card). Warrants its own ticket. Fixing it inside this round would mean touchingshowNextPending's surface contract — exactly what produced the round-1 and round-2 majors. - No CMM index exists for this repo. All three lenses loaded the navigation tools successfully but found no indexed project at or above
/Users/ahby/Sources/CodeIsland. Not a tooling failure and not a silent degradation — every symbol claim in this note is backed by an opened definition site — but worth fixing before any further round.
SAST review not applicable
Target default has security_stage: false in base-branches.json. No CI
security stage is wired for this target, so no SAST/SCA delta is computed.
⚠ QA agent token unavailable — this note was not posted by the QA agent.
QA performed by Claude Code (claude-opus-5), manager + 3-lens panel
Round 3 disposition (main loop)
No blocking findings — the round-2 gate holds under every state the lenses probed, including the ones this round was aimed at.
Fixed: the comment overstating what its assertion pins (c86fe891864254270664e169f5b22a2804ffb2ca, comment only).
Deferred, with the panel's own cost reasoning accepted:
- Sound predicate has no test coverage. Closing it needs a SoundManager dependency-injection seam — more new, unreviewed production code than this PR contains. Contract criterion 8 is therefore only partially satisfied and this PR should not be read as claiming the burst-sound behaviour is pinned.
- An open question card can be stolen by an older permission when a third session's request arrives. Self-healing and non-lossy. The fix is another card-gate condition — the exact change shape that produced the round-1 and round-2 majors. Deferring on those grounds specifically, not because it is unimportant.
Cycle ends here on the panel's diminishing_returns: 3 of 3 findings sit on code earlier rounds of this cycle wrote, and each carries a lens cost note arguing against fixing it now. This is the operator's call and it has been put to them.
Standing caveats: approval_eligible=false, so no QA approval was emitted regardless of the clean verdict; and no codebase-memory index exists for this repo, so all three rounds enumerated callers by definition-site reads rather than graph queries.
Verification: swift test — 691 tests, 2 skipped, 0 failures.
QA-Fix-Commit: c86fe89
Two defects found by reviewing this change merged with the wxtsky#308 answer-routing fix. Both are present on this branch alone, so they belong here rather than in the integration. 1. The gate asked `!permissionQueue.isEmpty` — a whole-queue question — while staleness is per-session. `drainPermissions` empties one SESSION's requests without clearing `surface` (a question arriving for that session does this), so a card can be left pointing at a session with nothing queued while others still wait. The whole-queue test reads that as "a card is up", and every later request queues silently behind a card for a session that has no pending request. Now asked per session. 2. `dismissedPermissionSessionIds.remove(sessionId)` ran before the replay-dedup early return, so a replayed tool_use_id for a dismissed session resurrected the request the user had hidden — taking the card the arriving session should have received, and counting as a burst already in progress so that session lost its sound too. A replay is the same decision arriving twice, not a new one. A same-id request with *differing* tool inputs is a distinct request (wxtsky#169) and still clears the dismissal; that path is pinned too. This one is latent on main as well: a resurrected request leads nextVisiblePermissionIndex(), so any later showNextPending() can raise the card the user dismissed. New AppStatePermissionGateTests covers all three cases. Both fixes verified by mutating the production line each names and confirming the test fails — and the replay test stops on that failure instead of hanging on the await that follows. 694 tests, 0 failures. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
2 participants
Fixes wxtsky#309.
Cause
Dismissing an approval hides it but deliberately leaves it queued, so the CLI stays blocked and the prompt stays recoverable. The card-and-sound trigger in
handlePermissionRequestwas gated on the queue having been empty:A dismissed-but-queued entry keeps that count at ≥ 1 forever, so every later request — from any session — landed on
count >= 2and showed nothing, silently, until the dismissed one was resolved some other way.Queue size was standing in for "is a card already showing". Dismissal is exactly the state where those two stop agreeing.
Change
Gate on the same predicate the display path already uses —
nextVisiblePermissionIndex(), which skips dismissed sessions — evaluated before the append.Showing the card through
showNextPending()rather than pointing the surface at this session by hand matters: the approval card renders the head of the queue, so with a dismissed entry still leading it, a hand-set surface would show the dismissed request's content under the new session's name.showNextPending()promotes the first visible request and applies the session-list and Smart Suppress rules that were previously duplicated here.Deliberately unchanged
The dismissed session's own next request stays hidden; dismissal is per-session and clears when that session's request resolves. That is existing behaviour, and it is now pinned by a test so this fix is not read as altering it. If it should instead re-show, that is a separate call worth its own change.
Tests
Two added to
AppStatePermissionFlowTests: a later session's request still raises a card while a dismissed one sits in the queue, and the same-session case stays hidden. The regression test was confirmed to fail against the old gate, and it stops on that failure rather than continuing into an await that would hang.swift test: 689 tests, 2 skipped, 0 failures.Branches from
main@ 9e3a1eb and is independent of wxtsky#310.