fix(knowledge): restore CJK recall on the FTS keyword leg

[chenmingwei23]

What is the problem?

Knowledge search loses the entire keyword leg for ordinary CJK queries. A
four-character Chinese query for "memory leak" returns zero FTS hits against
documents that plainly contain it.

The issue body names three whitespace-splitting sites and proposes a query-side
fix. That framing is only half the mechanism, and the proposed fix does not work.
Measured on main:

indexed document	terms unicode61 actually stores
a spaceless CJK sentence	['<the entire sentence as ONE token>']
the same words with a space between	['<word1>', '<word2>']

unicode61 classifies CJK ideographs as letters, so a whole spaceless run
becomes a single term. The query side then split on whitespace, which makes a
spaceless query a single token too. Both sides were wrong in the same
direction, so they agreed -- and a query only matched when it equalled an
entire indexed run byte for byte.

That agreement is why nobody saw it: the vector leg handles CJK, so RRF fusion
rescued the result set and the pure-keyword loss stayed invisible. On any
deployment with no embedding model configured, there is no vector leg to hide
behind and CJK keyword search is simply empty.

Why this issue matters to the user

CJK prose is written without spaces between words, so this is not an edge case
in those languages -- it is every query. A user searching their own knowledge
library in Chinese or Japanese gets a silently short result list: no error, no
log line, nothing to indicate the keyword half of a hybrid search contributed
nothing. Two characters is an ordinary word length in these scripts, and a
two-character query was equally dead.

How our fix solves it

Symptom: a CJK query returns no keyword hits. Root cause: the index stores one
term per spaceless run, so no sub-word term exists to match. Therefore the fix
has to be index-side -- and I verified that before choosing it, rather than
taking the issue thread's word for it.

Query-side expansion alone was measured and rejected. Expanding the query
into bigrams against the unchanged index recovers only the case where the
document has spaces between the CJK words -- exactly the toy shape in the
issue's reproduction step, and not how CJK is written. Against real spaceless
prose it still returns nothing, because the bigrams have no counterpart in the
index.

The trigram tokenizer was measured and rejected on two counts. A
two-character query returns nothing at all (trigram needs three characters), and
it switches on substring matching for every language -- with trigram, oke
starts matching tokens. Both are regressions in exchange for the fix.

What ships instead. The copy of title/content/tags handed to the index
is written with a boundary around each CJK character, so the tokenizer emits one
term per character. A query run expands to its overlapping adjacent-character
pairs, each as an FTS5 phrase, OR-ed together. A phrase over per-character
terms is exact substring matching, which buys recall without buying noise:

• a document spelling the run verbatim matches
• a document spelling the two words apart matches
• a document that merely reuses the same four characters inside other words
  ("internal", "to save", "relief valve", "water leak") does not match

