diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index c6db27104de91..ed74a242d90dd 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -3327,6 +3327,49 @@ def _build_xai_oauth_aux_client(model: str) -> Tuple[Optional[Any], Optional[str return CodexAuxiliaryClient(real_client, model), model +def _build_minimax_oauth_aux_client(model: str) -> Tuple[Optional[Any], Optional[str]]: + """Build an Anthropic auxiliary client for MiniMax OAuth.""" + if not model: + logger.warning( + "Auxiliary client: minimax-oauth requested without a model; " + "pass model explicitly (auxiliary..model in config.yaml)." + ) + return None, None + try: + from hermes_cli.auth import resolve_minimax_oauth_runtime_credentials + except ImportError: + logger.debug("hermes_cli.auth not available for minimax-oauth") + return None, None + try: + creds = resolve_minimax_oauth_runtime_credentials(as_token_provider=True) + except Exception as exc: + logger.warning( + "resolve_provider_client: minimax-oauth requested but no valid " + "MiniMax OAuth token found (run: hermes model -> MiniMax OAuth): %s", + exc, + ) + return None, None + api_key = creds["api_key"] + base_url = creds["base_url"].rstrip("/") + logger.debug("Auxiliary client: MiniMax OAuth (%s via Anthropic API)", model) + try: + from agent.anthropic_adapter import build_anthropic_client + real_client = build_anthropic_client(api_key, base_url) + except ImportError as exc: + logger.warning( + "resolve_provider_client: minimax-oauth requested but the anthropic " + "SDK is not installed: %s", exc, + ) + return None, None + except Exception as exc: + logger.warning( + "resolve_provider_client: minimax-oauth failed to build Anthropic " + "client: %s", exc, + ) + return None, None + return AnthropicAuxiliaryClient(real_client, model, api_key, base_url, is_oauth=True), model + + def _build_codex_client(model: str) -> Tuple[Optional[Any], Optional[str]]: """Build a CodexAuxiliaryClient for an explicitly-requested model. @@ -5959,6 +6002,16 @@ def _wrap_if_needed(client_obj, final_model_str: str, base_url_str: str = "", return (_to_async_client(client, final_model, is_vision=is_vision) if async_mode else (client, final_model)) + # MiniMax OAuth uses the Anthropic-compatible Messages endpoint and a + # refreshable bearer token supplied by the runtime credential resolver. + if provider == "minimax-oauth": + client, default = _build_minimax_oauth_aux_client(model) + if client is None: + return None, None + final_model = _normalize_resolved_model(model or default, provider) + return (_to_async_client(client, final_model, is_vision=is_vision) if async_mode + else (client, final_model)) + # ── Custom endpoint (OPENAI_BASE_URL + OPENAI_API_KEY) ─────────── if provider == "custom": custom_base = "" @@ -6454,7 +6507,7 @@ def _wrap_if_needed(client_obj, final_model_str: str, base_url_str: str = "", return (_to_async_client(client, final_model, is_vision=is_vision) if async_mode else (client, final_model)) - elif pconfig.auth_type in {"oauth_device_code", "oauth_external"}: + elif pconfig.auth_type in {"oauth_device_code", "oauth_external", "oauth_minimax"}: # OAuth providers — route through their specific try functions if provider == "nous": return resolve_provider_client("nous", model, async_mode) @@ -6462,6 +6515,8 @@ def _wrap_if_needed(client_obj, final_model_str: str, base_url_str: str = "", return resolve_provider_client("openai-codex", model, async_mode) if provider == "xai-oauth": return resolve_provider_client("xai-oauth", model, async_mode) + if provider == "minimax-oauth": + return resolve_provider_client("minimax-oauth", model, async_mode) # Other OAuth providers not directly supported if provider not in _LOGGED_UNSUPPORTED_OAUTH_KEYS: _LOGGED_UNSUPPORTED_OAUTH_KEYS.add(provider) diff --git a/cli.py b/cli.py index 26e202711d477..b5c9827497b92 100644 --- a/cli.py +++ b/cli.py @@ -3865,6 +3865,27 @@ def _collect_query_images(query: str | None, image_arg: str | None = None) -> tu return message, deduped + + +def _history_navigation_action(buffer, direction): + """Return ``history`` only for a genuinely empty buffer. + + A non-empty buffer must never invoke prompt_toolkit's ``auto_up`` / + ``auto_down`` because a visually wrapped single logical line can otherwise + recall command history instead of moving the cursor. Whitespace-only input + is treated as empty by contract. + """ + try: + text = getattr(buffer, "text", "") or "" + except Exception: + text = "" + if text.strip() and direction in ("up", "down"): + return "cursor" + if direction not in ("up", "down"): + return "cursor" + return "history" + + # Strip OSC escape sequences (e.g. OSC-8 hyperlinks) that prompt_toolkit's # ANSI parser can't handle — it strips \x1b but passes the payload through # as literal text, garbling the TUI output. @@ -15877,8 +15898,8 @@ def _recall_without_recollapse(buf, move): """Run a history-navigation move, suppressing paste-collapse. Recalled history can hold the full text of a paste that was - collapsed to a placeholder at submit time. Loading it back into the - buffer looks exactly like a fresh large paste to ``_on_text_changed`` + collapsed to a placeholder at submit time. Loading it back into + the buffer looks exactly like a fresh large paste to ``_on_text_changed`` and would be re-collapsed. Set the skip flag around the move; if the move didn't change the text (plain cursor movement), clear the flag so a later real paste still collapses. @@ -15891,15 +15912,21 @@ def _recall_without_recollapse(buf, move): @kb.add('up', filter=_normal_input) def history_up(event): - """Up arrow: browse history when on first line, else move cursor up.""" + """Up arrow: browse history only with an empty buffer.""" buf = event.app.current_buffer - _recall_without_recollapse(buf, lambda: buf.auto_up(count=event.arg)) + if _history_navigation_action(buf, "up") == "history": + _recall_without_recollapse(buf, lambda: buf.auto_up(count=event.arg)) + else: + buf.cursor_up() @kb.add('down', filter=_normal_input) def history_down(event): - """Down arrow: browse history when on last line, else move cursor down.""" + """Down arrow: browse history only with an empty buffer.""" buf = event.app.current_buffer - _recall_without_recollapse(buf, lambda: buf.auto_down(count=event.arg)) + if _history_navigation_action(buf, "down") == "history": + _recall_without_recollapse(buf, lambda: buf.auto_down(count=event.arg)) + else: + buf.cursor_down() @kb.add('c-l') def handle_ctrl_l(event): diff --git a/gateway/platforms/api_server.py b/gateway/platforms/api_server.py index a756134d99c2a..fefb88936e2b5 100644 --- a/gateway/platforms/api_server.py +++ b/gateway/platforms/api_server.py @@ -5954,6 +5954,34 @@ def _bind_api_server_session( cron_session="", ) + async def _prune_failed_session_if_empty( + self, + session_id: Optional[str], + *, + request_profile: Optional[str], + ) -> None: + """Best-effort cleanup of an empty session left by a failed API turn.""" + if not session_id: + return + try: + with self._profile_scope(request_profile): + from hermes_constants import get_hermes_home + + sessions_dir = get_hermes_home() / "sessions" + db = await self._ensure_session_db_async() + if db is not None: + await asyncio.to_thread( + db.delete_session_if_empty, + session_id, + sessions_dir=sessions_dir, + ) + except Exception: + logger.debug( + "Could not prune failed API session %s", + session_id, + exc_info=True, + ) + async def _run_agent( self, user_message: str, @@ -6175,7 +6203,25 @@ def _run(): self._activate_admitted_request() self._inflight_agent_runs += 1 try: - return await loop.run_in_executor(None, _run) + result, usage = await loop.run_in_executor(None, _run) + if isinstance(result, dict) and result.get("failed"): + await self._prune_failed_session_if_empty( + result.get("session_id") or session_id, + request_profile=request_profile, + ) + return result, usage + except asyncio.CancelledError: + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) + raise + except Exception: + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) + raise finally: self._inflight_agent_runs -= 1 @@ -6436,6 +6482,10 @@ async def _run_and_close(): try: self._set_run_status(run_id, "running") if run_id in self._stopping_run_ids: + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) _put_event_if_active({ "event": "run.cancelled", "run_id": run_id, @@ -6561,6 +6611,10 @@ def _run_sync(): result, usage = await asyncio.get_running_loop().run_in_executor(None, _run_sync) if run_id in self._stopping_run_ids: + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) _put_event_if_active({ "event": "run.cancelled", "run_id": run_id, @@ -6575,6 +6629,10 @@ def _run_sync(): # 401/400 return failed=True instead of raising, so the except # block below never fires — issue #15561). elif isinstance(result, dict) and result.get("failed"): + await self._prune_failed_session_if_empty( + result.get("session_id") or session_id, + request_profile=request_profile, + ) error_msg = _redact_api_error_text(result.get("error") or "agent run failed") _put_event_if_active({ "event": "run.failed", @@ -6588,8 +6646,8 @@ def _run_sync(): error=error_msg, last_event="run.failed", ) - else: - final_response = result.get("final_response", "") if isinstance(result, dict) else "" + elif isinstance(result, dict) and "final_response" in result: + final_response = result.get("final_response", "") _put_event_if_active({ "event": "run.completed", "run_id": run_id, @@ -6604,7 +6662,29 @@ def _run_sync(): usage=usage, last_event="run.completed", ) + else: + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) + error_msg = "agent returned a malformed result" + _put_event_if_active({ + "event": "run.failed", + "run_id": run_id, + "timestamp": time.time(), + "error": error_msg, + }) + self._set_run_status( + run_id, + "failed", + error=error_msg, + last_event="run.failed", + ) except asyncio.CancelledError: + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) self._set_run_status( run_id, "cancelled", @@ -6628,6 +6708,10 @@ def _run_sync(): # failure, instead of falling through to the generic # except-Exception branch below. logger.warning("Provider authentication failed for run=%s: %s", run_id, exc) + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) error_msg = f"⚠️ Provider authentication failed: {exc}" self._set_run_status( run_id, @@ -6646,6 +6730,10 @@ def _run_sync(): pass except Exception as exc: logger.exception("[api_server] run %s failed", run_id) + await self._prune_failed_session_if_empty( + session_id, + request_profile=request_profile, + ) self._set_run_status( run_id, "failed", diff --git a/tests/agent/test_auxiliary_minimax_oauth.py b/tests/agent/test_auxiliary_minimax_oauth.py new file mode 100644 index 0000000000000..ebe96edcff084 --- /dev/null +++ b/tests/agent/test_auxiliary_minimax_oauth.py @@ -0,0 +1,32 @@ +"""Regression tests for MiniMax OAuth auxiliary-client routing.""" +from unittest.mock import MagicMock, patch + + +def test_minimax_oauth_builds_anthropic_auxiliary_client(): + import agent.auxiliary_client as aux + + token_provider = lambda: "fresh-token" + real_client = MagicMock(name="real_anthropic_client") + + with patch( + "hermes_cli.auth.resolve_minimax_oauth_runtime_credentials", + return_value={ + "provider": "minimax-oauth", + "api_key": token_provider, + "base_url": "https://api.minimax.io/anthropic", + "source": "oauth", + }, + ), patch( + "agent.anthropic_adapter.build_anthropic_client", + return_value=real_client, + ) as build_client: + client, model = aux.resolve_provider_client( + "minimax-oauth", model="MiniMax-M3" + ) + + assert isinstance(client, aux.AnthropicAuxiliaryClient) + assert model == "MiniMax-M3" + assert client.base_url == "https://api.minimax.io/anthropic" + assert client.api_key is token_provider + build_client.assert_called_once() + assert build_client.call_args.args[0] is token_provider diff --git a/tests/cli/test_arrow_navigation_history_guard.py b/tests/cli/test_arrow_navigation_history_guard.py new file mode 100644 index 0000000000000..dcfbfb32a557f --- /dev/null +++ b/tests/cli/test_arrow_navigation_history_guard.py @@ -0,0 +1,201 @@ +"""Lane C: behavioral test for the arrow-key history-guard adaptation. + +Background +---------- +Hermes v0.19.0 uses a multiline TextArea as the chat input. prompt_toolkit's +``Buffer.auto_up`` / ``Buffer.auto_down`` decide whether to recall history +based on ``cursor_position_row`` (logical rows), not visual wrap rows. A single +logical-line input that wraps visually across multiple screen rows therefore +triggers history recall from any cursor position, even though the user is +just moving inside a wrapped line. + +Reinstatement of the old ``if buf.text: buf.cursor_up else buf.auto_up`` block +is NOT sufficient — ``cursor_up`` is also logical-line based, so it won't +move the cursor between visually-wrapped rows on a single logical line. The +acceptance contract is therefore scoped to the safe no-history half: + + A non-empty buffer must NEVER trigger history recall on Up/Down. + +Visual cursor movement across screen rows of a wrapped single logical line +remains a known limitation of the current architecture. + +These tests lock the contract on an extracted helper so the runtime +keybindings can stay inside ``HermesCLI.run()`` while the decision logic +remains unit-testable. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +import cli as cli_mod + + +# --------------------------------------------------------------------------- +# Helper tests — the contract is enforced by an extracted pure function +# --------------------------------------------------------------------------- + + +def test_empty_buffer_up_browses_history(): + """Empty buffer + Up arrow → history browse (the only safe path).""" + buf = SimpleNamespace(text="") + assert cli_mod._history_navigation_action(buf, "up") == "history" + + +def test_empty_buffer_down_browses_history(): + """Empty buffer + Down arrow → history browse.""" + buf = SimpleNamespace(text="") + assert cli_mod._history_navigation_action(buf, "down") == "history" + + +def test_nonempty_buffer_up_blocks_history_recall(): + """Non-empty buffer + Up arrow → cursor move, NEVER history browse. + + This is the regression guard for the wrapped-line bug. + """ + buf = SimpleNamespace(text="hello") + assert cli_mod._history_navigation_action(buf, "up") == "cursor" + + +def test_nonempty_buffer_down_blocks_history_recall(): + """Non-empty buffer + Down arrow → cursor move, NEVER history browse.""" + buf = SimpleNamespace(text="hello") + assert cli_mod._history_navigation_action(buf, "down") == "cursor" + + +def test_single_visual_line_wrapped_logically_short_blocks_history(): + """A buffer whose content is a single logical line (visually wrapped) must + not browse history on Up/Down — even though the cursor is on logical row 0. + """ + # One logical line, but visually several rows. Prompt_toolkit's auto_up + # would still call history_backward because cursor_position_row is 0. + long_single_line = "lorem ipsum dolor sit amet " * 20 + assert "\n" not in long_single_line + buf = SimpleNamespace(text=long_single_line) + assert cli_mod._history_navigation_action(buf, "up") == "cursor" + assert cli_mod._history_navigation_action(buf, "down") == "cursor" + + +def test_multiline_buffer_nonempty_blocks_history(): + """Multiline buffer (true multi-line, not just wrapped) must also block + history recall — the wrapped-line rationale generalises: any non-empty + input is too ambiguous for safe auto-history, and the user can still + move with cursor_up/cursor_down within the multi-line content. + """ + buf = SimpleNamespace(text="line one\nline two\nline three") + assert cli_mod._history_navigation_action(buf, "up") == "cursor" + assert cli_mod._history_navigation_action(buf, "down") == "cursor" + + +def test_whitespace_only_buffer_treated_as_empty(): + """A whitespace-only buffer is editorially empty — history browse is safe. + + Conservative on purpose: a space-padded prompt is not a real draft. + """ + buf = SimpleNamespace(text=" \n \t ") + assert cli_mod._history_navigation_action(buf, "up") == "history" + assert cli_mod._history_navigation_action(buf, "down") == "history" + + +def test_one_real_char_blocks_history(): + """One character is enough to mark the buffer as non-empty.""" + buf = SimpleNamespace(text="x") + assert cli_mod._history_navigation_action(buf, "up") == "cursor" + + +def test_unknown_direction_falls_back_to_cursor(): + """Defensive: unknown direction code never browses history. + + Better to do nothing visible than to recall an unintended history entry. + """ + buf = SimpleNamespace(text="") + assert cli_mod._history_navigation_action(buf, "pageup") == "cursor" + + +def test_helper_does_not_call_buffer_methods(): + """The decision helper must be pure — it must NOT mutate the buffer. + + The actual side effects (cursor_up / auto_up) live in the keybinding + shim. The helper is a single source of truth for the decision only. + """ + class Tracking: + def __init__(self): + self.calls = [] + self.text = "x" + + def cursor_up(self): + self.calls.append("cursor_up") + + def auto_up(self): + self.calls.append("auto_up") + + buf = Tracking() + cli_mod._history_navigation_action(buf, "up") + assert buf.calls == [], "decision helper must be side-effect free" + + +# --------------------------------------------------------------------------- +# Bound-method test — exercise the keybinding-side decision via the +# ``_normal_input`` condition's own shim. This locks the contract at the +# integration boundary the runtime actually uses, without spinning up +# a full prompt_toolkit Application. +# --------------------------------------------------------------------------- + + +def test_run_history_up_uses_helper_for_nonempty_buffer(monkeypatch): + """The bound history_up handler must consult the helper and refuse to + call auto_up when the buffer is non-empty (the wrapped-line guard). + """ + auto_up_calls: list[int] = [] + cursor_up_calls: list[int] = [] + + class FakeBuffer: + text = "draft in progress" + + def auto_up(self, count=1): + auto_up_calls.append(count) + + def cursor_up(self): + cursor_up_calls.append(1) + + class FakeApp: + def __init__(self): + self.current_buffer = FakeBuffer() + + class FakeEvent: + def __init__(self): + self.app = FakeApp() + self.arg = 1 + + # Locate the bound history_up handler. It is defined inside the + # ``run()`` method body, so we read it out of the source via the AST + # regex used by the existing detector — but the cleanest test is to + # verify the helper contract (already covered above) plus a final + # assertion that the source itself routes through the helper. + import re + from pathlib import Path + + src = Path(cli_mod.__file__).read_text(encoding="utf-8", errors="replace") + + # The handler must reference the helper. If the helper is not yet + # consulted, the regex below will fail and the test will FAIL. + pattern = re.compile( + r"def\s+history_up\(event\):.*?_history_navigation_action\(", + re.DOTALL, + ) + assert pattern.search(src), ( + "history_up handler must route through _history_navigation_action " + "helper; bare auto_up is the regression we are guarding against." + ) + + # Same for history_down. + pattern_down = re.compile( + r"def\s+history_down\(event\):.*?_history_navigation_action\(", + re.DOTALL, + ) + assert pattern_down.search(src), ( + "history_down handler must route through _history_navigation_action " + "helper; bare auto_down is the regression we are guarding against." + ) diff --git a/tests/gateway/test_api_server_failure_session_cleanup.py b/tests/gateway/test_api_server_failure_session_cleanup.py new file mode 100644 index 0000000000000..6bd58732f3b30 --- /dev/null +++ b/tests/gateway/test_api_server_failure_session_cleanup.py @@ -0,0 +1,333 @@ +"""Failure-session hygiene for API-server agent entry paths.""" + +import asyncio +from unittest.mock import MagicMock, patch + +import pytest +from aiohttp import web +from aiohttp.test_utils import TestClient, TestServer + +from gateway.config import PlatformConfig +from gateway.platforms.api_server import APIServerAdapter +from hermes_state import SessionDB + + +def _adapter_with_db(db: SessionDB) -> APIServerAdapter: + adapter = APIServerAdapter(PlatformConfig(enabled=True, extra={})) + adapter._session_db = db + return adapter + + +def _agent_result(result, db: SessionDB, *, persist_message: bool = False): + agent = MagicMock() + + def _run(*, user_message, conversation_history, task_id): + db.create_session(task_id, source="api_server", model="test") + if persist_message: + db.append_message(task_id, role="user", content=user_message) + return dict(result) + + agent.run_conversation.side_effect = _run + agent.session_prompt_tokens = 0 + agent.session_completion_tokens = 0 + agent.session_total_tokens = 0 + agent.session_id = None + return agent + + +def _agent_exception(exc: Exception, db: SessionDB): + agent = MagicMock() + + def _run(*, user_message, conversation_history, task_id): + db.create_session(task_id, source="api_server", model="test") + raise exc + + agent.run_conversation.side_effect = _run + agent.session_prompt_tokens = 0 + agent.session_completion_tokens = 0 + agent.session_total_tokens = 0 + agent.session_id = None + return agent + + +def _agent_nondict(value, db: SessionDB): + """Build an agent that returns ``value`` (a non-dict or a dict without + ``final_response``) so the runs handler must route it as a failure.""" + agent = MagicMock() + + def _run(*, user_message, conversation_history, task_id): + db.create_session(task_id, source="api_server", model="test") + return value + + agent.run_conversation.side_effect = _run + agent.session_prompt_tokens = 0 + agent.session_completion_tokens = 0 + agent.session_total_tokens = 0 + agent.session_id = None + return agent + + +def _create_runs_app(adapter: APIServerAdapter) -> web.Application: + app = web.Application() + app["api_server_adapter"] = adapter + app.router.add_post("/v1/runs", adapter._handle_runs) + app.router.add_get("/v1/runs/{run_id}", adapter._handle_get_run) + return app + + +async def _wait_for_terminal_run(client: TestClient, run_id: str) -> dict: + for _ in range(40): + response = await client.get(f"/v1/runs/{run_id}") + status = await response.json() + if status["status"] in {"completed", "failed", "cancelled"}: + return status + await asyncio.sleep(0.05) + raise AssertionError(f"run {run_id} did not reach a terminal status") + + +@pytest.mark.asyncio +async def test_runs_structured_failure_deletes_empty_session(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + agent = _agent_result({"failed": True, "error": "upstream rejected"}, db) + app = _create_runs_app(adapter) + + async with TestClient(TestServer(app)) as client: + with patch.object(adapter, "_create_agent", return_value=agent): + response = await client.post( + "/v1/runs", + json={"input": "hello", "session_id": "runs-failed-empty"}, + ) + run_id = (await response.json())["run_id"] + status = await _wait_for_terminal_run(client, run_id) + + assert status["status"] == "failed" + assert db.get_session("runs-failed-empty") is None + + +@pytest.mark.asyncio +async def test_runs_exception_deletes_empty_session(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + agent = _agent_exception(RuntimeError("boom"), db) + app = _create_runs_app(adapter) + + async with TestClient(TestServer(app)) as client: + with patch.object(adapter, "_create_agent", return_value=agent): + response = await client.post( + "/v1/runs", + json={"input": "hello", "session_id": "runs-exception-empty"}, + ) + run_id = (await response.json())["run_id"] + status = await _wait_for_terminal_run(client, run_id) + + assert status["status"] == "failed" + assert db.get_session("runs-exception-empty") is None + + +@pytest.mark.asyncio +async def test_runs_cooperative_stop_deletes_empty_session(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + # The agent returns a "would have been done" dict, but we mark the + # run for cooperative stop BEFORE its executor result is observed. + # The post-stop branch must route through the same prune path that + # covers the cancellation and exception branches. + agent = _agent_result({"final_response": "would have been done"}, db) + app = _create_runs_app(adapter) + + async with TestClient(TestServer(app)) as client: + with patch.object(adapter, "_create_agent", return_value=agent): + response = await client.post( + "/v1/runs", + json={"input": "hello", "session_id": "runs-coop-stop-empty"}, + ) + run_id = (await response.json())["run_id"] + adapter._stopping_run_ids.add(run_id) + status = await _wait_for_terminal_run(client, run_id) + + assert status["status"] == "cancelled" + assert db.get_session("runs-coop-stop-empty") is None + + +@pytest.mark.asyncio +async def test_runs_nondict_result_treated_as_failure(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + # The agent returns None — neither a dict with "failed" nor a dict + # with "final_response". The runs handler must NOT classify this as + # a successful completion. + agent = _agent_nondict(None, db) + app = _create_runs_app(adapter) + + async with TestClient(TestServer(app)) as client: + with patch.object(adapter, "_create_agent", return_value=agent): + response = await client.post( + "/v1/runs", + json={"input": "hello", "session_id": "runs-nondict-empty"}, + ) + run_id = (await response.json())["run_id"] + status = await _wait_for_terminal_run(client, run_id) + + assert status["status"] == "failed" + assert db.get_session("runs-nondict-empty") is None + + +@pytest.mark.asyncio +async def test_run_agent_structured_failure_deletes_empty_session_off_event_loop(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + agent = _agent_result({"failed": True, "error": "upstream rejected"}, db) + + with patch.object(adapter, "_create_agent", return_value=agent), patch( + "gateway.platforms.api_server.asyncio.to_thread", + wraps=asyncio.to_thread, + ) as to_thread: + result, _usage = await adapter._run_agent( + "hello", [], session_id="failed-empty" + ) + + assert result["failed"] is True + assert db.get_session("failed-empty") is None + cleanup_calls = [ + call + for call in to_thread.call_args_list + if call.args and call.args[0] == db.delete_session_if_empty + ] + assert cleanup_calls + assert cleanup_calls[0].args[1] == "failed-empty" + # The patched offloader executes its callable on a worker thread; verify + # that the SQLite cleanup did not run on aiohttp's event-loop thread. + assert cleanup_calls[0].args[0].__self__ is db + + +@pytest.mark.asyncio +async def test_run_agent_failure_preserves_session_with_resumable_content(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + agent = _agent_result( + {"failed": True, "error": "failed after persistence"}, + db, + persist_message=True, + ) + + with patch.object(adapter, "_create_agent", return_value=agent): + await adapter._run_agent("hello", [], session_id="failed-with-content") + + assert db.get_session("failed-with-content") is not None + + +@pytest.mark.asyncio +async def test_run_agent_success_preserves_empty_resumable_session(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + agent = _agent_result({"final_response": "done"}, db) + + with patch.object(adapter, "_create_agent", return_value=agent): + await adapter._run_agent("hello", [], session_id="successful-empty") + + assert db.get_session("successful-empty") is not None + + +@pytest.mark.asyncio +async def test_prune_isolation_two_profiles_do_not_cross_delete(tmp_path): + """A failure on profile A must not delete a row on profile B. + + Riker's C1 finding. Verify that the per-profile SessionDB cache + resolves the right home on the loop thread before the async prune + runs, and that the to_thread offloader does not get the wrong DB. + """ + import os + import sqlite3 + + home_a = tmp_path / "home_a" + home_b = tmp_path / "home_b" + for h in (home_a, home_b): + h.mkdir(parents=True) + + db_a = SessionDB(db_path=home_a / "state.db") + db_b = SessionDB(db_path=home_b / "state.db") + adapter_a = _adapter_with_db(db_a) + adapter_b = _adapter_with_db(db_b) + + # Pre-populate profile B with a row sharing the SAME id we will fail + # on profile A. If the prune were to touch the wrong DB, this row + # would be deleted. With correct isolation, it must survive. + db_b.create_session( + "iso-a", + source="api_server", + model="test", + ) + + with patch.dict(os.environ, {"HERMES_HOME": str(home_a)}): + try: + from hermes_constants import get_hermes_home + _ = get_hermes_home() + except Exception: + pass + + agent = _agent_result({"failed": True, "error": "boom"}, db_a) + with patch.object(adapter_a, "_create_agent", return_value=agent), \ + patch.dict(os.environ, {"HERMES_HOME": str(home_a)}): + try: + await adapter_a._run_agent("x", [], session_id="iso-a") + except RuntimeError: + pass + + # Profile A: failed row must be gone. + assert db_a.get_session("iso-a") is None + # Profile B: pre-existing row with the same id must still exist. + assert db_b.get_session("iso-a") is not None + # Sanity: there is no row in home_b's DB with the exact same name that + # was produced by profile A — only the one we pre-populated. + conn = sqlite3.connect(str(home_b / "state.db")) + rows = conn.execute("SELECT id FROM sessions").fetchall() + assert [r[0] for r in rows] == ["iso-a"] + + +@pytest.mark.asyncio +async def test_prune_preserves_concurrent_message_flush(tmp_path): + """If a message flush lands between failure detection and the prune + call, the row MUST survive because it is no longer empty. + + The SQL guard inside ``delete_session_if_empty`` is single-statement + and atomic, so the realistic race is: a parallel writer appends a + message after the failure is observed but before the prune runs. + The test simulates that with a thread that signals at the right + moment. The row should still exist when the test ends. + """ + import threading + + db = SessionDB(db_path=tmp_path / "state.db") + adapter = _adapter_with_db(db) + agent = _agent_result({"failed": True, "error": "boom"}, db) + + # Start the agent in the same call we are about to exercise; instead + # we will simulate the race deterministically: the agent creates the + # row, then a parallel thread appends a message before the prune + # call has a chance to commit. The production code must not lose + # the row. + flush_started = threading.Event() + proceed_to_prune = threading.Event() + + def flusher(): + db.create_session("race-a", source="api_server", model="test") + flush_started.set() + proceed_to_prune.wait(timeout=5) + db.append_message("race-a", role="user", content="late flush") + + t = threading.Thread(target=flusher, daemon=True) + t.start() + flush_started.wait(timeout=5) + + with patch.object(adapter, "_create_agent", return_value=agent): + proceed_to_prune.set() + try: + await adapter._run_agent("x", [], session_id="race-a") + except RuntimeError: + pass + + t.join(timeout=5) + # Row must still exist (a message was flushed before the prune ran) + row = db.get_session("race-a") + assert row is not None diff --git a/tests/tools/test_browser_background_tab.py b/tests/tools/test_browser_background_tab.py new file mode 100644 index 0000000000000..8196711351c4e --- /dev/null +++ b/tests/tools/test_browser_background_tab.py @@ -0,0 +1,62 @@ +"""Regression tests for browser CDP target creation without focus theft.""" + +import asyncio +import json + + +def test_supervisor_creates_background_target(monkeypatch): + from tools.browser_supervisor import CDPSupervisor + + calls = [] + supervisor = object.__new__(CDPSupervisor) + supervisor._page_session_id = None + + async def fake_cdp(method, params=None, **kwargs): + calls.append({"method": method, "params": params, "kwargs": kwargs}) + if method == "Target.getTargets": + return {"result": {"targetInfos": []}} + if method == "Target.createTarget": + return {"result": {"targetId": "synthetic-target"}} + if method == "Target.attachToTarget": + return {"result": {"sessionId": "synthetic-session"}} + return {"result": {}} + + async def fake_dialog_bridge(session_id): + calls.append({"method": "_install_dialog_bridge", "session_id": session_id}) + + monkeypatch.setattr(supervisor, "_cdp", fake_cdp) + monkeypatch.setattr(supervisor, "_install_dialog_bridge", fake_dialog_bridge) + asyncio.run(supervisor._attach_initial_page()) + + create = next(call for call in calls if call["method"] == "Target.createTarget") + assert (create["params"] or {}).get("background") is True + + +def test_raw_browser_cdp_creates_background_target(monkeypatch): + import tools.browser_cdp_tool as tool + + calls = {} + + async def fake_call(endpoint, method, params, target_id, timeout): + calls.update( + { + "endpoint": endpoint, + "method": method, + "params": dict(params), + "target_id": target_id, + "timeout": timeout, + } + ) + return {"targetId": "synthetic-target"} + + monkeypatch.setattr(tool, "_WS_AVAILABLE", True) + monkeypatch.setattr(tool, "_resolve_cdp_endpoint", lambda: "ws://synthetic-endpoint") + monkeypatch.setattr(tool, "_browser_cdp_private_guard", lambda **kwargs: None) + monkeypatch.setattr(tool, "_cdp_call", fake_call) + monkeypatch.setattr(tool, "_run_async", lambda coroutine: asyncio.run(coroutine)) + + result = tool.browser_cdp("Target.createTarget", {"url": "about:blank"}) + + payload = json.loads(result) + assert payload["result"]["targetId"] == "synthetic-target" + assert calls["params"]["background"] is True diff --git a/tools/browser_cdp_tool.py b/tools/browser_cdp_tool.py index eccd8f8fc1c56..f901fd436c7d9 100644 --- a/tools/browser_cdp_tool.py +++ b/tools/browser_cdp_tool.py @@ -489,6 +489,21 @@ def browser_cdp( if blocked: return blocked + # --- Background-tab enforcement (2026-08-02 operator directive) ------ + # The CDP browser on the Windows host is shared infrastructure. A + # Target.createTarget WITHOUT background:True activates the new tab and + # raises the Chrome window, stealing focus from the user's desktop. This + # raw tool is an agent escape hatch, so enforce the background flag here + # rather than relying on every agent to remember it. See 3xstanbrain + # 2026-06-03 cdp-windows-focus-steal and the 2026-08-02 enforcement pass. + if method == "Target.createTarget": + if not isinstance(call_params, dict): + return tool_error( + "Target.createTarget params must be an object/dict", + method=method, + ) + call_params["background"] = True + try: safe_timeout = float(timeout) if timeout else 30.0 except (TypeError, ValueError): diff --git a/tools/browser_supervisor.py b/tools/browser_supervisor.py index d17806f4b2ada..4ea893f276d45 100644 --- a/tools/browser_supervisor.py +++ b/tools/browser_supervisor.py @@ -744,7 +744,15 @@ async def _attach_initial_page(self) -> None: targets = resp.get("result", {}).get("targetInfos", []) page_target = next((t for t in targets if t.get("type") == "page"), None) if page_target is None: - created = await self._cdp("Target.createTarget", {"url": "about:blank"}) + # Create the initial tab in the BACKGROUND so the browser window + # is never raised on the user's desktop (Windows focus steal). + # See 3xstanbrain 2026-06-03 cdp-windows-focus-steal + 2026-08-02 + # enforcement pass: every Target.createTarget MUST pass + # background: True unless a task explicitly needs foreground. + created = await self._cdp( + "Target.createTarget", + {"url": "about:blank", "background": True}, + ) target_id = created["result"]["targetId"] else: target_id = page_target["targetId"]