fix(state): project multimodal text into FTS indexes - #69798
Conversation
7d2bcae to
c2a7e67
Compare
|
Rebased onto current main and resolved the session-state conflicts by retaining both the FTS text projection and upstream display metadata in the shared write paths. Validation: |
|
Rebased onto current main and resolved the session-state conflicts while retaining both the FTS text projection and upstream display-metadata handling. Validation: the new multimodal FTS tests pass (2), the FTS migration class passes with the one known SQLite held-reader timeout excluded (11 passed; the timeout reproduces on |
c2a7e67 to
72a41a1
Compare
|
Rebased onto current Also corrected 12 existing Validation: 8 affected state tests passed; scoped Ruff, the scoped Windows-footguns check, and |
72a41a1 to
73e90a7
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the raw multimodal payload through both substring indexes; current main still has that behavior in hermes_state_common.py:391-445 and hermes_state.py:1419-1475.
Problems
hermes_state.py:3254-3255callsexecutescript()inside_execute_write(). Currentmainexplicitly records that CJKexecutescript()calls implicitly commit and must run outside_execute_write'sBEGIN IMMEDIATEtransaction (hermes_state_search.py:350-357). The CJK projection upgrade must preserve that transaction boundary.- The new coverage exercises trigram only (
tests/test_hermes_state.py:6256+); it does not exercise the new CJK upgrade branch athermes_state.py:3241-3269.
Suggested changes
- Port the work to the split current-main modules and recreate the CJK schema outside
_execute_write, followinghermes_state_search.py:350-357. - Add a v1 CJK projection-upgrade regression test including text, an image payload, and an FTS integrity check.
Automated hermes-sweeper review.
| conn.execute(f"DROP TRIGGER IF EXISTS {trigger}") | ||
| conn.execute("DROP VIEW IF EXISTS messages_fts_cjk_src") | ||
| self._backfill_fts_content(conn) | ||
| conn.executescript(FTS_CJK_TABLE_SQL) |
There was a problem hiding this comment.
executescript() implicitly commits, so this cannot safely run inside _execute_write()'s BEGIN IMMEDIATE transaction. Current main documents this exact constraint in the CJK stale-reset path. Recreate the CJK schema outside _execute_write (or use transaction-safe individual statements) and add coverage for this projection-upgrade path.
|
Thanks for the feedback — I looked into this, but it can't be addressed on this PR right now:
|
SummaryOne PR addresses Issue #69672. #69798 adds a text-only multimodal projection across FTS write, view, trigger, and rebuild paths, removing the NUL-prefixed JSON and image payloads that cause SQLite-version-dependent integrity failures and index bloat, but its CJK upgrade path conflicts with the transaction boundary documented on current main. Related pull requests
Suggested consolidationAuthor action: rebase #69798 onto main, or split out the part that can merge. Consistent with the keep_open review on #69798, retain the text-projection approach, but first preserve the CJK transaction boundary described at Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I69672(["issue #69672 (open)"])
P69798["PR #69798 (open)"]
P69798 -->|best fix| I69672
class I69672 open
class P69798 open
class P69798 best
class P69798 target
click I69672 "https://github.com/NousResearch/hermes-agent/issues/69672"
click P69798 "https://github.com/NousResearch/hermes-agent/pull/69798"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 43 kB of PR diffs, 9 kB of issue/PR text, 4 kB of discussion (6 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
The triage summary matches the outstanding review feedback. This repair still cannot be made on this PR: the current-main implementation requires changes to |
|
Thanks for the production measurements — they confirm that the projection must apply to sentinel-prefixed multimodal rows regardless of role, including user-attached images; the existing This still cannot be done on this PR because current |
|
Thanks for the Windows runtime comparison. The same database producing different integrity results under SQLite 3.45.1 and 3.53.1, alongside the NUL-prefixed multimodal rows and reproducible rebuild flip, is strong cross-platform evidence for an FTS tokenization incompatibility rather than proven physical database damage. It also reinforces the need to keep the sentinel and image payloads out of the FTS projection. This still cannot be done on this PR: after the current-main split, the outstanding repair requires changes to |
The trigram and CJK-bigram indexes stored raw sentinel-prefixed multimodal JSON (including base64 image payloads) verbatim. SQLite's trigram tokenizer handles embedded NUL differently across versions, so an index written by one SQLite build reads as a malformed inverted index under another, and image payloads bloated the index with unsearchable bytes. Adds a SessionDB-maintained fts_content projection (text parts only, image_url/base64 dropped) and reads it through the trigram/CJK content views and every trigger/backfill/rebuild path, instead of raw content. FTS_STORAGE_VERSION 1 -> 2; `hermes sessions optimize-storage` rebuilds an older index onto the projection. Completes NousResearch#69798, ported onto the hermes_state_common/_search/_schema split it predates, and fixes the two review blockers: CJK schema recreation now runs outside _execute_write()'s transaction (executescript implicitly commits), and adds the requested CJK-projection-upgrade regression test. Fixes NousResearch#69672
|
Thanks for the feedback — I looked into this, but it can't be addressed on this PR right now:
|
What does this PR do?
Projects sentinel-prefixed multimodal message content into an FTS-only text field, so trigram and CJK indexes include text parts but exclude image URLs and base64 payloads. This removes the embedded NUL that makes trigram integrity checks SQLite-version-dependent, while preserving the original stored content for replay. Existing external-content indexes are upgraded deliberately by
hermes sessions optimize-storage.Related Issue
Fixes #69672
Type of Change
Changes Made
hermes_state.py: add a SessionDB-maintained multimodal text projection, use it in trigram/CJK FTS views, triggers, and rebuild paths, and version the opt-in FTS upgrade.hermes_cli/main.py: advertise the projection rebuild and describe the updated optimize-storage behavior.tests/test_hermes_state.py: cover text-only multimodal indexing, image-payload exclusion, FTS integrity, and rebuilding the prior external-content layout.How to Test
/opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' sh./opt/homebrew/bin/timeout -k 30 480 $VIRTUAL_ENV/bin/python -m pytest tests/test_hermes_state.py tests/test_fts_cjk_bigram.py -q -x --timeout=60.image_urlmultimodal message; confirm the text matchesmessages_fts_trigrambut the base64 token does not, then run the FTSintegrity-check.hermes sessions optimize-storageand verifyPRAGMA quick_checksucceeds.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A — no skill is added.
Screenshots / Logs
N/A — database/index behavior only.