fix(state): sessions repair should detect incomplete FTS schema, not just unopenable DBs - #71933
Open
Kyzcreig wants to merge 1 commit into
Open
fix(state): sessions repair should detect incomplete FTS schema, not just unopenable DBs#71933Kyzcreig wants to merge 1 commit into
Kyzcreig wants to merge 1 commit into
Conversation
…hema as healthy ## Summary `hermes sessions repair --check-only` reports ``` ✓ /Users/…/.hermes/state.db opens cleanly — no repair needed. ``` on a database that **cannot record a single new message**. Every `INSERT INTO messages` fails with `no such table: messages_fts_trigram`, but the health probe returns "clean", so the one diagnostic a user is told to reach for actively points them away from the real fault. ## The bug `_db_opens_cleanly()` deliberately drives a rolled-back message write through the FTS triggers, precisely so that FTS damage which leaves reads and `integrity_check` passing is still caught (NousResearch#50502). That probe works. The problem is how its failure is classified: ```python msg = str(exc).lower() if "no such table" in msg or "no such column" in msg: return None # assumed: brand-new file mid-init ``` The assumption holds for a genuinely fresh file, which has no `sessions`/`messages` tables yet. But the **same error text** is produced by a structurally broken store: an FTS trigger that outlived the virtual table it writes to. That happens after an interrupted migration, a partially-applied schema change, or a manual `DROP` — and in that state the store is completely unable to accept writes. Both cases returned `None`, so a fully-populated database whose every message write fails was reported as healthy. The probe already validates **openability**; it did not validate that the FTS schema is internally consistent. That gap is what makes this failure class hard to diagnose: the user runs the recommended repair command, is told nothing is wrong, and has no next step. ## Reproduction on `main` Seed a normal store, drop `messages_fts_trigram` but leave its triggers, then ask the repair probe what it thinks: ``` [broken] trigram table present: False; surviving triggers: ['messages_fts_trigram_insert', 'messages_fts_trigram_delete', 'messages_fts_trigram_update'] [ground truth] INSERT INTO messages -> OperationalError: no such table: main.messages_fts_trigram [_db_opens_cleanly] -> None RED ✗ repair reports 'opens cleanly — no repair needed' …while every message write fails. False negative. ``` With this patch, unchanged harness: ``` [_db_opens_cleanly] -> 'incomplete FTS schema — message writes fail: no such table: main.messages_fts_trigram. An FTS trigger references a table that no longer exists (interrupted migration or partial schema change).' GREEN ✓ repair flags the DB as needing repair and names the cause ``` ## The fix Narrow the `no such table` / `no such column` branch so it only absolves a database that is *actually* mid-init. If the base tables exist, the store is populated and a failing write probe is a real, actionable fault: ```python try: base_tables = {r[0] for r in conn.execute( "SELECT name FROM sqlite_master WHERE type = 'table' " "AND name IN ('sessions', 'messages')").fetchall()} except sqlite3.DatabaseError: return None if {"sessions", "messages"} - base_tables: return None # genuinely mid-init return (f"incomplete FTS schema — message writes fail: {exc}. " "An FTS trigger references a table that no longer exists " "(interrupted migration or partial schema change).") ``` Two deliberate details: - **The `cjk_unicode61` check moved above this branch.** That error is a *capability* gap on the probing process, not damage, and must keep returning `None` regardless of whether the base tables exist. Reordering keeps it unconditional. - **The returned string names the missing table**, so the reason surfaced by `sessions repair` is actionable rather than a bare sqlite error. Once the check reports the fault, the existing repair pipeline handles it: Strategy 0 (`rebuild_fts`) re-creates the FTS schema from the canonical `messages` table, which is the correct, least-destructive recovery for this shape. ## Blast radius This makes the health probe **stricter**, so the risk is false positives on databases that are fine. Guarded by construction and by test: - The `sessions`/`messages` existence check means fresh and mid-init databases are unchanged. - A **healthy store with the trigram index cleanly removed** (table *and* triggers — the supported trigram-disabled shape) still reports clean, because nothing writes to a missing table. That is asserted explicitly, and it is the configuration that would otherwise be most at risk of a spurious flag. - The `cjk_unicode61` capability path is preserved. ## Tests `TestRepairDetectsIncompleteFtsSchema` in `tests/test_hermes_state.py`: - `test_repair_check_detects_trigger_without_its_fts_table` — establishes **ground truth** first (asserts the raw `INSERT INTO messages` genuinely fails), then asserts `_db_opens_cleanly()` returns a reason naming the missing table. Fails on `main`. - `test_repair_check_still_passes_a_healthy_store` — the false-positive guard. Asserts a normal store reports clean, *and* that the same store with the trigram index cleanly removed still reports clean. Passes both before and after, by design: it exists to constrain this change, not to demonstrate it. `tests/test_hermes_state.py` passes in full. `ruff` clean. ## Scope Detection only. This PR does not change what `repair` *does* once a fault is found — the existing `rebuild_fts` strategy already covers this shape. Companion PR: `fix(state): skip the trigram sweep in _fts_rebuild_finish when the trigram index is unavailable` fixes the `optimize-storage` crash that produces this schema state in the first place. Independent; either can land first.
Contributor
|
Thanks for isolating a real false negative in the write-health probe. Current main still returns Problems
Suggested changes
This is an automated hermes-sweeper review. |
19 tasks
This was referenced Aug 8, 2026
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.
fix(state):
sessions repairmust not report a write-broken FTS schema as healthySummary
hermes sessions repair --check-onlyreportson a database that cannot record a single new message. Every
INSERT INTO messagesfails withno such table: messages_fts_trigram, but the healthprobe returns "clean", so the one diagnostic a user is told to reach for actively points
them away from the real fault.
The bug
_db_opens_cleanly()deliberately drives a rolled-back message write through the FTStriggers, precisely so that FTS damage which leaves reads and
integrity_checkpassing isstill caught (#50502). That probe works. The problem is how its failure is classified:
The assumption holds for a genuinely fresh file, which has no
sessions/messagestablesyet. But the same error text is produced by a structurally broken store: an FTS
trigger that outlived the virtual table it writes to. That happens after an interrupted
migration, a partially-applied schema change, or a manual
DROP— and in that state thestore is completely unable to accept writes.
Both cases returned
None, so a fully-populated database whose every message write failswas reported as healthy.
The probe already validates openability; it did not validate that the FTS schema is
internally consistent. That gap is what makes this failure class hard to diagnose: the
user runs the recommended repair command, is told nothing is wrong, and has no next step.
Reproduction on
mainSeed a normal store, drop
messages_fts_trigrambut leave its triggers, then ask therepair probe what it thinks:
With this patch, unchanged harness:
The fix
Narrow the
no such table/no such columnbranch so it only absolves a database thatis actually mid-init. If the base tables exist, the store is populated and a failing
write probe is a real, actionable fault:
Two deliberate details:
cjk_unicode61check moved above this branch. That error is acapability gap on the probing process, not damage, and must keep returning
Noneregardless of whether the base tables exist. Reordering keeps it unconditional.
sessions repairis actionable rather than a bare sqlite error.Once the check reports the fault, the existing repair pipeline handles it: Strategy 0
(
rebuild_fts) re-creates the FTS schema from the canonicalmessagestable, which isthe correct, least-destructive recovery for this shape.
Blast radius
This makes the health probe stricter, so the risk is false positives on databases that
are fine. Guarded by construction and by test:
sessions/messagesexistence check means fresh and mid-init databases areunchanged.
supported trigram-disabled shape) still reports clean, because nothing writes to a
missing table. That is asserted explicitly, and it is the configuration that would
otherwise be most at risk of a spurious flag.
cjk_unicode61capability path is preserved.Tests
TestRepairDetectsIncompleteFtsSchemaintests/test_hermes_state.py:test_repair_check_detects_trigger_without_its_fts_table— establishes ground truthfirst (asserts the raw
INSERT INTO messagesgenuinely fails), then asserts_db_opens_cleanly()returns a reason naming the missing table. Fails onmain.test_repair_check_still_passes_a_healthy_store— the false-positive guard. Asserts anormal store reports clean, and that the same store with the trigram index cleanly
removed still reports clean. Passes both before and after, by design: it exists to
constrain this change, not to demonstrate it.
tests/test_hermes_state.pypasses in full.ruffclean.Scope
Detection only. This PR does not change what
repairdoes once a fault is found — theexisting
rebuild_ftsstrategy already covers this shape.Companion PR:
fix(state): skip the trigram sweep in _fts_rebuild_finish when the trigram index is unavailablefixes theoptimize-storagecrash that produces this schema statein the first place. Independent; either can land first.