From 915942935d88a272fc8c60786bf18385a7b71f8a Mon Sep 17 00:00:00 2001 From: xue xinglong Date: Tue, 14 Jul 2026 16:15:24 +0800 Subject: [PATCH] fix(context-engine): fail open on empty select_context() result + doc public hooks - _apply_context_engine_selection: reject an empty list. all([]) is True, so a [] returned by a failing/buggy engine previously replaced a valid request with an empty message list the downstream sanitizers can't restore; now it falls open to the unmodified request (honors the fail-open contract). Thanks @johnnykor82 for catching this on #41918's review. - test: empty list keeps the original request (fail-open regression). - docs: document select_context()/on_turn_complete() in the public context-engine plugin guide (were still describing only the old contract). --- agent/conversation_loop.py | 11 ++++-- .../test_context_engine_select_context.py | 23 ++++++++++++ .../developer-guide/context-engine-plugin.md | 36 +++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 425d1ff1eec..c11694ed49e 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -758,12 +758,17 @@ def _apply_context_engine_selection( if selected is None: return api_messages - if isinstance(selected, list) and all(isinstance(m, dict) for m in selected): + # Require a NON-EMPTY list of dicts. An empty list must fall open to the + # original request: ``all([])`` is ``True``, so without the emptiness check + # a ``[]`` returned by a buggy/failing engine would replace a valid request + # with an empty message list that the downstream sanitizers cannot restore, + # reaching the provider as an invalid request instead of failing open. + if isinstance(selected, list) and selected and all(isinstance(m, dict) for m in selected): return selected logger.warning( - "Context engine select_context returned a non-list of dicts; " - "ignoring (session=%s)", + "Context engine select_context returned an invalid value " + "(not a non-empty list of dicts); ignoring (session=%s)", session_label, ) return api_messages diff --git a/tests/agent/test_context_engine_select_context.py b/tests/agent/test_context_engine_select_context.py index 3927ef9aa54..906441870e7 100644 --- a/tests/agent/test_context_engine_select_context.py +++ b/tests/agent/test_context_engine_select_context.py @@ -173,6 +173,29 @@ def test_list_of_non_dicts_is_ignored(): assert out is REQUEST +def test_empty_list_keeps_original_request(): + """An empty list must fall open to the original request. + + ``all([])`` is ``True``, so without an emptiness check a ``[]`` returned by + a failing/buggy engine would replace a valid assembled request with an + empty message list the downstream sanitizers cannot restore — reaching the + provider as an invalid request instead of failing open. Guards the fail-open + contract. + """ + + class _Engine(_MinimalEngine): + def select_context(self, request_messages, **kwargs): + return [] + + logger = MagicMock() + agent = _agent_with(_Engine()) + out = _apply_context_engine_selection( + agent, REQUEST, HISTORY, HISTORY[-1], logger=logger + ) + assert out is REQUEST + assert logger.warning.called + + def test_persisted_history_not_mutated(): """The hook must not mutate the persisted conversation history.""" diff --git a/website/docs/developer-guide/context-engine-plugin.md b/website/docs/developer-guide/context-engine-plugin.md index a6e53de9dbf..c0f2dacfa3e 100644 --- a/website/docs/developer-guide/context-engine-plugin.md +++ b/website/docs/developer-guide/context-engine-plugin.md @@ -97,6 +97,42 @@ These have sensible defaults in the ABC. Override as needed: | `handle_tool_call(name, args, **kwargs)` | Returns error JSON | You implement tool handlers | | `should_compress_preflight(messages)` | Returns `False` | You can do a cheap pre-API-call estimate | | `get_status()` | Standard token/threshold dict | You have custom metrics to expose | +| `select_context(request_messages, *, conversation_messages, incoming_message, budget_tokens)` | Returns `None` (no-op) | You select/route which context enters **this** request (retrieval, topic routing) — see below | +| `on_turn_complete(messages, usage=None, **kwargs)` | No-op | You ingest/index/observe the finished turn — see below | + +## Per-turn context selection and observation + +`compress()` answers "context is too long → make it shorter". Two optional, +no-op-default hooks cover the orthogonal *selection / observation* axis, so an +engine no longer has to force `should_compress()` to `True` and abuse +`compress()` as a per-turn callback: + +```python +def select_context(self, request_messages, *, conversation_messages=None, + incoming_message=None, budget_tokens=0): + """Choose/replace the context for THIS request, before dispatch. + + Return a new message list to use for this one provider call (retrieval, + topic routing, role/branch switching), or None to leave it unchanged. + Request-only: the persisted conversation history is never mutated. + """ + +def on_turn_complete(self, messages, usage=None, **kwargs): + """Observe a finished turn after the assistant/tool loop completes. + + Receives a shallow copy of the finalized transcript plus the turn's + canonical usage dict (or None if no provider response was reached), so the + engine can ingest/index/summarize for the next select_context(). The return + value is ignored. + """ +``` + +Contract: + +- **No-op by default, fail-open.** Both default to `return None`. A missing hook, an exception, or an invalid return value leaves the request untouched — so a failing engine is never worse than not installing one. +- **`select_context()` is request-only.** The returned list replaces the messages for a single provider call; persisted history is never written. Returning `None`, `[]`, a non-list, or a list containing non-dicts all fall open to the unmodified request. +- **Ordering / cache stability.** The hook runs **before** prompt cache-control and every request sanitizer, so (a) a replacement still passes the same validation as any request, and (b) the no-op default leaves the request byte-identical — prompt-cache behaviour is unchanged for non-implementing engines. An engine that replaces the list changes only its own cache prefix. Evaluated per provider request (re-runs on retries). +- **`on_turn_complete()`** is post-turn observation only; treat `messages` as read-only. ## Engine tools