diff --git a/src/octop/api/routers/chat/serialize.py b/src/octop/api/routers/chat/serialize.py index f78ecbfc..c1da2f44 100644 --- a/src/octop/api/routers/chat/serialize.py +++ b/src/octop/api/routers/chat/serialize.py @@ -11,11 +11,13 @@ from typing import Any from octop.api.common.agent_workspace import resolve_agent_workspace_dir +from octop.i18n.domains.attachment import attachment_empty_image from octop.infra.gateway.process.message_keys import ( COMPOSER_CTX_KEY, INBOUND_ATTACHMENTS_KEY, ) from octop.infra.utils.llm_text import strip_thinking as _strip_thinking +from octop.infra.utils.locale import normalize_locale logger = logging.getLogger(__name__) @@ -24,6 +26,21 @@ re.IGNORECASE, ) +# Matches the lightweight placeholder that ``MediaOffloadMiddleware`` writes +# into LangGraph state for already-offloaded inline images / audio. Format +# (see harness_agent.middleware.media_offload._placeholder_text_block): +# [ offloaded: sha= path= size=B mime=; +# use read_file to retrieve bytes] +# We strip these on history serialization because the original bytes are +# still available via ``inbound_attachments`` and the dashboard renders the +# thumbnail from there — leaving the placeholder visible made the chat show +# a "[image offloaded: sha=… path=…]" line under every user image after the +# second turn (when the middleware first re-encountered the block). +_OFFLOAD_PLACEHOLDER_RE = re.compile( + r"^\s*\[\s*(?:image|audio)\s+offloaded\s*:", + re.IGNORECASE, +) + # Must match harness_agent.agent.CHECKPOINT_TS_KEY (epoch-ms in additional_kwargs). CHECKPOINT_TS_KEY = "checkpoint_ts" @@ -35,6 +52,73 @@ def _clamp_history_limit(limit: int) -> int: return max(1, min(limit, HISTORY_MAX_LIMIT)) +def _is_offload_placeholder_block(block: Any) -> bool: + """True when *block* is a ``MediaOffloadMiddleware`` placeholder. + + The middleware rewrites an inline image/audio block into a single text + block of the form ``[image offloaded: sha=… path=… size=…B mime=…; use + read_file to retrieve bytes]`` on every turn after the first one. We + must not surface that text in the dashboard: the original attachment + is still available via ``inbound_attachments`` and the UI renders the + image from there. Showing the placeholder underneath is a UX bug. + """ + if not isinstance(block, dict): + return False + if str(block.get("type") or "").lower() != "text": + return False + text = str(block.get("text") or "") + return bool(_OFFLOAD_PLACEHOLDER_RE.match(text)) + + +def _user_message_has_image_attachment(additional_kwargs: Any) -> bool: + """True if the persisted ``INBOUND_ATTACHMENTS_KEY`` carries any image.""" + if not isinstance(additional_kwargs, dict): + return False + raw = additional_kwargs.get(INBOUND_ATTACHMENTS_KEY) + if not isinstance(raw, list): + return False + for item in raw: + if not isinstance(item, dict): + continue + kind = str(item.get("kind") or "").lower() + media_type = str(item.get("media_type") or item.get("mediaType") or "") + if kind == "image" or media_type.lower().startswith("image/"): + return True + return False + + +def _strip_image_only_text_blocks( + blocks: list[dict[str, Any]], + *, + locale: str, +) -> list[dict[str, Any]]: + """Drop placeholders + the LLM-facing "User sent an image." sentinel. + + Only safe when the original image is also being delivered to the + dashboard via ``inbound_attachments``; if not, removing the text + would make a pure-image turn look empty in the UI. + """ + empty_image = attachment_empty_image(normalize_locale(locale)).strip() + out: list[dict[str, Any]] = [] + for block in blocks: + if _is_offload_placeholder_block(block): + continue + if ( + empty_image + and isinstance(block, dict) + and str(block.get("type") or "").lower() == "text" + and str(block.get("text") or "").strip() == empty_image + ): + continue + out.append(block) + return out + + +def _user_locale(user: Any) -> str: + raw = getattr(user, "locale", None) if user is not None else None + return normalize_locale(str(raw) if raw else None) + + def _slice_message_page( raw: list[Any], *, @@ -112,7 +196,7 @@ async def _load_thread_messages( offset, ) for m in raw_messages: - entry = _serialize_history_message(m) + entry = _serialize_history_message(m, user=user) if entry is not None: out.append(entry) except Exception: @@ -474,7 +558,7 @@ def _split_string_thinking(text: str) -> list[dict[str, Any]]: return blocks -def _serialize_history_message(msg: Any) -> dict[str, Any] | None: +def _serialize_history_message(msg: Any, *, user: Any = None) -> dict[str, Any] | None: """Project a LangGraph checkpoint message into dashboard history shape.""" role = _message_role(msg) if role in ("system", ""): @@ -512,7 +596,19 @@ def _serialize_history_message(msg: Any) -> dict[str, Any] | None: if role == "assistant": blocks.extend(_tool_use_blocks(_msg_attr(msg, "tool_calls"))) - if not blocks: + raw_att = ( + additional_kwargs.get(INBOUND_ATTACHMENTS_KEY) + if isinstance(additional_kwargs, dict) + else None + ) + has_user_attachments = isinstance(raw_att, list) and bool(raw_att) + if role == "user" and _user_message_has_image_attachment(additional_kwargs): + # The original image is being delivered through inbound_attachments; + # the image/audio offload placeholders and the LLM-only "User sent + # an image." sentinel are redundant noise on the dashboard. + blocks = _strip_image_only_text_blocks(blocks, locale=_user_locale(user)) + + if not blocks and not (role == "user" and has_user_attachments): return None entry = {"role": role, "content": blocks} @@ -524,8 +620,7 @@ def _serialize_history_message(msg: Any) -> dict[str, Any] | None: raw_ctx = additional_kwargs.get(COMPOSER_CTX_KEY) if isinstance(raw_ctx, dict) and raw_ctx: entry["composer_context"] = raw_ctx - raw_att = additional_kwargs.get(INBOUND_ATTACHMENTS_KEY) - if isinstance(raw_att, list) and raw_att: + if has_user_attachments: entry["inbound_attachments"] = raw_att ts_ms = _extract_message_timestamp_ms(msg) if ts_ms is not None: diff --git a/tests/unit/api/test_chat_polish.py b/tests/unit/api/test_chat_polish.py index 7d9838f9..076881ab 100644 --- a/tests/unit/api/test_chat_polish.py +++ b/tests/unit/api/test_chat_polish.py @@ -2,6 +2,7 @@ from __future__ import annotations +from types import SimpleNamespace from typing import Any from unittest.mock import AsyncMock, MagicMock @@ -10,8 +11,10 @@ from octop.api.routers.chat.serialize import ( _entry_matches_thread, + _is_offload_placeholder_block, _merge_adjacent_messages, _serialize_history_message, + _strip_image_only_text_blocks, _strip_thinking, _ts_to_ms, ) @@ -181,6 +184,191 @@ def test_ts_to_ms_converts_seconds() -> None: assert _ts_to_ms(1_700_000_000.5) == 1_700_000_000_500 +# --------------------------------------------------------------------------- +# Image-offload placeholder filtering on history serialization +# --------------------------------------------------------------------------- + + +_PLACEHOLDER_TEXT = ( + "[image offloaded: sha=06bdd82d592a " + "path=C:\\Users\\me\\.octop\\agents\\DMQ318\\.media-cache/" + "06bdd82d592a5cd6371ccdb2ed49d347a4ffb0fca4b80ea57e991f51522e178e.png " + "size=173004B mime=image/png; use read_file to retrieve bytes]" +) + + +def test_is_offload_placeholder_block_matches_image_format() -> None: + assert _is_offload_placeholder_block({"type": "text", "text": _PLACEHOLDER_TEXT}) + # Same shape, but a 12-char short sha and trailing "]". + assert _is_offload_placeholder_block( + { + "type": "text", + "text": " [audio offloaded: sha=abcdef012345 path=/x.y size=1B mime=audio/mpeg; use read_file to retrieve bytes] ", + } + ) + + +def test_is_offload_placeholder_block_rejects_unrelated_text() -> None: + assert not _is_offload_placeholder_block({"type": "text", "text": "hello"}) + assert not _is_offload_placeholder_block( + {"type": "text", "text": "[image] some other bracket text"} + ) + # Image block (not text) is not a placeholder. + assert not _is_offload_placeholder_block( + {"type": "image_url", "image_url": {"url": "data:..."}} + ) + assert not _is_offload_placeholder_block("not a dict") + + +def test_serialize_history_message_drops_image_offload_placeholder_for_image_user() -> None: + msg = HumanMessage( + content=[ + {"type": "text", "text": "用户发送了图片。"}, + {"type": "text", "text": _PLACEHOLDER_TEXT}, + ], + additional_kwargs={ + "octop_inbound_attachments": [ + { + "filename": "image.png", + "media_type": "image/png", + "kind": "image", + "workspace_path": "inbound/123_image.png", + } + ], + }, + ) + user = SimpleNamespace(locale="zh") + entry = _serialize_history_message(msg, user=user) + assert entry is not None + # Both the localized "User sent an image." sentinel and the offload + # placeholder must be stripped — only the image (rendered from + # inbound_attachments) is meaningful on the dashboard. + assert entry["content"] == [] + # Attachments are still propagated for the frontend to render the image. + assert entry["inbound_attachments"][0]["workspace_path"] == "inbound/123_image.png" + + +def test_serialize_history_message_keeps_user_caption_alongside_image_placeholder() -> None: + msg = HumanMessage( + content=[ + {"type": "text", "text": "请帮我看看这张图"}, + {"type": "text", "text": _PLACEHOLDER_TEXT}, + ], + additional_kwargs={ + "octop_inbound_attachments": [ + { + "filename": "image.png", + "media_type": "image/png", + "kind": "image", + "workspace_path": "inbound/123_image.png", + } + ], + }, + ) + user = SimpleNamespace(locale="zh") + entry = _serialize_history_message(msg, user=user) + assert entry is not None + # Only the offload placeholder is dropped; the user-written caption + # is preserved verbatim so the dashboard still shows it. + assert entry["content"] == [{"type": "text", "text": "请帮我看看这张图"}] + + +def test_serialize_history_message_keeps_placeholder_when_no_image_attachment() -> None: + """Without inbound_attachments, the placeholder is the only sign of media.""" + msg = HumanMessage( + content=[{"type": "text", "text": _PLACEHOLDER_TEXT}], + ) + # No additional_kwargs → no inbound_attachments → no filtering. + entry = _serialize_history_message(msg, user=SimpleNamespace(locale="en")) + assert entry is not None + assert entry["content"] == [{"type": "text", "text": _PLACEHOLDER_TEXT}] + + +def test_serialize_history_message_drops_placeholder_for_en_locale() -> None: + msg = HumanMessage( + content=[ + {"type": "text", "text": "User sent an image."}, + {"type": "text", "text": _PLACEHOLDER_TEXT}, + ], + additional_kwargs={ + "octop_inbound_attachments": [ + { + "filename": "image.png", + "media_type": "image/png", + "kind": "image", + "workspace_path": "inbound/123_image.png", + } + ], + }, + ) + entry = _serialize_history_message(msg, user=SimpleNamespace(locale="en")) + assert entry is not None + assert entry["content"] == [] + + +def test_strip_image_only_text_blocks_without_user_skips_zh_default() -> None: + """Locale falls back to ``zh`` when no user is supplied.""" + msg = HumanMessage( + content=[ + {"type": "text", "text": "用户发送了图片。"}, + {"type": "text", "text": _PLACEHOLDER_TEXT}, + ], + additional_kwargs={ + "octop_inbound_attachments": [ + { + "filename": "image.png", + "media_type": "image/png", + "kind": "image", + "workspace_path": "inbound/x.png", + } + ], + }, + ) + # Pass no user — caller signature is ``user=None`` default. + entry = _serialize_history_message(msg) + assert entry is not None + assert entry["content"] == [] + + +def test_strip_image_only_text_blocks_keeps_voice_caption() -> None: + """A non-image attachment must not trigger placeholder stripping.""" + msg = HumanMessage( + content=[ + {"type": "text", "text": "请听这段录音"}, + {"type": "text", "text": _PLACEHOLDER_TEXT}, # NOT real, but tests shape + ], + additional_kwargs={ + "octop_inbound_attachments": [ + { + "filename": "voice.m4a", + "media_type": "audio/mp4", + "kind": "file", + "workspace_path": "inbound/voice.m4a", + } + ], + }, + ) + entry = _serialize_history_message(msg, user=SimpleNamespace(locale="zh")) + assert entry is not None + # The audio file is rendered from inbound_attachments, not as a + # placeholder text block, so the on-disk placeholder is the only + # signal — leave it alone. + assert entry["content"] == [ + {"type": "text", "text": "请听这段录音"}, + {"type": "text", "text": _PLACEHOLDER_TEXT}, + ] + + +def test_strip_image_only_text_blocks_directly() -> None: + blocks = [ + {"type": "text", "text": "用户发送了图片。"}, + {"type": "text", "text": _PLACEHOLDER_TEXT}, + {"type": "text", "text": "你好"}, + ] + out = _strip_image_only_text_blocks(blocks, locale="zh") + assert out == [{"type": "text", "text": "你好"}] + + @pytest.mark.asyncio async def test_load_checkpoint_messages_falls_back_to_graph_state() -> None: from octop.api.routers.chat.serialize import _load_checkpoint_messages