diff --git a/omnigent/claude_native_forwarder.py b/omnigent/claude_native_forwarder.py index b5a44548..f6d31863 100644 --- a/omnigent/claude_native_forwarder.py +++ b/omnigent/claude_native_forwarder.py @@ -256,17 +256,23 @@ _SUPERVISOR_HEALTHY_UPTIME_S = 60.0 # published on the per-conversation SSE stream. Unmapped events emit # no status. # -# ``Stop`` → idle and ``StopFailure`` → failed are the authoritative -# turn-end edges (each fires once when Claude finishes / errors a turn); -# they drive sub-agent terminal delivery via the codex-shared -# ``external_session_status`` path (→ parent inbox + wake). The -# PTY-activity ``idle`` cannot: it is a ~1s-quiescence heuristic that -# oscillates on every mid-turn lull, so delivering on it fired a -# premature completion and idempotently locked out the real one. -# ``UserPromptSubmit`` → running stays PTY-derived — the pane watcher -# drives the UI running/idle badge and catches what ``Stop`` misses -# (interrupts, compaction failures, TUI edits). ``_publish_status`` -# keeps ``failed`` sticky against the trailing PTY idle. +# Claude's own ``sessions/.json`` owns the running/idle badge (see +# :mod:`omnigent.claude_native_status_file`), so these two hooks exist for +# what the file cannot express: +# +# - ``Stop`` → idle: the sub-agent terminal-delivery edge (→ parent inbox + +# wake, via the codex-shared ``external_session_status`` path). It fires +# exactly once per finished turn, where the PTY-activity ``idle`` was a +# ~1s-quiescence heuristic that oscillated on mid-turn lulls, firing a +# premature completion that idempotently locked out the real one. It also +# carries the background-shell count. It agrees with the file rather than +# competing with it, so arrival order does not matter — the shared edge +# dedup collapses the pair. +# - ``StopFailure`` → failed: the file has no failure literal (it returns to +# ``idle`` on a turn error exactly as on success), so this is the only +# source of the red pill, ``last_task_error``, and a failed scheduled run. +# ``_publish_status`` keeps it sticky against a trailing ``idle``; the +# file's next ``busy`` clears it on the following turn. _HOOK_EVENT_TO_STATUS: dict[str, str] = { "Stop": "idle", "StopFailure": "failed", @@ -670,12 +676,6 @@ class _ForwardDedupeState: # sub-agent spend so the gate can block mid-turn. Separate baseline # because it can advance while ``posted_cost`` (S) is frozen. posted_policy_cost: float | None = None - # Response id of the last turn-start ``running`` status POSTed, so the - # id-bearing running edge fires exactly once per turn even when an - # assistant item is held across polls for delta ordering (which leaves - # ``state.current_response_id`` unadvanced). ``None`` until the first - # turn-start edge. Reset on /clear and /fork like the other baselines. - posted_running_response_id: str | None = None # Turn-settle latch driving the scheduled-wake boundary. The Stop edge # records the ended turn's id as PENDING; it activates (moves to # ``settled_response_id``) only once a fully-consumed transcript batch @@ -3057,19 +3057,18 @@ async def _forward_available_status_events( retry_key = f"hook:{record.event_cursor}:{record.byte_offset}:{status}" if retry_tracker.retry_delay_s(retry_key) is not None: return durable - effective_status = status - if status == "idle" and record.background_task_count > 0: - effective_status = "waiting" try: await post_external_session_status( client, session_id=session_id, - status=effective_status, + status=status, response_id=response_id, - # Only the ``Stop`` (idle/waiting) edge carries an authoritative + # Only the ``Stop`` (idle) edge carries an authoritative # background-shell count — ``0`` clears the tally, ``N`` sets it. - # ``StopFailure`` (failed) clears it on the server regardless, so - # leave its count off the wire. + # This is the one thing the status file cannot report: its + # ``shell`` literal is a boolean, and the indicator renders a + # number. ``StopFailure`` (failed) clears it on the server + # regardless, so leave its count off the wire. background_task_count=( None if status == "failed" else record.background_task_count ), @@ -3204,31 +3203,6 @@ async def _ensure_state_for_transcript( return state -def _turn_has_assistant_output(items: list[ClaudeTranscriptItem], response_id: str) -> bool: - """ - Whether ``response_id`` has assistant-generated output among ``items``. - - The turn-start ``running`` edge should open a streaming turn only for an id - that a later ``Stop``/``StopFailure`` hook will close — i.e. one produced by - an actual LLM turn. Assistant text (``message`` with ``role=assistant``) and - tool calls (``function_call``) qualify; a ``slash_command`` (``/model``, - ``/effort``) or ``terminal_command`` (``!cmd``) item opens an id with no LLM - turn behind it, so it must not. - - :param items: Transcript items read this poll. - :param response_id: The current turn's response id. - :returns: ``True`` when an assistant-output item carries ``response_id``. - """ - for item in items: - if item.response_id != response_id: - continue - if item.item_type == "function_call": - return True - if item.item_type == "message" and item.data.get("role") == "assistant": - return True - return False - - def _promote_pending_settle( dedupe: _ForwardDedupeState, items: list[ClaudeTranscriptItem] ) -> bool: @@ -3487,55 +3461,15 @@ async def _forward_available_items( current_response_id = result.current_response_id seen_source_ids = list(state.seen_source_ids) seen = set(seen_source_ids) - # NOTE: the old "re-assert running on resumed agent output" hack lived - # here. It only existed to paper over the hook model's compaction - # blind spot (``Stop`` → idle, then an ``isCompactSummary`` resume that - # never fired ``UserPromptSubmit``). PTY-activity status makes it - # obsolete: the pane keeps changing through a mid-turn compaction, so - # the runner's watcher holds the session ``running`` directly. - # - # Turn-start edge: the first time we see a turn's response id, publish a - # ``running`` status carrying it. The PTY watcher already drives the - # running/idle BADGE with a bare (id-less) status; this id-bearing edge is - # what lets ap-web open a *streaming* ``activeResponse`` for the turn, so - # the forwarded tool-call cards (which carry the same response id) render - # LIVE — spinner + elapsed timer — instead of as static completed cards. - # Deduped on the persistent ``dedupe`` baseline (NOT ``state``): when an - # assistant item is held across polls for delta ordering, this function - # early-returns with ``state`` unadvanced, so a ``state``-based guard would - # re-fire ``running`` every poll of the hold window. Best-effort — a failed - # status post must not abort item forwarding (the items below are the - # primary payload); the turn-end idle/failed edge still carries the id to - # close the lifecycle, and the badge is unaffected either way. - # - # Only open the streaming turn for an id that has ASSISTANT output in this - # poll's items. A surfaced CLI built-in (``/model``, ``/effort``) or a - # ``!cmd`` becomes a slash_command / terminal_command item that opens its - # own response id but runs no LLM turn, so no ``Stop`` hook ever fires to - # close it — a ``running`` opened for it would strand the web composer in - # its "Stop"/busy state until the next real message. A skill that DOES - # trigger an LLM turn shares its id with the assistant text it produces, so - # ``running`` still fires — one poll later, when that output appears. - if ( - current_response_id is not None - and dedupe.posted_running_response_id != current_response_id - and _turn_has_assistant_output(items, current_response_id) - ): - try: - await post_external_session_status( - client, - session_id=session_id, - status="running", - response_id=current_response_id, - ) - dedupe.posted_running_response_id = current_response_id - except httpx.HTTPError: - _logger.warning( - "Failed to forward Claude turn-start running status; session=%s response_id=%s", - session_id, - current_response_id, - exc_info=True, - ) + # This function publishes no session status. Claude's own + # ``sessions/.json`` owns the running/idle badge (see + # :mod:`omnigent.claude_native_status_file`), and it reports the turn ending + # the moment Claude settles. A status edge derived from the transcript can + # only fire once a poll has parsed assistant output, so it lands *after* the + # file's ``idle`` on a short turn and re-asserts ``running`` on a session + # that already finished — the user sees idle → running → idle. Items carry + # their own ``response_id`` (see :func:`_post_external_conversation_item`), + # so the transcript's job here is items, not status. updated = state for item in items: if item.source_id in seen: diff --git a/omnigent/claude_native_status_file.py b/omnigent/claude_native_status_file.py index c13df2b4..b247d178 100644 --- a/omnigent/claude_native_status_file.py +++ b/omnigent/claude_native_status_file.py @@ -4,8 +4,10 @@ Claude Code writes a per-process JSON file at ``/sessions/.json`` (its internal "concurrentSessions" registry, present since v2.1.139 — the file that also backs ``claude agents``). For an interactive session it carries a ``status`` -field that flips ``idle`` ⇄ ``busy`` ⇄ ``waiting`` as the agent works, -which is a cleaner running/idle signal than diffing the tmux pane. +field that flips ``idle`` ⇄ ``busy`` ⇄ ``waiting`` as the agent works. It +reports what Claude is doing rather than inferring it from pane redraws, so +it — not the tmux pane diff — is the session's running/idle status whenever +it is readable. This module owns two pure pieces the claude-native status watcher builds on: @@ -25,12 +27,15 @@ watcher that consumes this treats an unresolved / unreadable file as from __future__ import annotations import json +import logging import os import time from collections.abc import Callable from dataclasses import dataclass from pathlib import Path +_logger = logging.getLogger(__name__) + # Runner-side status vocabulary the file maps onto. ``busy`` and # ``waiting`` both mean "the turn is not finished" from the session's point # of view, so both map to ``running``; ``waiting`` is distinguished for the @@ -49,9 +54,9 @@ _STATUS_TO_RUNNER: dict[str, str] = { "waiting": RUNNING, "idle": IDLE, # The turn ended but a background shell is still alive (Claude Code - # >= v2.1.197). The agent loop is idle, so this maps to ``idle`` — the - # Stop hook separately relabels its own ``idle`` to ``waiting`` with the - # shell tally, which is what keeps the spinner lit. Mapping ``shell`` to + # >= v2.1.197). The agent loop is idle, so this maps to ``idle``; the + # working indicator stays lit off the ``Stop`` hook's shell tally, which + # carries the count this boolean literal cannot. Mapping ``shell`` to # ``running`` would strand the composer on the "(queued)" placeholder, # since the session never reads idle while a background shell runs. "shell": IDLE, @@ -273,10 +278,12 @@ class SessionStatusPoller: - **Exhausted:** if resolution never succeeds, :attr:`active` stays ``False`` permanently and the file contributes nothing. - The poller never displaces the PTY watcher: it supplies an *additional* - status edge at Claude's real turn boundary, plus the freshness-bounded - :meth:`asserts_running` level the watcher consults before declaring a - quiet pane idle. + While :attr:`active` the poller *is* the session's status — it reports what + Claude is doing, where the pane diff only infers it from redraws — and the + PTY watcher publishes none. The watcher takes over when no file was ever + resolved (Claude older than v2.1.139) and always owns pane death, which the + file structurally cannot report: a killed Claude leaves its record behind + (see :meth:`retire`). :param on_status: Callback invoked as ``(runner_status, blocked_on)`` on each transition (and once on first read). Fires when either part @@ -339,46 +346,61 @@ class SessionStatusPoller: def _try_resolve(self) -> None: """Attempt one resolution, retiring to the PTY watcher on timeout.""" self._attempts += 1 + pane_pid = self._pane_pid_getter() path = resolve_status_file( - pane_pid=self._pane_pid_getter(), + pane_pid=pane_pid, expected_session_id=self._session_id_getter(), config_dir=self._config_dir, ) if path is not None: + # Log the hit: whether the file was found at all decides which + # source owns the session's status, and without this the answer is + # only reachable by re-deriving the resolution by hand. + _logger.info( + "claude status file resolved: path=%s attempts=%d pane_pid=%s", + path, + self._attempts, + pane_pid, + ) self._path = path return if self._attempts >= _MAX_RESOLVE_ATTEMPTS: + _logger.warning( + "claude status file never resolved after %d attempts " + "(pane_pid=%s); session status falls back to the pane watcher", + self._attempts, + pane_pid, + ) self._exhausted = True - def asserts_running(self, *, ttl_s: float, now: float | None = None) -> bool: - """Whether the file *recently* reported the session as running. + def retire(self) -> None: + """Stop reading the file, permanently. - The file is written only when its value changes, so its status is a - level that can outlive the truth — Claude keeps reporting ``busy`` - while a delegate or background task is active, long after the turn - itself ended. Callers therefore treat it as authoritative only for - *ttl_s* after the write, and fall back to the pane watcher once it - goes stale rather than pinning the session to ``running`` forever. - - :param ttl_s: How long after ``statusUpdatedAt`` the level is still - trusted, in seconds. - :param now: Wall-clock override (tests); uses :func:`time.time` - when ``None``. - :returns: ``True`` when the last read said running and is still fresh. + Called when the pane's process is gone: a killed Claude does not unlink + its file, so the record survives holding whatever it last said. Since + the file owns the session's status while it is readable, a dead pane + must retire it or that final value would pin the session forever. """ - status = self._last_status - if status is None or status.runner_status != RUNNING: - return False - # ``waiting`` does not decay: a dialog owns Claude's input until it - # closes, and closing it changes the value — so a new write is - # guaranteed. ``busy`` decays, because a delegate or background task - # keeps it set long after the turn it belongs to has ended. - if status.raw_status == "waiting": - return True - if status.status_updated_at is None: - return False - clock = time.time() if now is None else now - return clock - status.status_updated_at / 1000.0 <= ttl_s + _logger.info("claude status file retired: path=%s", self._path) + self._exhausted = True + + def resync(self) -> None: + """Forget what was published so the next tick re-asserts the file. + + The file is written only when its value *changes*, so a poller that + already published ``running`` has nothing more to say until Claude's + status moves. That is a problem when the *listener* restarts: a server + recycle wipes its status cache, and the session would sit on a stale + ``idle`` for the rest of the turn because every source believes it + already reported. Dropping the edge/mtime baselines makes the next tick + publish the file's current value verbatim. + + Keeps the resolved path and the attempt count — this re-asserts a + working poller, it does not restart resolution. + """ + _logger.info("claude status file resync: path=%s", self._path) + self._last_mtime = None + self._last_edge = None @property def blocked_on(self) -> str | None: diff --git a/omnigent/runner/app.py b/omnigent/runner/app.py index 31ec9104..e659d63c 100644 --- a/omnigent/runner/app.py +++ b/omnigent/runner/app.py @@ -8824,6 +8824,18 @@ def create_runner_app( ) async def _catch_up_scan() -> None: + # The tunnel just reconnected, which usually means the SERVER restarted + # (deploy, crash, replica failover) and lost its in-memory session-status + # cache. This runner did not restart, so every status source still + # believes its last edge was delivered and nothing re-asserts — a + # native session mid-turn during the restart would sit on a stale + # ``idle`` for the rest of the turn. Re-arm them before the item scan + # below (which skips native harnesses entirely). + if resource_registry is not None: + try: + resource_registry.resync_session_statuses() + except Exception: # noqa: BLE001 — best-effort; never block catch-up. + _logger.warning("Session status resync failed after reconnect", exc_info=True) for session_id in list(_session_histories): if _is_native_harness(session_id): continue diff --git a/omnigent/runner/resource_registry.py b/omnigent/runner/resource_registry.py index 75fc0d86..c3f89337 100644 --- a/omnigent/runner/resource_registry.py +++ b/omnigent/runner/resource_registry.py @@ -97,16 +97,6 @@ _CLAUDE_NATIVE_STATUS_IDLE_THRESHOLD_SECONDS = 1.0 # don't 5x the capture-pane subprocess load on every terminal. _CLAUDE_NATIVE_STATUS_POLL_INTERVAL_SECONDS = 0.2 -# How long Claude's ``sessions/.json`` status stays trusted as a *level* -# after it was written. The file is rewritten only when its value changes, so -# a ``busy`` written for a delegate or background task outlives the turn that -# produced it; honouring it forever would pin the session to "Working…" with -# no way back. Inside this window a quiet pane is read as "parked on a prompt" -# (the case the pane diff genuinely cannot see) and the session stays running; -# past it the pane watcher decides. Comfortably above the 1s idle threshold so -# a real prompt is not lost to the gap between the write and the pane settling. -_CLAUDE_NATIVE_STATUS_FILE_LEVEL_TTL_SECONDS = 10.0 - # Minimum wall-clock interval (seconds) between consecutive # ``session.terminal.activity`` emissions for a single terminal. The # claude-native agent terminal polls its pane every 200ms @@ -382,6 +372,11 @@ class SessionResourceRegistry: # which the turn-start hook also writes — deduping against that one # would swallow the turn's real ``running``. self._published_session_status: dict[str, tuple[str, str | None]] = {} + # Live claude-native status-file pollers, per session. Held so a + # reconnect can re-arm them (see :meth:`resync_session_statuses`) — the + # poller keeps its own edge/mtime baselines on the watcher thread, and + # clearing the registry's baseline alone would leave those intact. + self._status_pollers: dict[str, SessionStatusPoller] = {} # Optional callback invoked on the event loop when a watched terminal # disappears unexpectedly. The callback receives the terminal's # lifecycle relationship so the runner can decide whether the owning @@ -472,6 +467,7 @@ class SessionResourceRegistry: """Pop and return the session's recorded PTY status (or ``None``).""" with self._lock: self._published_session_status.pop(session_id, None) + self._status_pollers.pop(session_id, None) return self._last_session_status.pop(session_id, None) def _claim_status_edge(self, session_id: str, status: str, blocked_on: str | None) -> bool: @@ -497,6 +493,42 @@ class SessionResourceRegistry: with self._lock: self._published_session_status[session_id] = (status, None) + def resync_session_statuses(self) -> None: + """Re-arm every status source so it republishes what it already sent. + + Called after the runner's tunnel reconnects. A server recycle (deploy, + crash, replica failover) restarts the *listener*, wiping its in-memory + status cache — but this runner keeps running, so every dedup baseline + still asserts the pre-restart edge was delivered. Nothing re-asserts on + its own: Claude's status file is written only when its value *changes*, + and the pane watcher's edges are coalesced, so a session mid-turn during + the restart would sit on a stale ``idle`` until its next turn boundary — + no spinner, no stop button, for the rest of the turn. + + Dropping the published-edge baselines here makes the next poll publish + the current status verbatim. The claude-native pollers are re-armed too: + they hold their own edge/mtime baselines on the watcher thread, so + clearing only this side would leave them silent. + + Deliberately does NOT clear ``_last_session_status`` — that memo + classifies terminal exits (clean vs mid-turn crash) and is unrelated to + what the server has heard. + """ + with self._lock: + sessions = sorted(self._published_session_status) + self._published_session_status.clear() + pollers = list(self._status_pollers.values()) + for poller in pollers: + poller.resync() + if sessions or pollers: + _logger.info( + "Re-arming session status after tunnel reconnect: " + "cleared_edges=%d pollers=%d sessions=%s", + len(sessions), + len(pollers), + sessions, + ) + def note_session_turn_started(self, session_id: str) -> None: """Mark a session as having an in-flight turn. @@ -1152,16 +1184,6 @@ class SessionResourceRegistry: # means "never emitted", so the first changed tick always fires. last_activity_emit: dict[str, float | None] = {"value": None} - def _blocked_reason() -> str | None: - # The reason rides every edge, not just the poller's own, so a - # redrawing pane under a dialog doesn't publish a bare ``running`` - # that erases it. - if status_poller is None or not status_poller.asserts_running( - ttl_s=_CLAUDE_NATIVE_STATUS_FILE_LEVEL_TTL_SECONDS - ): - return None - return status_poller.blocked_on - def _publish_status(status: str, blocked_on: str | None = None) -> None: # Publish one running/idle edge: dedup against the last value, # memo for exit classification, and hop to the loop (publishers @@ -1177,6 +1199,15 @@ class SessionResourceRegistry: self._set_session_status_memo(session_id, status) loop.call_soon_threadsafe(status_publisher, session_id, status, blocked_on) + def _file_owns_status() -> bool: + # Once Claude's own status file is readable it is the session's + # status: it reports what Claude is doing, where the pane diff only + # infers it from redraws. The pane keeps its activity-badge and + # pane-death jobs, but must not publish status alongside the file — + # two publishers is what made a post-turn redraw fight the file's + # ``idle`` and needed a freshness window to arbitrate. + return status_poller is not None and status_poller.active + # claude-native additionally reads Claude's own ``sessions/.json`` # status (present since Claude Code v2.1.139): it flips on the real # turn edge and knows when a dialog owns the input, neither of which @@ -1193,6 +1224,9 @@ class SessionResourceRegistry: if emit_status and resource_role == CLAUDE_NATIVE_TERMINAL_ROLE else None ) + if status_poller is not None: + with self._lock: + self._status_pollers[session_id] = status_poller def _on_activity() -> None: # Runs on the watcher daemon thread; hop to the loop so the @@ -1214,15 +1248,20 @@ class SessionResourceRegistry: loop.call_soon_threadsafe(activity_publisher, session_id, resource_id) # Pane changed → the agent is working. Coalesce to the # idle→running edge so a continuously-redrawing pane doesn't - # re-emit ``running`` every poll. Always published, never deferred - # to the status file: that file is rewritten only when its value - # *changes*, so a turn starting while it already reads ``busy`` - # produces no write at all — and the session would sit on its - # stale ``idle`` for the whole turn with no working indicator. - if emit_status: - _publish_status("running", _blocked_reason()) + # re-emit ``running`` every poll. Skipped once the status file owns + # the session (see :func:`_file_owns_status`) — a post-turn prompt + # redraw is not a new turn, and the file already said so. + if emit_status and not _file_owns_status(): + _publish_status("running") def _on_exit() -> None: + # The pane's process is gone, which the status file cannot report — + # a killed Claude never unlinks it, so the record survives holding + # its last value. Retire the poller before classifying the exit so + # that stale value can't keep owning the session's status. + if status_poller is not None: + status_poller.retire() + def _schedule() -> None: task = asyncio.create_task( self._handle_terminal_exit( @@ -1274,16 +1313,12 @@ class SessionResourceRegistry: # Pane quiet for the claude-native status threshold → the # agent has stopped. Edge-triggered: re-arms only after new # output mutates the pane (which flips back to ``running``). - # Held back only while the status file *freshly* reports running: - # a dialog owning the input quiets the pane without ending the - # turn, which the pane diff alone cannot tell from a finished one. - # Past that freshness window the pane decides, so a ``busy`` left - # standing by a background task can't pin the session to running. + # Skipped once the status file owns the session: a dialog owning + # the input quiets the pane without ending the turn, and only the + # file can tell that from a finished one. # Edge ordering: the watcher thread runs idle/exit serially, so # this idle commits before any later on_exit reads the memo. - if status_poller is None or not status_poller.asserts_running( - ttl_s=_CLAUDE_NATIVE_STATUS_FILE_LEVEL_TTL_SECONDS - ): + if not _file_owns_status(): _publish_status("idle") # Clear the activity throttle so the next working episode emits # its first pulse immediately, keeping the activity badge @@ -1510,6 +1545,10 @@ class SessionResourceRegistry: moved_status = self._last_session_status.pop(source_session_id, None) if moved_status is not None and target_session_id not in self._last_session_status: self._last_session_status[target_session_id] = moved_status + # The watcher restart below rebuilds the poller under the + # target, so drop the source's entry rather than leaving a + # retired poller to be re-armed on every later reconnect. + self._status_pollers.pop(source_session_id, None) try: await entry.instance.set_conversation_link( self._terminal_registry.conversation_link_for_id(target_session_id) diff --git a/omnigent/server/routes/_sessions/helpers.py b/omnigent/server/routes/_sessions/helpers.py index e2d8f584..317691e5 100644 --- a/omnigent/server/routes/_sessions/helpers.py +++ b/omnigent/server/routes/_sessions/helpers.py @@ -6479,9 +6479,12 @@ async def _run_compact_locked( code=ErrorCode.INVALID_INPUT, ) task_id = f"compact_{int(time.time() * 1000)}" - _publish_status(session_id, "running") - # compact() publishes its own in_progress / completed SSE events - # when conversation_id is set — don't double-publish here. + # compact() publishes its own in_progress / completed SSE events when + # conversation_id is set, and the web UI's compaction bubble owns the + # busy state from those. Deliberately no ``session.status`` bracket: + # compaction is not an agent turn, so reporting running→idle would + # invent one — and its idle would land mid-turn on a session that is + # genuinely working, which clients then had to second-guess. from omnigent.runtime.workflow import compact_conversation_now try: @@ -6497,12 +6500,10 @@ async def _run_compact_locked( _logger.exception("Explicit session compaction failed for %s", session_id) detail = str(exc) or repr(exc) _publish_compaction_failed(session_id) - _publish_status(session_id, "idle") raise OmnigentError( f"Compaction failed while generating a summary: {detail}", code=ErrorCode.INTERNAL_ERROR, ) from exc - _publish_status(session_id, "idle") def _agent_provider_family(agent: Agent) -> str | None: diff --git a/omnigent/server/routes/sessions/routes_events.py b/omnigent/server/routes/sessions/routes_events.py index a3507f9b..d0d1ccf6 100644 --- a/omnigent/server/routes/sessions/routes_events.py +++ b/omnigent/server/routes/sessions/routes_events.py @@ -473,7 +473,6 @@ def register_events_routes( # deny sentinel on the session stream so the # client/REPL sees feedback. reason = _input_verdict.get("reason", "Denied by policy") - _publish_status(session_id, "running") _publish_policy_deny(session_id, reason) await _persist_policy_deny_sentinel( session_id, @@ -482,8 +481,16 @@ def register_events_routes( conversation_store, agent_store, ) - # Terminal response.completed before idle so live-tail - # consumers (the headless ``-p`` client) unblock. + # Terminal ``response.completed`` renders the sentinel; the + # trailing ``idle`` ends the turn. A request-phase deny/refuse + # IS a turn boundary — the client optimistically went "working" + # on send — but the message never reached a harness, so nothing + # downstream (harness / interrupt / status file) emits the + # turn-end. This ``idle`` is that signal; clients that settle a + # turn only on a ``session.status`` edge (the REPL, the headless + # ``-p`` client) hang without it. No leading ``running``: the + # turn never dispatched, and a phantom ``running`` would fold a + # concurrent live bubble mid-stream. _publish_input_deny_terminal(session_id, conv, reason) _publish_status(session_id, "idle") # Return the same shape the client expects from POST @@ -505,7 +512,6 @@ def register_events_routes( ) if _input_verdict is not None: reason = _input_verdict.get("reason", "Denied by policy") - _publish_status(session_id, "running") _publish_policy_deny(session_id, reason) await _persist_policy_deny_sentinel( session_id, @@ -514,7 +520,9 @@ def register_events_routes( conversation_store, agent_store, ) - # Terminal response.completed before idle (see message branch). + # Terminal response.completed + trailing ``idle`` turn-end (see + # the message branch above for why the idle is required and the + # leading running is not). _publish_input_deny_terminal(session_id, conv, reason) _publish_status(session_id, "idle") return {"queued": False, "denied": True, "reason": reason} @@ -883,7 +891,9 @@ def register_events_routes( # A background-task ``waiting`` marks an ended turn, so deliver it # as ``idle``: the session takes a new message now, and for a # sub-agent the terminal-delivery branch below must fire (otherwise - # the orchestrator hangs). The tally still drives the spinner. + # the orchestrator hangs). The tally still drives the indicator. + # The claude-native forwarder no longer sends ``waiting`` at all — + # this normalizes it for runners that predate that change. effective_status = _background_task_delivery_status(status, bg_count, conv) if effective_status != status: status = effective_status diff --git a/tests/runner/test_resource_registry.py b/tests/runner/test_resource_registry.py index bed54f20..47fc4e4d 100644 --- a/tests/runner/test_resource_registry.py +++ b/tests/runner/test_resource_registry.py @@ -407,25 +407,29 @@ async def _observe_native_agent_terminal_and_capture( class _FakeStatusPoller: """Controllable stand-in for the claude-native status-file poller. - Lets a test flip :attr:`active` (file resolved vs. falling back), flip - :attr:`running_level` (the file freshly reports running vs. its value - having gone stale), and fire status edges through the registry's - callback, without touching a real ``sessions/.json``. + Lets a test flip :attr:`active` (file resolved and therefore owning the + session's status, vs. the PTY watcher falling back) and fire status edges + through the registry's callback, without touching a real + ``sessions/.json``. """ def __init__(self, on_status: object) -> None: self._on_status = on_status self.active = False - self.running_level = False self.blocked_on: str | None = None self.ticks = 0 + self.retired = False + self.resyncs = 0 def tick(self) -> None: self.ticks += 1 - def asserts_running(self, *, ttl_s: float, now: float | None = None) -> bool: - del ttl_s, now - return self.running_level + def retire(self) -> None: + self.retired = True + self.active = False + + def resync(self) -> None: + self.resyncs += 1 def emit(self, status: str, blocked_on: str | None = None) -> None: """Simulate the file reporting a new status.""" @@ -503,53 +507,60 @@ async def test_claude_native_wires_status_poller_tick(tmp_path: Path) -> None: @pytest.mark.asyncio -async def test_pty_activity_publishes_running_even_with_active_status_file( - tmp_path: Path, -) -> None: - """Pane activity publishes ``running`` even while the file poller is active. +async def test_pane_publishes_no_status_while_the_file_owns_it(tmp_path: Path) -> None: + """An active file poller is the only status source; the pane publishes none. - Claude rewrites ``sessions/.json`` only when its value *changes*, so a - turn that starts while the file already reads ``busy`` produces no write at - all. If the file were allowed to mute the pane watcher, nothing could - publish ``running`` and the session would sit on a stale ``idle`` — no - working indicator, no stop button — for the whole turn. + Claude redraws its prompt after a turn and blinks a cursor, so the pane + keeps changing once the file has already said ``idle``. Letting both + publish is what made that redraw contradict the file and needed a + freshness window to arbitrate — so while the file is readable it decides, + and the pane's edges are dropped. """ callbacks, statuses, pollers, _registry = await _observe_native_with_fake_poller( - tmp_path, "conv_pre" + tmp_path, "conv_file_owns" ) poller = pollers[0] poller.active = True - # The file already holds ``busy`` from an earlier turn, so it emits nothing. - callbacks["on_activity"]() - - # Status edges publish via loop.call_soon_threadsafe; let them drain. + poller.emit("running") + callbacks["on_activity"]() # pane redraws mid-turn — no second edge await asyncio.sleep(0) assert statuses == ["running"] + poller.emit("idle") + callbacks["on_activity"]() # post-turn prompt redraw is not a new turn + callbacks["on_idle"]() # nor does a quiet pane re-assert idle + await asyncio.sleep(0) + assert statuses == ["running", "idle"] + @pytest.mark.asyncio -async def test_pty_idle_deferred_only_while_file_freshly_running(tmp_path: Path) -> None: - """A quiet pane goes idle unless the file *freshly* reports running. +async def test_parked_pane_stays_running_then_recovers_on_pane_death(tmp_path: Path) -> None: + """A dialog keeps the session running; a dead pane still ends it. - A dialog owning Claude's input quiets the pane without ending the turn, so - a fresh ``running`` level holds the idle edge back. Once that level goes - stale — a ``busy`` left standing by a background task outlives its turn — - the pane decides again, so the session can never be pinned to ``running``. + While Claude is parked on a prompt the pane is quiet but the turn is not + over, and only the file knows that — so the quiet pane must not publish + ``idle``. But a killed Claude leaves that ``waiting`` record behind, so + pane death retires the poller and the PTY side owns the outcome. Without + that, the session would spin forever. """ callbacks, statuses, pollers, _registry = await _observe_native_with_fake_poller( - tmp_path, "conv_idle_gate" + tmp_path, "conv_parked" ) poller = pollers[0] poller.active = True - poller.running_level = True - callbacks["on_activity"]() # → running - callbacks["on_idle"]() # suppressed: file freshly says running + poller.emit("running", "permission prompt") + callbacks["on_idle"]() # pane quiet under the dialog — turn is NOT over await asyncio.sleep(0) assert statuses == ["running"] - poller.running_level = False # the file's level went stale + # Claude is killed at the prompt. Its file survives holding ``waiting``. + callbacks["on_exit"]() + assert poller.retired is True + assert poller.active is False + + # The pane now owns status again, so the session can settle. callbacks["on_idle"]() await asyncio.sleep(0) assert statuses == ["running", "idle"] @@ -557,29 +568,93 @@ async def test_pty_idle_deferred_only_while_file_freshly_running(tmp_path: Path) @pytest.mark.asyncio async def test_hook_status_resyncs_watcher_dedup(tmp_path: Path) -> None: - """A forwarder's hook-derived edge rebases the watcher's dedup. + """A forwarder's hook-derived edge rebases the shared dedup baseline. ``Stop`` → ``idle`` is posted to the server by the claude-native forwarder, - bypassing this watcher. Without adopting it as the baseline the watcher - still believes ``running`` is live and swallows the next turn's genuine - ``running`` as a duplicate, leaving the session stuck on the hook's idle. + bypassing this watcher. Adopting it as the baseline is what makes the pair + idempotent: the file's own ``idle`` lands on the same edge and is collapsed, + so the two agree regardless of which arrives first. """ callbacks, statuses, pollers, registry = await _observe_native_with_fake_poller( tmp_path, "conv_resync" ) - pollers[0].active = True + poller = pollers[0] + poller.active = True - callbacks["on_activity"]() + poller.emit("running") await asyncio.sleep(0) assert statuses == ["running"] # The forwarder posts Stop → idle straight to the server. registry.note_external_session_status("conv_resync", "idle") - # Next turn: the pane moves again and must re-publish ``running``. - callbacks["on_activity"]() + # The file catches up moments later with the same edge — deduped away, so + # the user sees one idle rather than a flicker. + poller.emit("idle") + await asyncio.sleep(0) + assert statuses == ["running"] + + # Next turn: the file reports work again and must publish. + poller.emit("running") await asyncio.sleep(0) assert statuses == ["running", "running"] + del callbacks + + +@pytest.mark.asyncio +async def test_reconnect_resync_republishes_a_running_session(tmp_path: Path) -> None: + """A server restart mid-turn must not strand the session on a stale status. + + The tunnel reconnecting means the *listener* restarted and lost its status + cache. This runner did not, so its dedup baseline still asserts ``running`` + was delivered — and Claude's file is written only when its value *changes*, + so nothing re-asserts on its own. Without the resync the session would show + no spinner and no stop button for the rest of the turn. + """ + _callbacks, statuses, pollers, registry = await _observe_native_with_fake_poller( + tmp_path, "conv_restart" + ) + poller = pollers[0] + poller.active = True + + poller.emit("running") + await asyncio.sleep(0) + assert statuses == ["running"] + + # Mid-turn, the file's value is unchanged, so a re-read publishes nothing: + # this is exactly what leaves the restarted server on a stale status. + poller.emit("running") + await asyncio.sleep(0) + assert statuses == ["running"] + + registry.resync_session_statuses() + assert poller.resyncs == 1 + + # The same value now republishes, so the fresh server learns the truth. + poller.emit("running") + await asyncio.sleep(0) + assert statuses == ["running", "running"] + + +@pytest.mark.asyncio +async def test_reconnect_resync_keeps_the_exit_classification_memo(tmp_path: Path) -> None: + """The resync clears published edges, not the exit memo. + + ``_last_session_status`` decides whether a terminal exit reads as a clean + shutdown or a mid-turn crash. It tracks what the PANE last did, not what the + server has heard, so a reconnect must leave it alone — clearing it would make + a crash right after a reconnect look like a tidy exit. + """ + _callbacks, _statuses, pollers, registry = await _observe_native_with_fake_poller( + tmp_path, "conv_memo" + ) + pollers[0].active = True + pollers[0].emit("running") + await asyncio.sleep(0) + + registry.resync_session_statuses() + + assert registry._take_session_status_memo("conv_memo") == "running" @pytest.mark.asyncio @@ -1336,13 +1411,13 @@ def test_resolve_environment_runner_workspace_overrides_absolute_spec_cwd( @pytest.mark.asyncio -async def test_blocked_reason_rides_pane_edges(tmp_path: Path) -> None: - """The parked reason travels with every edge, not just the file's own. +async def test_blocked_reason_survives_pane_redraws(tmp_path: Path) -> None: + """The parked reason survives the pane redrawing underneath the dialog. - Claude reports ``waitingFor`` once, when the dialog opens. The pane keeps - redrawing underneath it, so if a pane-derived ``running`` shipped without - the reason it would immediately erase what the file just said and the UI - would fall back to a bare spinner. + Claude reports ``waitingFor`` once, when the dialog opens, and the pane + keeps redrawing while it is up. Because the file owns status outright the + pane publishes nothing, so there is no bare ``running`` to erase the reason + — it stands until the file itself drops it. """ terminal_registry = TerminalRegistry() registry = SessionResourceRegistry(terminal_registry=terminal_registry) @@ -1387,15 +1462,15 @@ async def test_blocked_reason_rides_pane_edges(tmp_path: Path) -> None: poller = pollers[0] poller.active = True - poller.running_level = True poller.blocked_on = "permission prompt" poller.emit("running", "permission prompt") callbacks["on_activity"]() # pane redraw under the dialog + callbacks["on_idle"]() # and the quiet spells between redraws await asyncio.sleep(0) assert edges == [("running", "permission prompt")] - # Dialog answered: the file drops the reason and the pane keeps moving. + # Dialog answered: the file drops the reason on its own edge. poller.blocked_on = None poller.emit("running", None) await asyncio.sleep(0) diff --git a/tests/server/integration/test_sessions_child_sessions.py b/tests/server/integration/test_sessions_child_sessions.py index 8dcc39fe..da12c48e 100644 --- a/tests/server/integration/test_sessions_child_sessions.py +++ b/tests/server/integration/test_sessions_child_sessions.py @@ -1753,19 +1753,18 @@ async def test_subagent_idle_forward_recovers_via_parent_when_child_runner_stale assert recovered_for == [child["id"]] -async def test_subagent_background_task_waiting_delivers_to_parent_as_idle( +async def test_subagent_background_task_count_still_delivers_to_parent( client: httpx.AsyncClient, monkeypatch: pytest.MonkeyPatch, ) -> None: - """A sub-agent's background-task ``waiting`` still delivers terminal status. + """A lingering background shell must not strand the parent orchestrator. - Regression for the parent-orchestrator hang: a claude-native sub-agent - relabels its ``Stop`` turn-end ``idle`` to ``waiting`` when a background - shell lingers. The terminal-delivery branch only fires for - ``idle``/``failed``, so an un-collapsed ``waiting`` would skip delivery and - the parent would wait forever. The server must collapse the sub-agent's - background-task ``waiting`` to ``idle`` so delivery (here, the recovery - path) still runs for the child. + Regression for the parent-orchestrator hang. The ``Stop`` turn-end edge + carries the background-shell count, and the terminal-delivery branch fires + only for ``idle``/``failed`` — so the edge has to stay ``idle`` and let the + count ride alongside. (It used to be relabeled to ``waiting`` for the + spinner's sake, which skipped delivery and made the parent wait forever; + the spinner now stays lit off the count instead.) """ child = await _create_native_child(client, name="orch-bg-waiting") @@ -1788,13 +1787,13 @@ async def test_subagent_background_task_waiting_delivers_to_parent_as_idle( f"/v1/sessions/{child['id']}/events", json={ "type": "external_session_status", - "data": {"status": "waiting", "background_task_count": 1}, + "data": {"status": "idle", "background_task_count": 1}, }, ) - # Delivery fired despite the incoming `waiting`: the collapse to `idle` - # let the terminal-status branch run for THIS child (recovery invoked, - # 202 Accepted) instead of silently skipping and stranding the parent. + # A positive count does not suppress delivery: the terminal-status branch + # ran for THIS child (recovery invoked, 202 Accepted) rather than silently + # skipping and stranding the parent. assert resp.status_code == 202, resp.text assert recovered_for == [child["id"]] diff --git a/tests/test_claude_native_forwarder.py b/tests/test_claude_native_forwarder.py index fa4a26d8..c61a15e5 100644 --- a/tests/test_claude_native_forwarder.py +++ b/tests/test_claude_native_forwarder.py @@ -1180,13 +1180,10 @@ async def test_forwarder_posts_visible_transcript_items(tmp_path: Path) -> None: ) ) try: - # Collect the seven transcript items. This transcript's final turn is a - # ``!bash`` command (a ``terminal_command``, no assistant output), so - # ``current_response_id`` lands on a turn that runs no LLM turn and thus - # gets no id-bearing ``running`` edge (that would strand the web UI busy - # with no ``Stop`` hook to close it). The turn-start ``running`` edge is - # asserted for a real assistant turn in - # ``test_forwarder_emits_turn_start_running_with_response_id``. + # Collect the seven transcript items. The transcript path publishes no + # session status at all — Claude's status file owns the badge — which + # ``test_forwarder_publishes_no_status_for_assistant_output`` asserts + # directly. requests = [await _get_recorded_item_request(server) for _index in range(7)] finally: task.cancel() @@ -1385,9 +1382,8 @@ async def test_forwarder_posts_web_injected_terminal_transcript_items(tmp_path: ) ) try: - # The turn-start ``running`` status posts first (the transcript has an - # assistant turn), then the assistant message item. - running = await _get_recorded_request(server) + # The item posts FIRST: the transcript path publishes no status at all + # (Claude's status file owns the badge), so nothing precedes it. request = await _get_recorded_request(server) finally: task.cancel() @@ -1397,8 +1393,6 @@ async def test_forwarder_posts_web_injected_terminal_transcript_items(tmp_path: server.server_close() thread.join(timeout=5.0) - assert running["body"]["type"] == "external_session_status" - assert running["body"]["data"]["status"] == "running" assert request["path"] == "/v1/sessions/conv_abc/events" assert request["body"]["type"] == "external_conversation_item" assert request["body"]["data"]["item_type"] == "message" @@ -3023,19 +3017,17 @@ async def test_forwarder_drops_poison_item_after_bounded_permanent_retries( ) persisted = json.loads((bridge_dir / "transcript_forwarder.json").read_text("utf-8")) - # The turn-start ``running`` status (carrying the turn's response id) leads, - # then the poison item is attempted twice, then the forwarder-failed status. + # The poison item is attempted twice, then the forwarder-failed status. No + # status POST leads: the transcript path publishes none (Claude's status + # file owns the badge). assert [request["type"] for request in requests] == [ - "external_session_status", "external_conversation_item", "external_conversation_item", "external_session_status", ] - # The turn-start ``running`` edge carries the turn's response id, and the - # failed edge carries BOTH the drop reason as ``output`` (#1113 — the - # server surfaces it as the failure detail) and that same response id so + # The failed edge carries BOTH the drop reason as ``output`` (#1113 — the + # server surfaces it as the failure detail) and the turn's response id so # it closes the streaming turn instead of leaving its tool cards spinning. - assert requests[0]["data"]["status"] == "running" assert requests[-1]["data"] == { "status": "failed", "output": "transcript item poison-item:0:message rejected", @@ -5716,16 +5708,16 @@ def test_promote_pending_settle_waits_for_turn_quiescence() -> None: @pytest.mark.asyncio -async def test_scheduled_wake_forwards_marker_and_new_running_edge(tmp_path: Path) -> None: +async def test_scheduled_wake_forwards_marker_under_a_new_turn_id(tmp_path: Path) -> None: """ The full wake pipeline: settle → quiet-poll promote → marked new turn. Poll 1 forwards a turn; its Stop edge records the pending settle (covered by the status-events test — recorded directly here). Poll 2 is quiet and promotes the settle, persisting it. Poll 3 sees new - assistant entries — a cron firing writes no user entry — and must - POST a fresh turn-start ``running`` edge plus the scheduled-wake - marker ahead of the resumed output, all under a new response id. + assistant entries — a cron firing writes no user entry — and must POST + the scheduled-wake marker ahead of the resumed output, all under a new + response id. """ bridge_dir = tmp_path / "bridge" transcript_path = tmp_path / "session.jsonl" @@ -5818,13 +5810,15 @@ async def test_scheduled_wake_forwards_marker_and_new_running_edge(tmp_path: Pat dedupe=dedupe, ) - kinds = [request["type"] for request in requests] - assert kinds[0] == "external_session_status" - running = requests[0]["data"] - wake_turn_id = running["response_id"] - assert running["status"] == "running" - assert wake_turn_id != turn_one_id + # No status POST: the transcript path publishes none (Claude's status file + # owns the badge). The wake is observable entirely in the items — a fresh + # turn id plus the marker ahead of the resumed output. + assert [request["type"] for request in requests] == ["external_conversation_item"] * len( + requests + ) items = [r["data"] for r in requests if r["type"] == "external_conversation_item"] + wake_turn_id = items[0]["response_id"] + assert wake_turn_id != turn_one_id assert [item["item_data"]["role"] for item in items] == ["user", "assistant"] assert items[0]["item_data"]["content"] == [ {"type": "input_text", "text": "[System: scheduled prompt fired]"} @@ -7945,16 +7939,17 @@ async def test_subagent_start_drop_writes_dead_letter(tmp_path: Path) -> None: @pytest.mark.asyncio -async def test_forwarder_posts_waiting_when_stop_has_background_tasks( +async def test_forwarder_posts_idle_with_count_when_stop_has_background_tasks( tmp_path: Path, ) -> None: """ - ``Stop`` with ``background_tasks`` → ``waiting`` instead of ``idle``. + ``Stop`` with ``background_tasks`` posts ``idle`` plus the shell count. - When Claude Code's Stop hook carries a non-empty ``background_tasks`` - array (shells still running), the forwarder must publish ``waiting`` - so the web UI keeps showing the spinner. Without this, the chat - interface shows "idle" while the terminal shows "1 shell running". + The turn really has ended, so the status is ``idle`` — the spinner stays + lit off the count instead (``showsWorking`` is ``isWorking || tally > 0``). + The count is the one thing Claude's status file cannot report: its + ``shell`` literal is a boolean and the indicator renders a number, which + is why this hook still posts at all. """ bridge_dir = tmp_path / "bridge" transcript_path = tmp_path / "session.jsonl" @@ -8008,7 +8003,7 @@ async def test_forwarder_posts_waiting_when_stop_has_background_tasks( assert request["path"] == "/v1/sessions/conv_abc/events" assert request["body"] == { "type": "external_session_status", - "data": {"status": "waiting", "background_task_count": 1}, + "data": {"status": "idle", "background_task_count": 1}, } @@ -8106,15 +8101,15 @@ async def test_forward_status_events_stamps_response_id_on_idle(tmp_path: Path) @pytest.mark.asyncio -async def test_forwarder_emits_turn_start_running_with_response_id(tmp_path: Path) -> None: +async def test_forwarder_publishes_no_status_for_assistant_output(tmp_path: Path) -> None: """ - The first assistant output of a turn publishes ``running`` + its response id. + Assistant output forwards items and publishes NO session status. - Native Claude's running/idle BADGE stays PTY-derived; this id-bearing - ``running`` edge is the additional signal that lets ap-web open a streaming - ``activeResponse`` for the turn, so the forwarded tool cards (which share - the same response id) render LIVE rather than as static completed cards. - The running edge's response id must equal the forwarded items' response id. + Claude's ``sessions/.json`` owns the running/idle badge. A status edge + derived from the transcript can only fire once a poll has parsed assistant + output, so on a short turn it lands *after* the file's ``idle`` and + re-asserts ``running`` on a session that already finished — the user sees + idle → running → idle. The items still carry their own ``response_id``. """ bridge_dir = tmp_path / "bridge" transcript_path = tmp_path / "session.jsonl" @@ -8171,9 +8166,7 @@ async def test_forwarder_emits_turn_start_running_with_response_id(tmp_path: Pat ) ) try: - # First POST of the poll is the turn-start running status (it runs - # before the items in _forward_available_items); the two items follow. - running = await _get_recorded_request(server) + # Both POSTs of the poll are items — no status edge precedes them. item_a = await _get_recorded_request(server) item_b = await _get_recorded_request(server) finally: @@ -8184,20 +8177,90 @@ async def test_forwarder_emits_turn_start_running_with_response_id(tmp_path: Pat server.server_close() thread.join(timeout=5.0) - assert running["body"]["type"] == "external_session_status" - assert running["body"]["data"]["status"] == "running" - running_rid = running["body"]["data"]["response_id"] - assert isinstance(running_rid, str) and running_rid - # The running edge's response id matches the ASSISTANT turn's forwarded - # item (the function_call), so that bubble enters the streaming lifecycle - # on the client. The user message carries its own distinct response id. + # Neither POST is a status edge — the transcript path publishes none. + assert [body["body"]["type"] for body in (item_a, item_b)] == [ + "external_conversation_item", + "external_conversation_item", + ] + # The assistant turn's item still carries its own response id, which is + # what groups its bubble and its tool cards on the client. function_call = next( - body - for body in (item_a, item_b) - if body["body"]["type"] == "external_conversation_item" - and body["body"]["data"]["item_type"] == "function_call" + body for body in (item_a, item_b) if body["body"]["data"]["item_type"] == "function_call" ) - assert function_call["body"]["data"]["response_id"] == running_rid + rid = function_call["body"]["data"]["response_id"] + assert isinstance(rid, str) and rid + + +@pytest.mark.asyncio +async def test_short_turn_poll_posts_items_without_a_status_edge(tmp_path: Path) -> None: + """ + Regression: a short turn's poll must not re-assert ``running``. + + The status file reports the turn ending the moment Claude settles, but a + transcript-derived edge can only fire once a poll has parsed assistant + output — so it arrived *after* that ``idle`` and flipped the session back to + ``running``, then ``Stop`` closed it again: the user saw + idle → running → idle on every short turn. + """ + bridge_dir = tmp_path / "bridge" + transcript_path = tmp_path / "session.jsonl" + transcript_path.write_text( + "\n".join( + [ + json.dumps( + { + "type": "user", + "uuid": "u1", + "message": {"role": "user", "content": "i'll keep testing"}, + } + ), + json.dumps( + { + "type": "assistant", + "uuid": "a1", + "message": { + "role": "assistant", + "content": [{"type": "text", "text": "Sounds good."}], + }, + } + ), + ] + ) + + "\n", + encoding="utf-8", + ) + state = forwarder.TranscriptForwardState( + transcript_path=transcript_path, + line_cursor=0, + byte_offset=0, + cursor_fingerprint=forwarder._jsonl_cursor_fingerprint(transcript_path, 0), + ) + posted: list[dict[str, Any]] = [] + + def _handle_request(request: httpx.Request) -> httpx.Response: + """ + Record every forwarder POST body. + + :param request: Outbound HTTP request from the forwarder. + :returns: HTTP 202 for the mock Omnigent endpoint. + """ + posted.append(json.loads(request.content.decode("utf-8"))) + return httpx.Response(202, json={}) + + transport = httpx.MockTransport(_handle_request) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + await forwarder._forward_available_items( + client=client, + session_id="conv_abc", + bridge_dir=bridge_dir, + agent_name="claude-native-ui", + state=state, + retry_tracker=forwarder._PostRetryTracker(), + dedupe=forwarder._ForwardDedupeState(), + ) + + assert [body["type"] for body in posted] == ["external_conversation_item"] * 2 + assert not [body for body in posted if body["type"] == "external_session_status"] @pytest.mark.asyncio diff --git a/tests/test_claude_native_status_file.py b/tests/test_claude_native_status_file.py index dc81afc4..74112b98 100644 --- a/tests/test_claude_native_status_file.py +++ b/tests/test_claude_native_status_file.py @@ -266,13 +266,13 @@ def test_unknown_status_clears_dedup_so_next_read_publishes(tmp_path: Path) -> N assert published == [RUNNING, RUNNING] -def test_asserts_running_only_while_fresh(tmp_path: Path) -> None: - """The running level is trusted only briefly after it was written. +def test_stale_busy_is_reported_as_written(tmp_path: Path) -> None: + """A long-standing ``busy`` still reads as running — no freshness window. - Claude keeps reporting ``busy`` while a delegate or background task is - active, long after the turn ended. Treating that as authoritative forever - would pin the session to "Working…"; past the window the pane watcher - decides again. + Claude reports ``busy`` while a delegate or background shell works, which + can outlast the turn that started it, and that is correct: it *is* still + doing work. The file says what Claude is doing, so we report it as written + rather than timing it out and second-guessing with a pane diff. """ sessions = tmp_path / "sessions" now = 1785480100.0 @@ -281,7 +281,7 @@ def test_asserts_running_only_while_fresh(tmp_path: Path) -> None: pid=1, session_id="s", status="busy", - status_updated_at=int((now - 2) * 1000), + status_updated_at=int((now - 3600) * 1000), ) published: list[str] = [] poller = SessionStatusPoller( @@ -291,19 +291,100 @@ def test_asserts_running_only_while_fresh(tmp_path: Path) -> None: config_dir=tmp_path, ) poller.tick() - assert poller.asserts_running(ttl_s=10.0, now=now) is True - assert poller.asserts_running(ttl_s=1.0, now=now) is False + assert published == [RUNNING] + assert poller.active is True - # An idle level never asserts running, however fresh. + +def test_retire_stops_the_file_owning_status(tmp_path: Path) -> None: + """A dead pane retires the poller, even with a readable file left behind. + + Claude does not unlink its status file when killed, so the record survives + holding whatever it last said — here a ``waiting`` that would otherwise + keep asserting the session is parked mid-turn forever. Pane death is the + one thing the file cannot report, so the watcher retires the poller and + the PTY side owns the outcome. + """ + sessions = tmp_path / "sessions" _write_session_file( - sessions, - pid=1, - session_id="s", - status="idle", - status_updated_at=int(now * 1000), + sessions, pid=1, session_id="s", status="waiting", blocked_on="input needed" + ) + published: list[tuple[str, str | None]] = [] + poller = SessionStatusPoller( + on_status=lambda status, reason: published.append((status, reason)), + pane_pid_getter=_StubPidGetter(1), + session_id_getter=lambda: "s", + config_dir=tmp_path, ) poller.tick() - assert poller.asserts_running(ttl_s=10.0, now=now) is False + assert published == [(RUNNING, "input needed")] + assert poller.active is True + + poller.retire() + assert poller.active is False + + # The file is still there and still readable; a retired poller reads it no + # more, so its stale value cannot keep owning the session's status. + _write_session_file(sessions, pid=1, session_id="s", status="busy") + poller.tick() + assert published == [(RUNNING, "input needed")] + + +def test_resync_republishes_an_unchanged_file(tmp_path: Path) -> None: + """A resync makes the next tick re-assert the file's current value. + + The file is rewritten only when its value *changes*, so a poller mid-turn + has nothing more to say — which strands the server when the SERVER is what + restarted and lost its cache. A resync drops the edge/mtime baselines so the + same value publishes again. + """ + sessions = tmp_path / "sessions" + _write_session_file(sessions, pid=1, session_id="s", status="busy") + published: list[tuple[str, str | None]] = [] + poller = SessionStatusPoller( + on_status=lambda status, reason: published.append((status, reason)), + pane_pid_getter=_StubPidGetter(1), + session_id_getter=lambda: "s", + config_dir=tmp_path, + ) + poller.tick() + assert published == [(RUNNING, None)] + + # Unchanged file: normally silent, which is the whole problem. + poller.tick() + assert published == [(RUNNING, None)] + + poller.resync() + poller.tick() + assert published == [(RUNNING, None), (RUNNING, None)] + # Still reading the same resolved file — a resync re-asserts a working + # poller rather than restarting resolution. + assert poller.active is True + + +def test_resync_does_not_revive_a_retired_poller(tmp_path: Path) -> None: + """A retired poller stays retired across a reconnect. + + Retirement means the pane's process is gone (or no file ever resolved), so + the PTY side owns the outcome. A reconnect must not hand ownership back to a + dead Claude's leftover record. + """ + sessions = tmp_path / "sessions" + _write_session_file(sessions, pid=1, session_id="s", status="busy") + published: list[str] = [] + poller = SessionStatusPoller( + on_status=lambda status, _reason: published.append(status), + pane_pid_getter=_StubPidGetter(1), + session_id_getter=lambda: "s", + config_dir=tmp_path, + ) + poller.tick() + poller.retire() + + poller.resync() + poller.tick() + + assert poller.active is False + assert published == [RUNNING] def test_waiting_carries_its_reason(tmp_path: Path) -> None: @@ -351,12 +432,12 @@ def test_poller_publishes_when_only_the_reason_changes(tmp_path: Path) -> None: assert poller.blocked_on == "dialog open" -def test_waiting_level_does_not_decay(tmp_path: Path) -> None: +def test_parked_session_stays_running_indefinitely(tmp_path: Path) -> None: """A dialog holds the session open however long it stays up. - Unlike ``busy`` — which a background task keeps set past its turn — a - ``waiting`` clears only when Claude writes a new status, so it is safe to - trust indefinitely and wrong to time out (the pane is quiet the whole time). + ``waiting`` clears only when Claude writes a new status, so an hour-old + parked record still reports running — the pane is quiet the whole time and + only the file can tell that from a finished turn. """ sessions = tmp_path / "sessions" now = 1785480100.0 @@ -376,4 +457,5 @@ def test_waiting_level_does_not_decay(tmp_path: Path) -> None: config_dir=tmp_path, ) poller.tick() - assert poller.asserts_running(ttl_s=10.0, now=now) is True + assert published == [(RUNNING, "input needed")] + assert poller.blocked_on == "input needed" diff --git a/web/src/lib/renderItems.test.ts b/web/src/lib/renderItems.test.ts index 6963fcff..77913b82 100644 --- a/web/src/lib/renderItems.test.ts +++ b/web/src/lib/renderItems.test.ts @@ -751,6 +751,110 @@ describe("buildBubbles — tool joining", () => { }); }); +describe("buildBubbles — session-driven trailing tool spinner", () => { + // claude-native has no streaming `activeResponse` — its running/idle lives + // in `sessionStatus`. The `sessionRunning` arg spins the newest turn's + // trailing tool phase so a dispatched-but-unresolved tool shows a spinner + // instead of "No output", without any bubble reaching lifecycle "streaming". + function toolState(bubbles: Bubble[]): string { + const asst = bubbles[bubbles.length - 1] as Extract; + const tool = asst.items.find( + (item): item is Extract => item.kind === "tool", + ); + expect(tool).toBeDefined(); + return tool!.state; + } + + const danglingToolTurn: AnyBlock[] = [ + { + type: "tool_group", + ctx: ctx({ itemId: "fc_1", responseId: "resp_1" }), + executions: [mkExec("Bash", "c1")], + iteration: 0, + }, + ]; + + it("spins the newest turn's trailing tool while the session is running", () => { + // No activeResponse (claude-native never opens one), sessionRunning=true. + const bubbles = buildBubbles(danglingToolTurn, null, undefined, [], true); + expect(toolState(bubbles)).toBe("input-available"); + }); + + it("does not spin when the session is idle (dangling tool resolves to no-output)", () => { + // The idle edge can land before the tool's result block — the tool must + // settle, not spin forever. This is the property the never-spin tests pin, + // now exercised through the session-driven path. + const bubbles = buildBubbles(danglingToolTurn, null, undefined, [], false); + expect(toolState(bubbles)).toBe("no-output"); + }); + + it("a resolved tool shows its output regardless of session running", () => { + const blocks: AnyBlock[] = [ + ...danglingToolTurn, + { + type: "tool_result", + ctx: ctx({ itemId: "fco_1", responseId: "resp_1" }), + name: "", + callId: "c1", + agentName: "test", + output: "done", + }, + ]; + const bubbles = buildBubbles(blocks, null, undefined, [], true); + expect(toolState(bubbles)).toBe("output-available"); + }); + + it("only the NEWEST turn spins — an earlier turn's dangling tool stays no-output", () => { + const blocks: AnyBlock[] = [ + { + type: "tool_group", + ctx: ctx({ itemId: "fc_old", responseId: "resp_old" }), + executions: [mkExec("Read", "c_old")], + iteration: 0, + }, + { type: "user_message", ctx: ctx({ itemId: "u1", responseId: "" }), content: [] }, + { + type: "tool_group", + ctx: ctx({ itemId: "fc_new", responseId: "resp_new" }), + executions: [mkExec("Bash", "c_new")], + iteration: 0, + }, + ]; + const bubbles = buildBubbles(blocks, null, undefined, [], true); + const assistants = bubbles.filter( + (b): b is Extract => b.kind === "assistant", + ); + const oldTool = assistants[0].items.find((item) => item.kind === "tool"); + const newTool = assistants[1].items.find((item) => item.kind === "tool"); + expect((oldTool as Extract).state).toBe("no-output"); + expect((newTool as Extract).state).toBe("input-available"); + }); + + it("a trailing user message means no live turn — nothing spins", () => { + // A just-sent prompt with no assistant output yet: newestAssistantTurnId + // returns null, so the earlier turn's tool does not spin. + const blocks: AnyBlock[] = [ + ...danglingToolTurn, + { type: "user_message", ctx: ctx({ itemId: "u1", responseId: "" }), content: [] }, + ]; + const bubbles = buildBubbles(blocks, null, undefined, [], true); + const asst = bubbles.find( + (b): b is Extract => b.kind === "assistant", + )!; + const tool = asst.items.find((item) => item.kind === "tool"); + expect((tool as Extract).state).toBe("no-output"); + }); + + it("the cache re-walks on a running→idle flip so a dangling tool stops spinning", () => { + // The flip carries no block change; the cache key must still see it move. + const cache = createBubbleCache(); + expect(toolState(buildBubbles(danglingToolTurn, null, cache, [], true))).toBe( + "input-available", + ); + expect(toolState(buildBubbles(danglingToolTurn, null, cache, [], false))).toBe("no-output"); + }); +}); + describe("buildBubbles — cross-bubble tool_result pairing", () => { function resultBlock( callId: string, diff --git a/web/src/lib/renderItems.ts b/web/src/lib/renderItems.ts index d70c6f42..9dee74a0 100644 --- a/web/src/lib/renderItems.ts +++ b/web/src/lib/renderItems.ts @@ -238,6 +238,13 @@ export interface BubbleCache { blocks: AnyBlock[] | null; activeResponse: ActiveResponse | null; interruptedResponseIds: readonly string[] | null; + // Response id of the newest assistant turn while the SESSION is running, + // or null. Drives the trailing-tool spinner for harnesses whose + // running/idle lives in `sessionStatus` rather than a streaming + // `activeResponse` (see `newestAssistantTurnId`). Part of the cache key: a + // running→idle flip carries no block change, so the identity short-circuit + // must see it move or a dangling tool would spin forever. + liveTurnId: string | null; bubbles: Bubble[]; lastBubbleStart: number; lastBubbleCount: number; @@ -252,6 +259,7 @@ export function createBubbleCache(): BubbleCache { blocks: null, activeResponse: null, interruptedResponseIds: null, + liveTurnId: null, bubbles: [], lastBubbleStart: -1, lastBubbleCount: 1, @@ -259,6 +267,39 @@ export function createBubbleCache(): BubbleCache { }; } +/** + * Response id of the newest assistant turn — the turn whose trailing tool + * phase should spin while the SESSION is running. + * + * claude-native's running/idle lives in `sessionStatus` (the status file + * drives the badge; no streaming `activeResponse` is ever opened — see the + * transcript forwarder), so its in-flight tool calls can't key their spinner + * off `lifecycle === "streaming"` the way an in-process harness does. Instead + * the caller passes `sessionRunning` and this names which turn is live. + * + * Scans back from the end: the first assistant-side block carrying a real + * (non-anonymous) response id names that turn. A trailing user message (a + * just-sent prompt with no assistant output yet), a compaction/routing + * boundary, or an empty transcript yields `null` — there is no live turn. + */ +function newestAssistantTurnId(blocks: AnyBlock[]): string | null { + for (let i = blocks.length - 1; i >= 0; i -= 1) { + const b = blocks[i]!; + if ( + b.type === "user_message" || + b.type === "compaction" || + b.type === "compaction_loading" || + b.type === "routing_decision" + ) { + return null; + } + if (isNonRenderingBlock(b) || b.type === "tool_result") continue; + if (isAnonymousRid(b.ctx.responseId)) continue; + return b.ctx.responseId; + } + return null; +} + /** * Walk a flat block list and produce the bubble cluster list. * @@ -279,14 +320,24 @@ export function createBubbleCache(): BubbleCache { * and the unit tests rely on. * @param interruptedResponseIds - response ids whose bubbles should remain * labelled cancelled even after the active response sidecar has moved on. + * @param sessionRunning - whether the SESSION status is running/waiting. + * Lets the newest turn's trailing tool phase spin for a harness whose + * liveness lives in `sessionStatus` rather than a streaming `activeResponse` + * (claude-native). Does NOT change any bubble's `lifecycle` — fork, fold, + * cancelled, and failed are unaffected; it only reaches the tool-state gate. */ export function buildBubbles( blocks: AnyBlock[], activeResponse: ActiveResponse | null, cache?: BubbleCache, interruptedResponseIds: readonly string[] = EMPTY_INTERRUPTED_RESPONSE_IDS, + sessionRunning = false, ): Bubble[] { const interruptedResponses = new Set(interruptedResponseIds); + // The newest turn spins its trailing tools only while the session runs; an + // already-streaming `activeResponse` covers the in-process harnesses without + // it, so this is null unless the session is running. + const liveTurnId = sessionRunning ? newestAssistantTurnId(blocks) : null; // Resolved over the whole transcript before any reuse decision: a create-time // chip and the turn chip that repeats it verbatim are one verdict however far // apart they landed, and whether the pair is still visible decides whether the @@ -294,8 +345,16 @@ export function buildBubbles( const superseded = supersededRoutingChips(blocks); if (cache === undefined) { return markContinuedTurns( - walkBubbles(blocks, activeResponse, interruptedResponses, 0, [], new Map(), superseded) - .bubbles, + walkBubbles( + blocks, + activeResponse, + interruptedResponses, + 0, + [], + new Map(), + superseded, + liveTurnId, + ).bubbles, activeResponse, ); } @@ -304,14 +363,22 @@ export function buildBubbles( if ( cache.blocks === blocks && cache.activeResponse === activeResponse && - cache.interruptedResponseIds === interruptedResponseIds + cache.interruptedResponseIds === interruptedResponseIds && + cache.liveTurnId === liveTurnId ) { return cache.bubbles; } // Try the incremental path: reuse every finalized bubble (all but the // last) and rebuild only from where the last cached bubble started. - const reuse = reusablePrefix(blocks, activeResponse, interruptedResponses, cache, superseded); + const reuse = reusablePrefix( + blocks, + activeResponse, + interruptedResponses, + cache, + superseded, + liveTurnId, + ); if (reuse !== null) { const subIndexSeed = new Map(); for (const b of reuse.prefix) { @@ -327,10 +394,12 @@ export function buildBubbles( reuse.prefix, subIndexSeed, superseded, + liveTurnId, ); cache.blocks = blocks; cache.activeResponse = activeResponse; cache.interruptedResponseIds = interruptedResponseIds; + cache.liveTurnId = liveTurnId; cache.bubbles = markContinuedTurns(rest.bubbles, activeResponse); cache.lastBubbleStart = rest.lastBubbleStart; cache.lastBubbleCount = rest.lastBubbleCount; @@ -347,10 +416,12 @@ export function buildBubbles( [], new Map(), superseded, + liveTurnId, ); cache.blocks = blocks; cache.activeResponse = activeResponse; cache.interruptedResponseIds = interruptedResponseIds; + cache.liveTurnId = liveTurnId; cache.bubbles = markContinuedTurns(full.bubbles, activeResponse); cache.lastBubbleStart = full.lastBubbleStart; cache.lastBubbleCount = full.lastBubbleCount; @@ -495,6 +566,7 @@ function reusablePrefix( interruptedResponses: ReadonlySet, cache: BubbleCache, superseded: ReadonlySet, + liveTurnId: string | null, ): { prefix: Bubble[]; startBlock: number } | null { if (cache.blocks === null || cache.bubbles.length === 0 || cache.lastBubbleStart <= 0) { return null; @@ -549,6 +621,15 @@ function reusablePrefix( if (b.kind === "assistant" && b.responseId === activeId) return null; } } + // Same hazard for the session-driven spinner: the turn that WAS live (its + // trailing tool showed a spinner) or the one that IS now must be re-walked, + // not reused, so its tool state settles. Guard both — on an A→B transition A + // has moved into the prefix carrying its stale spinner, and on a + // running→idle flip the still-last bubble must drop it. + const liveIds = [liveTurnId, cache.liveTurnId]; + for (const b of prefix) { + if (b.kind === "assistant" && liveIds.includes(b.responseId)) return null; + } for (const b of prefix) { if (b.kind === "assistant" && interruptedResponses.has(b.responseId)) { return null; @@ -629,6 +710,7 @@ function walkBubbles( seedBubbles: Bubble[], subIndexByResp: Map, superseded: ReadonlySet, + liveTurnId: string | null = null, ): { bubbles: Bubble[]; lastBubbleStart: number; lastBubbleCount: number } { const bubbles: Bubble[] = [...seedBubbles]; // One cross-bubble result index per walk: the relay backdates a @@ -848,7 +930,15 @@ function walkBubbles( stableId, lifecycle, error, - items: buildAssistantItems(groupBlocks, lifecycle, crossBubbleResults), + // `sessionLive` spins this turn's trailing tools when the session is + // running and this is the newest turn — for a harness with no streaming + // `activeResponse`. `lifecycle` (and thus fork/fold) is untouched. + items: buildAssistantItems( + groupBlocks, + lifecycle, + crossBubbleResults, + liveTurnId !== null && groupResponseId === liveTurnId, + ), ...(workedForS !== undefined ? { workedForS } : {}), ...(lastActivityAtS !== undefined ? { lastActivityAtS } : {}), }); @@ -1194,11 +1284,12 @@ function buildAssistantItems( groupBlocks: AnyBlock[], lifecycle: ActiveResponse["state"], crossBubbleResults: Map, + sessionLive = false, ): RenderItem[] { // Results render only by folding into a call's card — strip them so // an absorbed out-of-band result can't split a text/reasoning run. const blocks = groupBlocks.filter((b) => b.type !== "tool_result"); - const liveToolCallIds = trailingLiveToolCallIds(blocks, lifecycle); + const liveToolCallIds = trailingLiveToolCallIds(blocks, lifecycle, sessionLive); // Pre-compute: is there any non-empty TextDone in this bubble? // Used to drop trailing-empty assistant messages — the server @@ -1386,9 +1477,17 @@ function buildAssistantItems( function trailingLiveToolCallIds( blocks: AnyBlock[], lifecycle: ActiveResponse["state"], + sessionLive = false, ): Set { const callIds = new Set(); - if (lifecycle !== "streaming") return callIds; + // Spin the trailing tool phase when EITHER this bubble is the streaming + // `activeResponse` (in-process harnesses) OR the session is running and this + // is its newest turn (`sessionLive`, for claude-native, whose liveness lives + // in `sessionStatus`). A settled turn — reloaded history, a finished or + // cancelled turn, a dead harness whose session reads idle — passes neither, + // so a result-less tool still resolves to `no-output`, never a perpetual + // spinner. + if (lifecycle !== "streaming" && !sessionLive) return callIds; for (let i = blocks.length - 1; i >= 0; i -= 1) { const b = blocks[i]!; diff --git a/web/src/pages/ChatPage.test.ts b/web/src/pages/ChatPage.test.ts index b663ec51..793d3f83 100644 --- a/web/src/pages/ChatPage.test.ts +++ b/web/src/pages/ChatPage.test.ts @@ -866,6 +866,54 @@ describe("computeShowsWorking", () => { false, ); }); + + it("an in-flight local send lights the indicator before any server edge", () => { + // Pressing Enter sets chatStore.status = "streaming" synchronously, while + // `sessionStatus` stays `idle` until the server publishes `running` (for + // claude-native, until the status file's next poll). The sidebar row + // already lights up off this flag, so the chat pane must too or the two + // disagree for the whole dispatch round-trip. + expect(computeShowsWorking("idle", opts({ localSendInFlight: true }))).toBe(true); + }); + + it("an in-flight local send survives a stale offline poll", () => { + // Sending to an asleep runner relaunches it; `/health` reads stale-offline + // during that window (10s cadence). The user dispatched, so the indicator + // must not be suppressed — same reasoning as live running/waiting above. + expect( + computeShowsWorking("idle", opts({ localSendInFlight: true, runnerOnline: false })), + ).toBe(true); + }); + + it("a spin-up in flight yields the slot to the Starting-up cue", () => { + // ChatPage passes `localSendInFlight: status === "streaming" && !spinUpInFlight`. + // `RunnerStartingIndicator` renders only when the shimmer is absent, and its + // copy ("Starting up…" / "Cloning repository…") is strictly more informative + // than a generic shimmer — so during a boot the optimistic path stands down. + expect(computeShowsWorking("idle", opts({ localSendInFlight: false }))).toBe(false); + }); + + it("a server-confirmed running still wins during a spin-up", () => { + // Once the harness reports work the spin-up cue has self-gated to null, so + // suppressing the shimmer too would leave the turn with no indicator at all. + // `localSendInFlight` is the only thing the spin-up gate touches. + expect(computeShowsWorking("running", opts({ localSendInFlight: false }))).toBe(true); + }); + + it("a pending elicitation still outranks an in-flight local send", () => { + // The elicitation prompt owns the in-progress slot; the two must never + // stack, so the suppression applies regardless of how the work was + // signalled. + expect( + computeShowsWorking("idle", opts({ localSendInFlight: true, hasPendingElicitation: true })), + ).toBe(false); + }); + + it("no local send in flight leaves an idle session idle", () => { + // The flag is opt-in: a cross-client or TUI-typed turn sets no local + // status here, so an idle session with no send stays dark. + expect(computeShowsWorking("idle", opts({ localSendInFlight: false }))).toBe(false); + }); }); // ── shouldShowWorkingIndicator ────────────────────────────────────────────── diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index 6bca38be..70186b85 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -797,6 +797,9 @@ export function ChatPage() { // handling). Overrides the liveness-derived unreachable affordances // below, which misread the not-yet-host-bound session as stranded. const sandboxLaunching = sandboxStatus !== null && sandboxStatus.stage !== "failed"; + // Terminal-first spin-up state, read here (not just in the child surfaces) so + // the working-indicator gate below can defer to the "Starting up…" cue. + const chatTerminalFirst = useTerminalFirst(); // Read runner liveness from the app-level batch poller (see // RunnerHealthProvider). `undefined` = not yet polled — the indicator // stays hidden until the first poll for this session resolves. @@ -839,7 +842,16 @@ export function ChatPage() { // Both keep the prompt on top across the pending → approved flip. const committed = stripGatedSubagentRoutingChips( reorderCommittedRequestElicitations( - buildBubbles(blocks, activeResponse, bubbleCacheRef.current, interruptedResponseIds), + buildBubbles( + blocks, + activeResponse, + bubbleCacheRef.current, + interruptedResponseIds, + // Spin the newest turn's in-flight tools while the session runs — + // for claude-native, whose running/idle lives in `sessionStatus` + // and never opens a streaming `activeResponse`. + computeIsWorking(sessionStatus), + ), ), subagentRoutingOverride, ); @@ -859,6 +871,7 @@ export function ChatPage() { interruptedResponseIds, pendingUserMessages, subagentRoutingOverride, + sessionStatus, ]); // Picker selection. ChatPage stays mounted across `/` to `/c/:id`, @@ -1006,10 +1019,25 @@ export function ChatPage() { // + shimmer/pill) for the main chat and is suppressed mid-elicitation or // when the runner is known offline. const isWorking = !hasPendingElicitation && computeIsWorking(sessionStatus); + // A spin-up in flight owns the in-progress slot with more specific copy + // ("Starting up…" / "Cloning repository…") than the generic shimmer, and + // `RunnerStartingIndicator` only renders when the shimmer is absent. So the + // OPTIMISTIC path must stand down here: a send that has to boot a runner is + // exactly when the user needs to know it's booting, not just that we asked. + // A server-confirmed `running`/`waiting` still wins — by then the harness is + // up and the spin-up cue has self-gated to null. + const spinUpInFlight = + sandboxLaunching || + Boolean(chatTerminalFirst?.isTerminalFirst && chatTerminalFirst.terminalStartingUp); const showsWorking = computeShowsWorking(sessionStatus, { hasPendingElicitation, runnerOnline, backgroundTaskCount, + // Optimistic: light up the moment this client dispatches, without waiting + // for the server's ``running``. The sidebar row already reads this same + // flag (``isStartingUp`` in Sidebar.tsx), so the two agreed only once the + // server confirmed; now they agree immediately. + localSendInFlight: status === "streaming" && !spinUpInFlight, }); // A fork of a coding session carries the source id in this label (set by @@ -5704,10 +5732,17 @@ export function computeIsWorking(sessionStatus: SessionStatus): boolean { * 10s cadence and reads stale-offline during the runner's connect window on a * fresh session's first turn (it would otherwise hide "Working…" for seconds). * @param options.backgroundTaskCount - Background shells still running after - * the turn ended. A claude-native turn settles to ``idle`` (the PTY-activity - * watcher's edge) even while shells run, so the bare status alone would hide - * the indicator; a positive count keeps it lit so "N background tasks still running" + * the turn ended. A claude-native turn settles to ``idle`` (the status file's + * edge) even while shells run, so the bare status alone would hide the + * indicator; a positive count keeps it lit so "N background tasks still running" * stays visible. + * @param options.localSendInFlight - This client's own send is in flight + * (``chatStore.status === "streaming"``). Lights the indicator optimistically + * the moment the user presses Enter, before any server edge confirms the turn + * — the sidebar row already does this (see ``isStartingUp`` in Sidebar.tsx), + * so without it the two disagree for the dispatch round-trip. Distinct from + * ``sessionStatus``, which mirrors the server: this one means "we asked", not + * "the agent is working". * @returns ``true`` when the main session's own status should render Working. */ export function computeShowsWorking( @@ -5716,6 +5751,7 @@ export function computeShowsWorking( hasPendingElicitation: boolean; runnerOnline: boolean | undefined; backgroundTaskCount?: number; + localSendInFlight?: boolean; }, ): boolean { if (options.hasPendingElicitation) return false; @@ -5723,9 +5759,10 @@ export function computeShowsWorking( // A running/waiting session is proof the runner is up, so a stale // poll-derived ``runnerOnline === false`` must not suppress it. Only gate on // known-offline for the not-actively-working case (e.g. a background-shell - // tally on an idle session). - if (options.runnerOnline === false && !isWorking) return false; - return isWorking || (options.backgroundTaskCount ?? 0) > 0; + // tally on an idle session). An in-flight local send is the same kind of + // proof — the user just dispatched — so it also survives the gate. + if (options.runnerOnline === false && !isWorking && !options.localSendInFlight) return false; + return isWorking || options.localSendInFlight === true || (options.backgroundTaskCount ?? 0) > 0; } /** diff --git a/web/src/store/chatStore.test.ts b/web/src/store/chatStore.test.ts index 4554c8f5..8178406a 100644 --- a/web/src/store/chatStore.test.ts +++ b/web/src/store/chatStore.test.ts @@ -51,7 +51,6 @@ import { consumePendingInitialPrompt, handleSessionEvent, isStaleCompletedResponse, - reviveStrayCompletedResponse, initChatStore, pumpStreamEvents, setPendingInitialPrompt, @@ -2303,45 +2302,16 @@ describe("chatStore — send while streaming (queueing)", () => { error: null, }); - // The server's deny short-circuit still publishes a session-level - // running→idle pair for the denied out-of-band input, and the client - // trusts `session.status` 1:1 — so this stray idle flips - // `sessionStatus` AND finalizes the streaming turn (a bare terminal - // edge is the NORMAL turn-end shape for id-less emitters like the - // PTY-activity relay, so it must settle the bubble; see the bare-idle - // tests below). The deny corner is healed by the live-delta revive: - // the still-streaming turn's next delta reopens it - // (reviveStrayCompletedResponse), so the misread is a brief flicker, - // not a mid-turn fold. - handleSessionEvent({ - type: "session_status", - conversationId: "conv_abc", - status: "idle", - }); - const afterIdle = useChatStore.getState(); - expect(afterIdle.sessionStatus).toBe("idle"); - expect(afterIdle.status).toBe("idle"); - expect(afterIdle.activeResponse).toEqual({ - responseId: "resp_in_flight", - state: "completed", - error: null, - completedAt: expect.any(Number), - }); - - // The turn was actually still live — its next delta revives it, and - // the session's busy signal comes back with it: leaving - // sessionStatus "idle" let a mid-turn send bypass shouldQueueSend's - // queue gate. Local `status` stays "idle" — this client sent - // nothing, so no local send is in flight. - reviveStrayCompletedResponse(useChatStore.setState); - expect(useChatStore.getState().activeResponse).toEqual({ + // The deny publishes no session-status pair at all: the agent never ran, + // so there is no turn to report. The live turn streaming alongside it is + // therefore untouched — no stray idle to fold its bubble, and no + // client-side revive needed to undo one. + expect(state.sessionStatus).toBe("running"); + expect(state.activeResponse).toEqual({ responseId: "resp_in_flight", state: "streaming", error: null, - completedAt: expect.any(Number), }); - expect(useChatStore.getState().sessionStatus).toBe("running"); - expect(useChatStore.getState().status).toBe("idle"); }); it("finalizes a streaming turn on a bare idle edge (no response id)", () => { @@ -2386,7 +2356,7 @@ describe("chatStore — send while streaming (queueing)", () => { it("preserves a cancelled turn across a bare idle edge", () => { // The user's interrupt verdict must survive the trailing idle the - // teardown publishes — and the revive must never resurrect it. + // teardown publishes. useChatStore.setState({ conversationId: "conv_abc", status: "idle", @@ -2399,8 +2369,6 @@ describe("chatStore — send while streaming (queueing)", () => { status: "idle", }); expect(useChatStore.getState().activeResponse?.state).toBe("cancelled"); - reviveStrayCompletedResponse(useChatStore.setState); - expect(useChatStore.getState().activeResponse?.state).toBe("cancelled"); }); it("surfaces a failed send without settling the live turn", async () => { @@ -2443,54 +2411,6 @@ describe("chatStore — send while streaming (queueing)", () => { expect(state.status).toBe("streaming"); }); - it("revive is a no-op for failed and absent responses", () => { - useChatStore.setState({ - conversationId: "conv_abc", - sessionStatus: "idle", - activeResponse: { responseId: "resp_a", state: "failed", error: "boom" }, - }); - reviveStrayCompletedResponse(useChatStore.setState); - expect(useChatStore.getState().activeResponse?.state).toBe("failed"); - expect(useChatStore.getState().sessionStatus).toBe("idle"); - - useChatStore.setState({ activeResponse: null }); - reviveStrayCompletedResponse(useChatStore.setState); - expect(useChatStore.getState().activeResponse).toBeNull(); - expect(useChatStore.getState().sessionStatus).toBe("idle"); - }); - - it("gates the revive to a window after the finalize", () => { - // A scheduled wake's first deltas stream ahead of the batch that - // names the new turn; reviving the minutes-old finished turn - // popped its "Worked for" fold open at every /loop iteration. A - // finalize moments ago is a plausible stray idle and still revives. - useChatStore.setState({ - conversationId: "conv_abc", - sessionStatus: "idle", - activeResponse: { - responseId: "resp_prev_iter", - state: "completed", - error: null, - completedAt: Date.now() - 60_000, - }, - }); - reviveStrayCompletedResponse(useChatStore.setState); - expect(useChatStore.getState().activeResponse?.state).toBe("completed"); - expect(useChatStore.getState().sessionStatus).toBe("idle"); - - useChatStore.setState({ - activeResponse: { - responseId: "resp_live", - state: "completed", - error: null, - completedAt: Date.now() - 1_000, - }, - }); - reviveStrayCompletedResponse(useChatStore.setState); - expect(useChatStore.getState().activeResponse?.state).toBe("streaming"); - expect(useChatStore.getState().sessionStatus).toBe("running"); - }); - it("isStaleCompletedResponse: only an old finalize is stale", () => { const base = { responseId: "r", error: null } as const; expect( @@ -3390,12 +3310,11 @@ describe("chatStore — handleSessionEvent (session.* events)", () => { // sessionStatus tracks the server's session-level status 1:1 — a server // idle means idle, and the "Working…" indicator (which reads only // sessionStatus) must turn off. The bubble lifecycle settles on the - // same edge: a bare (id-less) idle is the NORMAL turn-end shape for - // the PTY-activity relay and orchestration teardown, and leaving the - // turn "streaming" hid its "Worked for" fold and Fork action until a - // reload. A stray idle for a turn that is actually still live (the - // policy-deny short-circuit) is healed by the live-delta revive — - // see reviveStrayCompletedResponse. + // same edge: a bare (id-less) idle is the NORMAL turn-end shape for the + // status file and orchestration teardown, and leaving the turn + // "streaming" hid its "Worked for" fold and Fork action until a reload. + // Every idle that reaches here is a real turn end — control signals no + // longer publish status. expect(state.sessionStatus).toBe("idle"); expect(state.status).toBe("idle"); expect(state.activeResponse).toEqual({ @@ -7048,7 +6967,10 @@ describe("chatStore — startStreamPump reconnect loop", () => { await loop; expect(opens).toBe(11); // 1 initial open + 10 retries, then gives up - expect(useChatStore.getState().sessionStatus).toBe("failed"); + // Giving up on OUR stream says nothing about what the agent is doing, so + // the session's own status is left alone — only the server may declare a + // session failed. The dropped stream surfaces as offline liveness. + expect(useChatStore.getState().sessionStatus).toBe("running"); expect(useChatStore.getState().abortController).toBeNull(); }); @@ -7750,10 +7672,7 @@ describe("chatStore — live delta streaming (claude-native)", () => { it("keeps the synthetic response id when no turn is tracked", async () => { // A preview that lands before any turn id is known keeps its own id, so - // it can't be grouped into an unrelated bubble. (A `completed` turn does - // NOT hit this path: a live delta proves that turn is still running, so - // `reviveStrayCompletedResponse` reopens it first and the preview - // correctly joins it.) + // it can't be grouped into an unrelated bubble. useChatStore.setState({ conversationId: "conv_live_norid", blocks: [], diff --git a/web/src/store/chatStore.ts b/web/src/store/chatStore.ts index b5e0ef9b..73364f8e 100644 --- a/web/src/store/chatStore.ts +++ b/web/src/store/chatStore.ts @@ -3378,12 +3378,16 @@ export async function startStreamPump( // Release the unconsumed error-response body so the underlying fetch // connection is freed promptly rather than lingering across retries. void streamRes.body?.cancel().catch(() => {}); - // 401/403 won't fix themselves by retrying — give up and mark the - // session failed so the user isn't left on a silent spinner. + // 401/403 won't fix themselves by retrying — give up and settle the + // local send lifecycle so the user isn't left on a silent spinner. + // `sessionStatus` is NOT touched: losing our stream says nothing + // about what the agent is doing (it may well still be mid-turn), and + // only the server may declare a session failed. The dropped stream + // surfaces as offline liveness via ConnectionIndicator. if (streamRes.status === 401 || streamRes.status === 403) { console.warn(`Session ${id}: stream unavailable (${streamRes.status}), giving up`); finalizeActive(set, "failed", `stream unavailable (${streamRes.status})`, null); - set({ sessionStatus: "failed", status: "idle" }); + set({ status: "idle" }); break; } // A reverse proxy routinely serves 404 for the stream route while @@ -3396,8 +3400,10 @@ export async function startStreamPump( console.warn( `Session ${id}: stream unavailable (404) after ${consecutive404s} attempts, giving up`, ); + // Local lifecycle only — see the 401/403 branch above for why + // `sessionStatus` is left to the server. finalizeActive(set, "failed", "stream unavailable (404)", null); - set({ sessionStatus: "failed", status: "idle" }); + set({ status: "idle" }); break; } console.warn( @@ -3710,24 +3716,16 @@ async function* tapLiveDeltas( retired.add(ev.messageId); continue; } - reviveStrayCompletedResponse(set); applyLiveDelta(set, ev.messageId, ev.index ?? 0, ev.delta, lastIndex); } continue; } if (ev.type === "tool_output_delta") { if (get().conversationId === id && !isStaleCompletedResponse(get())) { - reviveStrayCompletedResponse(set); applyLiveToolOutputDelta(set, ev.callId, ev.delta); } continue; } - if ( - (ev.type === "text_delta" || ev.type === "reasoning_delta") && - get().conversationId === id - ) { - reviveStrayCompletedResponse(set); - } yield ev; } } @@ -3772,12 +3770,11 @@ export function adoptTrailingUnattributedBlocks( return next; } -// How long after a terminal edge a delta still revives the turn. A -// STRAY mid-turn idle is contradicted by the still-flowing stream -// within seconds; a scheduled wake (cron / wakeup fires at 60s -// minimum) streams its FIRST deltas ahead of the transcript batch that -// names the new turn — reviving the finished turn then popped its -// "Worked for" fold open at the start of every /loop iteration. +// How long after a terminal edge a delta still belongs to the finished +// turn. A scheduled wake (cron / wakeup fires at 60s minimum) streams +// its FIRST deltas ahead of the transcript batch that names the new +// turn; attributing those to the previous turn popped its "Worked for" +// fold open at the start of every /loop iteration. const REVIVE_WINDOW_MS = 15_000; /** @@ -3792,23 +3789,6 @@ export function isStaleCompletedResponse(s: { activeResponse: ActiveResponse | n ); } -export function reviveStrayCompletedResponse(set: Setter): void { - set((s) => { - if (s.activeResponse?.state !== "completed") return {}; - if (isStaleCompletedResponse(s)) return {}; - // The delta also proves the SESSION is mid-turn: restore the busy - // signal the stray idle edge cleared, so send gating - // (shouldQueueSend) queues instead of firing into the live turn and - // the Working indicator comes back before the next running edge. - // Local `status` stays untouched — it means "this client's send is - // in flight", which is false for cross-client and TUI-typed turns. - return { - activeResponse: { ...s.activeResponse, state: "streaming" }, - sessionStatus: "running", - }; - }); -} - /** * Flip an auto-resolved ApprovalCard back to answerable, in place. * @@ -4677,17 +4657,14 @@ export function handleSessionEvent(event: StreamEvent): void { } } else { // Terminal edge without a matching response id. This is the - // NORMAL turn-end shape for most emitters — the PTY-activity - // relay's bare `idle`, orchestration teardown, and mismatched - // Stop-hook `waiting` all carry none — so a still-streaming - // turn is finalized here rather than left "streaming" forever - // (which hid the settled turn's "Worked for" fold and Fork - // action until a reload re-derived lifecycle from the - // snapshot). The one edge this can misread — the server's - // policy-deny short-circuit publishing a stray running→idle - // pair while a real turn streams — is healed by - // `reviveStrayCompletedResponse`: the live turn's next delta - // reopens it. A `cancelled` turn is preserved as-is. + // NORMAL turn-end shape for most emitters — the status file's + // bare `idle` and orchestration teardown carry none — so a + // still-streaming turn is finalized here rather than left + // "streaming" forever (which hid the settled turn's "Worked for" + // fold and Fork action until a reload re-derived lifecycle from + // the snapshot). Every terminal edge that reaches this point is + // now a real turn end: control signals (policy deny, compaction) + // no longer publish status. A `cancelled` turn is preserved as-is. patch.status = "idle"; if (s.activeResponse?.state === "streaming") { patch.activeResponse = {