fix(panel): route answers to the card's session, not the queue head - #1
fix(panel): route answers to the card's session, not the queue head#1halindrome wants to merge 4 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
QA Round 1MR: #1
Contract Verification
3 of 7 criteria are partially satisfied. The cross-session routing fix — the substance of wxtsky#308 — is sound and correctly constructed. The gaps are all on the second clause of AC2 (panel state after a discard) and on how thoroughly the change is pinned by tests. Finding 1 — Stale card leaves an expanded but empty notch panel
What was tested: what the panel renders when Expected: per AC2, the panel collapses or falls through to whatever is genuinely waiting — "the panel must not be left displaying a dead card." Actual / risk: Sequence: approval card open for A → A answered in the terminal → Proportionality: this MR converts a wrong-content card (the wxtsky#308 symptom) into a blank one. Finding 2 — Keyboard shortcuts pass the surface session id for any card kind, collapsing a live card
What was tested: pressing the approve / approve-always / deny hotkey while a Expected: a hotkey that does not apply to the current card is inert. Pre-MR it was a silent Actual / risk: Proportionality: no wrong request is ever resolved, and the new behaviour is arguably safer than the old head-of-queue approve — hence minor. Filed so the behaviour change is a decision on record rather than an accident. Cheap fix: pass the id only when the surface case matches the action kind. Finding 3 —
|
| Severity | Contract | Regression | Test-quality | Total |
|---|---|---|---|---|
| critical | 0 | 0 | 0 | 0 |
| major | 1 (hypothetical) | 1 | 0 | 2 |
| minor | 0 | 3 | 5 | 8 |
| Blocking total | 10 | |||
| observations | 2 |
Confirmed blocking: 1 major, 8 minor. One further major (Finding 5) is hypothetical — mechanism confirmed at every cited line, timing window not reproduced.
The routing fix itself is correct and well-constructed. The index helpers are the right shape, the nil default is a genuine no-op for the surfaces AC5 protects, the Codex/subagent index substitution is faithful, and the new tests assert on real resumed continuation payloads rather than queue arithmetic. Every blocking finding is either a panel-state consequence of the new discard path (Findings 1–4), the within-session residue of the same defect class (Finding 5), or a gap in how tightly the change is pinned (Findings 6–10).
Cheapest items with the clearest payoff: Finding 7 (one-line reorder, turns CI hangs into named failures), Findings 6 and 8 (two lines each), Finding 3 (one expression).
What the panel verified clean
Recorded so a later round does not re-litigate it:
- Caller sweep. No call site of any of the six changed functions was missed.
handleBuddyControlCommand(AppState.swift:1467) andanswerCompanionQuestion(:1490) still resolve head-of-queue with noexpectedSessionId, matching AC5's carve-out. Codex app-server reply paths are reached only throughanswerQuestion/answerQuestionMulti, whose in-function Codex branches now index byindexrather than0consistently (:1800,:1811,:1865,:1884). - Session-list inline approvals (
NotchPanelView.swift:2284/2291/2298) pass their own row'ssessionIdand are gated byisActiveApproval(:2159,approvalQueueIndex == 0), so the change is a strict safety net there, not a behaviour change. - Multiple queued requests for the same session: render (
first {}) and resolve (firstIndex {}) use the same predicate and both pick the earliest, so FIFO within a session is preserved. (Finding 5 concerns eviction between the two calls, not predicate disagreement.) - Concurrency.
AppStateis@MainActor; queue mutation and surface assignment are synchronous on the main actor in every touched path. The change introduces no new suspension point between lookup and removal, so no TOCTOU window exists betweenpermissionIndex/questionIndexandremove(at:). - Security posture of the diff. No injection surface: every response body is a compile-time string literal (
AppState.swift:1580) or built viaJSONSerialization.data(withJSONObject:)from a dictionary (:1821-1830,:1832-1838), so answer text is never interpolated into JSON. No credentials, tokens, or file paths are read or written. The one trust decision — thatevent.sessionIdnil-coalesced to"default"is a sound identity — is pre-existing and applied consistently (:133,:137,:1351,:1357; matchingESP32StatePublisher.swift:418-421,NotchPanelView.swift:2157). - Vacuous-assertion audit of the new tests. One inert line (
XCTAssertFalse(firstResponse.isCancelled)at:38is trivially true), no empty-array assertions, no assert-on-a-value-the-test-just-set. Queue-state assertions all read state produced by production code.
Observations (non-blocking)
-
Session-id normalisation is inconsistent between the new matchers and the drain paths.
permissionIndex/questionIndex(AppState.swift:1349,:1355) andpendingPermission(forSession:)/pendingQuestion(forSession:)(:130,:134) all normalise with($0.event.sessionId ?? "default"), whereasdrainPermissions(:1997) anddrainQuestions(:2026) compareitem.event.sessionId == sessionIdwith no fallback. A request whose event carries nosessionIdis addressable as"default"by the new card path but is never drained byhandlePeerDisconnect("default"). Pre-existing in the drain functions, untouched by this MR, and every production surface observed supplies a session id. Fixing it here would widen the diff for no observed symptom. -
iPhone/Watch Buddy and companion answers still resolve the queue head — the same privilege-crossing class this MR closes for the panel.
handleBuddyControlCommand(AppState.swift:1467-1488) guards only on queue non-emptiness;answerCompanionQuestion(:1490-1519) reads and writesquestionQueue[0]directly (:1496,:1507). If the remote surface displays session A while the head is session B, a remote approve grants B. AC5 explicitly and correctly ring-fences this:BuddyControlCommandcarries no session id, so closing it means a protocol change plus companion-app changes on both ends. Not worth doing inside this MR — it warrants its own ticket. Recorded so the exclusion is a decision on record rather than an oversight.
Test-hygiene note (not a finding)
Most tests in the new file leave continuations unresumed at teardown — firstResponse.cancel() (:39) does not resume a CheckedContinuation, and the _ = Task { … } handles at :51, :78, :101, :127, :152, :156 are never drained. This produces SWIFT TASK CONTINUATION MISUSE … leaked its continuation noise on stderr rather than failures. Draining with handlePeerDisconnect(sessionId:) at the end of each test would silence it. Mentioned so it is not mistaken for a real failure in a later run.
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 claim is made about a pipeline; none ran for this target.
Schema
No schema change detected. Swift/SwiftUI package with no DDL, no persisted-store definition, and no code-only column or table dependency in the diff.
⚠ Posted with dev credentials — QA agent token unavailable.
QA performed by Claude Code (claude-opus-5), manager + 3-lens panel
Round 1 disposition (main loop)
Fixed in eb5ef930923ded019336572e5f4b4ff50285a321: findings 1, 2, 3, 4, 6, 7, 8, 9.
Not fixed, deliberately:
- Finding 5 (within-session render-vs-resolve keying) — hypothetical; mechanism confirmed by reading but the timing window was not reproduced.
- Finding 10 (reverting the view-layer call-site wiring leaves the suite green) — pinning it needs a SwiftUI view-test harness this project does not have. Accepted for this PR.
Verification: swift test from the repo root — 699 tests, 2 skipped, 0 failures. preflight reported verify.state=none-found; that detector gap is a harness issue, not a project one. The stale-card guard was confirmed to fail without the fix before being committed.
QA-Fix-Commit: eb5ef93
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
QA Round 2
MR: #1
Contract Verification
4 of 7 criteria satisfied, 3 partial. The cross-session routing fix remains sound, and round 1's eight actioned findings are genuinely fixed — the AC2 fix in particular is better than what was asked for. Finding 1 — A dismissed card is still on screen afterwards ↩ on code QA round 1 introduced
What was tested: what the panel shows after the user clicks dismiss on an approval card while another session is also queued and Smart Suppress is active. Expected: dismiss removes the card from view. Per AC2, "the panel must not be left displaying a dead card." Actual / risk: Sequence: queue No cross-session misrouting occurs — the wrong card is shown, never the wrong request resolved. Baseline comparison: on Proportionality: requires ≥2 queued sessions and Two smaller items ride the same lines and are recorded here rather than filed separately: Finding 2 — The two accessors that fix round-1 finding 2 have zero test references ↩ on code QA round 1 introduced
What was tested: whether Actual / risk: Proportionality — worth acting on. This is explicitly distinct from the accepted round-1 finding 10: these are pure computed properties on an enum, so pinning them needs no SwiftUI harness. Two lines: Finding 3 — The
|
| Severity | Contract | Regression | Test-quality | Total |
|---|---|---|---|---|
| critical | 0 | 0 | 0 | 0 |
| major | 0 | 1 | 0 | 1 |
| minor | 2 | 1 | 2 | 5 |
| Blocking total | 6 | |||
| observations | 2 |
Confirmed blocking: 1 major, 5 minor. Nothing critical; no security defect in the routing decisions themselves.
Round 1's fixes are correct. All eight actioned findings are genuinely closed, and two are closed better than asked: the AC2 fix was moved into showNextPending() so every drain path recovers the panel rather than only the discard path, and the shortcut fix uses kind-specific accessors that restore main's exact behaviour where no matching card is up. The one substantive new finding is the sliver that root-cause fix missed — a dismissed request stays queued, so the membership test says the card is alive.
Cheapest items with the clearest payoff: Finding 2 (two lines), Finding 5 (five one-line edits), Finding 6 (one comment). Finding 1 is the only one that changes behaviour.
What the panel verified clean
- Concurrency on the new code.
collapseStaleCardSurfaceis a synchronous@MainActormethod with no suspension point between the lookup and thesurfaceassignment. ThesurfacedidSet(AppState.swift:151-162) acts only onisExpanded == trueand does not re-entershowNextPending, so the new unconditional call introduces no recursion or TOCTOU window. - All 20
showNextPending()call sites were swept for the newly-unconditional collapse. It fires only when the surface is an approval/question card with no queued request for that session — a state that was already the bug. Multi-question wizards keep their queue entry (askUserQuestionStatelives on the queued item, removed only at:1898), so no mid-wizard collapse; completion cards hit the default branch. - Removing the unconditional
surface = .collapsedfromdiscardStalePanelActionis correct, not a regression. It is strictly narrower: a stale session-list inline approval now leaves.sessionListintact, and theif surface != .sessionListguard at:2081stopsshowNextPendingstealing it. Two lenses independently re-derived this. - No vacuous assertions in the 13 new tests. Each was mapped to the specific production line a break in which it would catch, and none answered "none". Two verified negatives recorded so a later round does not re-derive them: the
XCTAssertNotEqual(surface, …)assertions at:79and:222do not pincollapseStaleCardSurface(with the guard removed,shouldAutoOpenQuestionSurfacereturnstrueforAskUserQuestionand the surface becomes the other card — still not-equal, still green); onlytestStaleCardCollapsesWhenAutoOpenIsSuppressedgoes red. - Security posture unchanged from round 1. No injection surface, no credential or token handling; response bodies remain string literals or
JSONSerializationoutput.
Observations (non-blocking)
-
A hotkey with no matching card resolves the queue head unseen. With the island collapsed or showing a completion card or the session list,
approvalSessionId/questionSessionIdreturnnil,permissionIndex(expecting: nil)returns index 0, and the global shortcut (AppDelegate.swift:237-243, monitors at:201-211have no surface guard) grants or denies a request the user cannot see. This is byte-identical toorigin/main—eb5ef93deliberately restored it while fixing round-1 finding 2, and it is plausibly the intended "hotkeys work when the island is collapsed" behaviour. Both lenses that raised it agree it is a product decision for its own ticket, not a defect this MR introduced. Recorded so the trade is on record rather than accidental. -
The one test pinning the round-1 major fix depends on the machine's live GUI state.
AppStateAnswerRoutingTests.swift:98doestry XCTUnwrap(NSWorkspace.shared.frontmostApplication?.bundleIdentifier), which hard-fails on a headless runner, andshouldAutoOpenPendingSurfacere-reads frontmost state atAppState.swift:2081after the test's precondition assertion at:100-103, so a frontmost change in between fails the test spuriously. Recommended for acceptance, not fixing: the pattern is pre-existing house style (AppStateQuestionFlowTests.swift:96,:130), so CI must already tolerate it, and the real fix means threading the existingisTerminalFrontmostseam (AppState.swift:953) throughshowNextPending— production surgery in service of a test. The test asserts its own precondition, so it can only fail loudly, never pass vacuously.
Round-1 accepted findings — no argument against either acceptance
All three lenses were told findings 5 (within-session render-vs-resolve keying) and 10 (view-layer call-site wiring) were accepted rather than fixed, and were invited to argue. None did. Finding 10's reasoning was independently reinforced: the test-quality lens separated the enum accessor logic (cheap to pin — Finding 2 above) from the AppDelegate wiring (needs the harness), and recommends acceptance only for the latter.
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 claim is made about a pipeline; none ran for this target.
Schema
No schema change detected. Swift/SwiftUI package with no DDL, no persisted-store definition, and no code-only column or table dependency in the diff. All three lenses returned schema_change_detected: false.
⚠ 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 six findings fixed in c65b9d9e9d06c058886887ad84abd27f15da69b2 — including finding 4 (Codex-at-non-zero-index), which the panel scoped as cheap and was.
The major was again introduced by the previous round's fix: collapseStaleCardSurface (round 1) used queue membership as its liveness test, while dismissPermissionPrompt hides without dequeuing. Real regression against main, no misrouting. The guard now uses the same dismissed-filtered predicate as nextVisiblePermissionIndex(), and the test was verified red without it.
qa_introduced_blocking: 3 — half of this round's findings were on code round 1 wrote. Recorded here because that is the diminishing-returns signal the cycle is supposed to surface, not bury.
Verification: swift test — 703 tests, 2 skipped, 0 failures.
QA-Fix-Commit: c65b9d9
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
QA Round 3Panel: 3 lenses (contract-security, regression-edges, test-quality). No blocking findings. Contract VerificationContract synthesized from the PR title/description; no ticket linked (upstream defect wxtsky#308).
Finding 1 — Stale-dismiss test cannot fail on the routing bug its message names ↩ on code QA round 2 introduced
Finding 2 — Codex behind-another-session test asserts the dequeue but not the JSON-RPC reply path ↩ on code QA round 2 introduced
Summary
Both confirmed findings are ObservationsO-1 — Dismissed permission request stays queued, suppressing auto-open and sound for every later request (severity
SecurityNo blocking security findings. The change adds no auth, network, or credential surface; session ids SAST review not applicableTarget Round caveats
QA performed by Claude Code (claude-opus-5), manager + 3-lens panel
Round 3 disposition (main loop)Both findings fixed in One correction to finding 2 as scoped: the suggested discriminator (session status) does not distinguish the two paths — Cycle ending here. Round 3 raised Caveats this round carries, recorded rather than smoothed over:
Verification: QA-Fix-Commit: f40cee5 |
2 participants
Fixes the misrouting reported upstream in wxtsky#308: with several sessions waiting, answering the notch card could deliver the answer to a different session.
Cause
The card is addressed by session (
.questionCard(sessionId:)/.approvalCard(sessionId:)), but every answer path resolvedqueue.removeFirst(). Any mutation of the head while a card is open re-targets the answer:handlePeerDisconnect→drainQuestions/drainPermissions(forSession:)removes the head when a session's socket drops (answered in the terminal, CLI exited), andshowNextPending()only re-points the surface if Smart Suppress lets it auto-open — so a stale card can sit over a new head.showNextPending()reorderspermissionQueue; the tool-use cache removes entries by index.AskUserQuestionwizard collects answers over seconds of interaction.Card rendering had the same split: chrome (project name, cwd) came from the surface's session, question/tool content from the head.
Change
approvePermission/denyPermission/dismissPermissionPrompt/answerQuestion/answerQuestionMulti/skipQuestiontake an optionalexpectedSessionIdand resolve that session's queued request.showNextPending()re-opens whatever is genuinely waiting — no dead card, no fall-through to the head.pendingPermission(forSession:)/pendingQuestion(forSession:)back the cards, so what is displayed and what is resolved cannot disagree.expectedSessionId: nilkeeps head-of-queue behaviour for surfaces that only ever mirror the head (iPhone/Watch Buddy,answerCompanionQuestion, Codex app-server) — the Buddy wire protocol carries no session id, so that path is unchanged and still head-based.Tests
Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift— 7 cases covering cross-session answer/skip/approve routing, the drained-head stale-card case for both queues, session-scoped card lookup, and the unchanged head behaviour when no session is passed.Full suite: 694 tests, 0 failures.