diff --git a/.gitignore b/.gitignore index a72536f00779..cd05306af00b 100644 --- a/.gitignore +++ b/.gitignore @@ -152,6 +152,11 @@ docs/superpowers/* .update-incomplete .update-incomplete.lock +# Checkout fingerprint the __pycache__ tree was last validated against +# (launch-time stale-bytecode sweep). Runtime state, never a code change. +.bytecode-fingerprint +.bytecode-fingerprint.tmp + # Installer-written method stamp in the managed checkout root (scripts/install.sh). # Runtime metadata only — never a code change. Ignore so `git status` stays clean # and `hermes update`'s untracked autostash does not treat it as a local edit (#66189 / #54855). diff --git a/agent/codex_runtime.py b/agent/codex_runtime.py index 31871dca8f5a..0c5bb7b70503 100644 --- a/agent/codex_runtime.py +++ b/agent/codex_runtime.py @@ -778,12 +778,27 @@ def run_codex_app_server_turn( # the already-flushed user turn). See gateway/run.py agent_persisted. if getattr(agent, "_session_db", None) is not None: try: - agent._flush_messages_to_session_db(messages) + _codex_flush_ok = agent._flush_messages_to_session_db(messages) except Exception: - logger.debug( + _codex_flush_ok = False + logger.warning( "codex app-server projected-message flush failed", exc_info=True, ) + if _codex_flush_ok is False: + # Unlike the chat-completions loop (which fails closed BEFORE + # projection — see conversation_loop session_persistence_failed), + # codex output has already streamed to the user by the time this + # flush runs, so there is nothing left to withhold. We cannot + # flip agent_persisted=False either: the gateway fallback write + # would re-INSERT the already-flushed user turn (#860/#42039). + # Surface the durability gap loudly instead of a silent debug. + logger.warning( + "codex app-server turn was delivered but could NOT be " + "persisted to the session DB (session=%s) — this turn " + "will be missing after restart/resume", + getattr(agent, "session_id", None), + ) # Counter ticks for the agent-improvement loop. diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 68e5d3e1207a..1d5bc1cc799f 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -1061,6 +1061,9 @@ def run_conversation( # Commentary deduplication spans all provider continuations and tool calls # within one user turn, but must not suppress the same phrase next turn. agent._delivered_interim_texts = set() + # A configured SessionDB append failure halts only the affected turn. A + # cached gateway agent must recover on the next message if storage did. + agent._incremental_persistence_failed = False # Main conversation loop counters (pure locals consumed by the loop below). api_call_count = 0 @@ -5813,8 +5816,6 @@ def run_conversation( and previous_interim_visible == current_interim_visible ) messages.append(assistant_msg) - if not duplicate_previous_interim: - agent._emit_interim_assistant_message(assistant_msg) # Mixed batch: error-result the invalid calls and strip them # from the execution set. The assistant message above keeps @@ -5836,13 +5837,17 @@ def run_conversation( if tc.function.name in agent.valid_tool_names ] + _tool_turn_persisted = None try: # Persist the assistant tool-call turn before any tool # side effects run. If a destructive tool restarts or # terminates Hermes mid-turn, resume logic still sees the # exact tool-call block that already executed. - agent._flush_messages_to_session_db(messages, conversation_history) + _tool_turn_persisted = agent._flush_messages_to_session_db( + messages, conversation_history + ) except Exception as exc: + _tool_turn_persisted = False logger.warning( "Incremental tool-call persistence failed before execution " "(session=%s): %s", @@ -5850,6 +5855,22 @@ def run_conversation( exc, ) + if _tool_turn_persisted is False: + # The canonical append failed. Do not project the row or + # run side-effecting tools from state that exists only in + # this process. Breaking also avoids retrying the same + # unpersisted turn until the iteration budget is exhausted. + _turn_exit_reason = "session_persistence_failed" + final_response = "" + failed = True + break + + # A UI must never observe an assistant/tool-call row that is + # still only an ephemeral in-memory projection. Emit interim + # commentary only after the canonical SessionDB append above. + if not duplicate_previous_interim: + agent._emit_interim_assistant_message(assistant_msg) + # Close any open streaming display (response box, reasoning # box) before tool execution begins. Intermediate turns may # have streamed early content that opened the response box; @@ -5864,6 +5885,15 @@ def run_conversation( agent._execute_tool_calls(assistant_message, messages, effective_task_id, api_call_count) + if getattr(agent, "_incremental_persistence_failed", False): + # A tool result could not be made canonical. Do not send + # the in-memory result back to the model or project any + # later events from this turn. + _turn_exit_reason = "session_persistence_failed" + final_response = "" + failed = True + break + if agent._tool_guardrail_halt_decision is not None: decision = agent._tool_guardrail_halt_decision _turn_exit_reason = "guardrail_halt" diff --git a/agent/reasoning_timeouts.py b/agent/reasoning_timeouts.py index 8af5ab799f48..9c5fc2020152 100644 --- a/agent/reasoning_timeouts.py +++ b/agent/reasoning_timeouts.py @@ -106,6 +106,14 @@ _REASONING_STALE_TIMEOUT_FLOORS: tuple[tuple[str, int], ...] = ( ("claude-sonnet-5", 180), ("claude-sonnet-4.5", 180), ("claude-sonnet-4.6", 180), + # Anthropic Mythos-class named reasoning models (claude-fable-5, …). + # 1M context + 128K output — heavier thinking phase than the + # numbered Claude line, so the floor is in the deep-reasoning tier + # alongside o1 / deepseek-r1 / nemotron-3-ultra. Without this + # entry the stale-stream detector kills fable-5's thinking phase + # at the default 180s (300s with context scaling), tripping the + # cross-turn circuit breaker after 5 consecutive stale kills. + ("claude-fable", 600), # xAI Grok reasoning variants. Explicit reasoning-only keys # plus one for the ``non-reasoning`` variant so users picking # the fast variant don't get the 300s floor. Bare ``grok-3``, @@ -207,6 +215,8 @@ def get_reasoning_stale_timeout_floor(model: object) -> Optional[float]: 300.0 >>> get_reasoning_stale_timeout_floor("anthropic/claude-opus-4-6") 240.0 + >>> get_reasoning_stale_timeout_floor("anthropic/claude-fable-5") + 600.0 >>> get_reasoning_stale_timeout_floor("gpt-4o") is None True >>> get_reasoning_stale_timeout_floor("olmo-1") is None diff --git a/agent/tool_executor.py b/agent/tool_executor.py index 7c9fea123296..1dfcf1ebf6ef 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -141,8 +141,8 @@ def _flush_session_db_after_tool_progress( messages: list, *, stage: str, -) -> None: - """Best-effort incremental SessionDB flush for tool-call progress. +) -> bool: + """Flush tool-call progress before projecting it to any UI surface. Tool execution can perform side effects that terminate or restart the current Hermes process before the normal turn-end persistence path runs. @@ -150,9 +150,14 @@ def _flush_session_db_after_tool_progress( transcript survives destructive-but-valid tool calls. """ try: - agent._flush_messages_to_session_db(messages) + persisted = agent._flush_messages_to_session_db(messages) is not False + if not persisted: + agent._incremental_persistence_failed = True + return persisted except Exception as exc: + agent._incremental_persistence_failed = True logger.warning("Incremental tool-call persistence failed after %s: %s", stage, exc) + return False def _ra(): @@ -1099,6 +1104,8 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe ): r = results[i] blocked = False + is_error = True + progress_function_name = name # A worker can finish and write results[i] in the window between the # deadline snapshot (timed_out_indices, taken from not_done) and this # loop. Prefer that real result over a fabricated timeout message — the @@ -1156,6 +1163,7 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe function_name, function_args, function_result, tool_duration, is_error, blocked, middleware_trace = r name = function_name args = function_args + progress_function_name = function_name if blocked: effect_disposition = "none" @@ -1183,44 +1191,15 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe except Exception as _ver_err: logging.debug("file-mutation verifier record failed: %s", _ver_err) - if not blocked and agent.tool_progress_callback: - try: - agent.tool_progress_callback( - "tool.completed", function_name, None, None, - duration=tool_duration, is_error=is_error, - result=function_result, - ) - except Exception as cb_err: - logging.debug(f"Tool progress callback error: {cb_err}") - if agent.verbose_logging: logging.debug(f"Tool {function_name} completed in {tool_duration:.2f}s") logging.debug(f"Tool result ({len(function_result)} chars): {function_result}") - # Print cute message per tool - if agent._should_emit_quiet_tool_messages(): - cute_msg = _get_cute_tool_message_impl(name, args, tool_duration, result=function_result) - agent._safe_print(f" {cute_msg}") - elif not agent.quiet_mode and getattr(agent, "tool_progress_mode", "all") != "off": - _preview_str = _multimodal_text_summary(function_result) - if agent.verbose_logging: - print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s") - print(agent._wrap_verbose("Result: ", _preview_str)) - else: - response_preview = _preview_str[:agent.log_prefix_chars] + "..." if len(_preview_str) > agent.log_prefix_chars else _preview_str - print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s - {response_preview}") - agent._current_tool = None _status_suffix = " (error)" if is_error else "" agent._touch_activity(f"tool completed: {name} ({tool_duration:.1f}s){_status_suffix}") - if not blocked and agent.tool_complete_callback: - try: - display_args = _redact_tool_args_for_display(name, args) or args - agent.tool_complete_callback(tc.id, name, display_args, function_result) - except Exception as cb_err: - logging.debug(f"Tool complete callback error: {cb_err}") - + display_function_result = function_result function_result = maybe_persist_tool_result( content=function_result, tool_name=name, @@ -1255,6 +1234,50 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe ) messages.append(tool_message) risk_metadata = tool_message.get("_tool_output_risk") + if not _flush_session_db_after_tool_progress( + agent, + messages, + stage=f"tool result {name}", + ): + return + + # Every completion surface is downstream of the canonical append. If + # the UI bridge or process dies while projecting one of these events, + # resume can reconstruct the tool result that was already visible. + if not blocked and agent.tool_progress_callback: + try: + agent.tool_progress_callback( + "tool.completed", progress_function_name, None, None, + duration=tool_duration, is_error=is_error, + result=display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool progress callback error: {cb_err}") + + # Print cute message per tool + if agent._should_emit_quiet_tool_messages(): + cute_msg = _get_cute_tool_message_impl( + name, args, tool_duration, result=display_function_result, + ) + agent._safe_print(f" {cute_msg}") + elif not agent.quiet_mode and getattr(agent, "tool_progress_mode", "all") != "off": + _preview_str = _multimodal_text_summary(display_function_result) + if agent.verbose_logging: + print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s") + print(agent._wrap_verbose("Result: ", _preview_str)) + else: + response_preview = _preview_str[:agent.log_prefix_chars] + "..." if len(_preview_str) > agent.log_prefix_chars else _preview_str + print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s - {response_preview}") + + if not blocked and agent.tool_complete_callback: + try: + display_args = _redact_tool_args_for_display(name, args) or args + agent.tool_complete_callback( + tc.id, name, display_args, display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool complete callback error: {cb_err}") + if ( risk_metadata is not None and risk_metadata.get("risk") != "low" @@ -1271,11 +1294,6 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe ) except Exception as cb_err: logging.debug("Tool output risk callback error: %s", cb_err) - _flush_session_db_after_tool_progress( - agent, - messages, - stage=f"tool result {name}", - ) # ── Per-tool /steer drain ─────────────────────────────────── # Same as the sequential path: drain between each collected @@ -1307,6 +1325,8 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe # Resolve the context-scaled tool-output budget once per turn. _tool_budget = _budget_for_agent(agent) for i, tool_call in enumerate(assistant_message.tool_calls, 1): + if getattr(agent, "_incremental_persistence_failed", False): + return # SAFETY: check interrupt BEFORE starting each tool. # If the user sent "stop" during a previous tool's execution, # do NOT start any more tools -- skip them all immediately. @@ -1322,11 +1342,12 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe skipped_tc.id, effect_disposition="none", )) - _flush_session_db_after_tool_progress( + if not _flush_session_db_after_tool_progress( agent, messages, stage=f"cancelled tool result {skipped_name}", - ) + ): + return break function_name = tool_call.function.name @@ -1342,11 +1363,12 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe tool_call.id, ) ) - _flush_session_db_after_tool_progress( + if not _flush_session_db_after_tool_progress( agent, messages, stage=f"invalid tool arguments {function_name}", - ) + ): + return agent._apply_pending_steer_to_tool_results(messages, 1) continue @@ -1841,16 +1863,6 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe except Exception as _ver_err: logging.debug("file-mutation verifier record failed: %s", _ver_err) - if not _execution_blocked and agent.tool_progress_callback: - try: - agent.tool_progress_callback( - "tool.completed", function_name, None, None, - duration=tool_duration, is_error=_is_error_result, - result=function_result, - ) - except Exception as cb_err: - logging.debug(f"Tool progress callback error: {cb_err}") - agent._current_tool = None _status_suffix = " (error)" if _is_error_result else "" agent._touch_activity(f"tool completed: {function_name} ({tool_duration:.1f}s){_status_suffix}") @@ -1860,13 +1872,7 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe _log_result = _multimodal_text_summary(function_result) logging.debug(f"Tool result ({len(_log_result)} chars): {_log_result}") - if not _execution_blocked and agent.tool_complete_callback: - try: - display_args = _redact_tool_args_for_display(function_name, function_args) or function_args - agent.tool_complete_callback(tool_call.id, function_name, display_args, function_result) - except Exception as cb_err: - logging.debug(f"Tool complete callback error: {cb_err}") - + display_function_result = function_result function_result = maybe_persist_tool_result( content=function_result, tool_name=function_name, @@ -1889,6 +1895,40 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe tool_message = make_tool_result_message(function_name, _tool_content, tool_call.id) messages.append(tool_message) risk_metadata = tool_message.get("_tool_output_risk") + if not _flush_session_db_after_tool_progress( + agent, + messages, + stage=f"tool result {function_name}", + ): + return + + # UI completion/progress events are projections of the canonical tool + # row, never a competing in-memory authority. + if not _execution_blocked and agent.tool_progress_callback: + try: + agent.tool_progress_callback( + "tool.completed", function_name, None, None, + duration=tool_duration, is_error=_is_error_result, + result=display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool progress callback error: {cb_err}") + + if not _execution_blocked and agent.tool_complete_callback: + try: + display_args = ( + _redact_tool_args_for_display(function_name, function_args) + or function_args + ) + agent.tool_complete_callback( + tool_call.id, + function_name, + display_args, + display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool complete callback error: {cb_err}") + if ( risk_metadata is not None and risk_metadata.get("risk") != "low" @@ -1905,11 +1945,6 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe ) except Exception as cb_err: logging.debug("Tool output risk callback error: %s", cb_err) - _flush_session_db_after_tool_progress( - agent, - messages, - stage=f"tool result {function_name}", - ) # ── Per-tool /steer drain ─────────────────────────────────── # Drain pending steer BETWEEN individual tool calls so the @@ -1937,11 +1972,12 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe skipped_tc.id, effect_disposition="none", )) - _flush_session_db_after_tool_progress( + if not _flush_session_db_after_tool_progress( agent, messages, stage=f"skipped tool result {skipped_name}", - ) + ): + return break if agent.tool_delay > 0 and i < len(assistant_message.tool_calls): @@ -1992,6 +2028,8 @@ def execute_tool_calls_segmented(agent, assistant_message, messages: list, effec segments = _plan_tool_batch_segments(assistant_message.tool_calls, execution_cwd=_exec_cwd) for kind, calls in segments: + if getattr(agent, "_incremental_persistence_failed", False): + return segment_message = SimpleNamespace(tool_calls=list(calls)) if kind == "parallel": execute_tool_calls_concurrent( @@ -2004,6 +2042,9 @@ def execute_tool_calls_segmented(agent, assistant_message, messages: list, effec finalize=False, ) + if getattr(agent, "_incremental_persistence_failed", False): + return + # ── Whole-turn finalize (budget + /steer) ───────────────────────── total_tools = len(assistant_message.tool_calls) if total_tools > 0: diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts index c356fae3af84..c7a4e2f138ac 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts @@ -121,3 +121,44 @@ describe('useComposerTrigger — slash anywhere in the prompt', () => { expect(hook.result.current.trigger).toBeNull() }) }) + +describe('useComposerTrigger — free-text slash arguments', () => { + it('keeps a picked /goal command as editable text while retaining subcommand completion', () => { + const editor = mountEditor('/go') + const goal = item('/goal', 'Commands') + const { hook } = mountTrigger(editor, [goal]) + + act(() => hook.result.current.refreshTrigger()) + act(() => hook.result.current.replaceTriggerWithChip(goal)) + + expect(composerPlainText(editor)).toBe('/goal ') + expect(editor.querySelector('[data-slash-kind]')).toBeNull() + expect(hook.result.current.trigger).not.toBeNull() + }) + + it('does not seal a multi-word /goal into a chip when the option list runs empty', () => { + const editor = mountEditor('/goal finish the full prompt') + const { hook } = mountTrigger(editor, []) + + act(() => hook.result.current.refreshTrigger()) + + expect(hook.result.current.slashFreeTextArgStage).toBe(true) + expect(hook.result.current.commitTypedSlashDirective()).toBe(false) + expect(composerPlainText(editor)).toBe('/goal finish the full prompt') + expect(editor.querySelector('[data-slash-kind]')).toBeNull() + }) + + it('still commits a fully typed finite option as one directive chip', () => { + const editor = mountEditor('/personality creative') + const { hook } = mountTrigger(editor, []) + + act(() => hook.result.current.refreshTrigger()) + + expect(hook.result.current.slashFreeTextArgStage).toBe(false) + act(() => { + expect(hook.result.current.commitTypedSlashDirective()).toBe(true) + }) + expect(composerPlainText(editor)).toBe('/personality creative ') + expect(editor.querySelector('[data-slash-kind]')?.getAttribute('data-ref-text')).toBe('/personality creative') + }) +}) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts index 3c22fddb6348..2df218e78f30 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts @@ -2,7 +2,7 @@ import type { Unstable_TriggerAdapter, Unstable_TriggerItem } from '@assistant-u import { type MutableRefObject, type RefObject, useCallback, useEffect, useRef, useState } from 'react' import { hermesDirectiveFormatter } from '@/components/assistant-ui/directive-text' -import { desktopSlashCommandTakesArgs } from '@/lib/desktop-slash-commands' +import { desktopSlashCommandArgumentMode } from '@/lib/desktop-slash-commands' import { COMPLETION_ACTIONS, @@ -89,11 +89,16 @@ export function useComposerTrigger({ const before = textBeforeCaret(editor) const found = detectTrigger(before ?? composerPlainText(editor)) - // The arg-stage popover is only useful for commands with an options screen. - // For a no-arg command it would dead-end on "No matches", so drop it — the - // directive is already complete. + // A text-only command has no completion screen once its prose begins. Mixed + // commands such as /goal stay live so their finite subcommands can still be + // suggested, while arbitrary goal text remains valid. + const argumentMode = + found?.kind === '/' && slashArgStage(found.query) + ? desktopSlashCommandArgumentMode(slashCommandToken(found.query)) + : null + const detected = - found?.kind === '/' && slashArgStage(found.query) && !desktopSlashCommandTakesArgs(slashCommandToken(found.query)) + found?.kind === '/' && slashArgStage(found.query) && argumentMode !== 'options' && argumentMode !== 'mixed' ? null : found @@ -134,6 +139,11 @@ export function useComposerTrigger({ // Space/Tab — neither should dead-end on a popover. const argStageEmpty = trigger?.kind === '/' && slashArgStage(trigger.query) && !triggerLoading && !triggerItems.length + const slashFreeTextArgStage = + trigger?.kind === '/' && + slashArgStage(trigger.query) && + ['mixed', 'text'].includes(desktopSlashCommandArgumentMode(slashCommandToken(trigger.query)) ?? '') + const closeTrigger = () => { setTrigger(null) setTriggerItems([]) @@ -148,9 +158,16 @@ export function useComposerTrigger({ // the completion list is empty because the arg is already fully typed (the // backend completer drops exact matches). Reuses the chip path via a // synthetic item whose serialized form is the verbatim text. - const commitTypedSlashDirective = () => { + const commitTypedSlashDirective = (): boolean => { if (trigger?.kind !== '/') { - return + return false + } + + // Free prose must stay ordinary contentEditable text. This guard also + // protects against a stale completion result reaching the keydown path + // before refreshTrigger has caught up with the latest DOM input. + if (desktopSlashCommandArgumentMode(slashCommandToken(trigger.query)) !== 'options') { + return false } const text = `/${trigger.query.trimEnd()}` @@ -168,6 +185,8 @@ export function useComposerTrigger({ rawText: text } }) + + return true } const replaceTriggerWithChip = (item: Unstable_TriggerItem) => { @@ -208,15 +227,15 @@ export function useComposerTrigger({ // there's no command invocation for the args to belong to. const command = (item.metadata as { command?: string } | undefined)?.command ?? '' - const expandsToArgs = - trigger.kind === '/' && !trigger.inline && !serialized.includes(' ') && desktopSlashCommandTakesArgs(command) + const argumentMode = desktopSlashCommandArgumentMode(command) + const expandsToArgs = trigger.kind === '/' && !trigger.inline && !serialized.includes(' ') && argumentMode !== null const text = starter || serialized.endsWith(' ') ? serialized : `${serialized} ` const directive = !starter && serialized.match(/^@([^:]+):(.+)$/) // No pill while expanding — the bare command stays plain text until an arg // is picked, at which point a single pill is emitted for the full command. const slashKind = !expandsToArgs && trigger.kind === '/' ? slashChipKindForItem(item) : null - const keepTriggerOpen = starter || expandsToArgs + const keepTriggerOpen = starter || (expandsToArgs && argumentMode !== 'text') const finish = () => { draftRef.current = composerPlainText(editor) @@ -288,6 +307,7 @@ export function useComposerTrigger({ refreshTrigger, replaceTriggerWithChip, setTriggerActive, + slashFreeTextArgStage, trigger, triggerActive, triggerItems, diff --git a/apps/desktop/src/app/chat/composer/index.tsx b/apps/desktop/src/app/chat/composer/index.tsx index 5988d6103707..b5a76f349040 100644 --- a/apps/desktop/src/app/chat/composer/index.tsx +++ b/apps/desktop/src/app/chat/composer/index.tsx @@ -304,6 +304,7 @@ export function ChatBar({ refreshTrigger, replaceTriggerWithChip, setTriggerActive, + slashFreeTextArgStage, trigger, triggerActive, triggerItems, @@ -544,7 +545,9 @@ export function ChatBar({ // options step, and an arg option commits the full `/cmd arg` chip. Space // is slash-only (an `@` mention takes a literal space) and gated to a // non-empty query so a bare `/ ` still types a space. - const acceptOnSpace = event.key === ' ' && trigger.kind === '/' && Boolean(trigger.query.trim()) + const acceptOnSpace = + event.key === ' ' && trigger.kind === '/' && Boolean(trigger.query.trim()) && !slashFreeTextArgStage + const accept = event.key === 'Enter' || event.key === 'Tab' || acceptOnSpace if (accept) { @@ -579,11 +582,12 @@ export function ChatBar({ slashArgStage(trigger.query) && trigger.query.trim() ) { - event.preventDefault() - triggerKeyConsumedRef.current = true - commitTypedSlashDirective() + if (commitTypedSlashDirective()) { + event.preventDefault() + triggerKeyConsumedRef.current = true - return + return + } } // ArrowUp/ArrowDown navigate, in priority order: the queue (edit entries in diff --git a/apps/desktop/src/app/command-palette/index.tsx b/apps/desktop/src/app/command-palette/index.tsx index 42bcef4e820a..e86bd2415e39 100644 --- a/apps/desktop/src/app/command-palette/index.tsx +++ b/apps/desktop/src/app/command-palette/index.tsx @@ -71,6 +71,7 @@ import { COMMAND_CENTER_ROUTE, CRON_ROUTE, MESSAGING_ROUTE, + navigateToWorkspacePage, NEW_CHAT_ROUTE, PROFILES_ROUTE, sessionRoute, @@ -351,7 +352,7 @@ export function CommandPalette() { } }, [open, pendingPage]) - const go = useCallback((path: string) => () => navigate(path), [navigate]) + const go = useCallback((path: string) => () => navigateToWorkspacePage(navigate, path), [navigate]) // Step up one nested page (or back to the root list), clearing the filter so // the parent page doesn't reopen mid-search. diff --git a/apps/desktop/src/app/contrib/wiring.tsx b/apps/desktop/src/app/contrib/wiring.tsx index de2e0becffdb..0cbda7af36f1 100644 --- a/apps/desktop/src/app/contrib/wiring.tsx +++ b/apps/desktop/src/app/contrib/wiring.tsx @@ -78,10 +78,11 @@ import { closeAllTerminals } from '../right-sidebar/terminal/terminals' import { $workspaceIsPage, CRON_ROUTE, + navigateToWorkspacePage, routeSessionId, sessionRoute, SETTINGS_ROUTE, - syncWorkspaceIsPage + syncWorkspaceRoute } from '../routes' import { SessionPickerOverlay } from '../session-picker-overlay' import { SessionSwitcher } from '../session-switcher' @@ -186,11 +187,12 @@ export function ContribWiring({ children }: { children: ReactNode }) { routedSessionIdRef.current = null }, []) - // Mirror "the workspace is showing a full page" into its atom — the - // workspace pane contribution re-registers headerVeto from it, so the main - // zone's tab bar stands down on pages (and returns with the chat). + // Point the workspace at the route: the pane contribution re-registers + // headerVeto from $workspaceIsPage (so the main zone's tab bar stands down + // on pages), and a page route fronts the pane so it can't stay stuck behind + // a focused session tile. useEffect(() => { - syncWorkspaceIsPage(location.pathname) + syncWorkspaceRoute(location.pathname) }, [location.pathname]) const { @@ -1015,7 +1017,7 @@ export function ContribWiring({ children }: { children: ReactNode }) { initialSection={commandCenterInitialSection} onClose={closeOverlayToPreviousRoute} onDeleteSession={removeSession} - onNavigateRoute={path => navigate(path)} + onNavigateRoute={path => navigateToWorkspacePage(navigate, path)} onOpenSession={sessionId => navigate(sessionRoute(sessionId))} /> diff --git a/apps/desktop/src/app/hooks/use-keybinds.ts b/apps/desktop/src/app/hooks/use-keybinds.ts index 8a1fe275478e..7e49029d1cea 100644 --- a/apps/desktop/src/app/hooks/use-keybinds.ts +++ b/apps/desktop/src/app/hooks/use-keybinds.ts @@ -57,6 +57,7 @@ import { ARTIFACTS_ROUTE, CRON_ROUTE, MESSAGING_ROUTE, + navigateToWorkspacePage, PROFILES_ROUTE, sessionRoute, SETTINGS_ROUTE, @@ -138,9 +139,9 @@ export function useKeybinds(deps: KeybindRuntimeDeps): void { 'nav.commandCenter': deps.toggleCommandCenter, 'nav.settings': () => navigate(SETTINGS_ROUTE), 'nav.profiles': () => navigate(PROFILES_ROUTE), - 'nav.skills': () => navigate(SKILLS_ROUTE), - 'nav.messaging': () => navigate(MESSAGING_ROUTE), - 'nav.artifacts': () => navigate(ARTIFACTS_ROUTE), + 'nav.skills': () => navigateToWorkspacePage(navigate, SKILLS_ROUTE), + 'nav.messaging': () => navigateToWorkspacePage(navigate, MESSAGING_ROUTE), + 'nav.artifacts': () => navigateToWorkspacePage(navigate, ARTIFACTS_ROUTE), 'nav.cron': () => navigate(CRON_ROUTE), 'nav.agents': () => navigate(AGENTS_ROUTE), diff --git a/apps/desktop/src/app/routes.ts b/apps/desktop/src/app/routes.ts index b3c28a0ae017..9ee9365c3287 100644 --- a/apps/desktop/src/app/routes.ts +++ b/apps/desktop/src/app/routes.ts @@ -1,8 +1,11 @@ import { atom } from 'nanostores' import type { ReactNode } from 'react' +import { noteActiveTreeGroup, revealTreePane } from '@/components/pane-shell/tree/store' import { registry } from '@/contrib/registry' +type NavigateLike = (to: string, options?: { replace?: boolean }) => void + export const SESSION_ROUTE_PREFIX = '/' export const NEW_CHAT_ROUTE = '/' export const SETTINGS_ROUTE = '/settings' @@ -133,16 +136,30 @@ export function isOverlayView(view: AppView): boolean { return OVERLAY_VIEWS.has(view) } +/** The pathname of a router target. Every classifier below reasons about a + * PATH, but callers navigate to full targets (`/skills?tab=mcp`), and an + * unstripped query reaches the session-id parser — `/skills?tab=mcp` reads as + * the session `skills?tab=mcp`, so Capabilities classifies as a chat. + * `sessionRoute` percent-encodes ids, so `?`/`#` can only start a query or a + * hash. */ +export function routePathname(to: string): string { + const cut = to.search(/[?#]/) + + return cut === -1 ? to : to.slice(0, cut) +} + export function isNewChatRoute(pathname: string): boolean { - return pathname === NEW_CHAT_ROUTE + return routePathname(pathname) === NEW_CHAT_ROUTE } export function routeSessionId(pathname: string): string | null { - if (!pathname.startsWith(SESSION_ROUTE_PREFIX) || RESERVED_PATHS.has(pathname) || isContributedPath(pathname)) { + const path = routePathname(pathname) + + if (!path.startsWith(SESSION_ROUTE_PREFIX) || RESERVED_PATHS.has(path) || isContributedPath(path)) { return null } - const id = pathname.slice(SESSION_ROUTE_PREFIX.length) + const id = path.slice(SESSION_ROUTE_PREFIX.length) return id && !id.includes('/') ? decodeURIComponent(id) : null } @@ -169,15 +186,26 @@ export function sessionRoute(sessionId: string): string { } export function appViewForPath(pathname: string): AppView { - if (isNewChatRoute(pathname) || routeSessionId(pathname)) { + const path = routePathname(pathname) + + if (isNewChatRoute(path) || routeSessionId(path)) { return 'chat' } - if (isContributedPath(pathname)) { + if (isContributedPath(path)) { return 'extension' } - return APP_VIEW_BY_PATH.get(pathname) ?? 'chat' + return APP_VIEW_BY_PATH.get(path) ?? 'chat' +} + +/** Does `to` land on a full page rendered INSIDE the workspace pane + * (skills/messaging/artifacts/contributed routes)? Overlays don't count — + * they float over whatever the workspace is already showing. */ +function isWorkspacePageRoute(to: string): boolean { + const view = appViewForPath(to) + + return view !== 'chat' && !isOverlayView(view) } /** True while the workspace pane shows a FULL PAGE (skills/messaging/ @@ -187,11 +215,49 @@ export function appViewForPath(pathname: string): AppView { * (settings/…) don't count — the chat stays beneath them. */ export const $workspaceIsPage = atom(false) -export function syncWorkspaceIsPage(pathname: string): void { - const view = appViewForPath(pathname) - const isPage = view !== 'chat' && !isOverlayView(view) +function revealWorkspacePane(): void { + noteActiveTreeGroup(null) + revealTreePane('workspace') +} + +/** + * Point the workspace at `pathname`: mirror "showing a full page" into + * `$workspaceIsPage`, and FRONT the pane when it is one. + * + * A page renders inside `workspace`, so a main zone parked on a session tile + * keeps the tile on screen while the route and the page content change behind + * it — the navigation looks dead (#72602). Session switches already front the + * pane in `store/session-states.ts`; pages had no equivalent. + * + * The router location drives this, so every entry point gets it without opting + * in: sidebar, keybinds, command palette, Command Center, contributed + * statusbar/titlebar `to` targets, back/forward, and cold-start restore. + */ +export function syncWorkspaceRoute(pathname: string): void { + const isPage = isWorkspacePageRoute(pathname) if (isPage !== $workspaceIsPage.get()) { $workspaceIsPage.set(isPage) } + + if (isPage) { + revealWorkspacePane() + } +} + +/** + * Navigate to `to`, fronting the workspace pane when it is a page route. + * + * `syncWorkspaceRoute` covers route CHANGES; this covers the RE-CLICK, the one + * case it can't see — hitting Capabilities while already on `/skills` with a + * tile focused leaves the location untouched, so no effect fires and only an + * imperative reveal brings the page back. Use it wherever a nav affordance can + * be triggered from the page it targets. + */ +export function navigateToWorkspacePage(navigate: NavigateLike, to: string, options?: { replace?: boolean }): void { + navigate(to, options) + + if (isWorkspacePageRoute(to)) { + revealWorkspacePane() + } } diff --git a/apps/desktop/src/app/routes.workspace-reveal.test.ts b/apps/desktop/src/app/routes.workspace-reveal.test.ts new file mode 100644 index 000000000000..7db75e610aed --- /dev/null +++ b/apps/desktop/src/app/routes.workspace-reveal.test.ts @@ -0,0 +1,189 @@ +/** + * A full page (Capabilities/Messaging/Artifacts/a contributed route) renders + * INSIDE the `workspace` pane, so navigating to one has to front that pane — + * otherwise a main zone parked on a session tile keeps the tile on screen and + * the click looks dead until the app restarts (#72602). + * + * Two layers, both covered here: `syncWorkspaceRoute` (the router location, + * every entry point) and `navigateToWorkspacePage` (the re-click, where the + * location doesn't change). + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { registry } from '@/contrib/registry' + +import { + $workspaceIsPage, + AGENTS_ROUTE, + appViewForPath, + ARTIFACTS_ROUTE, + CRON_ROUTE, + MESSAGING_ROUTE, + navigateToWorkspacePage, + NEW_CHAT_ROUTE, + routePathname, + ROUTES_AREA, + routeSessionId, + sessionRoute, + SETTINGS_ROUTE, + SKILLS_ROUTE, + syncWorkspaceRoute +} from './routes' + +vi.mock('@/components/pane-shell/tree/store', async importOriginal => ({ + ...(await importOriginal>()), + noteActiveTreeGroup: vi.fn(), + revealTreePane: vi.fn() +})) + +const { noteActiveTreeGroup, revealTreePane } = await import('@/components/pane-shell/tree/store') + +const CONTRIBUTED_ROUTE = '/kanban' + +function contributeRoute(): () => void { + return registry.register({ + area: ROUTES_AREA, + data: { path: CONTRIBUTED_ROUTE }, + id: 'test-route', + render: () => null + }) +} + +/** Did the workspace pane get fronted? Both calls, or the tab stays put. */ +const fronted = () => + vi.mocked(revealTreePane).mock.calls.some(([pane]) => pane === 'workspace') && + vi.mocked(noteActiveTreeGroup).mock.calls.some(([group]) => group === null) + +beforeEach(() => { + vi.mocked(revealTreePane).mockClear() + vi.mocked(noteActiveTreeGroup).mockClear() + $workspaceIsPage.set(false) +}) + +afterEach(() => { + $workspaceIsPage.set(false) +}) + +describe('routePathname', () => { + it('keeps a bare path and drops a query or hash', () => { + expect(routePathname(SKILLS_ROUTE)).toBe('/skills') + expect(routePathname('/skills?tab=mcp')).toBe('/skills') + expect(routePathname('/skills?tab=mcp&server=ctx7')).toBe('/skills') + expect(routePathname('/settings#keys')).toBe('/settings') + }) + + it('leaves an encoded session id alone', () => { + const route = sessionRoute('a?b#c') + + expect(routePathname(route)).toBe(route) + expect(routeSessionId(route)).toBe('a?b#c') + }) +}) + +describe('classification of targets carrying a query', () => { + // The palette navigates to every one of these (Capabilities tabs, MCP + // servers), and Settings redirects old /settings?tab=mcp deep links to the + // last one. Unstripped, they parsed as SESSION ids and read as 'chat'. + it.each([ + [`${SKILLS_ROUTE}?tab=skills`, 'skills'], + [`${SKILLS_ROUTE}?tab=toolsets`, 'skills'], + [`${SKILLS_ROUTE}?tab=mcp&server=ctx7`, 'skills'], + [`${SETTINGS_ROUTE}?tab=keys`, 'settings'] + ])('%s is not a session route', (to, view) => { + expect(routeSessionId(to)).toBeNull() + expect(appViewForPath(to)).toBe(view) + }) +}) + +describe('syncWorkspaceRoute', () => { + it('publishes and fronts on a page route', () => { + syncWorkspaceRoute(SKILLS_ROUTE) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts on a page route reached with a query', () => { + syncWorkspaceRoute(`${SKILLS_ROUTE}?tab=mcp`) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts when moving between two pages — the atom never changes, the tab must', () => { + syncWorkspaceRoute(ARTIFACTS_ROUTE) + vi.mocked(revealTreePane).mockClear() + vi.mocked(noteActiveTreeGroup).mockClear() + + syncWorkspaceRoute(MESSAGING_ROUTE) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts on a contributed page route', () => { + const dispose = contributeRoute() + + try { + syncWorkspaceRoute(CONTRIBUTED_ROUTE) + + expect(appViewForPath(CONTRIBUTED_ROUTE)).toBe('extension') + expect(fronted()).toBe(true) + } finally { + dispose() + } + }) + + it.each([ + ['a session route', sessionRoute('sess-a')], + ['the new-chat route', NEW_CHAT_ROUTE], + ['an overlay', SETTINGS_ROUTE], + ['an overlay with a query', `${SETTINGS_ROUTE}?tab=keys`], + ['another overlay', CRON_ROUTE], + ['yet another overlay', AGENTS_ROUTE] + ])('leaves the tab alone on %s', (_label, to) => { + syncWorkspaceRoute(to) + + expect($workspaceIsPage.get()).toBe(false) + expect(revealTreePane).not.toHaveBeenCalled() + }) +}) + +describe('navigateToWorkspacePage', () => { + it('navigates and fronts, so a re-click on the page you are already on still shows it', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, SKILLS_ROUTE) + + expect(navigate).toHaveBeenCalledWith(SKILLS_ROUTE, undefined) + expect(fronted()).toBe(true) + }) + + it.each([`${SKILLS_ROUTE}?tab=skills`, `${SKILLS_ROUTE}?tab=toolsets`, `${SKILLS_ROUTE}?tab=mcp&server=ctx7`])( + 'fronts for the palette target %s', + to => { + navigateToWorkspacePage(vi.fn(), to) + + expect(fronted()).toBe(true) + } + ) + + it('passes navigation options through', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, ARTIFACTS_ROUTE, { replace: true }) + + expect(navigate).toHaveBeenCalledWith(ARTIFACTS_ROUTE, { replace: true }) + }) + + it('navigates without fronting for chat and overlay targets', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, sessionRoute('sess-a')) + navigateToWorkspacePage(navigate, SETTINGS_ROUTE) + + expect(navigate).toHaveBeenCalledTimes(2) + expect(revealTreePane).not.toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx index 531937b64b83..31882d91b66d 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx @@ -3,6 +3,7 @@ import type { MutableRefObject } from 'react' import { useEffect } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' +import { noteActiveTreeGroup, revealTreePane } from '@/components/pane-shell/tree/store' import { getSession, getSessionMessages, type SessionInfo } from '@/hermes' import { createClientSessionState } from '@/lib/chat-runtime' import { clearSessionDraft, stashSessionDraft, takeSessionDraft } from '@/store/composer' @@ -55,6 +56,12 @@ vi.mock('@/store/profile', async importOriginal => ({ ensureGatewayProfile: vi.fn().mockResolvedValue(undefined) })) +vi.mock('@/components/pane-shell/tree/store', async importOriginal => ({ + ...(await importOriginal>()), + noteActiveTreeGroup: vi.fn(), + revealTreePane: vi.fn() +})) + const RUNTIME_SESSION_ID = 'rt-new-001' function deferred() { @@ -69,7 +76,7 @@ function deferred() { type HarnessHandle = Pick< ReturnType, - 'createBackendSessionForSend' | 'startFreshSessionDraft' + 'createBackendSessionForSend' | 'selectSidebarItem' | 'startFreshSessionDraft' > function storedSession(overrides: Partial = {}): SessionInfo { @@ -1590,3 +1597,21 @@ describe('createBackendSessionForSend workspace target', () => { expect(params).toMatchObject({ cwd: '/clicked-workspace' }) }) }) +describe('selectSidebarItem', () => { + it('fronts the workspace pane when navigating to a sidebar route (issue #72602)', async () => { + const navigate = vi.fn() + const requestGateway = vi.fn(async () => ({}) as never) + let handle: HarnessHandle | null = null + + render( (handle = value)} requestGateway={requestGateway} />) + await waitFor(() => expect(handle).not.toBeNull()) + + act(() => { + handle!.selectSidebarItem({ icon: (() => null) as never, id: 'skills', label: 'Capabilities', route: '/skills' }) + }) + + expect(navigate).toHaveBeenCalledWith('/skills', undefined) + expect(noteActiveTreeGroup).toHaveBeenCalledWith(null) + expect(revealTreePane).toHaveBeenCalledWith('workspace') + }) +}) diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts index fd490666d126..5d293c012b21 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts @@ -68,7 +68,7 @@ import { broadcastSessionsChanged } from '@/store/session-sync' import { isWatchWindow } from '@/store/windows' import type { SessionCreateResponse, SessionMessage, SessionResumeResponse, UsageStats } from '@/types/hermes' -import { NEW_CHAT_ROUTE, sessionRoute, SETTINGS_ROUTE } from '../../../routes' +import { navigateToWorkspacePage, NEW_CHAT_ROUTE, sessionRoute, SETTINGS_ROUTE } from '../../../routes' import type { ClientSessionState, SidebarNavItem } from '../../../types' import { sessionContextDrift } from '../session-context-drift' @@ -458,7 +458,7 @@ export function useSessionActions({ } if (item.route) { - navigate(item.route) + navigateToWorkspacePage(navigate, item.route) } }, [navigate, startFreshSessionDraft] diff --git a/apps/desktop/src/lib/desktop-slash-commands.test.ts b/apps/desktop/src/lib/desktop-slash-commands.test.ts index d8aa1897652d..1fc70dcb5ad5 100644 --- a/apps/desktop/src/lib/desktop-slash-commands.test.ts +++ b/apps/desktop/src/lib/desktop-slash-commands.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { desktopSkinSlashCompletions, + desktopSlashCommandArgumentMode, desktopSlashDescription, desktopSlashUnavailableMessage, filterDesktopCommandsCatalog, @@ -65,7 +66,7 @@ describe('desktop slash command curation', () => { it('routes /pet through the desktop action handler and drops /pets', () => { expect(resolveDesktopCommand('/pet')?.surface).toEqual({ kind: 'action', action: 'pet' }) - expect(resolveDesktopCommand('/pet')?.args).toBe(true) + expect(desktopSlashCommandArgumentMode('/pet')).toBe('options') expect(isDesktopSlashSuggestion('/pet')).toBe(true) expect(isDesktopSlashCommand('/pet')).toBe(true) expect(resolveDesktopCommand('/pets')?.surface).toEqual({ kind: 'unavailable', reason: 'settings' }) @@ -81,14 +82,14 @@ describe('desktop slash command curation', () => { expect(desktopSlashUnavailableMessage('/browser')).toBeNull() expect(resolveDesktopCommand('/browser')?.surface).toEqual({ kind: 'action', action: 'browser' }) // Bare /browser expands to its sub-action options in the popover. - expect(resolveDesktopCommand('/browser')?.args).toBe(true) + expect(desktopSlashCommandArgumentMode('/browser')).toBe('options') }) it('routes /compress through the session-compression action', () => { // /compress must be an action (session.compress RPC), not exec: the slash // worker route times out on large sessions (#44456). expect(resolveDesktopCommand('/compress')?.surface).toEqual({ kind: 'action', action: 'compress' }) - expect(resolveDesktopCommand('/compress')?.args).toBe(true) + expect(desktopSlashCommandArgumentMode('/compress')).toBe('text') expect(isDesktopSlashCommand('/compress')).toBe(true) expect(isDesktopSlashSuggestion('/compress')).toBe(true) expect(desktopSlashUnavailableMessage('/compress')).toBeNull() @@ -144,12 +145,13 @@ describe('desktop slash command curation', () => { } }) - it('keeps /goal arg text editable instead of sealing it into a chip', () => { - // /goal takes free prose (the goal itself) plus subcommands. Without - // args:true, Space after the command name committed a sealed directive - // chip and the goal text rendered awkwardly after a pill. - expect(resolveDesktopCommand('/goal')?.surface).toEqual({ kind: 'exec' }) - expect(resolveDesktopCommand('/goal')?.args).toBe(true) + it('distinguishes free prose from finite slash option lists', () => { + expect(desktopSlashCommandArgumentMode('/goal')).toBe('mixed') + expect(desktopSlashCommandArgumentMode('/steer')).toBe('text') + expect(desktopSlashCommandArgumentMode('/queue')).toBe('text') + expect(desktopSlashCommandArgumentMode('/personality')).toBe('options') + expect(desktopSlashCommandArgumentMode('/handoff')).toBe('options') + expect(desktopSlashCommandArgumentMode('/version')).toBeNull() }) it('routes /journey (and aliases) to the memory graph overlay action', () => { diff --git a/apps/desktop/src/lib/desktop-slash-commands.ts b/apps/desktop/src/lib/desktop-slash-commands.ts index 5fccef48c719..81e4a694c015 100644 --- a/apps/desktop/src/lib/desktop-slash-commands.ts +++ b/apps/desktop/src/lib/desktop-slash-commands.ts @@ -91,6 +91,16 @@ export interface SlashCommandBuildCtx { sessionId: string } +/** + * How arguments behave in the Desktop composer. + * + * - `options` → a finite completion list; picking or fully typing an option may + * commit the complete directive as a chip. + * - `text` → arbitrary prose; the command and its argument stay editable. + * - `mixed` → offers subcommand completions but also accepts arbitrary prose. + */ +export type DesktopSlashArgumentMode = 'mixed' | 'options' | 'text' + export interface DesktopCommandSpec { /** Canonical command, leading slash included (e.g. `/resume`). */ name: string @@ -104,12 +114,8 @@ export interface DesktopCommandSpec { * the status bar), so the popover doesn't dead-end on inline completion. */ hidden?: boolean - /** - * The command has an inline options "screen" (theme / personality / session / - * platform / toolset list). Picking the bare command in the popover expands to - * that argument step instead of committing — mirroring typing `/ ` by hand. - */ - args?: boolean + /** Composer behavior for text following the command token. */ + argumentMode?: DesktopSlashArgumentMode } const exec = (): DesktopCommandSurface => ({ kind: 'exec' }) @@ -151,17 +157,22 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ name: '/handoff', description: 'Hand off this session to a messaging platform', surface: action('handoff'), - args: true + argumentMode: 'options' }, { name: '/profile', description: 'Switch the active Hermes profile', surface: action('profile') }, - { name: '/skin', description: 'Switch desktop theme or cycle to the next one', surface: action('skin'), args: true }, - { name: '/title', description: 'Rename the current session', surface: action('title') }, + { + name: '/skin', + description: 'Switch desktop theme or cycle to the next one', + surface: action('skin'), + argumentMode: 'options' + }, + { name: '/title', description: 'Rename the current session', surface: action('title'), argumentMode: 'text' }, { name: '/help', description: 'Show desktop slash commands', aliases: ['/commands'], surface: action('help') }, { name: '/browser', description: 'Manage browser CDP connection [connect|disconnect|status] (local gateway only)', surface: action('browser'), - args: true + argumentMode: 'options' }, { name: '/journey', @@ -177,7 +188,7 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ description: 'Resume a saved session', aliases: ['/sessions', '/switch'], surface: picker('session'), - args: true + argumentMode: 'options' }, // Backend-executed commands that render useful inline output. @@ -194,7 +205,7 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ name: '/approvals', description: 'Show or set approval mode [manual|smart|off]', surface: exec(), - args: true + argumentMode: 'options' }, { name: '/agents', @@ -202,7 +213,13 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ aliases: ['/tasks'], surface: exec() }, - { name: '/background', description: 'Run a prompt in the background', aliases: ['/bg', '/btw'], surface: exec() }, + { + name: '/background', + description: 'Run a prompt in the background', + aliases: ['/bg', '/btw'], + surface: exec(), + argumentMode: 'text' + }, // /compress must be an action (session.compress RPC), not exec: the slash // worker route times out on large sessions (30s WS / 45s pipe) before the // LLM summarise call finishes, then command.dispatch surfaces a bogus @@ -212,16 +229,26 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ description: 'Compress this conversation context', aliases: ['/compact'], surface: action('compress'), - args: true + argumentMode: 'text' }, { name: '/debug', description: 'Create a debug report', surface: exec() }, - { name: '/goal', description: 'Manage the standing goal for this session', surface: exec(), args: true }, - { name: '/personality', description: 'Switch personality for this session', surface: exec(), args: true }, + { + name: '/goal', + description: 'Manage the standing goal for this session', + surface: exec(), + argumentMode: 'mixed' + }, + { + name: '/personality', + description: 'Switch personality for this session', + surface: exec(), + argumentMode: 'options' + }, { name: '/pet', description: 'Toggle or adopt a petdex mascot (/pet, /pet list, /pet boba)', surface: action('pet'), - args: true + argumentMode: 'options' }, { name: '/hatch', @@ -229,7 +256,13 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ aliases: ['/generate-pet'], surface: action('hatch') }, - { name: '/queue', description: 'Queue a prompt for the next turn', aliases: ['/q'], surface: exec() }, + { + name: '/queue', + description: 'Queue a prompt for the next turn', + aliases: ['/q'], + surface: exec(), + argumentMode: 'text' + }, { name: '/retry', description: 'Retry the last user message', surface: exec() }, { name: '/rollback', description: 'List or restore filesystem checkpoints', surface: exec() }, { @@ -242,9 +275,19 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ description: 'Show current session status', surface: rpc('session.status', ctx => ({ session_id: ctx.sessionId })) }, - { name: '/steer', description: 'Steer the current run after the next tool call', surface: exec(), args: true }, + { + name: '/steer', + description: 'Steer the current run after the next tool call', + surface: exec(), + argumentMode: 'text' + }, { name: '/stop', description: 'Stop running background processes', surface: exec() }, - { name: '/tools', description: 'List or toggle tools available to the agent', surface: exec(), args: true }, + { + name: '/tools', + description: 'List or toggle tools available to the agent', + surface: exec(), + argumentMode: 'options' + }, { name: '/undo', description: 'Remove the last user/assistant exchange', surface: exec() }, { name: '/usage', description: 'Show token usage for this session', surface: exec() }, { name: '/version', description: 'Show Hermes Agent version', surface: exec() }, @@ -433,13 +476,8 @@ export function desktopSlashDescription(command: string, fallback = ''): string return SPEC_BY_NAME.get(canonicalDesktopSlashCommand(command))?.description || fallback } -/** - * True when picking the bare command should expand to its inline argument - * options (theme / personality / session / platform / toolset) rather than - * committing immediately. Lets the popover act as a two-step picker. - */ -export function desktopSlashCommandTakesArgs(command: string): boolean { - return resolveDesktopCommand(command)?.args ?? false +export function desktopSlashCommandArgumentMode(command: string): DesktopSlashArgumentMode | null { + return resolveDesktopCommand(command)?.argumentMode ?? null } export function desktopSkinSlashCompletions( diff --git a/contributors/emails/elco@thedaoist.gg b/contributors/emails/elco@thedaoist.gg new file mode 100644 index 000000000000..a2a41fca3807 --- /dev/null +++ b/contributors/emails/elco@thedaoist.gg @@ -0,0 +1 @@ +elcocoel diff --git a/contributors/emails/gercamjr.dev@gmail.com b/contributors/emails/gercamjr.dev@gmail.com new file mode 100644 index 000000000000..6ac577a3590c --- /dev/null +++ b/contributors/emails/gercamjr.dev@gmail.com @@ -0,0 +1,2 @@ +gercamjr +# PR #68945 salvage (update: recover the web UI build when npm leaves no tsc/vite) diff --git a/contributors/emails/reinbeumer@gmail.com b/contributors/emails/reinbeumer@gmail.com new file mode 100644 index 000000000000..26c0b64b9500 --- /dev/null +++ b/contributors/emails/reinbeumer@gmail.com @@ -0,0 +1 @@ +reinbeumer diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 3f3d5680647d..2ccfe808d4fe 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -4760,6 +4760,108 @@ def _clear_bytecode_cache(root: Path) -> int: return removed +_UPDATE_RUNTIME_RELOAD_MODULES = ( + "hermes_constants", + "tools.environments.local", + "tools.lazy_deps", +) + + +def _reload_updated_runtime_modules() -> None: + """Reload update-sensitive modules after the checkout changes in-place. + + ``hermes update`` keeps running in the pre-pull Python process. After a + large update, modules already present in ``sys.modules`` can still expose + old symbols even though their source files on disk are new. Refresh the + small module set used by lazy-backend refresh before that step imports + newly-updated code paths. + """ + try: + import importlib + + importlib.invalidate_caches() + for module_name in _UPDATE_RUNTIME_RELOAD_MODULES: + module = sys.modules.get(module_name) + if module is None: + continue + try: + importlib.reload(module) + except Exception as exc: + logger.debug("Could not reload updated module %s: %s", module_name, exc) + except Exception as exc: + logger.debug("Could not refresh update runtime modules: %s", exc) + + +# Stamp file recording the checkout fingerprint the bytecode cache was last +# validated against. Lives next to the checkout (NOT in HERMES_HOME) because +# __pycache__ is per-checkout state shared by every profile. +_BYTECODE_FINGERPRINT_FILE = ".bytecode-fingerprint" + + +def _record_bytecode_fingerprint() -> None: + """Persist the current checkout fingerprint after a bytecode sweep. + + Never raises. A failed write just means the next launch re-sweeps — + safe, merely redundant. + """ + try: + fingerprint = _read_git_revision_fingerprint(PROJECT_ROOT) + if not fingerprint: + return + stamp_path = PROJECT_ROOT / _BYTECODE_FINGERPRINT_FILE + tmp_path = stamp_path.with_name(stamp_path.name + ".tmp") + tmp_path.write_text(fingerprint, encoding="utf-8") + tmp_path.replace(stamp_path) + except OSError as exc: + logger.debug("Could not record bytecode fingerprint: %s", exc) + + +def _sweep_stale_bytecode_if_checkout_changed() -> None: + """Clear ``__pycache__`` at launch when the checkout changed underneath us. + + The stale-bytecode bug class (issues #6207, #60242; Dhruv's WhatsApp + ``cannot import name 'parse_model_flags_detailed'`` report) has one + shared shape: the checkout's ``.py`` files change (git pull inside + ``hermes update``, a manual ``git pull``, a ZIP update, a file-sync + restore) while ``__pycache__`` retains bytecode from the previous + revision, and a later process trusts the stale ``.pyc`` instead of the + fresh source. + + Update-time clears alone can never close this class: ``hermes update`` + always executes the PRE-pull updater code, so any hardening added to it + only takes effect one update late, and manual ``git pull`` never runs + the updater at all. This launch-time guard closes the loop: every + ``hermes`` entry point compares the checkout fingerprint (cheap file + reads, no git subprocess) against the last-validated stamp and sweeps + the bytecode cache once when they diverge. + + Never raises — a failure here must not block launch. + """ + try: + fingerprint = _read_git_revision_fingerprint(PROJECT_ROOT) + if not fingerprint: + return # non-git install — the ZIP update path clears explicitly + stamp_path = PROJECT_ROOT / _BYTECODE_FINGERPRINT_FILE + try: + recorded = stamp_path.read_text(encoding="utf-8").strip() + except OSError: + recorded = "" + if recorded == fingerprint: + return + removed = _clear_bytecode_cache(PROJECT_ROOT) + if removed: + logger.info( + "Checkout changed since last launch (%s -> %s): cleared %d stale __pycache__ director%s", + recorded or "unknown", + fingerprint, + removed, + "y" if removed == 1 else "ies", + ) + _record_bytecode_fingerprint() + except Exception as exc: + logger.debug("Stale-bytecode launch sweep failed: %s", exc) + + # Critical files that Hermes must be able to import immediately after an # update/install. Most are imported on every CLI startup; ``web_server.py`` # is the desktop/dashboard backend path that a fresh Windows install launches @@ -5226,6 +5328,62 @@ def _run_npm_install_deterministic( ) +def _npm_bin_exists(bin_dir: Path, name: str) -> bool: + """True when an npm bin shim for *name* exists (POSIX or Windows).""" + return any( + (bin_dir / candidate).exists() + for candidate in (name, f"{name}.cmd", f"{name}.ps1", f"{name}.exe") + ) + + +def _web_build_toolchain_ready(*roots: Path) -> bool: + """True when ``tsc`` and ``vite`` shims are reachable from any of *roots*. + + Callers must pass every root the build would search; checking only one + reports a healthy tree as broken. + """ + bin_dirs = [ + bin_dir + for bin_dir in (root / "node_modules" / ".bin" for root in roots) + if bin_dir.is_dir() + ] + return bool(bin_dirs) and all( + any(_npm_bin_exists(bin_dir, tool) for bin_dir in bin_dirs) + for tool in ("tsc", "vite") + ) + + +def _web_toolchain_roots(web_dir: Path) -> tuple[Path, ...]: + """Roots whose ``node_modules/.bin`` can satisfy the web build. + + ``npm run build`` prepends ``node_modules/.bin`` for the package and each + of its ancestors, so shims hoisted to the workspace root and shims nested + under a package that owns its lockfile (#42973) are equally valid. + """ + return (web_dir, web_dir.parent) + + +def _missing_web_build_tool(output: str) -> str | None: + """Return the build tool a failed ``npm run build`` could not resolve. + + Each shell words this differently: ``sh: 1: tsc: not found`` (dash), + ``vite: command not found`` (bash/zsh), and ``'tsc' is not recognized as + an internal or external command`` (cmd.exe). + """ + lowered = output.lower() + for tool in ("tsc", "vite"): + if any( + phrase in lowered + for phrase in ( + f"{tool}: not found", + f"{tool}: command not found", + f"'{tool}' is not recognized", + ) + ): + return tool + return None + + def _build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: """Build the web UI frontend if npm is available, serializing across processes. @@ -5333,12 +5491,16 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: npm_workspace_args: tuple[str, ...] = () if npm_cwd == web_dir else ("--workspace", "web") if _is_termux_startup_environment(): npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir) - r1 = _run_npm_install_deterministic( - npm, - npm_cwd, - extra_args=(*npm_workspace_args, "--silent"), - env=build_env, - ) + + def _install_web_deps(*, silent: bool) -> "subprocess.CompletedProcess": + return _run_npm_install_deterministic( + npm, + npm_cwd, + extra_args=(*npm_workspace_args, "--silent") if silent else npm_workspace_args, + env=build_env, + ) + + r1 = _install_web_deps(silent=True) if r1.returncode != 0: _say( f" {'✗' if fatal else '⚠'} Web UI npm install failed" @@ -5355,11 +5517,22 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: # recoverable (the stale-dist fallback below handles the kill path). r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: - # Retry once after a short delay — covers boot-time races on Windows - # (antivirus scanning Node.js binaries, npm cache not ready, transient - # I/O when launched via Scheduled Task at logon). See issue #23817. - _time.sleep(3) - r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + # The install above can exit 0 while leaving the tree without a build + # toolchain — a lockfile-hash skip over a half-installed tree, or an + # interrupted link step. The generic retry below just reruns the same + # command, so `tsc: not found` survives it and the stale dist is + # served forever. Reinstall (non-silent, so the user sees it) first. + missing_tool = _missing_web_build_tool((r2.stdout or "") + (r2.stderr or "")) + if missing_tool: + _say(f" ⚠ Build could not resolve {missing_tool} — reinstalling web dependencies...") + _install_web_deps(silent=False) + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + if r2.returncode != 0: + # Retry once after a short delay — covers boot-time races on Windows + # (antivirus scanning Node.js binaries, npm cache not ready, transient + # I/O when launched via Scheduled Task at logon). See issue #23817. + _time.sleep(3) + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: # _run_with_idle_timeout merges stderr into stdout; older callers @@ -7707,6 +7880,7 @@ def _update_via_zip(args): print( f" ✓ Cleared {removed} stale __pycache__ director{'y' if removed == 1 else 'ies'}" ) + _record_bytecode_fingerprint() # Reinstall Python dependencies. Prefer .[all], but if one optional extra # breaks on this machine, keep base deps and reinstall the remaining extras @@ -10140,6 +10314,15 @@ def _npm_lockfile_changed(hermes_root: Path) -> bool: # node_modules means the cache was recorded by another checkout. if not (PROJECT_ROOT / "node_modules").is_dir(): return True + # A matching lockfile hash over a tree whose web build toolchain never + # landed must NOT skip the reinstall — otherwise every later `hermes + # update` keeps rebuilding against a half-installed tree and serving a + # stale dist. + web_dir = PROJECT_ROOT / "web" + if (web_dir / "package.json").is_file() and not _web_build_toolchain_ready( + *_web_toolchain_roots(web_dir) + ): + return True try: # Key the cache by PROJECT_ROOT so parallel worktrees don't collide. cache_key = hashlib.sha256(str(PROJECT_ROOT).encode()).hexdigest()[:12] @@ -12196,6 +12379,7 @@ def _cmd_update_impl(args, gateway_mode: bool): print( f" ✓ Cleared {removed} stale __pycache__ director{'y' if removed == 1 else 'ies'}" ) + _record_bytecode_fingerprint() # Fork upstream sync logic (only for main branch on forks) if is_fork and branch == "main": @@ -12273,6 +12457,20 @@ def _cmd_update_impl(args, gateway_mode: bool): # based on a narrow 7-package import probe (#58004 review). _clear_update_incomplete_marker() + # The update process is still the old Python interpreter process. Run + # one final cache/module refresh immediately before lazy backend + # refresh, which imports newly-pulled modules that may depend on fresh + # symbols in hermes_constants or lazy_deps. The dependency install + # above may also have regenerated bytecode from build-cache copies — + # this second sweep catches those stragglers (#60242, #65240). + removed = _clear_bytecode_cache(PROJECT_ROOT) + if removed: + print( + f" ✓ Cleared {removed} stale __pycache__ director{'y' if removed == 1 else 'ies'}" + ) + _record_bytecode_fingerprint() + _reload_updated_runtime_modules() + # Upgrade pip before lazy refreshes — stale pip can fail source builds # and leave partially-written packages (#57828). _write_lazy_refresh_incomplete_marker() @@ -12431,18 +12629,6 @@ def _cmd_update_impl(args, gateway_mode: bool): except Exception as e: logger.debug("Model catalog seed during update failed: %s", e) - # After git pull, source files on disk are newer than cached Python - # modules in this process. Reload hermes_constants so that any lazy - # import executed below (skills sync, gateway restart) sees new - # attributes like display_hermes_home() added since the last release. - try: - import importlib - import hermes_constants as _hc - - importlib.reload(_hc) - except Exception: - pass # non-fatal — worst case a lazy import fails gracefully - # Sync bundled skills (copies new, updates changed, respects user deletions) try: from tools.skills_sync import sync_skills @@ -15474,6 +15660,12 @@ def main(): except Exception: pass + # If the checkout changed since the last launch (hermes update, manual + # git pull, old-updater update that predates newer clears), sweep stale + # __pycache__ once so no process — this one's lazy imports included — + # resolves fresh source against old bytecode. Never raises. + _sweep_stale_bytecode_if_checkout_changed() + # Self-heal a venv left half-built by an interrupted ``hermes update`` # (Ctrl-C, terminal close, WSL OOM mid-install). Skip when the user is # *running* update — that flow writes and clears its own marker, and we diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index 07f2f3198a9b..8bc1edd9d05e 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -660,6 +660,11 @@ def cmd_update(name: str) -> None: console.print(f"[red]Error:[/red] {output}") sys.exit(1) + # Same stale-bytecode class as the main checkout (#6207/#60242): the + # pull just changed .py files under this plugin dir, so drop any + # __pycache__ compiled from the previous revision. + _clear_plugin_bytecode(target) + # Copy any new .example files _copy_example_files(target, console) @@ -1954,6 +1959,10 @@ def dashboard_update_user_plugin(name: str) -> dict[str, Any]: if not ok: return {"ok": False, "error": msg} + # Sibling of the CLI ``hermes plugins update`` path: drop bytecode + # compiled from the pre-pull plugin revision. + _clear_plugin_bytecode(target) + from rich.console import Console _copy_example_files(target, Console()) @@ -1961,6 +1970,30 @@ def dashboard_update_user_plugin(name: str) -> dict[str, Any]: return {"ok": True, "name": name, "output": msg, "unchanged": unchanged} +def _clear_plugin_bytecode(target: Path) -> int: + """Remove ``__pycache__`` dirs under a just-updated plugin checkout. + + Plugin dirs live outside the main repo, so the launch-time checkout + fingerprint sweep in ``hermes_cli.main`` never covers them. After a + ``git pull`` changes a plugin's ``.py`` files, stale bytecode here can + produce the same ImportError class as #6207/#60242 in whichever + process imports the plugin next. Never raises. + """ + removed = 0 + try: + for cache_dir in target.rglob("__pycache__"): + if not cache_dir.is_dir(): + continue + try: + shutil.rmtree(cache_dir) + removed += 1 + except OSError: + pass + except OSError: + pass + return removed + + def _git_pull_plugin_dir(target: Path) -> tuple[bool, str]: git_exe = _resolve_git_executable() if not git_exe: diff --git a/run_agent.py b/run_agent.py index 12654a443c7c..bef4bf7f85aa 100644 --- a/run_agent.py +++ b/run_agent.py @@ -1918,9 +1918,9 @@ class AIAgent: # where the next live turn re-reads it as an instruction and the agent # "becomes" the curator. Hard-stop before any DB touch. if getattr(self, "_persist_disabled", False): - return + return None if not self._session_db: - return + return None # Persist user-message override (#48677 chokepoint): historically this # mutated the live `messages` list in place, which — on the early # crash-resilience persist that runs BEFORE the API call is built — @@ -2122,8 +2122,10 @@ class AIAgent: # allocated next turn at a recycled address. self._flushed_db_message_ids = set() self._last_flushed_db_idx = len(messages) + return True except Exception as e: logger.warning("Session DB append_message failed: %s", e) + return False def _get_messages_up_to_last_assistant(self, messages: List[Dict]) -> List[Dict]: """ @@ -3442,6 +3444,14 @@ class AIAgent: "the model produced no follow-up text. Send `continue` to " "let it summarize." ) + if reason == "session_persistence_failed": + return ( + prefix + + "the turn was stopped because session storage could not be " + "written (the transcript would have been lost on restart). " + "Check disk space / permissions for the state DB, then send " + "your message again." + ) # Unknown/diagnostic-only reasons (e.g. "unknown", guardrail_halt # which already surfaces its own message) — don't second-guess. return "" diff --git a/tests/agent/test_reasoning_stale_timeout_floor.py b/tests/agent/test_reasoning_stale_timeout_floor.py index ec05cb54dd60..608c83d10408 100644 --- a/tests/agent/test_reasoning_stale_timeout_floor.py +++ b/tests/agent/test_reasoning_stale_timeout_floor.py @@ -74,6 +74,10 @@ import pytest ("anthropic/claude-opus-4-20250514", 240.0), ("anthropic/claude-sonnet-4.5", 180.0), ("anthropic/claude-sonnet-4.6", 180.0), + # Anthropic Mythos-class named reasoning models — deep-reasoning tier. + ("anthropic/claude-fable-5", 600.0), + ("claude-fable-5", 600.0), + ("claude-fable", 600.0), # xAI Grok reasoning variants — explicit, not bare `grok`. ("x-ai/grok-4-fast-reasoning", 300.0), ("x-ai/grok-4.20-reasoning", 300.0), diff --git a/tests/hermes_cli/test_bytecode_sweep.py b/tests/hermes_cli/test_bytecode_sweep.py new file mode 100644 index 000000000000..59bc03d955cb --- /dev/null +++ b/tests/hermes_cli/test_bytecode_sweep.py @@ -0,0 +1,164 @@ +"""Tests for the launch-time stale-bytecode sweep (checkout fingerprint guard). + +Bug class: the checkout's ``.py`` files change (``hermes update``, manual +``git pull``, ZIP update) while ``__pycache__`` retains bytecode compiled +from the previous revision; the next process to import trusts the stale +``.pyc`` and dies with ``cannot import name ...`` (#6207, #60242). + +The launch-time guard compares the current checkout fingerprint against the +last-validated stamp and sweeps ``__pycache__`` once when they diverge — +covering paths no update-time clear can reach (manual pulls, pre-hardening +updaters). +""" + +from pathlib import Path + +from hermes_cli import main as hermes_main + + +def _make_repo(tmp_path: Path, sha: str = "a" * 40) -> Path: + """Minimal git checkout layout that _read_git_revision_fingerprint groks.""" + repo = tmp_path / "repo" + git_dir = repo / ".git" + (git_dir / "refs" / "heads").mkdir(parents=True) + (git_dir / "HEAD").write_text("ref: refs/heads/main\n", encoding="utf-8") + (git_dir / "refs" / "heads" / "main").write_text(sha + "\n", encoding="utf-8") + return repo + + +def _make_pycache(repo: Path, subdir: str = "hermes_cli") -> Path: + cache = repo / subdir / "__pycache__" + cache.mkdir(parents=True) + (cache / "main.cpython-311.pyc").write_bytes(b"stale") + return cache + + +def test_sweep_clears_pycache_when_checkout_changed(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="b" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + # Stamp records a different (older) fingerprint. + (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).write_text( + "git:refs/heads/main:" + "a" * 40, encoding="utf-8" + ) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert not cache.exists() + # Stamp updated to the current fingerprint. + recorded = (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).read_text(encoding="utf-8") + assert recorded.strip().endswith("b" * 40) + + +def test_sweep_noop_when_fingerprint_matches(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="c" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + fingerprint = hermes_main._read_git_revision_fingerprint(repo) + assert fingerprint is not None + (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).write_text(fingerprint, encoding="utf-8") + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert cache.exists() # untouched — no needless recompiles on every launch + + +def test_sweep_first_launch_clears_and_records(monkeypatch, tmp_path): + """No stamp yet (first launch with the guard) → sweep once, record.""" + repo = _make_repo(tmp_path, sha="d" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert not cache.exists() + assert (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).exists() + + +def test_sweep_noop_on_non_git_install(monkeypatch, tmp_path): + """No .git → no fingerprint → guard must not touch anything or raise.""" + repo = tmp_path / "repo" + repo.mkdir() + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert cache.exists() + assert not (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).exists() + + +def test_sweep_never_raises_on_unreadable_stamp(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="e" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + stamp = repo / hermes_main._BYTECODE_FINGERPRINT_FILE + stamp.mkdir() # a directory where a file is expected → OSError on read + + hermes_main._sweep_stale_bytecode_if_checkout_changed() # must not raise + + # Unreadable stamp is treated as "changed": cache swept. + assert not cache.exists() + + +def test_record_bytecode_fingerprint_writes_atomically(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="f" * 40) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._record_bytecode_fingerprint() + + stamp = repo / hermes_main._BYTECODE_FINGERPRINT_FILE + assert stamp.read_text(encoding="utf-8").endswith("f" * 40) + assert not stamp.with_name(stamp.name + ".tmp").exists() + + +def test_record_bytecode_fingerprint_noop_without_git(monkeypatch, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._record_bytecode_fingerprint() # must not raise + + assert not (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).exists() + + +def test_sweep_skips_venv_and_git_dirs(monkeypatch, tmp_path): + """The underlying clear must not touch venv/node_modules bytecode.""" + repo = _make_repo(tmp_path, sha="9" * 40) + repo_cache = _make_pycache(repo, "hermes_cli") + venv_cache = repo / "venv" / "lib" / "__pycache__" + venv_cache.mkdir(parents=True) + (venv_cache / "x.pyc").write_bytes(b"keep") + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert not repo_cache.exists() + assert venv_cache.exists() + +# --------------------------------------------------------------------------- +# Plugin-update sibling site: __pycache__ under ~/.hermes/plugins/ +# --------------------------------------------------------------------------- + +def test_clear_plugin_bytecode_removes_nested_caches(tmp_path): + from hermes_cli import plugins_cmd + + plugin = tmp_path / "myplugin" + top = plugin / "__pycache__" + nested = plugin / "sub" / "__pycache__" + top.mkdir(parents=True) + nested.mkdir(parents=True) + (top / "a.pyc").write_bytes(b"stale") + (nested / "b.pyc").write_bytes(b"stale") + + removed = plugins_cmd._clear_plugin_bytecode(plugin) + + assert removed == 2 + assert not top.exists() + assert not nested.exists() + + +def test_clear_plugin_bytecode_never_raises_on_missing_dir(tmp_path): + from hermes_cli import plugins_cmd + + assert plugins_cmd._clear_plugin_bytecode(tmp_path / "nope") == 0 diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index 5a1914bc76bb..d550a2297230 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -149,6 +149,41 @@ class TestCmdUpdateNpmLockfileCache: ) assert hm._npm_lockfile_changed(tmp_path) is True + def test_missing_web_build_toolchain_defeats_skip(self, tmp_path, monkeypatch): + """A hash recorded over a tree that never got tsc/vite must not skip. + + Otherwise the half-installed tree is permanent: every later update + trusts the hash, the build keeps failing, and the stale dist is served + forever. + """ + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "package.json").write_text('{"workspaces": ["web"]}') + (tmp_path / "web").mkdir() + (tmp_path / "web" / "package.json").write_text("{}") + bin_dir = tmp_path / "node_modules" / ".bin" + bin_dir.mkdir(parents=True) + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is True + + (bin_dir / "tsc").touch() + (bin_dir / "vite").touch() + assert hm._npm_lockfile_changed(tmp_path) is False + + def test_toolchain_check_skipped_without_a_web_package(self, tmp_path, monkeypatch): + """Prebuilt bundles ship no web/ source — they must still skip.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "node_modules").mkdir() + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is False + def test_workspace_package_json_edit_defeats_skip(self, tmp_path, monkeypatch): """The manifest list comes from the root package.json `workspaces` globs (npm's source of truth), so ANY workspace (desktop included) diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index 94fd27949f16..bd1638fcf651 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -568,6 +568,61 @@ def test_cmd_update_refreshes_active_memory_provider_dependencies(monkeypatch, t assert refresh_calls == [True] +def test_cmd_update_reloads_runtime_modules_before_lazy_refresh(monkeypatch, tmp_path): + """Lazy refresh must not see pre-pull modules cached in this process.""" + _setup_update_mocks(monkeypatch, tmp_path) + monkeypatch.setattr("shutil.which", lambda name: "/usr/bin/uv" if name == "uv" else None) + monkeypatch.setattr(hermes_main, "_is_termux_env", lambda env=None: False) + + events = [] + + def fake_run(cmd, **kwargs): + if cmd == ["git", "fetch", "origin", "main"]: + return SimpleNamespace(stdout="", stderr="", returncode=0) + if cmd == ["git", "rev-parse", "--abbrev-ref", "HEAD"]: + return SimpleNamespace(stdout="main\n", stderr="", returncode=0) + if cmd == ["git", "rev-list", "HEAD..origin/main", "--count"]: + return SimpleNamespace(stdout="1\n", stderr="", returncode=0) + if cmd == ["git", "pull", "--ff-only", "origin", "main"]: + events.append("pull") + return SimpleNamespace(stdout="Updating\n", stderr="", returncode=0) + if "pip" in cmd and "install" in cmd: + events.append("install") + return SimpleNamespace(returncode=0, stdout="", stderr="") + + def fake_reload_runtime_modules(): + events.append("reload") + + def fake_refresh_lazy_features(install_prefix=None, env=None): + events.append("lazy-refresh") + return True + + monkeypatch.setattr(hermes_main.subprocess, "run", fake_run) + monkeypatch.setattr(hermes_main, "_reload_updated_runtime_modules", fake_reload_runtime_modules) + monkeypatch.setattr(hermes_main, "_refresh_active_lazy_features", fake_refresh_lazy_features) + + hermes_main.cmd_update(SimpleNamespace()) + + assert ( + events.index("pull") + < events.index("install") + < events.index("reload") + < events.index("lazy-refresh") + ) + + +def test_reload_updated_runtime_modules_restores_new_hermes_constants_symbol(monkeypatch): + """A pre-pull module object missing a new helper is repaired by reload.""" + import hermes_constants + + monkeypatch.delattr(hermes_constants, "apply_subprocess_home_env", raising=False) + assert not hasattr(hermes_constants, "apply_subprocess_home_env") + + hermes_main._reload_updated_runtime_modules() + + assert callable(hermes_constants.apply_subprocess_home_env) + + def test_install_with_optional_fallback_honors_custom_group(monkeypatch): """Termux update path should target .[termux-all] when requested.""" calls = [] diff --git a/tests/hermes_cli/test_web_ui_build.py b/tests/hermes_cli/test_web_ui_build.py index 95c0960de41a..df273054d8e3 100644 --- a/tests/hermes_cli/test_web_ui_build.py +++ b/tests/hermes_cli/test_web_ui_build.py @@ -21,8 +21,11 @@ import pytest from hermes_cli.main import ( _web_ui_build_needed, _build_web_ui, - _run_npm_install_deterministic, _compute_web_ui_content_hash, + _missing_web_build_tool, + _run_npm_install_deterministic, + _web_build_toolchain_ready, + _web_toolchain_roots, _web_ui_stamp_path, _write_web_ui_build_stamp, ) @@ -563,3 +566,136 @@ class TestBuildWebUIFlock: def test_lock_file_is_gitignored(self): gitignore = Path(__file__).resolve().parents[2] / ".gitignore" assert ".web_ui_build.lock" in gitignore.read_text(encoding="utf-8") + + +def _link_shims(bin_dir: Path, *names: str) -> None: + bin_dir.mkdir(parents=True, exist_ok=True) + for name in names: + (bin_dir / name).touch() + + +class TestWebBuildToolchainReady: + """A tree is ready when the build can resolve tsc AND vite from any root. + + ``npm run build`` searches ``node_modules/.bin`` from the script's own + package up through every ancestor, so a shim in either place counts. + """ + + def test_missing_toolchain_is_not_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + + def test_partial_toolchain_is_not_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + + def test_hoisted_shims_at_workspace_root_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + def test_shims_nested_under_the_package_are_ready(self, tmp_path): + """#42973 layout: web/ owns its lockfile, so npm links shims there.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "node_modules").mkdir() + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + def test_shims_split_across_roots_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + _link_shims(web_dir / "node_modules" / ".bin", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + @pytest.mark.parametrize("shim", ["tsc.cmd", "tsc.ps1", "tsc.exe"]) + def test_windows_shim_extensions_count(self, tmp_path, shim): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", shim, "vite.cmd") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + +class TestWebToolchainRoots: + def test_searches_the_package_and_its_workspace_root(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + assert _web_toolchain_roots(web_dir) == (web_dir, tmp_path) + + +class TestMissingWebBuildTool: + """Every shell words an unresolvable binary differently.""" + + @pytest.mark.parametrize( + "output,expected", + [ + ("sh: 1: tsc: not found\nnpm error code 127", "tsc"), + ("bash: line 1: vite: command not found", "vite"), + ("'tsc' is not recognized as an internal or external command", "tsc"), + ("error TS2307: Cannot find module './x'", None), + ("", None), + ], + ) + def test_detects_the_unresolvable_tool(self, output, expected): + assert _missing_web_build_tool(output) == expected + + +class TestBuildRecoversFromMissingToolchain: + def test_reinstalls_and_retries_when_the_build_cannot_resolve_tsc(self, tmp_path): + """The generic retry reruns the same command, so it can't fix this alone.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_fail = __import__("subprocess").CompletedProcess( + [], 127, stdout="sh: 1: tsc: not found\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[build_fail, build_ok]) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 2 + assert mock_build.call_count == 2 + + def test_healthy_tree_builds_without_an_extra_install(self, tmp_path): + """No pre-build probing: a build that works is never second-guessed.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1 + assert mock_build.call_count == 1 + + def test_unrelated_build_failure_takes_the_generic_retry_only(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + type_error = __import__("subprocess").CompletedProcess( + [], 2, stdout="src/app.tsx(3,1): error TS2307: Cannot find module\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[type_error, build_ok]), \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1 diff --git a/tests/run_agent/test_tool_call_incremental_persistence.py b/tests/run_agent/test_tool_call_incremental_persistence.py index 34d4d79141d8..f9ceafec0620 100644 --- a/tests/run_agent/test_tool_call_incremental_persistence.py +++ b/tests/run_agent/test_tool_call_incremental_persistence.py @@ -28,7 +28,12 @@ from pathlib import Path import tempfile from unittest.mock import MagicMock, patch +import pytest + from agent.tool_dispatch_helpers import make_tool_result_message +from agent.agent_runtime_helpers import sanitize_api_messages +from agent.tool_executor import execute_tool_calls_segmented +from hermes_state import SessionDB from run_agent import AIAgent @@ -75,6 +80,31 @@ def _make_agent(): return agent +def _attach_real_session_db(agent, db_path: Path, session_id: str) -> SessionDB: + db = SessionDB(db_path=db_path) + db.create_session(session_id=session_id, source="tui", model="test/model") + agent._session_db = db + agent._session_db_created = True + agent.session_id = session_id + agent._last_flushed_db_idx = 0 + agent._flushed_db_message_ids = set() + agent._flushed_db_message_session_id = None + agent._persist_disabled = False + return db + + +def _durable_messages(db_path: Path, session_id: str) -> list[dict]: + restarted_db = SessionDB(db_path=db_path) + try: + return restarted_db.get_messages_as_conversation(session_id) + finally: + restarted_db.close() + + +def _durable_roles(db_path: Path, session_id: str) -> list[str]: + return [message["role"] for message in _durable_messages(db_path, session_id)] + + def _mock_tool_call(name="web_search", arguments="{}", call_id="call_1"): return SimpleNamespace( id=call_id, @@ -142,6 +172,78 @@ def test_run_conversation_flushes_assistant_tool_call_before_execution(): assert result["final_response"] == "done" +def test_interim_assistant_is_durable_before_ui_projection_on_abnormal_exit(tmp_path): + """A visible interim assistant row must survive an immediate process exit. + + ``GeneratorExit`` models an uncatchable turn interruption at the UI bridge: + no turn finalizer or graceful shutdown persistence is allowed to rescue the + row after the callback observes it. + """ + agent = _make_agent() + db_path = tmp_path / "state.db" + session_id = "interim-abnormal-exit" + db = _attach_real_session_db(agent, db_path, session_id) + tool_call = _mock_tool_call(call_id="visible-call") + agent.client.chat.completions.create.return_value = _mock_response( + content="I'll inspect the repository now.", + finish_reason="tool_calls", + tool_calls=[tool_call], + ) + + roles_seen_by_ui: list[str] = [] + + def _ui_projection(_text, *, already_streamed=False): + roles_seen_by_ui.extend(_durable_roles(db_path, session_id)) + raise GeneratorExit("simulated process termination after UI projection") + + agent.interim_assistant_callback = _ui_projection + try: + with pytest.raises(GeneratorExit, match="simulated process termination"): + agent.run_conversation("inspect the repository") + finally: + db.close() + + assert roles_seen_by_ui == ["user", "assistant"] + durable = _durable_messages(db_path, session_id) + assert [message["role"] for message in durable] == ["user", "assistant"] + assert durable[1]["content"] == "I'll inspect the repository now." + assert durable[1]["tool_calls"][0]["id"] == "visible-call" + + # Cold-resume reconciliation closes the interrupted call in the provider + # payload without mutating or duplicating the canonical transcript. + resumed = sanitize_api_messages(durable) + assert [message["role"] for message in resumed] == [ + "user", + "assistant", + "tool", + ] + assert resumed[2]["tool_call_id"] == "visible-call" + assert len(_durable_messages(db_path, session_id)) == 2 + + +def test_failed_assistant_persist_blocks_ui_projection_and_tool_side_effects(): + agent = _make_agent() + tool_call = _mock_tool_call(call_id="must-not-run") + agent.client.chat.completions.create.return_value = _mock_response( + content="I'll inspect the repository now.", + finish_reason="tool_calls", + tool_calls=[tool_call], + ) + agent._flush_messages_to_session_db = MagicMock(return_value=False) + agent.interim_assistant_callback = MagicMock() + agent._execute_tool_calls = MagicMock() + + result = agent.run_conversation("inspect the repository") + + agent.interim_assistant_callback.assert_not_called() + agent._execute_tool_calls.assert_not_called() + assert agent.client is not None + assert agent.client.chat.completions.create.call_count == 1 + assert result["failed"] is True + assert result["completed"] is False + assert result["turn_exit_reason"] == "session_persistence_failed" + + # --------------------------------------------------------------------------- # Contract 2: the SEQUENTIAL path flushes each tool result immediately, BEFORE # the next tool dispatches. Dispatch goes through run_agent.handle_function_call @@ -198,6 +300,145 @@ def test_execute_tool_calls_sequential_flushes_each_tool_result_before_next_disp ] +@pytest.mark.parametrize("executor_mode", ["sequential", "concurrent"]) +def test_tool_result_is_durable_before_ui_completion_on_abnormal_exit( + tmp_path, + executor_mode, +): + """A visible tool completion must already exist in the canonical DB.""" + agent = _make_agent() + db_path = tmp_path / "state.db" + session_id = f"tool-result-abnormal-exit-{executor_mode}" + db = _attach_real_session_db(agent, db_path, session_id) + tool_call = _mock_tool_call(call_id="visible-call") + messages = [ + {"role": "user", "content": "inspect the repository"}, + { + "role": "assistant", + "content": "I'll inspect the repository now.", + "tool_calls": [ + { + "id": "visible-call", + "type": "function", + "function": {"name": "web_search", "arguments": "{}"}, + } + ], + }, + ] + agent._flush_messages_to_session_db(messages) + + roles_seen_by_ui: list[str] = [] + + def _ui_completion(*_args): + roles_seen_by_ui.extend(_durable_roles(db_path, session_id)) + raise GeneratorExit("simulated process termination after tool completion") + + agent.tool_complete_callback = _ui_completion + assistant_message = SimpleNamespace(content="", tool_calls=[tool_call]) + dispatch_patch = ( + patch("run_agent.handle_function_call", return_value="repository result") + if executor_mode == "sequential" + else patch.object(agent, "_invoke_tool", return_value="repository result") + ) + try: + with ( + dispatch_patch, + patch( + "agent.tool_executor.maybe_persist_tool_result", + side_effect=lambda **kwargs: kwargs["content"], + ), + pytest.raises(GeneratorExit, match="simulated process termination"), + ): + if executor_mode == "sequential": + agent._execute_tool_calls_sequential( + assistant_message, + messages, + "task-1", + ) + else: + agent._execute_tool_calls_concurrent( + assistant_message, + messages, + "task-1", + ) + finally: + db.close() + + expected_roles = ["user", "assistant", "tool"] + assert roles_seen_by_ui == expected_roles + durable = _durable_messages(db_path, session_id) + assert [message["role"] for message in durable] == expected_roles + assert durable[2]["tool_call_id"] == "visible-call" + assert durable[2]["content"] == "repository result" + + +@pytest.mark.parametrize("executor_mode", ["sequential", "concurrent"]) +def test_failed_tool_result_persist_blocks_completion_projection(executor_mode): + agent = _make_agent() + tool_call = _mock_tool_call(call_id="failed-persist") + assistant_message = SimpleNamespace(content="", tool_calls=[tool_call]) + messages: list = [] + agent._flush_messages_to_session_db = MagicMock(return_value=False) + agent.tool_complete_callback = MagicMock() + dispatch_patch = ( + patch("run_agent.handle_function_call", return_value="repository result") + if executor_mode == "sequential" + else patch.object(agent, "_invoke_tool", return_value="repository result") + ) + + with ( + dispatch_patch, + patch( + "agent.tool_executor.maybe_persist_tool_result", + side_effect=lambda **kwargs: kwargs["content"], + ), + ): + if executor_mode == "sequential": + agent._execute_tool_calls_sequential( + assistant_message, + messages, + "task-1", + ) + else: + agent._execute_tool_calls_concurrent( + assistant_message, + messages, + "task-1", + ) + + agent.tool_complete_callback.assert_not_called() + assert getattr(agent, "_incremental_persistence_failed", False) is True + + +def test_segmented_batch_stops_before_later_segment_after_persist_failure(): + agent = _make_agent() + first = _mock_tool_call(call_id="first") + second = _mock_tool_call(call_id="second") + assistant_message = SimpleNamespace(tool_calls=[first, second]) + messages: list = [] + agent._flush_messages_to_session_db = MagicMock(return_value=False) + + with ( + patch.object(agent, "_invoke_tool", return_value="first result") as invoke, + patch("run_agent.handle_function_call", return_value="second result") as dispatch, + patch( + "agent.tool_executor.maybe_persist_tool_result", + side_effect=lambda **kwargs: kwargs["content"], + ), + ): + execute_tool_calls_segmented( + agent, + assistant_message, + messages, + "task-1", + segments=[("parallel", [first]), ("sequential", [second])], + ) + + invoke.assert_called_once() + dispatch.assert_not_called() + assert getattr(agent, "_incremental_persistence_failed", False) is True + + # --------------------------------------------------------------------------- # Contract 3: the CONCURRENT path flushes each collected tool result in append # order. Dispatch goes through agent._invoke_tool (the real concurrent diff --git a/tests/run_agent/test_turn_completion_explainer.py b/tests/run_agent/test_turn_completion_explainer.py index 95a7a4b54a8d..386a74c754c0 100644 --- a/tests/run_agent/test_turn_completion_explainer.py +++ b/tests/run_agent/test_turn_completion_explainer.py @@ -106,6 +106,16 @@ def test_explanation_for_all_retries_exhausted(): assert "retries" in out.lower() +def test_explanation_for_session_persistence_failed(): + """Fail-closed persistence exits (#72425) must explain themselves.""" + out = AIAgent._format_turn_completion_explanation( + "session_persistence_failed" + ) + assert out # non-empty + assert "session storage" in out.lower() + assert "disk space" in out.lower() + + # -------------------------------------------------------------------------- # 2. Enable/disable seam # -------------------------------------------------------------------------- diff --git a/tests/tools/test_interrupt.py b/tests/tools/test_interrupt.py index aca47df9e19b..5552ea496b18 100644 --- a/tests/tools/test_interrupt.py +++ b/tests/tools/test_interrupt.py @@ -122,6 +122,11 @@ class TestPreToolCheck: agent._interrupt_requested = True agent.log_prefix = "" agent._persist_session = MagicMock() + # PR #72425: execute_tool_calls_* read _incremental_persistence_failed + # via getattr at loop top. A bare MagicMock auto-creates a truthy value + # for any attribute access, which would short-circuit the interrupt + # skip path before any cancelled-tool messages are appended. + agent._incremental_persistence_failed = False # Import and call the method import types diff --git a/tests/tools/test_windows_native_support.py b/tests/tools/test_windows_native_support.py index b4187cf02a02..3b8a52fe8306 100644 --- a/tests/tools/test_windows_native_support.py +++ b/tests/tools/test_windows_native_support.py @@ -666,15 +666,21 @@ class TestTuiGatewayEntrySignalGuards: def test_source_guards_each_signal_installation(self): root = Path(__file__).resolve().parents[2] source = (root / "tui_gateway" / "entry.py").read_text(encoding="utf-8") - # Every signal.signal(...) at module scope must be preceded by a - # hasattr check. We look at the text: no bare "signal.signal(" - # call should appear outside a function body without a guard. - # Simpler heuristic: all SIGPIPE / SIGHUP references outside the - # dict-building loop must be wrapped in hasattr. - assert 'hasattr(signal, "SIGPIPE")' in source - assert 'hasattr(signal, "SIGHUP")' in source - assert 'hasattr(signal, "SIGTERM")' in source - assert 'hasattr(signal, "SIGINT")' in source + # Every signal installation at module scope must be guarded against + # missing signals (Windows: SIGPIPE/SIGHUP absent). Originally this + # was ``hasattr(signal, "SIGPIPE")`` inline; PR #72677 refactored to + # ``_install_signal("SIGPIPE", ...)`` which does the same guard via + # ``getattr(signal, signame, None)`` internally. Either form is + # acceptable — what matters is no bare ``signal.signal(SIGPIPE)`` + # at module scope without a guard. + for sig_name in ("SIGPIPE", "SIGHUP", "SIGTERM", "SIGINT"): + assert ( + f'hasattr(signal, "{sig_name}")' in source + or f'_install_signal("{sig_name}"' in source + ), ( + f"signal {sig_name} must be installed via a guarded path " + f"(hasattr or _install_signal), not bare signal.signal()" + ) def test_module_imports_cleanly(self): """Importing the module must not raise — verifies the guards work.""" diff --git a/tests/tui_gateway/test_entry_import_off_main_thread.py b/tests/tui_gateway/test_entry_import_off_main_thread.py new file mode 100644 index 000000000000..478053a8e5ad --- /dev/null +++ b/tests/tui_gateway/test_entry_import_off_main_thread.py @@ -0,0 +1,85 @@ +"""Regression test: importing tui_gateway.entry off the main thread must not crash. + +Background +---------- +``entry.py`` installs signal handlers (SIGPIPE, SIGTERM, …) at *import time*. +``signal.signal`` is only legal in the main thread, so a first import of +``entry`` from a worker thread raises +``ValueError: signal only works in main thread of the main interpreter``. + +The Desktop/WebSocket agent-build path (``server._build``) runs in a daemon +thread and does ``from tui_gateway.entry import ensure_mcp_discovery_started`` +(server.py:1866). On that path ``entry.main()`` is never run, so the worker +thread performs the *first* import of ``entry`` — which crashed and aborted +MCP discovery startup (#72667, regression from the 2026-07-26 websocket-MCP +discovery fix). + +Fix: guard the import-time signal installs with a main-thread check. Handlers +are process-global, so installing them only when reached in the main thread is +sufficient; importing from a worker thread becomes a safe no-op. + +This test runs the import in a *fresh* subprocess worker thread to guarantee a +clean first-import (the test process already imported entry in its main +thread, so an in-process import would not reproduce the bug). +""" + +import subprocess +import sys + +REPO_ROOT = "." + + +def _spawn_worker_import_entry(): + """Run `import tui_gateway.entry` for the first time inside a worker thread. + + Returns (returncode, stdout, stderr). + """ + code = ( + "import threading, sys, signal, os\n" + "sys.stdout.reconfigure(line_buffering=True)\n" + "sys.stderr.reconfigure(line_buffering=True)\n" + "errs = []\n" + "def _worker():\n" + " try:\n" + " import tui_gateway.entry\n" + " except Exception as e:\n" + " errs.append(repr(e))\n" + "t = threading.Thread(target=_worker, daemon=True)\n" + "t.start(); t.join(timeout=15)\n" + "if errs:\n" + " sys.stdout.write('IMPORT_FAILED: ' + errs[0] + '\\n')\n" + " sys.exit(2)\n" + "# main thread of this process still installs SIGPIPE handler\n" + "h = signal.getsignal(signal.SIGPIPE)\n" + "sys.stdout.write('OK handler_installed=' + str(h is signal.SIG_IGN or callable(h)) + '\\n')\n" + "sys.exit(0)\n" + ) + proc = subprocess.run( + [sys.executable, "-c", code], + cwd=REPO_ROOT, + capture_output=True, + text=True, + timeout=60, + env={**__import__("os").environ}, + ) + return proc.returncode, proc.stdout, proc.stderr + + +def test_entry_imports_cleanly_from_worker_thread(): + """First import of tui_gateway.entry from a worker thread must succeed.""" + rc, out, err = _spawn_worker_import_entry() + # entry import may emit to stdout or stderr depending on the runner; check both. + combined = out + err + assert rc == 0, f"entry import off main thread failed (rc={rc}): {err!r}" + assert "OK" in combined, f"unexpected output: {out!r} / {err!r}" + + +def test_entry_installs_sigpipe_handler_in_main_thread(): + """Even though it can be imported off-thread, the main-thread path still + installs the SIGPIPE handler (process-global, so it applies everywhere).""" + rc, out, err = _spawn_worker_import_entry() + combined = out + err + assert rc == 0, f"entry import off main thread failed (rc={rc}): {err!r}" + assert "handler_installed=True" in combined, ( + f"SIGPIPE handler not installed: {out!r} / {err!r}" + ) diff --git a/tui_gateway/entry.py b/tui_gateway/entry.py index bf41c1201816..2b8ccee8838a 100644 --- a/tui_gateway/entry.py +++ b/tui_gateway/entry.py @@ -13,6 +13,7 @@ hermes_bootstrap.harden_import_path() import json import logging import signal +import threading import time import traceback @@ -184,18 +185,49 @@ def _log_signal(signum: int, frame) -> None: # with hasattr so ``python -m tui_gateway.entry`` (spawned by # ``hermes --tui``) imports cleanly there. SIGBREAK (Windows' Ctrl+Break) # is installed when available as a weaker equivalent of SIGHUP. -if hasattr(signal, "SIGPIPE"): - signal.signal(signal.SIGPIPE, signal.SIG_IGN) -if hasattr(signal, "SIGTERM"): - signal.signal(signal.SIGTERM, _log_signal) +# +# signal.signal() is only legal in the MAIN thread. On the Desktop/WebSocket +# agent-build path, server._build() runs in a daemon thread and does +# ``from tui_gateway.entry import ensure_mcp_discovery_started`` as the first +# import of entry (entry.main() is never run there), which used to raise +# "ValueError: signal only works in main thread of the main interpreter" and +# abort MCP discovery startup. Install each handler only when we're in the +# main thread: handlers are process-global, so a main-thread import anywhere +# in the process still installs them for everyone, and an off-thread import +# (Desktop build path) simply no-ops instead of crashing the import. This +# preserves the original SIG_IGN/SIG_DFL behavior on the classic TUI/serve +# path while fixing the off-thread import crash. + + +def _install_signal(signame, handler): + """Install a signal handler if legal in this thread. + + signal.signal() raises ValueError outside the main thread; skip silently + there so a worker-thread import of this module (Desktop build path) does + not abort. On any main-thread import the handler is installed as before. + """ + if threading.current_thread() is not threading.main_thread(): + return + sig = getattr(signal, signame, None) + if sig is None: + return # Windows: SIGPIPE/SIGHUP absent + try: + signal.signal(sig, handler) + except (ValueError, OSError, RuntimeError): + # Not in the main thread despite the check, or handler rejected. + # Skip rather than crash the import (see above). + pass + + +_install_signal("SIGPIPE", signal.SIG_IGN) +_install_signal("SIGTERM", _log_signal) if hasattr(signal, "SIGHUP"): - signal.signal(signal.SIGHUP, _log_signal) + _install_signal("SIGHUP", _log_signal) elif hasattr(signal, "SIGBREAK"): # Windows-only: Ctrl+Break in a console window delivers SIGBREAK. # Route it through the same handler so kills are diagnosable. - signal.signal(signal.SIGBREAK, _log_signal) -if hasattr(signal, "SIGINT"): - signal.signal(signal.SIGINT, signal.SIG_IGN) + _install_signal("SIGBREAK", _log_signal) +_install_signal("SIGINT", signal.SIG_IGN) def _log_exit(reason: str) -> None: diff --git a/website/docs/user-guide/features/api-server.md b/website/docs/user-guide/features/api-server.md index 0311e2a228bc..8380d6313145 100644 --- a/website/docs/user-guide/features/api-server.md +++ b/website/docs/user-guide/features/api-server.md @@ -375,6 +375,17 @@ Statuses are retained briefly after terminal states (`completed`, `failed`, or ` Server-Sent Events stream of the run's tool-call progress, token deltas, and lifecycle events. Designed for dashboards and thick clients that want to attach/detach without losing state. +When the agent delegates work to background subagents, the stream also carries +`subagent.start` and `subagent.complete` lifecycle events, so clients can +observe delegation outcomes — including timeouts and failures — instead of the +run going silent while a child works. The `subagent.complete` payload carries +the child's status, summary, duration, token/cost figures, and a +`child_session_id` for correlation; free-text fields pass forced secret +redaction before leaving the process. Per-tool child events +(`subagent.tool`, progress ticks) are intentionally **not** forwarded — they +are high-volume UI noise; use the per-child live transcript files for +play-by-play. + Unconsumed event buffers expire after five minutes so a detached client cannot grow memory indefinitely. This expires transport state only: a run that is still executing remains visible to status polling, approval, stop control, and diff --git a/website/docs/user-guide/features/delegation.md b/website/docs/user-guide/features/delegation.md index 4790ce3b4200..d40a0dfc107b 100644 --- a/website/docs/user-guide/features/delegation.md +++ b/website/docs/user-guide/features/delegation.md @@ -191,10 +191,54 @@ delegation: A positive value enforces a hard wall-clock limit on each child; `0` or a negative value disables it. +When a configured cap fires, the child's result carries structured timeout +metadata alongside the error message so parents and hooks can distinguish a +stopwatch kill from other failures without parsing text: `timeout_seconds` +(the configured cap), `timed_out_after_seconds` (actual wall clock), and +`timeout_phase` (`before_first_llm_call` when the child never reached its +first request, `after_llm_calls` otherwise). All three are `null` on +non-timeout errors. + :::tip Diagnostic dump on zero-call timeout -With a hard cap configured, if a subagent times out having made **zero** API calls (usually: provider unreachable, auth failure, or tool-schema rejection), `delegate_task` writes a structured diagnostic to `~/.hermes/logs/subagent-timeout--.log` containing the subagent's config snapshot, credential-resolution trace, and any early error messages. Much easier to root-cause than the previous silent-timeout behavior. +With a hard cap configured, if a subagent times out having made **zero** API calls (usually: provider unreachable, auth failure, or tool-schema rejection), `delegate_task` writes a structured diagnostic to `~/.hermes/logs/subagent-timeout--.log` containing the subagent's config snapshot, credential-resolution trace, any early error messages, and stack traces for **all** live threads (not just the child's own) — a child parked waiting on a nested helper thread is indistinguishable from a slow provider without the full picture. ::: +## Stall Detection for Background Subagents + +Background delegations (`delegate_task(background=true)`) are watched by a +**progress-based stall monitor** — on by default, zero config. Unlike a +wall-clock timeout, it never touches a child that is making progress, no +matter how long it runs. + +The monitor samples each detached child's progress signals — API-call count, +current tool, and last-activity timestamp (which ticks on **every streamed +token**, tool transition, and API-call boundary, so a child mid-stream on a +long response always counts as alive): + +1. **Progressing children are never touched.** Any advancing signal resets + the clock. +2. A child whose progress is completely frozen past the stale threshold + (450s idle, 1200s while inside a tool — legitimately slow terminal + commands and web fetches get the higher ceiling) is **interrupted** and + given a 120s grace window. A child that unwinds in time delivers its + partial results through the normal completion path. +3. A child that never returns is force-finalized with a terminal `stalled` + completion event, so the owning session hears an outcome instead of + going silent, and the async slot frees for new work. + +The `stalled` event carries structured metadata mirroring the sync-path +timeout fields: `stalled_after_quiet_seconds`, `stall_threshold_seconds`, +`stall_phase` (`idle` / `in_tool`), and `stall_grace_seconds`. + +This closed a long-standing failure mode where a wedged background child +left its session looking dead until a process restart. The underlying wedge +(children hanging at their first API call after multi-day gateway uptime) +was also fixed at the root: delegated children now run their OpenAI-wire +API requests inline on their own conversation thread instead of a nested +worker thread — the layer where the wedge lived. The stall monitor remains +as the safety net for anything else. + + ## Monitoring Running Subagents (`/agents`) The TUI ships a `/agents` overlay (alias `/tasks`) that turns recursive `delegate_task` fan-out into a first-class audit surface: @@ -206,6 +250,22 @@ The TUI ships a `/agents` overlay (alias `/tasks`) that turns recursive `delegat The classic CLI just prints `/agents` as a text summary; the TUI is where the overlay shines. See [TUI — Slash commands](/user-guide/tui#slash-commands). +On the classic CLI and every gateway platform (Telegram, Discord, Slack, ...), +`/agents` also lists **background delegations with live per-child activity**, +sampled directly from each running child: + +``` +Background delegations: 1 running +- deleg_ab12cd34 · running · research the delegation stall monitor + - child 1: 4 api calls · in web_search · active 12s ago + - child 2: 7 api calls · between turns · active 3s ago +``` + +A delegation the stall monitor has flagged shows as +`stalling · no progress 450s — interrupting`, and long-quiet-but-healthy +children show their quiet time so you can tell "slow" from "stuck" at a +glance. + ## Live Transcripts Every `delegate_task` dispatch also creates one **append-only, human-readable log per task** so you (or the parent agent) can watch a subagent work in real time instead of waiting for the consolidated summary: diff --git a/website/sidebars.ts b/website/sidebars.ts index 7314af5a2b38..87f7a7e8d898 100644 --- a/website/sidebars.ts +++ b/website/sidebars.ts @@ -750,6 +750,7 @@ const sidebars: SidebarsConfig = { link: {type: 'doc', id: 'developer-guide/plugins/index'}, items: [ 'developer-guide/plugin-llm-access', + 'developer-guide/subagent-lifecycle-api', 'developer-guide/desktop-plugin-sdk', 'developer-guide/memory-provider-plugin', 'developer-guide/context-engine-plugin',