fix(state): honor boolean operators in trigram-off LIKE fallback + fail-closed drops - #114
Conversation
…il-closed drops Salvages the two genuine improvements from #111 onto the merged sessions.fts_trigram gate (#110), which shipped the same trigram-disable feature under a different config key. #111 is superseded and closed. 1. CJK/substring LIKE fallback now preserves query boolean structure: it walks tokens left-to-right honoring AND (intersection) and NOT (exclusion) instead of OR-joining every term. Once trigram is disabled this fallback is the only CJK path, so "A NOT B" no longer returns rows with B and "A AND B" no longer returns rows matching only A. COALESCE guards the AND NOT branch against NULL tool columns dropping every row. 2. _drop_trigram_schema now fails closed: the trigger + table drops run in one BEGIN IMMEDIATE transaction so a "database is locked" propagates instead of leaving a stale trigger pointing at a missing table. On a build missing the trigram tokenizer (DROP TABLE raises), the triggers are re-dropped in a standalone committed step so message INSERTs degrade to the base FTS/LIKE path instead of crashing on the unusable vtable. Regression tests: red-before/green-after for the boolean fallback and the locked-drop propagation; tokenizer-missing trigger cleanup.
|
@codex review |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the SQLite trigram schema teardown to execute transactionally, preventing partial drops during concurrent lock contention, and adds a fallback path to drop triggers standalone if the trigram tokenizer is missing. Additionally, the fallback LIKE search is refactored to preserve the query's boolean structure (AND, OR, NOT) and handle NULL columns correctly. The review feedback suggests propagating lock errors during the standalone trigger drop to avoid starting up in a state that crashes on subsequent writes, and correctly handling combined OR NOT operators to prevent them from being parsed as AND NOT.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fb266ad39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…upport OR NOT Addresses codex review on #114: (1) the tokenizer-missing trigger-drop recovery now propagates a lock instead of swallowing it (a swallowed lock left stale triggers crashing INSERTs); (2) the LIKE-fallback boolean parser now keeps the OR connector on 'A OR NOT B' instead of collapsing it to 'A AND NOT B'. Adds regressions for both.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
Salvages the two genuine improvements from #111 onto the already-merged
sessions.fts_trigramgate (#110). #100/#110/#113 merged in the sweep; #110 shipped the same "drop the trigram FTS index to shrink state.db" feature #111 proposed, but under the config keysessions.fts_trigraminstead of #111'ssessions.trigram_fts. Rather than land a second knob + duplicate implementation, #111 is closed as superseded and its two non-duplicate fixes are ported here against the merged key.Fixes
CJK/substring LIKE fallback honors boolean operators. When trigram is gated off, the LIKE fallback is the only CJK/substring search path. The old fallback OR-joined every non-operator token, so
A NOT Breturned rows containing B andA AND Breturned rows matching only A. It now walks tokens left-to-right honoring AND (intersection) and NOT (AND NOT, exclusion), withCOALESCE(col,'')so a NULL tool column under theAND NOTbranch yields FALSE instead of NULL (which would silently drop every row)._drop_trigram_schemafails closed. The trigger + table drops now run inside oneBEGIN IMMEDIATEtransaction, so adatabase is lockedduring startup contention (gateway + cron) propagates instead of being swallowed and leaving a stale trigram trigger pointing at a missing table. On a SQLite build missing the trigram tokenizer (theDROP TABLEraisesno such tokenizer: trigram), the triggers are re-dropped in a standalone committed step so message INSERTs degrade to the base FTS/LIKE path instead of crashing on the unusable vtable.Tests
Red-before/green-after regressions in
tests/test_hermes_state.py:test_trigram_off_like_fallback_honors_boolean_operators(AND/OR/NOT)test_drop_trigram_schema_propagates_locked_drop(lock fails closed)test_drop_trigram_schema_drops_triggers_when_tokenizer_missingFull
tests/test_hermes_state.pysuite: 352 passed. ruff clean.