Skip to content

refactor(gateway): salvage #29211 + persist platform_message_id for recall#29449

Merged
teknium1 merged 9 commits into
mainfrom
hermes/hermes-pr29211
May 20, 2026
Merged

refactor(gateway): salvage #29211 + persist platform_message_id for recall#29449
teknium1 merged 9 commits into
mainfrom
hermes/hermes-pr29211

Conversation

@teknium1

Copy link
Copy Markdown
Contributor

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_id column instead of falling back to content-match.

Changes (vs #29211)

  • All eight original commits from @yoniebans cherry-picked onto current main (authorship preserved).
  • New: platform_message_id column on messages table. Holds the external messaging platform's own message ID (yuanbao msg_id, telegram update_id, …) independent of the SQLite autoincrement primary key.
    • SCHEMA_VERSION bumped 11 → 12.
    • Column added declaratively via the existing _reconcile_columns() reconciler — no version-gated migration block needed for the column.
    • Partial index idx_messages_platform_msg_id on (session_id, platform_message_id) WHERE platform_message_id IS NOT NULL keeps recall's point-lookup cheap. Created after _reconcile_columns() runs so the index's WHERE clause 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 either message["platform_message_id"] (new explicit name) or message["message_id"] (yuanbao's existing convention on JSONL dicts).
    • replace_messages() reads the same dual key path so rewrite_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 as message_id — same shape platform code used to read from JSONL.
  • Yuanbao recall restored to its original three-branch structure:
    • Branch A1: exact message_id match — canonical path again
    • Branch A2: content-match fallback for rows without a platform id (agent-processed @bot messages, pre-column legacy rows)
    • Branch B: system-note when neither match
    • Both A-branches log which one fired so operators can see in gateway.log whether recall hit the canonical path or had to fall back.
  • The PR's existing TODO: add platform_message_id column comment is now resolved and removed.

Tests

  • New low-level round-trip tests in test_hermes_state.py:
    • test_platform_message_id_round_tripsappend_messageget_messages_as_conversation preserves the column.
    • test_replace_messages_preserves_platform_message_id — same for the rewrite path.
  • The PR's test_yuanbao_recall_db_only.py rewritten 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.
  • Three pre-existing change-detector tests (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

  • Only 3 production consumers of JSONL transcripts: gateway/session.py (PR rewrites), gateway/platforms/yuanbao.py (PR migrates), gateway/mirror.py (PR removes writer). All other gateway/run.py call sites go through SessionStore APIs.
  • On Teknium's box: 54 JSONL files, ~6.2GB sessions dir. State.db ≥ JSONL on every sampled session — the "longer source wins" fallback would never have triggered.

Validation

Result
tests/test_hermes_state.py + tests/gateway/test_session.py + recall tests + load_transcript test + retry/dm/mirror tests + 860_dedup 327 passed
tests/gateway/ + tests/hermes_state/ 5680 passed, 7 skipped
tests/run_agent/ + tests/agent/ + tests/cli/ 5312 passed, 3 skipped
E2E (live SessionStore) Schema v12 ✓ · column persists ✓ · load_transcript surfaces message_id ✓ · branch A1 exact-id match ✓ · rewrite_transcript preserves id ✓ · partial index exists ✓

Closes #29211. Credit to @yoniebans for the original refactor.

yoniebans and others added 9 commits May 20, 2026 12:49
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)
@teknium1 teknium1 merged commit 31a0100 into main May 20, 2026
17 of 18 checks passed
@teknium1 teknium1 deleted the hermes/hermes-pr29211 branch May 20, 2026 20:01
@github-actions

Copy link
Copy Markdown
Contributor

🔎 Lint report: hermes/hermes-pr29211 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 8982 on HEAD, 8972 on base (🆕 +10)

🆕 New issues (6):

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.

@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants