diff --git a/docs/middleware/README.md b/docs/middleware/README.md index 4a5c06f8cbe16..d57b9d1c14fca 100644 --- a/docs/middleware/README.md +++ b/docs/middleware/README.md @@ -62,6 +62,20 @@ return { Hermes stores those trace entries in later observer hook payloads as `middleware_trace`. +LLM request callbacks compose in registration order: each accepted complete +`{"request": {...}}` replacement becomes the next callback's effective request, +while `original_request` remains the pre-middleware snapshot. Each callback sees +isolated deep copies. If a callback mutates its candidate and raises, that +candidate is discarded and Hermes continues from the last committed request. +There is no shallow-copy fallback: if the complete request cannot be deep-copied, +Hermes logs the isolation failure, skips `llm_request` for that call, and sends +the original request unchanged. + +`llm_request` delivery performs first-use plugin discovery itself, so a +configured plugin runs even on surfaces that never import the model-tool layer +(dashboards, TUI slash workers, query mode, cron) and is never mistaken for a +missing listener in a fresh process. + Execution middleware receives a `next_call` callback. Call it to continue the chain: diff --git a/hermes_cli/middleware.py b/hermes_cli/middleware.py index e8c00e8200d57..126f9178507dc 100644 --- a/hermes_cli/middleware.py +++ b/hermes_cli/middleware.py @@ -78,12 +78,25 @@ def apply_llm_request_middleware( request: Dict[str, Any], **context: Any, ) -> RequestMiddlewareResult: - """Apply registered LLM request middleware. + """Apply registered LLM request middleware transactionally. - Middleware may return ``{"request": {...}}`` to replace the effective - provider kwargs before Hermes sends them. + Callbacks run in registration order on isolated copies of the complete + provider request. A replacement commits only after it can be deep-copied. + If the original request cannot be isolated, middleware is skipped and the + original request is returned unchanged. """ - if not _has_middleware(LLM_REQUEST_MIDDLEWARE): + # Plugin discovery is normally triggered as an import side effect of + # model_tools, but this seam must not depend on which surface imported + # Hermes first (dashboards, TUI slash workers, query mode, cron). Ensure + # first-use discovery has run before snapshotting callbacks so a + # configured plugin in a fresh process is not mistaken for "no middleware". + # discover_plugins() is idempotent. + from hermes_cli.plugins import discover_plugins + + discover_plugins() + + callbacks = _get_middleware_callbacks(LLM_REQUEST_MIDDLEWARE) + if not callbacks: return RequestMiddlewareResult( payload=request, original_payload=request, @@ -91,22 +104,71 @@ def apply_llm_request_middleware( trace=[], ) - original_request = _safe_copy(request) - current_request = _safe_copy(original_request) + try: + original_request = deepcopy(request) + except Exception as exc: + logger.warning( + "Middleware 'llm_request' skipped: request payload could not be " + "isolated transactionally: %s", + exc, + ) + return RequestMiddlewareResult( + payload=request, + original_payload=request, + changed=False, + trace=[], + ) + + current_request = original_request trace: List[Dict[str, Any]] = [] - for result in _invoke_middleware( - LLM_REQUEST_MIDDLEWARE, - request=current_request, - original_request=original_request, - **context, - ): + # ponytail: keep the transaction boundary inline until a second request + # chain actually needs the same abstraction. + for callback in callbacks: + try: + callback_request = deepcopy(current_request) + callback_original = deepcopy(original_request) + except Exception as exc: + logger.warning( + "Middleware 'llm_request' callback %s skipped: request payload " + "could not be isolated transactionally: %s", + getattr(callback, "__name__", repr(callback)), + exc, + ) + continue + + try: + result = callback( + **middleware_payload( + **context, + request=callback_request, + original_request=callback_original, + ) + ) + except Exception as exc: + logger.warning( + "Middleware 'llm_request' callback %s raised: %s", + getattr(callback, "__name__", repr(callback)), + exc, + ) + continue + if not isinstance(result, dict): continue next_request = result.get("request") if not isinstance(next_request, dict): continue - current_request = _safe_copy(next_request) + + try: + current_request = deepcopy(next_request) + except Exception as exc: + logger.warning( + "Middleware 'llm_request' callback %s returned a request that " + "could not be isolated transactionally: %s", + getattr(callback, "__name__", repr(callback)), + exc, + ) + continue trace.append(_trace_entry(result)) return RequestMiddlewareResult( diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index c86fd73ed5b57..ed57537f3351a 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -292,7 +292,13 @@ def test_plugin_can_register_and_invoke_middleware(self, tmp_path, monkeypatch): def test_middleware_helpers_skip_no_listener_work(self, monkeypatch): - manager = types.SimpleNamespace(_middleware={}) + manager = types.SimpleNamespace( + _middleware={}, + # apply_llm_request_middleware triggers first-use discovery before + # snapshotting callbacks; the no-op models an already-discovered + # manager with no registered listeners. + discover_and_load=lambda force=False: None, + ) monkeypatch.setattr("hermes_cli.plugins.get_plugin_manager", lambda: manager) request = {"messages": []} diff --git a/tests/hermes_cli/test_request_transform_contract.py b/tests/hermes_cli/test_request_transform_contract.py new file mode 100644 index 0000000000000..6628f5d07185e --- /dev/null +++ b/tests/hermes_cli/test_request_transform_contract.py @@ -0,0 +1,191 @@ +"""Regression tests for the outbound LLM request transform boundary.""" + +from __future__ import annotations + +import inspect +import logging +import types + +from agent import conversation_loop +from hermes_cli.middleware import apply_llm_request_middleware + + +class _UncopyableProviderObject: + def __deepcopy__(self, memo): + raise TypeError("opaque provider object cannot be deep-copied") + + +def _install(monkeypatch, *callbacks): + manager = types.SimpleNamespace( + _middleware={"llm_request": list(callbacks)}, + # The mocked manager models "already discovered"; discover_and_load is + # a no-op because the middleware is pre-registered. + discover_and_load=lambda force=False: None, + ) + monkeypatch.setattr("hermes_cli.plugins.get_plugin_manager", lambda: manager) + + +def test_fresh_process_discovers_configured_plugin_before_snapshot(monkeypatch): + """A configured llm_request plugin must run on first delivery even when + plugin discovery has not happened yet — this seam cannot depend on which + surface imported Hermes first. Without the discovery-aware snapshot a + fresh manager (empty registry, _discovered False) was mistaken for "no + middleware" and silently skipped the configured transform.""" + + manager = types.SimpleNamespace( + _middleware={}, + _discovered=False, + ) + + def _discover_and_load(force=False): + # Simulate first-use discovery registering a configured plugin. + if "llm_request" not in manager._middleware: + manager._middleware["llm_request"] = [ + lambda **kw: { + "request": {**kw["request"], "fresh": True}, + "source": "fresh", + } + ] + + manager.discover_and_load = _discover_and_load + monkeypatch.setattr("hermes_cli.plugins.get_plugin_manager", lambda: manager) + + request = {"messages": [{"role": "user", "content": "hello"}]} + result = apply_llm_request_middleware(request) + + assert result.payload == {**request, "fresh": True} + assert result.changed is True + assert result.trace == [{"source": "fresh"}] + + +def test_missing_middleware_preserves_request_identity(monkeypatch): + _install(monkeypatch) + request = {"messages": [{"role": "user", "content": "hello"}]} + + result = apply_llm_request_middleware(request) + + assert result.payload is request + assert result.original_payload is request + assert result.changed is False + assert result.trace == [] + + +def test_request_replacements_chain_with_stable_original(monkeypatch): + seen = [] + + def first(**kwargs): + kwargs["original_request"]["tampered"] = True + return {"request": {**kwargs["request"], "first": True}, "source": "first"} + + def second(**kwargs): + seen.append((kwargs["request"], kwargs["original_request"])) + return {"request": {**kwargs["request"], "second": True}, "source": "second"} + + _install(monkeypatch, first, second) + request = { + "model": "test-model", + "messages": [{"role": "user", "content": "hello"}], + "tools": [{"type": "function", "function": {"name": "demo"}}], + "stream": True, + } + + result = apply_llm_request_middleware(request) + + assert seen == [({**request, "first": True}, request)] + assert result.payload == {**request, "first": True, "second": True} + assert result.original_payload == request + assert result.trace == [{"source": "first"}, {"source": "second"}] + + +def test_callback_failure_discards_nested_mutation(monkeypatch, caplog): + seen = [] + + def first(**kwargs): + return {"request": {**kwargs["request"], "first": True}, "source": "first"} + + def failing(**kwargs): + kwargs["request"]["messages"][0]["content"] = "corrupted" + kwargs["request"]["partial"] = True + raise RuntimeError("broken transformer") + + def last(**kwargs): + seen.append(kwargs["request"]) + return {"request": {**kwargs["request"], "last": True}, "source": "last"} + + _install(monkeypatch, first, failing, last) + request = {"messages": [{"role": "user", "content": "original"}]} + + with caplog.at_level(logging.WARNING): + result = apply_llm_request_middleware(request) + + committed = {"messages": [{"role": "user", "content": "original"}], "first": True} + assert seen == [committed] + assert result.payload == {**committed, "last": True} + assert request == {"messages": [{"role": "user", "content": "original"}]} + assert "broken transformer" in caplog.text + + +def test_uncopyable_original_skips_callbacks_without_shared_state(monkeypatch, caplog): + called = False + + def callback(**kwargs): + nonlocal called + called = True + kwargs["request"]["messages"][0]["content"] = "changed" + raise RuntimeError("must not run") + + _install(monkeypatch, callback) + request = { + "messages": [{"role": "user", "content": "original"}], + "provider_state": _UncopyableProviderObject(), + } + + with caplog.at_level(logging.WARNING): + result = apply_llm_request_middleware(request) + + assert called is False + assert result.payload is request + assert result.original_payload is request + assert request["messages"][0]["content"] == "original" + assert "could not be isolated transactionally" in caplog.text + + +def test_uncopyable_replacement_is_not_committed(monkeypatch, caplog): + def callback(**kwargs): + kwargs["request"]["messages"][0]["content"] = "candidate-only" + return { + "request": { + **kwargs["request"], + "provider_state": _UncopyableProviderObject(), + }, + "source": "uncopyable", + } + + _install(monkeypatch, callback) + request = {"messages": [{"role": "user", "content": "original"}]} + + with caplog.at_level(logging.WARNING): + result = apply_llm_request_middleware(request) + + assert result.payload == request + assert result.changed is False + assert result.trace == [] + assert request["messages"][0]["content"] == "original" + assert "returned a request that could not be isolated transactionally" in caplog.text + + +def test_transform_runs_before_common_provider_dispatch(): + source = inspect.getsource(conversation_loop.run_conversation) + + transform_at = source.index("apply_llm_request_middleware(") + observer_at = source.index('"pre_api_request"', transform_at) + dispatch_at = source.index("def _perform_api_call", transform_at) + execution_at = source.index("run_llm_execution_middleware(", dispatch_at) + + assert transform_at < observer_at < dispatch_at < execution_at + + dispatch_block = source[dispatch_at:execution_at] + assert 'agent.api_mode == "codex_responses"' in dispatch_block + assert "agent._interruptible_streaming_api_call(" in dispatch_block + assert "relay_llm.execute(" in dispatch_block + assert "agent._interruptible_api_call" in dispatch_block