diff --git a/apps/desktop/src/lib/chat-messages.test.ts b/apps/desktop/src/lib/chat-messages.test.ts index e86ba5593ecc..ea9d5df436db 100644 --- a/apps/desktop/src/lib/chat-messages.test.ts +++ b/apps/desktop/src/lib/chat-messages.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it } from 'vitest' +import type { SessionMessage } from '@/types/hermes' + import type { ChatMessage, ChatMessagePart } from './chat-messages' import { appendAssistantTextPart, @@ -191,6 +193,30 @@ describe('toChatMessages', () => { 'background agent work finished' ]) }) + + // A backend older than this app serves display_metadata as unparsed JSON + // text. Indexing into that string used to throw and fail the whole resume. + it.each([ + ['an object', { delegation_id: 'deleg_1', task_count: 2 }, '2 background agents finished'], + ['JSON text', JSON.stringify({ delegation_id: 'deleg_1', task_count: 1 }), '1 background agent finished'], + ['unparseable text', '{not-json', 'background agent work finished'], + ['text that is not an object', '"deleg_1"', 'background agent work finished'], + ['a missing task count', { delegation_id: 'deleg_1' }, 'background agent work finished'] + ])('labels a delegation event given %s', (_case, displayMetadata, expected) => { + const read = () => + toChatMessages([ + { + role: 'user', + content: 'opaque delegation context payload', + display_kind: 'async_delegation_complete', + display_metadata: displayMetadata as SessionMessage['display_metadata'], + timestamp: 1 + } + ]) + + expect(read).not.toThrow() + expect(chatMessageText(read()[0])).toBe(expected) + }) }) describe('renderMediaTags', () => { diff --git a/apps/desktop/src/lib/chat-messages.ts b/apps/desktop/src/lib/chat-messages.ts index 8541c0bb8098..0019c3d4b5ef 100644 --- a/apps/desktop/src/lib/chat-messages.ts +++ b/apps/desktop/src/lib/chat-messages.ts @@ -311,16 +311,35 @@ function transcriptContent(displayKind: SessionMessage['display_kind'], content: return displayKind === 'hidden' ? null : content } +// A remote backend older than this app serves display_metadata as raw JSON text, +// and `in` throws on a primitive — which used to fail the whole session resume. +function timelineTaskCount(metadata: SessionMessage['display_metadata']): number | undefined { + let parsed: unknown = metadata + + if (typeof parsed === 'string') { + try { + parsed = JSON.parse(parsed) + } catch { + return undefined + } + } + + if (!parsed || typeof parsed !== 'object') { + return undefined + } + + const count = (parsed as { task_count?: unknown }).task_count + + return typeof count === 'number' ? count : undefined +} + function timelineDisplayContent(message: SessionMessage, content: string): string { if (message.display_kind === 'model_switch') { return 'model changed' } if (message.display_kind === 'async_delegation_complete') { - const count = - message.display_metadata && 'task_count' in message.display_metadata - ? message.display_metadata.task_count - : undefined + const count = timelineTaskCount(message.display_metadata) return count === undefined ? 'background agent work finished' diff --git a/apps/desktop/src/types/hermes.ts b/apps/desktop/src/types/hermes.ts index d811e07ed57c..1d5f104f4cc8 100644 --- a/apps/desktop/src/types/hermes.ts +++ b/apps/desktop/src/types/hermes.ts @@ -494,7 +494,11 @@ export interface SessionMessage { reasoning_content?: null | string reasoning_details?: unknown display_kind?: 'async_delegation_complete' | 'hidden' | 'model_switch' | string - display_metadata?: TimelineDisplayMetadata + /** + * A backend older than this app can still serve this as unparsed JSON text, + * so readers must narrow before indexing into it. + */ + display_metadata?: string | TimelineDisplayMetadata role: 'assistant' | 'system' | 'tool' | 'user' text?: unknown timestamp?: number diff --git a/hermes_state.py b/hermes_state.py index 6ae4863b1843..4ba71b295566 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -5983,6 +5983,58 @@ class SessionDB: return content return content + @staticmethod + def _encode_display_metadata(display_metadata: Any) -> Optional[str]: + """Serialize ``display_metadata`` for its TEXT column without double-encoding. + + Import/replace paths can hand us an already-serialized JSON string (the + same hazard ``tool_calls`` guards against above). ``json.dumps`` on that + string would store a quoted JSON string, and the single ``json.loads`` + on read then yields a ``str`` instead of a dict. + """ + if not display_metadata: + return None + if isinstance(display_metadata, str): + try: + parsed = json.loads(display_metadata) + except (json.JSONDecodeError, TypeError): + logger.warning("Ignoring non-JSON display metadata on write") + return None + if not isinstance(parsed, dict): + logger.warning("Ignoring non-object display metadata on write") + return None + return json.dumps(parsed) + if isinstance(display_metadata, dict): + return json.dumps(display_metadata) + logger.warning( + "Ignoring unexpected display metadata type on write: %s", + type(display_metadata).__name__, + ) + return None + + @staticmethod + def _decode_display_metadata(raw: Any) -> Optional[Dict[str, Any]]: + """Decode a ``display_metadata`` column into the dict every reader expects. + + Every message read path must go through this. Returning the raw TEXT + instead reaches the desktop as a string, where ``'task_count' in meta`` + throws and fails the whole resume. Rows written before the encode guard + landed are double-encoded, so unwrap a second layer when we find one. + """ + if raw is None: + return None + try: + meta = json.loads(raw) if isinstance(raw, str) else raw + if isinstance(meta, str): + meta = json.loads(meta) + except (json.JSONDecodeError, TypeError): + logger.warning("Ignoring invalid display metadata on message row") + return None + if not isinstance(meta, dict): + logger.warning("Ignoring non-object display metadata on message row") + return None + return meta + def append_message( self, session_id: str, @@ -6029,7 +6081,7 @@ class SessionDB: """ # Display metadata is presentation-only and never changes the model # context role/content replayed to providers. - display_metadata_json = json.dumps(display_metadata) if display_metadata else None + display_metadata_json = self._encode_display_metadata(display_metadata) # Serialize structured fields to JSON before entering the write txn reasoning_details_json = ( json.dumps(reasoning_details) @@ -6168,7 +6220,7 @@ class SessionDB: "UPDATE messages SET display_kind = ?, display_metadata = ? WHERE id = ?", ( _scrub_surrogates(display_kind), - json.dumps(display_metadata) if display_metadata else None, + self._encode_display_metadata(display_metadata), row[0], ), ) @@ -6262,7 +6314,7 @@ class SessionDB: 1, _scrub_surrogates(api_content) if isinstance(api_content, str) else None, _scrub_surrogates(msg.get("display_kind")) if isinstance(msg.get("display_kind"), str) else None, - json.dumps(msg["display_metadata"]) if msg.get("display_metadata") else None, + self._encode_display_metadata(msg.get("display_metadata")), ), ) inserted += 1 @@ -6476,6 +6528,8 @@ class SessionDB: except (json.JSONDecodeError, TypeError): logger.warning("Failed to deserialize tool_calls in get_messages, falling back to []") msg["tool_calls"] = [] + if msg.get("display_metadata") is not None: + msg["display_metadata"] = self._decode_display_metadata(msg["display_metadata"]) result.append(msg) return result @@ -6545,6 +6599,8 @@ class SessionDB: "Failed to deserialize tool_calls in get_messages_around, falling back to []" ) msg["tool_calls"] = [] + if msg.get("display_metadata") is not None: + msg["display_metadata"] = self._decode_display_metadata(msg["display_metadata"]) result.append(msg) # before_rows includes the anchor itself; subtract 1 for the count of @@ -6667,6 +6723,8 @@ class SessionDB: "Failed to deserialize tool_calls in get_anchored_view, falling back to []" ) msg["tool_calls"] = [] + if msg.get("display_metadata") is not None: + msg["display_metadata"] = self._decode_display_metadata(msg["display_metadata"]) return msg return { @@ -6865,10 +6923,9 @@ class SessionDB: if row["display_kind"]: msg["display_kind"] = row["display_kind"] if row["display_metadata"]: - try: - msg["display_metadata"] = json.loads(row["display_metadata"]) - except (TypeError, json.JSONDecodeError): - logger.warning("Ignoring invalid display metadata on message row") + decoded = self._decode_display_metadata(row["display_metadata"]) + if decoded is not None: + msg["display_metadata"] = decoded if row["timestamp"]: msg["timestamp"] = row["timestamp"] if row["tool_call_id"]: diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index a0299df447b2..2aa5199612ab 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -7424,3 +7424,110 @@ class TestDisplayMetadataPersistence: assert len(switched) == 1 assert switched[0]["display_metadata"] == meta + +class TestDisplayMetadataReadPaths: + """Every message read path must hand back the decoded dict. + + Returning the raw column instead reaches the desktop as a string, where + ``'task_count' in meta`` throws and fails the whole session resume. + """ + + META = { + "delegation_id": "deleg_0d84d484", + "task_count": 1, + "completed_count": 1, + "failed_count": 0, + "duration_seconds": 193.55, + } + + @staticmethod + def _seed(db): + db.create_session("s1", source="desktop") + message_id = db.append_message( + "s1", "user", "event", + display_kind="async_delegation_complete", + display_metadata=TestDisplayMetadataReadPaths.META, + ) + return message_id, db.append_message("s1", "assistant", "anchor") + + @staticmethod + def _read(db, reader, message_id, anchor_id): + if reader == "get_messages": + return db.get_messages("s1")[0] + if reader == "get_messages_around": + return db.get_messages_around("s1", message_id, window=0)["window"][0] + if reader == "get_anchored_view": + view = db.get_anchored_view("s1", anchor_id, window=0, bookend=1) + return view["bookend_start"][0] + return db.get_messages_as_conversation("s1")[0] + + READERS = ("get_messages", "get_messages_around", "get_anchored_view", "conversation") + + @pytest.mark.parametrize("reader", READERS) + def test_every_reader_decodes_display_metadata(self, db, reader): + message_id, anchor_id = self._seed(db) + assert self._read(db, reader, message_id, anchor_id)["display_metadata"] == self.META + + @pytest.mark.parametrize("reader", READERS) + def test_every_reader_unwraps_historically_double_encoded_rows(self, db, reader): + """Rows written before the encode guard landed carry a second JSON layer.""" + message_id, anchor_id = self._seed(db) + + def _corrupt(conn): + conn.execute( + "UPDATE messages SET display_metadata = ? WHERE id = ?", + (json.dumps(json.dumps(self.META)), message_id), + ) + + db._execute_write(_corrupt) + assert self._read(db, reader, message_id, anchor_id)["display_metadata"] == self.META + + @pytest.mark.parametrize("reader", READERS) + @pytest.mark.parametrize("raw", ["", "{not-json", "[]", '"text"', "0"]) + def test_every_reader_drops_unusable_display_metadata(self, db, reader, raw): + """Bad presentation metadata must not take the message down with it.""" + message_id, anchor_id = self._seed(db) + + def _corrupt(conn): + conn.execute( + "UPDATE messages SET display_metadata = ? WHERE id = ?", + (raw, message_id), + ) + + db._execute_write(_corrupt) + message = self._read(db, reader, message_id, anchor_id) + assert message.get("display_metadata") is None + assert message["content"] == "event" + + def test_export_import_round_trip_keeps_metadata_decodable(self, db, tmp_path): + """The read leak used to write a permanently double-encoded row here. + + ``export_session`` reads through ``get_messages``, so an undecoded + string went back through ``_insert_message_rows`` and got re-dumped. + """ + self._seed(db) + blob = db.export_session("s1") + assert isinstance(blob["messages"][0]["display_metadata"], dict) + + target = SessionDB(db_path=tmp_path / "imported.db") + try: + target.import_sessions([json.loads(json.dumps(blob))]) + assert target.get_messages_as_conversation("s1")[0]["display_metadata"] == self.META + assert target.get_messages("s1")[0]["display_metadata"] == self.META + finally: + target.close() + + def test_write_paths_do_not_double_encode_serialized_metadata(self, db): + """Import/replace can hand us metadata that is already a JSON string.""" + db.create_session("s1", source="cli") + db.replace_messages( + "s1", + [{ + "role": "user", + "content": "event", + "display_kind": "async_delegation_complete", + "display_metadata": json.dumps(self.META), + }], + ) + assert db.get_messages_as_conversation("s1")[0]["display_metadata"] == self.META +