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 4994bd7b8..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'; @@ -3849,7 +3848,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 +3860,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 +3959,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 +// 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); + 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 > 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. + 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 +4074,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 +4090,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 +4098,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 +4153,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/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" "$@" 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..32987c1c3 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -342,6 +342,33 @@ 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 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 + * 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..fd94ed372 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,87 @@ 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). + * + * 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("## 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 *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 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) { + 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 < 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); + + 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(); +} + + /* ═══════════════════════════════════════════════════════════════════ * Python __init__.py Module QN collision regression * ═══════════════════════════════════════════════════════════════════ */ @@ -5775,6 +5866,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_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\"," 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 2eb189ed5..488f13f5f 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,108 @@ 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(); +} + + SUITE(store_search) { RUN_TEST(store_search_by_label); RUN_TEST(store_search_by_name_pattern); @@ -1548,4 +1651,8 @@ 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); }