Integration: #308 answer routing + #309 dismissed-approval silence - #3
Open
halindrome wants to merge 11 commits into
Open
Integration: #308 answer routing + #309 dismissed-approval silence#3halindrome wants to merge 11 commits into
halindrome wants to merge 11 commits into
Conversation
Approving, denying, answering, or skipping from the notch resolved `queue.removeFirst()`, while the card itself is addressed by session id. Anything that mutated the head under an open card — a peer disconnect draining another session, a stale tool-use eviction, the reorder inside showNextPending() — delivered the answer to whichever request happened to be first, i.e. a different CLI than the one shown. With two sessions waiting this silently answered the wrong project. The answer paths now take the acting card's session and resolve that session's queued request; when it is gone (answered in the terminal, drained on disconnect) the action is discarded and the panel resyncs instead of falling through to the head. Cards also render the addressed session's request rather than the head, so what is shown and what is resolved cannot disagree. Head-of-queue behaviour is unchanged for the surfaces that only ever mirror the head (iPhone/Watch Buddy, Codex/companion paths). Upstream issue: wxtsky#308 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
Round 1 confirmed one major and eight minor findings against the wxtsky#308 fix. - major: a card whose request had been drained left the notch expanded and empty. Cards now render only their own session's request, so the previous "wrong content" case became "no content" whenever showNextPending() declined to reassign the surface (Smart Suppress). showNextPending() now collapses a card surface whose session has nothing queued, before deciding what to open — one guard covering every drain path, not just the answer paths. - keyboard shortcuts passed surface.sessionId for any card kind, so a permission shortcut fired over a question card addressed a non-existent approval and discarded the live card. They now use kind-matched accessors. - dismissPermissionPrompt got the stale-discard treatment the other actions had. - the cards' "N of M" position was hardcoded to 1 while they may render a non-head request. Tests: the stale-card case is now pinned by the suppression path that actually reproduces it (verified red without the guard); routing tests assert queue state before awaiting so a regression fails by name instead of hanging; the head-of-queue test uses a two-element queue so it can fail; added coverage for the single-answer path, dismiss routing, and the session-scoped lookups. 699 tests, 0 failures. Not fixed, deliberately: within-session render-vs-resolve keying (hypothetical, mechanism unreproduced) and pinning the view-layer call-site wiring, which needs a SwiftUI view-test harness this project does not have. Both noted on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
Round 2 confirmed one major and five minor findings; three of the six sat on code round 1 wrote. - major (introduced by eb5ef93): collapseStaleCardSurface tested card liveness by queue membership, but dismissPermissionPrompt hides a request without dequeuing it. Under Smart Suppress a dismissed approval card therefore stayed on screen, re-rendering the request the user had just dismissed. The guard now applies the same predicate nextVisiblePermissionIndex() uses: dismissed counts as not visible. No request was ever misrouted by this. - the routing tests failed by name but then deadlocked on the await that follows; each pre-await assertion now stops the test instead. - added coverage for the kind-matched surface accessors, both dismiss paths (routing and stale-discard), and a Codex app-server request answered while queued behind another session — those branches were only ever exercised at index 0. - corrected a doc comment describing a collapse that moved to another function. Both new guards were verified to fail without their fix before committing. 703 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
Round 3 found no blocking defects. Both findings were assertion-strength gaps in tests round 2 added — each passed whether or not the behaviour it named still held. - the stale-dismiss test observed only the queue, which is unchanged by a head-based dismiss (dismiss hides without dequeuing). It now drives showNextPending() and asserts the other session's card is still offerable, which is where a wrongly-dismissed session actually shows up. - the Codex-behind-another-session test asserted the dequeue, which a head-anchored Codex check would also satisfy while replying down the hook path and leaving the server waiting forever. The request now carries a capturing reply closure, so the test asserts the JSON-RPC path was the one taken. The session status it previously asserted could not discriminate: both paths land on .processing. Both were verified by mutating the production path each one names and watching it fail. 703 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
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 <noreply@anthropic.com> 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
Reviewing the two fixes together found three majors that neither branch's own cycle could have seen. 1. wxtsky#309's enqueue gate asked `!permissionQueue.isEmpty` — a whole-queue question — while staleness is per-session. With another session still queued, a card whose own request had been drained read as "on screen", so the gate skipped showNextPending() and with it wxtsky#308's stale-card collapse: the panel wedged behind a card rendering nothing and swallowed every later request. The lenses split on whether this was reachable; it is, without Smart Suppress — a question arriving for the card's session drains its permission, and if the question queue is already non-empty the surface is never reassigned. The gate now asks exactly what the card asks: is this session's request still queued. 2. The un-dismiss 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; the un-dismiss now happens only on the path that actually enqueues. 3. The merged tests were the strict union of the two branches — the gate and the session-routed actions never met. Added AppStateIntegrationRoutingTests covering the drained-card-with-others-queued state, an inline session-list approval routed to a session that is not the queue head, and the replay case; the existing wxtsky#309 burst test now routes its approvals by session. Both production fixes verified by mutating them back and watching the new tests fail. 710 tests, 0 failures. Local integration branch; not for upstream. The two PRs stay independent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
Round 2 found no blocking defects — both round-1 fixes verified correct by construction (every surface writer enumerated for the per-session gate's one candidate hole; all three mergeDuplicatePermissionRequest false paths checked for the moved un-dismiss). Four minors, all taken: - a question arriving for the card's own session drained that session's permission and left the island expanded on a card rendering nothing. Fixed in drainPermissions itself, which is where every drain path converges, rather than at the two call sites that happen to reach it today. - the wxtsky#309 burst test's new expectedSessionId comment claimed coverage the test does not provide (both of A's requests are at the front, so it cannot show the routing picked the right index); reworded to point at the test that does. - the integration test leaked three continuations, producing SWIFT TASK CONTINUATION MISUSE noise that would hide a real one. - pinned the merge==false branch of the moved un-dismiss: same tool_use_id with differing tool inputs is a distinct request (wxtsky#169), so it must still clear the dismissal. 711 tests, 0 failures; the drain-collapse verified red without its fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h
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
2 participants
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.
Not for upstream — this is the merge of the two independent fixes, opened so the combination can be reviewed as a unit before it is used as a local daily driver.
fix/answer-routing(upstream PR fix(panel): route answers to the card's session, not the queue head (#308) wxtsky/CodeIsland#310, fixes Answer from the notch card can be delivered to the wrong session when several sessions are waiting wxtsky/CodeIsland#308): answers resolve the card's session rather than the queue head; cards render the addressed session's request;showNextPendingcollapses a card surface whose session has nothing actionable left.fix/dismiss-silences-requests(upstream PR fix(panel): don't let a dismissed approval silence later requests (#309) wxtsky/CodeIsland#311, fixes Dismissing an approval card silences every later permission request, from all sessions wxtsky/CodeIsland#309): the enqueue gate no longer treats queue size as "is a card on screen"; card and sound conditions separated.Each branch had three QA rounds on its own. This merged combination has had none. The merge auto-resolved in
AppState.swift.The interaction to scrutinise: both changes touch the same question — "what should be on screen right now" — from opposite ends. wxtsky#308 added
collapseStaleCardSurface()insideshowNextPending(), which makes a stale.approvalCardself-correct; wxtsky#309 added a gate at enqueue that checks for that same stale state before callingshowNextPending(). My reading is that they are complementary rather than redundant, because wxtsky#309's gate is evaluated first, but that reading has not been reviewed.707 tests across both suites, 0 failures.