feat(search): index prose content for BM25 full-text search

[ShauryaaSharma]

What & why

search_graph BM25 only matched node names and headings, so it was blind to the
prose that documentation- and config-heavy repos carry. Markdown Section nodes
exposed only their heading; YAML/JSON Module nodes only their file name — the
section body and the description value were never indexed, and Section/Module
were excluded from BM25 results entirely. This indexes that prose so content is
searchable.

Closes #518
Closes #519

Changes

• store (store.c/.h): add a body column to the nodes_fts FTS5 table; new
  cbm_store_fts_rebuild() drops+recreates it (upgrading legacy 4-column DBs) and
  backfills body from each node's docstring, guarded by json_valid().
• pipeline: both FTS backfill sites call the shared cbm_store_fts_rebuild().
• mcp: stop excluding Section/Module from BM25 results (no rank boost, so code
  results still sort first).
• internal/cbm: capture the markdown section body (Section nodes don't index body text — BM25 can't search markdown content #518) and promote a top-level
  description/summary/purpose value onto the Module node (META.yaml/frontmatter description values not indexed for BM25 search #519), reusing the
  existing docstring property.

Testing

7 extraction cases + 3 store FTS cases added. Verified end-to-end: bodies are
extracted → indexed into nodes_fts.body → returned by BM25; json_valid() tolerates
malformed rows; legacy FTS tables upgrade on rebuild.

Notes

Backward compatible (additive column; legacy DBs upgrade on next index). No MCP
tool changes, no new deps, no new system()/popen()/network calls. #518 and #519
share the FTS body infra (#519 can't work without it), so they're together —
happy to split if preferred.

[ShauryaaSharma]

Rebased onto main (post-#667) , all tests green ✅

Rebased on top of the #667 merge. One minor conflict in extract_defs.c: upstream added qn_safe_segment() immediately before push_simple_class_def as part of the markdown QN slugification work. Resolution was additive, kept both the new helper and the docstring parameter this PR adds.

Verification against the #518 repro in the bug suite:

The #667 markup battery (repro_grammar_markup_markdown) now correctly expects "Section" as the label for headings, which is exactly what this PR emits. Running the 7 targeted extraction tests locally confirms the rebased code is still correct:

markdown_section_body_captured          PASS  ← Playwright body captured, sibling excluded
markdown_section_no_body                PASS  ← empty heading → no docstring
markdown_section_body_capped            PASS  ← body ≤ 500 bytes
yaml_description_promoted_to_module     PASS
yaml_summary_promoted_to_module         PASS
json_description_promoted_to_module     PASS
yaml_no_description_leaves_module_bare  PASS

[DeusData]

Huge thanks for opening this PR and for the work you put into it.

The maintainer shop is currently full, so this may sit for a bit before it gets a proper review. We will come back to this as soon as possible with real feedback; I wanted to make sure it did not sit unacknowledged in the meantime.

[ShauryaaSharma]

No worries at all @DeusData, take your time, there's no rush on my end! Happy to wait and appreciate you acknowledging it. 🙂

[ShauryaaSharma]

Hey @DeusData, just wanted to check in.
No rush at all, I know the queue is full, just bumping in case it fell off the radar.

I rebased and resolved the merge conflict in store.c (was just two comment blocks colliding with the new #768 schema-compat probe on main, no functional overlap), so this should be a clean merge again whenever you get a chance to take a look.

Happy to make any changes if you'd like a different direction on anything. Thanks again for your work maintaining this!

[DeusData]

This has not fallen off the queue. I rechecked the current head (2bc902e): the recorded full CI set is green, and there has been no newer contributor commit requiring another response. No further action is needed from you until the maintainer diff review; thanks for keeping the branch current.

[DeusData]

Reviewed this in full — thank you, and sorry it has waited so long. The engineering is solid and it is built at the right layer; what is holding it is a sizing question plus a rebase, not a design objection.

What I want to call out first, because it is the thing most contributions in this area get wrong: you did not hand-roll a BM25 implementation. You extended the SQLite FTS5 index we already vendor and already use for exactly this, added no dependency, and in the process deduplicated the two existing backfill sites into a shared cbm_store_fts_rebuild() — a net −25 lines. That is how a maintainer would have built it.

Several details showed real care: the json_valid() guard with a dedicated malformed-row test so legacy databases degrade to name-indexing instead of breaking; the legacy 4-column upgrade test, which is precisely the migration proof we want; the 500-byte cap matching our existing MAX_COMMENT_LEN; and the UTF-8-safe backoff. I also confirmed the migration story is genuinely benign — legacy DBs keep working read-only and get upgraded by the DROP+recreate at the next index run, which was already a full FTS rebuild, so there is no forced graph rebuild and no migration event to announce.

One factual correction worth having on record, in your favour: issue #518 asserts that bt/docstring content is already BM25-indexed. That is not true on main — nodes_fts carries only name, qualified_name, label and file_path. I verified the DDL. So this is a real gap, not a rediscovery.

The branch has drifted and needs a rebase. bm25_search on main now carries a label-boost CASE (Function/Method −10, Route −8, Class/Interface/Type/Enum −5) plus an ORDER BY rank, n.id tie-break, and TOON tree output. Your label-filter change needs re-applying onto both query strings by hand. Your comment about "(no rank boost, so code results still sort first)" should be rewritten while you are there — and the news is good: the boost actually strengthens your design, because Section and Module fall into the unboosted ELSE bucket, so code symbols keep their ordering advantage by construction rather than by luck.

The one thing I need from you before this can land: numbers.

The backfill indexes the docstring of every node, not only the Section and Module nodes that #518 and #519 need. On our reference corpus (~8.5M nodes) I estimate that at +150–400 MB of FTS index and +15–60 s per index run — and that rebuild happens on every run, including incremental background refreshes. Those are engineering estimates, not measurements, and I would rather not merge a scale change on an estimate.

Could you run scripts/benchmark-index.sh before and after on a large corpus and post the numbers? If the cost lands at the high end, there is an easy lever: restricting the body backfill to Section/Module rows is a one-line WHERE that removes roughly 95% of the cost, at the price of losing function-docstring search. I would rather see the measurement first and then choose deliberately.

One thing that is on us, not you. The PR un-excludes both Section and Module from the BM25 label filter. Because every file in every language has a Module node, that makes all of them returnable and countable — which re-admits the per-file nodes that filter deliberately treated as structural noise. #519 only needs the Modules carrying a promoted description, and there is no cheap SQL predicate that separates those. We are weighing that noise-versus-findability trade on our side and will come back with a direction. Nothing for you to redo there yet — please do not pre-emptively change it.

So: rebase, fix the stale ranking comment, and bring benchmark numbers. The direction call on Module scope is ours and I will get you an answer.

One last note — a small operational nuance rather than a defect: DROP+recreate opens a brief "no such table" window where a concurrent bm25_search returns NULL and silently degrades to the regex path. The old delete-all did not have that window. It degrades gracefully, but with daemon-era concurrent sessions it is now observable, so it is worth a thought during the rebase.

[DeusData]

Thank you for the sustained work on this contribution. I need to correct our earlier sequencing: asking first for a broad rebase and combined benchmark left you carrying two product decisions in one review, which was not the clearest path.

Please split this into two ordered PRs:

1. The Section nodes don't index body text — BM25 can't search markdown content #518 FTS body infrastructure and Markdown Section indexing
2. The META.yaml/frontmatter description values not indexed for BM25 search #519 YAML/JSON Module description promotion and Module search-scope decision

The second PR may be stacked on the first if that is the cleanest way to preserve dependencies. Please provide focused correctness and performance evidence for each slice rather than one aggregate benchmark over both behaviors. The umbrella PR can remain as the ledger that links the two. This structure lets us evaluate the shared storage/search change separately from the public Module semantics. Thank you for your patience with the earlier mixed guidance.

[DeusData]

Three things: the direction answer I owe you, a trap in the rebase that neither of my earlier reviews caught, and a correction that works in your favour.

First — confirmed, the gap is still entirely real. I re-checked against current main rather than assuming: the FTS5 table is still the four-column name, qualified_name, label, file_path (store.c:359); Section and Module are still excluded from BM25 in both the search and count queries (mcp.c:3031 and :3066); push_simple_class_def still takes no docstring and never sets one (extract_defs.c:3852); and the Module node is still zeroed without a description (extract_defs.c:7536). Issues #518 and #519 remain open, and nothing else on main makes prose findable — search_code is grep over code, not BM25 over content. Seven weeks on, this PR still closes something nothing else closes.

Second — the direction answer I said I owed you, and I am sorry it took this long: yes, Section and Module should become search-returnable. I had reserved that as a maintainer decision and then left you waiting on it, which was unfair since it gates half your PR.

The reasoning is the project's stated priority: findability first, token thrift second. A node carrying a human-written description is precisely what someone is searching for when they type a phrase rather than a symbol, and excluding it means the graph knows the answer and refuses to say so. The noise objection is real but bounded — a Module node only becomes interesting if it has a description, and the WHERE lever you already have limits indexing to nodes with prose. If it turns out noisier than I expect, narrowing the label set later is a one-line change; leaving the capability out is not recoverable without another PR like this one.

So: build for both labels returnable.

Third — the rebase is bigger than "re-apply the label filter to both queries", and this is the part I most want you to see before you start.

Since you opened this, main grew a third FTS write site that neither of my reviews mentioned, because it did not exist then:

/* src/pipeline/pipeline_delta.c:527 */
INSERT INTO nodes_fts (rowid, name, qualified_name, label, file_path)
SELECT … WHERE project = ?1 AND id > ?2

That is a row-level incremental insert on the delta-merge path, with a documented FTS policy block just above it at pipeline_delta.c:20. If you add a fifth column and leave that site alone, it keeps inserting four columns into a five-column table — so every node arriving through delta merge would carry an empty body. Prose added incrementally would be silently unsearchable, while a full reindex would work fine. That is the worst possible shape of bug: correct on the path you test, wrong on the path users actually hit day to day.

It needs to be part of the first slice, not a follow-up.

Related: pipeline_incremental.c no longer contains any nodes_fts code at all, so your +3/−11 hunk there is aimed at code that has since been deleted. Drop it. The full-index backfill also moved to pipeline.c:1496, and bm25_search moved to mcp.c:2987 and gained a bool toon parameter with TOON tree row emission plus a label-boost CASE and ORDER BY rank, n.id.

The correction in your favour, on the cost question. I framed the benchmark request as though this PR introduces a rebuild. It does not — the full-index path on main already does delete-all plus full re-INSERT at pipeline.c:1496. Your change makes an existing rebuild index more text; it does not add a pass. So the honest cost question is narrower than I implied: per-row body tokenisation, the storage the body column adds, and the DROP+CREATE window during the schema upgrade where a concurrent bm25_search finds no table and degrades to the regex path. That last one is transient and one-time-per-upgrade, but it should be stated in the PR rather than discovered.

I still want measured numbers per slice rather than estimates — mine were guesses and I labelled them as such — but you are measuring a smaller delta than my earlier comment suggested.

Where that leaves the split, unchanged from 18 August: slice one is the FTS body infrastructure plus Markdown Section capture, including the pipeline_delta.c write site; slice two is the YAML/JSON description promotion. This PR stays as the ledger.

And a straight question, because seven weeks is a long time and I would rather know than keep guessing. Do you still want to carry this? It is a real capability and the work you have done is sound — the FTS5 reuse instead of a hand-rolled BM25, the json_valid() guard, the legacy-schema upgrade test and the UTF-8 backoff are all things I would have asked for. But if you have moved on, say so and we will distill it with Co-authored-by credit to you rather than let the oldest PR in the repo keep aging. Either answer is completely fine; silence is the only one that costs us both.

[ShauryaaSharma]

Yes, I still want to carry this. I apologize for the delay. Thanks for asking directly rather than letting it drift, and thanks for the direction call on Section/Module; that was the piece I was parked on.

Slice 1 is up as #1778.

The split. Agreed, and I've stacked them:

• Slice 1 (Section nodes don't index body text — BM25 can't search markdown content #518) — feat(search): index markdown section bodies for BM25 full-text search (#518) #1778: the body column, cbm_store_fts_rebuild() with the legacy 4-column upgrade, the full-index backfill, the pipeline_delta.c write site, un-excluding Section from both queries, and the section-body extraction with its tests.
• Slice 2 (META.yaml/frontmatter description values not indexed for BM25 search #519) : YAML/JSON description promotion onto Module, plus un-excluding Module, stacked on slice 1.

This PR stays open as the ledger.

The pipeline_delta.c trap, thank you, that's the catch of the review. I'd have missed it, and it's a little nastier than your description: adding a fifth column doesn't break that INSERT, it just leaves body NULL. No compile error, no failing test, nothing red anywhere, prose added through delta merge would simply be unsearchable while a full reindex looked perfect. It's in slice 1, and it has its own test (fts_delta_insert_populates_body) rather than relying on the full-index path, since the whole failure mode is "the tested path works."

Confirmed against current main before starting, so I wasn't working from stale assumptions:

• nodes_fts was still the four-column table (store.c:360)
• Section/Module still excluded in both the search and count queries (mcp.c:3031, mcp.c:3066)
• push_simple_class_def still set no docstring (extract_defs.c:3852)
• the Module node still memset to zero with no description (extract_defs.c:7537)
• pipeline_incremental.c has zero nodes_fts references — you're right, my old hunk there was aimed at code that no longer exists. Dropped.

On the rebase: the old branch was ~1100 commits behind its merge base, and pipeline.c, mcp.c and store.c had all been reworked around exactly the regions I touched. I rebuilt slice 1 against current main using the old diff as the design spec rather than forcing a conflict resolution through that much drift, cleaner to review, and less risk of silently reintroducing something that was deliberately changed. #1778 merges clean.

Two small departures from the original branch, both because your review pointed at them. The cap now reuses the existing MAX_COMMENT_LEN rather than declaring a parallel 500-byte constant, so the match you noted is explicit rather than coincidental. And the body expression lives in store.h as CBM_SQL_FTS_BODY_EXPR rather than inside store.c, so the delta site and the wholesale rebuild are physically sharing one expression, with a comment at the definition explaining why a four-column INSERT is the failure mode.

The DROP+CREATE window is stated in the PR description rather than left to be discovered: during the one-time schema upgrade there's a brief interval where a concurrent bm25_search finds no table and degrades to the regex path. One-time per database, graceful, but real, and daemon-era sessions make it observable.

On the numbers, one thing I need to be straight with you about.

I can't run scripts/benchmark-index.sh meaningfully. It drives a built product binary and shells out to python3 for timing, and my local toolchain is 32-bit MinGW.org GCC 6.3.0 with no make and no sanitizer support. I can build and run targeted probes and the test suites, but not a full product binary, and a 32-bit address space wouldn't hold anything near your 8.5M-node reference corpus anyway. If I posted end-to-end numbers from what I can build here, they'd be measurements of the wrong thing at the wrong scale, and I'd rather say that than hand you a figure that looks authoritative and isn't.

What I can measure directly is the narrowed question from your correction, since the full-index path already does delete-all plus full re-INSERT, the real delta is per-row body tokenisation and the storage the column adds. I can build a standalone harness against the vendored SQLite with FTS5 and measure, at a range of node counts and realistic docstring lengths:

• index size, body column vs. no body column
• INSERT/tokenisation wall-clock for the same rows
• the same pair with the WHERE label IN ('Section','Module') lever applied, so the cost of the broad backfill versus the narrow one is an actual number rather than your estimate or mine

That isolates the variable your estimate was about and it's reproducible, I'd commit the harness so you can run it too. What it won't give you is real-corpus wall-clock at your scale. For that I think the honest options are that you run benchmark-index.sh on the reference corpus, or we get it into CI. Happy either way; I just can't be the one to produce that particular figure.

I've deliberately not built the harness yet, if it isn't the evidence you want, I'd rather hear that than measure the wrong thing twice. Say the word and it's a short job.

Per slice, as you asked, slice 1's numbers with slice 1, slice 2's with slice 2. Slice 2 goes up once #1778 has a direction.

[DeusData]

Yes to the harness — build it. That is exactly the evidence I want, and better than what I asked for.

Answering the question you are blocked on first, since you sensibly asked before measuring.

On the benchmark: your refusal was the right call and I am glad you made it. A 32-bit MinGW toolchain with no make cannot produce a meaningful end-to-end number against an 8.5M-node corpus, and posting one anyway would have given us a figure that looked authoritative and was not. Declining to generate false precision is worth more to me than the number would have been.

The harness you propose measures precisely the variable my own correction narrowed this to. Since the full-index path already does delete-all plus full re-INSERT, the real delta is per-row body tokenisation plus the column's storage — and that is portable, isolatable, and does not need the product binary at all. Index size with and without body, INSERT wall-clock for the same rows, and both again under a WHERE label IN (…) filter, is the shape that turns "broad versus narrow backfill" from an argument into a number. Please commit it.

And I will take the other half. Real-corpus wall-clock on the reference machine is my job, not yours — I have the corpus and the built binary and you have neither. I will run scripts/benchmark-index.sh before/after once slice 1 is otherwise ready, and post the numbers on that thread. That splits it the right way round: you measure the mechanism portably, I measure the reality on the only hardware that can.

Your read on the pipeline_delta.c trap is sharper than mine and I want that on the record. I described it as inserting four columns into a five-column table; you are right that it does not break at all — it simply leaves body NULL. No compile error, no failing assertion, nothing red anywhere, and prose added through delta merge is quietly unsearchable while a full reindex looks perfect. That is strictly worse than what I described, because my version would at least have announced itself.

Giving it a dedicated fts_delta_insert_populates_body that deliberately does not route through the full-index path is exactly right. The whole failure mode is that the tested path works, so a test that reaches it through the working path proves nothing.

Two design decisions of yours I want to endorse explicitly, because both are the kind of thing a reviewer might otherwise read as churn:

• Hoisting the body expression into store.h as CBM_SQL_FTS_BODY_EXPR so the delta site and the wholesale rebuild physically share one definition. That converts "remember to update both" into "there is only one", which is the only durable fix for the class of bug we just found. Recording the four-column INSERT as the failure mode in a comment at the definition is the right place for that note.
• Reusing MAX_COMMENT_LEN instead of declaring a parallel 500-byte constant. I flagged that coincidence; making it explicit rather than coincidental is better than either keeping two constants or silently relying on them matching.

On the from-scratch rebuild: agreed, and it was your call to make. A branch ~1,100 commits behind, across three files that were all reworked around the exact regions you touched, is not a rebase — it is an archaeology exercise where the most likely outcome is silently reintroducing something that was deliberately changed. Using the old diff as a design spec and rebuilding against current main is lower-risk and much easier to review. I would rather review 859 clean lines than a conflict resolution I cannot audit.

Thank you for the pre-flight verification too. Independently re-confirming the five locations against current main — and conceding that your own pipeline_incremental.c hunk targeted deleted code — is the check I have been asking every contributor in this backlog to run, and almost nobody does it unprompted.

Disclosing the DROP+CREATE window in the PR description rather than leaving it to be found is the right instinct as well. One-time and graceful is fine; undocumented is not.

I am picking up #1778 now. #617 stays open as the ledger, as you proposed.

Sorry again for the seven weeks. You have been the most responsive party in this exchange and you were waiting on a direction call that only I could make.

[ShauryaaSharma]

Harness is committed — b417ade4 on #1778, as scripts/benchmark-fts-body.c with a benchmark-fts-body.sh wrapper. Keeping the detail on that thread since you've picked it up there; this is just the ledger entry.

Two things from it worth flagging here, since they bear on the sizing question this PR has been held on:

• Your storage estimate held: ~24 bytes per node marginal, so ~200 MB at 8.5M nodes — inside the +150–400 MB you estimated.
• The WHERE lever removes ~66% of the added storage, not ~95%. The 95% is right for row count, but storage doesn't follow row count — section bodies run longer than function docstrings and the fifth column has fixed overhead regardless. Details and the table are in my comment on #1778.

Not quoting timings — my machine's run-to-run spread is 39–44%, large next to the delta, and the harness flags that itself rather than presenting a noisy column as authoritative. Your reference-machine run is the one that will produce meaningful wall-clock; thanks for taking that half.

Ledger status: slice 1 is #1778 (open, merges clean). Slice 2 (#519) stacks on it and I'll hold until #1778 has a direction, since review could move the base. This stays open as the ledger as agreed.

And no need to keep apologising for the seven weeks — you gave me a genuinely useful review and the pipeline_delta.c catch saved a bug I'd have shipped.