refactor(gateway): salvage #29211 + persist platform_message_id for recall#29449
Merged
Conversation
state.db is canonical. The 'use whichever source is longer' branch was defensive code for the pre-DB migration; on every real DB it has not fired (verified on a session corpus with 27 jsonl files / 950 sessions — zero jsonl-bigger cases). Test changes: - TestLoadTranscriptCorruptLines: deleted (tested dead JSONL code path) - TestLoadTranscriptPreferLongerSource: deleted (tested removed fallback) - Replaced with TestLoadTranscriptDBOnly (DB-only reads) - TestSessionStoreRewriteTranscript: fixture now creates DB session - test_gateway_retry_replaces_last_user_turn: fixture uses real DB
Yuanbao's recall feature was reading the gateway JSONL directly to look up messages by platform message_id, which state.db does not preserve. Migrated to use load_transcript() which returns DB messages. Recall branch A1 (message_id match) now falls through to A2 (content match) or B (system note) for all sessions — a documented degradation. Follow-up issue: add platform_message_id column to state.db messages to restore exact-id matching.
…te_transcript state.db is canonical. JSONL transcripts were a transition fallback; the fallback was removed in the previous commit. Existing *.jsonl files on disk are left untouched.
Mirror messages are persisted via _append_to_sqlite. JSONL writer was a redundant dual-write. Updated test assertions from JSONL file checks to SQLite mock verification.
…db writes Fixtures that instantiate SessionStore() trigger SessionDB() with no args, which resolves to ~/.hermes/state.db via the DEFAULT_DB_PATH module constant (snapshot of get_hermes_home() at hermes_state import time). The autouse _hermetic_environment fixture in tests/conftest.py monkeypatches HERMES_HOME env, but DEFAULT_DB_PATH is already cached by then. Per-test monkeypatch.setattr(hermes_state, 'DEFAULT_DB_PATH', tmp_path/'state.db') forces the DB into tmp_path so the tests can't leak into the real profile. Verified by counting u1-prefixed sessions in real state.db before/after: delta=0.
…fixture PR #29211 review findings: 1. test_retry_replacement: pin DEFAULT_DB_PATH so SessionDB() doesn't write to the real ~/.hermes/state.db. Same fix as the other DB-only fixtures. 2. yuanbao recall branch A1 (message_id exact match) was structurally dead once load_transcript() became DB-only — state.db never preserves the platform message_id. Removed the dead loop, consolidated to a single content-match branch (renamed 'A: content match'). Branch B (system note) unchanged. Updated the test name + docstring to reflect this. Note: self._lock is no longer taken in append_to_transcript (was guarding the JSONL file append). SQLite append_message handles its own concurrency via WAL mode, so this is safe; flagging for awareness.
… recall PR #29211 dropped JSONL gateway transcripts and noted that the platform's own `message_id` field (used by Yuanbao's recall guard to redact a message by exact platform id) was no longer preserved — falling back to content-match. That fallback works for the common case but redacts the wrong row when two messages share text (or fails to match when content is post-processed). Restore exact-id matching by giving state.db a column for it: - New `platform_message_id TEXT` column on the messages table (SCHEMA_VERSION bump 11 → 12; column added via declarative reconciler on existing DBs, no version-gated migration block needed) - Partial index `idx_messages_platform_msg_id` on (session_id, platform_message_id) to keep recall's point-lookup cheap even on large sessions - `append_message()` and `replace_messages()` accept the new value: the gateway-facing `append_to_transcript` in `gateway/session.py` forwards either `message["platform_message_id"]` or the legacy `message["message_id"]` key (yuanbao's existing convention) - `get_messages_as_conversation()` surfaces the column back on the message dict as `message_id` so platform code reads the same shape it used to read from JSONL - Yuanbao `_patch_transcript`: restore branch A1 (exact id match) ahead of A2 (content match) ahead of B (system-note). Both branches log which one fired so operators can tell from gateway.log whether recall hit the canonical path or had to fall back. Tests: - New low-level round-trip tests in `test_hermes_state.py` for both `append_message` and `replace_messages` paths - The PR's `test_yuanbao_recall_db_only.py` was rewritten to assert the new contract: branch A1 (id match) works against DB-only transcripts, and branch A2 (content match) still recovers rows that were observed without a platform id (e.g. agent-processed @bot messages where run.py doesn't carry msg_id through)
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-attribute |
5 |
unresolved-import |
1 |
First entries
tests/gateway/test_session.py:567: [unresolved-attribute] unresolved-attribute: Attribute `append_message` is not defined on `None` in union `None | SessionDB`
tests/gateway/test_load_transcript_db_only.py:4: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_load_transcript_db_only.py:25: [unresolved-attribute] unresolved-attribute: Attribute `create_session` is not defined on `None` in union `None | SessionDB`
tests/gateway/test_session.py:565: [unresolved-attribute] unresolved-attribute: Attribute `create_session` is not defined on `None` in union `None | SessionDB`
tests/gateway/test_retry_replacement.py:24: [unresolved-attribute] unresolved-attribute: Attribute `create_session` is not defined on `None` in union `None | SessionDB`
tests/gateway/platforms/test_yuanbao_recall_db_only.py:70: [unresolved-attribute] unresolved-attribute: Attribute `create_session` is not defined on `None` in union `None | SessionDB`
✅ Fixed issues: none
Unchanged: 4734 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Salvage of #29211. SQLite is the sole gateway message store; JSONL transcripts are gone. Yuanbao's exact-id recall path is restored by giving state.db a
platform_message_idcolumn instead of falling back to content-match.Changes (vs #29211)
platform_message_idcolumn onmessagestable. Holds the external messaging platform's own message ID (yuanbao msg_id, telegram update_id, …) independent of the SQLite autoincrement primary key.SCHEMA_VERSIONbumped 11 → 12._reconcile_columns()reconciler — no version-gated migration block needed for the column.idx_messages_platform_msg_idon(session_id, platform_message_id) WHERE platform_message_id IS NOT NULLkeeps recall's point-lookup cheap. Created after_reconcile_columns()runs so the index'sWHEREclause doesn't reference a column that doesn't exist yet on legacy DBs.append_message()+replace_messages()accept the new value.gateway/session.py::append_to_transcript()forwards eithermessage["platform_message_id"](new explicit name) ormessage["message_id"](yuanbao's existing convention on JSONL dicts).replace_messages()reads the same dual key path sorewrite_transcript()(used by/retry,/undo,/compress, and yuanbao's own recall rewrite) round-trips the value correctly.get_messages_as_conversation()surfaces the column back on the message dict asmessage_id— same shape platform code used to read from JSONL.message_idmatch — canonical path againTODO: add platform_message_id columncomment is now resolved and removed.Tests
test_hermes_state.py:test_platform_message_id_round_trips—append_message→get_messages_as_conversationpreserves the column.test_replace_messages_preserves_platform_message_id— same for the rewrite path.test_yuanbao_recall_db_only.pyrewritten to assert the new contract:test_recall_branch_a1_exact_id_match_round_trips_through_db— A1 hits via DB-only transcripts.test_recall_branch_a2_content_match_when_no_platform_id— A2 still recovers rows that lack a platform id.assert version == 11) replaced with relationship assertions (assert version == SCHEMA_VERSION) per AGENTS.md guidance — these were going to break on every schema bump regardless.Verified premise of #29211
gateway/session.py(PR rewrites),gateway/platforms/yuanbao.py(PR migrates),gateway/mirror.py(PR removes writer). All othergateway/run.pycall sites go throughSessionStoreAPIs.Validation
tests/test_hermes_state.py+tests/gateway/test_session.py+ recall tests + load_transcript test + retry/dm/mirror tests + 860_deduptests/gateway/+tests/hermes_state/tests/run_agent/+tests/agent/+tests/cli/load_transcriptsurfacesmessage_id✓ · branch A1 exact-id match ✓ ·rewrite_transcriptpreserves id ✓ · partial index exists ✓Closes #29211. Credit to @yoniebans for the original refactor.