Skip to content

fix(panel): route answers to the card's session, not the queue head (#308) - #310

Open
halindrome wants to merge 4 commits into
wxtsky:mainfrom
halindrome:fix/answer-routing
Open

fix(panel): route answers to the card's session, not the queue head (#308)#310
halindrome wants to merge 4 commits into
wxtsky:mainfrom
halindrome:fix/answer-routing

Conversation

@halindrome

Copy link
Copy Markdown
Contributor

Fixes #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 resolved queue.removeFirst(), and the card rendered the head of the queue while taking its chrome (project name, cwd) from the surface's session. Any mutation of the head while a card is open re-targets the answer:

  • handlePeerDisconnectdrainQuestions/drainPermissions(forSession:) removes the head when a session's socket drops (answered in the terminal, CLI exited), and showNextPending() only re-points the surface if Smart Suppress lets it auto-open — so a stale card can sit over a new head.
  • showNextPending() reorders permissionQueue; the tool-use cache removes entries by index.
  • A multi-question AskUserQuestion wizard collects answers over seconds of interaction.

Change

  • approvePermission / denyPermission / dismissPermissionPrompt / answerQuestion / answerQuestionMulti / skipQuestion take an optional expectedSessionId and resolve that session's queued request.
  • pendingPermission(forSession:) / pendingQuestion(forSession:) back the cards, so what is displayed and what is resolved cannot disagree.
  • showNextPending() collapses a card surface whose session has nothing actionable left, so a drained or dismissed request can never leave an expanded, empty notch. "Actionable" uses the same dismissed-filtered predicate as nextVisiblePermissionIndex().
  • Panel cards, session-list inline approvals, and keyboard shortcuts pass the acting session; the shortcut accessors are kind-matched so a permission hotkey fired over a question card addresses nothing.
  • expectedSessionId: nil keeps head-of-queue behaviour for the surfaces that only ever mirror the head — iPhone/Watch Buddy control commands, answerCompanionQuestion, Codex app-server replies. The Buddy wire protocol carries no session id, so that path is unchanged by design and could be tightened separately.

Tests

Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift (new, 15 cases) plus one addition to AppStateCodexRequestUserInputTests: cross-session answer/skip/approve/dismiss routing, the drained-head stale-card case for both queues, the Smart-Suppress path that produces a blank card, the dismissed-card case, session-scoped lookups, the kind-matched surface accessors, a Codex app-server request answered while queued behind another session, and the unchanged head behaviour when no session is passed.

swift test: 703 tests, 2 skipped, 0 failures.

Each behavioural guard was verified by mutating the production path it names and confirming the test fails without it — not just that it passes with it.

Review

This branch went through three review rounds before being opened here; the round notes are on the fork PR (halindrome#1) for anyone who wants the detail. Rounds 1 and 2 each found a real regression introduced by the previous round's fix — both are described in the commit messages, and both are fixed. Round 3 found no blocking defects.

Two things reviewers may want to weigh in on:

  • Within-session keying. Requests are addressed by session, not by request id. Two requests from the same session could in principle render one and resolve the other; the mechanism is reachable by reading but I could not reproduce the timing window, so I left it rather than widening the change. Carrying a request id into the surface enum would close it.
  • View-layer wiring is unpinned. Reverting the call-site changes in NotchPanelView / AppDelegate leaves the suite green — pinning that needs a SwiftUI view-test harness the project does not have.

Separately, #309 (dismissing an approval silences later requests from all sessions) was found while reviewing this and is untouched here.

shanemccarron-maker and others added 4 commits August 12, 2026 08:58
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
nguyenvanduocit pushed a commit to nguyenvanduocit/CodeIsland that referenced this pull request Aug 13, 2026
…ug added

Two new upstream bugs found (wxtsky/CodeIsland issues wxtsky#308/wxtsky#309, PRs wxtsky#310/wxtsky#311, Aug 12):
- T-083 (new, high, S): answer routing bug — approve/deny/answer always resolves
  queue.removeFirst() not the card's session; confirmed in our RequestQueueService.swift
- T-031 criteria updated: implementing dismiss button MUST fix the enqueuePermission
  count==1 gate simultaneously (PR wxtsky#311) to avoid silencing later permission requests
- Issue wxtsky#312 (SoundManager testability) noted, not separately tracked
- vibeisland issue wxtsky#216 (keychain dump) is vibe-island-specific, skip

Upstream v1.0.31 remains HEAD; PRs wxtsky#305, wxtsky#295, wxtsky#285 still open.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MDtfgMUiL8o81HJiWtqB2D
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.

Answer from the notch card can be delivered to the wrong session when several sessions are waiting

2 participants