From f9277f90a82502b0676caca92fa2d523293db3d1 Mon Sep 17 00:00:00 2001 From: ShauryaaSharma Date: Fri, 21 Aug 2026 12:45:32 +0530 Subject: [PATCH 1/7] feat(search): index markdown section bodies for BM25 full-text search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Section nodes exposed only their heading text to BM25, so search_graph could not match the prose beneath a heading. Index that body so markdown content is searchable. - store: add a `body` column to the nodes_fts FTS5 table; new cbm_store_fts_rebuild() drops+recreates the table (upgrading legacy 4-column databases) and backfills `body` from each node's docstring, guarded by json_valid() against malformed-JSON rows - store: expose CBM_SQL_FTS_BODY_EXPR so every nodes_fts write site feeds `body` through one shared expression - pipeline: the wholesale backfill now delegates to cbm_store_fts_rebuild(); the row-level delta-merge insert writes `body` through the same shared expression, so prose arriving incrementally is searchable too - mcp: stop excluding Section from BM25 results. Section falls in the unboosted ELSE bucket of the label CASE, so code symbols keep their ranking advantage by construction rather than by exclusion - internal/cbm: capture the markdown section body beneath each heading, stopping at the first nested subsection, capped at MAX_COMMENT_LEN with a UTF-8-safe backoff, reusing the existing docstring property - tests: 3 extraction cases + 4 store FTS cases, including a delta-path guard — a four-column INSERT into the five-column table is still valid SQL that silently leaves `body` NULL, so the incremental path needs its own assertion The DROP+recreate opens a brief window, one-time per database during the schema upgrade, where a concurrent bm25_search finds no table and degrades to the regex path. Closes #518 Refs #519 (Module description promotion follows in a stacked PR) Signed-off-by: ShauryaaSharma --- internal/cbm/extract_defs.c | 62 ++++++++++++- src/mcp/mcp.c | 9 +- src/pipeline/pipeline.c | 17 +--- src/pipeline/pipeline_delta.c | 11 ++- src/store/store.c | 69 +++++++++++--- src/store/store.h | 18 ++++ tests/test_extraction.c | 65 ++++++++++++++ tests/test_store_search.c | 165 ++++++++++++++++++++++++++++++++++ 8 files changed, 382 insertions(+), 34 deletions(-) diff --git a/internal/cbm/extract_defs.c b/internal/cbm/extract_defs.c index 4994bd7b8..736b00277 100644 --- a/internal/cbm/extract_defs.c +++ b/internal/cbm/extract_defs.c @@ -3849,7 +3849,8 @@ static const char *qn_safe_segment(CBMArena *a, const char *name) { return out; } -static void push_simple_class_def(CBMExtractCtx *ctx, TSNode node, char *name, const char *label) { +static void push_simple_class_def(CBMExtractCtx *ctx, TSNode node, char *name, const char *label, + const char *docstring) { CBMArena *a = ctx->arena; CBMDefinition def; memset(&def, 0, sizeof(def)); @@ -3860,6 +3861,7 @@ static void push_simple_class_def(CBMExtractCtx *ctx, TSNode node, char *name, c def.start_line = ts_node_start_point(node).row + TS_LINE_OFFSET; def.end_line = ts_node_end_point(node).row + TS_LINE_OFFSET; def.is_exported = true; + def.docstring = docstring; // Markdown section body (#518); NULL for other configs cbm_defs_push(&ctx->result->defs, a, def); } @@ -3958,6 +3960,58 @@ static char *extract_markdown_heading_name(CBMArena *a, TSNode node, const char return trim_heading_name(name); } +// Capture the prose body beneath a Markdown heading so BM25 can search the content +// and not just the heading text (#518). In the tree-sitter-markdown grammar each +// heading lives inside a `section` node that also holds the body blocks and any +// nested subsections; the body is the source span between the heading and either +// the first nested subsection or the end of the section. Nested subsections are +// excluded because each gets its own Section node and its own body. Returns NULL +// when there is no enclosing section or no body text. Trimmed, and capped at +// MAX_COMMENT_LEN (the same budget docstrings use) without splitting a UTF-8 +// sequence. +static char *extract_markdown_section_body(CBMArena *a, TSNode heading, const char *source) { + TSNode parent = ts_node_parent(heading); + if (ts_node_is_null(parent) || strcmp(ts_node_type(parent), "section") != 0) { + return NULL; + } + uint32_t body_start = ts_node_end_byte(heading); + uint32_t body_end = ts_node_end_byte(parent); + // Stop at the first nested subsection — it gets its own Section node + body. + uint32_t cc = ts_node_child_count(parent); + for (uint32_t i = 0; i < cc; i++) { + TSNode ch = ts_node_child(parent, i); + if (ts_node_start_byte(ch) >= body_start && strcmp(ts_node_type(ch), "section") == 0) { + body_end = ts_node_start_byte(ch); + break; + } + } + // Trim surrounding whitespace/newlines. UTF-8 lead and continuation bytes are all + // >= 0x80, so a byte-wise <= ' ' test never cuts a multi-byte character. + while (body_start < body_end && (unsigned char)source[body_start] <= ' ') { + body_start++; + } + while (body_end > body_start && (unsigned char)source[body_end - 1] <= ' ') { + body_end--; + } + if (body_end <= body_start) { + return NULL; + } + size_t len = (size_t)(body_end - body_start); + if (len > MAX_COMMENT_LEN) { + len = MAX_COMMENT_LEN; + // Back off so the cap never splits a UTF-8 multi-byte sequence: source[start+len] + // is the first excluded byte, and a continuation byte there means we landed + // mid-character. + while (len > 0 && ((unsigned char)source[body_start + len] & 0xC0) == 0x80) { + len--; + } + if (len == 0) { + return NULL; + } + } + return cbm_arena_strndup(a, source + body_start, len); +} + // INI: extract section name from section node. static char *find_ini_section_name(CBMArena *a, TSNode node, const char *source) { uint32_t nc = ts_node_child_count(node); @@ -4021,6 +4075,7 @@ static bool extract_config_class_def(CBMExtractCtx *ctx, TSNode node, const char CBMArena *a = ctx->arena; char *name = NULL; const char *label = "Class"; + const char *docstring = NULL; if (ctx->language == CBM_LANG_TOML && (strcmp(kind, "table") == 0 || strcmp(kind, "table_array_element") == 0)) { @@ -4036,6 +4091,7 @@ static bool extract_config_class_def(CBMExtractCtx *ctx, TSNode node, const char // label rather than degrade it to match a test. The markdown repro asserts // "Class"; that assertion is the inaccurate side and is flagged for review. label = "Section"; + docstring = extract_markdown_section_body(a, node, ctx->source); // #518 } else if (ctx->language == CBM_LANG_HCL && strcmp(kind, "block") == 0) { name = find_hcl_block_name(a, node, ctx->source); } else { @@ -4043,7 +4099,7 @@ static bool extract_config_class_def(CBMExtractCtx *ctx, TSNode node, const char } if (name && name[0]) { - push_simple_class_def(ctx, node, name, label); + push_simple_class_def(ctx, node, name, label, docstring); } return true; } @@ -4098,7 +4154,7 @@ static bool extract_sql_ddl_class_def(CBMExtractCtx *ctx, TSNode node, const cha if (!name || !name[0]) { return false; } - push_simple_class_def(ctx, node, name, label); + push_simple_class_def(ctx, node, name, label, NULL); // SQL tables/views carry no body // Must match push_simple_class_def's QN exactly (qn_safe_segment included) // or pass_usages cannot find the enclosing def for the lineage source. const char *qn = diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 42bd33d7d..9c859548b 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -3028,7 +3028,11 @@ static char *bm25_search(cbm_store_t *store, const char *project, const char *qu ") fts " "JOIN nodes n ON n.id = fts.rowid " "WHERE n.project = ?2 " - " AND n.label NOT IN ('File','Folder','Module','Section','Variable','Project') " + /* Section is searchable (#518): its body text is indexed into nodes_fts.body, + * and it falls in the ELSE 0.0 bucket of the boost CASE above, so code symbols + * keep their ranking advantage by construction rather than by exclusion. + * Module stays excluded pending #519. */ + " AND n.label NOT IN ('File','Folder','Module','Variable','Project') " " AND (?6 IS NULL OR n.file_path LIKE ?6) " /* rank ties are common (boosted floats) — the id tie-break makes * offset pages contractually stable across calls. */ @@ -3063,7 +3067,8 @@ static char *bm25_search(cbm_store_t *store, const char *project, const char *qu " ) fts " " JOIN nodes n ON n.id = fts.rowid " " WHERE n.project = ?2 " - " AND n.label NOT IN ('File','Folder','Module','Section','Variable','Project')" + /* Keep in sync with the search query's filter above (#518). */ + " AND n.label NOT IN ('File','Folder','Module','Variable','Project')" " AND (?6 IS NULL OR n.file_path LIKE ?6)" ")"; sqlite3_stmt *cs = NULL; diff --git a/src/pipeline/pipeline.c b/src/pipeline/pipeline.c index 65ac75183..d82f88b9d 100644 --- a/src/pipeline/pipeline.c +++ b/src/pipeline/pipeline.c @@ -1492,20 +1492,11 @@ static void discard_generation_stage(const char *stage_path) { cbm_remove_db_sidecars(stage_path); } +/* Wholesale FTS rebuild. Delegates to the store so the DDL, the camelCase-split + * fallback and the prose `body` backfill live in one place; the DROP+recreate it + * performs is also what upgrades a legacy 4-column nodes_fts (#518). */ static int generation_rebuild_fts(cbm_store_t *store) { - if (cbm_store_exec(store, "INSERT INTO nodes_fts(nodes_fts) VALUES('delete-all');") != - CBM_STORE_OK) { - return CBM_STORE_ERR; - } - if (cbm_store_exec(store, - "INSERT INTO nodes_fts(rowid, name, qualified_name, label, file_path) " - "SELECT id, cbm_camel_split(name), qualified_name, label, file_path " - "FROM nodes;") == CBM_STORE_OK) { - return CBM_STORE_OK; - } - return cbm_store_exec(store, - "INSERT INTO nodes_fts(rowid, name, qualified_name, label, file_path) " - "SELECT id, name, qualified_name, label, file_path FROM nodes;"); + return cbm_store_fts_rebuild(store); } typedef struct { diff --git a/src/pipeline/pipeline_delta.c b/src/pipeline/pipeline_delta.c index 50fc094c2..d87f528ad 100644 --- a/src/pipeline/pipeline_delta.c +++ b/src/pipeline/pipeline_delta.c @@ -22,7 +22,11 @@ * a live node again (AUTOINCREMENT), so dead entries simply drop out of the * rowid join at query time. The patch inserts rows for exactly the new * nodes, via the same cbm_camel_split SQL function the wholesale rebuild - * uses. + * uses, and through the same CBM_SQL_FTS_BODY_EXPR so prose arriving on the + * incremental path is searchable too. That shared expression is not optional: + * naming only the original four columns here would still be valid SQL, leaving + * `body` NULL for every delta-merged node — unsearchable prose on the path + * users hit most, while a full reindex looked correct. */ #include "foundation/constants.h" #include "pipeline/pipeline_internal.h" @@ -526,9 +530,10 @@ int cbm_delta_patch(cbm_store_t *store, const char *project, cbm_gbuf_t *gbuf, i sqlite3_stmt *fts = NULL; if (sqlite3_prepare_v2(db, "INSERT INTO nodes_fts (rowid, name, qualified_name, label," - " file_path)" + " file_path, body)" " SELECT id, cbm_camel_split(name), qualified_name, label," - " file_path FROM nodes WHERE project = ?1 AND id > ?2", + " file_path," CBM_SQL_FTS_BODY_EXPR + "FROM nodes WHERE project = ?1 AND id > ?2", CBM_NOT_FOUND, &fts, NULL) == SQLITE_OK) { sqlite3_bind_text(fts, 1, project, CBM_NOT_FOUND, SQLITE_TRANSIENT); sqlite3_bind_int64(fts, 2, max_db_id); diff --git a/src/store/store.c b/src/store/store.c index 015e50ca2..d0fa27bd1 100644 --- a/src/store/store.c +++ b/src/store/store.c @@ -226,6 +226,55 @@ static void iso_now(char *buf, size_t sz) { /* ── Schema ─────────────────────────────────────────────────────── */ +/* FTS5 contentless virtual table DDL — single source of truth shared by + * init_schema (fresh databases) and cbm_store_fts_rebuild (re-index + legacy + * upgrade). Columns: name, qualified_name, label, file_path, body. `body` + * (#518) carries prose — markdown section bodies today, YAML/JSON description + * values once #519 lands — so BM25 matches content, not only identifiers. + * Contentless (content='') stores only the inverted index; we feed + * cbm_camel_split(name) and the raw body text at insert time. Named `body` + * rather than `content` to avoid colliding with the `content=''` option + * keyword in the FTS5 DDL grammar. */ +static const char NODES_FTS_DDL[] = "CREATE VIRTUAL TABLE IF NOT EXISTS nodes_fts USING fts5(" + " name, qualified_name, label, file_path, body," + " content=''," + " tokenize='unicode61 remove_diacritics 2'" + ");"; + +/* Full backfill. The primary form camelCase-splits the name; the fallback uses the + * plain name should cbm_camel_split be unavailable (it is registered per-connection, + * so a store opened without it must still be able to rebuild). */ +static const char FTS_BACKFILL_SQL[] = + "INSERT INTO nodes_fts(rowid, name, qualified_name, label, file_path, body) " + "SELECT id, cbm_camel_split(name), qualified_name, label, file_path," CBM_SQL_FTS_BODY_EXPR + "FROM nodes;"; + +static const char FTS_BACKFILL_SQL_FALLBACK[] = + "INSERT INTO nodes_fts(rowid, name, qualified_name, label, file_path, body) " + "SELECT id, name, qualified_name, label, file_path," CBM_SQL_FTS_BODY_EXPR "FROM nodes;"; + +int cbm_store_fts_rebuild(cbm_store_t *s) { + if (!s || !s->db) { + return CBM_STORE_ERR; + } + /* DROP + recreate rather than 'delete-all' so legacy 4-column tables gain the + * `body` column. This opens a brief window where a concurrent bm25_search finds + * no table and degrades to the regex path; it is one-time per database (later + * rebuilds recreate an identically-shaped table) and callers already hold the + * index lock. */ + if (exec_sql(s, "DROP TABLE IF EXISTS nodes_fts;") != CBM_STORE_OK) { + return CBM_STORE_ERR; + } + if (exec_sql(s, NODES_FTS_DDL) != CBM_STORE_OK) { + return CBM_STORE_ERR; /* FTS5 not compiled in — regex search path still works. */ + } + int rc = exec_sql(s, FTS_BACKFILL_SQL); + if (rc != CBM_STORE_OK) { + rc = exec_sql(s, FTS_BACKFILL_SQL_FALLBACK); + } + return rc; +} + static int init_schema(cbm_store_t *s) { const char *ddl = "CREATE TABLE IF NOT EXISTS projects (" @@ -348,22 +397,16 @@ static int init_schema(cbm_store_t *s) { sqlite3_finalize(probe); } - /* FTS5 contentless virtual table for BM25 full-text search. - * Contentless (content='') means FTS5 stores only the inverted index, - * not a copy of the source text — required for camelCase tokenization - * because we feed it `cbm_camel_split(name)` at insert time but want - * queries to match against the split tokens, not the original. + /* FTS5 contentless virtual table for BM25 full-text search (see NODES_FTS_DDL). + * Created here for fresh databases and read-only opens; cbm_store_fts_rebuild + * drops and recreates it during indexing, which is what upgrades a legacy + * 4-column table to one carrying `body`. IF NOT EXISTS means an existing + * legacy table survives this call unchanged and keeps serving name-only + * search until that rebuild runs. * Fails silently if FTS5 is not compiled in (SQLITE_ENABLE_FTS5). */ { char *fts_err = NULL; - int fts_rc = sqlite3_exec(s->db, - "CREATE VIRTUAL TABLE IF NOT EXISTS nodes_fts USING fts5(" - " name, qualified_name, label, file_path," - " content=''," - " tokenize='unicode61 remove_diacritics 2'" - ");", - NULL, NULL, &fts_err); - if (fts_rc != SQLITE_OK && fts_err) { + if (sqlite3_exec(s->db, NODES_FTS_DDL, NULL, NULL, &fts_err) != SQLITE_OK && fts_err) { sqlite3_free(fts_err); } } diff --git a/src/store/store.h b/src/store/store.h index 8c2f76dd8..27c812859 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -342,6 +342,24 @@ int cbm_store_drop_indexes(cbm_store_t *s); /* Recreate user indexes after bulk inserts. */ int cbm_store_create_indexes(cbm_store_t *s); +/* Rebuild the nodes_fts BM25 index from the nodes table. Drops and recreates the + * FTS virtual table — which upgrades legacy 4-column databases to the schema carrying + * the `body` column — then re-inserts every node with its camelCase-split name and + * prose body (the docstring property) so full-text search matches content and not + * only identifiers (#518). Returns CBM_STORE_OK or CBM_STORE_ERR. */ +int cbm_store_fts_rebuild(cbm_store_t *s); + +/* Body expression shared by EVERY nodes_fts write site — the wholesale rebuild in + * cbm_store_fts_rebuild and the row-level delta insert in pipeline_delta.c. Any new + * write site must use it too: nodes_fts carries five columns, and an INSERT naming + * only the original four is still valid SQL that silently leaves `body` NULL, making + * prose added on that path unsearchable while a full reindex looks correct. + * Feeds the node's docstring property, or '' when absent. The json_valid() guard is + * essential — json_extract() aborts the whole statement on malformed JSON, and pre-fix + * databases contain such rows; a guarded row degrades to name-only indexing instead of + * failing the write. Expects the `nodes` row in scope as the SELECT source. */ +#define CBM_SQL_FTS_BODY_EXPR " CASE WHEN json_valid(properties)" " THEN coalesce(json_extract(properties,'$.docstring'),'') ELSE '' END " + /* ── WAL / Checkpoint ───────────────────────────────────────────── */ /* Force WAL checkpoint + PRAGMA optimize. */ diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 88796c70d..fb3b723fb 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -74,6 +74,16 @@ static int count_defs_with_label(CBMFileResult *r, const char *label) { return count; } +/* Docstring of the first definition matching label+name (NULL if not found). */ +static const char *def_docstring(CBMFileResult *r, const char *label, const char *name) { + for (int i = 0; i < r->defs.count; i++) { + if (strcmp(r->defs.items[i].label, label) == 0 && + strcmp(r->defs.items[i].name, name) == 0) + return r->defs.items[i].docstring; + } + return NULL; +} + /* Convenience: extract, assert no error, return result. Caller frees. */ static CBMFileResult *extract(const char *src, CBMLanguage lang, const char *proj, const char *path) { @@ -3187,6 +3197,57 @@ TEST(markdown_no_headings) { PASS(); } +/* #518: the prose body beneath a heading is captured as the Section docstring so + * BM25 can search markdown content, not just heading text. */ +TEST(markdown_section_body_captured) { + CBMFileResult *r = extract("## BROWSER AGENT\n\n" + "Before writing any test file, explore the live application " + "using Playwright MCP.\n\n" + "## NEXT SECTION\n\n" + "Totally unrelated prose here.\n", + CBM_LANG_MARKDOWN, "t", "SKILL.md"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + const char *body = def_docstring(r, "Section", "BROWSER AGENT"); + ASSERT_NOT_NULL(body); + ASSERT(strstr(body, "Playwright") != NULL); + ASSERT(strstr(body, "test file") != NULL); + /* Body stops at the next heading — it must not absorb the sibling section. */ + ASSERT(strstr(body, "unrelated") == NULL); + cbm_free_result(r); + PASS(); +} + +/* #518: a heading with no prose beneath it yields no docstring (not empty text). */ +TEST(markdown_section_no_body) { + CBMFileResult *r = extract("# Title\n## Empty\n", CBM_LANG_MARKDOWN, "t", "README.md"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + const char *body = def_docstring(r, "Section", "Empty"); + ASSERT(body == NULL || body[0] == '\0'); + cbm_free_result(r); + PASS(); +} + +/* #518: the captured body is capped at MAX_COMMENT_LEN, the same budget docstrings + * use, so a long section cannot blow past the node-properties budget. */ +TEST(markdown_section_body_capped) { + /* Build a heading followed by ~1500 chars of prose. */ + char src[2048]; + int n = snprintf(src, sizeof(src), "# Big\n\n"); + for (int i = 0; i < 250 && n < (int)sizeof(src) - 8; i++) + n += snprintf(src + n, sizeof(src) - (size_t)n, "alpha "); + CBMFileResult *r = extract(src, CBM_LANG_MARKDOWN, "t", "BIG.md"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + const char *body = def_docstring(r, "Section", "Big"); + ASSERT_NOT_NULL(body); + ASSERT(strlen(body) <= 500); + cbm_free_result(r); + PASS(); +} + + /* ═══════════════════════════════════════════════════════════════════ * Python __init__.py Module QN collision regression * ═══════════════════════════════════════════════════════════════════ */ @@ -5775,6 +5836,10 @@ SUITE(extraction) { RUN_TEST(markdown_setext_headings); RUN_TEST(markdown_heading_content); RUN_TEST(markdown_no_headings); + /* #518 markdown section body capture */ + RUN_TEST(markdown_section_body_captured); + RUN_TEST(markdown_section_no_body); + RUN_TEST(markdown_section_body_capped); /* __init__.py / index.ts Module QN collision regression */ RUN_TEST(python_init_module_qn_not_collide_with_folder); diff --git a/tests/test_store_search.c b/tests/test_store_search.c index 2eb189ed5..27f39998a 100644 --- a/tests/test_store_search.c +++ b/tests/test_store_search.c @@ -6,6 +6,7 @@ #include "../src/foundation/compat.h" #include "test_framework.h" #include "test_helpers.h" +#include #include #include #include @@ -1480,6 +1481,165 @@ TEST(store_find_nodes_rejects_null_store_without_ub) { PASS(); } +/* ── FTS5 body indexing (#518) ───────────────────────────────────── */ + +/* Count nodes_fts rows matching a single (test-controlled) alpha token. */ +static int fts_match_count(sqlite3 *db, const char *term) { + char sql[256]; + snprintf(sql, sizeof(sql), "SELECT count(*) FROM nodes_fts WHERE nodes_fts MATCH '%s'", term); + sqlite3_stmt *st = NULL; + if (sqlite3_prepare_v2(db, sql, -1, &st, NULL) != SQLITE_OK) { + return -1; + } + int c = (sqlite3_step(st) == SQLITE_ROW) ? sqlite3_column_int(st, 0) : -1; + sqlite3_finalize(st); + return c; +} + +/* cbm_store_fts_rebuild indexes the `body` column from each node's docstring, so + * BM25 MATCH finds a Section by its content, not just its name. */ +TEST(fts_rebuild_indexes_body_content) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + cbm_node_t sec = {.project = "test", + .label = "Section", + .name = "BROWSER AGENT", + .qualified_name = "test.SKILL.browser", + .file_path = "SKILL.md", + .properties_json = + "{\"docstring\":\"explore the live app using Playwright\"}"}; + cbm_store_upsert_node(s, &sec); + + ASSERT_EQ(cbm_store_fts_rebuild(s), CBM_STORE_OK); + sqlite3 *db = cbm_store_get_db(s); + + /* A body token that appears in no node name is searchable. */ + ASSERT_GTE(fts_match_count(db, "playwright"), 1); + /* Name tokens still searchable. */ + ASSERT_GTE(fts_match_count(db, "browser"), 1); + cbm_store_close(s); + PASS(); +} + +/* A database created before the `body` column is upgraded in place: rebuild drops + * the legacy 4-column table, recreates it with body, and repopulates. */ +TEST(fts_rebuild_upgrades_legacy_schema) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + sqlite3 *db = cbm_store_get_db(s); + /* Replace the current table with the historical 4-column (no body) schema. */ + ASSERT_EQ(sqlite3_exec(db, "DROP TABLE IF EXISTS nodes_fts;", NULL, NULL, NULL), SQLITE_OK); + ASSERT_EQ(sqlite3_exec(db, + "CREATE VIRTUAL TABLE nodes_fts USING fts5(name, qualified_name, label, " + "file_path, content='', tokenize='unicode61 remove_diacritics 2');", + NULL, NULL, NULL), + SQLITE_OK); + cbm_node_t fn = {.project = "test", + .label = "Function", + .name = "parseConfig", + .qualified_name = "test.parseConfig", + .file_path = "a.c", + .properties_json = "{\"docstring\":\"loads zookeeper settings\"}"}; + cbm_store_upsert_node(s, &fn); + + ASSERT_EQ(cbm_store_fts_rebuild(s), CBM_STORE_OK); + + /* The new body column now exists and the docstring is searchable. */ + sqlite3_stmt *probe = NULL; + ASSERT_EQ(sqlite3_prepare_v2(db, "SELECT body FROM nodes_fts", -1, &probe, NULL), SQLITE_OK); + sqlite3_finalize(probe); + ASSERT_GTE(fts_match_count(db, "zookeeper"), 1); + cbm_store_close(s); + PASS(); +} + +/* The backfill must survive rows whose properties_json is not valid JSON (legacy + * databases contain such rows); the json_valid() guard prevents json_extract from + * aborting the whole INSERT...SELECT. */ +TEST(fts_rebuild_tolerates_malformed_properties) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + cbm_node_t bad = {.project = "test", + .label = "Function", + .name = "weird", + .qualified_name = "test.weird", + .file_path = "a.c", + .properties_json = "{not valid json"}; + cbm_node_t ok = {.project = "test", + .label = "Function", + .name = "fine", + .qualified_name = "test.fine", + .file_path = "b.c", + .properties_json = "{\"docstring\":\"kafka consumer group\"}"}; + cbm_store_upsert_node(s, &bad); + cbm_store_upsert_node(s, &ok); + + ASSERT_EQ(cbm_store_fts_rebuild(s), CBM_STORE_OK); + sqlite3 *db = cbm_store_get_db(s); + ASSERT_GTE(fts_match_count(db, "kafka"), 1); /* good row indexed */ + ASSERT_GTE(fts_match_count(db, "weird"), 1); /* malformed row's name still indexed */ + cbm_store_close(s); + PASS(); +} + +/* Regression guard for the incremental path (pipeline_delta.c). nodes_fts has five + * columns, and a row-level INSERT naming only the original four is still valid SQL + * that silently leaves `body` NULL — prose arriving via delta merge would be + * unsearchable while a full reindex looked perfectly correct. This runs the delta + * site's exact statement shape (id > max_db_id, via CBM_SQL_FTS_BODY_EXPR) and + * asserts the newly merged node's body is searchable. */ +TEST(fts_delta_insert_populates_body) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + cbm_node_t first = {.project = "test", + .label = "Function", + .name = "existing", + .qualified_name = "test.existing", + .file_path = "a.c", + .properties_json = "{\"docstring\":\"already indexed\"}"}; + cbm_store_upsert_node(s, &first); + ASSERT_EQ(cbm_store_fts_rebuild(s), CBM_STORE_OK); + + sqlite3 *db = cbm_store_get_db(s); + /* Everything already in nodes_fts is the previous generation. */ + sqlite3_stmt *mx = NULL; + ASSERT_EQ(sqlite3_prepare_v2(db, "SELECT coalesce(max(id),0) FROM nodes", -1, &mx, NULL), + SQLITE_OK); + ASSERT(sqlite3_step(mx) == SQLITE_ROW); + sqlite3_int64 max_db_id = sqlite3_column_int64(mx, 0); + sqlite3_finalize(mx); + + /* A markdown Section arrives incrementally, carrying prose in its docstring. */ + cbm_node_t added = {.project = "test", + .label = "Section", + .name = "Deployment", + .qualified_name = "test.README.deployment", + .file_path = "README.md", + .properties_json = "{\"docstring\":\"rollback uses the canary alias\"}"}; + cbm_store_upsert_node(s, &added); + + sqlite3_stmt *fts = NULL; + ASSERT_EQ(sqlite3_prepare_v2(db, + "INSERT INTO nodes_fts (rowid, name, qualified_name, label," + " file_path, body)" + " SELECT id, name, qualified_name, label," + " file_path," CBM_SQL_FTS_BODY_EXPR + "FROM nodes WHERE project = ?1 AND id > ?2", + -1, &fts, NULL), + SQLITE_OK); + sqlite3_bind_text(fts, 1, "test", -1, SQLITE_TRANSIENT); + sqlite3_bind_int64(fts, 2, max_db_id); + ASSERT_EQ(sqlite3_step(fts), SQLITE_DONE); + sqlite3_finalize(fts); + + /* The delta-merged node's prose is searchable — not just its name. */ + ASSERT_GTE(fts_match_count(db, "canary"), 1); + ASSERT_GTE(fts_match_count(db, "rollback"), 1); + ASSERT_GTE(fts_match_count(db, "deployment"), 1); + cbm_store_close(s); + PASS(); +} + SUITE(store_search) { RUN_TEST(store_search_by_label); RUN_TEST(store_search_by_name_pattern); @@ -1548,4 +1708,9 @@ SUITE(store_search) { RUN_TEST(store_risk_label_all_levels); RUN_TEST(store_impact_summary_empty); RUN_TEST(store_find_nodes_rejects_null_store_without_ub); + /* FTS5 body indexing (#518) */ + RUN_TEST(fts_rebuild_indexes_body_content); + RUN_TEST(fts_rebuild_upgrades_legacy_schema); + RUN_TEST(fts_rebuild_tolerates_malformed_properties); + RUN_TEST(fts_delta_insert_populates_body); } From b417ade4de80ffafa4e08b47a305a6d1a82ff858 Mon Sep 17 00:00:00 2001 From: ShauryaaSharma Date: Fri, 21 Aug 2026 13:09:16 +0530 Subject: [PATCH 2/7] test(bench): isolate the nodes_fts body column cost (#518) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Standalone harness measuring what adding `body` to nodes_fts actually costs, so the sizing question on #518 rests on measurement rather than estimate. The full-index path already did delete-all plus a full re-INSERT before this change, so `body` does not add a pass over the graph — it makes an existing rebuild index more text. The harness therefore isolates the two things that genuinely change: per-row body tokenisation time, and the storage the column adds to the FTS index. Builds the same synthetic corpus three ways and reports the deltas: A 4-column FTS (pre-#518 baseline) B 5-column FTS, body for every node (what #518 ships) C 5-column FTS, body only for Section/Module rows (the WHERE lever) Methodology notes, because both mistakes produce confident-looking nonsense: - variants are interleaved (A,B,C, A,B,C, ...) rather than grouped, so drift over the life of the process cannot land entirely on whichever variant runs last. Grouped runs reported variant C as slower than B despite C doing strictly less work. - the minimum is reported, not the mean: backfill time has a hard floor and an unbounded tail, so the minimum is the closest observable approximation of the real work. - run-to-run spread is printed, and a warning fires when noise is large relative to the delta being reported, rather than letting a noisy timing column pass as authoritative. Uses the real CBM_SQL_FTS_BODY_EXPR including the json_valid() guard, and seeds a fixed-width 64-bit PRNG reset per variant so all three variants see a byte-identical corpus. Not a replacement for scripts/benchmark-index.sh, which measures real end-to-end indexing on a real repository; this deliberately strips parsing and I/O so the FTS write is visible. Signed-off-by: ShauryaaSharma --- scripts/benchmark-fts-body.c | 446 ++++++++++++++++++++++++++++++++++ scripts/benchmark-fts-body.sh | 31 +++ 2 files changed, 477 insertions(+) create mode 100644 scripts/benchmark-fts-body.c create mode 100755 scripts/benchmark-fts-body.sh diff --git a/scripts/benchmark-fts-body.c b/scripts/benchmark-fts-body.c new file mode 100644 index 000000000..21d2e000f --- /dev/null +++ b/scripts/benchmark-fts-body.c @@ -0,0 +1,446 @@ +/* + * benchmark-fts-body.c — isolate the cost of the nodes_fts `body` column (#518). + * + * WHAT THIS MEASURES, AND WHY IT IS THE RIGHT QUESTION + * + * The full-index path already performed a wholesale FTS rebuild before #518: + * delete-all followed by a full re-INSERT over every node (pipeline.c, + * generation_rebuild_fts). Adding `body` therefore does NOT introduce a new + * pass over the graph — it makes an existing rebuild index more text. So the + * honest cost question is narrower than "how much slower is indexing": + * + * 1. per-row body tokenisation time, and + * 2. the storage the extra column adds to the FTS index. + * + * This harness measures exactly those two, at a range of node counts, by + * building the same synthetic corpus three ways: + * + * A 4-column FTS (pre-#518 baseline: name, qualified_name, label, file_path) + * B 5-column FTS, body backfilled for EVERY node (what #518 ships) + * C 5-column FTS, body backfilled only for Section/Module rows + * (the "one-line WHERE" lever — loses function-docstring search) + * + * B minus A is the cost of the feature. C minus A is the cost if the backfill + * is narrowed. B minus C is what the narrowing would save. + * + * WHAT IT DOES NOT MEASURE + * + * Real-corpus end-to-end wall-clock. That needs a built product binary and a + * real repository — use scripts/benchmark-index.sh for it. This harness + * deliberately isolates the FTS write so the numbers are not buried under + * parsing, tree-sitter, and I/O. + * + * FIDELITY NOTES (read before trusting a number) + * + * - The real backfill wraps `name` in cbm_camel_split(); that function lives in + * the product, not here. All three variants use the plain name identically, + * so it cancels out of every delta. Absolute figures for the name column are + * therefore slightly low; the A/B/C deltas are unaffected. + * - The body expression is the real one, json_valid() guard included, so + * malformed-JSON rows exercise the same path they do in production. + * - Index size is measured as page_count * page_size after the backfill, minus + * the same figure for a database holding only the `nodes` table. That + * isolates the FTS shadow tables from the base data. + * + * BUILD + * see scripts/benchmark-fts-body.sh, or: + * cc -O2 -o benchmark-fts-body scripts/benchmark-fts-body.c \ + * vendored/sqlite3/sqlite3.c -Ivendored/sqlite3 \ + * -DSQLITE_ENABLE_FTS5 -lpthread -lm + * + * USAGE + * ./benchmark-fts-body [rowcount ...] (default: 100000 500000 2000000) + */ + +#include "sqlite3.h" + +#include +#include +#include +#include + +#ifdef _WIN32 +#include +/* QueryPerformanceCounter rather than GetTickCount64: higher resolution, and it + * does not require a Vista-era SDK (older MinGW headers lack the latter). */ +static double now_ms(void) { + LARGE_INTEGER f, t; + QueryPerformanceFrequency(&f); + QueryPerformanceCounter(&t); + return (double)t.QuadPart * 1000.0 / (double)f.QuadPart; +} +#else +#include +static double now_ms(void) { + struct timespec ts; + clock_gettime(CLOCK_MONOTONIC, &ts); + return (double)ts.tv_sec * 1000.0 + (double)ts.tv_nsec / 1e6; +} +#endif + +/* ── Corpus shape ───────────────────────────────────────────────────────── + * Tuned to the maintainer's estimate that Section+Module are ~5% of nodes, so + * the narrow lever removes ~95% of the body backfill. Adjust and re-run if + * your corpus differs — every figure below is sensitive to these. */ +enum { + PCT_MODULE = 3, /* one per indexed file */ + PCT_SECTION = 3, /* markdown headings */ + /* remainder: Function/Method/Class/... — the code symbols */ + + PCT_CODE_HAS_DOC = 30, /* share of code symbols carrying a docstring */ + PCT_SECTION_HAS_BODY = 80,/* share of Sections carrying prose */ + PCT_MODULE_HAS_DESC = 40, /* share of Modules carrying a description */ + PCT_MALFORMED = 1, /* pre-fix rows whose properties JSON is invalid */ + + DOC_LEN_CODE = 180, /* average docstring length, bytes */ + DOC_LEN_SECTION = 300,/* markdown section bodies run longer */ + DOC_LEN_MODULE = 120, + + MAX_BODY = 500 /* MAX_COMMENT_LEN — the extractor's cap */ +}; + +/* The real body expression from store.h (CBM_SQL_FTS_BODY_EXPR). */ +#define BODY_EXPR \ + " CASE WHEN json_valid(properties)" \ + " THEN coalesce(json_extract(properties,'$.docstring'),'') ELSE '' END " + +static const char *LABELS[] = {"Function", "Method", "Class", "Interface", "Route", "Variable"}; +enum { N_LABELS = (int)(sizeof(LABELS) / sizeof(LABELS[0])) }; + +/* Deterministic PRNG so runs are reproducible across machines. The state must be + * a fixed-width 64-bit type: `unsigned long` is 32 bits on 32-bit targets, which + * silently truncates the seed and degrades the xorshift period. */ +static uint64_t rng_state = UINT64_C(88172645463325252); +static unsigned rnd(unsigned mod) { + rng_state ^= rng_state << 13; + rng_state ^= rng_state >> 7; + rng_state ^= rng_state << 17; + return (unsigned)(rng_state % mod); +} + +/* Reset before each variant so all three see an identical corpus. */ +static void rng_reset(void) { rng_state = UINT64_C(88172645463325252); } + +static void die(sqlite3 *db, const char *what) { + fprintf(stderr, "FATAL: %s: %s\n", what, db ? sqlite3_errmsg(db) : "(no db)"); + exit(1); +} + +static void exec_or_die(sqlite3 *db, const char *sql) { + char *err = NULL; + if (sqlite3_exec(db, sql, NULL, NULL, &err) != SQLITE_OK) { + fprintf(stderr, "FATAL: %s\n while running: %s\n", err ? err : "?", sql); + sqlite3_free(err); + exit(1); + } +} + +/* Database size in bytes: page_count * page_size. */ +static long long db_bytes(sqlite3 *db) { + sqlite3_stmt *st = NULL; + long long pages = 0, psize = 0; + if (sqlite3_prepare_v2(db, "PRAGMA page_count;", -1, &st, NULL) != SQLITE_OK) { + die(db, "page_count"); + } + if (sqlite3_step(st) == SQLITE_ROW) { + pages = sqlite3_column_int64(st, 0); + } + sqlite3_finalize(st); + if (sqlite3_prepare_v2(db, "PRAGMA page_size;", -1, &st, NULL) != SQLITE_OK) { + die(db, "page_size"); + } + if (sqlite3_step(st) == SQLITE_ROW) { + psize = sqlite3_column_int64(st, 0); + } + sqlite3_finalize(st); + return pages * psize; +} + +/* Build one prose blob of roughly `avg` bytes from a small word pool. Varied + * vocabulary matters: FTS5 index size tracks distinct-term count, so a corpus + * of one repeated word would understate the cost badly. */ +static void make_prose(char *buf, size_t cap, int avg) { + static const char *W[] = {"deployment", "rollback", "canary", "pipeline", "consumer", + "throughput", "schema", "migration","validate", "handler", + "retries", "timeout", "cursor", "artifact", "snapshot", + "buffer", "resolve", "template", "namespace", "session"}; + enum { NW = (int)(sizeof(W) / sizeof(W[0])) }; + int target = avg / 2 + (int)rnd((unsigned)avg); /* spread around the average */ + if (target > MAX_BODY) { + target = MAX_BODY; /* the extractor caps before this ever reaches the store */ + } + size_t n = 0; + while (n < (size_t)target && n + 12 < cap) { + const char *w = W[rnd(NW)]; + size_t wl = strlen(w); + if (n + wl + 1 >= cap) { + break; + } + memcpy(buf + n, w, wl); + n += wl; + buf[n++] = ' '; + } + buf[n] = '\0'; +} + +/* Populate a `nodes` table shaped like the product's, with a realistic mix. */ +static void build_nodes(sqlite3 *db, long long rows) { + exec_or_die(db, "PRAGMA journal_mode=OFF; PRAGMA synchronous=OFF;"); + exec_or_die(db, "CREATE TABLE nodes (" + " id INTEGER PRIMARY KEY AUTOINCREMENT," + " project TEXT NOT NULL," + " label TEXT NOT NULL," + " name TEXT NOT NULL," + " qualified_name TEXT NOT NULL," + " file_path TEXT NOT NULL," + " properties TEXT DEFAULT '{}'" + ");"); + + sqlite3_stmt *ins = NULL; + if (sqlite3_prepare_v2(db, + "INSERT INTO nodes (project,label,name,qualified_name,file_path," + "properties) VALUES (?1,?2,?3,?4,?5,?6)", + -1, &ins, NULL) != SQLITE_OK) { + die(db, "prepare insert"); + } + + exec_or_die(db, "BEGIN;"); + char name[128], qn[256], fp[128], props[MAX_BODY + 64], prose[MAX_BODY + 32]; + for (long long i = 0; i < rows; i++) { + unsigned roll = rnd(100); + const char *label; + int has_doc, doclen; + if (roll < PCT_MODULE) { + label = "Module"; + has_doc = (int)rnd(100) < PCT_MODULE_HAS_DESC; + doclen = DOC_LEN_MODULE; + } else if (roll < PCT_MODULE + PCT_SECTION) { + label = "Section"; + has_doc = (int)rnd(100) < PCT_SECTION_HAS_BODY; + doclen = DOC_LEN_SECTION; + } else { + label = LABELS[rnd(N_LABELS)]; + has_doc = (int)rnd(100) < PCT_CODE_HAS_DOC; + doclen = DOC_LEN_CODE; + } + + snprintf(name, sizeof(name), "symbol_%lld_handler", i); + snprintf(fp, sizeof(fp), "src/pkg%u/mod%u.c", (unsigned)(i % 400), (unsigned)(i % 7919)); + snprintf(qn, sizeof(qn), "proj.pkg%u.%s", (unsigned)(i % 400), name); + + if ((int)rnd(100) < PCT_MALFORMED) { + snprintf(props, sizeof(props), "{not valid json"); /* exercises json_valid() */ + } else if (has_doc) { + make_prose(prose, sizeof(prose), doclen); + snprintf(props, sizeof(props), "{\"docstring\":\"%s\"}", prose); + } else { + snprintf(props, sizeof(props), "{}"); + } + + sqlite3_bind_text(ins, 1, "proj", -1, SQLITE_STATIC); + sqlite3_bind_text(ins, 2, label, -1, SQLITE_STATIC); + sqlite3_bind_text(ins, 3, name, -1, SQLITE_TRANSIENT); + sqlite3_bind_text(ins, 4, qn, -1, SQLITE_TRANSIENT); + sqlite3_bind_text(ins, 5, fp, -1, SQLITE_TRANSIENT); + sqlite3_bind_text(ins, 6, props, -1, SQLITE_TRANSIENT); + if (sqlite3_step(ins) != SQLITE_DONE) { + die(db, "insert node"); + } + sqlite3_reset(ins); + } + exec_or_die(db, "COMMIT;"); + sqlite3_finalize(ins); +} + +typedef struct { + double ms; + long long index_bytes; + long long bodies_indexed; +} result_t; + +/* variant: 0 = A (4-col), 1 = B (body, all rows), 2 = C (body, Section/Module only) */ +static result_t run_variant(const char *path, long long rows, int variant) { + sqlite3 *db = NULL; + remove(path); + if (sqlite3_open(path, &db) != SQLITE_OK) { + die(db, "open"); + } + rng_reset(); /* identical corpus for every variant — deltas must compare like with like */ + build_nodes(db, rows); + long long base = db_bytes(db); + + if (variant == 0) { + exec_or_die(db, "CREATE VIRTUAL TABLE nodes_fts USING fts5(" + " name, qualified_name, label, file_path," + " content='', tokenize='unicode61 remove_diacritics 2');"); + } else { + exec_or_die(db, "CREATE VIRTUAL TABLE nodes_fts USING fts5(" + " name, qualified_name, label, file_path, body," + " content='', tokenize='unicode61 remove_diacritics 2');"); + } + + const char *sql; + if (variant == 0) { + sql = "INSERT INTO nodes_fts(rowid,name,qualified_name,label,file_path) " + "SELECT id,name,qualified_name,label,file_path FROM nodes;"; + } else if (variant == 1) { + sql = "INSERT INTO nodes_fts(rowid,name,qualified_name,label,file_path,body) " + "SELECT id,name,qualified_name,label,file_path," BODY_EXPR "FROM nodes;"; + } else { + /* The narrowing lever: only Section/Module rows contribute prose. Every + * node is still indexed by name — only the body is withheld. */ + sql = "INSERT INTO nodes_fts(rowid,name,qualified_name,label,file_path,body) " + "SELECT id,name,qualified_name,label,file_path," + " CASE WHEN label IN ('Section','Module') AND json_valid(properties)" + " THEN coalesce(json_extract(properties,'$.docstring'),'') ELSE '' END " + "FROM nodes;"; + } + + double t0 = now_ms(); + exec_or_die(db, sql); + double t1 = now_ms(); + + result_t r; + r.ms = t1 - t0; + r.index_bytes = db_bytes(db) - base; + + /* How many rows actually contributed prose — the denominator for "cost per body". */ + const char *count_sql = + (variant == 0) + ? "SELECT 0;" + : ((variant == 1) + ? "SELECT count(*) FROM nodes WHERE json_valid(properties) AND " + "coalesce(json_extract(properties,'$.docstring'),'') <> '';" + : "SELECT count(*) FROM nodes WHERE label IN ('Section','Module') AND " + "json_valid(properties) AND " + "coalesce(json_extract(properties,'$.docstring'),'') <> '';"); + sqlite3_stmt *st = NULL; + r.bodies_indexed = 0; + if (sqlite3_prepare_v2(db, count_sql, -1, &st, NULL) == SQLITE_OK) { + if (sqlite3_step(st) == SQLITE_ROW) { + r.bodies_indexed = sqlite3_column_int64(st, 0); + } + sqlite3_finalize(st); + } + + sqlite3_close(db); + remove(path); + return r; +} + +static double mb(long long bytes) { return (double)bytes / (1024.0 * 1024.0); } + +/* Timing methodology. + * + * Two things matter here, and getting either wrong produces numbers that look + * authoritative and are not: + * + * 1. Take the MINIMUM, not the mean. Backfill time has a hard floor (the real + * work) and an unbounded tail (scheduler preemption, page-cache misses, + * thermal throttling). Averaging folds that tail into the estimate; the + * minimum is the closest observable approximation of the floor. + * + * 2. INTERLEAVE the variants (A,B,C, A,B,C, ...) rather than running each to + * completion in turn (A,A,A, B,B,B, C,C,C). Grouped runs let drift over the + * life of the process land entirely on whichever variant goes last — which + * on a loaded machine is enough to report variant C as *slower* than B even + * though C does strictly less work. Interleaving spreads drift across all + * three so it cancels from the deltas. + * + * The observed spread (max/min) is printed so noise stays visible instead of + * being quietly absorbed into a single confident-looking figure. */ +enum { REPEATS = 3 }; + +typedef struct { + result_t best; + double worst_ms; +} sampled_t; + +static void report(long long rows) { + sampled_t s[3]; + for (int v = 0; v < 3; v++) { + s[v].best.ms = 0; + s[v].worst_ms = 0; + } + /* A fresh filename per (iteration, variant). Reusing one path across + * iterations is enough to hit "database is locked" on Windows, where the + * previous handle can outlive close() briefly and remove() then silently + * leaves the old file in place. */ + char path[64]; + + for (int i = 0; i < REPEATS; i++) { + for (int v = 0; v < 3; v++) { + snprintf(path, sizeof(path), "bench_fts_%c_%d.db", (char)('a' + v), i); + result_t r = run_variant(path, rows, v); + if (i == 0 || r.ms < s[v].best.ms) { + s[v].best = r; + } + if (i == 0 || r.ms > s[v].worst_ms) { + s[v].worst_ms = r.ms; + } + } + } + result_t a = s[0].best, b = s[1].best, c = s[2].best; + + printf("\n== %lld nodes ==\n", rows); + printf(" %-34s %10s %12s %12s\n", "variant", "backfill", "FTS index", "bodies"); + printf(" %-34s %9.0fms %10.1fMB %12lld\n", "A 4-column (pre-#518)", a.ms, mb(a.index_bytes), + a.bodies_indexed); + printf(" %-34s %9.0fms %10.1fMB %12lld\n", "B +body, all nodes (#518)", b.ms, + mb(b.index_bytes), b.bodies_indexed); + printf(" %-34s %9.0fms %10.1fMB %12lld\n", "C +body, Section/Module only", c.ms, + mb(c.index_bytes), c.bodies_indexed); + + printf(" ----\n"); + printf(" B - A cost of the feature %+9.0fms %+10.1fMB (%+.1f%% time, %+.1f%% size)\n", + b.ms - a.ms, mb(b.index_bytes - a.index_bytes), + a.ms > 0 ? (b.ms - a.ms) * 100.0 / a.ms : 0.0, + a.index_bytes > 0 + ? (double)(b.index_bytes - a.index_bytes) * 100.0 / (double)a.index_bytes + : 0.0); + printf(" C - A cost if narrowed %+9.0fms %+10.1fMB\n", c.ms - a.ms, + mb(c.index_bytes - a.index_bytes)); + printf(" B - C what narrowing saves %+9.0fms %+10.1fMB\n", b.ms - c.ms, + mb(b.index_bytes - c.index_bytes)); + + /* Surface the noise floor. If the run-to-run spread is comparable to the + * B-A delta being reported, the timing half of this table is not telling + * you anything and needs a quieter machine or a larger row count. */ + double spread = 0.0; + for (int v = 0; v < 3; v++) { + double sp = s[v].best.ms > 0 ? (s[v].worst_ms - s[v].best.ms) * 100.0 / s[v].best.ms : 0.0; + if (sp > spread) { + spread = sp; + } + } + printf(" run-to-run spread: %.0f%% (worst variant, %d runs)\n", spread, REPEATS); + if (b.ms - a.ms > 0 && s[0].worst_ms - s[0].best.ms > (b.ms - a.ms) * 0.5) { + printf(" !! WARNING: noise is large relative to the B-A delta — treat the timing\n" + " column as unreliable on this machine. Size figures are deterministic\n" + " and remain valid. Re-run on a quiet machine or with more rows.\n"); + } + fflush(stdout); +} + +int main(int argc, char **argv) { + printf("nodes_fts `body` column cost isolation (#518)\n"); + printf("SQLite %s | corpus: %d%% Module, %d%% Section, rest code symbols\n", + sqlite3_libversion(), PCT_MODULE, PCT_SECTION); + printf("docstring coverage: code %d%%, Section %d%%, Module %d%%; %d%% malformed JSON\n", + PCT_CODE_HAS_DOC, PCT_SECTION_HAS_BODY, PCT_MODULE_HAS_DESC, PCT_MALFORMED); + printf("NOTE: cbm_camel_split() is not applied (it is product-side); identical across all\n" + " three variants, so it cancels from every delta below.\n"); + printf("timings: best of %d runs per variant\n", REPEATS); + + if (argc > 1) { + for (int i = 1; i < argc; i++) { + report(strtoll(argv[i], NULL, 10)); + } + } else { + report(100000); + report(500000); + report(2000000); + } + printf("\nExtrapolate to your corpus by scaling the B-A row; it is linear in node count.\n"); + return 0; +} diff --git a/scripts/benchmark-fts-body.sh b/scripts/benchmark-fts-body.sh new file mode 100755 index 000000000..f13b52b2d --- /dev/null +++ b/scripts/benchmark-fts-body.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Build and run the nodes_fts `body` column cost isolation (#518). +# Usage: scripts/benchmark-fts-body.sh [rowcount ...] +# +# Measures per-row body tokenisation time and FTS index storage for three +# variants (pre-#518 4-column, #518 body-for-all, body-for-Section/Module only). +# See the header comment in benchmark-fts-body.c for what this does and does +# not measure — in particular it is NOT a substitute for benchmark-index.sh on +# a real corpus. + +ROOT=$(cd "$(dirname "$0")/.." && pwd -P) +OUT="${TMPDIR:-/tmp}/benchmark-fts-body" +CC_BIN="${CC:-cc}" + +echo "Building $OUT ..." +"$CC_BIN" -O2 -o "$OUT" \ + "$ROOT/scripts/benchmark-fts-body.c" \ + "$ROOT/vendored/sqlite3/sqlite3.c" \ + -I"$ROOT/vendored/sqlite3" \ + -DSQLITE_ENABLE_FTS5 \ + -DSQLITE_THREADSAFE=0 \ + -lm ${LDLIBS:-} + +# Run from a scratch dir — the harness creates and removes temp .db files in cwd. +WORK=$(mktemp -d) +trap 'rm -rf "$WORK"' EXIT +cd "$WORK" + +"$OUT" "$@" From 465f2b9050173555e3f0385a7f55bd88de76b187 Mon Sep 17 00:00:00 2001 From: ShauryaaSharma Date: Fri, 21 Aug 2026 19:50:41 +0530 Subject: [PATCH 3/7] style(store): split CBM_SQL_FTS_BODY_EXPR across continuation lines The macro was emitted as a single 274-character physical line, past the 100-column limit, which failed clang-format and therefore the lint job. Because the test job is gated on lint, none of the tests added for #518 had executed in CI at all. Split across escaped-newline continuations with backslashes aligned per AlignEscapedNewlines: Left (longest content line + 2). Verified the macro still expands to a byte-identical SQL string, so this is formatting only with no behavioural change. Signed-off-by: ShauryaaSharma --- src/store/store.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/store/store.h b/src/store/store.h index 27c812859..24dcaaa71 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -358,7 +358,9 @@ int cbm_store_fts_rebuild(cbm_store_t *s); * essential — json_extract() aborts the whole statement on malformed JSON, and pre-fix * databases contain such rows; a guarded row degrades to name-only indexing instead of * failing the write. Expects the `nodes` row in scope as the SELECT source. */ -#define CBM_SQL_FTS_BODY_EXPR " CASE WHEN json_valid(properties)" " THEN coalesce(json_extract(properties,'$.docstring'),'') ELSE '' END " +#define CBM_SQL_FTS_BODY_EXPR \ + " CASE WHEN json_valid(properties)" \ + " THEN coalesce(json_extract(properties,'$.docstring'),'') ELSE '' END " /* ── WAL / Checkpoint ───────────────────────────────────────────── */ From 6463eec6340f90b13079e47ec5f375b268cf8c06 Mon Sep 17 00:00:00 2001 From: ShauryaaSharma Date: Fri, 21 Aug 2026 21:18:01 +0530 Subject: [PATCH 4/7] test(pipeline): guard the delta FTS write site through cbm_delta_patch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fts_delta_insert_populates_body hand-prepared its own INSERT inside the test body rather than calling any production entry point, so reverting pipeline_delta.c to a four-column nodes_fts insert left it passing. It guarded CBM_SQL_FTS_BODY_EXPR, not the site that consumes it — which is the exact failure mode it was written to catch, since that bug produces no compile error and no failing assertion, only a silently NULL body on the incremental path. Replaced with pipeline_delta_merge_indexes_body in test_pipeline.c, which drives the real path: seed a store, cbm_delta_preseed, add a Section node carrying prose, cbm_delta_patch, then assert the merged node's body text is matchable through nodes_fts. Verified the new test actually discriminates. Against a four-column revert of pipeline_delta.c the body assertions ('canary', 'rollback') fail while the name assertion ('deployment') still passes — confirming the node merged and only its body was lost, which is precisely the regression being guarded. Writing it against the production path also surfaced an ordering constraint 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 never merged by the "id > max_db_id" predicate. Documented in the test. Signed-off-by: ShauryaaSharma --- tests/test_pipeline.c | 80 +++++++++++++++++++++++++++++++++++++++ tests/test_store_search.c | 58 ---------------------------- 2 files changed, 80 insertions(+), 58 deletions(-) diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index fb33bcc72..504f8c99d 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -12069,6 +12069,84 @@ TEST(pipeline_lsp_surface_persisted_and_body_edit_invariant) { PASS(); } +/* ── #518: delta-merge FTS body indexing ─────────────────────────── */ + +/* Count nodes_fts rows matching a single (test-controlled) alpha token. */ +static int delta_fts_match_count(sqlite3 *db, const char *term) { + char sql[256]; + snprintf(sql, sizeof(sql), "SELECT count(*) FROM nodes_fts WHERE nodes_fts MATCH '%s'", term); + sqlite3_stmt *st = NULL; + if (sqlite3_prepare_v2(db, sql, -1, &st, NULL) != SQLITE_OK) { + return -1; + } + int n = (sqlite3_step(st) == SQLITE_ROW) ? sqlite3_column_int(st, 0) : -1; + sqlite3_finalize(st); + return n; +} + +/* Prose arriving through delta merge must land in nodes_fts.body (#518). + * + * This drives cbm_delta_patch itself rather than reimplementing its INSERT. + * That distinction is the whole point: nodes_fts has five columns, and a + * statement naming only the original four is still valid SQL that silently + * leaves body NULL — no compile error, no failing assertion, prose merged + * incrementally simply unsearchable while a full reindex looks perfect. A + * test that hand-rolls its own INSERT keeps passing when pipeline_delta.c + * regresses, because it is exercising the copy rather than the call site. + * + * Revert pipeline_delta.c's INSERT to four columns and this test must fail. */ +TEST(pipeline_delta_merge_indexes_body) { + cbm_store_t *store = cbm_store_open_memory(); + ASSERT_NOT_NULL(store); + cbm_store_upsert_project(store, "test", "/tmp/test"); + + /* Generation 1: an existing node, indexed the wholesale way. */ + cbm_node_t base = {.project = "test", + .label = "Function", + .name = "existing", + .qualified_name = "test.existing", + .file_path = "a.c", + .properties_json = "{\"docstring\":\"already indexed\"}"}; + cbm_store_upsert_node(store, &base); + ASSERT_EQ(cbm_store_fts_rebuild(store), CBM_STORE_OK); + + /* Generation 2 arrives incrementally: a markdown Section carrying prose. + * + * Order matters. cbm_delta_preseed reads MAX(id) and lifts the gbuf id + * watermark above it, which is what makes the patch's "id > max_db_id" + * predicate mean "inserted by this patch". A node added to the gbuf before + * preseed keeps a low temp id and is silently never merged, so the node + * must be created after. */ + cbm_gbuf_t *gb = cbm_gbuf_new("test", "/tmp/test"); + ASSERT_NOT_NULL(gb); + + int64_t max_db_id = cbm_delta_preseed(store, "test", gb); + ASSERT_GTE(max_db_id, 0); + + ASSERT_GT(cbm_gbuf_upsert_node(gb, "Section", "Deployment", "test.README.deployment", + "README.md", 1, 8, + "{\"docstring\":\"rollback uses the canary alias\"}"), + max_db_id); + + ASSERT_EQ(cbm_delta_patch(store, "test", gb, max_db_id, NULL, 0), 0); + + sqlite3 *db = cbm_store_get_db(store); + ASSERT_NOT_NULL(db); + + /* The merged node's prose is searchable — the assertion that fails if the + * delta write site stops feeding CBM_SQL_FTS_BODY_EXPR. */ + ASSERT_GTE(delta_fts_match_count(db, "canary"), 1); + ASSERT_GTE(delta_fts_match_count(db, "rollback"), 1); + /* ...and its name is still indexed on that path. */ + ASSERT_GTE(delta_fts_match_count(db, "deployment"), 1); + /* The pre-existing generation survived the patch. */ + ASSERT_GTE(delta_fts_match_count(db, "existing"), 1); + + cbm_gbuf_free(gb); + cbm_store_close(store); + PASS(); +} + SUITE(pipeline) { RUN_TEST(pipeline_lsp_surface_persisted_and_body_edit_invariant); /* Index lock */ @@ -12380,6 +12458,8 @@ SUITE(pipeline) { /* Project name edge cases */ RUN_TEST(project_name_special_chars); RUN_TEST(project_name_trailing_slash); + /* #518 delta-merge FTS body */ + RUN_TEST(pipeline_delta_merge_indexes_body); } /* Focused semantic-manifest and publication contracts. Kept separate from the diff --git a/tests/test_store_search.c b/tests/test_store_search.c index 27f39998a..488f13f5f 100644 --- a/tests/test_store_search.c +++ b/tests/test_store_search.c @@ -1582,63 +1582,6 @@ TEST(fts_rebuild_tolerates_malformed_properties) { PASS(); } -/* Regression guard for the incremental path (pipeline_delta.c). nodes_fts has five - * columns, and a row-level INSERT naming only the original four is still valid SQL - * that silently leaves `body` NULL — prose arriving via delta merge would be - * unsearchable while a full reindex looked perfectly correct. This runs the delta - * site's exact statement shape (id > max_db_id, via CBM_SQL_FTS_BODY_EXPR) and - * asserts the newly merged node's body is searchable. */ -TEST(fts_delta_insert_populates_body) { - cbm_store_t *s = cbm_store_open_memory(); - cbm_store_upsert_project(s, "test", "/tmp/test"); - cbm_node_t first = {.project = "test", - .label = "Function", - .name = "existing", - .qualified_name = "test.existing", - .file_path = "a.c", - .properties_json = "{\"docstring\":\"already indexed\"}"}; - cbm_store_upsert_node(s, &first); - ASSERT_EQ(cbm_store_fts_rebuild(s), CBM_STORE_OK); - - sqlite3 *db = cbm_store_get_db(s); - /* Everything already in nodes_fts is the previous generation. */ - sqlite3_stmt *mx = NULL; - ASSERT_EQ(sqlite3_prepare_v2(db, "SELECT coalesce(max(id),0) FROM nodes", -1, &mx, NULL), - SQLITE_OK); - ASSERT(sqlite3_step(mx) == SQLITE_ROW); - sqlite3_int64 max_db_id = sqlite3_column_int64(mx, 0); - sqlite3_finalize(mx); - - /* A markdown Section arrives incrementally, carrying prose in its docstring. */ - cbm_node_t added = {.project = "test", - .label = "Section", - .name = "Deployment", - .qualified_name = "test.README.deployment", - .file_path = "README.md", - .properties_json = "{\"docstring\":\"rollback uses the canary alias\"}"}; - cbm_store_upsert_node(s, &added); - - sqlite3_stmt *fts = NULL; - ASSERT_EQ(sqlite3_prepare_v2(db, - "INSERT INTO nodes_fts (rowid, name, qualified_name, label," - " file_path, body)" - " SELECT id, name, qualified_name, label," - " file_path," CBM_SQL_FTS_BODY_EXPR - "FROM nodes WHERE project = ?1 AND id > ?2", - -1, &fts, NULL), - SQLITE_OK); - sqlite3_bind_text(fts, 1, "test", -1, SQLITE_TRANSIENT); - sqlite3_bind_int64(fts, 2, max_db_id); - ASSERT_EQ(sqlite3_step(fts), SQLITE_DONE); - sqlite3_finalize(fts); - - /* The delta-merged node's prose is searchable — not just its name. */ - ASSERT_GTE(fts_match_count(db, "canary"), 1); - ASSERT_GTE(fts_match_count(db, "rollback"), 1); - ASSERT_GTE(fts_match_count(db, "deployment"), 1); - cbm_store_close(s); - PASS(); -} SUITE(store_search) { RUN_TEST(store_search_by_label); @@ -1712,5 +1655,4 @@ SUITE(store_search) { RUN_TEST(fts_rebuild_indexes_body_content); RUN_TEST(fts_rebuild_upgrades_legacy_schema); RUN_TEST(fts_rebuild_tolerates_malformed_properties); - RUN_TEST(fts_delta_insert_populates_body); } From b6f709289ba0ad53b219bf5dbec558ab805e1261 Mon Sep 17 00:00:00 2001 From: ShauryaaSharma Date: Fri, 21 Aug 2026 21:18:11 +0530 Subject: [PATCH 5/7] style(store): align CBM_SQL_FTS_BODY_EXPR continuations to clang-format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous split put the escaped newlines at column 78, anchored on the macro's final line. clang-format aligns them to the longest line that actually carries a backslash, and the final line of a macro carries none — so the anchor is the second line (39 chars) and the column is 41, matching the violation clang-format reported at store.h:362:40. Cross-checks against the existing multi-line macro at cli.c:2983, whose longest backslash-bearing line is 95 characters with its backslashes at column 97 — the same content+2. Macro expansion re-verified byte-identical; formatting only. Signed-off-by: ShauryaaSharma --- src/store/store.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/store/store.h b/src/store/store.h index 24dcaaa71..9c739ab1f 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -358,8 +358,8 @@ int cbm_store_fts_rebuild(cbm_store_t *s); * essential — json_extract() aborts the whole statement on malformed JSON, and pre-fix * databases contain such rows; a guarded row degrades to name-only indexing instead of * failing the write. Expects the `nodes` row in scope as the SELECT source. */ -#define CBM_SQL_FTS_BODY_EXPR \ - " CASE WHEN json_valid(properties)" \ +#define CBM_SQL_FTS_BODY_EXPR \ + " CASE WHEN json_valid(properties)" \ " THEN coalesce(json_extract(properties,'$.docstring'),'') ELSE '' END " /* ── WAL / Checkpoint ───────────────────────────────────────────── */ From fe27113741ebb0aa961733fbaba23f2f4d5125e0 Mon Sep 17 00:00:00 2001 From: ShauryaaSharma Date: Sat, 22 Aug 2026 00:27:22 +0530 Subject: [PATCH 6/7] test(extraction): make the two markdown body tests discriminate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both tests passed with the feature reverted, so neither was guarding it. markdown_section_no_body asserted only that a bare heading has no docstring. Every Section docstring is NULL on a build that never captures bodies, so that assertion held with or without the change. It now extracts two sections in one file and asserts the documented sibling HAS prose while the bare one does not — a pair that is only true once capture works and stops at the heading boundary. markdown_section_body_capped hardcoded 500 rather than referencing the budget, reintroducing at the test layer the parallel-constant coupling this branch removed from production. Its corpus was also "alpha " repeated, so the UTF-8 backoff branch never executed in any run of the suite. Promoted MAX_COMMENT_LEN to CBM_MAX_COMMENT_LEN in cbm.h so production and tests assert against one definition instead of a copied literal, and gave the test 498 ASCII bytes followed by U+20AC so that character straddles byte offsets 498-500. A naive cut at the cap lands on its final continuation byte; the test now asserts the result is exactly 498 bytes and ends on a character boundary, which fails if the backoff is removed. Verified locally: body length 498, not 500. Signed-off-by: ShauryaaSharma --- internal/cbm/cbm.h | 5 ++++ internal/cbm/extract_defs.c | 13 +++++----- tests/test_extraction.c | 50 +++++++++++++++++++++++++++++-------- 3 files changed, 51 insertions(+), 17 deletions(-) diff --git a/internal/cbm/cbm.h b/internal/cbm/cbm.h index aebd12291..a8e255989 100644 --- a/internal/cbm/cbm.h +++ b/internal/cbm/cbm.h @@ -177,6 +177,11 @@ typedef enum { CBM_LANG_COUNT } CBMLanguage; +// Byte budget for any prose attached to a definition as its docstring: leading +// doc comments, and Markdown section bodies (#518). Shared with tests so the cap +// is asserted against one definition rather than a copied literal. +#define CBM_MAX_COMMENT_LEN 500 + // --- Extraction result structs --- typedef struct { diff --git a/internal/cbm/extract_defs.c b/internal/cbm/extract_defs.c index 736b00277..55f9b0163 100644 --- a/internal/cbm/extract_defs.c +++ b/internal/cbm/extract_defs.c @@ -16,7 +16,6 @@ #include // Buffer sizes for local arrays (base classes, params, return types). -#define MAX_COMMENT_LEN 500 #define MAX_BASES 16 #define MAX_BASES_MINUS_1 15 #define MAX_PARAMS CBM_SZ_32 @@ -1236,12 +1235,12 @@ static bool is_comment_node(const char *kind) { strcmp(kind, "line_comment") == 0 || strcmp(kind, "multiline_comment") == 0); } -// Extract comment text, truncating to MAX_COMMENT_LEN. +// Extract comment text, truncating to CBM_MAX_COMMENT_LEN. // #1017: snap the cut point back to a complete UTF-8 codepoint boundary. static char *extract_comment_text(CBMArena *a, TSNode node, const char *source) { char *text = cbm_node_text(a, node, source); - if (text && strlen(text) > MAX_COMMENT_LEN) { - size_t cut = MAX_COMMENT_LEN; + if (text && strlen(text) > CBM_MAX_COMMENT_LEN) { + size_t cut = CBM_MAX_COMMENT_LEN; while (cut > 0 && ((unsigned char)text[cut] & 0xC0) == 0x80) cut--; text[cut] = '\0'; @@ -3967,7 +3966,7 @@ static char *extract_markdown_heading_name(CBMArena *a, TSNode node, const char // the first nested subsection or the end of the section. Nested subsections are // excluded because each gets its own Section node and its own body. Returns NULL // when there is no enclosing section or no body text. Trimmed, and capped at -// MAX_COMMENT_LEN (the same budget docstrings use) without splitting a UTF-8 +// CBM_MAX_COMMENT_LEN (the same budget docstrings use) without splitting a UTF-8 // sequence. static char *extract_markdown_section_body(CBMArena *a, TSNode heading, const char *source) { TSNode parent = ts_node_parent(heading); @@ -3997,8 +3996,8 @@ static char *extract_markdown_section_body(CBMArena *a, TSNode heading, const ch return NULL; } size_t len = (size_t)(body_end - body_start); - if (len > MAX_COMMENT_LEN) { - len = MAX_COMMENT_LEN; + if (len > CBM_MAX_COMMENT_LEN) { + len = CBM_MAX_COMMENT_LEN; // Back off so the cap never splits a UTF-8 multi-byte sequence: source[start+len] // is the first excluded byte, and a continuation byte there means we landed // mid-character. diff --git a/tests/test_extraction.c b/tests/test_extraction.c index fb3b723fb..fd94ed372 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -3218,31 +3218,61 @@ TEST(markdown_section_body_captured) { PASS(); } -/* #518: a heading with no prose beneath it yields no docstring (not empty text). */ +/* #518: a heading with no prose beneath it yields no docstring (not empty text). + * + * Asserting only that the bare section has no body would be vacuous — every + * Section docstring is NULL on a build that never captures bodies at all, so + * that half passes with the feature reverted. The documented sibling in the + * same file is what makes the pair meaningful: one has prose and one does not, + * which is only true once capture works AND stops at the heading boundary. */ TEST(markdown_section_no_body) { - CBMFileResult *r = extract("# Title\n## Empty\n", CBM_LANG_MARKDOWN, "t", "README.md"); + CBMFileResult *r = extract("## Documented\n\n" + "Prose about telemetry batching.\n\n" + "## Empty\n", + CBM_LANG_MARKDOWN, "t", "README.md"); ASSERT_NOT_NULL(r); ASSERT_FALSE(r->has_error); - const char *body = def_docstring(r, "Section", "Empty"); - ASSERT(body == NULL || body[0] == '\0'); + const char *documented = def_docstring(r, "Section", "Documented"); + ASSERT_NOT_NULL(documented); + ASSERT(strstr(documented, "telemetry") != NULL); + const char *empty = def_docstring(r, "Section", "Empty"); + ASSERT(empty == NULL || empty[0] == '\0'); cbm_free_result(r); PASS(); } -/* #518: the captured body is capped at MAX_COMMENT_LEN, the same budget docstrings - * use, so a long section cannot blow past the node-properties budget. */ +/* #518: the captured body is capped at CBM_MAX_COMMENT_LEN — the same budget + * docstrings use — and the cap must not split a UTF-8 sequence. + * + * The multi-byte character is placed deliberately, not decoratively: 498 ASCII + * bytes followed by U+20AC (3 bytes) puts that character across byte offsets + * 498-500, so a naive cut at 500 lands on its final continuation byte. An + * all-ASCII corpus never reaches the backoff branch at all, which is why the + * earlier version of this test exercised only the cap. */ TEST(markdown_section_body_capped) { - /* Build a heading followed by ~1500 chars of prose. */ + enum { ASCII_RUN = CBM_MAX_COMMENT_LEN - 2 }; /* 498: U+20AC then straddles the cap */ char src[2048]; int n = snprintf(src, sizeof(src), "# Big\n\n"); - for (int i = 0; i < 250 && n < (int)sizeof(src) - 8; i++) - n += snprintf(src + n, sizeof(src) - (size_t)n, "alpha "); + for (int i = 0; i < ASCII_RUN; i++) { + src[n++] = 'a'; + } + /* Three euro signs: the first straddles the cap, the rest push past it. */ + n += snprintf(src + n, sizeof(src) - (size_t)n, "\xE2\x82\xAC\xE2\x82\xAC\xE2\x82\xAC\n"); + src[n] = '\0'; + CBMFileResult *r = extract(src, CBM_LANG_MARKDOWN, "t", "BIG.md"); ASSERT_NOT_NULL(r); ASSERT_FALSE(r->has_error); const char *body = def_docstring(r, "Section", "Big"); ASSERT_NOT_NULL(body); - ASSERT(strlen(body) <= 500); + + size_t len = strlen(body); + ASSERT(len <= CBM_MAX_COMMENT_LEN); + /* The backoff walked off the partial character rather than keeping 2 of its + * 3 bytes, so the result is short of the cap by exactly those 2 bytes. */ + ASSERT_EQ((int)len, ASCII_RUN); + /* And the final byte is a character boundary, not a continuation byte. */ + ASSERT(((unsigned char)body[len - 1] & 0xC0) != 0x80); cbm_free_result(r); PASS(); } From 5442e9a48912d9ce261f6a00108d388fb817d195 Mon Sep 17 00:00:00 2001 From: ShauryaaSharma Date: Sat, 22 Aug 2026 00:29:10 +0530 Subject: [PATCH 7/7] test(mcp): build the FTS fixture through cbm_store_fts_rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue-552 fixture hand-rolled a delete-all plus a four-column nodes_fts INSERT. That stayed valid SQL against the five-column table and simply left body NULL, so the fixture had quietly stopped mirroring the schema search_graph actually queries — and nothing went red to say so. Replaced with a call to cbm_store_fts_rebuild, which is what production uses. A fixture that spells out its own column list has to be remembered on every schema change; one that calls the rebuild cannot drift. This was the last nodes_fts write site outside the store and the delta patch, so the CBM_SQL_FTS_BODY_EXPR comment in store.h no longer overstates its coverage. Reworded it to say what is actually true and why the failure mode needs review rather than the compiler to catch. Signed-off-by: ShauryaaSharma --- src/store/store.h | 17 ++++++++++++----- tests/test_mcp.c | 13 ++++++------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/store/store.h b/src/store/store.h index 9c739ab1f..32987c1c3 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -349,11 +349,18 @@ int cbm_store_create_indexes(cbm_store_t *s); * only identifiers (#518). Returns CBM_STORE_OK or CBM_STORE_ERR. */ int cbm_store_fts_rebuild(cbm_store_t *s); -/* Body expression shared by EVERY nodes_fts write site — the wholesale rebuild in - * cbm_store_fts_rebuild and the row-level delta insert in pipeline_delta.c. Any new - * write site must use it too: nodes_fts carries five columns, and an INSERT naming - * only the original four is still valid SQL that silently leaves `body` NULL, making - * prose added on that path unsearchable while a full reindex looks correct. +/* Body expression shared by both nodes_fts write sites: the wholesale rebuild in + * cbm_store_fts_rebuild and the row-level delta insert in pipeline_delta.c. Those + * two are the complete set as of this writing — test fixtures build the index via + * cbm_store_fts_rebuild rather than spelling out their own INSERT, deliberately, so + * they cannot drift from the real schema. + * + * Any new write site must use this expression too: nodes_fts carries five columns, + * and an INSERT naming only the original four is still valid SQL that silently + * leaves `body` NULL, making prose added on that path unsearchable while a full + * reindex looks correct. That failure has no compile error and no failing + * assertion, so it is caught by review and by tests that drive the production + * entry point, not by the compiler. * Feeds the node's docstring property, or '' when absent. The json_valid() guard is * essential — json_extract() aborts the whole statement on malformed JSON, and pre-fix * databases contain such rows; a guarded row degrades to name-only indexing instead of diff --git a/tests/test_mcp.c b/tests/test_mcp.c index 278d33e68..964edc518 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -2484,13 +2484,12 @@ TEST(tool_search_graph_query_honors_file_pattern_issue552) { component_status.end_line = 3; ASSERT_GT(cbm_store_upsert_node(st, &component_status), 0); - cbm_store_exec(st, "INSERT INTO nodes_fts(nodes_fts) VALUES('delete-all');"); - ASSERT_EQ(cbm_store_exec(st, - "INSERT INTO nodes_fts(rowid, name, qualified_name, label, " - "file_path) " - "SELECT id, cbm_camel_split(name), qualified_name, label, file_path " - "FROM nodes;"), - CBM_STORE_OK); + /* Build the FTS index through the production rebuild rather than a hand-rolled + * INSERT. A fixture that spells out its own column list silently drifts from + * the real schema — naming only the four original columns stays valid SQL + * against the five-column table and just leaves `body` NULL, so the fixture + * would stop mirroring what search_graph actually queries (#518). */ + ASSERT_EQ(cbm_store_fts_rebuild(st), CBM_STORE_OK); char *resp = cbm_mcp_server_handle(srv, "{\"jsonrpc\":\"2.0\",\"id\":552,\"method\":\"tools/call\","