diff --git a/agent/agent_init.py b/agent/agent_init.py index f982d038d1..db92f18487 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -909,14 +909,12 @@ def init_agent( agent._active_children = [] # Running child AIAgents (for interrupt propagation) agent._active_children_lock = threading.Lock() - # Background memory/skill review state (agent/background_review.py). Holds - # the forked review AIAgent while its run_conversation() is in flight, so - # the NEXT live turn can proactively interrupt a still-running review - # instead of letting the two race concurrently against the same - # session_id/credentials (observed as doubled prompt-token counts and a - # Ctrl+C-proof lockup when a live turn started before a review fired at - # the end of the prior turn had finished). + # Background memory/skill review state (agent/background_review.py). + # ``_background_review_run`` is installed before the worker starts and + # fences its first provider-capable phase; the direct agent pointer keeps + # normal interrupt propagation available once the fork is constructed. agent._background_review_agent = None + agent._background_review_run = None agent._background_review_lock = threading.Lock() # Store OpenRouter provider preferences @@ -1815,13 +1813,18 @@ def init_agent( agent._memory_nudge_interval = 10 agent._turns_since_memory = 0 agent._iters_since_skill = 0 - # A flush/background agent may pass skip_memory=True to avoid spinning up an - # external memory *provider*, but if the caller also explicitly enables the - # "memory" toolset it still needs the built-in file-backed store — otherwise - # the memory tool dispatches with store=None and every call fails (#65429). - # So the built-in store is created unless memory is globally disabled, while - # the external-provider block below stays gated on skip_memory. - _memory_toolset_requested = "memory" in (agent.enabled_toolsets or []) + # skip_memory=True skips the external memory *provider*. Flush/background + # agents can still pass enabled_toolsets=["memory"] so the built-in file + # store exists and the memory tool does not fail with store=None (#65429). + # A toolset on disabled_toolsets is not a request: a caller that denylists + # memory while its default toolset still names it must not get MEMORY.md + # loaded by an enabled-only check. (Cron agents now run with + # skip_memory=False and take the normal path here.) + _enabled_toolsets = agent.enabled_toolsets or [] + _disabled_toolsets = agent.disabled_toolsets or [] + _memory_toolset_requested = ( + "memory" in _enabled_toolsets and "memory" not in _disabled_toolsets + ) if not skip_memory or _memory_toolset_requested: try: from tools.memory_tool import ( diff --git a/agent/background_review.py b/agent/background_review.py index ae12a059d8..f48a3d5200 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -23,6 +23,7 @@ import json import logging import os from pathlib import Path +import threading from typing import Any, Dict, List, Optional from agent.thread_scoped_output import thread_scoped_silence @@ -30,6 +31,158 @@ from agent.thread_scoped_output import thread_scoped_silence logger = logging.getLogger(__name__) +_BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS = 2.0 + + +class _BackgroundReviewRun: + """Per-review cancellation and request-completion handshake.""" + + def __init__(self) -> None: + self.cancel_requested = threading.Event() + self.request_done = threading.Event() + self._lock = threading.Lock() + self._review_agent = None + self._request_finished = False + self._cancel_dispatched = False + + def begin_request(self, review_agent: Any) -> bool: + """Atomically admit the first provider-capable review phase.""" + with self._lock: + if self.cancel_requested.is_set() or self._request_finished: + return False + self._review_agent = review_agent + return True + + def cancel(self) -> Any: + """Fence startup and return the running fork, if one was admitted.""" + with self._lock: + self.cancel_requested.set() + if self._review_agent is not None and not self._cancel_dispatched: + self._cancel_dispatched = True + return self._review_agent + return None + + def mark_request_finished(self) -> bool: + """Latch request completion once; the caller publishes the event.""" + with self._lock: + if self._request_finished: + return False + self._request_finished = True + self._review_agent = None + return True + + +def prepare_background_review_run(agent: Any) -> Optional[_BackgroundReviewRun]: + """Install a unique run token on the parent before ``Thread.start()``.""" + lock = getattr(agent, "_background_review_lock", None) + if lock is None: + try: + lock = threading.Lock() + agent._background_review_lock = lock + except (AttributeError, TypeError): + return None + + run = _BackgroundReviewRun() + try: + with lock: + current = getattr(agent, "_background_review_run", None) + if current is not None and not current.request_done.is_set(): + return None + agent._background_review_run = run + except (AttributeError, TypeError): + return None + return run + + +def finish_background_review_run( + agent: Any, + run: Optional[_BackgroundReviewRun], +) -> None: + """Publish one run's request exit without clearing a successor (ABA-safe).""" + if run is None or not run.mark_request_finished(): + return + + lock = getattr(agent, "_background_review_lock", None) + if lock is not None: + with lock: + if getattr(agent, "_background_review_run", None) is run: + agent._background_review_run = None + elif getattr(agent, "_background_review_run", None) is run: + agent._background_review_run = None + run.request_done.set() + + +def _interrupt_background_review(review_agent: Any) -> None: + """Request abort off-thread so a broken abort hook cannot stall foreground. + + The bounded wait on ``request_done`` in + :func:`cancel_background_review_for_live_turn` is only effective if + ``interrupt()`` returns quickly. Off-loading to a daemon thread ensures + a slow or wedged abort path cannot block the foreground turn (#84423). + """ + + def _interrupt() -> None: + try: + from agent.interrupt_compat import request_hard_interrupt + + request_hard_interrupt(review_agent, "superseded by a new live turn") + except Exception: + logger.debug( + "Failed to cancel in-flight background review for a new turn", + exc_info=True, + ) + + try: + threading.Thread( + target=_interrupt, + daemon=True, + name="bg-review-cancel", + ).start() + except Exception: + logger.debug( + "Failed to start background-review cancellation thread", + exc_info=True, + ) + + +def cancel_background_review_for_live_turn(agent: Any) -> None: + """Cancel the current review and await its request-phase acknowledgement. + + Foreground priority is preserved: if the review does not acknowledge within + the bounded deadline, a warning is logged and the live turn proceeds + anyway. The review is non-critical self-improvement work and must never + block a user-facing turn (#84423). + """ + lock = getattr(agent, "_background_review_lock", None) + if lock is not None: + with lock: + run = getattr(agent, "_background_review_run", None) + legacy_agent = getattr(agent, "_background_review_agent", None) + else: + run = getattr(agent, "_background_review_run", None) + legacy_agent = getattr(agent, "_background_review_agent", None) + + if run is None: + if legacy_agent is None: + return + _interrupt_background_review(legacy_agent) + return + + review_agent = run.cancel() + if review_agent is not None: + _interrupt_background_review(review_agent) + + acknowledged = run.request_done.wait( + timeout=_BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS + ) + if not acknowledged: + logger.warning( + "Background review did not acknowledge cancellation within %.1fs; " + "proceeding with foreground live turn", + _BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS, + ) + + # --------------------------------------------------------------------------- # Background-review aux-model selector + routed digest. # @@ -862,13 +1015,23 @@ def _run_review_in_thread( messages_snapshot: List[Dict], prompt: str, task_cfg: Optional[Dict[str, Any]] = None, + review_run: Optional[_BackgroundReviewRun] = None, ) -> None: """Worker function executed in the background-review daemon thread. Spawns a forked ``AIAgent`` inheriting the parent's runtime, runs the review prompt, and surfaces a compact action summary back to the user via ``agent._safe_print`` and ``agent.background_review_callback``. + + ``review_run`` is the per-review cancellation token from + :func:`prepare_background_review_run`. If a live turn bumps the + cancel generation before this review reaches its first provider call, + the review aborts without entering ``run_conversation()`` (#84423). """ + if review_run is not None and review_run.cancel_requested.is_set(): + finish_background_review_run(agent, review_run) + return + # Local import to avoid a hard circular dep at module load. from run_agent import AIAgent from tools.terminal_tool import set_approval_callback as _set_approval_callback @@ -918,6 +1081,10 @@ def _run_review_in_thread( except (ValueError, AttributeError): pass + def _finish_request_phase(agent_ref) -> None: + _unregister_review_agent(agent_ref) + finish_background_review_run(agent, review_run) + try: # Silence stdout/stderr for THIS worker thread only. A process-global # ``contextlib.redirect_stdout(devnull)`` here would also blank @@ -1117,12 +1284,10 @@ def _run_review_in_thread( # Register this fork on the PARENT's _active_children (the same # list interrupt() fans out to for subagent delegation) and # _background_review_agent (a direct pointer the next live turn - # uses to proactively cancel a still-running review). Without - # this, a review still streaming when the next turn starts races - # the live turn against the same session_id/credentials — producing - # doubled prompt-token accounting and a Ctrl+C-proof lockup. - # Best-effort: agents built without agent_init.py (test stubs) - # degrade to "no cross-cancellation" rather than aborting the review. + # uses to interrupt an admitted request). The per-review run token + # separately fences startup and acknowledges request-phase exit. + # The legacy pointer/list remain best-effort for direct test stubs; + # a prepared run token is the live-turn cancellation authority. if hasattr(agent, "_background_review_agent"): _br_lock = getattr(agent, "_background_review_lock", None) if _br_lock is not None: @@ -1173,22 +1338,26 @@ def _run_review_in_thread( pass try: - # Routed to a different model -> replay a digest (cache is cold - # on that model anyway, so minimise cold-written tokens). Same - # model -> replay the full snapshot (warm cache reads). - _review_history = ( - _digest_history(messages_snapshot) if _routed - else messages_snapshot - ) - review_agent.run_conversation( - user_message=( - prompt - + "\n\nYou can only call memory and skill " - "management tools. Other tools will be denied " - "at runtime — do not attempt them." - ), - conversation_history=_review_history, + request_admitted = ( + review_run is None or review_run.begin_request(review_agent) ) + if request_admitted: + # Routed to a different model -> replay a digest (cache is cold + # on that model anyway, so minimise cold-written tokens). Same + # model -> replay the full snapshot (warm cache reads). + _review_history = ( + _digest_history(messages_snapshot) if _routed + else messages_snapshot + ) + review_agent.run_conversation( + user_message=( + prompt + + "\n\nYou can only call memory and skill " + "management tools. Other tools will be denied " + "at runtime — do not attempt them." + ), + conversation_history=_review_history, + ) finally: clear_thread_tool_whitelist() # Attribute the review fork's usage to the PARENT session. @@ -1199,12 +1368,9 @@ def _run_review_in_thread( if review_agent is not None: review_usage.update(_snapshot_review_usage(review_agent)) _record_review_usage_to_parent(agent, review_usage) - # Unregister as soon as run_conversation() itself has - # returned — that's the only phase making outbound API - # calls, i.e. the only phase that can race the parent's - # next live turn. Runs on both the success and exception - # path (this whole block is inside the try/finally above). - _unregister_review_agent(review_agent) + # Publish completion as soon as the provider-capable phase has + # returned or startup cancellation has fenced it out. + _finish_request_phase(review_agent) # Snapshot review actions before teardown. close() is allowed to # clean per-session state, but the user-visible self-improvement @@ -1284,13 +1450,10 @@ def _run_review_in_thread( # thread-scoped silence here so teardown output (Honcho flush, Hindsight # sync, background thread joins) stays quiet even on the exception path, # without blanking other threads' streams. - # Also a safety-net unregister: covers exceptions raised during setup - # (between registration and the run_conversation try/finally above) - # that the primary _unregister_review_agent call site never reaches. - # _unregister_review_agent is idempotent (checks `is`/`in` membership), - # so calling it again here after the primary call site already ran is - # a harmless no-op. - _unregister_review_agent(review_agent) + # Also a safety-net completion: covers exceptions raised during setup + # before the request-phase finally. Both tracking cleanup and the + # per-run completion publication are identity-scoped and idempotent. + _finish_request_phase(review_agent) if review_agent is not None: try: with thread_scoped_silence(): @@ -1319,6 +1482,7 @@ def spawn_background_review_thread( review_skills: bool = False, focus: Optional[str] = None, task_cfg: Optional[Dict[str, Any]] = None, + review_run: Optional[_BackgroundReviewRun] = None, ): """Build the review thread target and prompt for a background review. @@ -1359,7 +1523,13 @@ def spawn_background_review_thread( ) def _target() -> None: - _run_review_in_thread(agent, messages_snapshot, prompt, task_cfg) + _run_review_in_thread( + agent, + messages_snapshot, + prompt, + task_cfg=task_cfg, + review_run=review_run, + ) return _target, prompt diff --git a/agent/codex_responses_adapter.py b/agent/codex_responses_adapter.py index ac1129d4fc..1b02a626f0 100644 --- a/agent/codex_responses_adapter.py +++ b/agent/codex_responses_adapter.py @@ -479,6 +479,12 @@ def _chat_messages_to_responses_input( conversation is still on the wire. """ items: List[Dict[str, Any]] = [] + # Parallel to `items`: the raw chat message each converted item came + # from. Pruning needs this to read a canonical summary carrier's + # up-to-date, provenance-tagged content directly — the converted `item` + # can be a lossy shape (stale exact-replay, or a typed + # `function_call_output` wrapper) that no longer carries it (#90976). + item_sources: List[Optional[Dict[str, Any]]] = [] seen_item_ids: set = set() for msg in messages: @@ -567,6 +573,7 @@ def _chat_messages_to_responses_input( if k not in ("id", "_issuer_kind") } items.append(replay_item) + item_sources.append(msg) if item_id: seen_item_ids.add(item_id) has_codex_reasoning = True @@ -623,14 +630,17 @@ def _chat_messages_to_responses_input( if isinstance(phase, str) and phase.strip(): replay_item["phase"] = phase.strip() items.append(replay_item) + item_sources.append(msg) replayed_message_items += 1 if replayed_message_items > 0: pass elif content_parts: items.append({"role": "assistant", "content": content_parts}) + item_sources.append(msg) elif content_text.strip(): items.append({"role": "assistant", "content": content_text}) + item_sources.append(msg) elif has_codex_reasoning: # The Responses API requires a following item after each # reasoning item (otherwise: missing_following_item error). @@ -638,6 +648,7 @@ def _chat_messages_to_responses_input( # content, emit an empty assistant message as the required # following item. items.append({"role": "assistant", "content": ""}) + item_sources.append(msg) tool_calls = msg.get("tool_calls") if isinstance(tool_calls, list): @@ -680,6 +691,7 @@ def _chat_messages_to_responses_input( "name": fn_name, "arguments": arguments, }) + item_sources.append(msg) continue # Non-assistant (user) role: emit multimodal parts when present, @@ -688,6 +700,7 @@ def _chat_messages_to_responses_input( items.append({"role": role, "content": content_parts}) else: items.append({"role": role, "content": content_text}) + item_sources.append(msg) continue if role == "tool": @@ -722,24 +735,38 @@ def _chat_messages_to_responses_input( "call_id": _clamp_responses_call_id(call_id), "output": output_value, }) + item_sources.append(msg) # Native server-side compaction: when a replayed checkpoint is present, # restructure the wire around it. The server renders nothing placed # before a compaction item (live-verified Aug 2026), so pre-checkpoint - # history is dead upload weight and — worse — the user's plaintext asks - # from before the boundary silently vanish from the model's view. Keep - # the newest checkpoint first, retain pre-checkpoint USER messages - # verbatim within a token budget (Codex CLI parity), and leave the + # history is dead upload weight and — worse — the user's plaintext asks, + # and any local-compression summary already merged into that history, + # silently vanish from the model's view. Keep the newest checkpoint + # first, retain pre-checkpoint USER messages and compression-SUMMARY + # messages (whole, never byte-sliced) verbatim within a token budget + # each (Codex CLI parity for the user side), and leave the # post-checkpoint tail untouched. Gated on the CURRENT request's native # eligibility, not merely on the presence of a checkpoint: a persisted # checkpoint outlives the gate, and pruning for a request that carries no # ``context_management`` deletes history the server never compacted. + # + # ``item_sources`` (parallel to ``items``) carries the raw chat message + # each converted item came from. A canonical summary carrier's content + # can be lost or gone stale by the time it becomes a Responses item — a + # merge-into-tail tool-result carrier becomes a typed + # ``function_call_output`` (no ``content``/``role`` at all), and a + # merge-into-tail assistant carrier can be shadowed by a stale exact + # ``codex_message_items`` replay from before the merge rewrote its + # content. Pruning reads the source message's own up-to-date, + # provenance-tagged content directly instead of trying to recover it + # from whatever shape the conversion produced (#90976). if not native_compaction_eligible: return items from agent.native_compaction import prune_pre_checkpoint_items - return prune_pre_checkpoint_items(items) + return prune_pre_checkpoint_items(items, item_sources=item_sources) # --------------------------------------------------------------------------- diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 951b874701..3d7f07ef39 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -1820,31 +1820,6 @@ def run_conversation( agent._last_compression_attempt_recorded = False agent._last_compression_attempt_in_place = None - # If a background memory/skill review spawned at the end of a PRIOR turn - # (agent/background_review.py) is still running its own run_conversation() - # when THIS turn starts, cancel it now rather than letting both make - # outbound API calls concurrently against the same session_id/credentials. - # That concurrency can produce doubled prompt-token accounting on this - # turn's own calls and, because the review fork is a fully separate - # AIAgent with no route back to THIS agent's interrupt() by default, a - # lockup that survives a normal /stop and needs a hard Ctrl+C. - # ``review_agent.interrupt()`` is fire-and-forget here — it just flags - # cancellation and aborts the review's in-flight socket; it does not - # block waiting for the review's daemon thread to exit, so it can't add - # latency to this turn. Only ever set on the real owning agent (the - # review fork's own copy of this attribute stays None — reviews don't - # spawn nested reviews), so this is a no-op on every other run_conversation - # caller (subagents, the review fork itself, etc). - _pending_review = getattr(agent, "_background_review_agent", None) - if _pending_review is not None: - try: - _pending_review.interrupt("superseded by a new live turn") - except Exception: - logger.debug( - "Failed to cancel in-flight background review for a new turn", - exc_info=True, - ) - # Adopt any ~/.hermes/.env credential/base-url edits made since the last # turn — a Settings save updates .env but not this worker's client, which # was built at agent init (#67821). No-op when .env is unchanged. diff --git a/agent/model_metadata.py b/agent/model_metadata.py index af7bb194d3..8a8a4bee26 100644 --- a/agent/model_metadata.py +++ b/agent/model_metadata.py @@ -508,6 +508,9 @@ DEFAULT_CONTEXT_LENGTHS = { # ensures "glm-5.2" resolves to 1M while older variants still hit the # generic 202K fallback. "glm-5.2": 1_048_576, + # OpenRouter's free GLM-5.2 variant is capped at 256K (live metadata, + # 2026-08-21) — longer key wins over the 1M paid entry above. + "glm-5.2:free": 256_000, "glm": 202752, # xAI Grok — xAI /v1/models does not return context_length metadata, # so these hardcoded fallbacks prevent Hermes from probing-down to @@ -563,7 +566,15 @@ DEFAULT_CONTEXT_LENGTHS = { # (stealth/ox-alpha). 1M context per OpenRouter live metadata (2026-08-20). "ox-alpha": 1_048_576, # Nemotron — NVIDIA's open-weights series (128K context across all sizes) + # EXCEPT 3.5 Lightning, which ships a 1M window (OpenRouter live metadata + # + OpenCode Zen free tier, verified 2026-08-21). + "nemotron-3.5-lightning": 1_000_000, "nemotron": 131072, + # Poolside Laguna 2.1 (s/xs) — 256K window per OpenRouter live metadata + # (2026-08-21). Covers laguna-s-2.1:free, laguna-xs-2.1:free, and the + # OpenCode Zen laguna-s-2.1-free slug via substring matching. + "laguna-s-2.1": 262144, + "laguna-xs-2.1": 262144, # Arcee "trinity": 262144, # OpenRouter diff --git a/agent/native_compaction.py b/agent/native_compaction.py index c8835f1392..d718ad70b2 100644 --- a/agent/native_compaction.py +++ b/agent/native_compaction.py @@ -32,15 +32,25 @@ captured compaction items ride the existing ``codex_reasoning_items`` sidecar, which already handles persistence (state.db), gateway session replay, cross-issuer stamping, and the encrypted-replay kill switch. -This module is dependency-free on purpose so the transport, adapter, and -conversation loop can share the gate without import cycles. +This module stays free of transport/adapter dependencies so the transport, +adapter, and conversation loop can share the gate without import cycles. The +two exceptions — ``agent.context_compressor`` and ``agent.message_content`` — +sit below this module in the dependency graph (neither imports +``native_compaction``), so importing their provenance/text primitives here +introduces no cycle. """ from __future__ import annotations +import logging from typing import Any, Dict, List, Optional from urllib.parse import urlsplit +from agent.context_compressor import is_compaction_summary_message +from agent.message_content import flatten_message_text + +logger = logging.getLogger(__name__) + # Native compaction fires this many tokens below the local compressor's # trigger so the server always gets the first shot at compaction. LOCAL_TRIGGER_SAFETY_MARGIN = 8_192 @@ -147,73 +157,130 @@ def native_compaction_context_management( # Retention budget for plaintext user messages carried across a native # compaction boundary (mirrors Codex CLI's RETAINED_MESSAGE_TOKEN_BUDGET). # Live verification (Aug 2026, gpt-5.6 @ api.openai.com): the server renders -# NOTHING placed before a replayed compaction checkpoint — a fact stated in a -# pre-checkpoint input item is invisible to the model ("NONE" recall), while -# the same item placed after the checkpoint recalls perfectly. Without -# retention, every plaintext user ask from before the compaction survives -# only as whatever the opaque server summary kept — the goal-drift failure -# mode. Codex CLI solves this by rebuilding history with user messages -# retained verbatim; ``prune_pre_checkpoint_items`` is our wire-level -# equivalent. RETAINED_USER_MESSAGE_TOKEN_BUDGET = 64_000 +# Retention budget for local compression summary messages carried across a native +# compaction boundary to prevent summary token inflation. +RETAINED_SUMMARY_TOKEN_BUDGET = 32_000 + def _approx_tokens(text: str) -> int: """Cheap chars//4 token estimate — same shape Codex uses for retention.""" return max(1, len(text) // 4) -def _user_item_text(item: Dict[str, Any]) -> Optional[str]: - """Extract the retained-budget text of a user-role input item. +def _extract_item_text(item: Any) -> Optional[str]: + """Extract measurable text from string, list content, output_text, or nested metadata text. - Returns None when the item carries no measurable text (empty message). - Multimodal list content is measured by its ``input_text`` parts; images - count as zero, matching Codex's retention accounting. + Returns None when the item carries no measurable text. + Handles string content, multipart lists (input_text/text/output_text), and fallback keys. """ + if not isinstance(item, dict): + return None + content = item.get("content") + if content is None and "output_text" in item: + content = item.get("output_text") + if isinstance(content, str): return content if content.strip() else None + if isinstance(content, list): - text = "".join( - part.get("text", "") - for part in content - if isinstance(part, dict) and part.get("type") == "input_text" - ) - return text if text.strip() or content else None + parts = [] + for part in content: + if isinstance(part, str): + if part.strip(): + parts.append(part.strip()) + elif isinstance(part, dict): + part_text = part.get("text") or part.get("input_text") or part.get("output_text") + if isinstance(part_text, str) and part_text.strip(): + parts.append(part_text.strip()) + part_meta = part.get("metadata") + if isinstance(part_meta, dict) and isinstance(part_meta.get("text"), str): + if part_meta["text"].strip(): + parts.append(part_meta["text"].strip()) + text = " ".join(parts) + return text if text.strip() else None + return None +def _is_summary_item(item: Any) -> bool: + """True when *item* is a canonical Hermes compression-summary message. + + Delegates entirely to + ``agent.context_compressor.is_compaction_summary_message`` — the single + authoritative provenance check already used by every other summary + consumer (memory providers, frontends, the compactor itself). It prefers + the exact, truthy ``COMPRESSED_SUMMARY_METADATA_KEY`` marker and falls + back to the canonical prefix classifier (``SUMMARY_PREFIX`` / + ``LEGACY_SUMMARY_PREFIX`` / historical prefixes, including the + merge-into-tail shape) for the case where the underscore-prefixed key + was already stripped by a wire sanitizer. + + Deliberately NOT a second heuristic: no arbitrary underscore-key scan, no + inference from a falsy or unrelated metadata key, and no matching on + ad-hoc content headings like ``"## Summary"`` in ordinary text — any of + those can promote a normal user/assistant message (or adversarial + content) to durable retained history (#90975 review). + """ + return is_compaction_summary_message(item) + + def prune_pre_checkpoint_items( items: List[Dict[str, Any]], retained_user_token_budget: int = RETAINED_USER_MESSAGE_TOKEN_BUDGET, + retained_summary_token_budget: int = RETAINED_SUMMARY_TOKEN_BUDGET, + enable_summary_retention: bool = True, + item_sources: Optional[List[Any]] = None, ) -> List[Dict[str, Any]]: """Restructure Responses input around the newest compaction checkpoint. The server drops every input item that precedes a replayed ``compaction`` item (live-verified Aug 2026), so sending pre-checkpoint history is dead - weight AND silently erases the user's plaintext asks. When a checkpoint - is present, rebuild the wire as:: + weight AND silently erases the user's plaintext asks — including any + local-compression summary the agent already produced, which previously + vanished here because it carries ``role="assistant"``, not ``"user"`` + (#90975). When a checkpoint is present, rebuild the wire as:: - [checkpoint run] + [retained user messages (newest-first budget)] + [post] + [checkpoint run] + [retained user & summary messages (newest-first budget)] + [post] - - The NEWEST contiguous run of checkpoints wins (the server can emit - more than one compaction item in a single response — live-observed - Aug 2026 — and they arrive adjacent; a run from a newer response - cumulatively carries prior windows, so older runs are dropped). - - Retained user messages are the user-role items from before the - checkpoint, kept verbatim newest-first within + - The NEWEST contiguous run of checkpoints wins. + - Retained user messages are kept verbatim within ``retained_user_token_budget``; the boundary message is head-truncated - when it only partially fits (string content only). - - Everything after the checkpoint is untouched, so function_call / - function_call_output pairing is preserved (a checkpoint is captured on - an assistant response, and that response's own calls and their outputs - are all emitted after its reasoning items). - - No checkpoint in ``items`` → returned unchanged (self-gating: non-native - routes and kill-switched sessions never see a restructured wire). - - Deterministic for a given history, so the request prefix stays stable - across turns and server-side prompt caching keeps working. + when it only partially fits (string content only) — goals are usually + stated up front, so the head is the valuable end. + - Compression summary messages (``_is_summary_item``, the canonical + ``agent.context_compressor`` provenance check) are retained whole + within ``retained_summary_token_budget``. A summary is never + byte/character-sliced: Hermes summaries carry structural framing + (handoff prefix, end marker, merge-into-tail delimiters) that a blind + slice can corrupt, so one that doesn't fit whole is dropped instead. + A summary already retained once (identical text) is never duplicated, + so repeated checkpoints stay idempotent. + - ``enable_summary_retention`` is a function-level override (used by + tests and callers that need the pre-#90975 behavior back); it is not + wired to a user-facing config surface. + - Original relative chronological order between user messages and + summaries is preserved. + - ``item_sources`` (optional, parallel to ``items``) is the raw chat + message each Responses item was converted from. By the time a summary + reaches this function as a converted ``item`` it can already be lossy: + a merge-into-tail tool-result carrier becomes a typed + ``function_call_output`` (no ``content``/``role`` survives the + conversion at all), and a merge-into-tail assistant carrier can be + shadowed by a stale exact ``codex_message_items`` replay captured + before the merge rewrote its content. When a source is provided and is + itself a canonical summary carrier (``is_compaction_summary_message``), + its content is read directly from the source — never from the + converted item — and it is retained as a synthesized + ``role="assistant"`` message regardless of what shape the original + item took. Without ``item_sources`` (default), retention only sees + what survived conversion, matching pre-#90976 behavior (#90976). """ + if not isinstance(items, list) or not items: + return items + last_cp = None for i, item in enumerate(items): if isinstance(item, dict) and item.get("type") == "compaction": @@ -234,36 +301,105 @@ def prune_pre_checkpoint_items( checkpoint_run = items[first_cp : last_cp + 1] post = items[last_cp + 1 :] + if isinstance(item_sources, list) and len(item_sources) == len(items): + pre_sources: List[Any] = item_sources[:first_cp] + else: + pre_sources = [None] * len(pre) + retained_reversed: List[Dict[str, Any]] = [] - remaining = max(0, int(retained_user_token_budget)) - for item in reversed(pre): - if not isinstance(item, dict) or item.get("role") != "user": + user_remaining = max(0, int(retained_user_token_budget)) + summary_remaining = max(0, int(retained_summary_token_budget)) + seen_summary_texts: set = set() + + def _try_retain_summary(text: Optional[str]) -> Optional[Dict[str, Any]]: + """Check budget/dedup/cost for a summary; return cost info or None.""" + if not text or summary_remaining <= 0 or text in seen_summary_texts: + return None + cost = _approx_tokens(text) + if cost > summary_remaining: + # Never byte-slice a summary's structural framing — drop it + # whole rather than corrupt the handoff prefix / end marker. + return None + seen_summary_texts.add(text) + return {"cost": cost} + + for item, source in zip(reversed(pre), reversed(pre_sources)): + if not isinstance(item, dict): continue - # Skip typed items (function_call_output etc. never carry role=user, - # but stay defensive about future shapes). + + # Canonical source-based summary detection: reads the ORIGINAL chat + # message's own content, so it sees past a lossy conversion (a + # typed `function_call_output` wrapper, or a stale exact-replay + # message) that erased the summary from `item` itself (#90976). + # This is never a heuristic promotion of arbitrary item content — + # it only fires when the source message itself is a canonical, + # provenance-tagged summary carrier. + if enable_summary_retention and isinstance(source, dict) and _is_summary_item(source): + text = flatten_message_text(source.get("content")) if isinstance(source, dict) else "" + text = text if text.strip() else None + result = _try_retain_summary(text) + if result: + _src_role = source.get("role") + retained_reversed.append({ + "role": _src_role if _src_role in ("user", "assistant") else "assistant", + "content": text, + }) + summary_remaining -= result["cost"] + continue + + # Skip typed non-message items (function_call_output etc. never + # carry role=user or a summary flag, but stay defensive about + # future shapes). if "type" in item and item.get("type") != "message": continue - if remaining <= 0: - break - text = _user_item_text(item) + + is_summary = enable_summary_retention and _is_summary_item(item) + is_user = item.get("role") == "user" + + if not is_user and not is_summary: + continue + + text = _extract_item_text(item) if text is None: continue - cost = _approx_tokens(text) - if cost <= remaining: - retained_reversed.append(item) - remaining -= cost - elif isinstance(item.get("content"), str): - # Head-truncate the boundary message: goals are usually stated - # up front, so the head is the valuable end. - truncated = dict(item) - truncated["content"] = item["content"][: remaining * 4] - if truncated["content"].strip(): - retained_reversed.append(truncated) - remaining = 0 - # Multimodal boundary message that doesn't fit whole: skip rather - # than rewrite parts. + # Image-only user messages have empty text but non-empty content — + # main retains them at 1-token cost (images count as zero, matching + # Codex's retention accounting). Don't skip them just because text + # is falsy. + if not text and not is_user: + continue - return checkpoint_run + list(reversed(retained_reversed)) + post + if is_summary: + result = _try_retain_summary(text) + if result: + retained_reversed.append(item) + summary_remaining -= result["cost"] + elif is_user: + if user_remaining <= 0: + continue + cost = _approx_tokens(text) + if cost <= user_remaining: + retained_reversed.append(item) + user_remaining -= cost + elif isinstance(item.get("content"), str): + truncated = dict(item) + truncated["content"] = item["content"][: user_remaining * 4] + if truncated["content"].strip(): + retained_reversed.append(truncated) + user_remaining = 0 + + retained_ordered = list(reversed(retained_reversed)) + result = checkpoint_run + retained_ordered + post + + logger.debug( + "Pruned pre-checkpoint items: %d input -> %d retained (user_rem=%d, summary_rem=%d)", + len(items), + len(result), + user_remaining, + summary_remaining, + ) + + return result def is_native_compaction_rejection(error: Any, status_code: Any = None) -> bool: diff --git a/agent/reasoning_effort.py b/agent/reasoning_effort.py index 48b44a25ee..e29c0273e5 100644 --- a/agent/reasoning_effort.py +++ b/agent/reasoning_effort.py @@ -95,6 +95,13 @@ KIMI_K3_EFFORTS: tuple[str, ...] = ("low", "high", "max") #: Moonshot/Kimi K2-era models: low/medium/high. KIMI_K2_EFFORTS: tuple[str, ...] = ("low", "medium", "high") +#: OpenCode "Ox Alpha" stealth model (x-preview-f-free): thinking is always +#: on and the wire accepts exactly low/high/max — medium/none/xhigh 400 with +#: "This model always engages in thinking and cannot be disabled; please use +#: low, high, or max" (verified live 2026-08-21). xhigh rounds up to max. +OX_ALPHA_EFFORTS: tuple[str, ...] = ("low", "high", "max") +OX_ALPHA_OVERRIDES: dict[str, str] = {"xhigh": "max"} + #: Tencent TokenHub: low/medium/high. TOKENHUB_EFFORTS: tuple[str, ...] = ("low", "medium", "high") diff --git a/apps/desktop/e2e/glyph-spinner.spec.ts b/apps/desktop/e2e/glyph-spinner.spec.ts new file mode 100644 index 0000000000..9ef8790bd3 --- /dev/null +++ b/apps/desktop/e2e/glyph-spinner.spec.ts @@ -0,0 +1,198 @@ +/** + * E2E contract for the compositor-only GlyphSpinner. + * + * The spinner's whole reason for existing in this shape is a CSS animation: + * every frame is in the DOM from mount and a `transform` keyframes animation + * scrolls between them, so there is no JS timer and no per-tick DOM mutation + * scheduling document-scale style recalculation. + * + * None of that is observable in jsdom — it has no animation engine, no + * cascade resolution for `steps()`, and no `Element.getAnimations()`. The + * jsdom suite (src/components/ui/glyph-spinner.test.tsx) therefore pins the + * DATA and WIRING, and this spec pins the RENDERED BEHAVIOUR in a real + * browser, which is the only place the stylesheet actually runs. + * + * This replaces three tests that asserted on the TEXT of the stylesheet. + * Reading source in a test is banned outright (AGENTS.md) and those tests + * proved the point: a var()-fallback edit that changed no rendered pixel + * broke one of them, while none of them had ever executed the CSS. + * + * Prerequisite: `npm run build` must have been run so dist/ exists. + */ + +import { expect, type Page, test } from '@playwright/test' + +import { type MockBackendFixture, setupMockBackend, waitForAppReady } from './fixtures' + +const STRIP = '.glyph-spinner__strip' + +/** + * Send a message so a turn is in flight — the composer status stack mounts a + * GlyphSpinner while the agent is working. Resolves once a frame strip is in + * the DOM. + */ +async function mountSpinner(page: Page): Promise { + const composer = page.locator('[contenteditable="true"]').first() + await composer.waitFor({ state: 'visible', timeout: 10_000 }) + await composer.click() + await composer.type('hello from the glyph spinner spec', { delay: 10 }) + await page.keyboard.press('Enter') + + await page.waitForSelector(STRIP, { state: 'attached', timeout: 20_000 }) +} + +test.describe('GlyphSpinner (compositor animation)', () => { + let fixture: MockBackendFixture + + test.beforeAll(async () => { + fixture = await setupMockBackend() + await waitForAppReady(fixture) + }) + + test.afterAll(async () => { + await fixture?.cleanup() + }) + + test('animates with a steps() transform keyframes animation, one step per frame', async () => { + const { page } = fixture + await mountSpinner(page) + + const observed = await page.evaluate(strip => { + const el = document.querySelector(strip) + + if (!el) { + throw new Error('no frame strip in the DOM') + } + + const style = getComputedStyle(el) + const animations = el.getAnimations() + + return { + frameCount: el.querySelectorAll('.glyph-spinner__frame').length, + timingFunction: style.animationTimingFunction, + iterationCount: style.animationIterationCount, + durationMs: animations[0]?.effect?.getTiming().duration ?? null, + names: animations.map(a => (a as CSSAnimation).animationName), + // A percentage translate makes the animation layout-dependent, which + // Chromium refuses to composite. Read the engine's own keyframes: a + // revert to translateY(-100%) shows up here, while the computed + // `style.transform` always serializes to a matrix and can't tell. + travel: ((animations[0]?.effect as KeyframeEffect | undefined)?.getKeyframes() ?? []) + .map(k => String((k as Keyframe & { transform?: string }).transform ?? '')) + .join(' | ') + } + }, STRIP) + + // The strip carries every frame; `steps(N)` parks on each one in turn. + expect(observed.frameCount).toBeGreaterThan(1) + // Chromium has serialized jump-end as both `steps(N)` and `steps(N, end)`. + expect(observed.timingFunction).toMatch(new RegExp(`^steps\\(${observed.frameCount}\\b`)) + expect(observed.iterationCount).toBe('infinite') + expect(observed.names).toContain('glyph-spinner-advance') + // One full cycle is frames x interval, so the duration must be a positive + // multiple of the frame count — not the single-frame interval. + expect(observed.durationMs).toBeGreaterThan(0) + // Length-typed travel, never a percentage: `translateY(-100%)` would keep + // the animation off the compositor. + expect(observed.travel).toContain('calc(') + expect(observed.travel).not.toContain('%') + }) + + test('is promoted to a layer while running, and neither animates nor holds a layer when parked', async () => { + const { page } = fixture + await mountSpinner(page) + + const running = await page.evaluate(strip => { + const el = document.querySelector(strip)! + + return { + playState: getComputedStyle(el).animationPlayState, + willChange: getComputedStyle(el).willChange + } + }, STRIP) + + expect(running.playState).toBe('running') + // Scoped to active spinners — a permanently promoted layer per parked + // spinner is pure memory at fan-out breadth. + expect(running.willChange).toBe('transform') + + // 1. The per-spinner gate: a kept-alive but inactive pane, or an explicit + // `paused` prop (ChatSwapOverlay's fade-out). + const parked = await page.evaluate(strip => { + const el = document.querySelector(strip)! + const viewport = el.closest('.glyph-spinner')! + const previous = viewport.getAttribute('data-paused') + + viewport.setAttribute('data-paused', 'true') + + const state = { + playState: getComputedStyle(el).animationPlayState, + willChange: getComputedStyle(el).willChange + } + + if (previous === null) { + viewport.removeAttribute('data-paused') + } else { + viewport.setAttribute('data-paused', previous) + } + + return state + }, STRIP) + + expect(parked.playState).toBe('paused') + expect(parked.willChange).toBe('auto') + + // 2. The global gate: window blur / minimize / document-hidden, which + // main.tsx drives by arming this attribute on the root. The strip must + // be named in that rule, or every spinner keeps animating behind an + // inactive window — the CPU burn the original ticker's pause + // controller existed to avoid. + const globallyPaused = await page.evaluate(strip => { + const root = document.documentElement + const had = root.hasAttribute('data-renderer-animations-paused') + + root.setAttribute('data-renderer-animations-paused', '') + const playState = getComputedStyle(document.querySelector(strip)!).animationPlayState + + if (!had) { + root.removeAttribute('data-renderer-animations-paused') + } + + return playState + }, STRIP) + + expect(globallyPaused).toBe('paused') + }) + + test('advances in discrete frames and creates no timer-driven DOM churn', async () => { + const { page } = fixture + await mountSpinner(page) + + // Sample the resolved transform across one full cycle. A steps() animation + // holds each value for a whole interval and jumps between them, so the + // distinct values it visits must be bounded by the frame count — a linear + // animation would produce a new value on every sample. + const sampled = await page.evaluate(async strip => { + const el = document.querySelector(strip)! + const frames = el.querySelectorAll('.glyph-spinner__frame').length + const duration = Number(el.getAnimations()[0]?.effect?.getTiming().duration ?? 0) + const seen = new Set() + const textAtStart = el.textContent + + const deadline = performance.now() + duration + + while (performance.now() < deadline) { + seen.add(getComputedStyle(el).transform) + await new Promise(resolve => requestAnimationFrame(() => resolve(null))) + } + + return { distinct: seen.size, frames, textUnchanged: el.textContent === textAtStart } + }, STRIP) + + expect(sampled.distinct).toBeGreaterThan(1) + expect(sampled.distinct).toBeLessThanOrEqual(sampled.frames + 1) + // The old implementation rewrote textContent ~12x/second. Nothing may + // mutate the DOM as this animates — that mutation is the whole incident. + expect(sampled.textUnchanged).toBe(true) + }) +}) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index a1bb265ab4..adf7338ef5 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -353,7 +353,7 @@ import { installWindowsSystemCaTrust } from './windows-system-ca' import { readWindowsUserEnvVar } from './windows-user-env' import { isPackagedInstallPath as isPackagedInstallPathUnderRoots } from './workspace-cwd' import { readWslWindowsClipboardImage } from './wsl-clipboard-image' -import { resolvePickerDefaultPath } from './wsl-path-bridge' +import { resolvePickerDefaultPath, setActiveGatewayProfile, setWslBridgeProfileState } from './wsl-path-bridge' const USER_DATA_OVERRIDE = process.env.HERMES_DESKTOP_USER_DATA_DIR @@ -9898,6 +9898,7 @@ async function ensureBackend(profile) { if (route.backend === 'primary') { const connection = await startHermes() + setWslBridgeProfileState(key, connection.mode !== 'remote') // A shared backend still owes the caller its profile scope, so renderer-side // WebSocket, filesystem, and cache routing target the selected profile. @@ -9921,8 +9922,10 @@ async function ensureBackend(profile) { if (existing) { existing.lastActiveAt = Date.now() + const connection = await existing.connectionPromise + setWslBridgeProfileState(key, connection.mode !== 'remote') - return existing.connectionPromise + return connection } evictLruPoolBackends(POOL_MAX_BACKENDS - 1) @@ -9955,7 +9958,10 @@ async function ensureBackend(profile) { backendPool.set(key, entry) startPoolIdleReaper() - return entry.connectionPromise + const connection = await entry.connectionPromise + setWslBridgeProfileState(key, connection.mode !== 'remote') + + return connection } // ── Registry-scoped backends (multi-connection, PR 2 of the campaign) ────── @@ -10579,6 +10585,11 @@ async function startHermes() { } const connectionAttempt = backendConnectionState.startAttempt() + const primaryProfile = primaryProfileKey() + + // Legacy path callers without an explicit profile belong to the primary + // window backend. Profile-scoped callers still pass their key directly. + setActiveGatewayProfile(primaryProfile) // Classify this boot BEFORE the throwing resolve/mint runs: a remote failure // must NOT latch (it's transient — see shouldLatchBackendStartFailure), while @@ -10660,7 +10671,7 @@ async function startHermes() { // both for an already-saved remote and after first-run remote Apply. attemptedRemote = primaryBackendIsRemote() - return resolveRemoteBackend(primaryProfileKey()) + return resolveRemoteBackend(primaryProfile) }, waitForDecision: waitForFirstRunSetupChoice, // Mutual exclusion with an in-app update (#50238). Remote connections @@ -10669,9 +10680,18 @@ async function startHermes() { }) if (setup.kind === 'remote') { + // Paths from the remote backend belong to a host the Windows desktop + // cannot open via wsl.exe — disable WSL path bridging so native dialogs + // and file panels don't spawn wsl.exe (or the interactive install prompt + // on WSL-less machines) for unresolvable paths. (#66433) + setWslBridgeProfileState(primaryProfile, false) + return setup.connection } + // Local WSL backend — paths are bridgeable. + setWslBridgeProfileState(primaryProfile, true) + const backend = setup.backend // Route old runtimes (no `serve`) through the legacy `dashboard --no-open`. backend.args = getBackendArgsForRuntime(backend) @@ -13940,7 +13960,10 @@ ipcMain.handle('hermes:selectPaths', async (_event, options: any = {}) => { try { // On a Windows host with a WSL backend the cwd may be a POSIX/WSL path; // bridge it to a UNC/drive form the native dialog can actually open. - const bridged = IS_WINDOWS ? resolvePickerDefaultPath(String(options.defaultPath)) : String(options.defaultPath) + const bridged = IS_WINDOWS + ? resolvePickerDefaultPath(String(options.defaultPath), undefined, options?.profile) + : String(options.defaultPath) + resolvedDefaultPath = bridged ? path.resolve(bridged) : undefined } catch { resolvedDefaultPath = undefined @@ -15030,6 +15053,12 @@ app.whenReady().then(() => { registerPowerResumeListeners() keepAwake.set(readPersistedKeepAwake()) f12Blocked = readPersistedDisableF12() + // Seed this before the first window exists: a picker can open before + // startHermes() finishes resolving the configured backend. + const primaryProfile = primaryProfileKey() + + setActiveGatewayProfile(primaryProfile) + setWslBridgeProfileState(primaryProfile, !primaryBackendIsRemote()) // Quick Entry's global chord — registered on ready so a cold launch restores // it without the renderer visiting Settings. A failed registration is logged // here and surfaced in Settings via the IPC state (never silent). diff --git a/apps/desktop/electron/wsl-path-bridge-gate.test.ts b/apps/desktop/electron/wsl-path-bridge-gate.test.ts new file mode 100644 index 0000000000..1b864f922e --- /dev/null +++ b/apps/desktop/electron/wsl-path-bridge-gate.test.ts @@ -0,0 +1,75 @@ +/** + * Windows-platform regression for the WSL path-bridge gate (#66433). + * + * The behavioural tests in wsl-path-bridge.test.ts prove the no-op contract + * (paths pass through unchanged when the bridge is inactive). This file goes + * one rung further: with `process.platform` stubbed to `win32` and + * `child_process.execFileSync` mocked, it proves the actual `wsl.exe` spawn is + * suppressed — not just that the return value looks right. + * + * Each test re-imports the module fresh (vi.resetModules) so IS_WINDOWS is + * re-evaluated against the stubbed platform. + */ +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' + +const execFileSyncMock = vi.fn(() => 'Ubuntu\n') + +vi.mock('node:child_process', () => ({ execFileSync: execFileSyncMock })) + +describe('WSL bridge gate on Windows (#66433)', () => { + const realPlatform = process.platform + + beforeEach(() => { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + vi.resetModules() + execFileSyncMock.mockClear() + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: realPlatform, configurable: true }) + }) + + test('wsl.exe IS probed for a POSIX path when the bridge is active (control)', async () => { + const { resolveLocalReadPath } = await import('./wsl-path-bridge') + resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).toHaveBeenCalled() + // Sanity: it really was wsl.exe, not some other binary. + expect(execFileSyncMock).toHaveBeenNthCalledWith( + 1, + 'wsl.exe', + expect.arrayContaining(['-l', '-q']), + expect.anything() + ) + }) + + test('wsl.exe is NEVER probed when the bridge is inactive — even for POSIX paths', async () => { + const { resolveLocalReadPath, setWslBridgeActive } = await import('./wsl-path-bridge') + setWslBridgeActive(false) + // A POSIX path that WOULD trigger bridging (and the wsl.exe probe) when + // active — but with the bridge off, resolveDefaultWslDistro is never + // reached because resolveLocalReadPath returns before it. + const result = resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).not.toHaveBeenCalled() + expect(result).toBe('/home/ubuntu/project') + }) + + test('the picker default-path also skips the wsl.exe probe when inactive', async () => { + const { resolvePickerDefaultPath, setWslBridgeActive } = await import('./wsl-path-bridge') + setWslBridgeActive(false) + const result = resolvePickerDefaultPath('/home/ubuntu') + expect(execFileSyncMock).not.toHaveBeenCalled() + expect(result).toBe('/home/ubuntu') + }) + + test('re-enabling the bridge restores wsl.exe probing', async () => { + const { resolveLocalReadPath, setWslBridgeActive } = await import('./wsl-path-bridge') + setWslBridgeActive(false) + resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).not.toHaveBeenCalled() + + setWslBridgeActive(true) + execFileSyncMock.mockClear() + resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/electron/wsl-path-bridge-profile.test.ts b/apps/desktop/electron/wsl-path-bridge-profile.test.ts new file mode 100644 index 0000000000..8f83e0fa00 --- /dev/null +++ b/apps/desktop/electron/wsl-path-bridge-profile.test.ts @@ -0,0 +1,242 @@ +/** + * Profile-scoped eligibility for the WSL path bridge (#66447). + * + * The single-profile tests in wsl-path-bridge.test.ts and the Windows-platform + * gate tests in wsl-path-bridge-gate.test.ts cover the *what* (paths pass + * through unchanged when bridging is disabled) but not the *which profile*. The + * desktop is multi-profile: the renderer can swap the live gateway onto any + * profile (primary or pool) without reloading the window — so the bridge + * eligibility MUST be keyed off the **currently active profile's** backend + * configuration, not a process-global boolean. This file proves the + * per-profile contract: + * + * 1. local primary profile → bridge ON (preserved) + * 2. remote primary profile → bridge OFF (preserved) + * 3. local primary + remote non-primary → bridge OFF when the non-primary + * is active; bridge ON when the primary is active again — **no bleed**. + * 4. remote primary + local non-primary → bridge ON when the non-primary + * is active; bridge OFF when the primary is active again — **no bleed**. + * 5. profile-scoped calls accept a profile argument; absent it falls back + * to the live gateway profile that the renderer announced. + * 6. afterEach resets state so tests don't bleed into each other. + * + * These tests are pure behavior: they assert the eligibility function's output + * and the public selectors' output against observable calls. They do NOT read + * the implementation source — only the public surface exported from + * `./wsl-path-bridge`. + */ +import assert from 'node:assert/strict' + +import { afterEach, describe, test } from 'vitest' + +import { + isWslBridgeActive, + resolveLocalReadPath, + resolvePickerDefaultPath, + setActiveGatewayProfile, + setWslBridgeActive, + setWslBridgeProfileState, + wslPosixToWindowsAccessible +} from './wsl-path-bridge' + +// ── helpers ────────────────────────────────────────────────────────── + +const PROFILE_PRIMARY = 'default' +const PROFILE_LOCAL = 'team-local' +const PROFILE_REMOTE = 'team-remote' + +/** Reset every profile key the bridge knows about to a clean state. */ +afterEach(() => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_LOCAL, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + setWslBridgeActive(true) +}) + +// ── single-profile contract (preserved behaviour) ──────────────────── + +describe('WSL bridge profile eligibility — single-profile contract preserved', () => { + test('primary local → bridge ON (preserved)', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) + + test('primary remote → bridge OFF (preserved)', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), false) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), '/home/alex') + assert.equal(resolveLocalReadPath('/home/alex/proj', undefined, PROFILE_PRIMARY), '/home/alex/proj') + }) +}) + +// ── multi-profile regression (the gap) ──────────────────────────────── + +describe('WSL bridge profile eligibility — multi-profile (no cross-profile bleed)', () => { + test('local primary + remote non-primary → non-primary OFF, primary ON', () => { + // Primary is a local backend (WSL on this Windows host). A second profile + // points at a remote host whose POSIX paths the Windows host CANNOT open + // via wsl.exe — bridging must be OFF for it. + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + + // Non-primary remote is foregrounded — bridge must be OFF for it. + setActiveGatewayProfile(PROFILE_REMOTE) + assert.equal(isWslBridgeActive(), false) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + assert.equal(resolveLocalReadPath('/home/alex/proj', undefined, PROFILE_REMOTE), '/home/alex/proj') + + // Swap back to local primary — bridge must be ON again, no bleed. + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) + + test('remote primary + local non-primary → non-primary ON, primary OFF', () => { + // Primary is remote (no local WSL paths). A second profile is local — + // bridging should be ON for it because its paths CAN be opened locally. + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setWslBridgeProfileState(PROFILE_LOCAL, true) + + // Local non-primary foregrounded — bridge ON for it. + setActiveGatewayProfile(PROFILE_LOCAL) + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_LOCAL), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + + // Swap back to remote primary — bridge OFF for it, no bleed. + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), false) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), '/home/alex') + }) + + test('three profiles: each profile behaves independently', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_LOCAL, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + + // Same path, different profiles, different outcomes. + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_LOCAL), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + }) +}) + +// ── profile-argument contract ──────────────────────────────────────── + +describe('WSL bridge profile eligibility — selector argument contract', () => { + test("selector with explicit profile → that profile's bridge state", () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + + // Explicit profile wins over the live gateway. + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) + + test("selector with no profile → live gateway profile's bridge state", () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_REMOTE) + + // No profile argument → fallback to active gateway profile (remote → OFF). + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '/home/alex') + assert.equal(resolveLocalReadPath('/home/alex/proj'), '/home/alex/proj') + + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') + }) + + test('selector with unknown profile → bridge ON (defaults to active for new profiles)', () => { + // A profile that has never been seeded should default to the safe + // "bridge ON" behaviour so a brand-new local profile isn't accidentally + // disabled. The renderer seeds the state at boot; an unknown key here is + // either a renderer race or a profile created mid-session. + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', 'unknown-profile'), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) +}) + +// ── legacy back-compat: setWslBridgeActive targets the active profile ─ + +describe('WSL bridge profile eligibility — legacy toggle targets active profile', () => { + test('setWslBridgeActive(false) flips the active profile, not a process-global', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, true) + setActiveGatewayProfile(PROFILE_REMOTE) + + setWslBridgeActive(false) + + // Active profile (REMOTE) flipped to OFF; primary untouched. + assert.equal(isWslBridgeActive(), false) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + }) + + test('setWslBridgeActive(true) restores the active profile only', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_REMOTE) + + setWslBridgeActive(true) + + // Active (REMOTE) restored; primary unchanged. + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) +}) + +// ── wslPosixToWindowsAccessible stays pure / unchanged ─────────────── + +describe('WSL bridge profile eligibility — POSIX translation stays pure', () => { + test('wslPosixToWindowsAccessible ignores bridge state (pure translation)', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + + // The translator does NOT consult the bridge state — it's pure POSIX → UNC. + // Tests elsewhere assert that the bridge gate short-circuits BEFORE this + // function is reached. A regression here would mean coupling leaked into + // a helper that should stay side-effect-free. + assert.equal( + wslPosixToWindowsAccessible('/home/alex/proj', 'Ubuntu'), + '\\\\wsl.localhost\\Ubuntu\\home\\alex\\proj' + ) + assert.equal(wslPosixToWindowsAccessible('/mnt/c/Users/alex', 'Ubuntu'), 'C:\\Users\\alex') + }) +}) diff --git a/apps/desktop/electron/wsl-path-bridge.test.ts b/apps/desktop/electron/wsl-path-bridge.test.ts index 3dcf18e7b3..52073af3c3 100644 --- a/apps/desktop/electron/wsl-path-bridge.test.ts +++ b/apps/desktop/electron/wsl-path-bridge.test.ts @@ -1,8 +1,25 @@ import assert from 'node:assert/strict' -import { test } from 'vitest' +import { afterEach, test } from 'vitest' -import { parseDefaultDistro, resolvePickerDefaultPath, wslPosixToWindowsAccessible } from './wsl-path-bridge' +import { + isWslBridgeActive, + parseDefaultDistro, + resolveLocalReadPath, + resolvePickerDefaultPath, + setWslBridgeActive, + wslPosixToWindowsAccessible +} from './wsl-path-bridge' + +// ── helpers ────────────────────────────────────────────────────────── + +/** Reset the bridge to its default active state after every test so no test + * leaks global state into the next one. */ +afterEach(() => { + setWslBridgeActive(true) +}) + +// ── distro parsing (unchanged) ─────────────────────────────────────── test('parseDefaultDistro reads the first distro from clean utf-8 output', () => { assert.equal(parseDefaultDistro('Ubuntu\nDebian\n'), 'Ubuntu') @@ -20,6 +37,8 @@ test('parseDefaultDistro strips the default-marker and blank lines', () => { assert.equal(parseDefaultDistro(' \n\n'), null) }) +// ── wslPosixToWindowsAccessible ────────────────────────────────────── + test('wslPosixToWindowsAccessible maps a drvfs mount to its Windows drive', () => { assert.equal(wslPosixToWindowsAccessible('/mnt/c/Users/alex', 'Ubuntu'), 'C:\\Users\\alex') assert.equal(wslPosixToWindowsAccessible('/mnt/d', 'Ubuntu'), 'D:\\') @@ -34,8 +53,73 @@ test('wslPosixToWindowsAccessible leaves non-absolute / already-Windows paths al assert.equal(wslPosixToWindowsAccessible('relative/dir', 'Ubuntu'), 'relative/dir') }) +// ── resolvePickerDefaultPath (bridge active) ───────────────────────── + test('resolvePickerDefaultPath bridges a WSL cwd but passes Windows paths and empties through', () => { assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') assert.equal(resolvePickerDefaultPath('C:\\proj', 'Ubuntu'), 'C:\\proj') assert.equal(resolvePickerDefaultPath(undefined, 'Ubuntu'), undefined) }) + +// ── bridge active / inactive ───────────────────────────────────────── + +test('bridge defaults to active', () => { + assert.equal(isWslBridgeActive(), true) +}) + +test('setWslBridgeActive(false) → resolvePickerDefaultPath passes raw path through without bridging', () => { + setWslBridgeActive(false) + // Even a clear WSL POSIX path must pass through unchanged when the bridge + // is inactive — no distro probe, no wsl.exe, no install prompt. + assert.equal(resolvePickerDefaultPath('/home/alex'), '/home/alex') + assert.equal(resolvePickerDefaultPath('/mnt/c/Users/alex'), '/mnt/c/Users/alex') + // Windows paths and empties are unaffected either way. + assert.equal(resolvePickerDefaultPath('C:\\proj'), 'C:\\proj') + assert.equal(resolvePickerDefaultPath(undefined), undefined) +}) + +test('setWslBridgeActive(false) → resolveLocalReadPath passes raw path through without bridging', () => { + setWslBridgeActive(false) + // resolveLocalReadPath is used by fs-read-dir to make WSL paths readable + // on the Windows host. When the bridge is inactive (remote gateway), the + // raw POSIX path must be returned as-is — no UNC rewriting, no distro + // resolution. The downstream fs call will fail gracefully on non-WSL + // hosts, which is the desired behaviour. + assert.equal(resolveLocalReadPath('/home/alex/proj'), '/home/alex/proj') + assert.equal(resolveLocalReadPath('/mnt/c/Users/alex'), '/mnt/c/Users/alex') + // Non-POSIX paths are never bridged regardless of state. + assert.equal(resolveLocalReadPath('C:\\Users\\alex'), 'C:\\Users\\alex') + assert.equal(resolveLocalReadPath(''), '') +}) + +test('setWslBridgeActive(true) restores picker bridging', () => { + setWslBridgeActive(false) + assert.equal(resolvePickerDefaultPath('/home/alex'), '/home/alex') + + setWslBridgeActive(true) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') +}) + +test('toggling the bridge is idempotent and does not corrupt cached state', () => { + // Toggle twice each way. + setWslBridgeActive(false) + assert.equal(isWslBridgeActive(), false) + setWslBridgeActive(false) + assert.equal(isWslBridgeActive(), false) + + setWslBridgeActive(true) + assert.equal(isWslBridgeActive(), true) + setWslBridgeActive(true) + assert.equal(isWslBridgeActive(), true) + + // Bridging still works after the toggles. + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') +}) + +// ── state isolation: every test sees a clean active bridge ──────────── + +test('state isolation: bridge is active after a previous test toggled it off', () => { + // This test relies on afterEach resetting the bridge. + // If isolation is broken, isWslBridgeActive() would be false here. + assert.equal(isWslBridgeActive(), true) +}) diff --git a/apps/desktop/electron/wsl-path-bridge.ts b/apps/desktop/electron/wsl-path-bridge.ts index 60d3c0cf18..feff32a2f7 100644 --- a/apps/desktop/electron/wsl-path-bridge.ts +++ b/apps/desktop/electron/wsl-path-bridge.ts @@ -16,6 +16,42 @@ const WSL_MOUNT_RE = /^\/mnt\/([a-z])(?:\/(.*))?$/i let cachedDistro: null | string = null let cachedUncBase: null | string = null +/** + * WSL path eligibility belongs to the backend profile that produced the path. + * A single desktop process can keep a local primary backend and a remote pool + * backend alive simultaneously, so a process-global boolean can bleed between + * them. Unknown profiles retain the historical local default until Electron + * resolves and records their actual backend mode. + */ +const DEFAULT_WSL_BRIDGE_PROFILE = 'default' +const wslBridgeProfiles = new Map() +let activeWslBridgeProfile = DEFAULT_WSL_BRIDGE_PROFILE + +function normalizeWslBridgeProfile(profile?: null | string): string { + return String(profile || '').trim() || DEFAULT_WSL_BRIDGE_PROFILE +} + +/** Select the profile used by legacy callers that cannot pass one explicitly. */ +export function setActiveGatewayProfile(profile?: null | string): void { + activeWslBridgeProfile = normalizeWslBridgeProfile(profile) +} + +/** Record whether paths returned by one profile belong to this host's WSL. */ +export function setWslBridgeProfileState(profile: null | string, active: boolean): void { + wslBridgeProfiles.set(normalizeWslBridgeProfile(profile), Boolean(active)) +} + +/** Backward-compatible toggle: update only the current fallback profile. */ +export function setWslBridgeActive(active: boolean): void { + setWslBridgeProfileState(activeWslBridgeProfile, active) +} + +export function isWslBridgeActive(profile?: null | string): boolean { + const key = profile == null ? activeWslBridgeProfile : normalizeWslBridgeProfile(profile) + + return wslBridgeProfiles.get(key) ?? true +} + /** * Pick the default distro from `wsl.exe -l -q` output. * @@ -50,6 +86,11 @@ export function resolveDefaultWslDistro(): string { const out = execFileSync('wsl.exe', ['-l', '-q'], { encoding: 'utf8', env: { ...process.env, WSL_UTF8: '1' }, + // On WSL-less machines wsl.exe prints "The Windows Subsystem for Linux + // is not installed..." to stderr; stderr is inherited by default, so + // that banner leaks into whatever console the app is attached to + // (visible e.g. during the update hand-off). Discard it. (#80184) + stdio: ['ignore', 'pipe', 'ignore'], timeout: 2000, windowsHide: true }) @@ -116,22 +157,40 @@ export function wslPosixToWindowsAccessible(posixPath: string, distro: string = /** Native folder dialog `defaultPath`: open a WSL cwd in the Windows picker. */ export function resolvePickerDefaultPath( defaultPath: string | undefined, - distro: string = resolveDefaultWslDistro() + distro?: string, + profile?: null | string ): string | undefined { if (!defaultPath) { return undefined } + // Remote-gateway POSIX paths can't be opened via wsl.exe — no-op the bridge + // so the native dialog gets the raw path (it falls back gracefully) instead + // of triggering a wsl.exe spawn / install prompt. (#66433) + if (!isWslBridgeActive(profile)) { + return defaultPath + } + const value = String(defaultPath).trim() - return value.startsWith('/') && !WIN_DRIVE_RE.test(value) ? wslPosixToWindowsAccessible(value, distro) : defaultPath + return value.startsWith('/') && !WIN_DRIVE_RE.test(value) + ? wslPosixToWindowsAccessible(value, distro ?? resolveDefaultWslDistro()) + : defaultPath } /** fs read path: on Windows, make a WSL cwd readable via its UNC / drive form. */ -export function resolveLocalReadPath(dirPath: string, distro: string = resolveDefaultWslDistro()): string { +export function resolveLocalReadPath(dirPath: string, distro?: string, profile?: null | string): string { const value = String(dirPath || '').trim() + // In remote-gateway mode the POSIX paths belong to a host the Windows + // desktop cannot open locally — skip the WSL bridge entirely (no distro + // probe, no wsl.exe) so the file panel never spawns the install prompt on + // WSL-less machines. (#66433) + if (!isWslBridgeActive(profile)) { + return value + } + return IS_WINDOWS && value.startsWith('/') && !WIN_DRIVE_RE.test(value) - ? wslPosixToWindowsAccessible(value, distro) + ? wslPosixToWindowsAccessible(value, distro ?? resolveDefaultWslDistro()) : value } diff --git a/apps/desktop/scripts/run-short-session-hang-repro.mjs b/apps/desktop/scripts/run-short-session-hang-repro.mjs index 5645bd5e2c..8a97acaef2 100644 --- a/apps/desktop/scripts/run-short-session-hang-repro.mjs +++ b/apps/desktop/scripts/run-short-session-hang-repro.mjs @@ -925,7 +925,19 @@ async function runRealChatChecks(cdp, timed, measure, mock, runDir, appPid, nati const streamingRequestsBefore = mock.streamingCompletionRequests() const beforeAssistant = await timed(`real-chat.assistant-count.${exchange}`, () => cdp.eval( - `document.querySelectorAll('[data-slot="aui_assistant-message-root"]:not([data-streaming="true"])').length` + // Settled = no streaming marker anywhere in the row's subtree. The + // marker moved off the message root onto a hidden leaf inside it — a + // per-flip attribute write on the root widened style recalc across the + // whole message subtree — so this counts the descendant instead of the + // root's own attribute. Same rows, same gate. + // + // Counted by subtraction rather than with + // `:not(:has([data-message-streaming="true"]))`, which makes the engine + // walk every row's subtree on each evaluation — and this probe runs + // inside the very latency window it is measuring, so that cost lands in + // the number. At most one marker per row carries the attribute, so + // roots - streaming is exactly the settled count. + `document.querySelectorAll('[data-slot="aui_assistant-message-root"]').length - document.querySelectorAll('[data-message-streaming="true"]').length` ) ) const composer = await timed(`real-chat.composer-focus.${exchange}`, () => @@ -1020,7 +1032,9 @@ async function runRealChatChecks(cdp, timed, measure, mock, runDir, appPid, nati await measure(`real-chat.assistant-response.${exchange}`, () => waitForResponsive( cdp, - `document.querySelectorAll('[data-slot="aui_assistant-message-root"]:not([data-streaming="true"])').length > ${beforeAssistant}`, + // Same count-by-subtraction as the pre-send probe above: no `:has()` + // subtree walk inside the responsiveness measurement window. + `document.querySelectorAll('[data-slot="aui_assistant-message-root"]').length - document.querySelectorAll('[data-message-streaming="true"]').length > ${beforeAssistant}`, STREAM_RESPONSE_TIMEOUT_MS, `real assistant response ${exchange}`, STREAM_RESPONSE_EVALUATION_TIMEOUT_MS diff --git a/apps/desktop/src/app/chat/chat-swap-overlay.test.tsx b/apps/desktop/src/app/chat/chat-swap-overlay.test.tsx new file mode 100644 index 0000000000..958dedba3d --- /dev/null +++ b/apps/desktop/src/app/chat/chat-swap-overlay.test.tsx @@ -0,0 +1,54 @@ +// The overlay used to run its own 80ms setInterval + setState braille ticker — +// the same mechanism class (per-tick DOM mutation scheduling a style recalc) +// that GlyphSpinner was rewritten to remove. It now renders GlyphSpinner, so +// what needs pinning is that no timer comes back, that the label still survives +// the fade-out, and that the spinner stops animating once the swap is done. +import { cleanup, render, screen } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { ChatSwapOverlay } from './chat-swap-overlay' + +afterEach(() => { + cleanup() +}) + +describe('ChatSwapOverlay', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.clearAllTimers() + vi.useRealTimers() + }) + + it('animates the glyph without any timer', () => { + const { container } = render() + + expect(container.querySelector('.glyph-spinner__strip')).toBeTruthy() + expect(vi.getTimerCount()).toBe(0) + + vi.advanceTimersByTime(5_000) + + expect(vi.getTimerCount()).toBe(0) + }) + + it('names the waking profile', () => { + render() + + expect(screen.getByText(/turqoise/)).toBeTruthy() + }) + + it('keeps the last profile name through the fade-out, with the glyph frozen', () => { + const { container, rerender } = render() + + expect(container.querySelector('.glyph-spinner')?.hasAttribute('data-paused')).toBe(false) + + rerender() + + // Label held so the overlay doesn't blank while it fades. + expect(screen.getByText(/turqoise/)).toBeTruthy() + // ...and the spinner stops, the way clearing the interval used to stop it. + expect(container.querySelector('.glyph-spinner')?.getAttribute('data-paused')).toBe('true') + }) +}) diff --git a/apps/desktop/src/app/chat/chat-swap-overlay.tsx b/apps/desktop/src/app/chat/chat-swap-overlay.tsx index 9715dbc450..0226a63d34 100644 --- a/apps/desktop/src/app/chat/chat-swap-overlay.tsx +++ b/apps/desktop/src/app/chat/chat-swap-overlay.tsx @@ -1,17 +1,14 @@ import { useEffect, useState } from 'react' +import { GlyphSpinner } from '@/components/ui/glyph-spinner' import { useI18n } from '@/i18n' import { cn } from '@/lib/utils' -// Braille spinner frames — reads as a tiny ASCII loader in monospace. -const FRAMES = ['⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧', '⠇', '⠏'] - // Shown over the conversation while the live gateway swaps to another profile's // backend (lazily spawned). Keeps the last profile name through the fade-out so // the label doesn't blank. Purely visual — pointer-events-none. export function ChatSwapOverlay({ profile }: { profile: string | null }) { const { t } = useI18n() - const [frame, setFrame] = useState(0) const [label, setLabel] = useState(profile) useEffect(() => { @@ -20,16 +17,6 @@ export function ChatSwapOverlay({ profile }: { profile: string | null }) { } }, [profile]) - useEffect(() => { - if (!profile) { - return - } - - const id = window.setInterval(() => setFrame(value => (value + 1) % FRAMES.length), 80) - - return () => window.clearInterval(id) - }, [profile]) - return (
- {FRAMES[frame]} + {/* Was a local 80ms setInterval + setState braille ticker — the same + mechanism class (per-tick DOM mutation scheduling style recalc) + that GlyphSpinner was rewritten to remove. `braille` is exactly the + frame set and 80ms cadence this used. `justify-start` keeps the + glyph left-aligned in its w-3 box the way the bare span was, and + `paused` restores the old "no ticking once the swap is done" + behaviour while the overlay fades out still mounted. */} + {t.composer.wakingProfile(label ?? '')}
diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts index 0300430b30..3d0e7e35b6 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts @@ -52,6 +52,7 @@ import { $attentionSessionIds, $workingSessionIds, liveSessionScopes, + reconcileBusyStatesOnReconnect, recordSessionEventScope, resetTileRuntimeBindings } from '@/store/session-states' @@ -252,6 +253,12 @@ export function useGatewayBoot({ // A respawned backend re-mints (recycles) runtime ids, so any tile's // bound runtime id is now stale — drop them so each tile re-resumes. resetTileRuntimeBindings() + // Same staleness, other half: pre-reconnect busy flags are keyed by + // those dead runtime ids and would never receive their terminal + // busy:false — clear them or the sidebar running arc lies forever + // (#53902/#73082). A genuinely live turn re-asserts busy on its next + // post-reconnect event. + reconcileBusyStatesOnReconnect() // Resync state that may have moved on the backend while we were asleep. await callbacksRef.current.refreshHermesConfig().catch(() => undefined) await callbacksRef.current.refreshSessions().catch(() => undefined) diff --git a/apps/desktop/src/components/assistant-ui/thread/assistant-message.tsx b/apps/desktop/src/components/assistant-ui/thread/assistant-message.tsx index cdd725865f..f2cfdc09c1 100644 --- a/apps/desktop/src/components/assistant-ui/thread/assistant-message.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/assistant-message.tsx @@ -7,7 +7,7 @@ import { useMessageRuntime } from '@assistant-ui/react' import { useStore } from '@nanostores/react' -import { type FC, useCallback, useMemo, useState } from 'react' +import { type FC, type ReactNode, useCallback, useMemo, useState } from 'react' import { useSessionView } from '@/app/chat/session-view' import { ChangedFilesCard } from '@/components/assistant-ui/thread/changed-files-card' @@ -42,6 +42,14 @@ import { $voicePlayback } from '@/store/voice-playback' // would re-derive the changed-files card on every message re-render. const EMPTY_PARTS: readonly unknown[] = [] +// PERF: hoisted to module scope so the element OBJECT is identical on every +// render of every assistant message. React bails out of re-rendering a child +// whose element identity is unchanged, so a status flip on the message root +// (pending -> complete and back, N rows per stream flush) can no longer +// descend into the parts subtree at all. Its props were already the module +// constant MESSAGE_PARTS_COMPONENTS, so nothing per-message is captured here. +const MESSAGE_PARTS = + interface MessageActionProps { messageId: string /** Lazy accessor — reads the live message text at action time. Passing the @@ -52,14 +60,12 @@ interface MessageActionProps { onBranchInNewChat?: (messageId: string) => void } -export const AssistantMessage: FC<{ +interface AssistantMessageProps { onBranchInNewChat?: (messageId: string) => void onDismissError?: (messageId: string) => void -}> = ({ onBranchInNewChat, onDismissError }) => { - const messageId = useAuiState(s => s.message.id) - const messageRuntime = useMessageRuntime() - const { t } = useI18n() +} +export const AssistantMessage: FC = props => { // A reply to an inter-agent delivery is part of that exchange, not part of // the human conversation — collapse it under a compact notice ("Reply to // ", expandable), mirroring the sender-side notice the previous @@ -94,14 +100,77 @@ export const AssistantMessage: FC<{ return null }) - // PERF: this component must NOT subscribe to the streaming text. Every - // selector here returns a value that stays referentially stable across - // token flushes (booleans, status strings, '' while running), so the - // 30 Hz delta stream only re-renders the markdown part and the tiny - // TurnActivityIndicator leaf — not the footer/preview/root subtree. - const messageStatus = useAuiState(s => s.message.status?.type) - const isRunning = messageStatus === 'running' - const isPlaceholder = useAuiState(s => s.message.status?.type === 'running' && s.message.content.length === 0) + // The collapse gate below needs the LIVE running status, but only an + // inter-agent reply can ever be collapsed. Dispatching on that first keeps + // the status subscription out of the standard path entirely — the standard + // message root now re-renders for content, never for a pending flip. + return interAgentSender ? ( + + ) : ( + + ) +} + +/** The compact stand-in a settled inter-agent reply collapses to (Grok-bots + * parity — the transcript shows the event; the text is one click away). */ +const InterAgentCollapsedNotice: FC<{ sender: string }> = ({ sender }) => ( +
+ + + Replied to {sender} + +
+ + show reply + +
+ {MESSAGE_PARTS} +
+
+
+) + +/** + * An assistant reply that answers an inter-agent delivery. Owns the only + * root-level `isRunning` subscription left in this file, and it is confined to + * the rare inter-agent case: the reply renders collapsed once it settles, so + * the gate genuinely needs live status. Never collapse while streaming — the + * user should see progress. + * + * The collapse is expressed as a CHILD of the normal body, not as a competing + * root. Returning a bare MessagePrimitive.Root here for the settled case put a + * different element type in this position than the running case + * (AssistantMessageBody), so settling unmounted the whole row and mounted a + * fresh one — throwing away the DOM the scroll anchor was holding, which can + * jump the transcript under the reader. One component, one root, children + * vary: settling is now a prop change React applies in place. + */ +const InterAgentAssistantMessage: FC = ({ sender, ...props }) => { + const isRunning = useAuiState(s => s.message.status?.type === 'running') + + return ( + } + /> + ) +} + +const AssistantMessageBody: FC = ({ + collapsedNotice = null, + onBranchInNewChat, + onDismissError +}) => { + const messageId = useAuiState(s => s.message.id) + const messageRuntime = useMessageRuntime() + const { t } = useI18n() + + // PERF: this component must NOT subscribe to the streaming text, and no + // longer subscribes to the streaming STATUS either. Every selector here + // returns a value that stays referentially stable across token flushes + // (booleans, '' while running), so the 30 Hz delta stream only re-renders + // the markdown part and the tiny status leaves — not the footer, the + // preview block, or this root. const hasVisibleText = useAuiState(s => contentHasVisibleText(s.message.content)) // Sealed mid-turn commentary keeps its text but not the footer, so a // tool-heavy turn doesn't grow a copy/refresh bar per paragraph (see @@ -112,16 +181,154 @@ export const AssistantMessage: FC<{ // stable across the 30 Hz delta stream, so this adds no per-token renders). const turnDurationS = useAuiState(s => s.message.metadata?.custom?.durationS as number | undefined) - // The thinking/stall indicator belongs to the TAIL of the thread, period. A - // stale pending bubble mid-transcript (a turn that ended without its settle - // event, a steer race) must never show one — a spinner above a later user - // message reads as the agent answering out of order. Booleans are stable - // across token flushes, so this selector adds no streaming re-renders. - const isLastMessage = useAuiState(s => s.thread.messages[s.thread.messages.length - 1]?.id === s.message.id) + const getMessageText = useCallback(() => messageContentText(messageRuntime.getState().content), [messageRuntime]) - // Preview targets only materialize once the turn completes — while running - // the selector returns '' (stable), so per-token flushes skip the regex - // scan and the re-render it would cause. + // useEnterAnimation consults `enabled` ONLY when its callback ref fires, + // i.e. at mount: the hook parks the value in a ref and returns a + // useCallback([]) identity, and its own contract is "`enabled` is captured + // at mount-time only — flipping it later doesn't suddenly play the animation + // on existing nodes" (see lib/use-enter-animation.ts). So a live + // subscription here would re-render this root on every pending flip to feed + // a value the hook already ignores. Capture it once, off the runtime, with + // no subscription at all. + const [initiallyRunning] = useState(() => messageRuntime.getState().status?.type === 'running') + const enterRef = useEnterAnimation(initiallyRunning, `assistant-message:${messageId}`) + + // Double-click the reply to heart it (iMessage). Undefined while reactions + // are off, so the root carries no listener at all. + const onDoubleClick = useTapbackDoubleClick(messageId, 'assistant') + + return ( + + {collapsedNotice ?? ( + <> +
+ {/* Todos render in the composer status stack now, not inline. */} + {MESSAGE_PARTS} + + + + + + {onDismissError && ( + onDismissError(messageId)} + side="top" + tooltip={t.assistant.thread.dismissError} + > + + + )} + + +
+ + {hasVisibleText && !isInterim && ( + + )} + {/* Last thing in the turn — under the action bar, the way Cursor ends a + turn on its summary rather than burying it above the controls. */} + + + + )} + + ) +} + +/** + * PERF leaf: the only subscriber to this message's streaming status inside the + * message content. Previously `messageStatus` / `isPlaceholder` / + * `isLastMessage` were read by AssistantMessage itself, so every pending flip + * re-rendered the whole message subtree — at stream breadth N, N subtrees in a + * single commit, which is what widened the recalc scope. Reading them here + * confines the flip to this leaf; the sibling parts subtree is a hoisted + * constant element and bails out. + * + * Behaviour is byte-identical to the old inline expression, including the + * TAIL-ONLY rule: the activity row belongs to the tail of the thread, period. + * A stale pending bubble mid-transcript (a turn that ended without its settle + * event, a steer race) must never show one — a spinner above a later user + * message reads as the agent answering out of order. + * + * The activity row is mounted by the TAIL of the thread and decides for itself + * whether the turn owes the user a line, so there is deliberately no + * `isRunning` gate on the mount here. Gating it on this bubble's own `running` + * status was the hole: a turn that seals a bubble mid-flight (message.interim) + * or finishes one while the agent keeps going leaves a settled message at the + * tail, so the row unmounted and the seconds went uncounted while the + * composer's arc border and Stop button said work was still happening. + * TurnActivityIndicator subscribes to the status it needs internally, so it is + * itself a leaf and this stays off the message root either way. + */ +const AssistantStatusSlot: FC = () => { + // ONE subscription, not one per input. Each useAuiState is a separate store + // subscription with its own equality check and its own chance to schedule a + // render, and these inputs always move together on a status flip — so + // reading them separately just multiplies the wake-ups for a single logical + // change. The selector collapses them to one stable string, which bails out + // on every flush that does not actually change what this slot renders. + const slot = useAuiState(s => { + if (s.thread.messages[s.thread.messages.length - 1]?.id !== s.message.id) { + return 'none' + } + + return s.message.status?.type === 'running' && s.message.content.length === 0 ? 'placeholder' : 'activity' + }) + + if (slot === 'none') { + return null + } + + return slot === 'placeholder' ? : +} + +/** + * PERF leaf: owns the settled-text selector that feeds the link previews. + * + * This was the last status-dependent read at the message root, and the most + * expensive one: the selector flips between '' while running and the full + * `messageContentText(content)` join once settled, so every running <-> settled + * transition re-ran the join for the whole message AND re-rendered the root. + * At stream breadth N that is N joins plus N root re-renders per flip. Reading + * it here confines both to this leaf, which renders nothing at all in the + * common case. + * + * The streaming-side optimization is unchanged and still the point of the '' + * branch: preview targets only materialize once the turn completes, so while + * running the selector returns a stable '' and per-token flushes skip the + * regex scan and the re-render it would cause. + * + * Renders exactly what the root used to render at this position — the same + * wrapper div with the same classes, or nothing when there are no targets — + * so the DOM is byte-identical either way. A component boundary adds no node + * of its own, so unlike StreamingMarker this needs no placement care. + */ +const AssistantPreviewEmbeds: FC = () => { const completedText = useAuiState(s => s.message.status?.type === 'running' ? '' : messageContentText(s.message.content) ) @@ -134,120 +341,95 @@ export const AssistantMessage: FC<{ return pickPrimaryPreviewTarget(extractPreviewTargets(completedText)) }, [completedText]) - const getMessageText = useCallback(() => messageContentText(messageRuntime.getState().content), [messageRuntime]) + if (previewTargets.length === 0) { + return null + } - // Cursor's changed-files card only appears once the turn settles: while the - // agent is still editing, the tool rows narrate each patch and a card that - // grew a row per write would thrash the transcript. `[]` while running keeps - // this selector referentially stable across the 30 Hz delta stream. - // - // It also only rides the LAST turn. The card is a "here's what just landed" - // summary, not a per-turn artifact: leaving one behind on every reply would - // stack a wall of stale cards down the transcript. Sending the next message - // retires it — the working tree it describes is already history by then. + return ( +
+ {previewTargets.map(target => ( + + ))} +
+ ) +} + +/** + * PERF leaf: owns the `settledParts` selector so the tail's settle stops + * re-rendering the message root. This is the one status-derived selector that + * returns an OBJECT (`s.message.parts`) rather than a primitive, so it cannot + * bail out on identity churn — keeping it at the root meant every settle + * re-rendered the root and everything under it. + * + * Cursor's changed-files card only appears once the turn settles: while the + * agent is still editing, the tool rows narrate each patch and a card that + * grew a row per write would thrash the transcript. `EMPTY_PARTS` while + * running keeps this selector referentially stable across the 30 Hz delta + * stream. + * + * It also only rides the LAST turn. The card is a "here's what just landed" + * summary, not a per-turn artifact: leaving one behind on every reply would + * stack a wall of stale cards down the transcript. Sending the next message + * retires it — the working tree it describes is already history by then. + */ +const SettledChangedFiles: FC = () => { const settledParts = useAuiState(s => { const isLastMessage = s.thread.messages[s.thread.messages.length - 1]?.id === s.message.id return s.message.status?.type === 'running' || !isLastMessage ? EMPTY_PARTS : s.message.parts }) - const enterRef = useEnterAnimation(isRunning, `assistant-message:${messageId}`) + return +} - // Double-click the reply to heart it (iMessage). Undefined while reactions - // are off, so the root carries no listener at all. - const onDoubleClick = useTapbackDoubleClick(messageId, 'assistant') - - // Reply inside an inter-agent exchange: render collapsed (Grok-bots - // parity — the transcript shows the event; the text is one click away). - // Never collapse while streaming: the user should see progress, and the - // status selectors above stay live either way. - if (interAgentSender && !isRunning) { - return ( - -
- - - Replied to {interAgentSender} - -
- - show reply - -
- -
-
-
- - ) - } +/** + * Carries the streaming flag that used to sit on the message root as + * `data-streaming`. + * + * The flag has no CSS behind it (every `[data-streaming='true']` rule targets + * `[data-slot='code-card']`), but it is not dead: it is the settled-row signal + * for the short-session hang repro, which derives the settled count by + * subtracting the number of `[data-message-streaming='true']` markers from the + * number of message roots, and gates the assistant-response wait on that count + * growing. At most one marker per row carries the attribute, which is what + * makes the subtraction exact. + * + * Deliberately NOT named `data-streaming`: shiki-highlighter.tsx puts that + * exact attribute on a deferred `[data-slot='code-card']`, which is a + * descendant of this root. Once the repro matches on a descendant rather than + * the root's own attribute, a shared name would make any message holding a + * still-deferred code card read as "still streaming". A distinct name keeps + * the signal about the MESSAGE and immune to how deep it sits. + * + * On the root it was a per-flip attribute write on the element that owns the + * whole message subtree, which is the invalidation this prong exists to remove. + * Three properties make this placement cheap and behaviour-neutral: + * + * - A ROOT-LEVEL sibling, not a child of the message content. The + * `:first-child` / `:last-child` margin rules in styles.css match blocks + * *inside* `[data-slot='aui_assistant-message-content']`; a node added + * there would steal `:last-child` from the status indicator and silently + * change the gap between bubbles mid-stream. No rule selects message-root + * children by position, so this slot is inert. + * - PERMANENTLY MOUNTED, toggling only the attribute. Mounting/unmounting per + * flip would be a DOM structure change and dirty its siblings; an attribute + * write on a childless node invalidates exactly one element. + * - `display: none`, so it costs no layout or paint. `querySelectorAll` and + * `:has()` still match it — they read the DOM, not the box tree. + * + * Tracks plain `isRunning` (not tail-only), exactly like the old root + * attribute, so the repro's row accounting is unchanged. + */ +const StreamingMarker: FC = () => { + const isRunning = useAuiState(s => s.message.status?.type === 'running') return ( - -
- {/* Todos render in the composer status stack now, not inline. */} - - {/* The activity row is mounted by the TAIL of the thread and decides - for itself whether the turn owes the user a line. Gating the mount - on this bubble's own `running` status was the hole: a turn that - seals a bubble mid-flight (message.interim) or finishes one while - the agent keeps going leaves a settled message at the tail, so the - row unmounted and the seconds went uncounted while the composer's - arc border and Stop button said work was still happening. */} - {isLastMessage && (isPlaceholder ? : )} - {previewTargets.length > 0 && ( -
- {previewTargets.map(target => ( - - ))} -
- )} - - - - {onDismissError && ( - onDismissError(messageId)} - side="top" - tooltip={t.assistant.thread.dismissError} - > - - - )} - - -
- - {hasVisibleText && !isInterim && ( - - )} - {/* Last thing in the turn — under the action bar, the way Cursor ends a - turn on its summary rather than burying it above the controls. */} - - +