From 6d4ca15d38fe388813cfed4a139cb2e36efe5f6f Mon Sep 17 00:00:00 2001 From: salch-cred Date: Sun, 13 Sep 2026 09:36:04 +0530 Subject: [PATCH 01/96] fix(agent): mirror Claude Code OAuth refresh into the macOS Keychain (#98334) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On macOS the Keychain is Claude Code's authoritative credential store, but Hermes only ever wrote ~/.claude/.credentials.json. Since the refresh token is single-use and rotating, every Hermes-initiated refresh left the Keychain holding an already-invalidated token, which Claude Code then spent into invalid_grant and discarded ("Login: Expired"). _write_claude_code_credentials now mirrors the committed refresh into the existing "Claude Code-credentials" entry via security add-generic-password, merging the rotated token triple over the existing payload so subscriptionType / rateLimitTier / scopes survive. The payload is fed on stdin (bare -w), never argv. Fail-soft: a mirror failure is logged, never raised — the file commit already succeeded and the resolver resolves from it. No-op off Darwin and when no entry exists (never create one the user has not). Add a raw payload reader (metadata preserved), a pure merge helper, and the mirror; extend the conftest keychain guard to neutralize the new writer in any test that hasn't opted in. --- agent/anthropic_credentials.py | 91 ++++++++++++++++- tests/agent/test_anthropic_keychain.py | 135 ++++++++++++++++++++++++- tests/conftest.py | 8 ++ 3 files changed, 228 insertions(+), 6 deletions(-) diff --git a/agent/anthropic_credentials.py b/agent/anthropic_credentials.py index b03aee1b86..5956361384 100644 --- a/agent/anthropic_credentials.py +++ b/agent/anthropic_credentials.py @@ -12,6 +12,7 @@ re-reads them on every ``load_pool()``, so a failed write here is a failed refre import base64 import contextlib import functools +import getpass import hashlib import json import logging @@ -41,6 +42,10 @@ _OAUTH_TOKEN_URLS = [ _OAUTH_TOKEN_USER_AGENT = "axios/1.7.9" _OAUTH_REDIRECT_URI = "https://console.anthropic.com/oauth/code/callback" _OAUTH_SCOPES = "org:create_api_key user:profile user:inference" +# Claude Code's macOS Keychain entry (generic password). Hermes reads it +# (_read_claude_code_credentials_from_keychain) and, since #98334, mirrors the +# refresh write into it so the two stores stop diverging on a single-use rotation. +_CLAUDE_CODE_KEYCHAIN_SERVICE = "Claude Code-credentials" def _getenv(name: str, default: str = "") -> str: @@ -204,27 +209,41 @@ def _claude_oauth_record(data: Any, source: str) -> Optional[Dict[str, Any]]: } -def _read_claude_code_credentials_from_keychain() -> Optional[Dict[str, Any]]: - """Read the "Claude Code-credentials" macOS Keychain entry (Claude Code >=2.1.114).""" +def _read_claude_code_keychain_payload() -> Optional[Dict[str, Any]]: + """Raw ``{"claudeAiOauth": {...}, ...}`` payload from the macOS Keychain, or None. + + Returns the full entry (not the normalised credential record) so a refresh + write can merge the rotated token triple over the existing metadata + (``subscriptionType`` / ``rateLimitTier`` / ``scopes``) instead of clobbering it. + """ if platform.system() != "Darwin": return None try: result = subprocess.run( - ["security", "find-generic-password", "-s", "Claude Code-credentials", "-w"], + ["security", "find-generic-password", "-s", _CLAUDE_CODE_KEYCHAIN_SERVICE, "-w"], capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=5, stdin=subprocess.DEVNULL, ) except (OSError, subprocess.TimeoutExpired): logger.debug("Keychain: security command not available or timed out") return None if result.returncode != 0: - logger.debug("Keychain: no entry found for 'Claude Code-credentials'") + logger.debug("Keychain: no entry found for %r", _CLAUDE_CODE_KEYCHAIN_SERVICE) return None raw = result.stdout.strip() + if not raw: + return None try: - return _claude_oauth_record(json.loads(raw), "macos_keychain") if raw else None + payload = json.loads(raw) except json.JSONDecodeError: logger.debug("Keychain: credentials payload is not valid JSON") return None + return payload if isinstance(payload, dict) else None + + +def _read_claude_code_credentials_from_keychain() -> Optional[Dict[str, Any]]: + """Read the "Claude Code-credentials" macOS Keychain entry (Claude Code >=2.1.114).""" + payload = _read_claude_code_keychain_payload() + return _claude_oauth_record(payload, "macos_keychain") if payload else None def claude_code_credentials_path() -> Path: @@ -441,6 +460,68 @@ def _write_claude_code_credentials( oauth_data["scopes"] = existing["claudeAiOauth"]["scopes"] existing["claudeAiOauth"] = oauth_data _commit_private_json(cred_path, existing, "credentials") + _mirror_claude_code_credentials_to_keychain(access_token, refresh_token, expires_at_ms) + + +def _merge_keychain_credential_payload( + existing_payload: Dict[str, Any], access_token: str, refresh_token: str, expires_at_ms: int +) -> Dict[str, Any]: + """Rotate the ``claudeAiOauth`` token triple over the existing Keychain payload, + preserving its metadata (``subscriptionType`` / ``rateLimitTier`` / ``scopes``). + + Pure and host-agnostic so the merge semantics are unit-testable without a Keychain. + """ + merged = dict(existing_payload) + oauth = dict(existing_payload.get("claudeAiOauth") or {}) + oauth.update({"accessToken": access_token, "refreshToken": refresh_token, "expiresAt": expires_at_ms}) + merged["claudeAiOauth"] = oauth + return merged + + +def _mirror_claude_code_credentials_to_keychain( + access_token: str, refresh_token: str, expires_at_ms: int +) -> None: + """Mirror a committed refresh into the macOS Keychain when an entry already exists. + + On Darwin the Keychain is Claude Code's authoritative store, but Hermes historically + only wrote ``~/.claude/.credentials.json``. Because the refresh token is single-use and + rotating, that left the Keychain holding an already-invalidated token, which Claude Code + then spent into ``invalid_grant`` and discarded (``Login: Expired``). Mirror the rotated + pair back with ``security add-generic-password -U`` so both stores agree. + + Fail-soft: a Keychain mirror failure is logged, never raised. The file commit has already + succeeded and the resolver still resolves from it; only the secondary store stays stale. + No-op off Darwin and when no entry exists (we never create one the user has not). + """ + if platform.system() != "Darwin": + return + try: + existing = _read_claude_code_keychain_payload() + if not existing: + return + payload = _merge_keychain_credential_payload(existing, access_token, refresh_token, expires_at_ms) + encoded = json.dumps(payload) + # The bare ``-w`` (no value) tells ``security`` to read the password from + # stdin, so the live secret never lands on argv (process-table visible). + result = subprocess.run( + [ + "security", "add-generic-password", "-U", + "-a", getpass.getuser(), + "-s", _CLAUDE_CODE_KEYCHAIN_SERVICE, + "-w", + ], + input=encoded, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=10, + ) + except (OSError, subprocess.TimeoutExpired) as e: + logger.debug("Keychain mirror skipped (%s)", e) + return + if result.returncode != 0: + logger.debug("Keychain mirror failed (rc=%s): %s", result.returncode, (result.stderr or "").strip()[:200]) # ── Resolution ── diff --git a/tests/agent/test_anthropic_keychain.py b/tests/agent/test_anthropic_keychain.py index 3b7cfffeca..69c333cd56 100644 --- a/tests/agent/test_anthropic_keychain.py +++ b/tests/agent/test_anthropic_keychain.py @@ -1,13 +1,21 @@ """Tests for Bug #12905 fixes in agent/anthropic_adapter.py — macOS Keychain support.""" import json +import platform +import subprocess import threading import time from unittest.mock import patch, MagicMock import pytest -from agent.anthropic_credentials import _read_claude_code_credentials_from_keychain, read_claude_code_credentials, _refresh_oauth_token +from agent.anthropic_credentials import ( + _read_claude_code_credentials_from_keychain, + read_claude_code_credentials, + _refresh_oauth_token, + _merge_keychain_credential_payload, + _mirror_claude_code_credentials_to_keychain, +) # This module exercises the reader itself with explicit platform and subprocess @@ -343,3 +351,128 @@ class TestRefreshOAuthTokenAdoptsFreshCredential: assert results == {"a": "fresh-access", "b": "fresh-access"} assert calls == ["stale-refresh"], calls + +class TestMergeKeychainCredentialPayload: + """``_merge_keychain_credential_payload`` — the pure merge that a Keychain + refresh write performs over the existing entry. Host-agnostic, so it runs + on every lane and pins the #98334 invariant: rotate the token triple while + preserving the metadata Claude Code gates on.""" + + _EXISTING = { + "claudeAiOauth": { + "accessToken": "old-access", + "refreshToken": "old-refresh", + "expiresAt": 1, + "scopes": ["user:inference", "user:profile"], + "subscriptionType": "max", + }, + "rateLimitTier": "tier-1", + } + + def test_rotates_triple_preserves_metadata(self): + merged = _merge_keychain_credential_payload(self._EXISTING, "new-access", "new-refresh", 42) + oauth = merged["claudeAiOauth"] + assert oauth["accessToken"] == "new-access" + assert oauth["refreshToken"] == "new-refresh" + assert oauth["expiresAt"] == 42 + # The fields Claude Code >=2.1.81 gates on survive the merge. + assert oauth["scopes"] == ["user:inference", "user:profile"] + assert oauth["subscriptionType"] == "max" + assert merged["rateLimitTier"] == "tier-1" + # Input payload is not mutated (no aliasing surprise). + assert self._EXISTING["claudeAiOauth"]["refreshToken"] == "old-refresh" + + def test_tolerates_missing_oauth_block(self): + merged = _merge_keychain_credential_payload({"other": 1}, "a", "b", 7) + assert merged["claudeAiOauth"] == {"accessToken": "a", "refreshToken": "b", "expiresAt": 7} + assert merged["other"] == 1 + + +class TestMirrorClaudeCodeCredentialsToKeychain: + """``_mirror_claude_code_credentials_to_keychain`` — the #98334 write mirror. + + The write path is gated on ``platform.system() == "Darwin"`` and shells out + to ``security``. We force the gate to "Darwin" so the logic runs on every + lane, and mock only the raw reader and ``subprocess.run`` — no real Keychain + is ever touched and no real ``security`` binary is required. + """ + + @pytest.fixture(autouse=True) + def _darwin_gate(self, monkeypatch): + monkeypatch.setattr(platform, "system", lambda: "Darwin") + + def test_writes_via_stdin_not_argv_when_entry_exists(self, monkeypatch): + """The rotated pair must reach the existing Keychain item via + ``add-generic-password -U`` with the payload on stdin — never as a + ``-w`` argv token (a live secret would be visible in the process table).""" + existing = { + "claudeAiOauth": {"accessToken": "old", "refreshToken": "old-ref", + "expiresAt": 1, "scopes": ["user:inference"], + "subscriptionType": "max"}, + } + monkeypatch.setattr( + "agent.anthropic_credentials._read_claude_code_keychain_payload", lambda: existing) + calls = [] + + def fake_run(argv, **kwargs): + calls.append((list(argv), kwargs)) + return MagicMock(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(subprocess, "run", fake_run) + + _mirror_claude_code_credentials_to_keychain("new-access", "new-ref", 99) + + assert len(calls) == 1 + argv, kwargs = calls[0] + assert "add-generic-password" in argv + assert "-U" in argv + assert "-s" in argv and "Claude Code-credentials" in argv + # The bare -w flag reads the password from stdin, so the secret must NOT + # appear anywhere on the command line. + assert "-w" in argv + joined = " ".join(argv) + assert "new-access" not in joined + assert "new-ref" not in joined + # The payload goes to stdin and carries the rotated triple + preserved metadata. + payload = json.loads(kwargs["input"]) + assert payload["claudeAiOauth"]["refreshToken"] == "new-ref" + assert payload["claudeAiOauth"]["accessToken"] == "new-access" + assert payload["claudeAiOauth"]["subscriptionType"] == "max" + + def test_no_write_when_no_entry_exists(self, monkeypatch): + """Never create a Keychain item the user has not.""" + monkeypatch.setattr( + "agent.anthropic_credentials._read_claude_code_keychain_payload", lambda: None) + called = [] + monkeypatch.setattr(subprocess, "run", lambda *a, **k: called.append(a)) + + _mirror_claude_code_credentials_to_keychain("a", "b", 1) + + assert called == [] + + def test_fail_soft_when_security_raises(self, monkeypatch): + """A mirror failure is logged, never raised: the file commit already + succeeded and the resolver still resolves from it.""" + monkeypatch.setattr( + "agent.anthropic_credentials._read_claude_code_keychain_payload", + lambda: {"claudeAiOauth": {"accessToken": "x"}}) + + def boom(*a, **k): + raise OSError("security not available") + + monkeypatch.setattr(subprocess, "run", boom) + + # Must not raise. + _mirror_claude_code_credentials_to_keychain("a", "b", 1) + + def test_fail_soft_on_nonzero_exit(self, monkeypatch): + monkeypatch.setattr( + "agent.anthropic_credentials._read_claude_code_keychain_payload", + lambda: {"claudeAiOauth": {"accessToken": "x"}}) + monkeypatch.setattr( + subprocess, "run", + lambda *a, **k: MagicMock(returncode=1, stdout="", stderr="duplicate item")) + + # Must not raise. + _mirror_claude_code_credentials_to_keychain("a", "b", 1) + diff --git a/tests/conftest.py b/tests/conftest.py index b25f8a3bff..5c647e59d7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -729,6 +729,14 @@ def _neutralize_macos_keychain_creds(request, monkeypatch): lambda *_args, **_kwargs: None, raising=False, ) + # The #98334 refresh write also mirrors into the Keychain; keep that out of + # the real store in any test that hasn't explicitly opted in. + monkeypatch.setattr( + _mod, + "_mirror_claude_code_credentials_to_keychain", + lambda *_args, **_kwargs: None, + raising=False, + ) return None From bf5a6f6ae6b4fc9aec05245ecf4018de564a5aeb Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:02:04 +0530 Subject: [PATCH 02/96] fix(anthropic): mirror the Keychain item through `security -i` with a hex payload, under its own account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the cherry-picked mechanism, both found live on macOS: * `add-generic-password -w` with no value prompts on /dev/tty when a terminal exists, so the CLI refresh path hung for the 10 s timeout and wrote nothing; with no terminal it read one line from stdin, hit EOF on the confirmation read and stored an EMPTY password — bricking the very item this exists to keep fresh. The command line now goes to `security -i` on stdin with the payload hex-encoded (`-X`): no argv, no tty prompt, no quoting of the JSON. * `-a getpass.getuser()` assumed the item's account is the login user; `-U` matches on account + service, so a mismatch would have created a second item Claude Code never reads. The account is read from the existing item's `acct` attribute and the mirror is skipped without one. The command builder is a pure function so the no-argv / hex / account contract is tested on every lane; the Darwin-gated no-entry no-op test is `macos_only` instead of faking `platform.system`. Tests trimmed to the invariant bar; the merge test pins that `mcpOAuth` siblings survive. Live E2E on a throwaway Keychain item seeded with 24 mcpOAuth entries: triple rotated, scopes/subscriptionType and all 24 siblings preserved, one item, updated under the seeded account. --- agent/anthropic_credentials.py | 57 ++++++---- tests/agent/test_anthropic_keychain.py | 151 +++++++------------------ 2 files changed, 81 insertions(+), 127 deletions(-) diff --git a/agent/anthropic_credentials.py b/agent/anthropic_credentials.py index 5956361384..ef43cf6d0c 100644 --- a/agent/anthropic_credentials.py +++ b/agent/anthropic_credentials.py @@ -12,12 +12,12 @@ re-reads them on every ``load_pool()``, so a failed write here is a failed refre import base64 import contextlib import functools -import getpass import hashlib import json import logging import os import platform +import re import secrets import subprocess import threading @@ -240,6 +240,37 @@ def _read_claude_code_keychain_payload() -> Optional[Dict[str, Any]]: return payload if isinstance(payload, dict) else None +def _claude_code_keychain_account() -> str: + """The ``acct`` attribute of the existing Keychain item (``""`` when unreadable). + + ``add-generic-password -U`` matches on account AND service; writing under a different + account would create a second item instead of updating the one Claude Code reads. + """ + try: + result = subprocess.run( + ["security", "find-generic-password", "-s", _CLAUDE_CODE_KEYCHAIN_SERVICE], + capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=5, stdin=subprocess.DEVNULL, + ) + except (OSError, subprocess.TimeoutExpired): + return "" + match = re.search(r'"acct"="((?:[^"\\]|\\.)*)"', result.stdout) if result.returncode == 0 else None + return match.group(1) if match else "" + + +def _keychain_mirror_command(account: str, payload: Dict[str, Any]) -> tuple[list[str], str]: + """``(argv, stdin)`` that updates the Claude Code Keychain item with ``payload``. + + The command line goes to ``security -i`` on stdin, with the secret hex-encoded (``-X``): + a bare ``-w`` prompts twice on /dev/tty when a terminal exists (hangs the CLI) and, with + no terminal, reads only the first line and stores an EMPTY password when the confirmation + read hits EOF — either way the live token must never sit on argv. + """ + quoted = lambda v: '"' + v.replace("\\", "\\\\").replace('"', '\\"') + '"' # noqa: E731 - security -i tokenizer + encoded = json.dumps(payload, separators=(",", ":")).encode("utf-8").hex() + line = f"add-generic-password -U -a {quoted(account)} -s {quoted(_CLAUDE_CODE_KEYCHAIN_SERVICE)} -X {encoded}\n" + return ["security", "-i"], line + + def _read_claude_code_credentials_from_keychain() -> Optional[Dict[str, Any]]: """Read the "Claude Code-credentials" macOS Keychain entry (Claude Code >=2.1.114).""" payload = _read_claude_code_keychain_payload() @@ -487,7 +518,7 @@ def _mirror_claude_code_credentials_to_keychain( only wrote ``~/.claude/.credentials.json``. Because the refresh token is single-use and rotating, that left the Keychain holding an already-invalidated token, which Claude Code then spent into ``invalid_grant`` and discarded (``Login: Expired``). Mirror the rotated - pair back with ``security add-generic-password -U`` so both stores agree. + pair back with ``add-generic-password -U`` (through ``security -i``) so both stores agree. Fail-soft: a Keychain mirror failure is logged, never raised. The file commit has already succeeded and the resolver still resolves from it; only the secondary store stays stale. @@ -497,25 +528,13 @@ def _mirror_claude_code_credentials_to_keychain( return try: existing = _read_claude_code_keychain_payload() - if not existing: + account = _claude_code_keychain_account() if existing else "" + if not existing or not account: return - payload = _merge_keychain_credential_payload(existing, access_token, refresh_token, expires_at_ms) - encoded = json.dumps(payload) - # The bare ``-w`` (no value) tells ``security`` to read the password from - # stdin, so the live secret never lands on argv (process-table visible). + argv, line = _keychain_mirror_command( + account, _merge_keychain_credential_payload(existing, access_token, refresh_token, expires_at_ms)) result = subprocess.run( - [ - "security", "add-generic-password", "-U", - "-a", getpass.getuser(), - "-s", _CLAUDE_CODE_KEYCHAIN_SERVICE, - "-w", - ], - input=encoded, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - timeout=10, + argv, input=line, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=10, ) except (OSError, subprocess.TimeoutExpired) as e: logger.debug("Keychain mirror skipped (%s)", e) diff --git a/tests/agent/test_anthropic_keychain.py b/tests/agent/test_anthropic_keychain.py index 69c333cd56..50b67547c8 100644 --- a/tests/agent/test_anthropic_keychain.py +++ b/tests/agent/test_anthropic_keychain.py @@ -13,6 +13,7 @@ from agent.anthropic_credentials import ( _read_claude_code_credentials_from_keychain, read_claude_code_credentials, _refresh_oauth_token, + _keychain_mirror_command, _merge_keychain_credential_payload, _mirror_claude_code_credentials_to_keychain, ) @@ -353,126 +354,60 @@ class TestRefreshOAuthTokenAdoptsFreshCredential: class TestMergeKeychainCredentialPayload: - """``_merge_keychain_credential_payload`` — the pure merge that a Keychain - refresh write performs over the existing entry. Host-agnostic, so it runs - on every lane and pins the #98334 invariant: rotate the token triple while - preserving the metadata Claude Code gates on.""" + """``_merge_keychain_credential_payload`` — the pure merge a Keychain refresh write + performs over the existing entry (#98334): rotate the token triple, keep everything + Claude Code stores beside it.""" - _EXISTING = { - "claudeAiOauth": { - "accessToken": "old-access", - "refreshToken": "old-refresh", - "expiresAt": 1, - "scopes": ["user:inference", "user:profile"], - "subscriptionType": "max", - }, - "rateLimitTier": "tier-1", - } - - def test_rotates_triple_preserves_metadata(self): - merged = _merge_keychain_credential_payload(self._EXISTING, "new-access", "new-refresh", 42) + def test_rotates_triple_and_keeps_every_sibling(self): + existing = { + "claudeAiOauth": { + "accessToken": "old-access", "refreshToken": "old-refresh", "expiresAt": 1, + "scopes": ["user:inference", "user:profile"], "subscriptionType": "max", + }, + "rateLimitTier": "tier-1", + # Claude Code keeps its MCP server OAuth tokens in the same item; a refresh that + # dropped them would log the user out of every MCP server at once. + "mcpOAuth": {f"srv{i}": {"accessToken": f"t{i}"} for i in range(24)}, + } + merged = _merge_keychain_credential_payload(existing, "new-access", "new-refresh", 42) oauth = merged["claudeAiOauth"] - assert oauth["accessToken"] == "new-access" - assert oauth["refreshToken"] == "new-refresh" - assert oauth["expiresAt"] == 42 - # The fields Claude Code >=2.1.81 gates on survive the merge. - assert oauth["scopes"] == ["user:inference", "user:profile"] - assert oauth["subscriptionType"] == "max" - assert merged["rateLimitTier"] == "tier-1" - # Input payload is not mutated (no aliasing surprise). - assert self._EXISTING["claudeAiOauth"]["refreshToken"] == "old-refresh" + assert (oauth["accessToken"], oauth["refreshToken"], oauth["expiresAt"]) == ("new-access", "new-refresh", 42) + # Everything except the rotated triple is byte-identical to the input. + assert {k: v for k, v in oauth.items() if k not in ("accessToken", "refreshToken", "expiresAt")} == { + "scopes": ["user:inference", "user:profile"], "subscriptionType": "max"} + assert {k: v for k, v in merged.items() if k != "claudeAiOauth"} == { + k: v for k, v in existing.items() if k != "claudeAiOauth"} + assert existing["claudeAiOauth"]["refreshToken"] == "old-refresh" # input not mutated - def test_tolerates_missing_oauth_block(self): - merged = _merge_keychain_credential_payload({"other": 1}, "a", "b", 7) - assert merged["claudeAiOauth"] == {"accessToken": "a", "refreshToken": "b", "expiresAt": 7} - assert merged["other"] == 1 + +class TestKeychainMirrorCommand: + """``_keychain_mirror_command`` — host-agnostic: it builds the ``security`` invocation + without running it.""" + + def test_secret_travels_hex_encoded_on_stdin_under_the_items_own_account(self): + payload = {"claudeAiOauth": {"accessToken": "sk-ant-oat01-new", "refreshToken": 'r"q\\x'}, "mcpOAuth": {"a": 1}} + argv, line = _keychain_mirror_command("alice smith", payload) + + # ``security -i`` reads the command from stdin: nothing secret on argv. + assert argv == ["security", "-i"] + assert "sk-ant-oat01-new" not in line and "-w" not in line.split() + # -U updates the item Claude Code reads (matched on account + service), never a second one. + assert line.startswith('add-generic-password -U -a "alice smith" -s "Claude Code-credentials" -X ') + hex_blob = line.split(" -X ", 1)[1].strip() + assert json.loads(bytes.fromhex(hex_blob)) == payload class TestMirrorClaudeCodeCredentialsToKeychain: - """``_mirror_claude_code_credentials_to_keychain`` — the #98334 write mirror. - - The write path is gated on ``platform.system() == "Darwin"`` and shells out - to ``security``. We force the gate to "Darwin" so the logic runs on every - lane, and mock only the raw reader and ``subprocess.run`` — no real Keychain - is ever touched and no real ``security`` binary is required. - """ - - @pytest.fixture(autouse=True) - def _darwin_gate(self, monkeypatch): - monkeypatch.setattr(platform, "system", lambda: "Darwin") - - def test_writes_via_stdin_not_argv_when_entry_exists(self, monkeypatch): - """The rotated pair must reach the existing Keychain item via - ``add-generic-password -U`` with the payload on stdin — never as a - ``-w`` argv token (a live secret would be visible in the process table).""" - existing = { - "claudeAiOauth": {"accessToken": "old", "refreshToken": "old-ref", - "expiresAt": 1, "scopes": ["user:inference"], - "subscriptionType": "max"}, - } - monkeypatch.setattr( - "agent.anthropic_credentials._read_claude_code_keychain_payload", lambda: existing) - calls = [] - - def fake_run(argv, **kwargs): - calls.append((list(argv), kwargs)) - return MagicMock(returncode=0, stdout="", stderr="") - - monkeypatch.setattr(subprocess, "run", fake_run) - - _mirror_claude_code_credentials_to_keychain("new-access", "new-ref", 99) - - assert len(calls) == 1 - argv, kwargs = calls[0] - assert "add-generic-password" in argv - assert "-U" in argv - assert "-s" in argv and "Claude Code-credentials" in argv - # The bare -w flag reads the password from stdin, so the secret must NOT - # appear anywhere on the command line. - assert "-w" in argv - joined = " ".join(argv) - assert "new-access" not in joined - assert "new-ref" not in joined - # The payload goes to stdin and carries the rotated triple + preserved metadata. - payload = json.loads(kwargs["input"]) - assert payload["claudeAiOauth"]["refreshToken"] == "new-ref" - assert payload["claudeAiOauth"]["accessToken"] == "new-access" - assert payload["claudeAiOauth"]["subscriptionType"] == "max" + """The #98334 write mirror shells out to ``security``; ``subprocess.run`` is mocked so no + real Keychain is touched.""" + @pytest.mark.macos_only def test_no_write_when_no_entry_exists(self, monkeypatch): """Never create a Keychain item the user has not.""" - monkeypatch.setattr( - "agent.anthropic_credentials._read_claude_code_keychain_payload", lambda: None) + monkeypatch.setattr("agent.anthropic_credentials._read_claude_code_keychain_payload", lambda: None) called = [] monkeypatch.setattr(subprocess, "run", lambda *a, **k: called.append(a)) _mirror_claude_code_credentials_to_keychain("a", "b", 1) assert called == [] - - def test_fail_soft_when_security_raises(self, monkeypatch): - """A mirror failure is logged, never raised: the file commit already - succeeded and the resolver still resolves from it.""" - monkeypatch.setattr( - "agent.anthropic_credentials._read_claude_code_keychain_payload", - lambda: {"claudeAiOauth": {"accessToken": "x"}}) - - def boom(*a, **k): - raise OSError("security not available") - - monkeypatch.setattr(subprocess, "run", boom) - - # Must not raise. - _mirror_claude_code_credentials_to_keychain("a", "b", 1) - - def test_fail_soft_on_nonzero_exit(self, monkeypatch): - monkeypatch.setattr( - "agent.anthropic_credentials._read_claude_code_keychain_payload", - lambda: {"claudeAiOauth": {"accessToken": "x"}}) - monkeypatch.setattr( - subprocess, "run", - lambda *a, **k: MagicMock(returncode=1, stdout="", stderr="duplicate item")) - - # Must not raise. - _mirror_claude_code_credentials_to_keychain("a", "b", 1) - From 14a346345486418aa85ed6526549412ee2665ade Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:19:47 +0530 Subject: [PATCH 03/96] fix(anthropic): mirror only the Keychain item that held the spent pair; parse both `security` attribute encodings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the mirror, all verified live against throwaway items: * Identity gate. The service name is fixed but the file path honours CLAUDE_CONFIG_DIR, and Claude Code rotates on its own schedule, so the item under the service can hold a different login's pair or a newer rotation. Overwriting it would be the bug in the other direction. The refresh token that was just POSTed is threaded through `_write_claude_code_credentials(spent_refresh_token=...)` from both callers (the singleton refresher and the pool commit) and the mirror only updates an item whose `refreshToken` equals it. * Attribute parsing. `security` prints an attribute as `"text"` when it is plain printable ASCII — UNescaped, an embedded `"` appears raw — and as `0x ""` otherwise. The old regex modelled `\"` escaping that never happens: a `"` in the account truncated it and the mirror created a second item; any non-ASCII byte made it return "" and the mirror silently no-oped. One `find-generic-password -g` call now yields account and payload together, parsed line-anchored in both encodings. * Fail-soft is now total (`except Exception`): the file commit already succeeded when the mirror runs, and a raise here made the refresher mark a landed rotation as consumed-uncommitted. * `quoted` lambda -> nested def; `ensure_ascii=True` made explicit since `-w` returns non-ASCII payloads as hex. Two pre-existing test doubles for the writer accept the new keyword. --- agent/anthropic_credentials.py | 110 ++++++++++++------ agent/credential_pool.py | 2 +- .../test_anthropic_borrowed_row_authority.py | 4 +- tests/agent/test_anthropic_keychain.py | 51 +++++++- tests/agent/test_auxiliary_client.py | 3 +- 5 files changed, 124 insertions(+), 46 deletions(-) diff --git a/agent/anthropic_credentials.py b/agent/anthropic_credentials.py index ef43cf6d0c..424b9cfa25 100644 --- a/agent/anthropic_credentials.py +++ b/agent/anthropic_credentials.py @@ -209,6 +209,52 @@ def _claude_oauth_record(data: Any, source: str) -> Optional[Dict[str, Any]]: } +_KEYCHAIN_ATTR = r'(?:0x(?P[0-9A-Fa-f]+)\b.*|"(?P.*)")' + + +def _decode_keychain_attr(match: Optional["re.Match[str]"]) -> str: + """``security`` prints an attribute as ``"text"`` when it is plain printable ASCII and as + ``0x ""`` otherwise; the quoted form is NOT escaped (an embedded + ``"`` appears raw), so the text group must run to the last quote on the line.""" + if match is None: + return "" + if match.group("hex"): + try: + return bytes.fromhex(match.group("hex")).decode("utf-8") + except ValueError: + return "" + return match.group("text") or "" + + +def _find_claude_code_keychain_item() -> Optional[tuple[str, Dict[str, Any]]]: + """``(account, payload)`` of the ``Claude Code-credentials`` login Keychain item, or None. + + One ``find-generic-password -g`` call: attributes on stdout, ``password: …`` on stderr. The + account matters because ``add-generic-password -U`` matches on account AND service — writing + under another account would create a second item instead of updating the one Claude Code reads. + """ + if platform.system() != "Darwin": + return None + try: + result = subprocess.run( + ["security", "find-generic-password", "-s", _CLAUDE_CODE_KEYCHAIN_SERVICE, "-g"], + capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=5, stdin=subprocess.DEVNULL, + ) + except (OSError, subprocess.TimeoutExpired): + return None + if result.returncode != 0: + return None + account = _decode_keychain_attr(re.search(r'^\s*"acct"=' + _KEYCHAIN_ATTR + r"\s*$", result.stdout, re.M)) + raw = _decode_keychain_attr(re.search(r"^password: " + _KEYCHAIN_ATTR + r"\s*$", result.stderr, re.M)) + if not account or not raw: + return None + try: + payload = json.loads(raw) + except ValueError: + return None + return (account, payload) if isinstance(payload, dict) else None + + def _read_claude_code_keychain_payload() -> Optional[Dict[str, Any]]: """Raw ``{"claudeAiOauth": {...}, ...}`` payload from the macOS Keychain, or None. @@ -240,23 +286,6 @@ def _read_claude_code_keychain_payload() -> Optional[Dict[str, Any]]: return payload if isinstance(payload, dict) else None -def _claude_code_keychain_account() -> str: - """The ``acct`` attribute of the existing Keychain item (``""`` when unreadable). - - ``add-generic-password -U`` matches on account AND service; writing under a different - account would create a second item instead of updating the one Claude Code reads. - """ - try: - result = subprocess.run( - ["security", "find-generic-password", "-s", _CLAUDE_CODE_KEYCHAIN_SERVICE], - capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=5, stdin=subprocess.DEVNULL, - ) - except (OSError, subprocess.TimeoutExpired): - return "" - match = re.search(r'"acct"="((?:[^"\\]|\\.)*)"', result.stdout) if result.returncode == 0 else None - return match.group(1) if match else "" - - def _keychain_mirror_command(account: str, payload: Dict[str, Any]) -> tuple[list[str], str]: """``(argv, stdin)`` that updates the Claude Code Keychain item with ``payload``. @@ -265,8 +294,10 @@ def _keychain_mirror_command(account: str, payload: Dict[str, Any]) -> tuple[lis no terminal, reads only the first line and stores an EMPTY password when the confirmation read hits EOF — either way the live token must never sit on argv. """ - quoted = lambda v: '"' + v.replace("\\", "\\\\").replace('"', '\\"') + '"' # noqa: E731 - security -i tokenizer - encoded = json.dumps(payload, separators=(",", ":")).encode("utf-8").hex() + def quoted(value: str) -> str: # the ``security -i`` tokenizer: double quotes, backslash escapes + return '"' + value.replace("\\", "\\\\").replace('"', '\\"') + '"' + + encoded = json.dumps(payload, separators=(",", ":"), ensure_ascii=True).encode("utf-8").hex() line = f"add-generic-password -U -a {quoted(account)} -s {quoted(_CLAUDE_CODE_KEYCHAIN_SERVICE)} -X {encoded}\n" return ["security", "-i"], line @@ -451,7 +482,10 @@ def _refresh_oauth_token(creds: Dict[str, Any]) -> Optional[str]: # The POST spent ``refresh_token``; this write is the commit step. On failure, fail closed and # mark the pre-rotation pair as spent. try: - _write_claude_code_credentials(refreshed["access_token"], refreshed["refresh_token"], refreshed["expires_at_ms"]) + _write_claude_code_credentials( + refreshed["access_token"], refreshed["refresh_token"], refreshed["expires_at_ms"], + spent_refresh_token=refresh_token, + ) except Exception as e: logger.error( "Anthropic OAuth refresh rotated the single-use token but could not " @@ -473,7 +507,8 @@ def _refresh_oauth_token(creds: Dict[str, Any]) -> Optional[str]: def _write_claude_code_credentials( - access_token: str, refresh_token: str, expires_at_ms: int, *, scopes: Optional[list] = None + access_token: str, refresh_token: str, expires_at_ms: int, *, scopes: Optional[list] = None, + spent_refresh_token: str = "", ) -> None: """Commit refreshed credentials to ~/.claude/.credentials.json; ``CredentialPersistError`` on any failure (a corrupt existing file included). *scopes* (or the previously stored scopes) are persisted because Claude Code @@ -491,7 +526,8 @@ def _write_claude_code_credentials( oauth_data["scopes"] = existing["claudeAiOauth"]["scopes"] existing["claudeAiOauth"] = oauth_data _commit_private_json(cred_path, existing, "credentials") - _mirror_claude_code_credentials_to_keychain(access_token, refresh_token, expires_at_ms) + _mirror_claude_code_credentials_to_keychain( + access_token, refresh_token, expires_at_ms, spent_refresh_token=spent_refresh_token) def _merge_keychain_credential_payload( @@ -510,33 +546,33 @@ def _merge_keychain_credential_payload( def _mirror_claude_code_credentials_to_keychain( - access_token: str, refresh_token: str, expires_at_ms: int + access_token: str, refresh_token: str, expires_at_ms: int, *, spent_refresh_token: str ) -> None: - """Mirror a committed refresh into the macOS Keychain when an entry already exists. + """After a Hermes refresh, write the rotated pair into the Claude Code Keychain item too (#98334). - On Darwin the Keychain is Claude Code's authoritative store, but Hermes historically - only wrote ``~/.claude/.credentials.json``. Because the refresh token is single-use and - rotating, that left the Keychain holding an already-invalidated token, which Claude Code - then spent into ``invalid_grant`` and discarded (``Login: Expired``). Mirror the rotated - pair back with ``add-generic-password -U`` (through ``security -i``) so both stores agree. - - Fail-soft: a Keychain mirror failure is logged, never raised. The file commit has already - succeeded and the resolver still resolves from it; only the secondary store stays stale. - No-op off Darwin and when no entry exists (we never create one the user has not). + Claude Code on macOS reads the login Keychain first. Refresh tokens are single-use, so a refresh + that only updates the file leaves the Keychain holding a spent token and Claude Code logs itself + out. Only the item that held the pair we just spent is updated — a different pair there means a + different login (``CLAUDE_CONFIG_DIR``) or a rotation Claude Code already made, and clobbering it + would be the bug in the other direction. Best-effort: never raises, never creates an item. """ if platform.system() != "Darwin": return try: - existing = _read_claude_code_keychain_payload() - account = _claude_code_keychain_account() if existing else "" - if not existing or not account: + item = _find_claude_code_keychain_item() + if item is None: + return + account, existing = item + oauth = existing.get("claudeAiOauth") + if not isinstance(oauth, dict) or oauth.get("refreshToken") != spent_refresh_token: + logger.debug("Keychain mirror skipped: item does not hold the pair that was just rotated") return argv, line = _keychain_mirror_command( account, _merge_keychain_credential_payload(existing, access_token, refresh_token, expires_at_ms)) result = subprocess.run( argv, input=line, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=10, ) - except (OSError, subprocess.TimeoutExpired) as e: + except Exception as e: # the file commit already succeeded; a Keychain hiccup must not fail the rotation logger.debug("Keychain mirror skipped (%s)", e) return if result.returncode != 0: diff --git a/agent/credential_pool.py b/agent/credential_pool.py index ede5f7d0e1..4eee11c0e3 100644 --- a/agent/credential_pool.py +++ b/agent/credential_pool.py @@ -1372,7 +1372,7 @@ class CredentialPool(CredentialPoolAdminMixin, CredentialPoolModelCooldownMixin) from agent import anthropic_credentials as ac args = (refreshed["access_token"], refreshed["refresh_token"], refreshed["expires_at_ms"]) if entry.source == "claude_code": - ac._write_claude_code_credentials(*args) + ac._write_claude_code_credentials(*args, spent_refresh_token=entry.refresh_token or "") else: ac._write_hermes_oauth_credentials(*args) except Exception as wexc: diff --git a/tests/agent/test_anthropic_borrowed_row_authority.py b/tests/agent/test_anthropic_borrowed_row_authority.py index 1d320378f3..938a1d25b4 100644 --- a/tests/agent/test_anthropic_borrowed_row_authority.py +++ b/tests/agent/test_anthropic_borrowed_row_authority.py @@ -163,9 +163,9 @@ def test_refresh_from_persisted_sanitized_row_keeps_the_full_pair( real_write = AA._write_claude_code_credentials - def _counting_write(access_token, refresh_token, expires_at_ms): + def _counting_write(access_token, refresh_token, expires_at_ms, **kwargs): writes.append(refresh_token) - return real_write(access_token, refresh_token, expires_at_ms) + return real_write(access_token, refresh_token, expires_at_ms, **kwargs) monkeypatch.setattr(AA, "refresh_anthropic_oauth_pure", _counting_refresh) monkeypatch.setattr(AA, "_write_claude_code_credentials", _counting_write) diff --git a/tests/agent/test_anthropic_keychain.py b/tests/agent/test_anthropic_keychain.py index 50b67547c8..4bf04e3850 100644 --- a/tests/agent/test_anthropic_keychain.py +++ b/tests/agent/test_anthropic_keychain.py @@ -13,6 +13,7 @@ from agent.anthropic_credentials import ( _read_claude_code_credentials_from_keychain, read_claude_code_credentials, _refresh_oauth_token, + _find_claude_code_keychain_item, _keychain_mirror_command, _merge_keychain_credential_payload, _mirror_claude_code_credentials_to_keychain, @@ -397,6 +398,28 @@ class TestKeychainMirrorCommand: assert json.loads(bytes.fromhex(hex_blob)) == payload +class TestFindClaudeCodeKeychainItem: + """``_find_claude_code_keychain_item`` parses one ``find-generic-password -g`` call. ``security`` + prints plain-ASCII attributes quoted but UNescaped, anything else as ``0x ""``.""" + + @pytest.mark.macos_only + @pytest.mark.parametrize( + "acct_line, expected_account", + [ + (' "acct"="bob"', "bob"), + (' "acct"="my "user" name"', 'my "user" name'), # embedded quote, printed raw + (' "acct"=0x616C2269636520C3BC "al\\"ice \\303\\274"', 'al"ice \u00fc'), # non-ASCII → hex form + ], + ) + def test_reads_account_and_payload_in_both_output_encodings(self, monkeypatch, acct_line, expected_account): + payload = {"claudeAiOauth": {"refreshToken": "r"}, "mcpOAuth": {"a": 1}} + fake = MagicMock(returncode=0, stdout=f'keychain: "/Users/x/Library/Keychains/login.keychain-db"\n{acct_line}\n', + stderr=f"password: 0x{json.dumps(payload).encode().hex()} \"...\"\n") + monkeypatch.setattr(subprocess, "run", lambda argv, **k: fake) + + assert _find_claude_code_keychain_item() == (expected_account, payload) + + class TestMirrorClaudeCodeCredentialsToKeychain: """The #98334 write mirror shells out to ``security``; ``subprocess.run`` is mocked so no real Keychain is touched.""" @@ -404,10 +427,28 @@ class TestMirrorClaudeCodeCredentialsToKeychain: @pytest.mark.macos_only def test_no_write_when_no_entry_exists(self, monkeypatch): """Never create a Keychain item the user has not.""" - monkeypatch.setattr("agent.anthropic_credentials._read_claude_code_keychain_payload", lambda: None) - called = [] - monkeypatch.setattr(subprocess, "run", lambda *a, **k: called.append(a)) + monkeypatch.setattr("agent.anthropic_credentials._find_claude_code_keychain_item", lambda: None) + run = MagicMock(return_value=MagicMock(returncode=0)) + monkeypatch.setattr(subprocess, "run", run) - _mirror_claude_code_credentials_to_keychain("a", "b", 1) + _mirror_claude_code_credentials_to_keychain("a", "b", 1, spent_refresh_token="old") - assert called == [] + run.assert_not_called() + + @pytest.mark.macos_only + def test_only_the_item_holding_the_spent_pair_is_updated(self, monkeypatch): + """A different pair in the Keychain is another login or a rotation Claude Code already made; + overwriting it would be the bug in the other direction.""" + item = ("bob", {"claudeAiOauth": {"accessToken": "A0", "refreshToken": "R0"}}) + monkeypatch.setattr("agent.anthropic_credentials._find_claude_code_keychain_item", lambda: item) + run = MagicMock(return_value=MagicMock(returncode=0)) + monkeypatch.setattr(subprocess, "run", run) + + _mirror_claude_code_credentials_to_keychain("A1", "R1", 1, spent_refresh_token="not-R0") + run.assert_not_called() + + _mirror_claude_code_credentials_to_keychain("A1", "R1", 1, spent_refresh_token="R0") + (argv,), kwargs = run.call_args + assert argv == ["security", "-i"] + assert json.loads(bytes.fromhex(kwargs["input"].split(" -X ", 1)[1].strip()))["claudeAiOauth"] == { + "accessToken": "A1", "refreshToken": "R1", "expiresAt": 1} diff --git a/tests/agent/test_auxiliary_client.py b/tests/agent/test_auxiliary_client.py index fcb8028ab5..06570e42fe 100644 --- a/tests/agent/test_auxiliary_client.py +++ b/tests/agent/test_auxiliary_client.py @@ -2940,7 +2940,8 @@ class TestAuxiliaryAuthRefreshRetry: assert cache_key not in aux._client_cache # evicted, not closed (in-flight users) mock_refresh_oauth.assert_called_once_with("refresh-token", use_json=False) - mock_write.assert_called_once_with("fresh-token", "refresh-token-2", 9999999999999) + mock_write.assert_called_once_with( + "fresh-token", "refresh-token-2", 9999999999999, spent_refresh_token="refresh-token") stale_client.close.assert_not_called() def test_refresh_provider_credentials_remints_vertex_token_and_evicts_cache(self): From 3de7140bcd6755985031b42d94f58a11716f59f1 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:07:59 +0530 Subject: [PATCH 04/96] fix(agent_init): the 64K context floor judges the window Ollama serves, not the GGUF metadata An Ollama server serves num_ctx. A Modelfile or model.ollama_num_ctx at 65536 is a usable window even when the model metadata advertises 40960, yet the floor ran before num_ctx was resolved and read only the probed window, so the agent (a cron job reaching a local fallback in the report) refused to construct with "context window of 40,960 tokens". num_ctx resolution now runs before the floor and the floor takes max(probed, served). The compressor keeps tek's one-directional clamp (cb71d5f1b1): it still targets the smaller probed window, so nothing about compaction thresholds changes; a served window below 64K is still rejected. Rebuilt from PR 100475 by fangliquan (the agent_init it targeted was decomposed since); the unrelated cron pin contract test there is not taken. Co-authored-by: fangliquan --- agent/agent_init.py | 8 +++++++- tests/agent/test_ollama_num_ctx.py | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/agent/agent_init.py b/agent/agent_init.py index 7fa654fb07..a76cb4b007 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1912,6 +1912,11 @@ def _enforce_minimum_context(agent): # Reject windows below the 64K floor needed for reliable tool-calling; an explicit # positive model.context_length on LM Studio is allowed below the floor. _ctx = getattr(agent.context_compressor, "context_length", 0) + # An Ollama server serves num_ctx, not the GGUF's advertised window: a Modelfile or + # model.ollama_num_ctx at 64K+ is a usable window even when the metadata says 40K (#100437). + _served = getattr(agent, "_ollama_num_ctx", None) + if isinstance(_served, int) and not isinstance(_served, bool) and _served > 0: + _ctx = max(_ctx or 0, _served) _allow_lmstudio_explicit_below_floor = ( str(agent.provider or "").strip().lower() == "lmstudio" and isinstance(agent._config_context_length, int) @@ -2326,11 +2331,12 @@ def init_agent( agent, _agent_cfg, base_url ) _build_context_engine(agent, _agent_cfg, cs, _custom_providers, _effective_context_length, session_db) + # num_ctx before the floor: the served Ollama window is part of what the floor judges. + _configure_ollama_num_ctx(agent, _model_cfg, _config_context_length) _enforce_minimum_context(agent) _warn_nonagentic_hermes_model(agent) _inject_context_engine_tools(agent) _init_usage_state(agent) - _configure_ollama_num_ctx(agent, _model_cfg, _config_context_length) _emit_compression_summary(agent, cs) _snapshot_primary_runtime(agent) diff --git a/tests/agent/test_ollama_num_ctx.py b/tests/agent/test_ollama_num_ctx.py index d4f9b8607f..9bbb882f31 100644 --- a/tests/agent/test_ollama_num_ctx.py +++ b/tests/agent/test_ollama_num_ctx.py @@ -7,6 +7,8 @@ Covers: from unittest.mock import patch, MagicMock +import pytest + from agent.model_metadata import query_ollama_num_ctx, query_ollama_supports_vision @@ -187,3 +189,18 @@ class TestCompressorClampsToNumCtx: # num_ctx above the resolved window must not RAISE the compressor # window: the clamp is one-directional. assert agent.context_compressor.context_length == 65536 + + +class TestServedNumCtxSatisfiesTheFloor(TestCompressorClampsToNumCtx): + """#100437: the 64K floor judges the window Ollama actually serves. A Modelfile or + model.ollama_num_ctx at 64K+ is usable even when the GGUF metadata advertises 40K, so + construction must succeed; the compressor still targets the smaller probed window.""" + + def test_explicit_num_ctx_above_the_floor_admits_a_small_metadata_window(self): + agent = self._build_agent({"agent": {}, "model": {"ollama_num_ctx": 65536}}, probed_ctx=40960) + assert agent._ollama_num_ctx == 65536 + assert agent.context_compressor.context_length == 40960 # one-directional clamp unchanged + + def test_served_window_below_the_floor_is_still_rejected(self): + with pytest.raises(ValueError, match="below the minimum"): + self._build_agent({"agent": {}, "model": {"ollama_num_ctx": 32768}}, probed_ctx=40960) From afaa53e5fe8c99d52bb0df512638a3223e30da99 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:38:54 +0530 Subject: [PATCH 05/96] fix(agent_init): judge the served window only for local endpoints; clamp after the floor as before Review findings on the first cut: * A hosted provider with a stale model.ollama_num_ctx passed the floor for a 40K model although nothing ever raises a hosted window. The served window now counts only when the endpoint is local (is_local_endpoint), the same gate the num_ctx probe uses. * Moving the whole num_ctx phase ahead of the floor also moved tek's compressor clamp ahead of it, which flipped two observables: a model.context_length above a sub-64K num_ctx was rejected instead of constructed-and-clamped, and the floor's message reported the clamped value with advice (set model.context_length) that could not help. The phase is split: resolution runs before the floor, the clamp (_clamp_compressor_to_ollama_num_ctx) stays at its original position, so every case main constructed still constructs with identical compressor numbers and the floor's message is unchanged. Tests: the harness is a module-level helper so the new class no longer re-collects the parent's tests (13 -> 11 collected); the negative pins the local-endpoint gate (red when the gate is dropped) instead of a case main already rejected. --- agent/agent_init.py | 13 +++--- tests/agent/test_ollama_num_ctx.py | 71 ++++++++++++++++-------------- 2 files changed, 45 insertions(+), 39 deletions(-) diff --git a/agent/agent_init.py b/agent/agent_init.py index a76cb4b007..b48037180b 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1912,11 +1912,11 @@ def _enforce_minimum_context(agent): # Reject windows below the 64K floor needed for reliable tool-calling; an explicit # positive model.context_length on LM Studio is allowed below the floor. _ctx = getattr(agent.context_compressor, "context_length", 0) - # An Ollama server serves num_ctx, not the GGUF's advertised window: a Modelfile or + # A local Ollama server serves num_ctx, not the GGUF's advertised window: a Modelfile or # model.ollama_num_ctx at 64K+ is a usable window even when the metadata says 40K (#100437). - _served = getattr(agent, "_ollama_num_ctx", None) - if isinstance(_served, int) and not isinstance(_served, bool) and _served > 0: - _ctx = max(_ctx or 0, _served) + # Only a local endpoint can honour num_ctx, so a stale override never admits a hosted model. + if agent._ollama_num_ctx and agent.base_url and is_local_endpoint(agent.base_url): + _ctx = max(_ctx or 0, agent._ollama_num_ctx) _allow_lmstudio_explicit_below_floor = ( str(agent.provider or "").strip().lower() == "lmstudio" and isinstance(agent._config_context_length, int) @@ -2043,6 +2043,9 @@ def _configure_ollama_num_ctx(agent, _model_cfg, _config_context_length): "Ollama num_ctx: will request %d tokens (model max from /api/show)", agent._ollama_num_ctx, ) + + +def _clamp_compressor_to_ollama_num_ctx(agent): # Recalibrate the compressor to the served window: every request runs at num_ctx, so a # trigger derived from the probed model window could sit above it and never fire. # A config that sets only model.ollama_num_ctx (without model.context_length) previously left the @@ -2331,12 +2334,12 @@ def init_agent( agent, _agent_cfg, base_url ) _build_context_engine(agent, _agent_cfg, cs, _custom_providers, _effective_context_length, session_db) - # num_ctx before the floor: the served Ollama window is part of what the floor judges. _configure_ollama_num_ctx(agent, _model_cfg, _config_context_length) _enforce_minimum_context(agent) _warn_nonagentic_hermes_model(agent) _inject_context_engine_tools(agent) _init_usage_state(agent) + _clamp_compressor_to_ollama_num_ctx(agent) _emit_compression_summary(agent, cs) _snapshot_primary_runtime(agent) diff --git a/tests/agent/test_ollama_num_ctx.py b/tests/agent/test_ollama_num_ctx.py index 9bbb882f31..bffb9ef2c4 100644 --- a/tests/agent/test_ollama_num_ctx.py +++ b/tests/agent/test_ollama_num_ctx.py @@ -140,39 +140,40 @@ class TestQueryOllamaSupportsVision: # ═══════════════════════════════════════════════════════════════════════ +def _build_agent(cfg, probed_ctx, base_url="http://localhost:11434/v1"): + import agent.context_compressor as cc_mod + with ( + patch("model_tools.get_tool_definitions", return_value=[]), + patch("model_tools.check_toolset_requirements", return_value={}), + patch("agent.process_bootstrap.OpenAI"), + patch("hermes_cli.config.load_config", return_value=cfg), + patch("hermes_cli.config.load_config_readonly", return_value=cfg), + patch( + "agent.model_metadata.get_model_context_length", + return_value=probed_ctx, + ), + patch.object( + cc_mod, "get_model_context_length", return_value=probed_ctx, + ), + ): + from run_agent import AIAgent + return AIAgent( + model="gemma3:27b", + api_key="ollama", + base_url=base_url, + quiet_mode=True, + skip_context_files=True, + skip_memory=True, + ) + + class TestCompressorClampsToNumCtx: """A config setting ONLY model.ollama_num_ctx (no model.context_length) must not leave the compressor targeting the probed model window while requests run at the smaller served num_ctx.""" - def _build_agent(self, cfg, probed_ctx): - import agent.context_compressor as cc_mod - with ( - patch("model_tools.get_tool_definitions", return_value=[]), - patch("model_tools.check_toolset_requirements", return_value={}), - patch("agent.process_bootstrap.OpenAI"), - patch("hermes_cli.config.load_config", return_value=cfg), - patch("hermes_cli.config.load_config_readonly", return_value=cfg), - patch( - "agent.model_metadata.get_model_context_length", - return_value=probed_ctx, - ), - patch.object( - cc_mod, "get_model_context_length", return_value=probed_ctx, - ), - ): - from run_agent import AIAgent - return AIAgent( - model="gemma3:27b", - api_key="ollama", - base_url="http://localhost:11434/v1", - quiet_mode=True, - skip_context_files=True, - skip_memory=True, - ) - def test_num_ctx_only_config_clamps_compressor_window(self): - agent = self._build_agent( + agent = _build_agent( {"agent": {}, "model": {"ollama_num_ctx": 65536}}, probed_ctx=262144 ) assert agent._ollama_num_ctx == 65536 @@ -183,7 +184,7 @@ class TestCompressorClampsToNumCtx: assert agent.context_compressor.threshold_tokens < 65536 def test_larger_num_ctx_does_not_inflate_compressor_window(self): - agent = self._build_agent( + agent = _build_agent( {"agent": {}, "model": {"ollama_num_ctx": 131072}}, probed_ctx=65536 ) # num_ctx above the resolved window must not RAISE the compressor @@ -191,16 +192,18 @@ class TestCompressorClampsToNumCtx: assert agent.context_compressor.context_length == 65536 -class TestServedNumCtxSatisfiesTheFloor(TestCompressorClampsToNumCtx): - """#100437: the 64K floor judges the window Ollama actually serves. A Modelfile or - model.ollama_num_ctx at 64K+ is usable even when the GGUF metadata advertises 40K, so +class TestServedNumCtxSatisfiesTheFloor: + """#100437: the 64K floor judges the window a local Ollama server actually serves. A Modelfile + or model.ollama_num_ctx at 64K+ is usable even when the GGUF metadata advertises 40K, so construction must succeed; the compressor still targets the smaller probed window.""" def test_explicit_num_ctx_above_the_floor_admits_a_small_metadata_window(self): - agent = self._build_agent({"agent": {}, "model": {"ollama_num_ctx": 65536}}, probed_ctx=40960) + agent = _build_agent({"agent": {}, "model": {"ollama_num_ctx": 65536}}, probed_ctx=40960) assert agent._ollama_num_ctx == 65536 assert agent.context_compressor.context_length == 40960 # one-directional clamp unchanged - def test_served_window_below_the_floor_is_still_rejected(self): + def test_served_window_counts_only_for_a_local_endpoint(self): + """Only a local server honours num_ctx; a stale override must not admit a hosted 40K model.""" with pytest.raises(ValueError, match="below the minimum"): - self._build_agent({"agent": {}, "model": {"ollama_num_ctx": 32768}}, probed_ctx=40960) + _build_agent({"agent": {}, "model": {"ollama_num_ctx": 65536}}, probed_ctx=40960, + base_url="https://openrouter.ai/api/v1") From 8600cb0744bd77fd8d2fccc282a2f1b15587ddf7 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 10:54:03 +0530 Subject: [PATCH 06/96] chore: map psam-717 contributor email for #112834 salvage --- contributors/emails/155116265+psam-717@users.noreply.github.com | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 contributors/emails/155116265+psam-717@users.noreply.github.com diff --git a/contributors/emails/155116265+psam-717@users.noreply.github.com b/contributors/emails/155116265+psam-717@users.noreply.github.com new file mode 100644 index 0000000000..843939862a --- /dev/null +++ b/contributors/emails/155116265+psam-717@users.noreply.github.com @@ -0,0 +1,2 @@ +psam-717 +# PR #112834 salvage From 69c51bb008ff4f2c5e6ec0b125da20a254789f72 Mon Sep 17 00:00:00 2001 From: psam <155116265+psam-717@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:34:20 +0000 Subject: [PATCH 07/96] fix(desktop): stop forced wake reconnects from churning live secondary sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-profile desktop setups flicker continuously: every wake signal (power resume, network 'online', focus) forced a reconnectNow({forceOpenSockets: true}) that closed EVERY open secondary socket and redialed it, and the live-work pruner could dispose a freshly opened idle socket before its consumer registered in the keep-set. Closing a socket a mounted surface is bound to detaches its runtime, the backend orphan-reaps it, session.reclaimed unbinds the surface, and the re-resume lands on a fresh socket the same signal closes again — a reconnect/remount loop every 2-5s (#94769). Three guards, all on the renderer side: - Coalesce forced wake reconnects: one forced reconnect per 15s holdoff (renderer twin of the main process's POWER_RESUME_REVALIDATION_HOLDOFF_MS). Windows fires 'online' on any interface change; a socket dropped in the holdoff window is still healed by the ordinary close/reconnect backoff and the non-forced focus/visibility nudges. - reconnectSecondaryGateways({forceOpenSockets}) spares sockets with in-flight requests or a foreground-pinned surface — the same pins the live-work pruner already honors (#93892). - pruneSecondaryGateways grants freshly opened secondaries a 30s min-lifetime grace so the prune <-> on-demand-dial race cannot close a socket its consumer has not registered yet. Aged sockets reap exactly as before. Fixes #94769 --- .../src/app/gateway/hooks/use-gateway-boot.ts | 30 ++++++++++++++- .../gateway-activation-prune-lease.test.ts | 29 ++++++++++++++ .../gateway-connection-lifecycle.test.ts | 33 ++++++++++++++++ .../store/gateway-connection-scope.test.ts | 17 +++++++-- .../gateway-foreground-retention.test.ts | 23 +++++++---- apps/desktop/src/store/gateway.ts | 38 +++++++++++++++++++ .../src/store/session-request-router.test.ts | 4 ++ 7 files changed, 162 insertions(+), 12 deletions(-) diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts index e678b755dc..9e77961cb0 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts @@ -121,6 +121,11 @@ const RECONNECT_ESCALATE_AFTER_MS = 300_000 // only a STREAK of unanswered pings rebuilds the transport. const GATEWAY_LIVENESS_PROBE_TIMEOUT_MS = 5_000 +// Renderer twin of the main process's POWER_RESUME_REVALIDATION_HOLDOFF_MS: +// forced wake reconnects (online / power resume) are coalesced into one per +// window instead of tearing down every secondary socket on each signal (#94769). +const WAKE_RECONNECT_HOLDOFF_MS = 15_000 + // Bounded self-heal for a failed REMOTE boot (#82679): main classifies every // fault it can see (via getBootProgress().retryable); the renderer adds the one // it cannot — a valid remote WebSocket dial that fails before becoming usable. @@ -1001,7 +1006,30 @@ export function useGatewayBoot({ // Wake signals: power resume (macOS/Windows), network coming back, and the // window regaining focus/visibility. Each nudges an immediate reconnect. - const forceReconnectNow = () => reconnectNow({ forceOpenSocket: true }) + // + // Forced reconnects (power resume / 'online') close and redial every open + // secondary socket. Windows fires 'online' on any interface change — VPN + // connects, Wi-Fi blips, virtual adapter enumeration — so an unthrottled + // handler reaped healthy sockets in bursts and the UI remounted on every + // redial: the #94769 flicker loop. Coalesce forced wakes like the main + // process already does for power-resume revalidation + // (POWER_RESUME_REVALIDATION_HOLDOFF_MS): one forced reconnect per + // holdoff window; a socket dropped in between is still picked up by the + // ordinary close/reconnect backoff and by the non-forced focus/visibility + // nudges below. + let lastForcedWakeReconnectAt = 0 + + const forceReconnectNow = () => { + const now = Date.now() + + if (now - lastForcedWakeReconnectAt < WAKE_RECONNECT_HOLDOFF_MS) { + return + } + + lastForcedWakeReconnectAt = now + void reconnectNow({ forceOpenSocket: true }) + } + const offPowerResume = desktop.onPowerResume?.(() => void forceReconnectNow()) const offConnectionApplied = desktop.onConnectionApplied?.(() => void softSwitch()) diff --git a/apps/desktop/src/store/gateway-activation-prune-lease.test.ts b/apps/desktop/src/store/gateway-activation-prune-lease.test.ts index 184f75f5ed..ca04ae69fe 100644 --- a/apps/desktop/src/store/gateway-activation-prune-lease.test.ts +++ b/apps/desktop/src/store/gateway-activation-prune-lease.test.ts @@ -152,7 +152,11 @@ describe('activation lease vs. the live-work pruner (#89622)', () => { // lease released, the idle entry must be disposed exactly as before. await ensureGatewayForProfile('default') + // Age the socket past the min-lifetime grace so this prune asserts the + // lease release, not the freshly-opened spare (#94769). + vi.useFakeTimers({ now: Date.now() + 31_000 }) pruneSecondaryGateways(new Set()) + vi.useRealTimers() expect(secondaryGateways[0].close).toHaveBeenCalled() }) @@ -182,4 +186,29 @@ describe('activation lease vs. the live-work pruner (#89622)', () => { releaseConnect() }) + + it('a freshly opened idle secondary rides one prune tick before reaping (#94769)', async () => { + vi.useFakeTimers() + + // A socket that just opened is a prune ↔ on-demand-dial race in the making: + // its consumer may not have registered in the keep-set yet, and closing it + // detaches the runtime → backend orphan-reap → `session.reclaimed` → + // re-resume on a fresh socket the next recompute closes again. The + // min-lifetime grace bounds that race without pinning the entry forever. + await ensureGatewayForProfile('bot') + expect(secondaryGateways[0].connectionState).toBe('open') + + // Move away so 'bot' is neither active nor in the keep-set. + await ensureGatewayForProfile('default') + expect(secondaryGateways).toHaveLength(1) + + // Immediately after open: spared by the grace window. + pruneSecondaryGateways(new Set()) + expect(secondaryGateways[0].close).not.toHaveBeenCalled() + + // Past the grace window: reclaimed as idle, as before. + vi.setSystemTime(Date.now() + 31_000) + pruneSecondaryGateways(new Set()) + expect(secondaryGateways[0].close).toHaveBeenCalled() + }) }) diff --git a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts index 6b2c7fa382..26d569d253 100644 --- a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts +++ b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts @@ -318,7 +318,12 @@ describe('secondary reconnect runtime scope', () => { await openGatewayForAgent('homelab', 'writer') const firstSocket = gatewayMocks.instances[0] + + // Age the socket past the min-lifetime grace so this prune exercises the + // stale-binding invalidation path, not the freshly-opened spare (#94769). + vi.useFakeTimers({ now: Date.now() + 31_000 }) pruneSecondaryGateways(new Set()) + vi.useRealTimers() expect(firstSocket.close).toHaveBeenCalledOnce() let finishReconnect!: () => void @@ -436,6 +441,34 @@ describe('reconnectSecondaryGateways', () => { expect(getConnectionFor).toHaveBeenCalledTimes(2) expect(gatewayMocks.instances[0].connectionState).toBe('open') }) + + it('spares a foreground-pinned secondary from the forced wake redial (#94769)', async () => { + // A forced wake (power resume / network online) closing a socket a mounted + // surface is bound to detaches its runtime → backend orphan-reap → + // `session.reclaimed` → re-resume on a fresh socket the same signal may + // close again: the reconnect/remount flicker loop. The registry's + // foregroundScopes hook is the same pin the live-work pruner honors. + configureGatewayRegistry({ + onEvent: vi.fn(), + foregroundScopes: () => new Set(['conn:homelab::default']) + } as never) + + const getConnectionFor = vi.fn(async ({ connectionId, profile }: { connectionId: string; profile: string }) => + descriptorFor(connectionId, profile) + ) + + installDesktop({ getConnectionFor }) + + await ensureGatewayForAgent('homelab', 'default') + expect(gatewayMocks.instances[0].connectionState).toBe('open') + + reconnectSecondaryGateways({ forceOpenSockets: true }) + await Promise.resolve() + + expect(gatewayMocks.instances[0].close).not.toHaveBeenCalled() + expect(gatewayMocks.instances[0].connectionState).toBe('open') + expect(getConnectionFor).toHaveBeenCalledTimes(1) + }) }) describe('reconnect fail-stop on a removed connection', () => { diff --git a/apps/desktop/src/store/gateway-connection-scope.test.ts b/apps/desktop/src/store/gateway-connection-scope.test.ts index 5fb959a00b..047c6013d9 100644 --- a/apps/desktop/src/store/gateway-connection-scope.test.ts +++ b/apps/desktop/src/store/gateway-connection-scope.test.ts @@ -129,6 +129,15 @@ describe('primary gateway registry scope', () => { }) describe('pruneSecondaryGateways with registry-scoped entries', () => { + // The min-lifetime grace (#94769) spares a freshly opened idle socket for + // one prune tick, so reclamation assertions age the socket past the grace + // window first; spare assertions are unaffected by aging. + const pruneAged = (keep?: Set) => { + vi.useFakeTimers({ now: Date.now() + 31_000 }) + pruneSecondaryGateways(keep ?? new Set()) + vi.useRealTimers() + } + it('keeps the previous source socket open when Sessions switches backends', async () => { await ensureGatewayForAgent('work', 'default') await ensureGatewayForAgent('homelab', 'default') @@ -144,7 +153,7 @@ describe('pruneSecondaryGateways with registry-scoped entries', () => { // LOCAL source has live work; that must not pin homelab's socket. await openGatewayForAgent('homelab', 'default') - pruneSecondaryGateways(new Set(['default'])) + pruneAged(new Set(['default'])) expect(gatewayMocks.closed).toEqual(['wss://homelab.invalid/api/ws?profile=default']) }) @@ -172,11 +181,11 @@ describe('pruneSecondaryGateways with registry-scoped entries', () => { it('still keeps a local (profile-keyed) secondary via its bare profile name', async () => { await openGatewayForAgent(null, 'research') - pruneSecondaryGateways(new Set(['research'])) + pruneAged(new Set(['research'])) expect(gatewayMocks.closed).toEqual([]) - pruneSecondaryGateways(new Set()) + pruneAged() expect(gatewayMocks.closed).toHaveLength(1) }) @@ -189,7 +198,7 @@ describe('pruneSecondaryGateways with registry-scoped entries', () => { await openGatewayForAgent(null, 'default') await openGatewayForAgent('homelab', 'default') - pruneSecondaryGateways(new Set(['conn:homelab::default'])) + pruneAged(new Set(['conn:homelab::default'])) expect(gatewayMocks.closed).toEqual(['wss://local.invalid/api/ws?token=t']) }) diff --git a/apps/desktop/src/store/gateway-foreground-retention.test.ts b/apps/desktop/src/store/gateway-foreground-retention.test.ts index baf801bdf2..8e16e2dcd8 100644 --- a/apps/desktop/src/store/gateway-foreground-retention.test.ts +++ b/apps/desktop/src/store/gateway-foreground-retention.test.ts @@ -97,6 +97,15 @@ afterEach(() => { }) describe('foreground tile retention vs. the live-work pruner (#93892)', () => { + // The min-lifetime grace (#94769) spares a freshly opened idle socket for + // one prune tick, so reclamation assertions age the socket past the grace + // window first; spare assertions are unaffected by aging. + const pruneAged = () => { + vi.useFakeTimers({ now: Date.now() + 31_000 }) + pruneSecondaryGateways(idleKeepSet()) + vi.useRealTimers() + } + it('keeps an idle Bot Chat tile’s owner socket across prune recomputes', async () => { // The BOTS workspace dials the bot's own backend without activating it // (keepAllProfilesScope) and opens the canonical chat as a tile on that @@ -107,8 +116,8 @@ describe('foreground tile retention vs. the live-work pruner (#93892)', () => { // Idle: no working / needs-input session anywhere. Before the fix this // recompute closed the socket → backend reaped the runtime → reclaim → // unbind → resume → … forever. - pruneSecondaryGateways(idleKeepSet()) - pruneSecondaryGateways(idleKeepSet()) + pruneAged() + pruneAged() expect(gatewayMocks.closed).toEqual([]) }) @@ -117,7 +126,7 @@ describe('foreground tile retention vs. the live-work pruner (#93892)', () => { await openGatewayForAgent('local', 'bot') $sessionTiles.set([{ ...BOT_TILE, runtimeId: undefined }]) - pruneSecondaryGateways(idleKeepSet()) + pruneAged() expect(gatewayMocks.closed).toEqual([]) }) @@ -125,11 +134,11 @@ describe('foreground tile retention vs. the live-work pruner (#93892)', () => { it('releases the socket once the tile is closed — the pin never latches', async () => { await openGatewayForAgent('local', 'bot') $sessionTiles.set([BOT_TILE]) - pruneSecondaryGateways(idleKeepSet()) + pruneAged() expect(gatewayMocks.closed).toEqual([]) $sessionTiles.set([]) - pruneSecondaryGateways(idleKeepSet()) + pruneAged() expect(gatewayMocks.closed).toEqual(['wss://local.invalid/api/ws?profile=bot']) }) @@ -161,7 +170,7 @@ describe('foreground tile retention vs. the live-work pruner (#93892)', () => { // the same `retained` flag; with no tile bound to it, it is idle garbage. await openGatewayForAgent('local', 'bot') - pruneSecondaryGateways(idleKeepSet()) + pruneAged() expect(gatewayMocks.closed).toEqual(['wss://local.invalid/api/ws?profile=bot']) }) @@ -170,7 +179,7 @@ describe('foreground tile retention vs. the live-work pruner (#93892)', () => { await openGatewayForAgent('homelab', 'bot') $sessionTiles.set([BOT_TILE]) - pruneSecondaryGateways(idleKeepSet()) + pruneAged() expect(gatewayMocks.closed).toEqual(['wss://homelab.invalid/api/ws?profile=bot']) }) diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index 81c49a688c..4e3f43adb6 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -105,6 +105,14 @@ interface Secondary { gateway: HermesGateway /** True after this entry completed at least one socket connection. */ openedOnce: boolean + /** + * Date.now() of the most recent socket 'open'. The live-work pruner's + * min-lifetime grace reads this: an idle prune can race an on-demand dial + * (prune → redial → prune) and close freshly opened sockets before their + * consumer registers in the keep-set, re-triggering the orphan-reap / + * remount loop (#94769). 0 = never opened. + */ + lastOpenedAt: number activeRequests: number connectPromise: Promise | null offEvent: () => void @@ -726,6 +734,7 @@ async function openSecondary(entry: Secondary, spawnPriority: SpawnPriority = 'b } entry.openedOnce = true + entry.lastOpenedAt = Date.now() openedScopes.add(entry.scope) try { @@ -908,6 +917,7 @@ function createSecondary(profile: string, connectionId: null | string = null): S connection: null, gateway, openedOnce: false, + lastOpenedAt: 0, activeRequests: 0, connectPromise: null, offEvent: () => {}, @@ -1891,6 +1901,11 @@ export async function ensureActiveGatewayOpen({ explicit = false }: { explicit?: // activation before reporting the gateway as unavailable. const ACTIVE_GATEWAY_OPEN_WAIT_MS = 8_000 +// Grace period before the live-work pruner may dispose a freshly opened +// secondary socket; see the min-lifetime guard in pruneSecondaryGateways +// (#94769 prune ↔ redial race). +const SECONDARY_MIN_LIFETIME_MS = 30_000 + // Recovery signal: nudge every live secondary back open. Power-resume/network // signals can force sockets that still report open to retire before redialing. export function reconnectSecondaryGateways({ forceOpenSockets = false }: { forceOpenSockets?: boolean } = {}): void { @@ -1912,6 +1927,17 @@ export function reconnectSecondaryGateways({ forceOpenSockets = false }: { force continue } + // A forced wake (power resume / network online) used to close EVERY open + // secondary socket before redialing. Closing one that is mid-use detaches + // its runtime → the backend orphan-reaps it → `session.reclaimed` → the + // surface re-resumes on a fresh socket the same signal may close again: + // the #94769 flicker loop. Only force-redial quiescent sockets; live + // ones ride through and ordinary close/reconnect still heals them if + // the wake genuinely dropped them. + if (entry.activeRequests > 0 || foregroundPinned(entry)) { + continue + } + entry.gateway.close() } @@ -2077,6 +2103,18 @@ export function pruneSecondaryGateways(keep: Set): void { continue } + // Min-lifetime grace: an idle prune can race an on-demand dial (prune → + // redial → prune) and dispose a socket that opened moments ago, before + // its consumer registered in the keep-set — closing it detaches the + // runtime, the backend orphan-reaps it, and the reclaimed surface + // re-resumes on a fresh socket the next recompute closes again: the + // #94769 flicker loop. A young socket rides one prune tick; the idle + // reap still catches it on a later recompute. Number guard: legacy/HMR + // entries may predate the field. + if (entry.lastOpenedAt > 0 && now - entry.lastOpenedAt < SECONDARY_MIN_LIFETIME_MS) { + continue + } + // The route is no longer live work. Release turn leases first so their // counted request holds cannot outlive a disposed route or leave a stale // release closure attached to a later same-key socket. diff --git a/apps/desktop/src/store/session-request-router.test.ts b/apps/desktop/src/store/session-request-router.test.ts index 8c3261b402..376b6787af 100644 --- a/apps/desktop/src/store/session-request-router.test.ts +++ b/apps/desktop/src/store/session-request-router.test.ts @@ -507,7 +507,11 @@ describe('requestForSessionProfile', () => { }) expect(secondaryGateways[0].close).not.toHaveBeenCalled() + // Age the socket past the min-lifetime grace (#94769) so this prune + // asserts the turn-lease release, not the freshly-opened spare. + vi.useFakeTimers({ now: Date.now() + 31_000 }) pruneSecondaryGateways(new Set()) + vi.useRealTimers() expect(secondaryGateways[0].close).toHaveBeenCalledOnce() await requestForSessionProfile(route, ambient as never, 'session.resume', { session_id: 'rt-pruned' }) From a915f0b3b2bb1084df9930b7862505d5f9b334be Mon Sep 17 00:00:00 2001 From: psam <155116265+psam-717@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:13:19 +0000 Subject: [PATCH 08/96] fix(desktop): probe live secondaries on forced wake instead of skipping them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up for #94769: a live-in-use socket cannot simply be SKIPPED by the forced wake path either — a half-open TCP connection reports 'open' forever and fires no close event, so an in-flight request would ride the dead transport until its per-call timeout (30 minutes for prompt.submit). On backends that predate ping/heartbeat there is no other healing path. The wake path now runs the same bounded ping probe the primary's reconnectNow uses: a healthy-but-busy backend answers and keeps its socket; a dead transport is closed and healed by the entry's ordinary reconnect backoff. -32601 (method not found) keeps the socket — a version-skewed but healthy backend, the same carve-out the primary's probe makes. Also stop burning the 15s forced-wake holdoff while boot is incomplete or a gateway switch is in flight — reconnectNow no-ops there, so stamping the holdoff would drop the next 'online' (often the one with the network actually back) and leave recovery to the backoff loops. --- .../src/app/gateway/hooks/use-gateway-boot.ts | 8 +++ .../gateway-connection-lifecycle.test.ts | 56 ++++++++++++++++++- apps/desktop/src/store/gateway.ts | 46 ++++++++++++++- 3 files changed, 106 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts index 9e77961cb0..51fd4ec77c 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts @@ -1020,6 +1020,14 @@ export function useGatewayBoot({ let lastForcedWakeReconnectAt = 0 const forceReconnectNow = () => { + // reconnectNow no-ops while boot is incomplete or a gateway switch is in + // flight; stamping the holdoff then would burn the window and drop the + // next 'online' (often the one with the network actually back), leaving + // recovery to the backoff loops. Stamp only when it will proceed. + if (cancelled || !bootCompleted || $gatewaySwitching.get()) { + return + } + const now = Date.now() if (now - lastForcedWakeReconnectAt < WAKE_RECONNECT_HOLDOFF_MS) { diff --git a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts index 26d569d253..fba1e33642 100644 --- a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts +++ b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts @@ -1,3 +1,4 @@ +import { JsonRpcGatewayError } from '@hermes/shared' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // Connection lifecycle for registry-scoped secondary gateways: @@ -13,7 +14,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // the entry instead of retrying forever. const gatewayMocks = vi.hoisted(() => { - const instances: { close: ReturnType; connectionState: string }[] = [] + const instances: { + close: ReturnType + request: ReturnType + connectionState: string + }[] = [] return { connect: vi.fn(async (_wsUrl: string): Promise => undefined), @@ -38,6 +43,7 @@ vi.mock('@/hermes', () => ({ await gatewayMocks.connect(wsUrl) this.connectionState = 'open' } + request = vi.fn(async (_method: string, _params: Record) => ({})) onEvent = vi.fn((handler: (event: unknown) => void) => { gatewayMocks.eventHandlers.push(handler) @@ -469,6 +475,54 @@ describe('reconnectSecondaryGateways', () => { expect(gatewayMocks.instances[0].connectionState).toBe('open') expect(getConnectionFor).toHaveBeenCalledTimes(1) }) + + it('closes a live-in-use secondary on a forced wake only when its liveness probe fails', async () => { + // A half-open socket never fires a close event, so skipping it would strand + // an in-flight request until its per-call timeout (30 min for + // prompt.submit). The wake path probes instead: a dead transport is + // closed; a healthy one — including a version-skewed backend answering + // -32601 — keeps its socket (#94769 review). + configureGatewayRegistry({ + onEvent: vi.fn(), + foregroundScopes: () => new Set(['conn:homelab::default']) + } as never) + + const getConnectionFor = vi.fn(async ({ connectionId, profile }: { connectionId: string; profile: string }) => + descriptorFor(connectionId, profile) + ) + + installDesktop({ getConnectionFor }) + + await ensureGatewayForAgent('homelab', 'default') + const socket = gatewayMocks.instances[0] + expect(socket.connectionState).toBe('open') + + // Healthy version-skewed backend: -32601 (method not found) is a live + // answer, not a dead socket — the same carve-out the primary's probe + // makes. The socket stays open. + socket.request = vi.fn(async () => { + throw new JsonRpcGatewayError('Method not found', { code: -32601 }) + }) + + reconnectSecondaryGateways({ forceOpenSockets: true }) + await Promise.resolve() + await Promise.resolve() + + expect(socket.close).not.toHaveBeenCalled() + expect(socket.connectionState).toBe('open') + + // Dead transport: the probe rejects with a non-RPC error (timeout family) + // and the socket is torn down so its reconnect backoff can heal it. + socket.request = vi.fn(async () => { + throw new Error('probe timeout') + }) + + reconnectSecondaryGateways({ forceOpenSockets: true }) + await vi.waitFor(() => { + expect(socket.close).toHaveBeenCalledOnce() + }) + expect(socket.connectionState).toBe('closed') + }) }) describe('reconnect fail-stop on a removed connection', () => { diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index 4e3f43adb6..3187697060 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -2,6 +2,7 @@ import { type ConnectionState, type GatewayEvent, isGatewayReauthRequired, + JsonRpcGatewayError, reconnectBackoffDelayMs, registryBackendScopeKey, resolveGatewayWsUrl, @@ -1906,6 +1907,41 @@ const ACTIVE_GATEWAY_OPEN_WAIT_MS = 8_000 // (#94769 prune ↔ redial race). const SECONDARY_MIN_LIFETIME_MS = 30_000 +// Wake-path liveness probe budget for a live-in-use secondary: mirrors +// GATEWAY_LIVENESS_PROBE_TIMEOUT_MS in use-gateway-boot (the primary's probe). +const SECONDARY_WAKE_PROBE_TIMEOUT_MS = 5_000 + +// Probe a live-in-use secondary instead of blind-closing it on a forced wake, +// and close it only when the probe proves it not alive. A half-open TCP +// connection (sleep/wake, silent network drop) reports connectionState +// 'open' forever and fires no close event, so without this close an in-flight +// request rides a dead transport until its per-call timeout — prompt.submit's +// is 30 minutes. Closing arms the entry's ordinary reconnect backoff via its +// onState('closed') handler; a healthy-but-busy backend answers the ping and +// keeps its socket (#94769 review). +function probeSecondaryLiveness(entry: Secondary): void { + if (typeof entry.gateway.request !== 'function') { + return + } + + void entry.gateway.request('ping', {}, SECONDARY_WAKE_PROBE_TIMEOUT_MS).catch((error: unknown) => { + // -32601 (method not found) = a version-skewed but HEALTHY backend that + // predates the ping method — the same compatibility carve-out the + // primary's probe makes in use-gateway-boot. + if (error instanceof JsonRpcGatewayError && error.code === -32601) { + return + } + + // The entry may have been pruned or redialed while the probe was + // pending; only the very same socket may be torn down. + if (g.secondaries.get(entry.scope) !== entry || !isOpen(entry.gateway)) { + return + } + + entry.gateway.close() + }) +} + // Recovery signal: nudge every live secondary back open. Power-resume/network // signals can force sockets that still report open to retire before redialing. export function reconnectSecondaryGateways({ forceOpenSockets = false }: { forceOpenSockets?: boolean } = {}): void { @@ -1931,10 +1967,14 @@ export function reconnectSecondaryGateways({ forceOpenSockets = false }: { force // secondary socket before redialing. Closing one that is mid-use detaches // its runtime → the backend orphan-reaps it → `session.reclaimed` → the // surface re-resumes on a fresh socket the same signal may close again: - // the #94769 flicker loop. Only force-redial quiescent sockets; live - // ones ride through and ordinary close/reconnect still heals them if - // the wake genuinely dropped them. + // the #94769 flicker loop. But a live socket also cannot simply be + // SKIPPED: a half-open socket never fires a close event, so an in-flight + // request would hang until its per-call timeout. Probe liveness instead — + // a healthy-but-busy backend answers and keeps its socket; a dead + // transport is closed and healed by the ordinary reconnect backoff. if (entry.activeRequests > 0 || foregroundPinned(entry)) { + probeSecondaryLiveness(entry) + continue } From ad6c7a5a4cc28f42803dc3f5fe143f950f65b2f7 Mon Sep 17 00:00:00 2001 From: psam <155116265+psam-717@users.noreply.github.com> Date: Sat, 19 Sep 2026 02:28:33 +0000 Subject: [PATCH 09/96] fix(desktop): defer secondary wake-probe force-close behind a failure streak One unanswered 5s ping on a live-in-use secondary could fire while the backend's event loop is starved by a long tool call: force-closing it fed the backend's ws_orphan_reap and interrupted the valid turn (#94769 review). The secondary wake probe now applies the SAME policy the primary's reconnectNow probe uses (decideLivenessForceClose, gateway-liveness-policy): while a request is in flight, the first failure defers behind a bounded re-probe (LIVENESS_REPROBE_DELAY_MS) and only an exhausted streak (2 unanswered probes) closes the socket; an idle-but-pinned socket still closes on the first failure. The streak resets on any answered ping (including the -32601 version-skew carve-out) and on every fresh socket open; a deferred re-probe is cleared on dispose and re-dial. Tests: the wake-probe regression test now holds an in-flight request and proves the first timeout keeps the socket while the unanswered re-probe closes it (red on the previous commit, green here). --- .../gateway-connection-lifecycle.test.ts | 31 +++++- apps/desktop/src/store/gateway.ts | 96 ++++++++++++++++--- 2 files changed, 108 insertions(+), 19 deletions(-) diff --git a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts index fba1e33642..dac711f97f 100644 --- a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts +++ b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts @@ -1,6 +1,8 @@ import { JsonRpcGatewayError } from '@hermes/shared' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { LIVENESS_REPROBE_DELAY_MS } from '@/lib/gateway-liveness-policy' + // Connection lifecycle for registry-scoped secondary gateways: // // 1. Removing a connection must dispose its secondaries — remote/cloud @@ -77,6 +79,7 @@ const { parkSecondariesForRetiredBackend, pruneSecondaryGateways, reconnectSecondaryGateways, + requestGatewayForAgent, retainGatewayForAgent, retainGatewayForSessionTurn, retireLocalProfileGateways, @@ -511,16 +514,34 @@ describe('reconnectSecondaryGateways', () => { expect(socket.close).not.toHaveBeenCalled() expect(socket.connectionState).toBe('open') - // Dead transport: the probe rejects with a non-RPC error (timeout family) - // and the socket is torn down so its reconnect backoff can heal it. + // Mid-turn: an in-flight request holds the entry's lease, and the backend + // — alive, but starved past the probe budget by a long tool call — cannot + // answer the ping. ONE unanswered probe must NOT close it: force-closing + // feeds the backend's ws_orphan_reap and interrupts the valid turn + // (#94769 review). The first failure defers behind a bounded re-probe. + socket.request = vi.fn(() => new Promise(() => {})) + void requestGatewayForAgent('homelab', 'default', 'prompt.submit', {}) + await vi.waitFor(() => { + expect(socket.request).toHaveBeenCalled() + }) + + vi.useFakeTimers() socket.request = vi.fn(async () => { throw new Error('probe timeout') }) reconnectSecondaryGateways({ forceOpenSockets: true }) - await vi.waitFor(() => { - expect(socket.close).toHaveBeenCalledOnce() - }) + await vi.advanceTimersByTimeAsync(0) + + expect(socket.close).not.toHaveBeenCalled() + expect(socket.connectionState).toBe('open') + + // The bounded re-probe also goes unanswered: the failure streak is + // exhausted and the socket is torn down so its reconnect backoff can + // heal it — a persistently unresponsive backend is never trusted forever. + await vi.advanceTimersByTimeAsync(LIVENESS_REPROBE_DELAY_MS) + + expect(socket.close).toHaveBeenCalledOnce() expect(socket.connectionState).toBe('closed') }) }) diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index 3187697060..c625ca640f 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -13,6 +13,7 @@ import { atom } from 'nanostores' import type { HermesConnection } from '@/global' import { HermesGateway, setApiRequestConnection } from '@/hermes' import { translateNow } from '@/i18n' +import { decideLivenessForceClose, LIVENESS_REPROBE_DELAY_MS } from '@/lib/gateway-liveness-policy' import { isTimeoutError, RECONNECT_ATTEMPT_TIMEOUT_MS, withTimeout } from '@/lib/with-timeout' import { notifyError, RECOVERY_ACTIONS } from '@/store/notifications' import { markNativeNotifyBaseline } from '@/store/notify-baseline' @@ -121,6 +122,15 @@ interface Secondary { offState: () => void reconnectTimer: ReturnType | null reconnectAttempt: number + /** + * Consecutive unanswered wake-probe pings on this entry's current socket; + * drives the same streak tolerance the primary's probe applies + * (decideLivenessForceClose). Reset on every answered probe and on every + * fresh socket open. + */ + livenessProbeFailures: number + /** Pending deferred liveness re-probe after an in-flight-work deferral. */ + livenessReprobeTimer: ReturnType | null /** Consecutive automatic dials that stalled (slot wait / dial timeout) * rather than failing fast; see SECONDARY_STALLED_DIAL_BUDGET. */ stalledDials: number @@ -736,6 +746,9 @@ async function openSecondary(entry: Secondary, spawnPriority: SpawnPriority = 'b entry.openedOnce = true entry.lastOpenedAt = Date.now() + // A fresh socket owes nothing to a previous socket's missed pings. + entry.livenessProbeFailures = 0 + clearSecondaryLivenessReprobe(entry) openedScopes.add(entry.scope) try { @@ -926,6 +939,8 @@ function createSecondary(profile: string, connectionId: null | string = null): S offState: () => {}, reconnectTimer: null, reconnectAttempt: 0, + livenessProbeFailures: 0, + livenessReprobeTimer: null, stalledDials: 0, reconnecting: false, pendingConnectionRedial: false, @@ -1919,27 +1934,79 @@ const SECONDARY_WAKE_PROBE_TIMEOUT_MS = 5_000 // is 30 minutes. Closing arms the entry's ordinary reconnect backoff via its // onState('closed') handler; a healthy-but-busy backend answers the ping and // keeps its socket (#94769 review). +// A deferred liveness re-probe for one entry: cleared when the probe is +// answered, the socket is redialed (fresh streak), or the entry is disposed. +function clearSecondaryLivenessReprobe(entry: Secondary): void { + if (entry.livenessReprobeTimer !== null) { + clearTimeout(entry.livenessReprobeTimer) + entry.livenessReprobeTimer = null + } +} + function probeSecondaryLiveness(entry: Secondary): void { if (typeof entry.gateway.request !== 'function') { return } - void entry.gateway.request('ping', {}, SECONDARY_WAKE_PROBE_TIMEOUT_MS).catch((error: unknown) => { - // -32601 (method not found) = a version-skewed but HEALTHY backend that - // predates the ping method — the same compatibility carve-out the - // primary's probe makes in use-gateway-boot. - if (error instanceof JsonRpcGatewayError && error.code === -32601) { - return - } + void entry.gateway.request('ping', {}, SECONDARY_WAKE_PROBE_TIMEOUT_MS).then( + () => { + entry.livenessProbeFailures = 0 + clearSecondaryLivenessReprobe(entry) + }, + (error: unknown) => { + // -32601 (method not found) = a version-skewed but HEALTHY backend that + // predates the ping method — the same compatibility carve-out the + // primary's probe makes in use-gateway-boot. + if (error instanceof JsonRpcGatewayError && error.code === -32601) { + entry.livenessProbeFailures = 0 + clearSecondaryLivenessReprobe(entry) - // The entry may have been pruned or redialed while the probe was - // pending; only the very same socket may be torn down. - if (g.secondaries.get(entry.scope) !== entry || !isOpen(entry.gateway)) { - return - } + return + } - entry.gateway.close() - }) + // The entry may have been pruned or redialed while the probe was + // pending; only the very same socket may be torn down. + if (g.secondaries.get(entry.scope) !== entry || !isOpen(entry.gateway)) { + return + } + + // ONE missed ping is not proof of death: a live backend mid tool call + // can starve its event loop past the probe budget, and force-closing it + // feeds the backend's ws_orphan_reap, interrupting the valid turn + // (#94769 review). Apply the SAME streak policy as the primary's probe + // (decideLivenessForceClose): defer the first failure while work is + // in flight behind a bounded re-probe, close only when the streak is + // exhausted — or immediately when nothing is in flight to protect. + entry.livenessProbeFailures += 1 + + const decision = decideLivenessForceClose({ + workingSessionCount: entry.activeRequests, + consecutiveFailures: entry.livenessProbeFailures + }) + + if (!decision.close) { + if (entry.livenessReprobeTimer === null) { + entry.livenessReprobeTimer = setTimeout(() => { + entry.livenessReprobeTimer = null + + // The entry may have been pruned or redialed while the re-probe + // waited; only the same open socket may be probed again. + if (g.secondaries.get(entry.scope) !== entry || !isOpen(entry.gateway)) { + return + } + + probeSecondaryLiveness(entry) + }, LIVENESS_REPROBE_DELAY_MS) + } + + return + } + + entry.livenessProbeFailures = 0 + clearSecondaryLivenessReprobe(entry) + entry.gateway.close() + } + ) } // Recovery signal: nudge every live secondary back open. Power-resume/network @@ -2071,6 +2138,7 @@ export function parkSecondariesForRetiredBackend(poolKey: string): string[] { function disposeSecondary(entry: Secondary): void { entry.wantOpen = false clearTimer(entry) + clearSecondaryLivenessReprobe(entry) entry.offEvent() entry.offRequest() entry.offState() From e796a2986629351f9cab14bb7184ccb2965635c9 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:38:17 +0530 Subject: [PATCH 10/96] fix(desktop): wake probe counts the foreground turn as in-flight work; sibling pins and prune follow-through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the #112834 salvage: - The secondary's wake probe fed `activeRequests` into decideLivenessForceClose, but prompt.submit returns before the turn ends, so a foreground turn mid tool call showed 0 and ONE missed ping closed the socket — the #95327 false-kill the streak policy exists to prevent. The registry now exposes `liveScopes` (session-states.ts::liveSessionScopes, already used by the pruner's keep-set) and the probe counts a live scope as work in flight. The lifecycle test drives that production shape instead of a parked prompt.submit. - A relay-retained secondary (bot relay pin) was still blind-closed on a forced wake; it joins the probe-instead-of-close predicate the pruner and redial drain already use. - The pruner is event-driven, so a socket spared by the 30 s grace with no store change afterwards held its pool slot forever; the existing 60 s keepalive tick now recomputes the keep-set. - A second wake inside the 15 s holdoff skipped reconnectNow entirely (primary probe, redial of already-closed secondaries); only the destructive forceOpenSockets half is coalesced now. - Shape: LIVENESS_PROBE_TIMEOUT_MS lives once in gateway-liveness-policy.ts (was mirrored in two files); the -32601 carve-out uses lib/gateway-rpc.ts::isMissingRpcMethod; the always-true `typeof gateway.request` guard, the `openedOnce` twin of `lastOpenedAt` and the dead `> 0` guard are gone; SECONDARY_MIN_LIFETIME_MS is exported so the tests stop hard-coding 31_000. --- .../src/app/gateway/hooks/use-gateway-boot.ts | 26 +++++-- .../src/lib/gateway-liveness-policy.ts | 3 + .../gateway-activation-prune-lease.test.ts | 9 +-- .../gateway-connection-lifecycle.test.ts | 47 +++++++------ .../store/gateway-connection-scope.test.ts | 5 +- .../gateway-foreground-retention.test.ts | 5 +- apps/desktop/src/store/gateway.ts | 68 ++++++++++--------- .../src/store/session-request-router.test.ts | 5 +- 8 files changed, 96 insertions(+), 72 deletions(-) diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts index 51fd4ec77c..62eeff9c57 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts @@ -14,7 +14,11 @@ import type { DesktopBootProgress, HermesConnection, HermesWindowState } from '@ import { HermesGateway } from '@/hermes' import { translateNow } from '@/i18n' import { desktopDefaultCwd } from '@/lib/desktop-fs' -import { decideLivenessForceClose, LIVENESS_REPROBE_DELAY_MS } from '@/lib/gateway-liveness-policy' +import { + decideLivenessForceClose, + LIVENESS_PROBE_TIMEOUT_MS, + LIVENESS_REPROBE_DELAY_MS +} from '@/lib/gateway-liveness-policy' import { BACKEND_BOOT_WAIT_TIMEOUT_MS, RECONNECT_ATTEMPT_TIMEOUT_MS, withTimeout } from '@/lib/with-timeout' import { $desktopBoot, @@ -119,7 +123,6 @@ const RECONNECT_ESCALATE_AFTER_MS = 300_000 // TIMEOUT alone no longer tears the socket down mid-turn (#95327): while a // turn is in flight the first timeout defers behind one bounded re-probe, so // only a STREAK of unanswered pings rebuilds the transport. -const GATEWAY_LIVENESS_PROBE_TIMEOUT_MS = 5_000 // Renderer twin of the main process's POWER_RESUME_REVALIDATION_HOLDOFF_MS: // forced wake reconnects (online / power resume) are coalesced into one per @@ -571,7 +574,7 @@ export function useGatewayBoot({ // one inconclusive probe DEFERS the teardown behind a bounded re-probe; // only an exhausted streak (or no in-flight work) closes. try { - await gateway.request('ping', {}, GATEWAY_LIVENESS_PROBE_TIMEOUT_MS) + await gateway.request('ping', {}, LIVENESS_PROBE_TIMEOUT_MS) livenessProbeFailures = 0 } catch (probeErr) { // A version-skewed backend that predates the ping method answers @@ -905,6 +908,7 @@ export function useGatewayBoot({ // primary thread or a just-created session's owner hold is bound to // (#93892). foregroundScopes: foregroundSessionScopes, + liveScopes: liveSessionScopes, onLocalProfileRetired: forgetProfileOnlyRuntimeOwners, onActiveConnectionChanged: publish, // Keep $activeGatewayProfile in lockstep with the registry's OWN record @@ -1028,14 +1032,18 @@ export function useGatewayBoot({ return } + // Only the destructive half is coalesced: a second wake inside the + // holdoff (macOS fires resume then 'online' seconds apart) still runs + // the cheap, idempotent nudge — primary ping probe, redial of already + // closed secondaries — without touching open sockets. const now = Date.now() + const forced = now - lastForcedWakeReconnectAt >= WAKE_RECONNECT_HOLDOFF_MS - if (now - lastForcedWakeReconnectAt < WAKE_RECONNECT_HOLDOFF_MS) { - return + if (forced) { + lastForcedWakeReconnectAt = now } - lastForcedWakeReconnectAt = now - void reconnectNow({ forceOpenSocket: true }) + void reconnectNow({ forceOpenSocket: forced }) } const offPowerResume = desktop.onPowerResume?.(() => void forceReconnectNow()) @@ -1136,6 +1144,10 @@ export function useGatewayBoot({ const keepaliveTimer = setInterval(() => { touchActiveGatewayBackend() touchSecondaryGateways() + // The pruner is otherwise event-driven: a socket spared by the + // min-lifetime grace with no store change afterwards would hold its + // pool slot forever. + recomputeKeptGateways() }, 60_000) // Bound concurrency cost to consumers: keep a background socket while its diff --git a/apps/desktop/src/lib/gateway-liveness-policy.ts b/apps/desktop/src/lib/gateway-liveness-policy.ts index 7e525f765b..b5653d521f 100644 --- a/apps/desktop/src/lib/gateway-liveness-policy.ts +++ b/apps/desktop/src/lib/gateway-liveness-policy.ts @@ -35,6 +35,9 @@ */ /** Consecutive unanswered probes tolerated while work is in flight. */ +/** Ping budget for a liveness probe (primary and secondary sockets share it). */ +export const LIVENESS_PROBE_TIMEOUT_MS = 5_000 + export const LIVENESS_PROBE_FAILURE_STREAK = 2 /** How long after a deferred probe we try again (bounded, coalesced). */ diff --git a/apps/desktop/src/store/gateway-activation-prune-lease.test.ts b/apps/desktop/src/store/gateway-activation-prune-lease.test.ts index ca04ae69fe..c38f2fa41e 100644 --- a/apps/desktop/src/store/gateway-activation-prune-lease.test.ts +++ b/apps/desktop/src/store/gateway-activation-prune-lease.test.ts @@ -53,7 +53,8 @@ const { ensureGatewayForAgent, ensureGatewayForProfile, pruneSecondaryGateways, - setPrimaryGateway + setPrimaryGateway, + SECONDARY_MIN_LIFETIME_MS } = await import('./gateway') function installDesktop(): void { @@ -154,7 +155,7 @@ describe('activation lease vs. the live-work pruner (#89622)', () => { // Age the socket past the min-lifetime grace so this prune asserts the // lease release, not the freshly-opened spare (#94769). - vi.useFakeTimers({ now: Date.now() + 31_000 }) + vi.useFakeTimers({ now: Date.now() + SECONDARY_MIN_LIFETIME_MS + 1_000 }) pruneSecondaryGateways(new Set()) vi.useRealTimers() @@ -180,7 +181,7 @@ describe('activation lease vs. the live-work pruner (#89622)', () => { // Past the lease window: reclaimed. (Lease is wall-clock bounded so a // leaked lease cannot pin a dead entry forever.) - vi.setSystemTime(Date.now() + 31_000) + vi.setSystemTime(Date.now() + SECONDARY_MIN_LIFETIME_MS + 1_000) pruneSecondaryGateways(new Set()) expect(secondaryGateways[0].close).toHaveBeenCalled() @@ -207,7 +208,7 @@ describe('activation lease vs. the live-work pruner (#89622)', () => { expect(secondaryGateways[0].close).not.toHaveBeenCalled() // Past the grace window: reclaimed as idle, as before. - vi.setSystemTime(Date.now() + 31_000) + vi.setSystemTime(Date.now() + SECONDARY_MIN_LIFETIME_MS + 1_000) pruneSecondaryGateways(new Set()) expect(secondaryGateways[0].close).toHaveBeenCalled() }) diff --git a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts index dac711f97f..155341c930 100644 --- a/apps/desktop/src/store/gateway-connection-lifecycle.test.ts +++ b/apps/desktop/src/store/gateway-connection-lifecycle.test.ts @@ -83,7 +83,8 @@ const { retainGatewayForAgent, retainGatewayForSessionTurn, retireLocalProfileGateways, - setPrimaryGateway + setPrimaryGateway, + SECONDARY_MIN_LIFETIME_MS } = await import('./gateway') function installDesktop(stub: Record): void { @@ -330,7 +331,7 @@ describe('secondary reconnect runtime scope', () => { // Age the socket past the min-lifetime grace so this prune exercises the // stale-binding invalidation path, not the freshly-opened spare (#94769). - vi.useFakeTimers({ now: Date.now() + 31_000 }) + vi.useFakeTimers({ now: Date.now() + SECONDARY_MIN_LIFETIME_MS + 1_000 }) pruneSecondaryGateways(new Set()) vi.useRealTimers() expect(firstSocket.close).toHaveBeenCalledOnce() @@ -485,9 +486,14 @@ describe('reconnectSecondaryGateways', () => { // prompt.submit). The wake path probes instead: a dead transport is // closed; a healthy one — including a version-skewed backend answering // -32601 — keeps its socket (#94769 review). + // The foreground turn is in flight on this scope. prompt.submit has long + // since returned (turn completion arrives as stream events), so the entry + // shows no counted request — the registry's live-scope hook is what tells + // the probe there is work to protect. configureGatewayRegistry({ onEvent: vi.fn(), - foregroundScopes: () => new Set(['conn:homelab::default']) + foregroundScopes: () => new Set(['conn:homelab::default']), + liveScopes: () => new Set(['conn:homelab::default']) } as never) const getConnectionFor = vi.fn(async ({ connectionId, profile }: { connectionId: string; profile: string }) => @@ -514,17 +520,11 @@ describe('reconnectSecondaryGateways', () => { expect(socket.close).not.toHaveBeenCalled() expect(socket.connectionState).toBe('open') - // Mid-turn: an in-flight request holds the entry's lease, and the backend - // — alive, but starved past the probe budget by a long tool call — cannot - // answer the ping. ONE unanswered probe must NOT close it: force-closing - // feeds the backend's ws_orphan_reap and interrupts the valid turn - // (#94769 review). The first failure defers behind a bounded re-probe. - socket.request = vi.fn(() => new Promise(() => {})) - void requestGatewayForAgent('homelab', 'default', 'prompt.submit', {}) - await vi.waitFor(() => { - expect(socket.request).toHaveBeenCalled() - }) - + // Mid-turn, the backend — alive, but starved past the probe budget by a + // long tool call — cannot answer the ping. ONE unanswered probe must NOT + // close it: force-closing feeds the backend's ws_orphan_reap and + // interrupts the valid turn (#94769 review). The first failure defers + // behind a bounded re-probe. vi.useFakeTimers() socket.request = vi.fn(async () => { throw new Error('probe timeout') @@ -974,12 +974,12 @@ describe('cooperative pool retirement (supersedes #104871)', () => { }) }) - describe('rejected secondary authentication', () => { it('parks only the rejected source across automatic nudges and recovers on explicit selection', async () => { vi.useFakeTimers() const getConnectionFor = vi.fn(async ({ connectionId, profile }: { connectionId: string; profile: string }) => ({ - ...descriptorFor(connectionId, profile), authMode: 'oauth' + ...descriptorFor(connectionId, profile), + authMode: 'oauth' })) const getGatewayWsUrlFor = vi.fn(async () => ({ ok: true, wsUrl: 'wss://cloud.invalid/api/ws?ticket=fresh' })) installDesktop({ getConnectionFor, getGatewayWsUrlFor }) @@ -1008,7 +1008,8 @@ describe('rejected secondary authentication', () => { vi.useFakeTimers() const getConnectionFor = vi.fn(async ({ connectionId, profile }: { connectionId: string; profile: string }) => ({ - ...descriptorFor(connectionId, profile), authMode: 'oauth' + ...descriptorFor(connectionId, profile), + authMode: 'oauth' })) const getGatewayWsUrlFor = vi.fn(async () => ({ ok: true, wsUrl: 'wss://cloud.invalid/api/ws?ticket=fresh' })) @@ -1030,11 +1031,11 @@ describe('rejected secondary authentication', () => { }) }) - it('keeps background auth rejection after socket disposal until recovery or connection removal', async () => { const { requestGatewayForAgent } = await import('./gateway') const getConnectionFor = vi.fn(async ({ connectionId, profile }: { connectionId: string; profile: string }) => ({ - ...descriptorFor(connectionId, profile), authMode: 'oauth' + ...descriptorFor(connectionId, profile), + authMode: 'oauth' })) const getGatewayWsUrlFor = vi.fn(async () => ({ ok: false, needsOauthLogin: true, error: 'Sign in again' })) installDesktop({ getConnectionFor, getGatewayWsUrlFor }) @@ -1054,14 +1055,16 @@ it('keeps background auth rejection after socket disposal until recovery or conn expect(getGatewayWsUrlFor).toHaveBeenCalledTimes(3) }) - it('does not let a removed connection repopulate the auth rejection', async () => { const { requestGatewayForAgent } = await import('./gateway') const getConnectionFor = vi.fn(async ({ connectionId, profile }: { connectionId: string; profile: string }) => ({ - ...descriptorFor(connectionId, profile), authMode: 'oauth' + ...descriptorFor(connectionId, profile), + authMode: 'oauth' })) let rejectTicket!: (error: Error) => void - const ticket = new Promise((_resolve, reject) => { rejectTicket = reject }) + const ticket = new Promise((_resolve, reject) => { + rejectTicket = reject + }) const getGatewayWsUrlFor = vi.fn(() => ticket) installDesktop({ getConnectionFor, getGatewayWsUrlFor }) const pending = requestGatewayForAgent('cloud', 'default', 'session.list') diff --git a/apps/desktop/src/store/gateway-connection-scope.test.ts b/apps/desktop/src/store/gateway-connection-scope.test.ts index 047c6013d9..c043675e1b 100644 --- a/apps/desktop/src/store/gateway-connection-scope.test.ts +++ b/apps/desktop/src/store/gateway-connection-scope.test.ts @@ -49,7 +49,8 @@ const { openGatewayForAgent, pruneSecondaryGateways, setPrimaryGateway, - setPrimaryGatewayConnectionId + setPrimaryGatewayConnectionId, + SECONDARY_MIN_LIFETIME_MS } = await import('./gateway') const { setApiRequestConnection } = await import('@/hermes') @@ -133,7 +134,7 @@ describe('pruneSecondaryGateways with registry-scoped entries', () => { // one prune tick, so reclamation assertions age the socket past the grace // window first; spare assertions are unaffected by aging. const pruneAged = (keep?: Set) => { - vi.useFakeTimers({ now: Date.now() + 31_000 }) + vi.useFakeTimers({ now: Date.now() + SECONDARY_MIN_LIFETIME_MS + 1_000 }) pruneSecondaryGateways(keep ?? new Set()) vi.useRealTimers() } diff --git a/apps/desktop/src/store/gateway-foreground-retention.test.ts b/apps/desktop/src/store/gateway-foreground-retention.test.ts index 8e16e2dcd8..b51eb01ef8 100644 --- a/apps/desktop/src/store/gateway-foreground-retention.test.ts +++ b/apps/desktop/src/store/gateway-foreground-retention.test.ts @@ -41,7 +41,8 @@ const { openGatewayForAgent, pruneSecondaryGateways, requestGatewayForAgent, - setPrimaryGateway + setPrimaryGateway, + SECONDARY_MIN_LIFETIME_MS } = await import('./gateway') const { $sessionTiles, foregroundSessionScopes, liveSessionScopes } = await import('./session-states') @@ -101,7 +102,7 @@ describe('foreground tile retention vs. the live-work pruner (#93892)', () => { // one prune tick, so reclamation assertions age the socket past the grace // window first; spare assertions are unaffected by aging. const pruneAged = () => { - vi.useFakeTimers({ now: Date.now() + 31_000 }) + vi.useFakeTimers({ now: Date.now() + SECONDARY_MIN_LIFETIME_MS + 1_000 }) pruneSecondaryGateways(idleKeepSet()) vi.useRealTimers() } diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index c625ca640f..77c01ca245 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -2,7 +2,6 @@ import { type ConnectionState, type GatewayEvent, isGatewayReauthRequired, - JsonRpcGatewayError, reconnectBackoffDelayMs, registryBackendScopeKey, resolveGatewayWsUrl, @@ -13,7 +12,12 @@ import { atom } from 'nanostores' import type { HermesConnection } from '@/global' import { HermesGateway, setApiRequestConnection } from '@/hermes' import { translateNow } from '@/i18n' -import { decideLivenessForceClose, LIVENESS_REPROBE_DELAY_MS } from '@/lib/gateway-liveness-policy' +import { + decideLivenessForceClose, + LIVENESS_PROBE_TIMEOUT_MS, + LIVENESS_REPROBE_DELAY_MS +} from '@/lib/gateway-liveness-policy' +import { isMissingRpcMethod } from '@/lib/gateway-rpc' import { isTimeoutError, RECONNECT_ATTEMPT_TIMEOUT_MS, withTimeout } from '@/lib/with-timeout' import { notifyError, RECOVERY_ACTIONS } from '@/store/notifications' import { markNativeNotifyBaseline } from '@/store/notify-baseline' @@ -94,6 +98,12 @@ interface RegistryConfig { * follows the tile set, so closing the tile releases the socket. */ foregroundScopes?: () => ReadonlySet + /** + * Scopes with a running or needs-input session runtime. The wake-path + * liveness probe counts these as in-flight work: prompt.submit returns + * before the turn ends, so `activeRequests` is 0 during most of a turn. + */ + liveScopes?: () => ReadonlySet } // ── Secondary (pool) backends ────────────────────────────────────────────── @@ -105,8 +115,6 @@ interface Secondary { connectionId: null | string connection: HermesConnection | null gateway: HermesGateway - /** True after this entry completed at least one socket connection. */ - openedOnce: boolean /** * Date.now() of the most recent socket 'open'. The live-work pruner's * min-lifetime grace reads this: an idle prune can race an on-demand dial @@ -672,7 +680,7 @@ async function openSecondary(entry: Secondary, spawnPriority: SpawnPriority = 'b // open. Awaiting it is intentional: correctness at the generation boundary // outranks the single local-module microtask this adds to a reconnect. const openedScopes = openedSecondaryScopes() - const reopening = entry.openedOnce || entry.connection !== null || openedScopes.has(entry.scope) + const reopening = entry.lastOpenedAt > 0 || entry.connection !== null || openedScopes.has(entry.scope) let reconcileBusyAfterOpen: null | (() => void) = null if (reopening) { @@ -744,7 +752,6 @@ async function openSecondary(entry: Secondary, spawnPriority: SpawnPriority = 'b throw error } - entry.openedOnce = true entry.lastOpenedAt = Date.now() // A fresh socket owes nothing to a previous socket's missed pings. entry.livenessProbeFailures = 0 @@ -930,7 +937,6 @@ function createSecondary(profile: string, connectionId: null | string = null): S connectionId, connection: null, gateway, - openedOnce: false, lastOpenedAt: 0, activeRequests: 0, connectPromise: null, @@ -1919,21 +1925,9 @@ const ACTIVE_GATEWAY_OPEN_WAIT_MS = 8_000 // Grace period before the live-work pruner may dispose a freshly opened // secondary socket; see the min-lifetime guard in pruneSecondaryGateways -// (#94769 prune ↔ redial race). -const SECONDARY_MIN_LIFETIME_MS = 30_000 +// (#94769 prune ↔ redial race). Exported for the tests that age a socket past it. +export const SECONDARY_MIN_LIFETIME_MS = 30_000 -// Wake-path liveness probe budget for a live-in-use secondary: mirrors -// GATEWAY_LIVENESS_PROBE_TIMEOUT_MS in use-gateway-boot (the primary's probe). -const SECONDARY_WAKE_PROBE_TIMEOUT_MS = 5_000 - -// Probe a live-in-use secondary instead of blind-closing it on a forced wake, -// and close it only when the probe proves it not alive. A half-open TCP -// connection (sleep/wake, silent network drop) reports connectionState -// 'open' forever and fires no close event, so without this close an in-flight -// request rides a dead transport until its per-call timeout — prompt.submit's -// is 30 minutes. Closing arms the entry's ordinary reconnect backoff via its -// onState('closed') handler; a healthy-but-busy backend answers the ping and -// keeps its socket (#94769 review). // A deferred liveness re-probe for one entry: cleared when the probe is // answered, the socket is redialed (fresh streak), or the entry is disposed. function clearSecondaryLivenessReprobe(entry: Secondary): void { @@ -1943,12 +1937,16 @@ function clearSecondaryLivenessReprobe(entry: Secondary): void { } } +// Probe a live-in-use secondary instead of blind-closing it on a forced wake, +// and close it only when the probe proves it not alive. A half-open TCP +// connection (sleep/wake, silent network drop) reports connectionState +// 'open' forever and fires no close event, so without this close an in-flight +// request rides a dead transport until its per-call timeout — prompt.submit's +// is 30 minutes. Closing arms the entry's ordinary reconnect backoff via its +// onState('closed') handler; a healthy-but-busy backend answers the ping and +// keeps its socket (#94769 review). function probeSecondaryLiveness(entry: Secondary): void { - if (typeof entry.gateway.request !== 'function') { - return - } - - void entry.gateway.request('ping', {}, SECONDARY_WAKE_PROBE_TIMEOUT_MS).then( + void entry.gateway.request('ping', {}, LIVENESS_PROBE_TIMEOUT_MS).then( () => { entry.livenessProbeFailures = 0 clearSecondaryLivenessReprobe(entry) @@ -1957,7 +1955,7 @@ function probeSecondaryLiveness(entry: Secondary): void { // -32601 (method not found) = a version-skewed but HEALTHY backend that // predates the ping method — the same compatibility carve-out the // primary's probe makes in use-gateway-boot. - if (error instanceof JsonRpcGatewayError && error.code === -32601) { + if (isMissingRpcMethod(error)) { entry.livenessProbeFailures = 0 clearSecondaryLivenessReprobe(entry) @@ -1979,8 +1977,13 @@ function probeSecondaryLiveness(entry: Secondary): void { // exhausted — or immediately when nothing is in flight to protect. entry.livenessProbeFailures += 1 + // Counted RPCs alone under-report in-flight work: prompt.submit returns + // before the turn ends, so a foreground turn mid tool call shows + // activeRequests 0. The registry's live-scope hook supplies the turn. + const turnInFlight = g.config?.liveScopes?.().has(entry.scope) ? 1 : 0 + const decision = decideLivenessForceClose({ - workingSessionCount: entry.activeRequests, + workingSessionCount: entry.activeRequests + turnInFlight, consecutiveFailures: entry.livenessProbeFailures }) @@ -2039,7 +2042,7 @@ export function reconnectSecondaryGateways({ forceOpenSockets = false }: { force // request would hang until its per-call timeout. Probe liveness instead — // a healthy-but-busy backend answers and keeps its socket; a dead // transport is closed and healed by the ordinary reconnect backoff. - if (entry.activeRequests > 0 || foregroundPinned(entry)) { + if (entry.activeRequests > 0 || relayRetained(entry) || foregroundPinned(entry)) { probeSecondaryLiveness(entry) continue @@ -2216,10 +2219,9 @@ export function pruneSecondaryGateways(keep: Set): void { // its consumer registered in the keep-set — closing it detaches the // runtime, the backend orphan-reaps it, and the reclaimed surface // re-resumes on a fresh socket the next recompute closes again: the - // #94769 flicker loop. A young socket rides one prune tick; the idle - // reap still catches it on a later recompute. Number guard: legacy/HMR - // entries may predate the field. - if (entry.lastOpenedAt > 0 && now - entry.lastOpenedAt < SECONDARY_MIN_LIFETIME_MS) { + // #94769 flicker loop. A young socket rides one prune tick; the keepalive + // tick recomputes the keep-set so an idle one is still reaped within a minute. + if (now - entry.lastOpenedAt < SECONDARY_MIN_LIFETIME_MS) { continue } diff --git a/apps/desktop/src/store/session-request-router.test.ts b/apps/desktop/src/store/session-request-router.test.ts index 376b6787af..579b1744a4 100644 --- a/apps/desktop/src/store/session-request-router.test.ts +++ b/apps/desktop/src/store/session-request-router.test.ts @@ -76,7 +76,8 @@ const { ensureGatewayForProfile, pruneSecondaryGateways, retireLocalProfileGateways, - setPrimaryGateway + setPrimaryGateway, + SECONDARY_MIN_LIFETIME_MS } = await import('./gateway') const { requestForSessionProfile, sessionRpcNeedsProfileRoute } = await import('./session-request-router') @@ -509,7 +510,7 @@ describe('requestForSessionProfile', () => { // Age the socket past the min-lifetime grace (#94769) so this prune // asserts the turn-lease release, not the freshly-opened spare. - vi.useFakeTimers({ now: Date.now() + 31_000 }) + vi.useFakeTimers({ now: Date.now() + SECONDARY_MIN_LIFETIME_MS + 1_000 }) pruneSecondaryGateways(new Set()) vi.useRealTimers() expect(secondaryGateways[0].close).toHaveBeenCalledOnce() From 4f766f8fa5f075339ab6df6ff4450755ed32a016 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:52:28 +0530 Subject: [PATCH 11/96] fix(desktop): profile-only secondaries count their live turn too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-gate follow-up: liveSessionScopes() holds only registry-scoped `conn:…::profile` keys, so a local/legacy secondary (connectionId null, scope = bare profile) never registered a turn in flight and one missed wake ping closed it mid tool call. The boot hook now derives one liveWorkScopes() — registry scopes plus the bare profile of every working/needs-input local session — that feeds both the pruner's keep-set (unchanged behaviour) and the registry's liveScopes hook; the probe matches the same way foregroundPinned does (composite key, or bare profile for a connectionId-less entry). The hook is referenced lazily because the registry is configured earlier in the effect than the helper's const. Also restores the misplaced doc comment on LIVENESS_PROBE_FAILURE_STREAK. --- .../src/app/gateway/hooks/use-gateway-boot.ts | 25 +++++++++++++------ .../src/lib/gateway-liveness-policy.ts | 2 +- apps/desktop/src/store/gateway.ts | 12 ++++++--- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts index 62eeff9c57..60846e343e 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts @@ -908,7 +908,8 @@ export function useGatewayBoot({ // primary thread or a just-created session's owner hold is bound to // (#93892). foregroundScopes: foregroundSessionScopes, - liveScopes: liveSessionScopes, + // Defined further down the effect body; read at call time, never during boot. + liveScopes: () => liveWorkScopes(), onLocalProfileRetired: forgetProfileOnlyRuntimeOwners, onActiveConnectionChanged: publish, // Keep $activeGatewayProfile in lockstep with the registry's OWN record @@ -1157,20 +1158,28 @@ export function useGatewayBoot({ // and its backend is free to idle-reap. The active profile is always spared. // Do not key this off `entry.retained` — that flag only skips dispose-after- // RPC; idle prune is what reclaims hover-warmed sockets after you leave. - const recomputeKeptGateways = () => { + // Scopes with a running or needs-input session: registry-scoped + // (connectionId, profile) keys plus the bare profile of every live local + // session. Two sources can expose the same profile name (every source has + // a 'default'), so bare profile names can't represent a non-local + // source's liveness without keeping the wrong gateway alive. Feeds the + // pruner's keep-set and the wake probe's in-flight-work signal. + const liveWorkScopes = (): Set => { const live = new Set([...$workingSessionIds.get(), ...$attentionSessionIds.get()]) - // Registry-scoped (connectionId, profile) scopes with live work. Two - // sources can expose the same profile name (every source has a - // 'default'), so bare profile names can't represent a non-local - // source's liveness without keeping the wrong gateway alive. - const keep = new Set([...liveSessionScopes(), ...foregroundSessionScopes()]) + const scopes = liveSessionScopes() for (const session of $sessions.get()) { if (live.has(session.id)) { - keep.add(normalizeProfileKey(session.profile)) + scopes.add(normalizeProfileKey(session.profile)) } } + return scopes + } + + const recomputeKeptGateways = () => { + const keep = new Set([...liveWorkScopes(), ...foregroundSessionScopes()]) + for (const scope of openTileGatewayScopes()) { keep.add(scope) } diff --git a/apps/desktop/src/lib/gateway-liveness-policy.ts b/apps/desktop/src/lib/gateway-liveness-policy.ts index b5653d521f..3f219a433b 100644 --- a/apps/desktop/src/lib/gateway-liveness-policy.ts +++ b/apps/desktop/src/lib/gateway-liveness-policy.ts @@ -34,10 +34,10 @@ * (mirroring gateway-liveness usage in use-gateway-boot). */ -/** Consecutive unanswered probes tolerated while work is in flight. */ /** Ping budget for a liveness probe (primary and secondary sockets share it). */ export const LIVENESS_PROBE_TIMEOUT_MS = 5_000 +/** Consecutive unanswered probes tolerated while work is in flight. */ export const LIVENESS_PROBE_FAILURE_STREAK = 2 /** How long after a deferred probe we try again (bounded, coalesced). */ diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index 77c01ca245..d318c3df3a 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -99,9 +99,11 @@ interface RegistryConfig { */ foregroundScopes?: () => ReadonlySet /** - * Scopes with a running or needs-input session runtime. The wake-path - * liveness probe counts these as in-flight work: prompt.submit returns - * before the turn ends, so `activeRequests` is 0 during most of a turn. + * Scopes with a running or needs-input session runtime, in the same key + * language as `foregroundScopes` (composite registry keys, bare profiles + * for local/legacy entries). The wake-path liveness probe counts these as + * in-flight work: prompt.submit returns before the turn ends, so + * `activeRequests` is 0 during most of a turn. */ liveScopes?: () => ReadonlySet } @@ -1980,7 +1982,9 @@ function probeSecondaryLiveness(entry: Secondary): void { // Counted RPCs alone under-report in-flight work: prompt.submit returns // before the turn ends, so a foreground turn mid tool call shows // activeRequests 0. The registry's live-scope hook supplies the turn. - const turnInFlight = g.config?.liveScopes?.().has(entry.scope) ? 1 : 0 + const liveScopes = g.config?.liveScopes?.() + const turnInFlight = + liveScopes && (liveScopes.has(entry.scope) || (!entry.connectionId && liveScopes.has(entry.profile))) ? 1 : 0 const decision = decideLivenessForceClose({ workingSessionCount: entry.activeRequests + turnInFlight, From 11b7a1e7fe5e7d344d38792f5a4c78021f2f08ed Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:15:29 +0530 Subject: [PATCH 12/96] fix(tui_gateway): the periodic memory trim waits until no session is busy or attached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idle reaper ran hermes_cli.mem_trim.trim_memory on every 300 s scan regardless of what the sessions were doing. The trim's gc.collect() holds the GIL and glibc malloc_trim(0) takes every arena lock, so on multi-GB heaps the event loop stalled 20-50 s each scan — long enough to blow the 10 s WS write deadline, drop the Desktop client and interrupt its running turn (#58576, Ufonik88's duration_ms 21065/49632 matching the stall lengths 1:1). The periodic trim now runs only on a quiescent scan: no session mid-turn, building, awaiting input, holding live delegations, or on a live transport (the LRU reaper's existing exemption predicate). Forced trims (agent close, cache pressure) are unchanged. Refs #58576 --- tests/tui_gateway/test_tui_gateway_server.py | 43 ++++++++++++++++++++ tui_gateway/session_reaper.py | 11 ++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/tests/tui_gateway/test_tui_gateway_server.py b/tests/tui_gateway/test_tui_gateway_server.py index 42d2de3c2b..705e2a8c61 100644 --- a/tests/tui_gateway/test_tui_gateway_server.py +++ b/tests/tui_gateway/test_tui_gateway_server.py @@ -19692,6 +19692,49 @@ def test_reap_idle_sessions_calls_periodic_trim(monkeypatch): server._sessions.clear() +def _periodic_trim_calls(monkeypatch): + """Stub the reaper's side effects and capture trim_memory calls (delayed import → patch the module attr).""" + import hermes_cli.mem_trim as mem_trim + + calls = [] + monkeypatch.setattr(server, "_session_pending_kind", lambda sid: "") + monkeypatch.setattr(server, "_close_session_by_id", lambda *a, **k: None) + monkeypatch.setattr(server, "_enforce_session_cap", lambda: None) + monkeypatch.setattr(server, "_reclaim_orphaned_leases", lambda: None) + monkeypatch.setattr(mem_trim, "trim_memory", lambda **kw: calls.append(kw.get("reason", "")) or True) + return calls + + +def test_periodic_trim_deferred_while_a_session_is_busy_or_attached(monkeypatch): + """The gen-2 collect + malloc_trim stalls the loop for its whole duration (#58576): it must not run + while any session is mid-turn or still holds a live client, whatever the other sessions look like.""" + calls = _periodic_trim_calls(monkeypatch) + now = time.time() + live = types.SimpleNamespace(_closed=False) + for busy in ({"running": True}, {"transport": live}): + server._sessions.clear() + server._sessions["idle"] = _idle_evictable_session(now) + server._sessions["busy"] = _idle_evictable_session(now) | {"last_active": now, "created_at": now} | busy + try: + server._reap_idle_sessions() + assert calls == [], busy + finally: + server._sessions.clear() + + +def test_periodic_trim_runs_once_every_session_is_quiescent(monkeypatch): + calls = _periodic_trim_calls(monkeypatch) + now = time.time() + server._sessions.clear() + # Recent (not TTL-evictable) but detached and idle: still quiescent, so the trim proceeds. + server._sessions["parked"] = _idle_evictable_session(now) | {"last_active": now, "created_at": now} + try: + server._reap_idle_sessions() + assert calls == ["idle reaper periodic trim"] + finally: + server._sessions.clear() + + def test_reap_idle_sessions_logs_trim_failure(monkeypatch, caplog): monkeypatch.setattr(server, "_session_pending_kind", lambda sid: "") monkeypatch.setattr(server, "_close_session_by_id", lambda *a, **k: None) diff --git a/tui_gateway/session_reaper.py b/tui_gateway/session_reaper.py index f825b99641..9ec7994c46 100644 --- a/tui_gateway/session_reaper.py +++ b/tui_gateway/session_reaper.py @@ -185,7 +185,16 @@ def _reap_idle_sessions() -> None: _enforce_session_cap() _reclaim_orphaned_leases() # Long-lived processes: gen2 GC rarely runs at steady state and glibc retains freed pages as RSS, so trim - # every scan to prevent unbounded RSS growth over days/weeks. + # every scan to prevent unbounded RSS growth over days/weeks. The trim holds the GIL (gc.collect) and every + # glibc arena lock (malloc_trim) for its whole duration — 20-50 s on multi-GB heaps — which stalls the event + # loop, drops WS clients past the write deadline and interrupts their turns (#58576). So the periodic trim + # waits for a quiescent scan: no session mid-turn, building, awaiting input, or on a live transport. Forced + # trims (agent close, cache pressure) are unaffected. + with _sessions_lock: + quiescent = all(_session_is_lru_evictable(sid, s) for sid, s in _sessions.items()) + if not quiescent: + logger.debug("idle reaper periodic trim deferred: a session is busy or attached") + return try: from hermes_cli.mem_trim import trim_memory trim_memory(reason="idle reaper periodic trim") From e0e54981e21ffd00c6d2460f1326f837f39b14b4 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:43:10 +0530 Subject: [PATCH 13/96] fix(tui_gateway): the turn-completion trim waits for the other sessions too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: prompt_turn.py::_finish_turn ran the non-forced "tui turn completion" trim on every turn end from the turn thread, while sessions B..N in the same process could be mid-turn — the same GIL + arena-lock stall the reaper gate closes, and it fires more often than the 300 s scan. The quiescence predicate moves to session_reaper.py::_sessions_quiescent(exclude=) so both callers share it; the finishing session is excluded because it is still marked running at that point, so a sole TUI session keeps its post-turn trim. Tests: one contract for the sibling path; the two pre-existing trim tests adopt the shared stub helper, the "every scan" docstring that the gate made false is folded into the quiescent case, and an inert age override is dropped from the busy case. --- tests/tui_gateway/test_tui_gateway_server.py | 65 +++++++++----------- tui_gateway/prompt_turn.py | 4 +- tui_gateway/session_reaper.py | 20 +++--- 3 files changed, 45 insertions(+), 44 deletions(-) diff --git a/tests/tui_gateway/test_tui_gateway_server.py b/tests/tui_gateway/test_tui_gateway_server.py index 705e2a8c61..07c2b8fd2c 100644 --- a/tests/tui_gateway/test_tui_gateway_server.py +++ b/tests/tui_gateway/test_tui_gateway_server.py @@ -19666,32 +19666,6 @@ def test_reap_idle_sessions_closes_only_evictable(monkeypatch): server._sessions.clear() -def test_reap_idle_sessions_calls_periodic_trim(monkeypatch): - """The idle reaper must call trim_memory every scan, even with no victims.""" - trim_calls = [] - monkeypatch.setattr(server, "_session_pending_kind", lambda sid: "") - monkeypatch.setattr(server, "_close_session_by_id", lambda *a, **k: None) - monkeypatch.setattr(server, "_enforce_session_cap", lambda: None) - monkeypatch.setattr(server, "_reclaim_orphaned_leases", lambda: None) - - # Patch the delayed import path: the function does - # `from hermes_cli.mem_trim import trim_memory` at call time. - import hermes_cli.mem_trim as mem_trim - - monkeypatch.setattr( - mem_trim, "trim_memory", - lambda **kw: trim_calls.append(kw.get("reason", "")) or True, - ) - - server._sessions.clear() - try: - server._reap_idle_sessions() - assert len(trim_calls) == 1 - assert trim_calls[0] == "idle reaper periodic trim" - finally: - server._sessions.clear() - - def _periodic_trim_calls(monkeypatch): """Stub the reaper's side effects and capture trim_memory calls (delayed import → patch the module attr).""" import hermes_cli.mem_trim as mem_trim @@ -19714,7 +19688,7 @@ def test_periodic_trim_deferred_while_a_session_is_busy_or_attached(monkeypatch) for busy in ({"running": True}, {"transport": live}): server._sessions.clear() server._sessions["idle"] = _idle_evictable_session(now) - server._sessions["busy"] = _idle_evictable_session(now) | {"last_active": now, "created_at": now} | busy + server._sessions["busy"] = _idle_evictable_session(now) | busy try: server._reap_idle_sessions() assert calls == [], busy @@ -19723,25 +19697,46 @@ def test_periodic_trim_deferred_while_a_session_is_busy_or_attached(monkeypatch) def test_periodic_trim_runs_once_every_session_is_quiescent(monkeypatch): + """Every quiescent scan trims, even with no victims: no sessions at all, or only recent (not yet + TTL-evictable) sessions that are detached and idle.""" calls = _periodic_trim_calls(monkeypatch) now = time.time() + for sessions in ({}, {"parked": _idle_evictable_session(now) | {"last_active": now, "created_at": now}}): + calls.clear() + server._sessions.clear() + server._sessions.update(sessions) + try: + server._reap_idle_sessions() + assert calls == ["idle reaper periodic trim"], sessions + finally: + server._sessions.clear() + + +def test_turn_completion_trim_skips_while_another_session_is_running(monkeypatch): + """The finishing session is still marked running when _finish_turn runs, so only OTHER sessions gate its + trim: a sole session trims at every turn end; a second in-flight turn defers it (#58576).""" + calls = _periodic_trim_calls(monkeypatch) + monkeypatch.setattr(server, "_clear_session_context", lambda tokens: None) + now = time.time() + own = _idle_evictable_session(now) | {"running": True, "transport": types.SimpleNamespace(_closed=False)} server._sessions.clear() - # Recent (not TTL-evictable) but detached and idle: still quiescent, so the trim proceeds. - server._sessions["parked"] = _idle_evictable_session(now) | {"last_active": now, "created_at": now} + server._sessions["own"] = own try: - server._reap_idle_sessions() - assert calls == ["idle reaper periodic trim"] + server._finish_turn("own", own, server._TurnRun(agent=None, one_turn_restore=None, terminal_callback=None, receipt_committed=True)) + assert calls == ["tui turn completion"] + + calls.clear() + server._sessions["other"] = _idle_evictable_session(now) | {"running": True} + server._finish_turn("own", own, server._TurnRun(agent=None, one_turn_restore=None, terminal_callback=None, receipt_committed=True)) + assert calls == [] finally: server._sessions.clear() def test_reap_idle_sessions_logs_trim_failure(monkeypatch, caplog): - monkeypatch.setattr(server, "_session_pending_kind", lambda sid: "") - monkeypatch.setattr(server, "_close_session_by_id", lambda *a, **k: None) - monkeypatch.setattr(server, "_enforce_session_cap", lambda: None) - monkeypatch.setattr(server, "_reclaim_orphaned_leases", lambda: None) import hermes_cli.mem_trim as mem_trim + _periodic_trim_calls(monkeypatch) monkeypatch.setattr(mem_trim, "trim_memory", lambda **_kw: (_ for _ in ()).throw(RuntimeError("boom"))) server._sessions.clear() try: diff --git a/tui_gateway/prompt_turn.py b/tui_gateway/prompt_turn.py index b06a99dade..fb55232711 100644 --- a/tui_gateway/prompt_turn.py +++ b/tui_gateway/prompt_turn.py @@ -799,7 +799,9 @@ def _finish_turn(sid: str, session: dict, st: _TurnRun) -> None: run_kwargs.clear() try: # while the profile HERMES_HOME override is still active (session's own config) from hermes_cli.mem_trim import trim_memory - trim_memory(reason="tui turn completion") + # The finishing session is still marked running here; every OTHER session must be idle (#58576). + if _sessions_quiescent(exclude=sid): + trim_memory(reason="tui turn completion") except Exception: logger.debug("post-turn memory trim failed", exc_info=True) if st.thinking_started: diff --git a/tui_gateway/session_reaper.py b/tui_gateway/session_reaper.py index 9ec7994c46..1aba5061a8 100644 --- a/tui_gateway/session_reaper.py +++ b/tui_gateway/session_reaper.py @@ -160,6 +160,15 @@ def _session_is_lru_evictable(sid: str, session: dict) -> bool: return _transport_is_dead(session.get("transport")) +def _sessions_quiescent(exclude: str | None = None) -> bool: + """No session but ``exclude`` is mid-turn, building, awaiting input, holding live delegations, or on a live + transport. A non-forced memory trim holds the GIL (gc.collect) and every glibc arena lock (malloc_trim) for + its whole duration — 20-50 s on multi-GB heaps — which stalls the event loop, drops WS clients past the + write deadline and interrupts their turns (#58576); this is the only moment it costs nobody.""" + with _sessions_lock: + return all(_session_is_lru_evictable(sid, s) for sid, s in _sessions.items() if sid != exclude) + + def _session_is_evictable(sid: str, session: dict, now: float) -> bool: """TTL eviction: the LRU exemptions plus idle-for-TTL AND older-than-TTL.""" if not _session_is_lru_evictable(sid, session): @@ -185,14 +194,9 @@ def _reap_idle_sessions() -> None: _enforce_session_cap() _reclaim_orphaned_leases() # Long-lived processes: gen2 GC rarely runs at steady state and glibc retains freed pages as RSS, so trim - # every scan to prevent unbounded RSS growth over days/weeks. The trim holds the GIL (gc.collect) and every - # glibc arena lock (malloc_trim) for its whole duration — 20-50 s on multi-GB heaps — which stalls the event - # loop, drops WS clients past the write deadline and interrupts their turns (#58576). So the periodic trim - # waits for a quiescent scan: no session mid-turn, building, awaiting input, or on a live transport. Forced - # trims (agent close, cache pressure) are unaffected. - with _sessions_lock: - quiescent = all(_session_is_lru_evictable(sid, s) for sid, s in _sessions.items()) - if not quiescent: + # every quiescent scan to prevent unbounded RSS growth over days/weeks. Forced trims (agent close, cache + # pressure) are unaffected. + if not _sessions_quiescent(): logger.debug("idle reaper periodic trim deferred: a session is busy or attached") return try: From 4626656c744f9d4fcf7f0419d4f016ffcffed9fc Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:55:27 +0530 Subject: [PATCH 14/96] refactor(tui_gateway): evaluate the quiescence predicate outside _sessions_lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-gate follow-up: _session_is_lru_evictable may read state.db (_session_has_active_delegations) and the predicate now also runs on the turn thread at every turn end; the repo already bounds state.db work under _sessions_lock (prompt_turn.py routing heal). Snapshot the other sessions under the lock, evaluate outside — the answer is advisory either way. --- tui_gateway/session_reaper.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tui_gateway/session_reaper.py b/tui_gateway/session_reaper.py index 1aba5061a8..081f0cf6c9 100644 --- a/tui_gateway/session_reaper.py +++ b/tui_gateway/session_reaper.py @@ -164,9 +164,12 @@ def _sessions_quiescent(exclude: str | None = None) -> bool: """No session but ``exclude`` is mid-turn, building, awaiting input, holding live delegations, or on a live transport. A non-forced memory trim holds the GIL (gc.collect) and every glibc arena lock (malloc_trim) for its whole duration — 20-50 s on multi-GB heaps — which stalls the event loop, drops WS clients past the - write deadline and interrupts their turns (#58576); this is the only moment it costs nobody.""" + write deadline and interrupts their turns (#58576); this is the moment it costs no other session. The + predicate is advisory (a turn can start right after), so the per-session checks — one may read state.db — + run outside ``_sessions_lock``.""" with _sessions_lock: - return all(_session_is_lru_evictable(sid, s) for sid, s in _sessions.items() if sid != exclude) + others = [(sid, s) for sid, s in _sessions.items() if sid != exclude] + return all(_session_is_lru_evictable(sid, s) for sid, s in others) def _session_is_evictable(sid: str, session: dict, now: float) -> bool: From efe042abe9216c09b4a987097f9d3693e5e18016 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:19:56 +0530 Subject: [PATCH 15/96] fix(shared): a failed gateway dial says which failure it hit JsonRpcGatewayClient.connect() rejected every failure with the bare connectErrorMessage, so the Desktop boot overlay showed "Could not connect to Hermes gateway" whether the server closed the handshake with 4403 token_mismatch, the socket errored before opening (TLS, DNS, refused) or nothing answered within the connect timeout. Reporters on #41566 verified their gateways with curl and a plain WebSocket client and still could not say what the app had hit. The rejection now carries the failure class: "WebSocket closed during handshake: code 4403 token_mismatch", "WebSocket error before open[: detail]" or "no WebSocket open within N ms". The base message stays as the prefix, so the web sidebar's includes()-based transport matchers and the desktop recovery tests are unaffected. Refs #41566 --- .../src/json-rpc-gateway-connect.test.ts | 75 +++++++++++++++++++ apps/shared/src/json-rpc-gateway.ts | 25 +++++-- 2 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 apps/shared/src/json-rpc-gateway-connect.test.ts diff --git a/apps/shared/src/json-rpc-gateway-connect.test.ts b/apps/shared/src/json-rpc-gateway-connect.test.ts new file mode 100644 index 0000000000..25e10b719f --- /dev/null +++ b/apps/shared/src/json-rpc-gateway-connect.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from 'vitest' + +import { JsonRpcGatewayClient } from './json-rpc-gateway' + +/** EventTarget-based WebSocket stand-in that never opens, so each failure leg can be driven by hand. */ +class StuckSocket extends EventTarget { + static OPEN = 1 + static last: StuckSocket | null = null + + readyState = 0 + + constructor() { + super() + StuckSocket.last = this + } + + send(): void {} + + close(): void { + this.readyState = 3 + } +} + +const connectErrorMessage = 'Could not connect to Hermes gateway' + +const rejection = (pending: Promise): Promise => + pending.then( + () => new Error('connect resolved'), + (error: Error) => error + ) + +const dial = (connectTimeoutMs = 1000) => { + const client = new JsonRpcGatewayClient({ + socketFactory: () => new StuckSocket() as unknown as WebSocket, + heartbeatIntervalMs: 0, + heartbeatDeadlineMs: 0, + connectTimeoutMs, + connectErrorMessage + }) + + const pending = client.connect('ws://gateway.test/api/ws') + pending.catch(() => {}) + + return { client, pending, socket: StuckSocket.last as StuckSocket } +} + +// Regression for #41566: the overlay showed the same sentence for an auth rejection, a TLS failure and a +// silent host, so users verified the gateway with curl and still could not tell what the app hit. +describe('JsonRpcGatewayClient.connect failure classes', () => { + it('a handshake close carries its code and reason, distinct from a timeout', async () => { + const closed = dial() + closed.socket.dispatchEvent(new CloseEvent('close', { code: 4403, reason: 'token_mismatch' })) + const closeError = await rejection(closed.pending) + + const timedOut = dial(20) + const timeoutError = await rejection(timedOut.pending) + + expect(closeError.message).toContain(connectErrorMessage) + expect(closeError.message).toContain('4403') + expect(closeError.message).toContain('token_mismatch') + expect(timeoutError.message).toContain(connectErrorMessage) + expect(timeoutError.message).toContain('20 ms') + expect(timeoutError.message).not.toBe(closeError.message) + expect(timedOut.client.connectionState).toBe('error') + }) + + it('an error before open still names the base message so existing "sidecar down" matchers keep working', async () => { + const { pending, socket } = dial() + socket.dispatchEvent(new Event('error')) + const error = await rejection(pending) + + expect(error.message.startsWith(connectErrorMessage)).toBe(true) + expect(error.message).toContain('error before open') + }) +}) diff --git a/apps/shared/src/json-rpc-gateway.ts b/apps/shared/src/json-rpc-gateway.ts index 365e6d0feb..9637ea0b59 100644 --- a/apps/shared/src/json-rpc-gateway.ts +++ b/apps/shared/src/json-rpc-gateway.ts @@ -252,7 +252,10 @@ export class JsonRpcGatewayClient { void this.fetchReplay() } - const onError = () => { + // Every rejection below names its failure class. The boot overlay renders this message verbatim, and + // a bare connectErrorMessage collapses "server refused the token", "TLS/DNS/refused before open" and + // "nothing answered" into one sentence nobody can act on (#41566). + const onError = (event: Event) => { if (settled || this.socket !== socket) { return } @@ -260,7 +263,10 @@ export class JsonRpcGatewayClient { settled = true cleanup() this.setState('error') - reject(new Error(this.options.connectErrorMessage)) + const detail = (event as { message?: unknown }).message + reject( + this.connectFailure(`WebSocket error before open${typeof detail === 'string' && detail ? `: ${detail}` : ''}`) + ) } // A server that closes during the handshake (auth gate, 4401/4403) @@ -270,7 +276,7 @@ export class JsonRpcGatewayClient { // and moved the generation to 'closed'; the branch below only runs // when `onSocketClose` intercepted that transition and left the // half-open socket bound. - const onClose = () => { + const onClose = (event: Event) => { if (settled) { return } @@ -283,7 +289,12 @@ export class JsonRpcGatewayClient { this.setState('error') } - reject(new Error(this.options.connectErrorMessage)) + const { code, reason } = event as { code?: unknown; reason?: unknown } + reject( + this.connectFailure( + `WebSocket closed during handshake: code ${typeof code === 'number' ? code : 'unknown'}${typeof reason === 'string' && reason ? ` ${reason}` : ''}` + ) + ) } socket.addEventListener('open', onOpen, { once: true }) @@ -312,12 +323,16 @@ export class JsonRpcGatewayClient { this.setState('error') } - reject(new Error(this.options.connectErrorMessage)) + reject(this.connectFailure(`no WebSocket open within ${this.options.connectTimeoutMs} ms`)) }, this.options.connectTimeoutMs) } }) } + private connectFailure(detail: string): Error { + return new Error(`${this.options.connectErrorMessage} (${detail})`) + } + close(): void { this.invalidate() } From 6338e988bdb436162c376751d6484f7745ee523e Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:55:09 +0530 Subject: [PATCH 16/96] refactor(shared): type the handshake close as CloseEvent; drop the dead error-detail branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: the close listener is a CloseEvent by construction (code is always a number), so the `as { code?: unknown }` cast and the 'unknown' fallback guarded nothing. Renderer 'error' events carry no message, and every consumer of this client is renderer-side, so the `: detail` suffix was never produced — the class alone is the detail. Test: the fake socket loses its unused OPEN/static-last bookkeeping and the prefix test asserts the real contract (prefix kept, not equal to the bare message) instead of prose. --- .../src/json-rpc-gateway-connect.test.ts | 18 ++++++------------ apps/shared/src/json-rpc-gateway.ts | 13 +++++-------- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/apps/shared/src/json-rpc-gateway-connect.test.ts b/apps/shared/src/json-rpc-gateway-connect.test.ts index 25e10b719f..bff00a1846 100644 --- a/apps/shared/src/json-rpc-gateway-connect.test.ts +++ b/apps/shared/src/json-rpc-gateway-connect.test.ts @@ -4,16 +4,8 @@ import { JsonRpcGatewayClient } from './json-rpc-gateway' /** EventTarget-based WebSocket stand-in that never opens, so each failure leg can be driven by hand. */ class StuckSocket extends EventTarget { - static OPEN = 1 - static last: StuckSocket | null = null - readyState = 0 - constructor() { - super() - StuckSocket.last = this - } - send(): void {} close(): void { @@ -30,8 +22,10 @@ const rejection = (pending: Promise): Promise => ) const dial = (connectTimeoutMs = 1000) => { + let socket!: StuckSocket + const client = new JsonRpcGatewayClient({ - socketFactory: () => new StuckSocket() as unknown as WebSocket, + socketFactory: () => (socket = new StuckSocket()) as unknown as WebSocket, heartbeatIntervalMs: 0, heartbeatDeadlineMs: 0, connectTimeoutMs, @@ -41,7 +35,7 @@ const dial = (connectTimeoutMs = 1000) => { const pending = client.connect('ws://gateway.test/api/ws') pending.catch(() => {}) - return { client, pending, socket: StuckSocket.last as StuckSocket } + return { client, pending, socket } } // Regression for #41566: the overlay showed the same sentence for an auth rejection, a TLS failure and a @@ -64,12 +58,12 @@ describe('JsonRpcGatewayClient.connect failure classes', () => { expect(timedOut.client.connectionState).toBe('error') }) - it('an error before open still names the base message so existing "sidecar down" matchers keep working', async () => { + it('every failure keeps the base message as its prefix — the overlay headline and includes() matchers bind to it', async () => { const { pending, socket } = dial() socket.dispatchEvent(new Event('error')) const error = await rejection(pending) expect(error.message.startsWith(connectErrorMessage)).toBe(true) - expect(error.message).toContain('error before open') + expect(error.message).not.toBe(connectErrorMessage) }) }) diff --git a/apps/shared/src/json-rpc-gateway.ts b/apps/shared/src/json-rpc-gateway.ts index 9637ea0b59..fe8f5759b1 100644 --- a/apps/shared/src/json-rpc-gateway.ts +++ b/apps/shared/src/json-rpc-gateway.ts @@ -255,7 +255,7 @@ export class JsonRpcGatewayClient { // Every rejection below names its failure class. The boot overlay renders this message verbatim, and // a bare connectErrorMessage collapses "server refused the token", "TLS/DNS/refused before open" and // "nothing answered" into one sentence nobody can act on (#41566). - const onError = (event: Event) => { + const onError = () => { if (settled || this.socket !== socket) { return } @@ -263,10 +263,8 @@ export class JsonRpcGatewayClient { settled = true cleanup() this.setState('error') - const detail = (event as { message?: unknown }).message - reject( - this.connectFailure(`WebSocket error before open${typeof detail === 'string' && detail ? `: ${detail}` : ''}`) - ) + // A browser/renderer 'error' event carries no detail; the class is the message. + reject(this.connectFailure('WebSocket error before open')) } // A server that closes during the handshake (auth gate, 4401/4403) @@ -276,7 +274,7 @@ export class JsonRpcGatewayClient { // and moved the generation to 'closed'; the branch below only runs // when `onSocketClose` intercepted that transition and left the // half-open socket bound. - const onClose = (event: Event) => { + const onClose = (event: CloseEvent) => { if (settled) { return } @@ -289,10 +287,9 @@ export class JsonRpcGatewayClient { this.setState('error') } - const { code, reason } = event as { code?: unknown; reason?: unknown } reject( this.connectFailure( - `WebSocket closed during handshake: code ${typeof code === 'number' ? code : 'unknown'}${typeof reason === 'string' && reason ? ` ${reason}` : ''}` + `WebSocket closed during handshake: code ${event.code}${event.reason ? ` ${event.reason}` : ''}` ) ) } From ad28e3a558a48b417d6aa89e6c47927279b6c981 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:47:54 +0530 Subject: [PATCH 17/96] chore(contributors): map dankkush (PR #111472 salvage) --- contributors/emails/brandan@wrengineers.com | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 contributors/emails/brandan@wrengineers.com diff --git a/contributors/emails/brandan@wrengineers.com b/contributors/emails/brandan@wrengineers.com new file mode 100644 index 0000000000..2c405020f2 --- /dev/null +++ b/contributors/emails/brandan@wrengineers.com @@ -0,0 +1,2 @@ +dankkush +# PR #111472 salvage From f0b88402dc8f9ebc9b360c94e2cd62fb268bbeb9 Mon Sep 17 00:00:00 2001 From: dankkush Date: Sat, 19 Sep 2026 11:51:46 +0530 Subject: [PATCH 18/96] =?UTF-8?q?test(agent):=20degenerate-final=20recover?= =?UTF-8?q?y=20=E2=80=94=20a=20collapsed=20fragment=20after=20tool=20work?= =?UTF-8?q?=20is=20re-prompted,=20not=20accepted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loop tests through run_conversation with a mocked client (harness mirrors test_dropped_tool_call_recovery): fragment after a tool round is re-prompted with the degenerate-final nudge and the real answer lands; a second fragment ends the turn (bounded, never a loop); a chat-only terse reply is untouched. Predicate tests pin the fragments the guard exists for and the terse legitimate answers a shape-only guard was shown to re-prompt. Adapted from PR #111472 to the reused ack-continuation path. --- tests/agent/test_degenerate_final_recovery.py | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 tests/agent/test_degenerate_final_recovery.py diff --git a/tests/agent/test_degenerate_final_recovery.py b/tests/agent/test_degenerate_final_recovery.py new file mode 100644 index 0000000000..573b7b313c --- /dev/null +++ b/tests/agent/test_degenerate_final_recovery.py @@ -0,0 +1,120 @@ +"""Degenerate-final recovery (#103483). + +Provider-side collapse: a turn executes its tool work correctly, then ends on ``finish_reason=stop`` +with an ENTIRE visible answer that is a fragment — a stray wrong-script word (``пар``), a token +starting mid-punctuation (``?warming up``). Before the fix the loop accepted the fragment as the +answer, the turn reported ``completed``, and an unattended job silently abandoned the task. + +The guard rides the existing ack-continuation path: same scope knob (``agent.intent_ack_continuation``, +default ``auto`` = Responses transports), same bounded per-turn counter, same durable interim + +nudge rows. Shape alone cannot prove a collapse, so the predicate is narrow and the nudge asks for +the same answer again when it WAS complete — a false positive costs one call, never the answer. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from agent.agent_runtime_helpers import looks_like_degenerate_final +from agent.conversation_loop import _DEGENERATE_FINAL_NUDGE + + +@pytest.fixture() +def loop_agent(): + """AIAgent with a mocked OpenAI client (mirrors test_dropped_tool_call_recovery).""" + from run_agent import AIAgent + with ( + patch("model_tools.get_tool_definitions", return_value=[]), + patch("model_tools.check_toolset_requirements", return_value={}), + patch("agent.process_bootstrap.OpenAI"), + ): + agent = AIAgent( + api_key="test-key-1234567890", + base_url="https://openrouter.ai/api/v1", + quiet_mode=True, + skip_context_files=True, + skip_memory=True, + ) + agent.client = MagicMock() + agent._cached_system_prompt = "You are helpful." + agent._use_prompt_caching = False + agent.tool_delay = 0 + agent.compression_enabled = False + agent.save_trajectories = False + # Explicit opt-in: the loop tests exercise the mechanism, not the transport-scoped default. + agent._intent_ack_continuation = True + return agent + + +def _run(agent, stages): + agent.client.chat.completions.create.side_effect = stages + with ( + patch.object(agent, "_persist_session"), + patch.object(agent, "_save_trajectory"), + patch.object(agent, "_cleanup_task_resources"), + patch("model_tools.handle_function_call", return_value="ok"), + ): + return agent.run_conversation("build the workbook") + + +def _tool_round(call_id): + from tests.agent.test_run_agent import _mock_response, _mock_tool_call + return _mock_response( + content="", finish_reason="tool_calls", + tool_calls=[_mock_tool_call(name="web_search", arguments="{}", call_id=call_id)], + ) + + +def _final(text): + from tests.agent.test_run_agent import _mock_response + return _mock_response(content=text, finish_reason="stop") + + +def _user_rows_sent(agent, call_index): + kwargs = agent.client.chat.completions.create.call_args_list[call_index].kwargs + return [m["content"] for m in kwargs["messages"] if m.get("role") == "user"] + + +class TestFragmentAfterToolWork: + def test_fragment_after_tool_work_reprompts_instead_of_exiting(self, loop_agent): + result = _run(loop_agent, [_tool_round("call_1"), _final("пар"), _final("Workbook built: 3 sheets.")]) + + assert result["final_response"] == "Workbook built: 3 sheets." + assert loop_agent.client.chat.completions.create.call_count == 3 + # The re-prompt carried the degenerate-final nudge, not the generic ack nudge. + assert _user_rows_sent(loop_agent, 2)[-1] == _DEGENERATE_FINAL_NUDGE + # The fragment stays in the transcript as an interim row, the nudge right after it. + contents = [m.get("content") for m in result["messages"]] + assert contents.index("пар") + 1 == contents.index(_DEGENERATE_FINAL_NUDGE) + + def test_persistent_collapse_is_bounded_and_keeps_the_fragment(self, loop_agent): + """One re-prompt per collapse: the nudge row closes the tool-work window, so a second + fragment is the answer — today's behaviour, never a loop.""" + result = _run(loop_agent, [_tool_round("call_1"), _final("пар"), _final("?warming up")]) + + assert loop_agent.client.chat.completions.create.call_count == 3 + assert result["final_response"] == "?warming up" + + def test_fragment_without_tool_work_is_a_plain_answer(self, loop_agent): + """A chat-only turn that answers tersely is not a collapse; nothing is re-prompted.""" + result = _run(loop_agent, [_final("пар")]) + + assert loop_agent.client.chat.completions.create.call_count == 1 + assert result["final_response"] == "пар" + + +class TestLooksLikeDegenerateFinal: + @pytest.mark.parametrize("text", ["пар", "?warming up", ",and", "ошибка", "你好", "):"]) + def test_fragments(self, text): + assert looks_like_degenerate_final(text) + + @pytest.mark.parametrize("text", [ + # Terse legitimate answers that reviewers showed a shape-only guard re-prompting (#111472). + "42", "SQLite", "report.csv", "3.14159", "€12.50", "你好。", "Done.", "True", "a51143fbbe", + "2026-09-19", "/tmp/out.log", "$5", "#123", "-1", "(a)", ".env", "~/x", "+1", "✅", + "Reading is a skill", "The answer is 43.", "x" * 25, "", + ]) + def test_terse_answers_are_not_fragments(self, text): + assert not looks_like_degenerate_final(text) From 80f0d1b52f97ee6430f99a66800020f649c6f505 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:51:46 +0530 Subject: [PATCH 19/96] fix(agent): re-prompt once when a turn that did tool work ends on a collapsed fragment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Responses-wire collapse (#103483): the tool work runs correctly, then the final text stop is a fragment — a stray wrong-script word ("пар"), a token starting mid-punctuation ("?warming up") — and the loop accepted it as the answer, so the turn reported completed and an unattended job abandoned the task. The guard rides the existing ack-continuation path in finalize_turn: same scope knob (agent.intent_ack_continuation, default auto = Responses transports), the same bounded per-turn counter, the same durable interim + nudge rows, and the same synthetic-user recognition in the compressor. It fires only when the turn has tool results after the last user row and the answer matches a deliberately narrow predicate: <= 24 chars, no sentence terminal, and either leading punctuation no answer begins with or letters with no ASCII letter/digit at all. "42", "SQLite", "report.csv", "€12.50", "你好。", "Done." never match. Because shape cannot prove a collapse, the nudge asks for the same answer again when it WAS complete, so a false positive costs one call, never the answer. The nudge row closes the tool-work window, so a second fragment ends the turn as before. Rebuilt from PR #111472 by dankkush against current main; the mid-task "stall note" arm and the ephemeral-row popping are not taken (the former false-positives on declarative answers, the latter buries flagged rows once a recovery response calls a tool). Co-authored-by: dankkush --- agent/agent_runtime_helpers.py | 36 ++++++++++++++++++++++++++++++++++ agent/context_compressor.py | 10 +++++----- agent/conversation_loop.py | 8 ++++++++ agent/turn_final_response.py | 32 ++++++++++++++++++++++++++---- 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 02f298bcde..5c9974862c 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -2961,6 +2961,42 @@ def looks_like_codex_intermediate_ack( ) +# Degenerate-final detector (#103483): after real tool work a text stop whose ENTIRE answer is a +# fragment — a stray wrong-script word ("пар"), a token starting mid-punctuation ("?warming up") — +# is a provider-side collapse, not an answer, yet the loop accepted it and the turn reported +# completed. Shape alone cannot PROVE a collapse, so this is deliberately narrower than "short": +# a terse legitimate answer ("42", "SQLite", "report.csv", "€12.50", "你好。", "Done.") never +# matches, and the re-prompt it triggers asks for the same answer again if it was complete. +_DEGENERATE_FINAL_MAX_CHARS = 24 +_SENTENCE_TERMINALS = (".", "!", "?", "\u3002", "\uff01", "\uff1f") +# Punctuation no answer begins with; "$5", "#123", "-1", "/tmp", ".env", "(a)" all stay answers. +_DEGENERATE_LEADING_PUNCT = "?!,;:)]}" + + +def looks_like_degenerate_final(text: str) -> bool: + """Whether a text stop reads as a collapsed fragment rather than a (terse) answer.""" + t = (text or "").strip() + if not t or len(t) > _DEGENERATE_FINAL_MAX_CHARS or t.endswith(_SENTENCE_TERMINALS): + return False + if t[0] in _DEGENERATE_LEADING_PUNCT: + return True + # A word with letters but no ASCII letter or digit at all: the wrong-script fragment. + return any(ch.isalpha() for ch in t) and not any(ch.isascii() and ch.isalnum() for ch in t) + + +def tool_results_this_turn(messages: List[Dict[str, Any]]) -> int: + """Tool-result rows after the most recent user row — whether the turn did real tool work.""" + count = 0 + for msg in reversed(messages or ()): + if not isinstance(msg, dict): + continue + if msg.get("role") == "user": + break + if msg.get("role") == "tool": + count += 1 + return count + + # Narrow "trailing continue-intent" detector for the stall guard (agent.stall_guards): only the # message TAIL announcing a next action, so mid-sentence "I will" never trips it. _TRAILING_CONTINUE_INTENT_RE = re.compile( diff --git a/agent/context_compressor.py b/agent/context_compressor.py index e2750fc09d..6c03642e49 100644 --- a/agent/context_compressor.py +++ b/agent/context_compressor.py @@ -3771,15 +3771,15 @@ Write only the summary body. Do not include any preamble or prefix.""" text = _content_text_for_contains(message.get("content")).strip() # Recovery nudges are scaffolding, not human turns; lazy import avoids an import cycle. from agent.conversation_loop import ( - _CODEX_ACK_CONTINUATION_NUDGE, _CODEX_INCOMPLETE_NUDGE, _DROPPED_TOOLCALL_NUDGE_CONTENT, - _EMPTY_TOOL_RESPONSE_NUDGE, _LENGTH_CONTINUATION_DROPPED_TOOLS_PREFIX, _LENGTH_CONTINUATION_NETWORK_STUB, - _LENGTH_CONTINUATION_OUTPUT_LIMIT, + _CODEX_ACK_CONTINUATION_NUDGE, _CODEX_INCOMPLETE_NUDGE, _DEGENERATE_FINAL_NUDGE, + _DROPPED_TOOLCALL_NUDGE_CONTENT, _EMPTY_TOOL_RESPONSE_NUDGE, _LENGTH_CONTINUATION_DROPPED_TOOLS_PREFIX, + _LENGTH_CONTINUATION_NETWORK_STUB, _LENGTH_CONTINUATION_OUTPUT_LIMIT, ) return text in { COMPRESSION_CONTINUATION_USER_CONTENT, _LEGACY_COMPRESSION_CONTINUATION_USER_CONTENT, MAX_ITERATIONS_SUMMARY_REQUEST, _CODEX_INCOMPLETE_NUDGE, _CODEX_ACK_CONTINUATION_NUDGE, - _DROPPED_TOOLCALL_NUDGE_CONTENT, _EMPTY_TOOL_RESPONSE_NUDGE, _LENGTH_CONTINUATION_NETWORK_STUB, - _LENGTH_CONTINUATION_OUTPUT_LIMIT, + _DEGENERATE_FINAL_NUDGE, _DROPPED_TOOLCALL_NUDGE_CONTENT, _EMPTY_TOOL_RESPONSE_NUDGE, + _LENGTH_CONTINUATION_NETWORK_STUB, _LENGTH_CONTINUATION_OUTPUT_LIMIT, } or text.startswith(( _BACKGROUND_PROCESS_NOTIFICATION_PREFIX, TODO_INJECTION_HEADER + "\n", _LENGTH_CONTINUATION_DROPPED_TOOLS_PREFIX, )) diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 4ce7008935..2343c74547 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -870,6 +870,14 @@ _CODEX_ACK_CONTINUATION_NUDGE = ( "after completing the task.]" ) +# Re-prompt after a collapsed fragment ended a turn that had done real tool work (#103483). Asks +# for the same answer again when it WAS complete, so a false positive costs one call, never the answer. +_DEGENERATE_FINAL_NUDGE = ( + "[System: Your previous message ended the turn with a fragment that is not a usable answer. " + "If the task is unfinished, continue it and then give the complete answer. If that fragment " + "WAS your complete answer, send it again exactly as before.]" +) + # Re-prompt for finish_reason="tool_calls" with empty tool_calls (an interrupt mid-retry can persist it). _DROPPED_TOOLCALL_NUDGE_CONTENT = ( "Your previous turn indicated a tool call but none was included. Do not narrate a plan or " diff --git a/agent/turn_final_response.py b/agent/turn_final_response.py index ecda17c965..dd568cf217 100644 --- a/agent/turn_final_response.py +++ b/agent/turn_final_response.py @@ -56,7 +56,8 @@ def finish_text_response( iteration-limit summarization; the final message is appended and flushed only after the stop gates accept it.""" from agent.conversation_loop import ( - _CODEX_ACK_CONTINUATION_NUDGE, _DROPPED_TOOLCALL_NUDGE_CONTENT, _join_truncated_parts + _CODEX_ACK_CONTINUATION_NUDGE, _DEGENERATE_FINAL_NUDGE, _DROPPED_TOOLCALL_NUDGE_CONTENT, + _join_truncated_parts ) def _verdict(action: str, result: Optional[Dict[str, Any]] = None) -> FinalResponseVerdict: @@ -142,7 +143,8 @@ def finish_text_response( # delivery channel (gateway status message / CLI print). NEVER appended to messages/api_messages: # conversation context and the cached prompt prefix stay byte-identical. from agent.agent_runtime_helpers import ( - intent_ack_continuation_mode, promoted_reasoning_announces_action, trailing_continue_intent + intent_ack_continuation_mode, looks_like_degenerate_final, promoted_reasoning_announces_action, + tool_results_this_turn, trailing_continue_intent, ) _ack_mode = intent_ack_continuation_mode(agent) @@ -162,7 +164,17 @@ def finish_text_response( or (bool(_promoted) and promoted_reasoning_announces_action(_stall_text)) ) ) - if _stall_continue_intent or ( + # Degenerate-final guard (#103483): the turn did real tool work and then stopped on a + # fragment. Same scope knob and the SAME bounded counter as the ack continuation; the nudge + # row itself closes the tool-work window, so a second fragment ends the turn as the answer. + _degenerate_final = ( + bool(getattr(agent, "_stall_guards", True)) + and _ack_mode != "off" + and codex_ack_continuations < 2 + and tool_results_this_turn(messages) > 0 + and looks_like_degenerate_final(_stall_text) + ) + if _stall_continue_intent or _degenerate_final or ( _ack_mode != "off" and agent.valid_tool_names and codex_ack_continuations < 2 @@ -177,6 +189,12 @@ def finish_text_response( "intent with no tool calls — re-prompting to act " "(%d/2)", codex_ack_continuations + 1, ) + elif _degenerate_final: + logger.warning( + "Degenerate final: %d-char fragment %r ended the turn after %d tool result(s) — " + "re-prompting (%d/2)", len(_stall_text), _stall_text[:40], + tool_results_this_turn(messages), codex_ack_continuations + 1, + ) codex_ack_continuations += 1 interim_msg = agent._build_assistant_message(assistant_message, "incomplete") if _promoted: @@ -185,7 +203,13 @@ def finish_text_response( interim_msg["api_content"] = final_response append_message(messages, interim_msg) agent._emit_interim_assistant_message(interim_msg) - append_message(messages, {"role": "user", "content": _CODEX_ACK_CONTINUATION_NUDGE}) + append_message(messages, { + "role": "user", + "content": ( + _DEGENERATE_FINAL_NUDGE if _degenerate_final and not _stall_continue_intent + else _CODEX_ACK_CONTINUATION_NUDGE + ), + }) agent._session_messages = messages # An acknowledgment is non-final: its text must not suppress iteration-limit # summarization if the continuation exhausts budget. From 112446afd1b292d44e248886bd5e1172ea663eb3 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 12:01:03 +0530 Subject: [PATCH 20/96] fix(agent): judge "wrong script" against the user's own message; one continuation kind per stop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the first cut: * The letters-but-no-ASCII arm flagged every terse non-Latin answer ("是", "Готово", "はい", "تم") after tool work, so a Chinese/Russian conversation on the Responses transport would pay a re-prompt on most one-word answers. "Wrong script" now means wrong relative to the conversation: when the user's message carries non-ASCII letters, a non-Latin reply is an answer. "пар" after an English prompt still counts. * The leading-punctuation arm caught ":8080", ":)", ";;", ":=", "}". It now requires a letter right after the punctuation ("?warming", "!warming"). * finalize_turn chose the continuation kind twice (log ladder and nudge pick); it is computed once, the tool-row count too. * tool_results_this_turn documents the load-bearing invariant: any user row (the nudges included) closes the window, which is what bounds the guard to one re-prompt per collapse. Tests: user-script negatives in Chinese/Russian/Japanese/Arabic incl. a multi-part user message; ":8080"/":)"/";;"/"}" negatives; both red when the script check or the letter-after rule is dropped. --- agent/agent_runtime_helpers.py | 40 +++++++++++++------ agent/turn_final_response.py | 26 ++++++++---- tests/agent/test_degenerate_final_recovery.py | 20 ++++++++-- 3 files changed, 62 insertions(+), 24 deletions(-) diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 5c9974862c..7ff760954d 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -2962,30 +2962,46 @@ def looks_like_codex_intermediate_ack( # Degenerate-final detector (#103483): after real tool work a text stop whose ENTIRE answer is a -# fragment — a stray wrong-script word ("пар"), a token starting mid-punctuation ("?warming up") — -# is a provider-side collapse, not an answer, yet the loop accepted it and the turn reported -# completed. Shape alone cannot PROVE a collapse, so this is deliberately narrower than "short": -# a terse legitimate answer ("42", "SQLite", "report.csv", "€12.50", "你好。", "Done.") never -# matches, and the re-prompt it triggers asks for the same answer again if it was complete. +# fragment — a stray wrong-script word ("пар" in an English conversation), a token starting +# mid-punctuation ("?warming up") — is a provider-side collapse, not an answer, yet the loop +# accepted it and the turn reported completed. Shape alone cannot PROVE a collapse, so this is +# deliberately narrower than "short": a terse legitimate answer ("42", "SQLite", "report.csv", +# "€12.50", "你好。", "Done.", ":8080", "да" to a Russian prompt) never matches, English-script +# fragments ("the", "ing") are knowingly not covered, and the re-prompt it triggers asks for the +# same answer again if it was complete. ``turn_finalizer._SENTENCE_END`` encodes the same +# "≤ 24 chars, no terminal" heuristic for the finish explainer. _DEGENERATE_FINAL_MAX_CHARS = 24 _SENTENCE_TERMINALS = (".", "!", "?", "\u3002", "\uff01", "\uff1f") -# Punctuation no answer begins with; "$5", "#123", "-1", "/tmp", ".env", "(a)" all stay answers. +# Punctuation no answer begins with when a letter follows ("?warming"); "$5", "#123", "-1", +# "/tmp", ".env", "(a)", ":8080", ":)", ";;" all stay answers. _DEGENERATE_LEADING_PUNCT = "?!,;:)]}" -def looks_like_degenerate_final(text: str) -> bool: - """Whether a text stop reads as a collapsed fragment rather than a (terse) answer.""" +def looks_like_degenerate_final(text: str, user_message: Any = None) -> bool: + """Whether a text stop reads as a collapsed fragment rather than a (terse) answer. + + "Wrong script" is judged against the conversation: when the user's own message carries + non-ASCII letters, a terse non-Latin reply ("是", "Готово") is an answer, not a collapse. + """ t = (text or "").strip() if not t or len(t) > _DEGENERATE_FINAL_MAX_CHARS or t.endswith(_SENTENCE_TERMINALS): return False - if t[0] in _DEGENERATE_LEADING_PUNCT: + if t[0] in _DEGENERATE_LEADING_PUNCT and len(t) > 1 and t[1].isalpha(): return True - # A word with letters but no ASCII letter or digit at all: the wrong-script fragment. - return any(ch.isalpha() for ch in t) and not any(ch.isascii() and ch.isalnum() for ch in t) + if not any(ch.isalpha() for ch in t) or any(ch.isascii() and ch.isalnum() for ch in t): + return False + from agent.codex_responses_adapter import _summarize_user_message_for_log + user_text = _summarize_user_message_for_log(user_message) if user_message else "" + return not any(ch.isalpha() and not ch.isascii() for ch in user_text) def tool_results_this_turn(messages: List[Dict[str, Any]]) -> int: - """Tool-result rows after the most recent user row — whether the turn did real tool work.""" + """Tool-result rows after the most recent user row — whether the turn did real tool work. + + ANY user row ends the window, the continuation nudges included: that is what bounds the + degenerate-final guard to one re-prompt per collapse. Skipping synthetic user rows here + would turn it into a two-nudge loop. + """ count = 0 for msg in reversed(messages or ()): if not isinstance(msg, dict): diff --git a/agent/turn_final_response.py b/agent/turn_final_response.py index dd568cf217..af36bc717f 100644 --- a/agent/turn_final_response.py +++ b/agent/turn_final_response.py @@ -167,14 +167,20 @@ def finish_text_response( # Degenerate-final guard (#103483): the turn did real tool work and then stopped on a # fragment. Same scope knob and the SAME bounded counter as the ack continuation; the nudge # row itself closes the tool-work window, so a second fragment ends the turn as the answer. + _tool_rows = tool_results_this_turn(messages) _degenerate_final = ( bool(getattr(agent, "_stall_guards", True)) and _ack_mode != "off" and codex_ack_continuations < 2 - and tool_results_this_turn(messages) > 0 - and looks_like_degenerate_final(_stall_text) + and _tool_rows > 0 + and looks_like_degenerate_final(_stall_text, user_message=user_message) ) - if _stall_continue_intent or _degenerate_final or ( + # Precedence: an announced next action outranks the fragment shape; the codex ack is last. + if _stall_continue_intent: + _continuation_kind = "stall" + elif _degenerate_final: + _continuation_kind = "degenerate" + elif ( _ack_mode != "off" and agent.valid_tool_names and codex_ack_continuations < 2 @@ -183,17 +189,21 @@ def finish_text_response( require_workspace=(_ack_mode == "codex_only"), ) ): - if _stall_continue_intent: + _continuation_kind = "ack" + else: + _continuation_kind = None + if _continuation_kind: + if _continuation_kind == "stall": logger.info( "Stall guard: turn ending on trailing continue-" "intent with no tool calls — re-prompting to act " "(%d/2)", codex_ack_continuations + 1, ) - elif _degenerate_final: + elif _continuation_kind == "degenerate": logger.warning( "Degenerate final: %d-char fragment %r ended the turn after %d tool result(s) — " - "re-prompting (%d/2)", len(_stall_text), _stall_text[:40], - tool_results_this_turn(messages), codex_ack_continuations + 1, + "re-prompting (%d/2)", len(_stall_text), _stall_text[:40], _tool_rows, + codex_ack_continuations + 1, ) codex_ack_continuations += 1 interim_msg = agent._build_assistant_message(assistant_message, "incomplete") @@ -206,7 +216,7 @@ def finish_text_response( append_message(messages, { "role": "user", "content": ( - _DEGENERATE_FINAL_NUDGE if _degenerate_final and not _stall_continue_intent + _DEGENERATE_FINAL_NUDGE if _continuation_kind == "degenerate" else _CODEX_ACK_CONTINUATION_NUDGE ), }) diff --git a/tests/agent/test_degenerate_final_recovery.py b/tests/agent/test_degenerate_final_recovery.py index 573b7b313c..5bfd654a6f 100644 --- a/tests/agent/test_degenerate_final_recovery.py +++ b/tests/agent/test_degenerate_final_recovery.py @@ -106,15 +106,27 @@ class TestFragmentAfterToolWork: class TestLooksLikeDegenerateFinal: - @pytest.mark.parametrize("text", ["пар", "?warming up", ",and", "ошибка", "你好", "):"]) - def test_fragments(self, text): - assert looks_like_degenerate_final(text) + EN = "Build the workbook and report back." + + @pytest.mark.parametrize("text", ["пар", "?warming up", ",and", "ошибка", "!warming"]) + def test_fragments_in_an_english_conversation(self, text): + assert looks_like_degenerate_final(text, user_message=self.EN) @pytest.mark.parametrize("text", [ # Terse legitimate answers that reviewers showed a shape-only guard re-prompting (#111472). "42", "SQLite", "report.csv", "3.14159", "€12.50", "你好。", "Done.", "True", "a51143fbbe", "2026-09-19", "/tmp/out.log", "$5", "#123", "-1", "(a)", ".env", "~/x", "+1", "✅", + ":8080", "::1", ":)", ";;", ":=", "}", "N/A", "**Done**", "`ok`", "{}", "null", "Reading is a skill", "The answer is 43.", "x" * 25, "", ]) def test_terse_answers_are_not_fragments(self, text): - assert not looks_like_degenerate_final(text) + assert not looks_like_degenerate_final(text, user_message=self.EN) + + @pytest.mark.parametrize("prompt, text", [ + ("把工作簿建好然后告诉我", "是"), ("把工作簿建好然后告诉我", "已完成"), + ("Собери таблицу и отчитайся", "Готово"), ("Собери таблицу и отчитайся", "да"), + ("ワークブックを作って報告して", "はい"), ("أنشئ المصنف ثم أخبرني", "تم"), + ([{"type": "text", "text": "Собери таблицу"}], "нет"), # multi-part user content + ]) + def test_terse_answers_in_the_users_own_script_are_not_fragments(self, prompt, text): + assert not looks_like_degenerate_final(text, user_message=prompt) From ded0789f9ac5e3a3b2daeb8bb8e51b738fea9998 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 12:05:03 +0530 Subject: [PATCH 21/96] docs(agent): the finish explainer's terminal set is a sibling heuristic, not the same one --- agent/agent_runtime_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 7ff760954d..093c9d5338 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -2968,7 +2968,7 @@ def looks_like_codex_intermediate_ack( # deliberately narrower than "short": a terse legitimate answer ("42", "SQLite", "report.csv", # "€12.50", "你好。", "Done.", ":8080", "да" to a Russian prompt) never matches, English-script # fragments ("the", "ing") are knowingly not covered, and the re-prompt it triggers asks for the -# same answer again if it was complete. ``turn_finalizer._SENTENCE_END`` encodes the same +# same answer again if it was complete. ``turn_finalizer._SENTENCE_END`` encodes a sibling # "≤ 24 chars, no terminal" heuristic for the finish explainer. _DEGENERATE_FINAL_MAX_CHARS = 24 _SENTENCE_TERMINALS = (".", "!", "?", "\u3002", "\uff01", "\uff1f") From cb9cddc73894814eba15bfa28de4b970cf409027 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:06:04 +0530 Subject: [PATCH 22/96] chore: map Anton Vykhovanets to @vykhovanets for #68621 salvage --- contributors/emails/anton.vykhovanets@icloud.com | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 contributors/emails/anton.vykhovanets@icloud.com diff --git a/contributors/emails/anton.vykhovanets@icloud.com b/contributors/emails/anton.vykhovanets@icloud.com new file mode 100644 index 0000000000..e71b6468aa --- /dev/null +++ b/contributors/emails/anton.vykhovanets@icloud.com @@ -0,0 +1,2 @@ +vykhovanets +# PR #68621 salvage From e1866bf7a69ae713151f7ed7150ded4e8227da74 Mon Sep 17 00:00:00 2001 From: Anton Vykhovanets Date: Tue, 21 Jul 2026 13:58:09 +0300 Subject: [PATCH 23/96] fix(desktop): create a project from a folder in one step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a folder is picked in the New Project dialog and no name was typed, auto-fill it from the folder's last path segment. The submit guard is relaxed so creation works with just a folder — the name is derived as a fallback. This removes the redundant naming step for the common case where the folder already has a meaningful name, and does so entirely within the left-sidebar Projects flow (no right-rail or per-session cwd involved). Creating with `use: true` set the active-project pointer but left the view scope at the overview, so a new chat still started detached. The dialog now calls `enterProject` on success, matching what clicking the project row does, so `pick folder → Create → working in it` completes in one gesture. The derivation imports the sidebar's existing `baseName` helper directly from its pure workspace-groups module rather than hand-rolling a POSIX-only split, so Windows paths resolve correctly without pulling the Projects component barrel into the dialog test. Both the submit path and the footer button's disabled state read one `resolvedName`, so the button can never render enabled but do nothing. --- .../src/app/chat/sidebar/project-dialog.tsx | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/apps/desktop/src/app/chat/sidebar/project-dialog.tsx b/apps/desktop/src/app/chat/sidebar/project-dialog.tsx index b29b6bc8ed..33d135f8e5 100644 --- a/apps/desktop/src/app/chat/sidebar/project-dialog.tsx +++ b/apps/desktop/src/app/chat/sidebar/project-dialog.tsx @@ -28,11 +28,14 @@ import { clearNewProjectDropPlacement, closeProjectDialog, createProject, + enterProject, generateProjectIdea, pickProjectFolder, renameProject } from '@/store/projects' +import { baseName } from './projects/workspace-groups' + // Single dialog mounted once in the sidebar; it renders create / rename / // add-folder flows driven by the $projectDialog atom. Folders are chosen via // the native directory picker (reused from the default-project-dir setting). @@ -67,6 +70,10 @@ export function ProjectDialog() { } }, [open]) + // Picking a folder with no name typed names the project after the folder (the + // ⌘O "Open folder…" naming), so one pick + Create is enough (#53004). + const resolvedName = name.trim() || (folders[0] ? (baseName(folders[0]) ?? '') : '') + useEffect(() => { if (open) { setName(state?.name ?? '') @@ -128,6 +135,14 @@ export function ProjectDialog() { } setFolders(prev => (prev.includes(dir) ? prev : [...prev, dir])) + + if (mode === 'create') { + const base = baseName(dir) + + if (base) { + setName(prev => (prev.trim() ? prev : base)) + } + } } catch (err) { notifyError(err, p.createFailed) } @@ -147,14 +162,23 @@ export function ProjectDialog() { // A project owns sessions by folder (cwd-prefix), so creation requires at // least one — a folder-less project couldn't hold a session anyway. - if (mode === 'create' && trimmed && folders.length) { + if (mode === 'create' && folders.length && resolvedName) { // The arm is consumed exactly on SUCCESS (before the close): a failed // create leaves the dialog open for a retry that still lands where it // was dropped; the open-state effect discards it on cancel/teardown. - await runSubmit( - () => createProject({ dropPlacement, folders, idea: idea.trim() || undefined, name: trimmed, use: true }), - clearNewProjectDropPlacement - ) + await runSubmit(async () => { + const created = await createProject({ + dropPlacement, + folders, + idea: idea.trim() || undefined, + name: resolvedName, + use: true + }) + + if (created) { + enterProject(created.id) + } + }, clearNewProjectDropPlacement) } } @@ -321,7 +345,7 @@ export function ProjectDialog() { {t.common.cancel} } description={c.tryAnother} icon="search" title={c.noResults} /> - ) : view === 'browse' && cardView ? ( - <> -
- {c.results(filtered.length)}} /> -
-
- {filtered.slice(0, limit).map(entry => ( -
- { - selectedCardRef.current = event.currentTarget - setSelectedId(entry.id) - setDetailOpen(true) - }} - > - - - {prettyName(entry.source)} - {entry.stars !== null && ☆ {entry.stars.toLocaleString()}} - - {entry.name} - {entry.description} - {entry.author || prettyName(entry.source)} - -
- {prettyName(entry.categoryLabel)} - {installButton(entry)} -
-
- ))} -
- {filtered.length > limit && } -
-
- - { - event.preventDefault() - selectedCardRef.current?.focus() - }} - > - {details} - - - ) : ( -
+
{c.results(filtered.length)}} />} key={`${source}:${category}:${deferredQuery}`}> {filtered.slice(0, limit).map(entry => ( @@ -312,7 +185,38 @@ export const CatalogBrowser = memo(function CatalogBrowser({
- {installedDetail || details} + {installedDetail || <> +
+
+ +
+

{selected.name}

+

{selected.author || prettyName(selected.source)}

+
+
+
+ {installButton(selected)} + {prettyName(selected.source)} + {selected.stars !== null && ☆ {selected.stars.toLocaleString()}} +
+
+
+

{c.about}

+

{selected.description}

+ {selected.overview && selected.overview !== selected.description &&

{selected.overview}

} +
+
+ {metadata.filter(([, value]) => Boolean(value)).map(([label, value]) =>
{label}
{value}
)} +
+ {[[c.tools, selected.tools], [c.hooks, selected.hooks], [c.requires, selected.requirements]].map(([label, values]) => (values as string[]).length > 0 && ( +

{label as string}

{(values as string[]).map(value => {value})}
+ ))} +
+ {selected.sourceUrl && {c.repository}} + {selected.docsUrl && {c.documentation}} +
+

{c.installHint}

+ }
diff --git a/apps/desktop/src/app/skills/index.test.tsx b/apps/desktop/src/app/skills/index.test.tsx index a99f14dd13..e1e9b60923 100644 --- a/apps/desktop/src/app/skills/index.test.tsx +++ b/apps/desktop/src/app/skills/index.test.tsx @@ -11,7 +11,6 @@ import type * as HubActions from '@/store/hub-actions' import { parseCatalog } from './catalog-data' import { SkillCatalog } from './skill-catalog' -import { $catalogCardView } from './store' const getSkills = vi.fn() const getToolsets = vi.fn() @@ -99,8 +98,6 @@ async function renderSkills() { } beforeEach(() => { - // Scope/install cases exercise the retained list layout; cards have dedicated coverage. - $catalogCardView.set(false) getSkills.mockResolvedValue([]) getToolsets.mockResolvedValue([toolset()]) setToolsetEnabled.mockResolvedValue({ ok: true, name: 'web', enabled: false }) diff --git a/apps/desktop/src/app/skills/plugins-tab.test.tsx b/apps/desktop/src/app/skills/plugins-tab.test.tsx index 15057d52cc..d6987e4e31 100644 --- a/apps/desktop/src/app/skills/plugins-tab.test.tsx +++ b/apps/desktop/src/app/skills/plugins-tab.test.tsx @@ -14,7 +14,6 @@ import { PageSearchShell } from '../page-search-shell' import { CapabilityTabs } from './capability-tabs' import { parseCatalog } from './catalog-data' import { PluginActions, PluginsTab } from './plugins-tab' -import { $catalogCardView } from './store' const requestGateway = vi.fn(async () => ({ plugins: $agentPlugins.get() })) @@ -90,7 +89,6 @@ describe('PluginsTab', () => { $pluginRecords.set({}) $agentPlugins.set([]) $agentPluginsStatus.set('ready') - $catalogCardView.set(false) closePluginInstallRequest() requestGateway.mockReset() requestGateway.mockImplementation(async () => ({ plugins: $agentPlugins.get() })) @@ -347,7 +345,6 @@ describe('PluginsTab catalog UX', () => { beforeEach(() => { $agentPlugins.set([]) $agentPluginsStatus.set('ready') - $catalogCardView.set(false) closePluginInstallRequest() requestGateway.mockReset() requestGateway.mockImplementation(async () => ({ plugins: $agentPlugins.get() })) diff --git a/apps/desktop/src/app/skills/store.ts b/apps/desktop/src/app/skills/store.ts index 7be2e7baa3..2105a5d45b 100644 --- a/apps/desktop/src/app/skills/store.ts +++ b/apps/desktop/src/app/skills/store.ts @@ -4,6 +4,3 @@ import { Codecs, persistentAtom } from '@/lib/persisted' // remembers most/least-used across navigations and restarts. export const $skillsSortDesc = persistentAtom('hermes.desktop.capabilities.skillsSortDesc', true, Codecs.bool) export const $toolsetsSortDesc = persistentAtom('hermes.desktop.capabilities.toolsetsSortDesc', true, Codecs.bool) - -// One browsing layout across Skills and Plugins; Installed keeps its own list. -export const $catalogCardView = persistentAtom('hermes.desktop.capabilities.catalogCardView', true, Codecs.bool) diff --git a/apps/desktop/src/i18n/ar.ts b/apps/desktop/src/i18n/ar.ts index 4ce519901c..e28a483653 100644 --- a/apps/desktop/src/i18n/ar.ts +++ b/apps/desktop/src/i18n/ar.ts @@ -2,8 +2,6 @@ import { defineLocale } from './define-locale' export const ar = defineLocale({ catalog: { - listView: 'عرض القائمة', - cardView: 'عرض البطاقات', installTitle: (name: string) => `تثبيت «${name}»؟`, installDescription: 'ستتوفر هذه المهارة في الجلسات الجديدة. ثبّت من المصادر التي تثق بها فقط.', installTo: 'التثبيت في', diff --git a/apps/desktop/src/i18n/en.ts b/apps/desktop/src/i18n/en.ts index 23445ac2fb..3ae0ee3ed1 100644 --- a/apps/desktop/src/i18n/en.ts +++ b/apps/desktop/src/i18n/en.ts @@ -4,8 +4,6 @@ import type { Translations } from './types' export const en: Translations = { catalog: { - listView: 'List view', - cardView: 'Card view', installTitle: (name: string) => `Install “${name}”?`, installDescription: 'This skill will be available in new sessions. Only install sources you trust.', installTo: 'Install to', diff --git a/apps/desktop/src/i18n/ja.ts b/apps/desktop/src/i18n/ja.ts index 26bb6ccf4b..7ffbe388e5 100644 --- a/apps/desktop/src/i18n/ja.ts +++ b/apps/desktop/src/i18n/ja.ts @@ -4,8 +4,6 @@ import { defineLocale } from './define-locale' export const ja = defineLocale({ catalog: { - listView: 'リスト表示', - cardView: 'カード表示', installTitle: (name: string) => `「${name}」をインストールしますか？`, installDescription: 'このスキルは新しいセッションで利用できます。信頼できる提供元からのみインストールしてください。', installTo: 'インストール先', diff --git a/apps/desktop/src/i18n/ru.ts b/apps/desktop/src/i18n/ru.ts index 2115298a84..ee2af97cc2 100644 --- a/apps/desktop/src/i18n/ru.ts +++ b/apps/desktop/src/i18n/ru.ts @@ -25,8 +25,6 @@ const RU_NOUN = (count: number | string, one: string, few: string, many: string) export const ru = defineLocale({ catalog: { - listView: 'Список', - cardView: 'Карточки', installTitle: (name: string) => `Установить «${name}»?`, installDescription: 'Навык будет доступен в новых сессиях. Устанавливайте только из источников, которым доверяете.', installTo: 'Установить в', diff --git a/apps/desktop/src/i18n/types.ts b/apps/desktop/src/i18n/types.ts index a4e1ee1506..c5f0ad2eba 100644 --- a/apps/desktop/src/i18n/types.ts +++ b/apps/desktop/src/i18n/types.ts @@ -60,8 +60,6 @@ interface AuxTaskCopy { export interface Translations { catalog: { - listView: string - cardView: string installTitle: (name: string) => string installDescription: string installTo: string diff --git a/apps/desktop/src/i18n/zh-hant.ts b/apps/desktop/src/i18n/zh-hant.ts index f324224bd1..86bf26e2ac 100644 --- a/apps/desktop/src/i18n/zh-hant.ts +++ b/apps/desktop/src/i18n/zh-hant.ts @@ -4,8 +4,6 @@ import { defineLocale } from './define-locale' export const zhHant = defineLocale({ catalog: { - listView: '清單檢視', - cardView: '卡片檢視', installTitle: (name: string) => `安裝「${name}」？`, installDescription: '此技能將於新的工作階段中可用。請僅安裝可信來源的內容。', installTo: '安裝至', diff --git a/apps/desktop/src/i18n/zh.ts b/apps/desktop/src/i18n/zh.ts index b750eb2e82..3cfd6b0f72 100644 --- a/apps/desktop/src/i18n/zh.ts +++ b/apps/desktop/src/i18n/zh.ts @@ -4,8 +4,6 @@ import { defineLocale } from './define-locale' export const zh = defineLocale({ catalog: { - listView: '列表视图', - cardView: '卡片视图', installTitle: (name: string) => `安装“${name}”？`, installDescription: '此技能将在新会话中可用。请仅安装可信来源的内容。', installTo: '安装到', diff --git a/website/docs/user-guide/desktop.md b/website/docs/user-guide/desktop.md index 65bfbaf62c..bd5e003c9f 100644 --- a/website/docs/user-guide/desktop.md +++ b/website/docs/user-guide/desktop.md @@ -221,7 +221,7 @@ When you have two or more [profiles](./profiles.md), the config-backed settings The app also surfaces the broader Hermes management surface so you don't have to drop to a terminal: -- **Skills** — open **Capabilities → Skills** to manage [skills](./features/skills.md). **Installed** shows the selected profile's actual skills and enable/disable state. **Browse** searches the same full published catalog as the public Skills Hub, with cards by default and an optional list/detail view. +- **Skills** — open **Capabilities → Skills** to manage [skills](./features/skills.md). **Installed** shows the selected profile's actual skills and enable/disable state. **Browse** searches the same full published catalog as the public Skills Hub, with native list and detail views. - **Plugins** — **Capabilities → Plugins** uses the same **Installed / Browse** layout. Installed combines actual app-level desktop plugins with agent plugins from the selected profile; Browse shows the public [Plugin Catalog](./features/plugin-catalog.md). Search stays at the top, and the tab switch and actions share one row on both pages. - **Memory graph (Star Map)** — type `/journey` (aliases `/learning`, `/memory-graph`) in chat to open an interactive constellation of learned skills and memories over time, with a playback scrubber. Nodes can be edited or deleted right from the panel (skills are archived, memories removed). See [Learning Journey](./features/memory.md#learning-journey-journey). - **Cron** — view and manage [scheduled jobs](../reference/cli-commands.md#hermes-cron). @@ -229,10 +229,6 @@ The app also surfaces the broader Hermes management surface so you don't have to - **Messaging** — set up gateway channels. Telegram has a **Quick setup** card: click **Create with QR**, scan the code (or open the link) in Telegram, and Hermes creates the bot, detects your user ID for the allowlist, saves the credentials, and restarts the gateway for you. Any credential save, clear, or enable toggle keeps a **Restart now** banner on the page until the gateway has actually restarted; if a restart fails, the banner stays so you can retry or restart manually. - **Agents** and **Command Center** — orchestration surfaces for multi-agent work. -Use the list and card icons at the right of the Browse filters to change layouts. -The choice is remembered across Skills and Plugins. Search and filters stay in -place; click a card to open its details or use its Install button directly. - #### Where Browse gets its data These are native Desktop views, **not embedded website pages**. Desktop and diff --git a/website/docs/user-guide/features/plugin-catalog.md b/website/docs/user-guide/features/plugin-catalog.md index 07e8d4bf74..bcf0f86571 100644 --- a/website/docs/user-guide/features/plugin-catalog.md +++ b/website/docs/user-guide/features/plugin-catalog.md @@ -24,9 +24,6 @@ view. It is not an embedded website. **Installed** is a separate tab backed by the app's desktop-plugin registry and the selected profile's agent-plugin state, rather than catalog metadata. Skills uses the same **Installed / Browse** layout; search stays at the top and the tab switch and actions share one row. -Browse defaults to cards. The list and card icons beside the filters switch -layouts, preserving search and filters and remembering the choice across both -catalogs. The catalog complements — it does not replace — the existing [plugin system](plugins.md). Anything you can install from the catalog is a diff --git a/website/docs/user-guide/features/plugins.md b/website/docs/user-guide/features/plugins.md index b1aab7ae3f..09400508e1 100644 --- a/website/docs/user-guide/features/plugins.md +++ b/website/docs/user-guide/features/plugins.md @@ -388,10 +388,7 @@ registry and the selected profile's actual agent-plugin state, combining both halves in one row where appropriate. It is not a list of catalog entries assumed to be installed. **Browse** is a native catalog view, not an embedded website; it uses the same **Installed / Browse** tabs as Skills, with search -at the top and the tab switch and actions on one row. Browse defaults to cards, -with list and card icons at the right of the filters. The layout choice is shared -with Skills and remembered. Click a card to read its details; Install opens the -existing review-then-install dialog. +at the top and the tab switch and actions on one row. Desktop and the public [Plugin Catalog](/plugins) consume the same CDN snapshot, [`/docs/api/plugins.json`](https://hermes-agent.nousresearch.com/docs/api/plugins.json). diff --git a/website/docs/user-guide/features/skills.md b/website/docs/user-guide/features/skills.md index 07522d5e62..5ebc743cd4 100644 --- a/website/docs/user-guide/features/skills.md +++ b/website/docs/user-guide/features/skills.md @@ -23,10 +23,7 @@ Open **Capabilities → Skills** and switch between **Installed** and **Browse** Search stays at the top; the tab switch and actions share one row. **Installed** reads the selected profile's actual skills and enabled state; it is not inferred from the public catalog. **Browse** is a native catalog UI, -not an embedded website or a second, smaller catalog. Cards are the default; -the list and card icons at the right of the filters switch layouts without -clearing search or filters. The choice is remembered across Skills and Plugins. -Click a card for details or use its Install button directly. +not an embedded website or a second, smaller catalog. Desktop and the public [Skills Hub](/skills) read the same published CDN snapshot: [`/docs/api/skills.json`](https://hermes-agent.nousresearch.com/docs/api/skills.json). From 8bd0da2b8c9ef36163bce6b21e0ccc6d90258a09 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 18 Sep 2026 23:50:04 -0700 Subject: [PATCH 29/96] Revert "feat(desktop): restore native skill and plugin catalogs" This reverts commit cbd76e4ea312e9a3172730671fec95fc25f3f9c2. --- .../hooks/use-desktop-integrations.test.tsx | 92 +--- .../contrib/hooks/use-desktop-integrations.ts | 16 +- apps/desktop/src/app/master-detail.tsx | 9 +- .../settings/plugin-install-modal.test.tsx | 3 +- .../src/app/skills/capability-tabs.tsx | 29 -- .../src/app/skills/catalog-browser.tsx | 227 ---------- .../src/app/skills/catalog-data.test.ts | 136 ------ apps/desktop/src/app/skills/catalog-data.ts | 157 ------- .../src/app/skills/embedded-hub-picker.tsx | 249 +++++++++++ apps/desktop/src/app/skills/index.test.tsx | 287 ++++--------- apps/desktop/src/app/skills/index.tsx | 296 +++++++++++-- .../src/app/skills/plugins-tab.test.tsx | 321 ++++++-------- apps/desktop/src/app/skills/plugins-tab.tsx | 398 ++++++++++++++---- apps/desktop/src/app/skills/skill-catalog.tsx | 46 -- .../src/app/skills/update-skills-button.tsx | 21 - .../src/components/confirm-host.test.tsx | 36 -- apps/desktop/src/components/confirm-host.tsx | 26 +- .../src/components/ui/confirm-dialog.tsx | 3 - apps/desktop/src/i18n/ar.ts | 37 -- apps/desktop/src/i18n/en.ts | 37 -- apps/desktop/src/i18n/ja.ts | 37 -- apps/desktop/src/i18n/ru.ts | 38 -- apps/desktop/src/i18n/types.ts | 37 -- apps/desktop/src/i18n/zh-hant.ts | 37 -- apps/desktop/src/i18n/zh.ts | 37 -- apps/desktop/src/lib/catalog-install.test.ts | 37 -- apps/desktop/src/lib/deeplink-routes.test.ts | 48 --- apps/desktop/src/lib/deeplink-routes.ts | 27 +- .../src/lib/hermes-open-target.test.ts | 1 - apps/desktop/src/lib/hermes-open-target.ts | 3 +- apps/desktop/src/lib/query-client.ts | 1 - apps/desktop/src/store/confirm.ts | 48 +-- .../src/store/skill-deeplink-install.test.ts | 100 ----- .../src/store/skill-deeplink-install.ts | 62 --- apps/shared/src/catalog-install.ts | 40 -- apps/shared/src/index.ts | 1 - 36 files changed, 1052 insertions(+), 1928 deletions(-) delete mode 100644 apps/desktop/src/app/skills/capability-tabs.tsx delete mode 100644 apps/desktop/src/app/skills/catalog-browser.tsx delete mode 100644 apps/desktop/src/app/skills/catalog-data.test.ts delete mode 100644 apps/desktop/src/app/skills/catalog-data.ts create mode 100644 apps/desktop/src/app/skills/embedded-hub-picker.tsx delete mode 100644 apps/desktop/src/app/skills/skill-catalog.tsx delete mode 100644 apps/desktop/src/app/skills/update-skills-button.tsx delete mode 100644 apps/desktop/src/lib/catalog-install.test.ts delete mode 100644 apps/desktop/src/store/skill-deeplink-install.test.ts delete mode 100644 apps/desktop/src/store/skill-deeplink-install.ts delete mode 100644 apps/shared/src/catalog-install.ts diff --git a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.test.tsx b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.test.tsx index 0633d6f50f..dca32b58a0 100644 --- a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.test.tsx +++ b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.test.tsx @@ -1,13 +1,9 @@ -import { act, renderHook, waitFor } from '@testing-library/react' +import { renderHook } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { setApiRequestConnection, setApiRequestProfile } from '@/hermes' import { createClientSessionState } from '@/lib/chat-runtime' import { adoptNewSessionDraft, stashSessionDraft, takeSessionDraft } from '@/store/composer' -import { $confirmRequest, runConfirm, settleConfirm } from '@/store/confirm' -import { $hubInstalledOverride } from '@/store/hub-actions' import { requestMcpInstallFromDeepLink } from '@/store/mcp-deeplink-install' -import { $pluginInstallRequest } from '@/store/plugin-install-request' import { _resetLegacyDiscardForTests } from '@/store/session' import { dropSessionState, publishSessionState } from '@/store/session-states' import type * as WindowsStore from '@/store/windows' @@ -589,92 +585,6 @@ describe('useDesktopIntegrations', () => { }) }) - describe('catalog install deep links', () => { - function listen() { - render({ profileReady: true, resumeLastSession: false }) - - return vi.mocked(window.hermesDesktop.onDeepLink!).mock.calls[0]![0] - } - - afterEach(() => { - settleConfirm(false) - $pluginInstallRequest.set(null) - $hubInstalledOverride.set({}) - setApiRequestConnection(null) - setApiRequestProfile(null) - }) - - it('opens the existing plugin confirmation with catalog metadata intact', () => { - const deepLink = listen() - const params = { repo: 'owner/repo#plugin', catalog_name: 'catalog-plugin', sha: 'display-pin' } - deepLink({ kind: 'plugin', name: 'install', params }) - - expect($pluginInstallRequest.get()).toMatchObject({ - repo: params.repo, catalogName: params.catalog_name, sha: params.sha, enable: true, force: false - }) - expect(navigate).not.toHaveBeenCalled() - }) - - it('requires skill confirmation, preserves the request scope, and uses the hub pipeline', async () => { - const api = vi.fn(async (request: { path: string }) => { - if (request.path === '/api/skills/hub/install') { - return { name: 'skill-link-test' } - } - - if (request.path.startsWith('/api/actions/skill-link-test/')) { - return { name: 'skill-link-test', running: false, exit_code: 0, lines: ['Installed'], pid: 123 } - } - - return {} - }) - - desktopWindow.hermesDesktop = { ...desktopWindow.hermesDesktop, api } as unknown as Window['hermesDesktop'] - const deepLink = listen() - const installs = () => api.mock.calls.filter(([r]) => r.path === '/api/skills/hub/install') - const identifier = 'skills-sh/owner/repo/skill' - const payload = { kind: 'skill', name: 'install', params: { identifier } } - - for (const [connection, profile] of [['server-a', 'research'], ['server-b', 'work'], ['server-a', 'research']]) { - setApiRequestConnection(connection) - setApiRequestProfile(profile) - api.mockClear() - $hubInstalledOverride.set({}) - act(() => deepLink(payload)) - expect($confirmRequest.get()?.title).toBe('Install “skill”?') - expect($confirmRequest.get()?.details).toEqual([ - { label: 'Source', value: identifier }, - { label: 'Install to', value: `${connection} · ${profile}` } - ]) - expect(installs()).toHaveLength(0) - await act(async () => settleConfirm(false)) - expect(installs()).toHaveLength(0) - - act(() => deepLink(payload)) - await act(async () => runConfirm($confirmRequest.get()!)) - expect($confirmRequest.get()?.phase).toBe('done') - settleConfirm(true) - await waitFor(() => expect($hubInstalledOverride.get()[identifier]).toBe(true)) - expect(installs()).toEqual([[{ - connectionId: connection, profile, priority: 'foreground', path: '/api/skills/hub/install', method: 'POST', body: { identifier } - }]]) - expect(api).toHaveBeenCalledWith({ - connectionId: connection, profile, priority: 'foreground', path: '/api/actions/skill-link-test/status?lines=200' - }) - } - - api.mockClear() - act(() => deepLink(payload)) - setApiRequestConnection('server-b') - setApiRequestProfile('work') - await expect(runConfirm($confirmRequest.get()!)).rejects.toThrow('The destination changed') - settleConfirm(false) - expect(installs()).toHaveLength(0) - act(() => deepLink({ kind: 'skill', name: 'install', params: {} })) - expect($confirmRequest.get()).toBeNull() - expect(navigate).not.toHaveBeenCalled() - }) - }) - describe('notification click -> focus-session id translation', () => { function withFocusSession(): (sessionId: string) => void { let handler: ((sessionId: string) => void) | undefined diff --git a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts index a2d72ed60e..6ad036723b 100644 --- a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts +++ b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts @@ -30,7 +30,6 @@ import { } from '@/store/session' import { $botChatScopes, $sessionTiles, storedSessionIdForRuntimeId } from '@/store/session-states' import { onSessionsChanged } from '@/store/session-sync' -import { requestSkillInstallFromDeepLink } from '@/store/skill-deeplink-install' import { openUpdatesWindow, startUpdatePoller, stopUpdatePoller } from '@/store/updates' import { isBrowserWindow, isHudWindow, isSecondaryWindow } from '@/store/windows' import type { SessionInfo } from '@/types/hermes' @@ -298,7 +297,6 @@ export function useDesktopIntegrations({ // - mcp/install?… → pending MCP install (explicit confirm, never auto-install) // - plugin/install?… (and legacy plugin-agent/plugin-desktop) → plugin install // modal awaiting explicit confirmation. Never auto-installs. - // - skill/install?identifier=… → confirmation, then the existing hub pipeline // - blueprint/?… → reviewable /blueprint command in the composer // - /?… → in-app navigate (e.g. index-network/intent/1) // - open/?… → in-app navigate (generic) @@ -337,24 +335,12 @@ export function useDesktopIntegrations({ repo: action.repo, enable: action.enable, force: action.force, - legacyHint: action.legacyHint, - catalogName: action.catalogName, - sha: action.sha + legacyHint: action.legacyHint }) return } - if (action.type === 'skill-install') { - void requestSkillInstallFromDeepLink(action.identifier) - - return - } - - if (payload.kind === 'skill') { - return - } - // Not a core action — treat as a plugin-scoped or open/ navigation deep // link (hermes://index-network/intent/1, hermes://open/…). The resolver // rejects reserved kinds and unsafe paths. diff --git a/apps/desktop/src/app/master-detail.tsx b/apps/desktop/src/app/master-detail.tsx index dddc87e421..95e117697b 100644 --- a/apps/desktop/src/app/master-detail.tsx +++ b/apps/desktop/src/app/master-detail.tsx @@ -391,12 +391,6 @@ export function ListStripMenu({ ) } -const LIST_STRIP_LABEL_CLASS = 'text-[0.68rem] font-medium text-muted-foreground/70' - -export function ListStripLabel({ children }: { children: ReactNode }) { - return {children} -} - export function ListStripButton({ active, children, @@ -411,8 +405,7 @@ export function ListStripButton({ return ( - ) - } - - const metadata = selected ? [ - [c.author, selected.author], - [c.source, prettyName(selected.source)], - [c.category, prettyName(selected.categoryLabel)], - [c.version, selected.version], - [c.platforms, selected.platforms.join(', ')], - [c.requires, selected.requiresHermes ? `Hermes ${selected.requiresHermes}` : ''], - [c.pinned, selected.sha ? {selected.sha.slice(0, 8)} : ''] - ] as [string, ReactNode][] : [] - - return ( -
- {view === 'browse' &&
-
-
- -
-
- -
-
-
} -
- {isPending && view === 'browse' && !entries.length ? : error && !entries.length ? ( -
- - - -
- ) : !selected ? ( - {c.clearFilters}} description={c.tryAnother} icon="search" title={c.noResults} /> - ) : ( -
- - {c.results(filtered.length)}} />} key={`${source}:${category}:${deferredQuery}`}> - {filtered.slice(0, limit).map(entry => ( - { setSelectedId(entry.id); setDetailOpen(true) }} - > - - - - {entry.name} - {isInstalled(entry) && } - - {entry.description} - - {entry.author || prettyName(entry.source)} - {entry.stars !== null && ☆ {entry.stars.toLocaleString()}} - - - - ))} - {filtered.length > limit && } - - -
- {installedDetail || <> -
-
- -
-

{selected.name}

-

{selected.author || prettyName(selected.source)}

-
-
-
- {installButton(selected)} - {prettyName(selected.source)} - {selected.stars !== null && ☆ {selected.stars.toLocaleString()}} -
-
-
-

{c.about}

-

{selected.description}

- {selected.overview && selected.overview !== selected.description &&

{selected.overview}

} -
-
- {metadata.filter(([, value]) => Boolean(value)).map(([label, value]) =>
{label}
{value}
)} -
- {[[c.tools, selected.tools], [c.hooks, selected.hooks], [c.requires, selected.requirements]].map(([label, values]) => (values as string[]).length > 0 && ( -

{label as string}

{(values as string[]).map(value => {value})}
- ))} -
- {selected.sourceUrl && {c.repository}} - {selected.docsUrl && {c.documentation}} -
-

{c.installHint}

- } - - -
- )} -
-
- ) -}) diff --git a/apps/desktop/src/app/skills/catalog-data.test.ts b/apps/desktop/src/app/skills/catalog-data.test.ts deleted file mode 100644 index 4062999cf4..0000000000 --- a/apps/desktop/src/app/skills/catalog-data.test.ts +++ /dev/null @@ -1,136 +0,0 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' - -import { fetchCatalog, parseCatalog } from './catalog-data' - -afterEach(() => vi.unstubAllGlobals()) - -describe('public catalog data', () => { - it('preserves every source and distinct install identifier, including same-named skills', () => { - const rows = ['official', 'github', 'community', 'future-source'].map(source => ({ - name: 'research', - identifier: `${source}/research`, - source, - author: `${source} maintainer`, - description: `Research from ${source}`, - category: 'research', - categoryLabel: 'Research', - tags: [`${source}-tag`], - envVars: [`${source.toUpperCase()}_KEY`] - })) - // Same source, different identifier is also a distinct install target. - rows.push({ ...rows[0], identifier: 'official/alternate/research' }) - - const entries = parseCatalog('skills', rows) - - expect(entries).toHaveLength(rows.length) - expect(new Set(entries.map(entry => entry.id)).size).toBe(rows.length) - rows.forEach(row => { - const entry = entries.find(candidate => candidate.identifier === row.identifier) - expect(entry).toMatchObject({ - name: row.name, - identifier: row.identifier, - source: row.source, - author: row.author, - description: row.description, - categoryLabel: row.categoryLabel, - tags: row.tags, - requirements: row.envVars - }) - expect(entry?.search).toContain(row.source) - expect(entry?.search).toContain(row.tags[0]) - }) - }) - - it('keeps catalog identity separate from the source-qualified skill install target', () => { - const [entry] = parseCatalog('skills', [{ - name: 'Apple Design', identifier: 'apple-design', source: 'ClawHub', - installCmd: 'hermes skills install clawhub/apple-design' - }]) - - expect(entry.identifier).toBe('apple-design') - expect(entry.installIdentifier).toBe('clawhub/apple-design') - }) - - it('keeps each plugin tier, pinned install target, and published detail metadata together', () => { - const rows = ['official', 'community', 'future-tier'].map(tier => ({ - name: `${tier}-plugin`, - tier, - repo: `https://github.com/example/${tier}-plugins`, - subdir: 'packages/weather', - sha: 'a'.repeat(40), - maintainer: `${tier} maintainer`, - overview: `Overview for ${tier}`, - version: '1.2.3', - requiresHermes: '>=0.17', - platforms: ['macos', 'linux'], - docsUrl: `https://example.com/${tier}/docs`, - capabilities: { - providesTools: [`${tier}_forecast`], - providesHooks: ['on_session_start'], - requiresEnv: ['WEATHER_KEY'] - } - })) - - const entries = parseCatalog('plugins', rows) - - expect(entries).toHaveLength(rows.length) - rows.forEach(row => { - const entry = entries.find(candidate => candidate.name === row.name) - expect(entry).toMatchObject({ - name: row.name, - source: row.tier, - repo: row.repo, - sha: row.sha, - subdir: row.subdir, - author: row.maintainer, - overview: row.overview, - version: row.version, - requiresHermes: row.requiresHermes, - platforms: row.platforms, - docsUrl: row.docsUrl, - sourceUrl: row.repo, - tools: row.capabilities.providesTools, - hooks: row.capabilities.providesHooks, - requirements: row.capabilities.requiresEnv - }) - expect(entry?.search).toContain(row.capabilities.providesTools[0]) - }) - }) - - it.each(['skills', 'plugins'] as const)('fetches only the published %s snapshot and parses it', async kind => { - const rows = [{ - name: 'weather', - identifier: 'official/weather', - source: 'official', - tier: 'official', - repo: 'https://github.com/example/weather', - sourceUrl: 'https://github.com/example/weather/blob/main/SKILL.md' - }] - const fetch = vi.fn().mockResolvedValue({ ok: true, json: async () => rows }) - vi.stubGlobal('fetch', fetch) - - expect(await fetchCatalog(kind)).toEqual(parseCatalog(kind, rows)) - expect(fetch).toHaveBeenCalledExactlyOnceWith( - `https://nousresearch.github.io/hermes-agent/docs/api/${kind}.json`, - expect.objectContaining({ credentials: 'omit', signal: expect.any(AbortSignal) }) - ) - }) - - it('only renders plugin images hosted on GitHub so the browser never fans out to third-party hosts', () => { - const base = { name: 'x', tier: 'community', repo: 'https://github.com/o/r', sha: 'a'.repeat(40), version: '1.4.0' } - const [github, offhost, http] = parseCatalog('plugins', [ - { ...base, name: 'github', image: 'https://raw.githubusercontent.com/o/r/abc/banner.png' }, - { ...base, name: 'offhost', image: 'https://cdn.example.com/banner.png' }, - { ...base, name: 'http', image: 'http://github.com/o/r/banner.png' } - ]) - - expect(github.imageUrl).toBe('https://raw.githubusercontent.com/o/r/abc/banner.png') - expect(github.version).toBe('1.4.0') - expect(offhost.imageUrl).toBeNull() - expect(http.imageUrl).toBeNull() - }) - - it('rejects a non-catalog response instead of treating an error payload as an empty catalog', () => { - expect(() => parseCatalog('skills', { error: 'Service unavailable' })).toThrow('Invalid catalog response') - }) -}) diff --git a/apps/desktop/src/app/skills/catalog-data.ts b/apps/desktop/src/app/skills/catalog-data.ts deleted file mode 100644 index e8774431e3..0000000000 --- a/apps/desktop/src/app/skills/catalog-data.ts +++ /dev/null @@ -1,157 +0,0 @@ -import { skillCatalogInstallIdentifier } from '@hermes/shared' -import { useQuery } from '@tanstack/react-query' - -export type CatalogKind = 'skills' | 'plugins' - -export interface CatalogEntry { - id: string - name: string - description: string - overview: string - category: string - categoryLabel: string - source: string - author: string - identifier: string - installIdentifier?: string | null - repo: string - sha: string - subdir: string - version: string - requiresHermes: string - tags: string[] - platforms: string[] - requirements: string[] - tools: string[] - hooks: string[] - sourceUrl: string | null - docsUrl: string | null - /** GitHub-hosted banner for plugin entries; the only third-party fetch the browser makes. */ - imageUrl: string | null - stars: number | null - search: string -} - -const DOCS_ORIGIN = 'https://hermes-agent.nousresearch.com' -// The public domain redirects here without CORS headers on the redirect. -// Use the docs' actual static host, not GitHub's API or repository endpoints. -const CATALOG_BASE = 'https://nousresearch.github.io/hermes-agent/docs/api' -const text = (value: unknown): string => typeof value === 'string' ? value : '' -const strings = (value: unknown): string[] => Array.isArray(value) ? value.filter(v => typeof v === 'string') : [] - -const IMAGE_HOSTS = new Set(['raw.githubusercontent.com', 'github.com']) - -/** Mirrors scripts/validate_plugin_catalog.py: https on a GitHub host, else no image. */ -export function catalogImageUrl(value: unknown): string | null { - try { - const url = new URL(text(value)) - const host = url.hostname.toLowerCase() - - return url.protocol === 'https:' && (IMAGE_HOSTS.has(host) || host.endsWith('.githubusercontent.com')) ? url.href : null - } catch { - return null - } -} - -function webUrl(value: unknown): string | null { - try { - const url = new URL(text(value)) - - return url.protocol === 'https:' || url.protocol === 'http:' ? url.href : null - } catch { - return null - } -} - -export function parseCatalog(kind: CatalogKind, data: unknown): CatalogEntry[] { - if (!Array.isArray(data)) { - throw new Error('Invalid catalog response') - } - - const entries = new Map() - - for (const row of data) { - if (!row || typeof row !== 'object' || !text(row.name)) { - continue - } - - const name = text(row.name) - const source = text(kind === 'plugins' ? row.tier : row.source) - const identifier = text(row.identifier) || name - const id = `${source}:${identifier}` - const caps = row.capabilities ?? {} - const category = text(row.category) || 'uncategorized' - const tags = strings(row.tags) - const tools = strings(caps.providesTools) - const hooks = strings(caps.providesHooks) - const author = text(row.maintainer ?? row.author) - const description = text(row.description) - - entries.set(id, { - id, - name, - description, - overview: text(row.overview), - category, - categoryLabel: text(row.categoryLabel) || category, - source, - author, - identifier, - installIdentifier: kind === 'skills' ? skillCatalogInstallIdentifier({ - name, source, identifier: text(row.identifier), installIdentifier: text(row.installIdentifier) - }) : null, - repo: text(row.repo), - sha: text(row.sha), - subdir: text(row.subdir), - version: text(row.version), - requiresHermes: text(row.requiresHermes), - tags, - tools, - hooks, - platforms: strings(row.platforms), - requirements: strings(kind === 'plugins' ? caps.requiresEnv : row.envVars), - sourceUrl: webUrl(row.repo || row.sourceUrl), - docsUrl: webUrl(row.docsUrl) || (text(row.docsPath) - ? `${DOCS_ORIGIN}/docs/user-guide/skills/${text(row.docsPath)}` - : null), - imageUrl: kind === 'plugins' ? catalogImageUrl(row.image) : null, - stars: typeof row.stars === 'number' && Number.isFinite(row.stars) ? row.stars : null, - search: [name, description, author, category, row.categoryLabel, source, ...tags, ...tools, ...hooks] - .filter(Boolean).join(' ').toLowerCase() - }) - } - - return [...entries.values()] -} - -export async function fetchCatalog(kind: CatalogKind): Promise { - // These are the same published snapshots as the docs galleries. Never fan - // out to repositories, README previews, avatars, or live hub searches. - const response = await fetch(`${CATALOG_BASE}/${kind}.json`, { - credentials: 'omit', - signal: AbortSignal.timeout(60_000) - }) - - if (!response.ok) { - throw new Error(`Catalog HTTP ${response.status}`) - } - - return parseCatalog(kind, await response.json()) -} - -export function useCatalog(kind: CatalogKind, enabled = true) { - return useQuery({ - queryKey: ['public-catalog', kind], - // Re-enabling a mounted query retries errors even with retryOnMount off. - // Keep failures parked until the user explicitly chooses Try again. - enabled: query => enabled && query.state.status !== 'error', - queryFn: () => fetchCatalog(kind), - staleTime: 30 * 60_000, - gcTime: Infinity, - refetchOnWindowFocus: false, - refetchOnReconnect: false, - refetchOnMount: false, - retryOnMount: false, - retry: false - }) -} diff --git a/apps/desktop/src/app/skills/embedded-hub-picker.tsx b/apps/desktop/src/app/skills/embedded-hub-picker.tsx new file mode 100644 index 0000000000..9139cecbe9 --- /dev/null +++ b/apps/desktop/src/app/skills/embedded-hub-picker.tsx @@ -0,0 +1,249 @@ +import { useStore } from '@nanostores/react' +import { memo, type PointerEvent as ReactPointerEvent, useEffect, useRef, useState } from 'react' + +import { Button } from '@/components/ui/button' +import type { ProfileScope } from '@/hermes' +import { useI18n } from '@/i18n' +import { Loader2 } from '@/lib/icons' +import { useStoreSelector } from '@/lib/use-session-slice' +import { cn } from '@/lib/utils' +import { $hubActions, installHubSkill, notifyHubActionFailed, UPDATE_ALL_KEY, updateHubSkills } from '@/store/hub-actions' +import { notify, notifyError } from '@/store/notifications' +import { $paneHeightOverride, setPaneHeightOverride } from '@/store/panes' + +// The REAL Skills Hub page (docs site) embedded as a one-click picker — the +// same trick the Bot Mode agent editor uses. `?embed=picker` hides the docs +// chrome and adds a "+ Add to this Agent" button per card, which posts +// { type: 'hermes-skill-pick', name, identifier, installCmd, source } +// to the parent window. We validate the origin and route the install through +// the standard hub action pipeline (background action + tailed log + Skills +// list invalidation), scoped to the Capabilities profile selector. +const HUB_ORIGIN = 'https://hermes-agent.nousresearch.com' +const HUB_PICKER_URL = `${HUB_ORIGIN}/docs/skills?embed=picker` + +// Hub viewport height: persisted through the shared pane store (same one the +// terminal/editor panes use), dragged from the section's TOP edge — "pull the +// hub up" — clamped so neither the hub nor the skills list above vanishes. +const HUB_PANE_ID = 'capabilities-hub' +const HUB_DEFAULT_PX = 380 +const HUB_MIN_PX = 120 +const HUB_MAX_VH = 0.75 +// Collapse threshold, mirroring DetailPane: a persisted height at/below this +// reads as "collapsed to the header" (the toggle stores 0). +const HUB_COLLAPSED_PX = 4 +// Room the sash must always leave for the content ABOVE the picker (the +// installed-skills list plus its strip) so dragging the hub up can never +// crush the list to zero and shove its chrome under the hub header. +const HUB_LIST_RESERVED_PX = 176 + +interface SkillPickMessage { + identifier?: string + installCmd?: string + name?: string + source?: string + type?: string +} + +interface EmbeddedHubPickerProps { + /** Kept mounted but fully hidden (display:none). The Capabilities view uses + * this to preserve the loaded hub iframe across tab switches — a plain + * unmount would reload the whole docs site on every return to Skills. */ + hidden?: boolean + /** Names of skills already installed in the scoped profile — a pick that + * matches is refused with a toast instead of re-running the install. */ + installedNames: ReadonlySet + /** Capabilities profile-scope override — installs land in THIS profile; + * undefined/null targets the app-wide active profile. */ + profile?: ProfileScope +} + +/** The Skills Hub browser for the Skills tab: a resizable iframe of the live + * hub where every card installs with one click. Expanded by default — + * discovery IS the point — with a collapse toggle (persisted, like every + * other pane) and an update-all action. Memoized: the iframe must not sit in + * the parent's keystroke/re-render path. */ +export const EmbeddedHubPicker = memo(function EmbeddedHubPicker({ + hidden = false, + installedNames, + profile +}: EmbeddedHubPickerProps) { + const { t } = useI18n() + const h = t.skills.hub + // Subscribe to the ONE flag this header renders, not the whole action map — + // $hubActions churns on every tailed log line during an install. + const updating = useStoreSelector($hubActions, actions => actions[UPDATE_ALL_KEY]?.running ?? false) + // Collapse state rides the same persisted height override the sash writes + // (0 = collapsed to the header), so "Hide the hub browser" survives tab + // switches and restarts instead of re-expanding — and re-loading the docs + // site — on every visit. Same contract as DetailPane. + const heightOverride = useStore($paneHeightOverride(HUB_PANE_ID)) + const height = heightOverride ?? HUB_DEFAULT_PX + const open = height > HUB_COLLAPSED_PX + const [dragging, setDragging] = useState(false) + const sectionRef = useRef(null) + + // Top-edge sash: dragging UP grows the hub (shrinking the skills list above, + // which is the flex-1 sibling). Same gesture as DetailPane / the shell's + // bottom panes; double-click resets to the default height. The iframe gets + // pointer-events disabled for the duration or it swallows the pointermoves. + const startDrag = (event: ReactPointerEvent) => { + if (event.button !== 0) { + return + } + + event.preventDefault() + const startY = event.clientY + const startHeight = height + // Clamp against the actual Capabilities column, not just the window: the + // hub may never grow past "column minus the list's reserved strip", so + // the installed list always keeps real height and its header/footer can't + // end up sharing pixels with the hub header. + const column = sectionRef.current?.parentElement + const columnMax = column ? column.clientHeight - HUB_LIST_RESERVED_PX : Number.POSITIVE_INFINITY + const max = Math.max(HUB_MIN_PX, Math.round(Math.min(window.innerHeight * HUB_MAX_VH, columnMax))) + setDragging(true) + + const onMove = (move: globalThis.PointerEvent) => { + setPaneHeightOverride( + HUB_PANE_ID, + Math.round(Math.min(max, Math.max(HUB_MIN_PX, startHeight + (startY - move.clientY)))) + ) + } + + const onUp = () => { + window.removeEventListener('pointermove', onMove) + setDragging(false) + } + + window.addEventListener('pointermove', onMove) + window.addEventListener('pointerup', onUp, { once: true }) + } + + // Picker messages from the embedded hub page. Origin-checked; installs route + // through the same store pipeline the hub rows use, so the action log, + // optimistic flips, and Skills-list refresh all come for free. + useEffect(() => { + if (!open) { + return undefined + } + + const onMessage = (event: MessageEvent) => { + if (event.origin !== HUB_ORIGIN) { + return + } + + const data = event.data as SkillPickMessage | null + + if (!data || data.type !== 'hermes-skill-pick' || !data.name) { + return + } + + const target = String(data.identifier || data.name) + const label = String(data.name) + + // Already installed in this scope → tell the user, don't reinstall. + if (installedNames.has(label) || installedNames.has(target)) { + notify({ kind: 'success', title: h.alreadyInstalled(label), message: '' }) + + return + } + + notify({ kind: 'success', title: h.installStarted(label), message: h.actionLog }) + void installHubSkill(target, profile).catch(err => notifyHubActionFailed(err, h.actionFailed, label, profile)) + } + + window.addEventListener('message', onMessage) + + return () => window.removeEventListener('message', onMessage) + }, [h, installedNames, open, profile]) + + const updateAll = () => { + notify({ kind: 'success', title: h.updateStarted, message: h.actionLog }) + void updateHubSkills(profile).catch(err => notifyError(err, h.actionFailed)) + } + + return ( +