diff --git a/src/ccbot/bot/messages.py b/src/ccbot/bot/messages.py index 641193c8..ea246193 100644 --- a/src/ccbot/bot/messages.py +++ b/src/ccbot/bot/messages.py @@ -1306,6 +1306,12 @@ async def _dispatch_text_to_active( ) sess = session_manager.find_session_by_window(wid) + # ``send_to_window`` and Codex's submit verification can take long + # enough for the user to switch sessions. The ``owns_card`` value + # captured before those awaits is no longer authoritative: using it + # below would let the old session resume/repost the carrier that the + # switcher has already handed to the new active session. + owns_card = sess is not None and is_active_for_user(user_id, sess) if sess is not None: session_manager.touch_session(sess.id) # ``maybe_auto_name`` honours the user's ``haiku_naming`` @@ -1326,6 +1332,9 @@ async def _dispatch_text_to_active( if sess is None: return True + # Re-check immediately before the card mutation as well. Auto-name, + # interactive-UI handling, and other post-send work above may await. + owns_card = is_active_for_user(user_id, sess) if not owns_card: # Background session (voice pinned here, user moved on). # Its only chat surface is a row in the active card's diff --git a/src/ccbot/handlers/notifications.py b/src/ccbot/handlers/notifications.py index 3c5a5552..406b12b4 100644 --- a/src/ccbot/handlers/notifications.py +++ b/src/ccbot/handlers/notifications.py @@ -1108,24 +1108,18 @@ async def resume_card_view(bot: Bot, user_id: int, sess: Session) -> None: """Drop the menu-pause so future events render again, and re-paint the carrier with the buffered events. - CRITICAL: clears ``in_menu_view`` UNCONDITIONALLY when the state - exists — even when ``msg_id`` was lost (carrier stale / deleted / - not yet created). Earlier this returned early without clearing - the pause, leaving the card stuck in ``must_buffer=True`` forever - (symptom: chronic ``card_update buffered`` log, body never updates - even though claude is producing events). When ``msg_id`` is None - we still clear the flag; the next claude event spawns a fresh - card via ``_send_card`` because ``state.msg_id is None``. + For the currently-active session, clears ``in_menu_view`` even when + ``msg_id`` was lost (carrier stale / deleted / not yet created). Earlier + this returned early without clearing the pause, leaving the active card + stuck in ``must_buffer=True`` forever. A background session is the one + exception: its pause and carrier binding must remain untouched so a late + voice dispatch cannot reclaim the newly-active session's carrier. """ # ``setdefault`` so a session with no card-state yet (just-switched # bg session via Shot's switcher) still lands on a visible surface. # Without this, resume_card_view silently bailed and Back left the # user staring at empty chat. state = _cards.setdefault((user_id, sess.id), CardState()) - state.in_menu_view = False - if state.pending_edit is not None and not state.pending_edit.done(): - state.pending_edit.cancel() - state.pending_edit = None async def _spawn_fresh() -> None: await _ensure_seeded(user_id, sess, state) @@ -1140,25 +1134,44 @@ async def _spawn_fresh() -> None: # during ``_ensure_seeded`` / ``_send_card`` can race and produce a # duplicate card via ``update_session_card``. async with _card_lock(user_id, sess.id): - if state.msg_id is None: - # No carrier — spawn a fresh card now so the user lands on a - # visible surface immediately (used by Shot → Back after #51's - # ``close_card_view`` drops msg_id). Previously we waited for - # the next claude event; on quiet sessions that left the user - # staring at empty chat. + # The active-session check and the Telegram edit share the same + # cross-session barrier as switcher hand-off. A slow voice dispatch + # may have started while this session was active and resumed after the + # carrier moved elsewhere; it must not clear the old card's pause or + # repaint the new owner's carrier. + async with _carrier_edit_lock(user_id): + if not is_active_for_user(user_id, sess): + logger.info( + "card_resume skip user=%d sess=%s reason=background", + user_id, + sess.id, + ) + return + state.in_menu_view = False + if state.pending_edit is not None and not state.pending_edit.done(): + state.pending_edit.cancel() + state.pending_edit = None + if state.msg_id is None: + # No carrier — spawn a fresh card now so the user lands on a + # visible surface immediately (used by Shot → Back after #51's + # ``close_card_view`` drops msg_id). Previously we waited for + # the next claude event; on quiet sessions that left the user + # staring at empty chat. + await _spawn_fresh() + return + text = _render_card(sess, state, user_id=user_id) + keyboard = build_footer_keyboard(user_id, screen="main", is_busy=True) + if await _edit_card_unlocked( + bot, user_id, state, text=text, reply_markup=keyboard + ): + state.last_rendered = text + state.last_edit_ts = time.monotonic() + return + # ``_edit_card_unlocked`` returned False — the carrier was lost + # (stale msg, already-deleted, or bot can't edit it) and already + # reset msg_id internally. Spawn a fresh card so the user still + # lands on a visible live surface. await _spawn_fresh() - return - text = _render_card(sess, state, user_id=user_id) - keyboard = build_footer_keyboard(user_id, screen="main", is_busy=True) - if await _edit_card(bot, user_id, state, text=text, reply_markup=keyboard): - state.last_rendered = text - state.last_edit_ts = time.monotonic() - return - # ``_edit_card`` returned False — the carrier was lost (stale msg, - # already-deleted, or bot can't edit it) and ``_edit_card`` has - # already reset msg_id internally. Spawn a fresh card so the user - # still lands on a visible live surface. - await _spawn_fresh() async def paint_card_on_carrier( diff --git a/src/ccbot/terminal_parser.py b/src/ccbot/terminal_parser.py index 72304cd9..8ca6445a 100644 --- a/src/ccbot/terminal_parser.py +++ b/src/ccbot/terminal_parser.py @@ -103,6 +103,20 @@ class UIPattern: re.compile(r"^\s*Press enter to confirm or esc to cancel", re.IGNORECASE), ), ), + UIPattern( + # A tall Codex approval can push its header and the first two choices + # above the visible tmux viewport. The negative third choice and the + # footer remain pinned at the bottom, so use that pair as the fallback + # signature. Keep the CodexApproval classification: auto-approve must + # send Codex's documented ``y`` hotkey even though the visible pane no + # longer contains the ``1. Yes, proceed (y)`` line. + name="CodexApproval", + top=(re.compile(r"^\s*3\.\s*No\b.*\(esc\)\s*$", re.IGNORECASE),), + bottom=( + re.compile(r"^\s*Press enter to confirm or esc to cancel", re.IGNORECASE), + ), + min_gap=1, + ), UIPattern( # Permission menu with numbered choices (no "Esc to cancel" line) name="PermissionPrompt", diff --git a/tests/ccbot/handlers/test_card_switch_race.py b/tests/ccbot/handlers/test_card_switch_race.py index e66fbc89..3862febe 100644 --- a/tests/ccbot/handlers/test_card_switch_race.py +++ b/tests/ccbot/handlers/test_card_switch_race.py @@ -128,3 +128,27 @@ async def controlled_rich_edit( assert rendered[0] == "old-session late update" assert "target-session" in rendered[1] assert target_state.in_menu_view is False + + +@pytest.mark.asyncio +async def test_late_resume_cannot_reclaim_background_carrier(monkeypatch): + """A voice dispatch that resumes after hand-off must leave the old + session paused and must not edit the carrier now owned by the target.""" + user_id = 42 + carrier_msg_id = 8000 + old = _session("session-a", "old-session", "@1") + target = _session("session-b", "target-session", "@2") + session_manager.sessions.update({old.id: old, target.id: target}) + session_manager.active_sessions[user_id] = target.id + + old_state = CardState(msg_id=carrier_msg_id, in_menu_view=True) + notifications._cards[(user_id, old.id)] = old_state + monkeypatch.setattr(session_manager, "save_state", lambda: None) + rich_edit = AsyncMock(return_value=True) + monkeypatch.setattr(message_sender, "try_rich_edit", rich_edit) + + await notifications.resume_card_view(AsyncMock(), user_id, old) + + assert old_state.in_menu_view is True + assert old_state.msg_id == carrier_msg_id + rich_edit.assert_not_awaited() diff --git a/tests/ccbot/test_card_in_front.py b/tests/ccbot/test_card_in_front.py index 7638a5dd..ec598276 100644 --- a/tests/ccbot/test_card_in_front.py +++ b/tests/ccbot/test_card_in_front.py @@ -122,6 +122,62 @@ async def test_bg_dispatch_does_not_arm_repost_intent(self): begin.assert_not_called() + @pytest.mark.asyncio + async def test_switch_during_dispatch_rechecks_before_repainting(self): + """The session starts active, but loses the carrier while Codex is + accepting the prompt. A stale pre-send owns_card value must not wake + its paused card after the switch.""" + update = _make_update() + context = _make_context() + + pinned = MagicMock() + pinned.id = "sessA" + pinned.backend = "codex" + active = True + + async def _send_and_switch(*args, **kwargs): + nonlocal active + active = False + return True, "ok" + + mock_sm = MagicMock() + mock_sm.find_session_by_window.return_value = pinned + mock_sm.send_to_window = AsyncMock(side_effect=_send_and_switch) + + repost = AsyncMock() + resume = AsyncMock() + refresh = AsyncMock() + + with ( + patch("ccbot.bot.messages.session_manager", mock_sm), + patch( + "ccbot.bot.messages.is_active_for_user", + side_effect=lambda user_id, sess: active, + ), + patch("ccbot.bot.messages.repost_card", new=repost), + patch("ccbot.bot.messages.resume_card_view", new=resume), + patch("ccbot.bot.messages.refresh_panel", new=refresh), + patch("ccbot.bot.messages.fire_typing", new=AsyncMock()), + patch("ccbot.bot.messages.get_interactive_window", return_value=None), + patch( + "ccbot.bot.messages.tmux_manager.ensure_codex_prompt_submitted", + new=AsyncMock(return_value=True), + ), + patch( + "ccbot.handlers.bg_status.update_status", return_value=True + ) as bg_update, + ): + from ccbot.bot.messages import _dispatch_text_to_active + + await _dispatch_text_to_active(update, context, 1, "@5", "hi there") + + # Entry wakes the then-active card once. After the switch no second + # resume/repost is allowed; only the new active card's bg panel moves. + assert resume.await_count == 1 + repost.assert_not_awaited() + bg_update.assert_called_once_with(1, "sessA", "working") + refresh.assert_awaited_once() + class TestActiveDispatchPutsCardInFront: @pytest.mark.asyncio diff --git a/tests/ccbot/test_terminal_parser.py b/tests/ccbot/test_terminal_parser.py index 445f02b9..9404eb97 100644 --- a/tests/ccbot/test_terminal_parser.py +++ b/tests/ccbot/test_terminal_parser.py @@ -197,6 +197,22 @@ def test_codex_command_approval(self): assert "YT_PROXY=hahn" in result.content assert "Press enter to confirm" in result.content + def test_codex_command_approval_header_and_yes_choices_scrolled_off(self): + # Long command previews can leave only choice 3 and the footer in the + # visible tmux pane. This must stay a CodexApproval so auto-approve + # uses the off-screen but still active ``y`` hotkey. + pane = ( + ' print(json.dumps([{"id": r.get("id")} for r in items]))\n' + " 3. No, and tell Codex what to do differently (esc)\n" + "\n" + " Press enter to confirm or esc to cancel\n" + ) + result = extract_interactive_content(pane) + assert result is not None + assert result.name == "CodexApproval" + assert "3. No, and tell Codex" in result.content + assert "Press enter to confirm" in result.content + def test_codex_numbered_permission_cursor_is_recognized(self): pane = ( "› 1. Yes, proceed (y)\n 2. Yes, and don't ask again (p)\n 3. No (esc)\n" diff --git a/tests/test_kb_mode_debounce.py b/tests/test_kb_mode_debounce.py index e8ec18ee..f271980f 100644 --- a/tests/test_kb_mode_debounce.py +++ b/tests/test_kb_mode_debounce.py @@ -223,6 +223,28 @@ async def test_codex_approval_uses_documented_y_hotkey_without_enter(): send_keys.assert_awaited_once_with("@1", "y", enter=False, literal=True) +@pytest.mark.asyncio +async def test_truncated_codex_approval_uses_documented_y_hotkey_without_enter(): + pane = ( + "tail of a long command preview\n" + " 3. No, and tell Codex what to do differently (esc)\n" + "\n" + "Press enter to confirm or esc to cancel\n" + ) + send_keys = AsyncMock() + with ( + patch.object( + status_polling.session_manager, + "get_user_settings", + lambda u: {"auto_approve": "on"}, + ), + patch.object(status_polling.tmux_manager, "send_keys", send_keys), + ): + assert await _maybe_auto_approve(1, "@1", pane) is True + + send_keys.assert_awaited_once_with("@1", "y", enter=False, literal=True) + + @pytest.mark.asyncio async def test_auto_approve_distinct_prompts_never_escalate(): """Different prompts (distinct signatures) each get a one-shot auto-Yes;