Skip to content

fix(search): index docstrings and accept non-ASCII query bytes - #1403

Closed
Metatau wants to merge 1 commit into
DeusData:mainfrom
Metatau:feat/non-ascii-fulltext-search
Closed

Metatau wants to merge 1 commit into
DeusData:mainfrom
Metatau:feat/non-ascii-fulltext-search

Conversation

@Metatau

@Metatau Metatau commented Aug 1, 2026

Copy link
Copy Markdown

Fixes #1401

BM25 search was unusable for any project documented in a non-Latin script. Two independent causes, both in this diff.

1. The query tokenizer dropped non-ASCII bytes

bm25_build_match() accepted only [a-zA-Z0-9_] as token bytes, so a Cyrillic query was tokenized to nothing and reached FTS5 as an empty MATCH — the caller got unranked noise rather than an empty result, which is why this went unnoticed.

Bytes >= 0x80 are now token bytes. No UTF-8 decoding is introduced: the only requirement here is to keep multi-byte sequences intact, since the FTS5 unicode61 tokenizer already applies the real word-boundary rules on both the index and the query side.

2. Docstrings were never indexed

nodes_fts covered name, qualified_name, label, file_path — identifiers only. In real projects identifiers are English even when the team documents in another language, so the one field that could match a native-language query was the one field missing from the index.

A docstring column is added and populated from json_extract(properties, '$.docstring') in both backfill paths (pipeline.c, pipeline_incremental.c).

Schema upgrade. Databases created before this carry the 4-column table, and CREATE VIRTUAL TABLE IF NOT EXISTS would leave them that way — every subsequent backfill would fail on the column count. The new column is probed with a cheap SELECT docstring FROM nodes_fts LIMIT 0 and the table dropped only when it is missing. The table is contentless, so nothing is lost: the next backfill repopulates it from nodes.

Result

Same project, same query, before and after:

# before (v0.9.0)
$ ... search_graph --query 'провалидировать аргументы схеме'
  .env.example      File
  Branch            Class
  Coordinates       Class      ← the documented function is nowhere in the results

# after
  ...server._validate_args   Function   src/…/server.py   78-96   -18.89

English search improves as well rather than regressing — validate arguments schema now ranks _validate_args first, because docstrings carry intent that identifiers alone do not. camelCase splitting is unaffected.

Testing

  • New regression test tool_search_graph_query_matches_non_ascii_docstring in tests/test_mcp.c: a node whose only Cyrillic text is its docstring must be found by a Cyrillic query, and an unrelated node must not. It fails on main and passes here.
  • Full suite: 6825 passed, 4 skipped, 0 failed (build/c/test-runner, macOS 15.5 arm64).
  • One earlier run reported a single failure that did not reproduce on a clean re-run; I could not identify the test because that run's log was truncated on my side. The mcp suite specifically is stable across runs (190 passed, 2 skipped).
  • make -f Makefile.cbm lint-format could not run locally — clang-format is not installed on this machine. The new code follows the surrounding style; happy to adjust if CI disagrees.

Scope

No API surface change, no new dependency, no new pipeline pass. json_extract is already available in the vendored SQLite (3.51.3).

@Metatau
Metatau requested a review from DeusData as a code owner August 1, 2026 10:29
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

BM25 search was unusable for any project documented in a non-Latin
script, for two independent reasons.

bm25_build_match() treated only [a-zA-Z0-9_] as token bytes, so a
Cyrillic (or Greek, or CJK) query was tokenized to nothing, reached
FTS5 as an empty MATCH and returned unranked noise. Bytes >= 0x80 are
now token bytes: no UTF-8 decoding is needed here, only keeping
multi-byte sequences intact, since the unicode61 tokenizer already
applies the real word-boundary rules on both sides.

nodes_fts also indexed identifiers only. Identifiers are English in
practice even when the team writes prose in another language, so the
one field that could match a native-language query was the one field
never indexed. A docstring column is added and populated from
json_extract(properties, '$.docstring') in both backfill paths.
Databases created before this carry the 4-column table, which
CREATE ... IF NOT EXISTS would leave in place and every backfill would
then fail on, so the new column is probed and the table dropped when
missing — it is contentless, nothing is lost, the next backfill
repopulates it from nodes.

English search improves as a side effect: docstrings carry intent that
identifiers alone do not.

Fixes #1401

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Metatau
Metatau force-pushed the feat/non-ascii-fulltext-search branch from 8284486 to 9ef1c23 Compare August 1, 2026 13:48
@Metatau Metatau closed this Aug 1, 2026
@Metatau

Metatau commented Aug 1, 2026

Copy link
Copy Markdown
Author

Force-pushed a fix to a defect in my own patch — worth describing, because the first version would have been worse than the bug it fixes.

What was wrong. The schema upgrade dropped the 4-column nodes_fts and left repopulation to the pipeline's backfill. But a project that is already indexed and whose files have not changed never reaches that backfill — there is nothing to index. The FTS index therefore stayed empty until something happened to modify a file, and search returned nothing at all in the meantime: not just non-ASCII queries, every query. Anyone upgrading would have seen search break across all their existing projects.

I hit this on my own machine right after installing the patched binary: a previously working project stopped matching even plain validate.

Fix. The upgrade is now self-contained — immediately after recreating the table it repopulates from nodes, without waiting for the pipeline. cbm_camel_split is registered per connection later in startup, so the recovery insert uses plain names; the next full index run rewrites the table with split tokens.

Verified on real databases, reproducing the exact sequence that broke:

step English query non-ASCII query
index built by released 0.9.0 (4-column table) works —
first run of the patched binary works works
second run, no files changed works works

The third row is where the previous revision went empty.

One behavioural note I should flag: the upgrade path runs when the schema is created, not on read. Upgrading the binary alone therefore neither breaks nor fixes an existing database — it keeps working under the old schema until the project is indexed again. That seemed preferable to migrating on every open, but say the word if you would rather it happened eagerly.

Testing after the fix: full suite 6824 passed, 1 failed, 4 skipped. The failure is tool_detect_changes_contained_commands_clean_up_error_and_success at ASSERT(merge_base_reported) — git merge-base reporting, untouched by this diff. It passes 3/3 when run in isolation, and the same build alternated between 0 and 1 failures across full runs, so I believe it is flaky rather than a regression here. Happy to be corrected if it is known-stable on your CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

search_graph: non-ASCII queries return noise, and docstrings are never indexed

1 participant