feat(search): index markdown section bodies for BM25 full-text search (#518)

[ShauryaaSharma]

Slice 1 of the #617 split, per the review guidance there: the FTS body infrastructure plus Markdown Section indexing. #617 stays open as the ledger; the #519 YAML/JSON Module description promotion follows as a second PR stacked on this one.

Rebuilt against current main rather than rebased — the original branch was ~1100 commits behind and pipeline.c, mcp.c and store.c had all been reworked around the touched regions, so the old diff served as the design spec.

What & why

search_graph BM25 only matched node names and headings, so it was blind to markdown prose. Section nodes exposed only their heading text and were excluded from BM25 results entirely. This indexes the section body so content is searchable.

Closes #518.

Changes

store — adds a body column to the nodes_fts FTS5 table. New cbm_store_fts_rebuild() drops and recreates the table (which is what upgrades legacy 4-column databases) and backfills body from each node's docstring, guarded by json_valid() so malformed-JSON rows degrade to name-only indexing instead of aborting the whole INSERT...SELECT.

CBM_SQL_FTS_BODY_EXPR is exported from store.h so every nodes_fts write site feeds body through one expression.

pipeline — the wholesale backfill delegates to cbm_store_fts_rebuild() (net −17 lines). The row-level delta-merge insert in pipeline_delta.c writes body through the same shared expression.

That third write site is the one worth reviewing closely. nodes_fts now has five columns, and an INSERT naming only the original four is still perfectly valid SQL that silently leaves body NULL — prose arriving through delta merge would have been unsearchable while a full reindex looked correct. It has its own test rather than relying on the full-index path.

mcp — stops excluding Section from BM25 results in both the search and count queries. Section falls in the unboosted ELSE 0.0 bucket of the label-boost CASE, so code symbols keep their ranking advantage by construction rather than by exclusion. Module stays excluded here and is handled in the #519 slice.

internal/cbm — captures the markdown section body beneath each heading, stopping at the first nested subsection (each subsection gets its own Section node and its own body). Capped at the existing MAX_COMMENT_LEN (500) with a UTF-8-safe backoff, reusing the existing docstring property.

Testing

Seven cases across three suites. CI runs them on ubuntu/macOS/Windows including MSan, TSan and LSan.

test	guards
markdown_section_body_captured	body captured, sibling section excluded
markdown_section_no_body	documented sibling has prose while a bare heading does not
markdown_section_body_capped	cap holds and the UTF-8 backoff does not split a character
fts_rebuild_indexes_body_content	body tokens searchable, names still searchable
fts_rebuild_upgrades_legacy_schema	legacy 4-column table upgraded in place
fts_rebuild_tolerates_malformed_properties	json_valid() guard holds
pipeline_delta_merge_indexes_body	the incremental write site, driven through cbm_delta_patch

The delta test is the one worth looking at. It drives the real merge — cbm_delta_preseed, then cbm_delta_patch — rather than reimplementing the INSERT in the test body. Verified that it discriminates: against a four-column revert of pipeline_delta.c the body assertions fail while the name assertion still passes, confirming the node merged and only its body was lost.

Writing it against the production path also surfaced an ordering constraint a hand-rolled version could not: cbm_delta_preseed lifts the gbuf id watermark above MAX(id), so a node added to the gbuf before that call is silently never merged.

The extractor's grammar assumption was verified directly against tree_sitter_markdown rather than assumed: for nested headings the parser produces section [0-81] containing the heading, a paragraph, and a nested section [48-81], and the helper correctly stops at byte 48.

Performance

scripts/benchmark-fts-body.c (added here) isolates what the body column actually costs. Since the full-index path already did delete-all plus a full re-INSERT, body does not add a pass over the graph — it makes an existing rebuild index more text. So the harness measures the two things that genuinely change: per-row body tokenisation and the storage the column adds. It builds one synthetic corpus three ways:

• A — 4-column FTS (pre-Section nodes don't index body text — BM25 can't search markdown content #518 baseline)
• B — 5-column, body for every node (what this PR ships)
• C — 5-column, body only for Section/Module rows (the WHERE lever)

Worth knowing when reading the diff: the harness is 477 of the ~934 added lines, so a little over half of this PR is a measurement tool rather than feature code.

FTS index size

nodes	A	B	C	B − A
100k	7.2 MB	10.2 MB	7.6 MB	+3.0 MB (+41%)
200k	15.0 MB	19.7 MB	15.8 MB	+4.7 MB (+31%)
500k	36.6 MB	48.6 MB	40.7 MB	+12.0 MB (+33%)

Beyond the 100k point (where fixed page overhead distorts the ratio) the marginal cost settles at ~24 bytes per node. Extrapolated to the ~8.5M-node reference corpus that is ~200 MB — inside the +150–400 MB band estimated in review.

One correction to the estimate

The WHERE label IN ('Section','Module') lever was expected to remove ~95% of the cost. Measured, it removes ~66% of the added storage (at 500k: +12.0 MB → +4.1 MB).

The 95% holds for row count — variant C indexes 17.8k bodies against B's 157k, which is 11% — but storage does not follow row count. Markdown section bodies run substantially longer than function docstrings, and the fifth column carries fixed overhead whether or not a given row has prose. So narrowing buys less than it appears to, which seemed worth knowing before trading away function-docstring search for it.

On timing

I am deliberately not quoting backfill timings. My machine shows 39–44% run-to-run spread, which is large relative to the delta being measured; the harness detects this and prints a warning rather than letting a noisy column pass as authoritative. The size figures above are deterministic and unaffected. Timings on a quieter machine would be meaningful — scripts/benchmark-fts-body.sh builds and runs it.

Two methodology notes, since both mistakes produce confident-looking nonsense and I hit them: variants are interleaved rather than grouped (grouped runs let drift land on whichever variant goes last, which reported C as slower than B despite C doing strictly less work), and the minimum is reported rather than the mean.

This does not replace scripts/benchmark-index.sh, which measures real end-to-end indexing on a real repository. I cannot run that one — it needs a built product binary and shells out to python3, and my toolchain is 32-bit MinGW.org GCC 6.3.0 with no make; a 32-bit address space also will not hold anything near the reference corpus. Per the split agreed on #617, that half is being run on the reference machine — this harness measures the mechanism portably, the reference run measures the reality.

Behavioural changes

Two things change for existing callers, both deliberate, neither purely additive:

Section is now returned by BM25 and counted in the total. That is the intended effect of the direction call on #617 — it changes both the result set and the total for existing search_graph callers. Section sits in the unboosted ELSE 0.0 bucket of the label-boost CASE, so code symbols keep their ranking advantage by construction rather than by exclusion.

BM25 scores shift for every row, including rows with no body. bm25(nodes_fts) is called with no column weights, so all columns sit at the default 1.0. FTS5 normalises by document length against the corpus average, and body tokens count toward row length — so avgdl moves as soon as body is populated, and every row's normalisation moves with it. This is not repairable by weighting: even body at 0.0 removes the contribution but not the length.

So the ranking question is not whether scores move but whether they move deliberately. I have left the weights alone pending a steer, since the ranking model is the maintainer's call. My reasoning is in the review thread: weights are worth setting to bound how far prose can outrank a symbol match, which argues for body below name/qualified_name rather than at parity.

Notes

The schema change is backward compatible — additive column, legacy databases keep working read-only and are upgraded by the DROP+recreate at the next index run, which was already a full FTS rebuild. Note that this is a statement about the schema, not about ranking; see the behavioural changes above.

One operational caveat worth stating rather than leaving to be discovered: the DROP+recreate opens a brief window during the schema upgrade where a concurrent bm25_search finds no table and degrades to the regex path. It is one-time per database and degrades gracefully, but with daemon-era concurrent sessions it is observable.

No MCP tool changes, no new dependencies, no new system()/popen()/network calls.

[github-actions]

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.

[DeusData]

Read in full. The structure is right and slice 1 is complete with no slice-2 leakage. But the delta test has exactly the hole it was written to close, and there is a second behavioural change the description does not mention. Both are fixable; neither is a rethink.

The delta test does not guard the delta call site

This is the important one, and I checked it rather than reading the name and moving on.

fts_delta_insert_populates_body never calls cbm_delta_patch() or any production entry point. It hand-prepares its own INSERT INTO nodes_fts (...) inside the test body — and the copy is not even identical to production, since it uses plain name where pipeline_delta.c uses cbm_camel_split(name).

So: revert pipeline_delta.c's INSERT to the original four columns and this test still passes. It guards CBM_SQL_FTS_BODY_EXPR; it does not guard the site that consumes it. That is precisely the failure mode you described better than I did — no compile error, nothing red, body silently NULL on the path users hit daily — and the test written to catch it cannot.

The fix is to drive the production delta path: build a base index, add nodes, run the real delta merge, then assert fts_match_count finds body text from the newly merged rows. If that is awkward to reach from test_store_search.c, it belongs wherever cbm_delta_patch is already exercised. A test that reimplements the thing it is testing proves the reimplementation.

A fourth write site exists

tests/test_mcp.c:2489 does a delete-all plus a four-column INSERT INTO nodes_fts(rowid, name, qualified_name, label, file_path). It stays valid SQL against a five-column table, so nothing goes red — it just leaves body NULL and stops mirroring production shape.

Not a correctness bug today, but the store.h comment claims CBM_SQL_FTS_BODY_EXPR covers every nodes_fts write site, and that is now inaccurate. Either route that site through the macro too or narrow the claim, because a comment that overstates its own coverage is how the next person gets caught.

BM25 ranking changes for every query, including nodes with no body

bm25(nodes_fts) is called at mcp.c:3025 and :3062 with no column weights, so every column carries the default 1.0. Adding a fifth column changes the score for every row in the table — including rows whose body is empty. Existing search_graph result ordering shifts for queries that have nothing to do with prose.

The PR body says "backward compatible — additive column". That is true of the schema and not of the ranking. This is worth an explicit decision rather than an accident: either accept the shift and say so, or pass explicit column weights to bm25() so the four original columns keep their current relative influence and body is added deliberately. I lean towards explicit weights — it makes the ranking a stated design rather than a side effect of column count — but I would rather hear your reasoning first, since you have been closer to the FTS behaviour than I have.

Alongside that: Section is now returned by BM25 and counted in the total, which changes both the result set and the count for existing callers. That is the intended effect of the direction call I gave you, so it is correct — it just belongs in the description as a behavioural note.

Two test weaknesses

• markdown_section_no_body is vacuous. On main every Section docstring is already NULL, so ASSERT(body == NULL || body[0] == '\0') passes unchanged with or without your production code. It needs to assert something only true after the change — for instance that a sibling section in the same file does have a body while this one does not.
• markdown_section_body_capped hardcodes 500 rather than MAX_COMMENT_LEN, which reintroduces exactly the parallel-constant coupling you removed from production. And its corpus is "alpha " repeated — pure ASCII — so the UTF-8 backoff never executes. A multi-byte character straddling the cap is the case that backoff exists for.

Mechanical

lint fails on src/store/store.h:361: CBM_SQL_FTS_BODY_EXPR is one 273-byte physical line. Valid C via adjacent string concatenation, but past the column limit — split it across continuations and it clears.

Worth knowing that this is currently blocking everything else: the test job is skipped the moment lint fails, so none of your seven tests have executed in CI even once. Fix the line and you get a real signal.

What is right, and it is most of it

CBM_SQL_FTS_BODY_EXPR as a single definition consumed by both production sites is the correct structural answer — it converts "remember to update both" into "there is only one", which is the durable fix for the class of bug we found. The DDL is likewise single-source. fts_rebuild_tolerates_malformed_properties genuinely guards the json_valid() wrapper, and fts_rebuild_upgrades_legacy_schema proving a four-column database upgrades in place is the test most people would have skipped. Documenting the DROP+CREATE window in the body, the commit message and at the function is three places more than it needed to be.

Scope is clean: Module stays excluded in both queries, no YAML/JSON promotion, nothing from slice 2 leaked in.

On the benchmark harness — you committed it about twenty minutes after asking whether it was wanted. I have since said yes on #617, so keep it; I mention it only because "asked, then shipped before the answer" is a habit worth not forming. It is 477 of the 859 lines here, which is also worth a sentence in the description so a reviewer knows 55% of the diff is a measurement tool rather than the feature.

Fix the delta test, decide the BM25 weights question, tighten the two tests and split that header line, and I will merge this. Then I will run the real-corpus benchmark on my side as promised.

[ShauryaaSharma]

Note: written and posted before I had read your review above — the two crossed. Replying to the review properly in a follow-up; leaving this as posted since the benchmark numbers stand on their own.

Harness is in — b417ade4, as scripts/benchmark-fts-body.c plus a benchmark-fts-body.sh wrapper. The PR description above now carries the full table; this is the part that might change a decision you're weighing.

Your storage estimate was right. Beyond the 100k point, where fixed page overhead distorts the ratio, the marginal cost settles at ~24 bytes per node — about 200 MB extrapolated to 8.5M nodes, inside the +150–400 MB you estimated.

The 95% figure for the WHERE lever does not hold for storage. Measured, it removes ~66% of the added index, not ~95%:

nodes	A (pre-#518)	B (body, all)	C (Section/Module only)	B − A
100k	7.2 MB	10.2 MB	7.6 MB	+3.0 MB
200k	15.0 MB	19.7 MB	15.8 MB	+4.7 MB
500k	36.6 MB	48.6 MB	40.7 MB	+12.0 MB

At 500k the lever takes +12.0 MB down to +4.1 MB. The 95% is correct for row count — C indexes 17.8k bodies against B's 157k, which is 11% — but storage doesn't follow row count. Markdown section bodies run substantially longer than function docstrings, and the fifth column carries fixed overhead whether or not a given row has prose. So narrowing buys about two thirds, not nineteen twentieths, and it costs function-docstring search. Given your findability-first call I'd read that as weakening the case for the lever, but it's your trade to make and the harness will re-measure it against whatever corpus shape you think is realistic — the distribution constants are at the top of the file.

I'm not quoting timings. My machine shows 39–44% run-to-run spread, which is large next to the delta being measured. The harness detects that and prints a warning rather than letting a noisy column pass as authoritative, so the timing half is there but self-flagged as unreliable here. Size figures are deterministic and unaffected. Your reference-machine run is the one that will produce meaningful wall-clock, and thank you for taking that half.

Two methodology notes, because I got both wrong first and each produced confident-looking nonsense. Running the variants grouped (A,A,A, B,B,B, C,C,C) let drift over the life of the process land entirely on whichever went last, which reported C as slower than B despite C doing strictly less work — variants are now interleaved so drift cancels. And the seed was a unsigned long xorshift, which is 32 bits on a 32-bit target, so it truncated and the three variants weren't seeing identical corpora; it's uint64_t now with a per-variant reset. Both are called out in the file so the next person doesn't reintroduce them.

No rush from my side on slice 2 — it stacks on whatever this lands as, so I'll hold until this has a direction.

[ShauryaaSharma]

My previous comment crossed with your review — I posted it without having read this. Apologies; it reads as if I ignored you. Replying properly now.

I checked every finding against the tree rather than taking them on trust. All six are correct, and the first one is a real hole in the thing I was most confident about.

The delta test doesn't guard the delta call site — you're right

fts_delta_insert_populates_body contains zero references to cbm_delta_patch. It hand-prepares its own INSERT, and as you spotted the copy isn't even faithful — plain name where production uses cbm_camel_split(name).

So revert pipeline_delta.c to four columns and my test still passes. I wrote a test that proves my own reimplementation works, then described it in the PR body as the guard for a bug whose whole signature is "the tested path works." That's the exact trap I claimed to be closing, and I walked into it while writing the test for it.

Fixing it to drive the real merge: build a base index, add nodes, run cbm_delta_patch, then assert fts_match_count finds body text from the newly merged rows. If that's awkward from test_store_search.c it goes where cbm_delta_patch is already exercised. Agreed that a test reimplementing its subject proves the reimplementation.

Fourth write site — and my comment overstates its own coverage

tests/test_mcp.c:2489 confirmed: delete-all plus a four-column INSERT. Valid SQL, nothing red, body left NULL, and it stops mirroring production shape.

The store.h comment says the macro covers every nodes_fts write site, which is now false. I'll route that site through CBM_SQL_FTS_BODY_EXPR rather than narrow the claim — the point of hoisting it was to make "there is only one" true, and a test fixture drifting from production shape is how the next person gets a misleading green run. You're right that a comment overstating its coverage is worse than no comment.

BM25 weights — you asked for my reasoning, and it changes the recommendation slightly

Confirmed: bm25(nodes_fts) at mcp.c:3025 and :3062, no weights, so every column sits at the default 1.0.

One thing worth putting on the record before choosing, because it affects what weights can actually deliver: explicit weights cannot restore the old ordering. FTS5's bm25 normalises by document length against the corpus average. Column weights scale each column's term contributions, but body tokens still count toward row length and therefore toward the corpus average — so avgdl shifts the moment body is populated at all, and every row's normalisation shifts with it, including rows with empty bodies. Even body weighted 0.0 would not pin the ranking, because a zero weight removes the contribution but not the length.

So "additive column, backward compatible" is wrong about ranking in a way weights can't repair, and I should not have written it. The real choice is not whether ranking moves but whether it moves deliberately.

Given that, I agree with your lean, for a slightly different reason than preservation: weights are worth setting because they let us bound how far prose can outrank a symbol match, which is the actual risk. A term in a name is a stronger signal of intent than the same term in a paragraph. I'd propose weighting body below name/qualified_name rather than at parity — findability-first still argues for indexing it, but not for letting a passing mention in prose outrank the function actually named that.

I don't want to pick the numbers unilaterally since you own the ranking model, and I'd rather tune them against the label-boost CASE than guess. Happy to propose a concrete set with before/after orderings on a sample corpus if that's useful. And the Section-now-returnable behavioural note belongs in the description regardless — that was intended, but intended is not the same as documented.

The two weak tests

Both fair.

markdown_section_no_body is vacuous exactly as you say — docstring is NULL for every Section on main, so it passes with my production change reverted. Rewriting it so a sibling section in the same file does have a body while this one does not, which is only true after the change.

markdown_section_body_capped hardcoding 500 reintroduces the parallel-constant coupling I'd just removed from production — that's the same mistake one layer down, and I didn't notice because the test passed. It'll assert against MAX_COMMENT_LEN. And the corpus being "alpha " repeated means the UTF-8 backoff has never executed in any run of this suite; I'll put a multi-byte character straddling the cap boundary, which is the only case that code exists for.

Lint, and what it cost

store.h:361 is a single 274-character line. My continuations collapsed when I generated the header and I didn't re-check the file's column widths afterwards — I checked the benchmark file and assumed the header was fine.

The expensive part is the one you pointed out: lint failing skipped the test job, so none of these seven tests have run in CI even once. Every "verified" claim I've made rests on local runs on a toolchain that stubs TRE regex and can't build the product binary. Splitting that line is the first thing I'll do, so the next push produces a real signal rather than my assurances.

On shipping the harness before your answer

Fair, and I'd rather you said it. I asked whether it was the evidence you wanted specifically so I wouldn't measure the wrong thing twice, and then didn't wait for the answer — which made the asking decorative. Noted, and I'll wait next time.

I'll add the note that scripts/benchmark-fts-body.c is 477 of the 859 lines, so the diff size reads honestly as a measurement tool rather than feature code.

---

Plan, in the order I'll push them: split the header line so CI actually runs, then the delta test against cbm_delta_patch, the two test fixes, the fourth write site, and the description notes. The BM25 weights I'd like your steer on before I touch — it's your ranking model, and it's the one item here that's a design decision rather than a defect.

Thanks for checking the delta test by reading what it does rather than what it's called. That's the finding I'd least have caught myself.

[ShauryaaSharma]

Five of the six items are pushed. The BM25 weights decision is the one left, and it is yours.

Lint, and the signal it was hiding. store.h:361 is split. My first attempt still failed, and the log was precise about why — I had anchored the backslashes on the macro's last line, but clang-format aligns to the longest line carrying a backslash, and a macro's final line carries none. Column 41, not 78, which cross-checks against the existing multi-line macro at cli.c:2983.

CI is green now, all 34 checks, and the test job has run for the first time on this PR — including MSan, TSan and LSan across ubuntu/ubuntu-arm/macOS/Windows. Every claim I made before this rested on local runs against a toolchain that stubs TRE regex and cannot build the product binary, so this is the first real signal any of it has had. You were right that the skipped test job was the expensive part.

The delta test. Replaced with pipeline_delta_merge_indexes_body in test_pipeline.c, which drives cbm_delta_preseed then cbm_delta_patch rather than reimplementing the INSERT. I verified it discriminates rather than assuming: against a four-column revert of pipeline_delta.c the body assertions (canary, rollback) fail while the name assertion (deployment) still passes — the node merged, only its body was lost, which is exactly the regression.

Driving the production path immediately paid for itself by surfacing something the hand-rolled version could not: cbm_delta_preseed lifts the gbuf id watermark above MAX(id), so a node added to the gbuf before that call keeps a low temp id and is silently never merged by the id > max_db_id predicate. My first attempt did exactly that and merged zero nodes. Documented in the test.

The fourth write site. Rather than routing test_mcp.c:2489 through the macro, it now calls cbm_store_fts_rebuild — the production function. A fixture that spells out its own column list has to be remembered on every schema change; one that calls the rebuild cannot drift. That was the last nodes_fts INSERT outside the store and the delta patch, so the store.h comment no longer overstates its coverage, and I reworded it to say what is actually true.

The two weak tests. markdown_section_no_body now extracts two sections and asserts the documented sibling has prose while the bare one does not — a pair only true once capture works and stops at the heading boundary. markdown_section_body_capped asserts against CBM_MAX_COMMENT_LEN, which I promoted into cbm.h so production and tests share one definition instead of the test copying the literal; a test-layer copy is the same coupling this branch removed from production. Its corpus is now 498 ASCII bytes followed by U+20AC so that character straddles offsets 498–500. Measured result is 498 bytes, not 500 — the backoff branch executes, which it never did once under the old all-ASCII corpus.

BM25 weights — yours to call, with one correction that narrows it.

Confirmed no column weights at mcp.c:3025 and :3062. But explicit weights cannot restore the old ordering, which I think changes the decision: FTS5 normalises by document length against the corpus average, and body tokens count toward row length regardless of weight. avgdl therefore moves the moment body is populated, and every row's normalisation moves with it — including empty-body rows. Even body at 0.0 removes the contribution but not the length.

So "additive column, backward compatible" was wrong about ranking in a way weights cannot repair, and I have corrected that in the description rather than leaving it standing. The real choice is not whether scores move but whether they move deliberately.

That is why I still agree with your lean, for a different reason than preservation: weights are worth setting to bound how far prose can outrank a symbol match. A term in a name is a stronger signal of intent than the same term in a paragraph, so I would put body below name/qualified_name rather than at parity. I have not picked numbers — it is your ranking model and it interacts with the label-boost CASE. Happy to propose a set with before/after orderings on a sample corpus if that is useful.

Description now carries the Section-returnable and ranking-shift notes as behavioural changes rather than burying them, and says plainly that the harness is 477 of the ~934 added lines so the diff size reads honestly.

On shipping the harness before your answer — fair, and noted. I asked precisely so I would not measure the wrong thing twice, then did not wait, which made the asking decorative.