That is deliberately the same adjacency floor parse_search_query already
applies to session search (#3681), so the two surfaces now agree on what a CJK
hit means. The helpers live in _sqlite_compat.py, the module that already
declares itself the single FTS5 dialect and is the one module both knowledge and
memory import.

Three consequences worth calling out, each pinned by a test:

1. Non-CJK behaviour is bit-for-bit unchanged. fts5_cjk_match_groups
   returns exactly what fts5_quote_tokens returns for any input with no CJK,
   and fts5_segment_for_index returns non-CJK text unmodified. fts5_quote_tokens
   itself is untouched, so memory search is not perturbed.
2. Only the index copy is transformed. items_fts is an external-content
   table, so snippet()/highlight() and every read of the item still return
   the original text.
3. All five FTS writers now funnel through _fts_index / _fts_unindex.
   This is the subtle one. FTS5's 'delete' command subtracts the terms it is
   handed, so un-indexing with raw text against a segmented index leaves the
   original terms in place -- the row keeps serving deleted or superseded
   content as a live hit. I confirmed 'integrity-check' does not report
   this, so it would have been a silent stale-result bug with nothing
   downstream to catch it. Centralising the two paths makes the segmentation
   impossible to omit at a call site.

Migration. The term representation is versioned by PRAGMA user_version
(previously unused in this database) rather than a schema probe, because the
CREATE VIRTUAL TABLE statement is byte-identical before and after -- nothing
in the schema records which representation a database holds. _migrate_fts_index
rebuilds from items in batches and bumps the marker only after the whole
rebuild commits, so an interrupted rebuild restarts on the next open instead of
resting half-built.

The rebuild is triggered by readers, not by the constructor (GPT review
round 1, fixed). KnowledgeStore.__init__ runs on the event-loop thread --
setup_knowledge_routes reads the lazy state.knowledge_store property during
dashboard startup -- while its FTS readers run on worker threads via
run_in_embed_pool / asyncio.to_thread. That split is the store's own
documented threading contract and the reason it hands each thread a separate
connection. Migrating in __init__ therefore put a data-scaled reindex on the
gateway boot path: measured at 4,000 CJK items it is ~137 ms, and it grows
linearly, so a large legacy library would visibly stall startup.
ensure_fts_index_current moves that cost to the first search instead. It is
lock-guarded, so concurrent readers wait rather than each starting a rebuild,
and steady state is a single boolean check (~1 us). Measured after the change:
construction of that same 4,000-item legacy database is 1.3 ms and leaves
user_version at 0; the first search absorbs the 137 ms.

Pattern harvest

Rule candidate: semgrep
Pattern: direct write to an FTS5 shadow table outside its owning wrapper

A transformation applied on a search index's write path has to be applied
identically on its read path and on its un-index path. This defect was
the read/write halves disagreeing; the sharpest hazard found while fixing it was
the un-index half disagreeing, which FTS5 reports nowhere -- 'integrity-check'
passes over a mismatched 'delete'. The generalizable guard is structural
rather than semantic: once a table's terms are derived rather than literal, every
writer must go through the one function that derives them. A rule that flags
INSERT INTO <name>_fts appearing anywhere outside that table's owning
wrapper methods would have caught the omission at any of the five call sites,
and would keep catching it as new writers are added.

Secondary, not proposed as a rule because it needs judgement rather than a
pattern: a migration added to a constructor is only safe if the constructor is
off the event loop, and in this codebase it is not. The existing
no-blocking-call-on-event-loop anchor already covers it -- it is what GPT
matched on -- so no new rule is warranted.

What tests we did

New: TestCjkKeywordRecall (17 tests) and TestCjkFts5Primitives (7 tests) in
test/test_knowledge.py. Vocabulary is shared with TestCjkSearch in
test_history.py so both search surfaces are read against the same examples.

Red on base. With the tests present and src/ reverted to origin/main,
18 of 21 fail (measured on the first revision; the 3 that pass assert
unchanged behaviour -- ASCII parity, non-CJK segmentation, and the tags
limitation below -- which is what they are for).

Coverage: spaceless query recall; scattered-character exclusion; two-character
words; title matching; the retriever leg with no embedder (so only keyword can
answer); mixed Latin+CJK tokens; no stale hit after update_item; no stale hit
after delete_item; ASCII behaviour unchanged including the oke/tokens
non-match that rules out trigram; stored text not segmented; the graph leg
finding an entity named inside a run; legacy-index rebuild; a rebuild spanning
more than one batch; construction not rebuilding (the event-loop fix); one
rebuild across four concurrent reader threads; and the retriever leg migrating a
legacy index.

Suites run (targeted, -n 2): test_knowledge.py,
test_knowledge_search_upgrade.py, test_mcp_knowledge_search.py,
test_knowledge_dedup.py, test_knowledge_handlers_coverage.py,
test_knowledge_cross_thread.py, test_knowledge_agent_source.py,
test_knowledge_delete_off_loop.py, test_memory_markdown_read.py (the
existing fts5_quote_tokens contract) -- 619 passed, 0 failed. The full
suite was not run.

Gates: flake8, isort --check-only src/kiro_crew test, mypy on the three
changed modules, ./scripts/docs-lint.sh, git diff --check -- all clean.

Write-path cost measured, since segmentation now runs on every FTS write:
105 KB of pure ASCII costs 0.71 ms and returns byte-identical text (one C-level
regex pass, no rebuild); a 54k-character CJK document costs 9.2 ms and grows the
index copy 3x. The regex character class is derived from the same range tuple
is_cjk_char uses, so the two cannot drift apart.

Any other suggestions on the work

Four things I found and deliberately did not fold in, each a separate root
cause with its own blast radius:

1. CJK tags are unsearchable for an unrelated reason. add_item persists
   tags via json.dumps at its default ensure_ascii=True, so a CJK tag is
   stored backslash-escaped and reaches the index as terms like u6a21. No
   query-side change can reach it -- the CJK never arrives in the column. Fixing
   it means changing a stored data format for existing rows. Pinned as an
   explicit known-limitation test (test_cjk_tags_are_stored_ascii_escaped) so
   the boundary is visible and a future fix must update it deliberately.
2. memory_fts has the identical latent gap (tokenize='porter unicode61',
   matching through fts5_quote_tokens). The helpers were put in the shared
   module specifically so that surface can adopt them, but it needs its own
   index rebuild and its own decision about AND semantics. Documented in
   memory-skills-hooks.md rather than silently left as a surprise.
3. Four divergent CJK range definitions now exist in the tree:
   history_search._is_cjk_char, vector_memory._DENSE_SCRIPT_RANGES (which
   includes Hangul), chat_title._UNSPACED_SCRIPT_RANGES (which includes Thai
   and excludes Hangul), and the new one here. I mirrored the session-search set
   deliberately, since this is a search surface, and left the others alone --
   consolidating them would change session-search and chat-title behaviour,
   which does not belong in a knowledge-recall fix.
4. One len(w) > 2 CJK-dropping filter survives at vector_memory.py:2966
   (lesson-dedup keyword extraction). fix(memory): keep two-character CJK words in the episodic keyword fallback #6460 fixed the episodic-search instance
   of this family; this one is in a different helper and out of scope here.

Closes #3691

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of e540e8f094f6fa01205fcab64cd5942bb0b2f05a — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause index-side fix with measured rejection of alternatives, disclosed one-way migration on rebuildable derived data, and gaps scoped out explicitly rather than silently.

[DESIGN-REVIEWED] e540e8f

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e540e8f094f6fa01205fcab64cd5942bb0b2f05a and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/_sqlite_compat.py:159 -- "parts.append(...chunk...)" ANDs punctuation-only runs, so 内存泄漏？ returns zero FTS hits -> Fix: skip punctuation-only non-CJK chunks.
[GPT-REVIEWED] e540e8f

False positive or not applicable? A repository writer can comment:
/ai-review override gpt e540e8f094f6fa01205fcab64cd5942bb0b2f05a: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of e540e8f094f6fa01205fcab64cd5942bb0b2f05a — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All counts verified. Producing the review.

First-Principles-Verdict: PASS

A reported recall-zero defect fixed at its root — the index representation — with every rider derived from the same issue and every count checked.

What this change ships

Intent: make a knowledge search typed in Chinese/Japanese actually return keyword matches. FIX.

1. Spaceless CJK queries now return keyword hits — justified (reported defect, index-side cause named and measured)
2. Two-character CJK words match — justified (same cause)
3. Deleting/updating a CJK item no longer leaves stale hits — justified (FTS5 'delete' semantics; five writers funneled)
4. Legacy databases rebuild on first search, not at boot — justified (measured 137 ms data-scaled boot stall avoided)
5. Entity-items endpoint moved off the event loop — rides along, derived from item 4's data-scaled first read
6. CJK entity names now find their items on that endpoint — justified (same root cause, third counted reader)
7. Graph leg finds entities named inside a run (_cjk_subruns) — rides along, derived (issue Knowledge search whitespace-splits CJK queries (FTS keyword leg loses recall) #3691's second enumerated site)
8. All store transactions become BEGIN IMMEDIATE — justified (serializes the representation read; every changed txn is an FTS writer)
9. Session search now imports the shared CJK primitives — justified subtraction (two drifted hand copies deleted)
10. memory_fts / preferences_fts stay unfixed — declared, deferred with reasons

Watch

Sibling count confirmed: 3 FTS5 product tables (grepped CREATE VIRTUAL TABLE.*fts5 + preferences_fts), 1 fixed. The deferral is declared in memory-skills-hooks.md with per-surface reasons, so this is accepted-and-deferred — but it leaves the shared helpers at 2 consumers each until a second surface adopts them.

[FIRST-PRINCIPLES-REVIEWED] e540e8f

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed e540e8f094f6fa01205fcab64cd5942bb0b2f05a — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] e540e8f

Verdict parsed from the review's SHA-scoped output markers for commit e540e8f094f6fa01205fcab64cd5942bb0b2f05a.

False positive or not applicable? A repository writer can comment:
/ai-review override fable e540e8f094f6fa01205fcab64cd5942bb0b2f05a: <one-sentence reason>

[chenmingwei23]

Review round 1 disposition (head 95d67818a)

GPT 5.6 -- BLOCKING: FTS migration blocks the gateway event loop -- FIXED

The finding is correct and I confirmed the chain rather than taking it on the
anchor alone:

• setup_knowledge_routes reads app["state"].knowledge_store (handlers/knowledge.py:1975)
• that is the lazy property at dashboard/state.py:5379, which constructs KnowledgeStore synchronously
• and KnowledgeStore.__init__'s own comment states the contract: "callers like HybridRetriever.search() run on worker threads via run_in_embed_pool / asyncio.to_thread while the store is created on the event-loop thread"

So the reindex was on the boot path, on the loop. Measured on a 4,000-item CJK
corpus: 137 ms, growing linearly with the corpus, so a large legacy library
would visibly stall startup.

I did not apply the prescribed fix as written ("Remove the migration-on-construction
call"), because removing it outright leaves every pre-existing knowledge base
permanently un-migrated -- the CJK recall this PR exists to restore would never
reach any database that already has content, which is all of them. Instead the
migration moved to the place that is already off the loop:

• ensure_fts_index_current() is called by the two FTS readers (KnowledgeStore.search_items_fts, HybridRetriever._keyword_search), which by the contract above always run on a worker thread
• lock-guarded, so four concurrent readers produce exactly one rebuild (pinned by test_rebuild_runs_once_across_concurrent_readers)
• steady state is one boolean check

Measured after the change, same 4,000-item legacy database:

	before	after
construction (boot path, on the loop)	~137 ms	1.3 ms
user_version after construction	migrated	0 (not migrated)
first search (worker thread)	-	137 ms
subsequent readers	-	~1.3 us

Three new tests pin it: test_construction_does_not_rebuild_the_index,
test_rebuild_runs_once_across_concurrent_readers,
test_retriever_leg_migrates_a_legacy_index.

PR Hygiene -- FAILURE: missing ## Pattern harvest -- FIXED

Added to the PR body. Rule candidate is a semgrep pattern for direct writes to an
FTS5 shadow table outside its owning wrapper: the un-index half of a derived-term
index disagreeing with the write half is invisible to FTS5 ('integrity-check'
passes over a mismatched 'delete'), so the guard has to be structural. The
constructor-migration half is already covered by the existing
no-blocking-call-on-event-loop anchor, so I did not propose a second rule for it.

Verification on this head

619 tests pass across nine targeted suites; flake8, isort, mypy,
./scripts/docs-lint.sh and git diff --check all clean. Specs moved with the
code (knowledge.md, memory-skills-hooks.md). Still one commit.

[chenmingwei23]

Review round 2 disposition (head 74225f5ac)

GPT 5.6 -- BLOCKING: legacy CJK writes corrupt the FTS operation before migration -- FIXED

Correct, and self-introduced by my round-1 fix: making the migration
reader-triggered opened a window in which a writer can reach a not-yet-migrated
index. Before round 1 the constructor migrated first, so the window did not
exist. I reproduced it rather than reasoning about it -- a segmented 'delete'
against a raw v0 index:

attempting segmented 'delete' against raw terms...
  the delete itself RAISED: DatabaseError: database disk image is malformed

So this is worse than a stale hit -- it aborts the operation, which on the
watcher sweep means ingestion dies.

GPT's prescribed fix is the right one and I took it: FTS writes now use the
representation user_version declares (_fts_terms_segmented ->
_fts_terms), with the rebuild flipping the declaration transactionally.

Two things worth recording, because they rule out the simpler alternatives:

1. "Just have writers trigger the migration too" does not work. _migrate's
   own orphan-source reclaim deletes items -- and therefore writes to FTS --
   from inside __init__, which is the event-loop thread and is exactly what
   round 1 established cannot afford a data-scaled rebuild. store.update_item,
   store.delete_item and import_bundle are also called directly in async
   handlers with no to_thread, so a writer-triggered rebuild would put the
   round-1 stall back on the write path.
2. The declaration is latched only in the True direction. user_version
   only increases, so a True answer is free to cache and the steady state costs
   nothing. A False answer is deliberately not cached: an MCP tool server is a
   second process on the same knowledge.db, and a cached False would have this
   process keep writing raw terms into an index that process migrated. The
   rebuild also sets the declaration before re-inserting, since
   PRAGMA user_version is still the old value until its transaction commits --
   flipping it afterwards would silently re-insert raw terms and bump the marker,
   leaving CJK permanently unsearchable with nothing to retry it. That one is
   pinned by test_migration_declares_segmented_before_reinserting.

Three new tests, and the fix is mutation-verified -- forcing unconditional
segmentation reproduces the crash, as shipped it passes:

  MUTATED (always segment)   update_item RAISED: DatabaseError: database disk image is malformed
  as shipped                 update_item: OK

Backend Lint & Type Check (3.10) -- FAILURE: black -- FIXED

Step Check formatting (black, baselined): not black-formatted. Run: black --target-version py310 src/kiro_crew/_sqlite_compat.py. _sqlite_compat.py is
not in .github/black-baseline.txt (unlike knowledge/store.py and
knowledge/retrieval.py, which are grandfathered there), so it has to be
black-clean. Formatted; the change is one joined line. black --check now
passes and flake8/isort/mypy are unaffected.

Verification on this head

626 tests pass across ten targeted suites. black --check (on the
non-baselined file), flake8, isort --check-only src/kiro_crew test, mypy,
./scripts/docs-lint.sh, git diff --check all clean; zero literal CJK. Specs
moved with the code. Still one commit.

[chenmingwei23]

Review round 3 disposition (head 955792ac0)

GPT 5.6 -- BLOCKING: lazy migration inverts the writer lock order -- FIXED

Correct, serious, and again self-introduced -- by the round-2 fix this time,
which put with self._fts_lock: inside _fts_index / _fts_unindex. The
inversion is exactly as described:

• the rebuilding reader holds _fts_lock, then wants SQLite's writer lock
• an event-loop writer already owns SQLite's writer lock (its BEGIN wrote
  items first), then wants _fts_lock

Reproduced by re-applying the pre-fix acquisition, 60-item legacy corpus,
one reader and one writer:

  MUTATED (lock on write path)    25.00s  DEADLOCKED  done=[]  stuck=1  errs=['writer:OperationalError']
  as shipped                       0.05s  completed  done=['reader', 'writer']  stuck=0  errs=none

The writer's OperationalError is busy_timeout expiring -- the 500 named in
the finding.

I did not apply the prescribed fix as written ("Revert the lock-guarded lazy
migration"), because reverting reinstates whichever of the two earlier BLOCKING
findings the revert lands on -- the boot-path stall (round 1) or the malformed
-image crash (round 2). The second half of the same sentence names the real fix
("until both paths acquire SQLite write ownership before _fts_lock"), and that
is what shipped, in its stronger form: the FTS write path takes no Python lock
at all.

• Every FTS-touching transaction is now BEGIN IMMEDIATE, so a reader of the
  representation already owns SQLite's writer lock and therefore excludes the
  only thing that can change it. That also closes the cross-process race the
  earlier pass in this same run raised, which a Python lock could never have
  closed -- an MCP tool server is a separate process on the same database.
• _fts_lock now guards the rebuild alone (so two reader threads in one process
  do not both start one) and is always acquired before SQLite's, never after.
  One consistent order, so the inversion is structurally impossible rather than
  merely absent.

Pinned by test_migrating_reader_and_concurrent_writer_do_not_deadlock, which
joins with a 30s budget against a ~0.1s expectation, well under SQLite's 10s
busy_timeout, so a hang is the deadlock and not slowness.

GPT 5.6 -- FINDING: get_entity_items executes MATCH without migration -- FIXED, and it was more than a doc inaccuracy

Following this up found a third FTS reader I had missed, and it was not just
an over-broad docstring: get_entity_items built its own query as
f'"{sanitized}"', quoting the whole entity name. Against a character-segmented
index a single quoted CJK token matches nothing, so this endpoint would have
returned no items for any CJK entity name -- a recall regression introduced by
this PR, on a path none of my tests covered.

It now goes through the shared dialect and runs under asyncio.to_thread like
the other knowledge readers in that module, which also makes its
ensure_fts_index_current call off-loop. Pinned by
test_entity_items_lookup_matches_a_cjk_entity_name, which also asserts ASCII
names and a blank name still behave. The docstring is narrowed to name the three
readers explicitly.

GPT 5.6 -- FINDING: "exact substring matching" overclaims -- FIXED

Fair. OR-of-adjacent-pairs matches a document sharing a single pair, which is
not exact substring matching. Reworded to adjacent-pair matching in
_sqlite_compat.py; the PR body already described the behaviour correctly.

Verification on this head

649 tests pass across eleven targeted suites. black --check, flake8,
isort, ./scripts/docs-lint.sh, git diff --check clean; zero literal CJK.

mypy reports 2 errors, both in src/kiro_crew/transcribe.py (a
Credentials(str | None) arg-type pair), pulled in transitively. Not from this
PR: that file is not in the diff, and the identical 2 errors reproduce with
src/ reverted to base. Left alone rather than folded in.

Still one commit.

[chenmingwei23]

Review round 4 disposition (head fe4ffd0c5)

Both advisory lanes completed on 955792ac0 with CONCERNS bodies (checks green).
Dispositioning each item explicitly. Three fixed, one rebutted.

Design -- _entity_items_rows loosened non-CJK matching -- FIXED (real regression)

The sharpest item in either review, and correct. The old handler quoted the whole
entity name as one FTS5 phrase, so New York required those words adjacent.
My " AND ".join(fts5_cjk_match_groups(name)) matched a document containing both
words anywhere -- a different and wrong answer for an entity name. The review
is also right that my "non-CJK behaviour is bit-for-bit unchanged" claim was true
of the helper but not of that call site, which makes it a description/code
mismatch, not just a behaviour question.

Fixed by matching the name as ONE phrase over the segmented text:

phrase = '"' + fts5_segment_for_index(name).strip().replace('"', '""') + '"'

This is strictly better than what I had. For a name with no CJK,
fts5_segment_for_index returns the text unchanged, so the expression is
byte-identical to the old quoting -- exact parity restored, not merely
approximated. For a CJK name the segmentation is what lets the phrase address the
characters the index stores. Pinned by
test_entity_items_lookup_keeps_multiword_ascii_adjacent (an "adjacent" document
matches, a "New ... York" scattered one does not) plus a mixed-script case.

Design -- no downgrade story for the migrated index -- FIXED (documented)

Correct: the migration is a one-way door, and a rolled-back build writes raw
terms against a segmented index with no rebuild path to recover. Added to
knowledge.md: rolling back past this change requires dropping and recreating
items_fts (or deleting knowledge.db, which is a derived cache of ingested
sources); forward upgrades need nothing, since the first search migrates.

Design -- suggestion: pin the BEGIN IMMEDIATE invariant -- FIXED

Good suggestion, because that invariant is exactly what makes reading the
representation without a Python lock safe, and prose cannot enforce it. Added
test_every_store_transaction_is_begin_immediate, which asserts no deferred
execute("BEGIN") remains in store.py. A future writer re-opening the race now
fails a test instead of passing review.

First Principles -- second hand-maintained CJK character table -- FIXED

Correct, and it was my duplication: I added the fifth copy. history_search._is_cjk_char
now delegates to _sqlite_compat.is_cjk_char, so the ranges -- the thing that
can silently drift -- have exactly one owner, and session search cannot disagree
with knowledge search about what counts as a spaceless script. Its docstring is
kept (it carries the Hangul reasoning) and now names where the ranges live.

I did not also merge the two _script_runs, deliberately: they have different
return contracts (history_search's is a generator whose callers consume it
lazily; _sqlite_compat's returns a list) and they carry no data that can drift.
Verified behaviour-preserving: 443 tests pass across test_history.py,
test_history_composition_contract.py, test_dashboard_sessions_search.py and
test_discord_sessions.py, including TestCjkSearch.

First Principles -- _cjk_subruns rides along in a FIX -- REBUTTED

The graph leg is not a rider here; it is one of the three sites the issue names.
Issue #3691's body lists the whitespace-splitting sites as
knowledge/retrieval.py:248, knowledge/retrieval.py:261, and
knowledge/store.py:1204 -- and :261 is _graph_search. Its task list then
says "Fix the three call sites' query tokenization for CJK". Deferring it would
close the issue with one of its three enumerated sites still broken.

The review itself notes the zero option "still costs CJK entity recall", so we
agree it is not free to drop; we disagree only on whether it belongs to this
change, and the issue text settles that. Happy to split it if a maintainer
prefers a narrower diff -- say so and I will move it out.

Verification on this head

960 tests pass across twelve targeted suites (the knowledge set plus the
session-search set touched by the consolidation). black --check clean on both
non-baselined files (_sqlite_compat.py, history_search.py); flake8, isort,
./scripts/docs-lint.sh, git diff --check clean; zero literal CJK added.

mypy still reports the same 2 pre-existing transcribe.py errors, reproduced
with src/ reverted to base and not in this diff. Still one commit.

[chenmingwei23]

Round 5 (head 175a6c12a)

Backend Lint & Type Check (3.10) -- FAILURE -- FIXED

Step Check for blocking IO inside async def (gate, baselined):

1 baselined file(s) now have fewer on-loop blocking calls. Record the progress
so the baseline keeps shrinking: python3 scripts/check_sync_io_in_async.py --update-baseline

Not a defect -- the opposite. Moving the entity-items lookup onto a worker thread
removed an on-loop blocking call, and the ratchet is shrink-only, so the progress
has to be recorded. Ran the command it names; the diff is one line,
63 -> 62 for dashboard/handlers/knowledge.py. Nothing new is grandfathered
(pruned 0, lowered 1), and the gate now passes: "nothing in scope blocks the
loop outside the baseline".

(Backend Lint & Type Check (3.12) shows cancelled -- matrix fail-fast on its
3.10 sibling, not a separate failure.)

First Principles -- both remaining CONCERNS items -- ADDRESSED

Its verdict on fe4ffd0c5 reduced to two declaration gaps, and it was right
that these are about visibility rather than scope:

1. The graph-leg change was undeclared. My earlier reply defended its scope
   (issue Knowledge search whitespace-splits CJK queries (FTS keyword leg loses recall) #3691 enumerates retrieval.py:261, which is _graph_search), but
   that was answering a different question. The review's actual point stands: the
   description never mentioned the graph leg, and knowledge.md's graph-leg
   bullet sat unchanged in a PR that rewrote the neighbouring keyword-leg bullet.
   That bullet now documents _cjk_subruns, its longest-first ordering, its
   _CJK_SUBRUN_MAX_LEN / _CJK_SUBRUN_MAX_CANDIDATES bound and why one exists
   (each candidate costs a find_entity query), and names it as the second of
   the issue's three sites. The review asked that "a human should see the rider" --
   this is that.
2. A third FTS5 surface was uncounted. Verified independently:
   preferences_fts (apps/builtins/personal_shopper/backend/store.py:173) is
   created with the default unicode61 tokenizer and matches through a
   whitespace-built query, so it carries the identical silent CJK recall loss.
   memory_fts was already declared-and-deferred; this one was invisible. The
   memory spec now states that three product FTS5 tables share the root cause and
   only items_fts is fixed, naming both deferred surfaces.

I have not taken its subtraction (defer _cjk_subruns to a follow-up). The
issue's task list says "Fix the three call sites' query tokenization for CJK" and
:261 is one of them, so deferring it would close #3691 with an enumerated site
still broken. The review itself grants the change survives the zero option
because CJK entity resolution stays dead without it. Still happy to split it on a
maintainer's word.

Lane status on the superseded head fe4ffd0c5

Opus 4.8 PASS, no blocking findings. Design Review PASS (all three of its earlier
items resolved). First Principles CONCERNS -> addressed above. 47 checks green,
zero failures.

GPT 5.6 never produced a verdict there: its run stalled 34 minutes inside the
model step against a 4-9 minute norm for every completed run on this branch, so I
cancelled and re-ran that lane rather than waiting out its 90-minute timeout for a
fail-closed. Worth noting its earlier "review incomplete" on 955792ac0 was the
same shape -- that run was cancelled at 30 minutes when a push superseded it, not
a model error.

Verification on this head

32 CJK tests pass; 960 across twelve suites at the previous head with no
functional change since. black --check clean on both non-baselined files, the
sync-io gate passes, flake8, isort, ./scripts/docs-lint.sh and
git diff --check clean. One commit.

[buluoray]

Verdict: 0 blocking, 2 non-blocking. Substantive review of cc551c7c0a236b5f8de99fe0cbe220fd3b101147.

This is a real root-cause fix. unicode61 stores a spaceless CJK run as a single term, so a query only matched when it equalled an entire indexed run byte-for-byte; the fix segments the index copy to one term per CJK character and expands a query to OR-ed adjacent-character phrases. I verified it against the FTS-injection standard rather than trusting the green lanes.

What I verified

• Injection safety (the primary concern). Every user-derived substring reaching an FTS5 MATCH expression is wrapped in a double-quoted phrase with internal " doubled. In _sqlite_compat.py, fts5_cjk_match_groups quotes non-CJK chunks ('"' + chunk.replace('"', '""') + '"') and builds CJK bigram phrases from characters that by construction (_CJK_RANGES) can never be ", *, ^, or an operator word. Only (, ), OR, AND are emitted unquoted, never from user content. So *, ^, a lone ", or AND/OR/NOT in user input become literal phrase text — no operator injection and no FTS5 syntax error on ordinary input. test_quotes_in_input_cannot_escape_the_literal (a" OR body:* -> ['"a"""', '"OR"', '"body:*"']) pins this.
• All three FTS readers route through the shared dialect. KnowledgeStore._sanitize_fts5 (store.py:1561, " AND ".join(fts5_cjk_match_groups(...))), HybridRetriever._sanitize_fts5_query (retrieval.py:320, " OR ".join(...)), and _entity_items_rows (dashboard/handlers/knowledge.py:600, single segmented phrase). All use items_fts MATCH ? parameter binding; the expression string is the only thing constructed, which is inherent to FTS5 and correctly escaped.
• Latin/ASCII recall unchanged. fts5_cjk_match_groups is proven byte-identical to the previous fts5_quote_tokens for non-CJK input (test_non_cjk_expression_is_unchanged), the join operators are unchanged from before, and space-join vs " AND " are equivalent in FTS5. test_ascii_search_behaviour_is_unchanged confirms oke does not match tokens (no trigram-style substring bleed).
• Existing rows are reindexed, not just new ones. _migrate_fts_index rebuilds items_fts from items, gated on PRAGMA user_version (the CREATE text is identical across formats), triggered lazily by the first reader off the event loop, atomic, and idempotent on interruption. Legacy-rebuild is pinned (test_legacy_index_is_rebuilt_on_open, test_rebuild_spans_more_than_one_batch, test_retriever_leg_migrates_a_legacy_index). The one-way-door downgrade cost is documented in docs/system-specs/modules/knowledge.md.
• Un-index correctness. _fts_unindex deletes with the same representation the index holds (_fts_terms_segmented), avoiding the silent stale-hit / malformed image failure that integrity-check does not catch. The write funnel is mechanically enforced (test_every_items_fts_write_goes_through_the_wrappers, mutation-verified per the round-6 disposition).
• Pathological input. The graph-leg substring enumeration is explicitly bounded (_CJK_SUBRUN_MAX_LEN=6, _CJK_SUBRUN_MAX_CANDIDATES=24, retrieval.py). Query-side bigram expansion is O(n) in query length, not exponential.
• Spec/AGENTS requirement. knowledge.md and memory-skills-hooks.md moved in the same commit; Docs Lint is green.

Findings

1. Non-blocking — store.py ensure_fts_index_current catches OperationalError more broadly than its docstring claims. The docstring says "Only lock/contention errors are absorbed," but sqlite3.OperationalError also covers disk-full and disk-I/O errors. On such an error the migration is swallowed with a WARNING and the search is served against the un-migrated (legacy) index. Consequence: a persistent disk fault degrades CJK recall silently and indefinitely (retried every read) rather than surfacing. This is graceful degradation, not a crash or data loss — matching GPT 5.6's own non-blocking finding. Suggestion: narrow the catch to the SQLITE_BUSY/LOCKED codes (inspect exc.sqlite_errorcode), or reword the docstring to state that all OperationalErrors are absorbed by design.

2. Non-blocking (observation, not a defect) — query-side CJK expansion has no explicit length cap. A pathologically long spaceless CJK query expands to n-1 OR-ed bigram phrases in one MATCH group. This is linear and bounded by the user's own query length (a search box never realistically sees multi-thousand-character input), and the far more dangerous graph-leg enumeration is already capped. No change needed; noted only because the changed path is a user-text-to-FTS surface.

What I could not verify

• I did not re-run the suite or the "red on base" claim — the shared checkout is read-only for this session and no writes were permitted. I relied on tests that structurally cannot pass on main (e.g. fts5_cjk_match_groups does not exist there) plus the green CI lanes and the mutation evidence in the author's round dispositions.
• The 137 ms migration / write-path segmentation cost figures are taken from the PR body, not independently measured.
• The PR is currently mergeable=dirty (conflicting) and needs a rebase before merge; the full green check suite ran on this SHA before the conflict appeared, so signals remain valid, but a rebase will re-dispatch the lanes.

[buluoray]

Verdict: Approve. 0 blocking, 2 non-blocking. This is a re-review after a rebase that dismissed an earlier approval of cc551c7.

What I verified

This is a re-review triggered by the rebase from the previously-approved cc551c7c0a236b5f8de99fe0cbe220fd3b101147 to the current head e540e8f094f6fa01205fcab64cd5942bb0b2f05a. I confirmed the rebase is content-neutral before re-deriving anything: the blob SHAs of all six source files, test/test_knowledge.py, and docs/system-specs/modules/knowledge.md are byte-identical between cc551c7 and e540e8f. The base moved (which auto-dismissed the approval and re-ran the bots); the PR's own tree did not. All four AI lanes re-passed on the current head (Design PASS, First Principles PASS, GPT 5.6 no blocking, Opus 4.8 no findings), 60+ checks green/skipped, none failing, mergeable_state=blocked is just the missing approval.

Re-confirmed at the current head that the properties the earlier review relied on still hold:

• FTS injection safety holds across every reader. There are exactly three FTS readers and each binds MATCH ? with user text carried only as a bound parameter, never interpolated:
  • HybridRetriever._keyword_search (retrieval.py:280-282): WHERE items_fts MATCH ?, params=[safe_query], and the execute is wrapped in except sqlite3.OperationalError: return [] (retrieval.py:294-297).
  • KnowledgeStore.search_items_fts (store.py:1556): WHERE items_fts MATCH ?, bound to _sanitize_fts5(query).
  • _entity_items_rows (dashboard/handlers/knowledge.py): MATCH ? bound to (phrase,).
  • The expression builders (fts5_cjk_match_groups, _sanitize_fts5, the entity phrase) wrap user text as double-quote-escaped FTS5 phrases (internal " doubled), so *, ^, a lone quote and AND/OR/NOT become literal phrase text and cannot inject operators or raise a syntax error. test_quotes_in_input_cannot_escape_the_literal pins this ('a" OR body:*' -> ['"a"""', '"OR"', '"body:*"']).
• ASCII/Latin recall is unchanged, not merely CJK improved. fts5_cjk_match_groups returns exactly what fts5_quote_tokens returns for non-CJK input, and fts5_segment_for_index returns non-CJK text byte-identical, so both index and query paths are untouched for Latin. The store's direct search moved from space-joined phrases to " AND ".join(...), which is the same AND semantics FTS5 already applied to space-separated phrases. Pinned by test_ascii_search_behaviour_is_unchanged (incl. the oke/tokens non-match that rules out substring/trigram behavior) and test_non_cjk_expression_is_unchanged.
• The reindex covers pre-existing rows. _migrate_fts_index does 'delete-all' then re-inserts every row from items in _FTS_REBUILD_BATCH batches, gated on PRAGMA user_version vs FTS_INDEX_VERSION, triggered by all three readers via ensure_fts_index_current. Not new-documents-only. Pinned by test_legacy_index_is_rebuilt_on_open, test_retriever_leg_migrates_a_legacy_index, and test_rebuild_spans_more_than_one_batch.
• Un-index uses the same representation as index. _fts_unindex and _fts_index both derive terms through _fts_terms / _fts_terms_segmented, so a 'delete' subtracts the terms that were actually stored. All five writers (add_item, update_item, _delete_item_cascade, delete_source_cascade, import_bundle) route through the two wrappers, enforced structurally by test_every_items_fts_write_goes_through_the_wrappers; the legacy-before-migration window is handled because _fts_terms_segmented() reads the DB's declared representation and writes raw terms to a raw index (test_legacy_write_before_any_search_does_not_corrupt, test_legacy_delete_of_cjk_item_does_not_raise).
• The diff matches what the body claims (index-side segmentation, per-character-pair phrase query OR-ed, five writers funneled, versioned lazy migration, reader-triggered rebuild, three readers migrating). The system-specs update knowledge.md / memory-skills-hooks.md is in-commit and the Docs/Feature-Map gates are green.

Findings (non-blocking)

1. _sqlite_compat.py:159 (fts5_cjk_match_groups) — a non-CJK chunk inside a mixed token is emitted as its own quoted phrase and AND-ed with the CJK group. A query that appends fullwidth punctuation to a CJK run (e.g. 内存泄漏？) splits into a CJK run plus the punctuation-only chunk "？", which tokenizes to zero terms under unicode61, so the AND yields no keyword hits. Consequence: a residual CJK recall gap for punctuation-bearing queries. Non-blocking because it is not a regression — CJK keyword recall was entirely absent on main, so this is an improvement with a remaining edge, and the keyword leg is guarded (return []) so nothing crashes. Suggestion: drop chunks that tokenize to nothing (punctuation-only non-CJK runs) before building the group. This matches GPT 5.6's single advisory finding.

2. PR body, "What tests we did" — states TestCjkKeywordRecall (17 tests); the class at head actually has 28 tests (TestCjkFts5Primitives's 7 is correct). Stale prose count only, no code impact; flagged for accuracy since I cannot edit the body.

What I could not verify

• I did not execute the test suite (shared checkout is diverged from main and read-only here); I relied on the green Backend Tests lanes at the head SHA and on reading each test against the code it pins. I therefore did not independently mutation-verify that each new test reddens on revert, though the tests target behavior that does not exist without this diff and the body reports 18/21 red on base for the first revision.
• The FTS5 zero-token-phrase behavior in finding 1 is reasoned from the tokenizer's treatment of fullwidth punctuation, not executed.