diff --git a/agent/turn_context.py b/agent/turn_context.py index 120763de10..2fc545ffd3 100644 --- a/agent/turn_context.py +++ b/agent/turn_context.py @@ -79,72 +79,29 @@ def _agent_stale_thinking_on_wire(agent: Any) -> bool: return True -def _context_injection_parts( - ext_prefetch_cache: str, - plugin_user_context: str, -) -> list[str]: - """The ephemeral context pieces (memory prefetch + ``pre_llm_call``). - - Shared by the string sidecar (:func:`compose_user_api_content`) and the - multimodal text-part (:func:`compose_multimodal_context_part`) paths so - both inject byte-identical context regardless of the turn's content shape. - """ - injections: list[str] = [] - if ext_prefetch_cache: - fenced = build_memory_context_block(ext_prefetch_cache) - if fenced: - injections.append(fenced) - if plugin_user_context: - injections.append(plugin_user_context) - return injections +def compose_multimodal_context_part( + ext_prefetch_cache: str, plugin_user_context: str, +) -> Optional[str]: + """The ephemeral context of one turn (memory prefetch + ``pre_llm_call``) as one text + block; ``None`` when nothing is injected. The string sidecar appends it to ``content``; + a multimodal (list) turn carries it as a durable text part (#71998).""" + fenced = build_memory_context_block(ext_prefetch_cache) if ext_prefetch_cache else "" + injections = [part for part in (fenced, plugin_user_context) if part] + return "\n\n".join(injections) if injections else None def compose_user_api_content( content: Any, ext_prefetch_cache: str, plugin_user_context: str ) -> Optional[str]: - """Compose the API-bound content of the current turn's user message. + """Compose the API-bound content of the current turn's string user message. - Sources: memory-manager prefetch + ``pre_llm_call`` plugin context with - target="user_message" (the default). Both are appended to the *API copy* - of the user message only — the stored content stays clean. - - This is the single source of that composition. The prologue stamps the - result onto the live message as ``api_content`` (persisted alongside the - clean content) and the ``api_messages`` build in ``conversation_loop`` - sends the same helper's output, so the persisted sidecar can never drift - from the bytes on the wire — which is the whole prompt-cache invariant: - what turn N sends must be what turn N+1 replays. - - Returns ``None`` when nothing is injected (multimodal/non-string content, - or no ephemeral context), meaning the message is sent as-is. Multimodal - (list) turns take the injection via :func:`compose_multimodal_context_part` - instead, since the string sidecar can't ride on list content. - """ + Single source for the ``api_content`` sidecar and the wire bytes so they never drift + (what turn N sends is what turn N+1 replays). ``None`` when nothing is injected or the + content is not a string (list content takes the text-part path).""" if not isinstance(content, str): return None - injections = _context_injection_parts(ext_prefetch_cache, plugin_user_context) - if not injections: - return None - return content + "\n\n" + "\n\n".join(injections) - - -def compose_multimodal_context_part( - ext_prefetch_cache: str, - plugin_user_context: str, -) -> Optional[str]: - """The memory-prefetch + ``pre_llm_call`` context as a single text part. - - :func:`compose_user_api_content` returns ``None`` for multimodal (list) - turns, so the string ``api_content`` sidecar can't carry this context and - it would silently drop on image/attachment turns (#71998). Callers append - the returned text as a durable content part instead — the same channel the - gateway must-deliver notes use (:func:`append_notes_to_multimodal_content`) - — so an image-only turn still reaches the model with the injected context. - - Returns ``None`` when nothing is injected. - """ - injections = _context_injection_parts(ext_prefetch_cache, plugin_user_context) - return "\n\n".join(injections) if injections else None + injection = compose_multimodal_context_part(ext_prefetch_cache, plugin_user_context) + return None if injection is None else content + "\n\n" + injection def substitute_api_content(api_msg: Dict[str, Any]) -> Optional[str]: @@ -189,7 +146,7 @@ def consume_surface_switch_note(agent: Any) -> str: return _pop_turn_note(agent, "_surface_switch_note") -def append_notes_to_multimodal_content(content: Any, notes: str) -> bool: +def append_notes_to_multimodal_content(content: Any, notes: Optional[str]) -> bool: """Append must-deliver notes as a durable text part on a multimodal (list) user message (the sidecar path returns ``None`` for non-string content).""" if not notes or not isinstance(content, list): @@ -885,16 +842,9 @@ def _bind_interrupt_scope(agent: Any, ra) -> None: def _memory_query_text(original_user_message: Any) -> str: - """The semantic text of a turn's user content for memory queries. - - A multimodal (list) turn carries its text in content parts, so keying the - query off ``isinstance(str)`` alone collapses it to ``""`` — ``on_turn_start`` - sees an empty turn and ``is_trivial_prompt("")`` skips ``prefetch_all`` - entirely, so memory recall never even runs on an image+text turn. That is the - execution-side twin of the delivery gap this PR (#71998) closes: the sidecar - can now carry recall on a multimodal turn, but only if prefetch produced any. - Flatten str/list to text; an image-only turn still yields ``""`` and is - correctly treated as trivial (no semantic text to query on).""" + """Semantic text of the turn for memory queries: a multimodal (list) turn carries its text + in parts, so keying off ``isinstance(str)`` collapsed it to ``""`` and ``is_trivial_prompt`` + skipped prefetch entirely. An image-only turn still flattens to ``""`` (trivial).""" if isinstance(original_user_message, (str, list)): return flatten_message_text(original_user_message) return "" @@ -936,26 +886,9 @@ def _stamp_api_content_sidecar( plugin_user_context: str, *, preflight_compressed: bool, ) -> None: """api_content sidecar — persist what you send: injected context lives only in the - API copy, so stamp the exact sent bytes on the live dict for replay. - - Multimodal (list) turns can't carry the string ``api_content`` sidecar — - :func:`compose_user_api_content` returns ``None`` for them — so the memory/ - plugin context would silently drop on an image-only turn (#71998). For those, - deliver the context as a durable text part instead (the same channel the - gateway must-deliver notes use), keeping the wire byte-identical to what is - persisted and replayed.""" + API copy, so stamp the exact sent bytes on the live dict for replay.""" _turn_user_msg = messages[current_turn_user_idx] live_content = _turn_user_msg.get("content") - # Multimodal (list) turns can't carry the string ``api_content`` sidecar — - # :func:`compose_user_api_content` returns ``None`` for them — so the memory/ - # plugin context would silently drop on an image-only turn (#71998). Deliver it - # as a durable text part instead (the same channel the gateway must-deliver notes - # use), which is persisted and replayed as ordinary content — no sidecar needed. - if isinstance(live_content, list): - _mm_ctx = compose_multimodal_context_part(ext_prefetch_cache, plugin_user_context) - if _mm_ctx: - append_notes_to_multimodal_content(live_content, _mm_ctx) - return from agent.session_persistence import _persist_lock, durable_user_row_content # Match the row the flush wrote (persist override = clean transcript), not the live bytes. durable_content, _api_content = durable_user_row_content( @@ -995,6 +928,38 @@ def _stamp_api_content_sidecar( logger.warning("api_content backfill failed for session=%s", agent.session_id or "none", exc_info=True) +def _append_multimodal_context( + agent: Any, turn_user_msg: Dict[str, Any], ext_prefetch_cache: str, plugin_user_context: str, + *, preflight_compressed: bool, +) -> None: + """Multimodal (list) content takes no string sidecar: the turn's context becomes a durable + text part on the current turn's live list (the gateway must-deliver-note channel, #71998), + so wire, persisted row, compaction and replay all carry the same parts. Runs once per turn, + before the first request; historical rows are never touched. + + A user row another writer materialized BEFORE the prologue (in-place preflight compaction, + a close/early flush that raced it) is updated in place: the crash persist marker-skips that + message, so without this a resumed session replays a view the model never saw. Same + ``_row_id``-under-lock protocol as the string sidecar backfill; the row keeps its writer's + shape (compaction inserted the raw parts, a flush the text projection).""" + _mm_ctx = compose_multimodal_context_part(ext_prefetch_cache, plugin_user_context) + if not append_notes_to_multimodal_content(turn_user_msg.get("content"), _mm_ctx): + return + from agent.session_persistence import _durable_content, _persist_lock + + with _persist_lock(agent): + _row_id = turn_user_msg.get("_row_id") + _db = getattr(agent, "_session_db", None) + if _db is None or not isinstance(_row_id, int): + return + _in_place_compacted = preflight_compressed and bool(getattr(agent, "_last_compaction_in_place", False)) + content = turn_user_msg["content"] if _in_place_compacted else _durable_content(turn_user_msg["content"]) + try: + _db.set_user_message_content(agent.session_id, _row_id, content) + except Exception: + logger.warning("multimodal context backfill failed for session=%s", agent.session_id or "none", exc_info=True) + + def _persist_turn_start( agent: Any, messages: List[Any], conversation_history: Optional[List[Any]], pending_cli_message: Any, @@ -1158,24 +1123,26 @@ def build_turn_context( _bind_interrupt_scope(agent, ra) ext_prefetch_cache = _memory_turn_start_and_prefetch(agent, original_user_message, turn_author) - # Sidecar skipped for codex_app_server/MoA. - if ( - not moa_active - and getattr(agent, "api_mode", None) != "codex_app_server" - and 0 <= current_turn_user_idx < len(messages) - and messages[current_turn_user_idx].get("role") == "user" - ): - _stamp_api_content_sidecar( - agent, messages, current_turn_user_idx, ext_prefetch_cache, - plugin_user_context, preflight_compressed=compaction.compressed, - ) + # Title the session now: titling depends only on the user's ask (before any injected + # context lands on list content), so it runs concurrently with the turn. Daemon thread, + # no-op once titled; it ensures the session row itself. + _maybe_title_session_at_turn_start(agent, messages) + + # Sidecar skipped for codex_app_server/MoA; list content carries its context as a part in every mode. + if 0 <= current_turn_user_idx < len(messages) and messages[current_turn_user_idx].get("role") == "user": + if isinstance(messages[current_turn_user_idx].get("content"), list): + _append_multimodal_context( + agent, messages[current_turn_user_idx], ext_prefetch_cache, plugin_user_context, + preflight_compressed=compaction.compressed, + ) + elif not moa_active and getattr(agent, "api_mode", None) != "codex_app_server": + _stamp_api_content_sidecar( + agent, messages, current_turn_user_idx, ext_prefetch_cache, + plugin_user_context, preflight_compressed=compaction.compressed, + ) _persist_turn_start(agent, messages, conversation_history, pending_cli_message) - # Title the session now: the row exists and titling depends only on the user's ask, - # so it runs concurrently with the turn. Daemon thread, no-op once titled. - _maybe_title_session_at_turn_start(agent, messages) - return TurnContext( user_message=user_message, original_user_message=original_user_message, messages=messages, conversation_history=conversation_history, active_system_prompt=active_system_prompt, diff --git a/tests/agent/test_api_content_sidecar.py b/tests/agent/test_api_content_sidecar.py index 3084231e0b..2912372ce4 100644 --- a/tests/agent/test_api_content_sidecar.py +++ b/tests/agent/test_api_content_sidecar.py @@ -57,67 +57,27 @@ class TestComposeUserApiContent: class TestComposeMultimodalContextPart: - """#71998: the injection composed as a standalone text part for the - multimodal (list) content path, mirroring the string sidecar's sources.""" - - def test_none_when_nothing_to_inject(self): + def test_is_the_string_sidecar_injection_tail(self): + """Both content shapes inject byte-identical context (#71998): the text part a list + turn carries is exactly what the string sidecar appends after ``content``.""" assert compose_multimodal_context_part("", "") is None - - def test_plugin_context_only(self): - assert compose_multimodal_context_part("", "CTX") == "CTX" - - def test_memory_block_and_plugin_context(self): - out = compose_multimodal_context_part("likes tea", "CTX") - fenced = build_memory_context_block("likes tea") - assert out == fenced + "\n\n" + "CTX" - - def test_matches_string_sidecar_injection_tail(self): - # The multimodal part is exactly the string sidecar minus the leading - # content + separator, so both paths inject byte-identical context. sidecar = compose_user_api_content("hello", "likes tea", "CTX") part = compose_multimodal_context_part("likes tea", "CTX") assert sidecar == "hello\n\n" + part class TestMemoryQueryText: - """#71998 execution side: the memory query must flatten multimodal (list) - content to its text, or prefetch/on_turn_start never run on an image+text - turn and the delivery sidecar has nothing to carry.""" - - def test_str_passthrough(self): - assert _memory_query_text("what is my address") == "what is my address" - - def test_text_plus_image_list_yields_text(self): - content = [ - {"type": "text", "text": "what is in this photo of my house"}, - {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}}, - ] - assert _memory_query_text(content) == "what is in this photo of my house" - - def test_image_only_list_yields_empty(self): - content = [ - {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}}, - ] - assert _memory_query_text(content) == "" - - def test_non_text_types_yield_empty(self): - assert _memory_query_text(None) == "" - assert _memory_query_text(12345) == "" - - def test_drives_trivial_prompt_gate_correctly(self): + def test_list_turn_queries_its_text_and_image_only_stays_trivial(self): + """#71998 execution side: a text+image turn must drive prefetch off its text (it used to + collapse to ``""`` and skip recall silently); an image-only turn has no text to query.""" from agent.memory_provider import is_trivial_prompt - text_plus_image = [ - {"type": "text", "text": "remind me what my dog's name is"}, - {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}}, - ] - image_only = [ - {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}}, - ] - # A text+image turn is NOT trivial once flattened, so prefetch runs... + image = {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}} + text_plus_image = [{"type": "text", "text": "remind me what my dog's name is"}, image] + assert _memory_query_text(text_plus_image) == "remind me what my dog's name is" assert is_trivial_prompt(_memory_query_text(text_plus_image)) is False - # ...while an image-only turn stays trivial (no semantic text to query). - assert is_trivial_prompt(_memory_query_text(image_only)) is True + assert _memory_query_text([image]) == "" + assert is_trivial_prompt(_memory_query_text([image])) is True # --------------------------------------------------------------------------- @@ -376,20 +336,6 @@ class TestPrologueStamping: # Multimodal turns carry the context durably, not via the string sidecar. assert "api_content" not in ctx.messages[ctx.current_turn_user_idx] - def test_no_context_part_for_multimodal_without_injections(self): - """A multimodal turn with no ephemeral context is left untouched.""" - agent = _FakeAgent() - blocks = [{"type": "image_url", "image_url": {"url": "data:img"}}] - with patch("hermes_cli.plugins.invoke_hook", return_value=[]): - ctx = _build( - agent, - user_message=blocks, - summarize_user_message_for_log=lambda _m: "[image]", - ) - content = ctx.messages[ctx.current_turn_user_idx]["content"] - assert content == blocks # no stray text part appended - assert "api_content" not in ctx.messages[ctx.current_turn_user_idx] - # --------------------------------------------------------------------------- # Flush: persist-override rows keep the sent bytes in the sidecar (#48677) @@ -655,6 +601,32 @@ class TestWireInvariant: current = _user_messages(_chat_requests(handler)[0])[-1] assert current["content"] == "second question\n\nPLUGIN-CTX" + def test_multimodal_turn_sends_persists_and_replays_context_part(self, wire_env): + """#71998: on a list-content (image) turn the ``pre_llm_call`` context reaches the + wire as a text part, the persisted row carries it, and a resumed turn N+1 replays + the same view — same contract as the string sidecar path.""" + make_agent, handler, db, sid = wire_env + from run_agent import AIAgent + image = {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}} + turn = [{"type": "text", "text": "what is this"}, image] + + agent1 = make_agent() + with patch.object(AIAgent, "_model_supports_vision", return_value=True): # keep native parts on the wire + agent1.run_conversation(list(turn), conversation_history=[], task_id="t1") + + sent = _user_messages(_chat_requests(handler)[0])[0]["content"] + assert sent == [*turn, {"type": "text", "text": "PLUGIN-CTX"}] + + history = db.get_messages_as_conversation(sid) + assert "PLUGIN-CTX" in history[0]["content"] # persisted with the turn, not dropped + + handler.captured_requests = [] + agent2 = make_agent() + with patch.object(AIAgent, "_model_supports_vision", return_value=True): + agent2.run_conversation("second question", conversation_history=history, task_id="t2") + replayed = _user_messages(_chat_requests(handler)[0])[0]["content"] + assert replayed == history[0]["content"] + # --------------------------------------------------------------------------- # Review fixes: re-anchoring, MoA, in-place compaction backfill, override @@ -1013,7 +985,7 @@ class TestSessionRowExistsBeforePreflightCompaction: before the delayed persist. Drives the real ``compress_context`` path against a real, empty SessionDB.""" - def _make_agent(self, db, sid, *, in_place): + def _make_agent(self, db, sid, *, in_place, current_user_content="hello"): from run_agent import AIAgent with patch.dict(os.environ, {"OPENROUTER_API_KEY": "test-key"}): @@ -1044,7 +1016,7 @@ class TestSessionRowExistsBeforePreflightCompaction: seen = {} compacted = [ {"role": "assistant", "content": "[CONTEXT COMPACTION] summary"}, - {"role": "user", "content": "hello"}, + {"role": "user", "content": current_user_content}, ] def _compress(_messages, **_kwargs): @@ -1101,6 +1073,31 @@ class TestSessionRowExistsBeforePreflightCompaction: finally: db.close() + def test_in_place_compaction_multimodal_context_part_survives_reload(self, tmp_path): + """#71998 persistence: in-place ``archive_and_compact`` writes the current-turn user + row BEFORE the prologue appends the ``pre_llm_call`` text part, and the crash persist + identity-skips compacted dicts — the part must be pushed into that row, or a + resumed session replays a view the model never saw.""" + db = SessionDB(db_path=tmp_path / "state.db") + sid = "sess-inplace-mm" + image = {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}} + turn = [{"type": "text", "text": "what is this"}, image] + try: + agent, _seen = self._make_agent(db, sid, in_place=True, current_user_content=list(turn)) + with patch("hermes_cli.plugins.invoke_hook", return_value=[{"context": "PLUGIN-CTX"}]): + ctx = _build( + agent, user_message=list(turn), conversation_history=self._oversized_history(), + summarize_user_message_for_log=lambda _m: "[image]", + ) + assert agent._last_compaction_in_place is True + live = ctx.messages[ctx.current_turn_user_idx]["content"] + assert live == [*turn, {"type": "text", "text": "PLUGIN-CTX"}] + # Reload: the durable row carries the same parts the model saw. + reloaded = [m for m in db.get_messages_as_conversation(sid) if m["role"] == "user"] + assert reloaded[-1]["content"] == live + finally: + db.close() + def test_rotation_first_turn_compaction_creates_child(self, tmp_path): db = SessionDB(db_path=tmp_path / "state.db") sid = "sess-fresh-rot" diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index 6e998f0c19..9f52d1750e 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -682,7 +682,7 @@ def my_callback(session_id: str, user_message: str, conversation_history: list, | Parameter | Type | Description | |-----------|------|-------------| | `session_id` | `str` | Unique identifier for the current session | -| `user_message` | `str` | The user's original message for this turn (before any skill injection) | +| `user_message` | `str \| list` | The user's original message for this turn (before any skill injection). A multimodal turn (image or other attachment) is the list of content parts, exactly as sent | | `conversation_history` | `list` | Copy of the full message list (OpenAI format: `[{"role": "user", "content": "..."}]`) | | `is_first_turn` | `bool` | `True` if this is the first turn of a new session, `False` on subsequent turns | | `model` | `str` | The model identifier (e.g. `"anthropic/claude-sonnet-4.6"`) | @@ -707,6 +707,8 @@ return None The clean user-message `content` remains unchanged. For replay and prompt-cache stability, Hermes may persist the exact API-bound message, including plugin-injected context, in the row's `api_content` sidecar. +On a **multimodal turn** (the user message is a list of content parts — an image attachment, or text sent as parts) there is no string sidecar: the joined context is appended to that turn's content as one extra `{"type": "text"}` part, before the first request, and the part is persisted with the turn so a resumed session, compaction and replay all see the same message the model saw. Earlier messages and the system prompt are never touched. + When **multiple plugins** return context, their outputs are joined with double newlines in plugin discovery order (alphabetical by directory name). **Use cases:** Memory recall, RAG context injection, guardrails, per-turn analytics.