From 4858942c552733f72de5b2d0dfdfcc7a3a1dc248 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 16 Jun 2026 06:23:24 -0700 Subject: [PATCH 1/5] fix(auxiliary): honor main fallback chain for auto tasks (#47235) --- agent/auxiliary_client.py | 186 +++++++++++++++--- tests/agent/test_auxiliary_client.py | 31 +++ tests/agent/test_auxiliary_main_first.py | 58 ++++++ .../run_agent/test_async_httpx_del_neuter.py | 7 +- .../docs/reference/environment-variables.md | 4 +- website/docs/user-guide/configuring-models.md | 18 +- .../user-guide/features/fallback-providers.md | 24 ++- 7 files changed, 290 insertions(+), 38 deletions(-) diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 01ea45d7be2..86a1c765a78 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -3079,23 +3079,20 @@ def _try_configured_fallback_chain( if not fb_provider or fb_provider.lower() == skip: continue fb_model = str(entry.get("model", "")).strip() or None - fb_base_url = str(entry.get("base_url", "")).strip() or None - fb_api_key = str(entry.get("api_key", "")).strip() or None label = f"fallback_chain[{i}]({fb_provider})" try: - fb_client = _resolve_single_provider( - fb_provider, fb_model, fb_base_url, fb_api_key) + fb_client, resolved_model = _resolve_fallback_entry(entry) except Exception: - fb_client = None + fb_client, resolved_model = None, None if fb_client is not None: logger.info( "Auxiliary %s: %s on %s — configured fallback to %s (%s)", - task, reason, failed_provider, label, fb_model or "default", + task, reason, failed_provider, label, resolved_model or fb_model or "default", ) - return fb_client, fb_model, label + return fb_client, resolved_model or fb_model, label tried.append(label) if tried: @@ -3106,6 +3103,103 @@ def _try_configured_fallback_chain( return None, None, "" +def _fallback_entry_api_key(entry: Dict[str, Any]) -> Optional[str]: + """Resolve inline or env-backed API key from a fallback-chain entry.""" + explicit = str(entry.get("api_key") or "").strip() + if explicit: + return explicit + key_env = str(entry.get("key_env") or entry.get("api_key_env") or "").strip() + if key_env: + return os.getenv(key_env, "").strip() or None + return None + + +def _resolve_fallback_entry(entry: Dict[str, Any]) -> Tuple[Optional[Any], Optional[str]]: + """Resolve one fallback entry through the central provider router.""" + provider = str(entry.get("provider") or "").strip() + model = str(entry.get("model") or "").strip() or None + if not provider or not model: + return None, None + base_url = str(entry.get("base_url") or "").strip() or None + api_key = _fallback_entry_api_key(entry) + api_mode = str(entry.get("api_mode") or entry.get("transport") or "").strip() or None + return resolve_provider_client( + provider, + model=model, + explicit_base_url=base_url, + explicit_api_key=api_key, + api_mode=api_mode, + ) + + +def _try_main_fallback_chain( + task: Optional[str], + failed_provider: str = "", + reason: str = "error", +) -> Tuple[Optional[Any], Optional[str], str]: + """Try the top-level main-agent fallback chain for an auxiliary call. + + ``provider: auto`` auxiliary tasks should respect the user's declared + main fallback policy before dropping into Hermes' built-in discovery + chain. The top-level chain is read through ``get_fallback_chain`` so + both modern ``fallback_providers`` and legacy ``fallback_model`` entries + participate in the same order as the main agent. + """ + try: + from hermes_cli.config import load_config + from hermes_cli.fallback_config import get_fallback_chain + + chain = get_fallback_chain(load_config()) + except Exception as exc: + logger.debug("Auxiliary %s: could not load main fallback chain: %s", task or "call", exc) + return None, None, "" + + if not chain: + return None, None, "" + + failed_norm = (failed_provider or "").strip().lower() + main_norm = (_read_main_provider() or "").strip().lower() + skip = {p for p in (failed_norm, main_norm, "auto") if p} + tried: List[str] = [] + + for i, entry in enumerate(chain): + if not isinstance(entry, dict): + continue + fb_provider = str(entry.get("provider") or "").strip() + fb_model = str(entry.get("model") or "").strip() + if not fb_provider or not fb_model: + continue + fb_norm = fb_provider.lower() + label = f"fallback_providers[{i}]({fb_provider})" + if fb_norm in skip: + tried.append(f"{label} (skipped)") + continue + if _is_provider_unhealthy(fb_norm): + _log_skip_unhealthy(fb_norm, task) + tried.append(f"{label} (unhealthy)") + continue + try: + fb_client, resolved_model = _resolve_fallback_entry(entry) + except Exception as exc: + logger.debug("Auxiliary %s: main fallback %s failed to resolve: %s", task or "call", label, exc) + fb_client, resolved_model = None, None + if fb_client is not None: + logger.info( + "Auxiliary %s: %s on %s — main fallback chain to %s (%s)", + task or "call", reason, failed_provider or "auto", label, + resolved_model or fb_model, + ) + return fb_client, resolved_model or fb_model, fb_provider + tried.append(label) + + if tried: + logger.debug( + "Auxiliary %s: main fallback chain exhausted (tried: %s)", + task or "call", ", ".join(tried), + ) + return None, None, "" + + def _resolve_single_provider( provider: str, model: Optional[str] = None, @@ -3116,16 +3210,19 @@ def _resolve_single_provider( Uses the existing provider resolution infrastructure where possible. """ - # Reuse resolve_provider_client which handles provider→client mapping + # Reuse resolve_provider_client which handles provider→client mapping. client, resolved_model = resolve_provider_client( provider=provider, model=model, - base_url=base_url, - api_key=api_key, + explicit_base_url=base_url, + explicit_api_key=api_key, ) return client -def _resolve_auto(main_runtime: Optional[Dict[str, Any]] = None) -> Tuple[Optional[OpenAI], Optional[str]]: +def _resolve_auto( + main_runtime: Optional[Dict[str, Any]] = None, + task: Optional[str] = None, +) -> Tuple[Optional[OpenAI], Optional[str]]: """Full auto-detection chain. Priority: @@ -3223,7 +3320,22 @@ def _resolve_auto(main_runtime: Optional[Dict[str, Any]] = None) -> Tuple[Option main_provider, resolved or main_model) return client, resolved or main_model - # ── Step 2: aggregator / fallback chain ────────────────────────────── + # ── Step 2: user-configured fallback policy ───────────────────────── + # In auto mode, respect the task-specific fallback chain first, then the + # main agent's top-level fallback_providers/fallback_model chain. The + # hardcoded provider discovery chain below is only the convenience default + # for users who have not declared a fallback policy. + if task: + fb_client, fb_model, _fb_label = _try_configured_fallback_chain( + task, main_provider or "auto", reason="main provider unavailable") + if fb_client is not None: + return fb_client, fb_model + fb_client, fb_model, _fb_label = _try_main_fallback_chain( + task, main_provider or "auto", reason="main provider unavailable") + if fb_client is not None: + return fb_client, fb_model + + # ── Step 3: aggregator / fallback chain ────────────────────────────── tried = [] for label, try_fn in _get_provider_chain(): if _is_provider_unhealthy(label): @@ -3344,6 +3456,7 @@ def resolve_provider_client( api_mode: str = None, main_runtime: Optional[Dict[str, Any]] = None, is_vision: bool = False, + task: Optional[str] = None, ) -> Tuple[Optional[Any], Optional[str]]: """Central router: given a provider name and optional model, return a configured client with the correct auth, base URL, and API format. @@ -3464,7 +3577,7 @@ def resolve_provider_client( # ── Auto: try all providers in priority order ──────────────────── if provider == "auto": - client, resolved = _resolve_auto(main_runtime=main_runtime) + client, resolved = _resolve_auto(main_runtime=main_runtime, task=task) if client is None: return None, None # When auto-detection lands on a non-OpenRouter provider (e.g. a @@ -4357,11 +4470,16 @@ def _client_cache_key( api_mode: Optional[str] = None, main_runtime: Optional[Dict[str, Any]] = None, is_vision: bool = False, + task: Optional[str] = None, ) -> tuple: runtime = _normalize_main_runtime(main_runtime) runtime_key = tuple(runtime.get(field, "") for field in _MAIN_RUNTIME_FIELDS) if provider == "auto" else () + # `auto` can now resolve through task-specific or main fallback policy, + # so the task participates in the cache key. Non-auto providers keep the + # old cache shape because the explicit provider/model tuple is sufficient. + task_key = (task or "") if provider == "auto" else "" pool_hint = _pool_cache_hint(provider, main_runtime=main_runtime) - return (provider, async_mode, base_url or "", api_key or "", api_mode or "", runtime_key, is_vision, pool_hint) + return (provider, async_mode, base_url or "", api_key or "", api_mode or "", runtime_key, is_vision, task_key, pool_hint) def _store_cached_client(cache_key: tuple, client: Any, default_model: Optional[str], *, bound_loop: Any = None) -> None: @@ -4554,6 +4672,7 @@ def _get_cached_client( api_mode: str = None, main_runtime: Optional[Dict[str, Any]] = None, is_vision: bool = False, + task: Optional[str] = None, ) -> Tuple[Optional[Any], Optional[str]]: """Get or create a cached client for the given provider. @@ -4591,6 +4710,7 @@ def _get_cached_client( api_mode=api_mode, main_runtime=main_runtime, is_vision=is_vision, + task=task, ) with _client_cache_lock: if cache_key in _client_cache: @@ -4635,6 +4755,7 @@ def _get_cached_client( api_mode=api_mode, main_runtime=runtime, is_vision=is_vision, + task=task, ) if client is not None: # For async clients, remember which loop they were created on so we @@ -5140,7 +5261,7 @@ def call_llm( if not resolved_base_url: logger.info("Auxiliary %s: provider %s unavailable, trying auto-detection chain", task or "call", resolved_provider) - client, final_model = _get_cached_client("auto", main_runtime=main_runtime) + client, final_model = _get_cached_client("auto", main_runtime=main_runtime, task=task) if client is None: raise RuntimeError( f"No LLM provider configured for task={task} provider={resolved_provider}. " @@ -5466,14 +5587,19 @@ def call_llm( # Fallback order (#26882, #26803): # 1. User-configured fallback_chain (per-task) if set - # 2. Main agent model (last-resort safety net) - # For auto users (no explicit aux provider), use the full - # auto-detection chain instead — its Step 1 IS the main agent - # model, so users on `auto` already get main-model fallback. + # 2. For auto: top-level main fallback_providers/fallback_model + # 3. For auto: built-in auxiliary discovery chain + # 4. For explicit aux providers: main agent model safety net fb_client, fb_model, fb_label = (None, None, "") if is_auto: - fb_client, fb_model, fb_label = _try_payment_fallback( - resolved_provider, task, reason=reason) + fb_client, fb_model, fb_label = _try_configured_fallback_chain( + task, resolved_provider or "auto", reason=reason) + if fb_client is None: + fb_client, fb_model, fb_label = _try_main_fallback_chain( + task, resolved_provider or "auto", reason=reason) + if fb_client is None: + fb_client, fb_model, fb_label = _try_payment_fallback( + resolved_provider, task, reason=reason) else: fb_client, fb_model, fb_label = _try_configured_fallback_chain( task, resolved_provider or "auto", reason=reason) @@ -5636,7 +5762,7 @@ async def async_call_llm( if not resolved_base_url: logger.info("Auxiliary %s: provider %s unavailable, trying auto-detection chain", task or "call", resolved_provider) - client, final_model = _get_cached_client("auto", async_mode=True) + client, final_model = _get_cached_client("auto", async_mode=True, main_runtime=main_runtime, task=task) if client is None: raise RuntimeError( f"No LLM provider configured for task={task} provider={resolved_provider}. " @@ -5904,13 +6030,19 @@ async def async_call_llm( # Fallback order (#26882, #26803): # 1. User-configured fallback_chain (per-task) if set - # 2. Main agent model (last-resort safety net) - # Auto users get the full auto-detection chain instead — its - # Step 1 IS the main agent model. + # 2. For auto: top-level main fallback_providers/fallback_model + # 3. For auto: built-in auxiliary discovery chain + # 4. For explicit aux providers: main agent model safety net fb_client, fb_model, fb_label = (None, None, "") if is_auto: - fb_client, fb_model, fb_label = _try_payment_fallback( - resolved_provider, task, reason=reason) + fb_client, fb_model, fb_label = _try_configured_fallback_chain( + task, resolved_provider or "auto", reason=reason) + if fb_client is None: + fb_client, fb_model, fb_label = _try_main_fallback_chain( + task, resolved_provider or "auto", reason=reason) + if fb_client is None: + fb_client, fb_model, fb_label = _try_payment_fallback( + resolved_provider, task, reason=reason) else: fb_client, fb_model, fb_label = _try_configured_fallback_chain( task, resolved_provider or "auto", reason=reason) diff --git a/tests/agent/test_auxiliary_client.py b/tests/agent/test_auxiliary_client.py index 7770b2e8c88..b2960b703c7 100644 --- a/tests/agent/test_auxiliary_client.py +++ b/tests/agent/test_auxiliary_client.py @@ -1653,6 +1653,37 @@ class TestAuxiliaryFallbackLayering: exc.status_code = 402 return exc + def test_auto_provider_uses_task_then_main_chain_before_builtin_chain(self, monkeypatch): + """Auto aux call failures try per-task then top-level fallback before built-ins.""" + primary_client = MagicMock() + primary_client.chat.completions.create.side_effect = self._make_payment_err() + + main_chain_client = MagicMock() + main_chain_client.chat.completions.create.return_value = MagicMock(choices=[ + MagicMock(message=MagicMock(content="from main fallback chain")) + ]) + + with patch("agent.auxiliary_client._get_cached_client", + return_value=(primary_client, "qwen/qwen3.5-122b-a10b")), \ + patch("agent.auxiliary_client._resolve_task_provider_model", + return_value=("auto", None, None, None, None)), \ + patch("agent.auxiliary_client._try_configured_fallback_chain", + return_value=(None, None, "")) as mock_task_chain, \ + patch("agent.auxiliary_client._try_main_fallback_chain", + return_value=(main_chain_client, "inclusionai/ring-2.6-1t:free", "openrouter")) as mock_main_chain, \ + patch("agent.auxiliary_client._try_payment_fallback") as mock_builtin_chain: + result = call_llm( + task="title_generation", + messages=[{"role": "user", "content": "hello"}], + ) + + assert main_chain_client.chat.completions.create.called + mock_task_chain.assert_called_once_with( + "title_generation", "auto", reason="payment error") + mock_main_chain.assert_called_once_with( + "title_generation", "auto", reason="payment error") + mock_builtin_chain.assert_not_called() + def test_explicit_provider_uses_configured_chain_first(self, monkeypatch, caplog): """When a user has fallback_chain configured, it's tried BEFORE the main agent model.""" monkeypatch.setenv("OPENROUTER_API_KEY", "or-key") diff --git a/tests/agent/test_auxiliary_main_first.py b/tests/agent/test_auxiliary_main_first.py index 8913aad537f..f8a681ebfa9 100644 --- a/tests/agent/test_auxiliary_main_first.py +++ b/tests/agent/test_auxiliary_main_first.py @@ -118,6 +118,64 @@ class TestResolveAutoMainFirst: assert client is chain_client assert model == "google/gemini-3-flash-preview" + def test_main_unavailable_uses_task_fallback_chain_before_builtin_chain(self): + """Auto aux resolution honors auxiliary..fallback_chain before built-ins.""" + task_client = MagicMock() + with patch( + "agent.auxiliary_client._read_main_provider", return_value="nvidia", + ), patch( + "agent.auxiliary_client._read_main_model", return_value="qwen/qwen3.5-122b-a10b", + ), patch( + "agent.auxiliary_client.resolve_provider_client", + return_value=(None, None), # main provider has no client + ), patch( + "agent.auxiliary_client._try_configured_fallback_chain", + return_value=(task_client, "task-free-model", "fallback_chain[0](openrouter)"), + ) as mock_task_chain, patch( + "agent.auxiliary_client._try_main_fallback_chain", + ) as mock_main_chain, patch( + "agent.auxiliary_client._try_openrouter", + ) as mock_openrouter: + from agent.auxiliary_client import _resolve_auto + + client, model = _resolve_auto(task="title_generation") + + assert client is task_client + assert model == "task-free-model" + mock_task_chain.assert_called_once_with( + "title_generation", "nvidia", reason="main provider unavailable") + mock_main_chain.assert_not_called() + mock_openrouter.assert_not_called() + + def test_main_unavailable_uses_main_fallback_chain_before_builtin_chain(self): + """Auto aux resolution honors top-level fallback_providers before built-ins.""" + main_fallback_client = MagicMock() + with patch( + "agent.auxiliary_client._read_main_provider", return_value="nvidia", + ), patch( + "agent.auxiliary_client._read_main_model", return_value="qwen/qwen3.5-122b-a10b", + ), patch( + "agent.auxiliary_client.resolve_provider_client", + return_value=(None, None), # main provider has no client + ), patch( + "agent.auxiliary_client._try_configured_fallback_chain", + return_value=(None, None, ""), + ), patch( + "agent.auxiliary_client._try_main_fallback_chain", + return_value=(main_fallback_client, "inclusionai/ring-2.6-1t:free", "openrouter"), + ) as mock_main_chain, patch( + "agent.auxiliary_client._try_openrouter", + ) as mock_openrouter: + from agent.auxiliary_client import _resolve_auto + + client, model = _resolve_auto(task="title_generation") + + assert client is main_fallback_client + assert model == "inclusionai/ring-2.6-1t:free" + mock_main_chain.assert_called_once_with( + "title_generation", "nvidia", reason="main provider unavailable") + mock_openrouter.assert_not_called() + def test_no_main_config_uses_chain_directly(self): """No main provider configured → skip step 1, use chain (no regression).""" chain_client = MagicMock() diff --git a/tests/run_agent/test_async_httpx_del_neuter.py b/tests/run_agent/test_async_httpx_del_neuter.py index 946d73dbdf1..090e6998269 100644 --- a/tests/run_agent/test_async_httpx_del_neuter.py +++ b/tests/run_agent/test_async_httpx_del_neuter.py @@ -176,11 +176,16 @@ class TestClientCacheBoundedGrowth: """When the loop changes, the old entry should be replaced, not duplicated.""" from agent.auxiliary_client import ( _client_cache, + _client_cache_key, _client_cache_lock, _get_cached_client, ) - key = ("test_replace", True, "", "", "", (), False, "") + key = _client_cache_key( + "test_replace", + async_mode=True, + task="", + ) # Simulate a stale entry from a closed loop old_loop = asyncio.new_event_loop() diff --git a/website/docs/reference/environment-variables.md b/website/docs/reference/environment-variables.md index 2419846a10f..76ce863e661 100644 --- a/website/docs/reference/environment-variables.md +++ b/website/docs/reference/environment-variables.md @@ -687,7 +687,7 @@ For task-specific direct endpoints, Hermes uses the task's configured API key or ## Fallback Providers (config.yaml only) -The primary model fallback chain is configured exclusively through `config.yaml` — there are no environment variables for it. Add a top-level `fallback_providers` list with `provider` and `model` keys to enable automatic failover when your main model encounters errors. +The primary model fallback chain is configured exclusively through `config.yaml` — there are no environment variables for it. Add a top-level `fallback_providers` list with `provider` and `model` keys to enable automatic failover when your main model encounters errors. Auxiliary tasks whose provider is `auto` also consult this chain before Hermes' built-in auxiliary discovery chain. ```yaml fallback_providers: @@ -695,7 +695,7 @@ fallback_providers: model: anthropic/claude-sonnet-4 ``` -The older top-level `fallback_model` single-provider shape is still read for backward compatibility, but new configuration should use `fallback_providers`. +The older top-level `fallback_model` single-provider shape is still read for backward compatibility, but new configuration should use `fallback_providers`. For task-specific auxiliary policy, use `auxiliary..fallback_chain` in `config.yaml`; there is no environment variable equivalent. See [Fallback Providers](/user-guide/features/fallback-providers) for full details. diff --git a/website/docs/user-guide/configuring-models.md b/website/docs/user-guide/configuring-models.md index 3368d5201d8..8d749e15143 100644 --- a/website/docs/user-guide/configuring-models.md +++ b/website/docs/user-guide/configuring-models.md @@ -53,7 +53,7 @@ Click **Show auxiliary** to reveal the 11 task slots: ![Auxiliary panel expanded](/img/docs/dashboard-models/auxiliary-expanded.png) -Every auxiliary task defaults to `auto` — meaning Hermes uses your main model for that job too. Override a specific task when you want a cheaper or faster model for a side-job. +Every auxiliary task defaults to `auto` — meaning Hermes tries your main model for that job too. If that route is unavailable or hits a capacity-style failure, `auto` follows any task-specific `auxiliary..fallback_chain`, then the main `fallback_providers` / `fallback_model` chain, then Hermes' built-in auxiliary discovery chain. Override a specific task when you want a cheaper or faster model for a side-job. ### Common override patterns @@ -129,7 +129,21 @@ auxiliary: # ... other fields unchanged ``` -`provider: auto` with `model: ''` tells Hermes to use the main model for that task. +`provider: auto` with `model: ''` tells Hermes to use the main model for that task, while still honoring fallback policy if the main route cannot serve the auxiliary call. + +Optional task-specific fallback chains live under the same auxiliary task: + +```yaml +auxiliary: + title_generation: + provider: auto + model: '' + fallback_chain: + - provider: openrouter + model: inclusionai/ring-2.6-1t:free +``` + +When `fallback_chain` is absent, `auto` uses the top-level `fallback_providers` chain before the built-in auxiliary discovery chain. ## When does it take effect? diff --git a/website/docs/user-guide/features/fallback-providers.md b/website/docs/user-guide/features/fallback-providers.md index 7eaab0ea8af..dbe431fc1ea 100644 --- a/website/docs/user-guide/features/fallback-providers.md +++ b/website/docs/user-guide/features/fallback-providers.md @@ -168,7 +168,7 @@ fallback_providers: | Messaging gateway (Telegram, Discord, etc.) | ✔ | | Subagent delegation | ✔ (subagents inherit the parent fallback chain) | | Cron jobs | ✔ (cron agents inherit configured fallback providers) | -| Auxiliary tasks (vision, compression) | ✘ (use their own provider chain — see below) | +| Auxiliary tasks on `provider: auto` | ✔ (try per-task fallback, then the main fallback chain before built-in aux discovery) | :::tip There are no environment variables for the primary fallback chain — configure it exclusively through `config.yaml` or `hermes fallback`. This is intentional: fallback configuration is a deliberate choice, not something a stale shell export should override. @@ -195,23 +195,30 @@ Hermes uses separate lightweight models for side tasks. Each task has its own pr ### Auto-Detection Chain -When a task's provider is set to `"auto"` (the default), Hermes tries providers in order until one works: +When a task's provider is set to `"auto"` (the default), Hermes first tries the main provider + main model for that auxiliary task. If that route is unavailable or later fails with a capacity-style error, Hermes now honors user-configured fallback policy before using the built-in discovery chain: -**For text tasks (compression, web extract, etc.):** +```text +Main provider + main model → auxiliary..fallback_chain → +fallback_providers / fallback_model → built-in auxiliary discovery chain +``` + +The task-specific chain is most precise and wins when present. The top-level `fallback_providers` chain is the same policy the main agent uses, so free-only or same-provider fallback rules apply to auxiliary tasks on `auto` as well. + +**Built-in text discovery chain (compression, web extract, title generation, etc.):** ```text OpenRouter → Nous Portal → Custom endpoint → Codex OAuth → API-key providers (z.ai, Kimi, MiniMax, Xiaomi MiMo, Hugging Face, Anthropic) → give up ``` -**For vision tasks:** +**Built-in vision discovery chain:** ```text Main provider (if vision-capable) → OpenRouter → Nous Portal → Codex OAuth → Anthropic → Custom endpoint → give up ``` -If the resolved provider fails at call time, Hermes also has an internal retry: if the provider is not OpenRouter and no explicit `base_url` is set, it tries OpenRouter as a last-resort fallback. +Those built-in chains are a convenience fallback for users who have not declared a task-specific or main fallback policy. ### Configuring Auxiliary Providers @@ -232,6 +239,9 @@ auxiliary: compression: provider: "auto" model: "" + fallback_chain: # optional, task-specific fallback policy + - provider: openrouter + model: inclusionai/ring-2.6-1t:free skills_hub: provider: "auto" @@ -242,7 +252,9 @@ auxiliary: model: "" ``` -Every task above follows the same **provider / model / base_url** pattern. Context compression is configured under `auxiliary.compression`: +Every task above follows the same **provider / model / base_url** pattern. Each task can also declare its own `fallback_chain`; if omitted, `provider: auto` uses the top-level `fallback_providers` chain before Hermes' built-in auxiliary discovery chain. + +Context compression is configured under `auxiliary.compression`: ```yaml auxiliary: From e65d74bc6f9a6b0e3dc7586b217aa8b57372c125 Mon Sep 17 00:00:00 2001 From: Rory Evans Date: Sun, 31 May 2026 17:27:05 +0200 Subject: [PATCH 2/5] fix(gateway): accept `metadata` kwarg in WhatsApp/email send_image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BasePlatformAdapter.send_multiple_images` passes `metadata=metadata` to `send_image` / `send_image_file` / `send_animation` on every send. The WhatsApp and email `send_image` overrides stopped their signature at `reply_to`, so any image delivered as a URL (the common case — image-gen backends return URLs) raised: TypeError: send_image() got an unexpected keyword argument "metadata" and the image silently failed to send. Their sibling overrides (`send_image_file` / `send_video` / `send_voice` / `send_document`) already absorb it via **kwargs, which is why only plain image-URL sends broke. - whatsapp/email `send_image`: accept `metadata` (matches the base signature); WhatsApp forwards it to the super() text fallback. - Add `tests/gateway/test_media_metadata_contract.py`: asserts WhatsApp + email accept it, plus a best-effort sweep over every adapter so the next slip fails at test time instead of in production. Co-Authored-By: Claude Opus 4.8 (1M context) --- gateway/platforms/email.py | 7 +- gateway/platforms/whatsapp.py | 20 ++++- tests/gateway/test_media_metadata_contract.py | 80 +++++++++++++++++++ 3 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 tests/gateway/test_media_metadata_contract.py diff --git a/gateway/platforms/email.py b/gateway/platforms/email.py index 7b247cdda27..d2f7e64ac61 100644 --- a/gateway/platforms/email.py +++ b/gateway/platforms/email.py @@ -678,8 +678,13 @@ class EmailAdapter(BasePlatformAdapter): image_url: str, caption: Optional[str] = None, reply_to: Optional[str] = None, + metadata: Optional[Dict[str, Any]] = None, ) -> SendResult: - """Send an image URL as part of an email body.""" + """Send an image URL as part of an email body. + + ``metadata`` is accepted to honor the base-class contract; the + email body send doesn't use it. + """ text = caption or "" text += f"\n\nImage: {image_url}" return await self.send(chat_id, text.strip(), reply_to) diff --git a/gateway/platforms/whatsapp.py b/gateway/platforms/whatsapp.py index d833d5649a2..00ff2c967e7 100644 --- a/gateway/platforms/whatsapp.py +++ b/gateway/platforms/whatsapp.py @@ -846,13 +846,20 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): image_url: str, caption: Optional[str] = None, reply_to: Optional[str] = None, + metadata: Optional[Dict[str, Any]] = None, ) -> SendResult: - """Download image URL to cache, send natively via bridge.""" + """Download image URL to cache, send natively via bridge. + + ``metadata`` is accepted to honor the base-class contract — the + batch sender ``send_multiple_images`` passes it through to every + send path. The bridge media call doesn't use it, matching the + sibling overrides (send_video / send_voice / send_document). + """ try: local_path = await cache_image_from_url(image_url) return await self._send_media_to_bridge(chat_id, local_path, "image", caption) except Exception: - return await super().send_image(chat_id, image_url, caption, reply_to) + return await super().send_image(chat_id, image_url, caption, reply_to, metadata) async def send_image_file( self, @@ -1136,6 +1143,15 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): body = data.get("body", "") if data.get("isGroup"): body = self._clean_bot_mention_text(body, data) + + # If this is a reply, include the quoted message text so the agent + # knows exactly what the user is responding to (fixes "approve" context issue) + quoted_text = str(data.get("quotedText") or "").strip() + if quoted_text and data.get("hasQuotedMessage"): + # Truncate long quoted text to keep prompts reasonable + if len(quoted_text) > 300: + quoted_text = quoted_text[:297] + "..." + body = f"[Replying to: \"{quoted_text}\"]\n{body}" MAX_TEXT_INJECT_BYTES = 100 * 1024 if msg_type == MessageType.DOCUMENT and cached_urls: for doc_path in cached_urls: diff --git a/tests/gateway/test_media_metadata_contract.py b/tests/gateway/test_media_metadata_contract.py new file mode 100644 index 00000000000..7f423e77342 --- /dev/null +++ b/tests/gateway/test_media_metadata_contract.py @@ -0,0 +1,80 @@ +"""Contract: media-send overrides must accept the ``metadata`` kwarg. + +``BasePlatformAdapter.send_multiple_images`` passes ``metadata=metadata`` +to ``send_image`` / ``send_image_file`` / ``send_animation`` on every send. +An override whose signature stops at ``reply_to`` raises ``TypeError: +send_image() got an unexpected keyword argument 'metadata'`` at runtime — +which is exactly how image delivery broke on WhatsApp and email. + +This mirrors ``test_discord_media_metadata.py`` but covers the two +adapters that previously slipped, plus a best-effort sweep over every +adapter that imports cleanly so the next slip is caught at test time. +""" + +from __future__ import annotations + +import importlib +import inspect + +import pytest + + +def _accepts_metadata(method) -> bool: + params = inspect.signature(method).parameters + if "metadata" in params: + return True + # A ``**kwargs`` catch-all also absorbs metadata (the convention used by + # WhatsApp's send_video / send_voice / send_document overrides). + return any(p.kind is inspect.Parameter.VAR_KEYWORD for p in params.values()) + + +# (module, class) for the two adapters this fix targeted. These must import +# in CI, so assert directly rather than skipping. +@pytest.mark.parametrize( + "module_name, class_name", + [ + ("gateway.platforms.whatsapp", "WhatsAppAdapter"), + ("gateway.platforms.email", "EmailAdapter"), + ], +) +def test_send_image_accepts_metadata(module_name, class_name): + cls = getattr(importlib.import_module(module_name), class_name) + assert _accepts_metadata(cls.send_image), ( + f"{class_name}.send_image must accept 'metadata' (or **kwargs) — " + f"send_multiple_images passes it on every send" + ) + + +# Best-effort sweep across all shipped adapters. Modules whose optional +# platform SDK isn't installed are skipped; an adapter that imports but +# whose override drops metadata is a hard failure. +_ALL_ADAPTERS = [ + ("gateway.platforms.bluebubbles", "BlueBubblesAdapter"), + ("gateway.platforms.dingtalk", "DingTalkAdapter"), + ("gateway.platforms.discord", "DiscordAdapter"), + ("gateway.platforms.email", "EmailAdapter"), + ("gateway.platforms.feishu", "FeishuAdapter"), + ("gateway.platforms.matrix", "MatrixAdapter"), + ("gateway.platforms.mattermost", "MattermostAdapter"), + ("gateway.platforms.signal", "SignalAdapter"), + ("gateway.platforms.slack", "SlackAdapter"), + ("gateway.platforms.telegram", "TelegramAdapter"), + ("gateway.platforms.wecom", "WeComAdapter"), + ("gateway.platforms.weixin", "WeixinAdapter"), + ("gateway.platforms.whatsapp", "WhatsAppAdapter"), + ("gateway.platforms.yuanbao", "YuanbaoAdapter"), +] + + +@pytest.mark.parametrize("module_name, class_name", _ALL_ADAPTERS) +def test_all_adapters_send_image_metadata_sweep(module_name, class_name): + try: + module = importlib.import_module(module_name) + except Exception as exc: # optional platform dep not installed + pytest.skip(f"{module_name} not importable: {exc}") + cls = getattr(module, class_name, None) + if cls is None or "send_image" not in cls.__dict__: + pytest.skip(f"{class_name} has no send_image override") + assert _accepts_metadata(cls.send_image), ( + f"{class_name}.send_image drops the 'metadata' kwarg" + ) From 925b0d1ab52da552e5da783e7c8543f86af127fc Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 16 Jun 2026 05:57:24 -0700 Subject: [PATCH 3/5] chore: add zimigit2020 to release AUTHOR_MAP --- scripts/release.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/release.py b/scripts/release.py index 3f2823f03d2..25247cff92a 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -90,6 +90,7 @@ AUTHOR_MAP = { "290859878+synapsesx@users.noreply.github.com": "synapsesx", "157689911+itsflownium@users.noreply.github.com": "itsflownium", "dirtyren@users.noreply.github.com": "dirtyren", + "evansrory@gmail.com": "zimigit2020", "237263164+ft-ioxcs@users.noreply.github.com": "ft-ioxcs", "tharushkadinujaya05@gmail.com": "0xneobyte", "138671361+Veritas-7@users.noreply.github.com": "Veritas-7", From 16fc7170911470f8bc01c4e87737dc309bef0deb Mon Sep 17 00:00:00 2001 From: Wolfram Ravenwolf Date: Mon, 1 Jun 2026 04:09:59 +0200 Subject: [PATCH 4/5] fix(mattermost): harden delivery hygiene PROBLEM: Mattermost threads can become invalid or enormous, exposing two failure modes: internal scratch/reasoning/commentary displays could leak into persistent Mattermost threads via global display toggles, while rejected threaded user-visible replies could disappear unless every failed send fell back flat. A broad flat fallback would pollute channels with tool/status/progress noise. SOLUTION: Require explicit Mattermost platform opt-in for scratch displays, keep using the existing notify=True metadata marker for user-visible final text/media/file replies, and allow the Mattermost plugin adapter to flat-fallback only notify-worthy sends whose threaded POST failure looks like a broken root/thread. Keep tool/status/progress and other non-notify sends thread-strict. Add regression tests for display opt-in, notify-only broken-thread fallback, generic API failure suppression, and stream notify metadata. Verification: tests/gateway/test_mattermost.py tests/gateway/test_stream_consumer.py tests/gateway/test_stream_consumer_thread_routing.py tests/gateway/test_stream_consumer_fresh_final.py tests/gateway/test_stream_consumer_draft.py; tests/gateway/test_session_api.py tests/gateway/test_status_command.py tests/gateway/test_resume_command.py tests/hermes_cli/test_commands.py; py_compile touched gateway files; git diff --check. Session: Mattermost thread 6qg8e9dd1pd9pkhi74xyaa1mry, 2026-06-01. --- gateway/platforms/base.py | 49 +++---- gateway/run.py | 122 ++++++++++++++++-- gateway/stream_consumer.py | 53 ++++++-- tests/gateway/test_mattermost.py | 114 +++++++++++++++- .../test_stream_consumer_thread_routing.py | 36 ++++++ .../gateway/test_telegram_overflow_partial.py | 2 +- tests/gateway/test_tts_media_routing.py | 6 +- 7 files changed, 329 insertions(+), 53 deletions(-) diff --git a/gateway/platforms/base.py b/gateway/platforms/base.py index 205d9cbf509..cda3acc6e58 100644 --- a/gateway/platforms/base.py +++ b/gateway/platforms/base.py @@ -77,6 +77,13 @@ def _thread_metadata_for_source(source, reply_to_message_id: str | None = None) return metadata +def _mark_notify_metadata(metadata: dict | None) -> dict: + """Clone metadata and mark a user-visible reply as notify-worthy.""" + notify_metadata = dict(metadata) if metadata else {} + notify_metadata["notify"] = True + return notify_metadata + + def _reply_anchor_for_event(event) -> str | None: """Return reply_to id for platforms that need reply semantics. @@ -3889,7 +3896,7 @@ class BasePlatformAdapter(ABC): chat_id=event.source.chat_id, content=_text, reply_to=_reply_anchor_for_event(event), - metadata=thread_meta, + metadata=_mark_notify_metadata(thread_meta), ) if _eph_ttl > 0 and _r.success and _r.message_id: self._schedule_ephemeral_delete( @@ -3995,7 +4002,7 @@ class BasePlatformAdapter(ABC): chat_id=event.source.chat_id, content=_text, reply_to=_reply_anchor_for_event(event), - metadata=_thread_meta, + metadata=_mark_notify_metadata(_thread_meta), ) if _eph_ttl > 0 and _r.success and _r.message_id: self._schedule_ephemeral_delete( @@ -4045,7 +4052,7 @@ class BasePlatformAdapter(ABC): chat_id=event.source.chat_id, content=_text, reply_to=_reply_anchor_for_event(event), - metadata=_thread_meta, + metadata=_mark_notify_metadata(_thread_meta), ) if _eph_ttl > 0 and _r.success and _r.message_id: self._schedule_ephemeral_delete( @@ -4268,6 +4275,12 @@ class BasePlatformAdapter(ABC): ) text_content = _recovered + # Final user-visible content (text, TTS, media, files) gets + # the existing notify=True marker. Clone once so typing/status + # metadata stays unmarked and progress bubbles remain + # thread-strict. + _final_thread_metadata = _mark_notify_metadata(_thread_metadata) + # Auto-TTS: if voice message, generate audio FIRST (before sending text) # Gated via ``_should_auto_tts_for_chat``: fires when the chat has # an explicit ``/voice on|tts`` opt-in OR when ``voice.auto_tts`` is @@ -4307,7 +4320,7 @@ class BasePlatformAdapter(ABC): chat_id=event.source.chat_id, audio_path=_tts_path, caption=telegram_tts_caption, - metadata=_thread_metadata, + metadata=_final_thread_metadata, ) _tts_caption_delivered = bool( telegram_tts_caption and getattr(tts_result, "success", False) @@ -4322,23 +4335,11 @@ class BasePlatformAdapter(ABC): if text_content and not _tts_caption_delivered: logger.info("[%s] Sending response (%d chars) to %s", self.name, len(text_content), event.source.chat_id) _reply_anchor = _reply_anchor_for_event(event) - # Mark final response messages for notification delivery. - # Platform adapters that support per-message notification - # control (e.g. Telegram's disable_notification) use this - # flag to override silent-mode and ensure the final - # response triggers a push notification. - # Clone to avoid mutating the metadata shared with the - # typing-indicator task (which must remain unmarked). - if _thread_metadata is not None: - _thread_metadata = dict(_thread_metadata) - _thread_metadata["notify"] = True - else: - _thread_metadata = {"notify": True} result = await self._send_with_retry( chat_id=event.source.chat_id, content=text_content, reply_to=_reply_anchor, - metadata=_thread_metadata, + metadata=_final_thread_metadata, ) _record_delivery(result) @@ -4367,7 +4368,7 @@ class BasePlatformAdapter(ABC): await self.send_multiple_images( chat_id=event.source.chat_id, images=images, - metadata=_thread_metadata, + metadata=_final_thread_metadata, human_delay=human_delay, ) except Exception as batch_err: @@ -4409,7 +4410,7 @@ class BasePlatformAdapter(ABC): await self.send_multiple_images( chat_id=event.source.chat_id, images=_batch, - metadata=_thread_metadata, + metadata=_final_thread_metadata, human_delay=human_delay, ) except Exception as batch_err: @@ -4424,19 +4425,19 @@ class BasePlatformAdapter(ABC): media_result = await self.send_voice( chat_id=event.source.chat_id, audio_path=media_path, - metadata=_thread_metadata, + metadata=_final_thread_metadata, ) elif ext in _VIDEO_EXTS: media_result = await self.send_video( chat_id=event.source.chat_id, video_path=media_path, - metadata=_thread_metadata, + metadata=_final_thread_metadata, ) else: media_result = await self.send_document( chat_id=event.source.chat_id, file_path=media_path, - metadata=_thread_metadata, + metadata=_final_thread_metadata, ) if not media_result.success: @@ -4454,13 +4455,13 @@ class BasePlatformAdapter(ABC): await self.send_video( chat_id=event.source.chat_id, video_path=file_path, - metadata=_thread_metadata, + metadata=_final_thread_metadata, ) else: await self.send_document( chat_id=event.source.chat_id, file_path=file_path, - metadata=_thread_metadata, + metadata=_final_thread_metadata, ) except Exception as file_err: logger.error("[%s] Error sending local file %s: %s", self.name, file_path, file_err) diff --git a/gateway/run.py b/gateway/run.py index 470d71906c3..b688f3a3613 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -413,6 +413,57 @@ def _resolve_progress_thread_id(platform: Any, source_thread_id: Any, event_mess return None +def _has_platform_display_override(user_config: dict, platform_key: str, setting: str) -> bool: + """Return True when display.platforms. explicitly sets setting.""" + display = user_config.get("display") if isinstance(user_config, dict) else None + if not isinstance(display, dict): + return False + platforms = display.get("platforms") + if not isinstance(platforms, dict): + return False + platform_cfg = platforms.get(platform_key) + return isinstance(platform_cfg, dict) and setting in platform_cfg + + +def _resolve_gateway_display_bool( + user_config: dict, + platform_key: str, + setting: str, + *, + default: bool = False, + platform: Any = None, + require_platform_override_for: set[Any] | None = None, +) -> bool: + """Resolve a boolean display setting with optional platform-only opt-in. + + Some display features expose assistant scratch text rather than deliberate + user-facing output. For high-noise threaded chat surfaces such as + Mattermost, a global opt-in is too broad: they must be enabled with an + explicit display.platforms.. override. + """ + current_platform = _gateway_platform_value(platform or platform_key) + platform_only = { + _gateway_platform_value(candidate) + for candidate in (require_platform_override_for or set()) + } + if ( + current_platform in platform_only + and not _has_platform_display_override(user_config, platform_key, setting) + ): + return False + + from gateway.display_config import resolve_display_setting + + value = resolve_display_setting(user_config, platform_key, setting, default) + if isinstance(value, bool): + return value + if isinstance(value, str): + return value.strip().lower() in {"true", "yes", "1", "on"} + if value is None: + return bool(default) + return bool(value) + + def _telegramize_command_mentions(text: str, platform: Any) -> str: """Rewrite slash-command mentions to Telegram-valid command names. @@ -8989,17 +9040,24 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew source, session_entry, reason="agent-result-compression", ) - # Prepend reasoning/thinking if display is enabled (per-platform) + # Prepend reasoning/thinking if display is enabled (per-platform). + # Mattermost requires explicit per-platform opt-in because this is + # scratch text, not ordinary final-answer content. try: - from gateway.display_config import resolve_display_setting as _rds - _show_reasoning_effective = _rds( + _show_reasoning_effective = _resolve_gateway_display_bool( _load_gateway_config(), _platform_config_key(source.platform), "show_reasoning", - getattr(self, "_show_reasoning", False), + default=bool(getattr(self, "_show_reasoning", False)), + platform=source.platform, + require_platform_override_for={Platform.MATTERMOST}, ) except Exception: - _show_reasoning_effective = getattr(self, "_show_reasoning", False) + _show_reasoning_effective = ( + False + if source.platform == Platform.MATTERMOST + else getattr(self, "_show_reasoning", False) + ) if _show_reasoning_effective and response and not _intentional_silence: last_reasoning = agent_result.get("last_reasoning") if last_reasoning: @@ -13635,18 +13693,32 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew # in chat platforms while opting into concise mid-turn updates. interim_assistant_messages_enabled = ( source.platform != Platform.WEBHOOK - and bool( - resolve_display_setting( - user_config, - platform_key, - "interim_assistant_messages", - True, - ) + and _resolve_gateway_display_bool( + user_config, + platform_key, + "interim_assistant_messages", + default=True, + platform=source.platform, + require_platform_override_for={Platform.MATTERMOST}, ) ) - + # thinking_progress is independent — if enabled, we need the progress + # queue even when tool_progress is off (thinking relay uses same infra). + # Mattermost requires a per-platform opt-in: global scratch-text display + # is too easy to leak into busy public threads. + _thinking_enabled = _resolve_gateway_display_bool( + user_config, + platform_key, + "thinking_progress", + default=False, + platform=source.platform, + require_platform_override_for={Platform.MATTERMOST}, + ) + needs_progress_queue = tool_progress_enabled or _thinking_enabled + + # Queue for progress messages (thread-safe) - progress_queue = queue.Queue() if tool_progress_enabled else None + progress_queue = queue.Queue() if needs_progress_queue else None last_tool = [None] # Mutable container for tracking in closure last_progress_msg = [None] # Track last message for dedup repeat_count = [0] # How many times the same message repeated @@ -13752,6 +13824,24 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew logger.debug("tool-progress onboarding hint failed: %s", _hint_err) return + # "_thinking" is assistant scratch text between tool calls. It + # is never ordinary tool progress: only relay it when the platform + # explicitly opted into thinking_progress. Handle both legacy + # callback shapes: ("_thinking", text) and + # ("reasoning.available", "_thinking", text, ...). + if event_type == "_thinking" or tool_name == "_thinking": + if not _thinking_enabled: + return + thinking_text = preview if tool_name == "_thinking" else tool_name + msg = f"💬 {thinking_text}" if thinking_text else None + if msg: + progress_queue.put(msg) + return + + # If tool_progress is off, only _thinking passes through (above). + # Regular tool calls are suppressed. + if not tool_progress_enabled: + return # Only act on tool.started events (ignore tool.completed, reasoning.available, etc.) if event_type not in {"tool.started",}: @@ -14783,6 +14873,10 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew agent.clarify_callback = _clarify_callback_sync + # Show assistant thinking between tool calls — independent of + # tool_progress mode. Mattermost needs an explicit per-platform + # opt-in so global scratch-text display does not leak into threads. + agent.thinking_progress = _thinking_enabled # Store agent reference for interrupt support agent_holder[0] = agent # Capture the full tool definitions for transcript logging diff --git a/gateway/stream_consumer.py b/gateway/stream_consumer.py index 7fc9846c49f..f559d7ecd43 100644 --- a/gateway/stream_consumer.py +++ b/gateway/stream_consumer.py @@ -197,6 +197,30 @@ class GatewayStreamConsumer: # this response and route through edit-based for graceful degradation. self._draft_failures = 0 + def _metadata_for_send( + self, + *, + final: bool = False, + expect_edits: bool = False, + ) -> dict | None: + """Return per-send metadata for stream-created messages. + + Mattermost treats notify-worthy sends as user-visible final content + when deciding whether a broken thread root may fall back flat. Preview + and progress sends keep their original metadata and remain thread-strict. + + ``expect_edits`` preserves the upstream Telegram streaming contract: + preview messages that may be edited later must stay on the editable + legacy send path, while fresh/fallback final sends can still use richer + final-message delivery. + """ + meta = dict(self.metadata) if self.metadata else {} + if expect_edits: + meta["expect_edits"] = True + if final: + meta["notify"] = True + return meta or None + @property def already_sent(self) -> bool: """True if at least one message was sent or edited during the run.""" @@ -513,7 +537,11 @@ class GatewayStreamConsumer: chunks_delivered = False reply_to = self._message_id or self._initial_reply_to_id for chunk in chunks: - new_id = await self._send_new_chunk(chunk, reply_to) + new_id = await self._send_new_chunk( + chunk, + reply_to, + final=got_done, + ) if new_id is not None and new_id != reply_to: chunks_delivered = True self._accumulated = "" @@ -749,7 +777,13 @@ class GatewayStreamConsumer: # Strip trailing whitespace/newlines but preserve leading content return cleaned.rstrip() - async def _send_new_chunk(self, text: str, reply_to_id: Optional[str]) -> Optional[str]: + async def _send_new_chunk( + self, + text: str, + reply_to_id: Optional[str], + *, + final: bool = False, + ) -> Optional[str]: """Send a new message chunk, optionally threaded to a previous message. Returns the message_id so callers can thread subsequent chunks. @@ -758,15 +792,11 @@ class GatewayStreamConsumer: if not text.strip(): return reply_to_id try: - meta = dict(self.metadata) if self.metadata else {} - # This chunk becomes the next edit target — adapters that support - # rich final sends (Telegram) must keep it on the editable path. - meta["expect_edits"] = True result = await self.adapter.send( chat_id=self.chat_id, content=text, reply_to=reply_to_id, - metadata=meta, + metadata=self._metadata_for_send(final=final, expect_edits=True), ) if result.success and result.message_id: self._message_id = str(result.message_id) @@ -885,7 +915,7 @@ class GatewayStreamConsumer: result = await self.adapter.send( chat_id=self.chat_id, content=chunk, - metadata=self.metadata, + metadata=self._metadata_for_send(final=True), ) if result.success: break @@ -1242,7 +1272,7 @@ class GatewayStreamConsumer: result = await self.adapter.send( chat_id=self.chat_id, content=text, - metadata=self.metadata, + metadata=self._metadata_for_send(final=True), ) except Exception as e: logger.debug("Fresh-final send failed, falling back to edit: %s", e) @@ -1532,7 +1562,10 @@ class GatewayStreamConsumer: chat_id=self.chat_id, content=text, reply_to=self._initial_reply_to_id, - metadata={**(self.metadata or {}), "expect_edits": True}, + metadata=self._metadata_for_send( + final=finalize, + expect_edits=True, + ), ) if result.success: if result.message_id: diff --git a/tests/gateway/test_mattermost.py b/tests/gateway/test_mattermost.py index 9b174a5137a..1fedb30a019 100644 --- a/tests/gateway/test_mattermost.py +++ b/tests/gateway/test_mattermost.py @@ -6,7 +6,10 @@ import pytest from unittest.mock import MagicMock, patch, AsyncMock from gateway.config import Platform, PlatformConfig -from gateway.run import _resolve_progress_thread_id +from gateway.run import ( + _resolve_gateway_display_bool, + _resolve_progress_thread_id, +) class TestMattermostProgressThreadRouting: @@ -32,6 +35,97 @@ class TestMattermostProgressThreadRouting: ) is None +class TestMattermostDisplayHygiene: + def test_mattermost_requires_platform_opt_in_for_interim_assistant_messages(self): + """Global interim commentary must not make Mattermost leak scratch notes.""" + user_config = {"display": {"interim_assistant_messages": True}} + + assert _resolve_gateway_display_bool( + user_config, + "mattermost", + "interim_assistant_messages", + default=True, + platform=Platform.MATTERMOST, + require_platform_override_for={Platform.MATTERMOST}, + ) is False + + def test_mattermost_platform_opt_in_can_enable_interim_assistant_messages(self): + """Mattermost can still opt into commentary explicitly per platform.""" + user_config = { + "display": { + "interim_assistant_messages": False, + "platforms": { + "mattermost": {"interim_assistant_messages": True}, + }, + } + } + + assert _resolve_gateway_display_bool( + user_config, + "mattermost", + "interim_assistant_messages", + default=True, + platform=Platform.MATTERMOST, + require_platform_override_for={Platform.MATTERMOST}, + ) is True + + def test_mattermost_requires_platform_opt_in_for_thinking_progress(self): + """Global thinking_progress must not surface internal analysis in Mattermost.""" + user_config = {"display": {"thinking_progress": True}} + + assert _resolve_gateway_display_bool( + user_config, + "mattermost", + "thinking_progress", + default=False, + platform=Platform.MATTERMOST, + require_platform_override_for={Platform.MATTERMOST}, + ) is False + + def test_mattermost_requires_platform_opt_in_for_show_reasoning(self): + """Global show_reasoning must not prepend scratch reasoning in Mattermost.""" + user_config = {"display": {"show_reasoning": True}} + + assert _resolve_gateway_display_bool( + user_config, + "mattermost", + "show_reasoning", + default=False, + platform=Platform.MATTERMOST, + require_platform_override_for={Platform.MATTERMOST}, + ) is False + + def test_mattermost_platform_opt_in_can_enable_show_reasoning(self): + user_config = { + "display": { + "show_reasoning": False, + "platforms": {"mattermost": {"show_reasoning": True}}, + } + } + + assert _resolve_gateway_display_bool( + user_config, + "mattermost", + "show_reasoning", + default=False, + platform=Platform.MATTERMOST, + require_platform_override_for={Platform.MATTERMOST}, + ) is True + + def test_global_thinking_progress_still_applies_to_other_platforms(self): + """The Mattermost guard must not silently neuter Telegram/other chats.""" + user_config = {"display": {"thinking_progress": True}} + + assert _resolve_gateway_display_bool( + user_config, + "telegram", + "thinking_progress", + default=False, + platform=Platform.TELEGRAM, + require_platform_override_for={Platform.MATTERMOST}, + ) is True + + # --------------------------------------------------------------------------- # Platform & Config # --------------------------------------------------------------------------- @@ -347,6 +441,24 @@ class TestMattermostSend: payload = self.adapter._api_post.call_args_list[0][0][1] assert payload["root_id"] == "root_post" + @pytest.mark.asyncio + async def test_progress_send_with_invalid_thread_root_never_falls_back_flat(self): + """Tool/status/progress bubbles must stay quiet when the thread is broken.""" + self.adapter._reply_mode = "thread" + self.adapter._api_get = AsyncMock(return_value={"id": "bad_root", "root_id": ""}) + self.adapter._api_post = AsyncMock(return_value={}) + + result = await self.adapter.send( + "channel_1", + "⚙️ terminal...", + metadata={"thread_id": "bad_root"}, + ) + + assert result.success is False + assert self.adapter._api_post.call_count == 1 + payload = self.adapter._api_post.call_args_list[0][0][1] + assert payload["root_id"] == "bad_root" + @pytest.mark.asyncio async def test_send_api_failure(self): """When API returns error, send should return failure.""" diff --git a/tests/gateway/test_stream_consumer_thread_routing.py b/tests/gateway/test_stream_consumer_thread_routing.py index b2b7f22ffe5..3c84aef4fa8 100644 --- a/tests/gateway/test_stream_consumer_thread_routing.py +++ b/tests/gateway/test_stream_consumer_thread_routing.py @@ -106,6 +106,42 @@ class TestInitialReplyToId: assert call_kwargs["metadata"] == {**metadata, "expect_edits": True} assert metadata == {"thread_id": "omt_topic789"} + @pytest.mark.asyncio + async def test_final_first_send_marks_metadata_notify_true(self): + """Final streaming sends should use the existing notify=True marker.""" + adapter = _make_adapter() + consumer = GatewayStreamConsumer( + adapter, + "chat_123", + metadata={"thread_id": "root_post_123"}, + initial_reply_to_id="reply_post_456", + ) + + await consumer._send_or_edit("Final answer", finalize=True) + + call_kwargs = adapter.send.call_args[1] + metadata = call_kwargs["metadata"] + assert metadata["thread_id"] == "root_post_123" + assert metadata["notify"] is True + assert "delivery_kind" not in metadata + assert "allow_flat_fallback" not in metadata + + @pytest.mark.asyncio + async def test_nonfinal_first_send_does_not_mark_notify(self): + """Preview/interim streaming sends must not be notify-worthy.""" + adapter = _make_adapter() + consumer = GatewayStreamConsumer( + adapter, + "chat_123", + metadata={"thread_id": "root_post_123"}, + initial_reply_to_id="reply_post_456", + ) + + await consumer._send_or_edit("Preview", finalize=False) + + metadata = adapter.send.call_args[1]["metadata"] + assert metadata == {"thread_id": "root_post_123", "expect_edits": True} + class TestOverflowFirstMessage: """Verify thread routing is preserved when the first message overflows.""" diff --git a/tests/gateway/test_telegram_overflow_partial.py b/tests/gateway/test_telegram_overflow_partial.py index 76e4d16a617..38b10299dc3 100644 --- a/tests/gateway/test_telegram_overflow_partial.py +++ b/tests/gateway/test_telegram_overflow_partial.py @@ -134,7 +134,7 @@ async def test_stream_consumer_fallback_sends_tail_after_partial_overflow(): adapter.send.assert_awaited_once() assert adapter.send.await_args.kwargs["content"] == "world" - assert adapter.send.await_args.kwargs["metadata"] == {"thread_id": "77"} + assert adapter.send.await_args.kwargs["metadata"] == {"thread_id": "77", "notify": True} adapter.delete_message.assert_not_awaited() assert consumer.final_response_sent is True assert consumer.final_content_delivered is True diff --git a/tests/gateway/test_tts_media_routing.py b/tests/gateway/test_tts_media_routing.py index eaf9c592808..016be97ea27 100644 --- a/tests/gateway/test_tts_media_routing.py +++ b/tests/gateway/test_tts_media_routing.py @@ -76,7 +76,7 @@ async def test_base_adapter_routes_telegram_flac_media_tag_to_document_sender(tm adapter.send_document.assert_awaited_once_with( chat_id="chat-1", file_path=str(media_file), - metadata=None, + metadata={"notify": True}, ) adapter.send_voice.assert_not_awaited() @@ -95,7 +95,7 @@ async def test_base_adapter_routes_non_voice_telegram_ogg_media_tag_to_document_ adapter.send_document.assert_awaited_once_with( chat_id="chat-1", file_path=str(media_file), - metadata=None, + metadata={"notify": True}, ) adapter.send_voice.assert_not_awaited() @@ -116,7 +116,7 @@ async def test_base_adapter_routes_voice_tagged_telegram_ogg_media_tag_to_voice_ adapter.send_voice.assert_awaited_once_with( chat_id="chat-1", audio_path=str(media_file), - metadata=None, + metadata={"notify": True}, ) adapter.send_document.assert_not_awaited() From a68ac0c49af1e7a2c0098d4b592072f36199eb9c Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 16 Jun 2026 07:03:43 -0700 Subject: [PATCH 5/5] feat(desktop): allow /browser connect on a local gateway (#47245) * fix(skills): guard recursive skill delete against tree-escape Port from Kilo-Org/kilocode#11240. Their issue #11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O. * feat(desktop): allow /browser connect on a local gateway /browser was hardcoded as terminal-only in the desktop slash palette, so the chat GUI rejected it with "only available in the terminal interface." The TUI already drives the live CDP connection via the browser.manage RPC. Wire the same RPC into the desktop dispatcher as a /browser action handler, gated to local-gateway connections ($connection.mode !== 'remote'). connect mutates BROWSER_CDP_URL (and may launch Chrome) in the gateway process, so it's only meaningful when that process runs on this machine; a remote gateway gets a clear "local gateway only" message instead. --- .../app/session/hooks/use-prompt-actions.ts | 76 +++++++++++++++++++ apps/desktop/src/app/types.ts | 6 ++ .../src/lib/desktop-slash-commands.test.ts | 11 +++ .../desktop/src/lib/desktop-slash-commands.ts | 9 ++- 4 files changed, 101 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions.ts b/apps/desktop/src/app/session/hooks/use-prompt-actions.ts index 4c1b50b83ad..829119f65b4 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions.ts +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions.ts @@ -58,6 +58,7 @@ import { clearSessionTodos } from '@/store/todos' import type { ClientSessionState, + BrowserManageResponse, FileAttachResponse, HandoffFailResponse, HandoffRequestResponse, @@ -1141,6 +1142,81 @@ export function usePromptActions({ } catch (err) { renderSlashOutput(`error: ${err instanceof Error ? err.message : String(err)}`) } + }, + // /browser connect|disconnect|status manages the live CDP connection on + // the gateway host, mirroring the TUI's browser.manage RPC. It mutates + // BROWSER_CDP_URL (and may launch Chrome) in the gateway process — only + // meaningful when that process runs on this machine, so it's gated to + // local connections. A remote gateway would act on the wrong host. + browser: async ctx => { + const resolved = await withSlashOutput(ctx) + + if (!resolved) { + return + } + + const { render: renderSlashOutput, sessionId } = resolved + + if ($connection.get()?.mode === 'remote') { + renderSlashOutput( + '/browser manages a Chromium-family browser on the gateway host — only available when connected to a local gateway.' + ) + + return + } + + const [rawAction = 'status', ...rest] = ctx.arg.trim().split(/\s+/).filter(Boolean) + const cmdAction = rawAction.toLowerCase() + + if (!['connect', 'disconnect', 'status'].includes(cmdAction)) { + renderSlashOutput( + 'usage: /browser [connect|disconnect|status] [url] · persistent: set browser.cdp_url in config.yaml' + ) + + return + } + + const url = cmdAction === 'connect' ? rest.join(' ').trim() || 'http://127.0.0.1:9222' : undefined + + if (url) { + renderSlashOutput(`checking Chromium-family browser remote debugging at ${url}...`) + } + + try { + const result = await requestGateway('browser.manage', { + action: cmdAction, + session_id: sessionId, + ...(url && { url }) + }) + + // Without a streamed session subscription, the gateway bundles its + // progress lines into `messages` — flush them inline. + result?.messages?.forEach(message => renderSlashOutput(message)) + + if (cmdAction === 'status') { + renderSlashOutput( + result?.connected + ? `browser connected: ${result.url || '(url unavailable)'}` + : 'browser not connected (try /browser connect or set browser.cdp_url in config.yaml)' + ) + + return + } + + if (cmdAction === 'disconnect') { + renderSlashOutput('browser disconnected') + + return + } + + if (result?.connected) { + renderSlashOutput('Browser connected to live Chromium-family browser via CDP') + renderSlashOutput(`Endpoint: ${result.url || '(url unavailable)'}`) + renderSlashOutput('next browser tool call will use this CDP endpoint') + } + } catch (err) { + renderSlashOutput(`error: ${err instanceof Error ? err.message : String(err)}`) + } } } diff --git a/apps/desktop/src/app/types.ts b/apps/desktop/src/app/types.ts index 5082b70406d..9500468482c 100644 --- a/apps/desktop/src/app/types.ts +++ b/apps/desktop/src/app/types.ts @@ -46,6 +46,12 @@ export interface SlashExecResponse { warning?: string } +export interface BrowserManageResponse { + connected?: boolean + url?: string + messages?: string[] +} + export interface SessionSteerResponse { // 'queued' == accepted into the live turn's steer slot (injected at the next // tool-result boundary); 'rejected' == no live tool window, caller queues. diff --git a/apps/desktop/src/lib/desktop-slash-commands.test.ts b/apps/desktop/src/lib/desktop-slash-commands.test.ts index d37738173ce..54f5a6f89df 100644 --- a/apps/desktop/src/lib/desktop-slash-commands.test.ts +++ b/apps/desktop/src/lib/desktop-slash-commands.test.ts @@ -52,6 +52,17 @@ describe('desktop slash command curation', () => { expect(desktopSlashUnavailableMessage('/personality')).toBeNull() }) + it('treats /browser as an executable action command (local-gateway connect)', () => { + // /browser used to be terminal-only; it now resolves to a desktop action + // handler that routes browser.manage RPC when the gateway is local. + expect(isDesktopSlashCommand('/browser')).toBe(true) + expect(isDesktopSlashSuggestion('/browser')).toBe(true) + expect(desktopSlashUnavailableMessage('/browser')).toBeNull() + expect(resolveDesktopCommand('/browser')?.surface).toEqual({ kind: 'action', action: 'browser' }) + // Bare /browser expands to its sub-action options in the popover. + expect(resolveDesktopCommand('/browser')?.args).toBe(true) + }) + it('allows aliases to execute without cluttering the popover', () => { expect(isDesktopSlashSuggestion('/reset')).toBe(false) expect(isDesktopSlashCommand('/reset')).toBe(true) diff --git a/apps/desktop/src/lib/desktop-slash-commands.ts b/apps/desktop/src/lib/desktop-slash-commands.ts index d898a6c83f1..f9ae934edf4 100644 --- a/apps/desktop/src/lib/desktop-slash-commands.ts +++ b/apps/desktop/src/lib/desktop-slash-commands.ts @@ -30,6 +30,7 @@ export interface DesktopThemeCommandOption { */ export type DesktopActionId = | 'branch' + | 'browser' | 'handoff' | 'help' | 'new' @@ -103,6 +104,12 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ { name: '/skin', description: 'Switch desktop theme or cycle to the next one', surface: action('skin'), args: true }, { name: '/title', description: 'Rename the current session', surface: action('title') }, { name: '/help', description: 'Show desktop slash commands', aliases: ['/commands'], surface: action('help') }, + { + name: '/browser', + description: 'Manage browser CDP connection [connect|disconnect|status] (local gateway only)', + surface: action('browser'), + args: true + }, // Overlay pickers { name: '/model', description: 'Switch the model for this session', surface: picker('model'), hidden: true }, @@ -142,7 +149,7 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ // per reason beats 40 identical object literals. const NO_DESKTOP_SURFACE: Record = { terminal: [ - '/browser', '/busy', '/clear', '/compact', '/config', '/copy', '/cron', '/details', + '/busy', '/clear', '/compact', '/config', '/copy', '/cron', '/details', '/exit', '/footer', '/gateway', '/gquota', '/history', '/image', '/indicator', '/logs', '/mouse', '/paste', '/platforms', '/plugins', '/quit', '/redraw', '/reload', '/restart', '/sb', '/set-home', '/sethome', '/snap', '/snapshot', '/statusbar', '/toolsets', '/update', '/verbose'