diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 3259221c4..827f0e9f9 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -4195,18 +4195,19 @@ static char *bm25_search(cbm_store_t *store, const char *project, const char *qu } /* Extract keyword strings from a yyjson array into `keywords`. Returns the - * number of strings copied (capped at `max_out`). */ + * number of strings copied (capped at `max_out`), or -1 when any element is + * not a string: a mixed-type array is a caller error and is never silently + * narrowed to its string members. */ static int extract_semantic_keywords(yyjson_val *sq_val, const char **keywords, int max_out) { - int kw_count = (int)yyjson_arr_size(sq_val); - if (kw_count > max_out) { - kw_count = max_out; - } size_t kw_idx = 0; size_t kw_max = 0; yyjson_val *kw_val; int ki = 0; yyjson_arr_foreach(sq_val, kw_idx, kw_max, kw_val) { - if (ki < kw_count && yyjson_is_str(kw_val)) { + if (!yyjson_is_str(kw_val)) { + return -1; + } + if (ki < max_out) { keywords[ki++] = yyjson_get_str(kw_val); } } @@ -4237,13 +4238,20 @@ static void emit_semantic_results(yyjson_mut_doc *doc, yyjson_mut_val *root, yyjson_mut_obj_add_val(doc, root, "semantic", sem); } +typedef enum { + SQ_RUN_OK = 0, + SQ_RUN_TYPE_ERROR, /* semantic_query is not an array of strings */ + SQ_RUN_STORE_ERROR, /* the vector scan itself failed */ +} sq_run_status_t; + /* Run the semantic_query vector search from raw args. Sets *out_vresults / * *out_vcount (caller frees via cbm_store_free_vector_results when vcount>0). - * Returns true if semantic_query was provided as a non-array (type error — - * caller should surface to the user). */ -static bool run_semantic_query_core(const char *args, cbm_store_t *store, const char *project, - int materialize_limit, cbm_vector_result_t **out_vresults, - int *out_vcount, bool *out_present) { + * A store without a vector table (lean index) is an empty result; a failed + * scan is SQ_RUN_STORE_ERROR and must never be rendered as zero matches. */ +static sq_run_status_t run_semantic_query_core(const char *args, cbm_store_t *store, + const char *project, int materialize_limit, + cbm_vector_result_t **out_vresults, int *out_vcount, + bool *out_present) { enum { MAX_KW_SEARCH = 32 }; *out_vresults = NULL; *out_vcount = 0; @@ -4256,25 +4264,31 @@ static bool run_semantic_query_core(const char *args, cbm_store_t *store, const if (out_present && sq_val) { *out_present = true; } - bool type_error = false; + sq_run_status_t status = SQ_RUN_OK; if (sq_val && !yyjson_is_arr(sq_val)) { - type_error = true; + status = SQ_RUN_TYPE_ERROR; } else if (sq_val && yyjson_arr_size(sq_val) > 0) { const char *keywords[MAX_KW_SEARCH]; int ki = extract_semantic_keywords(sq_val, keywords, MAX_KW_SEARCH); - cbm_vector_result_t *vresults = NULL; - int vcount = 0; - if (cbm_store_vector_search(store, project, keywords, ki, materialize_limit, &vresults, - &vcount) == CBM_STORE_OK && - vcount > 0) { - *out_vresults = vresults; - *out_vcount = vcount; + if (ki < 0) { + status = SQ_RUN_TYPE_ERROR; + } else { + cbm_vector_result_t *vresults = NULL; + int vcount = 0; + int rc = cbm_store_vector_search(store, project, keywords, ki, materialize_limit, + &vresults, &vcount); + if (rc == CBM_STORE_ERR) { + status = SQ_RUN_STORE_ERROR; + } else if (rc == CBM_STORE_OK && vcount > 0) { + *out_vresults = vresults; + *out_vcount = vcount; + } } } if (args_doc) { yyjson_doc_free(args_doc); } - return type_error; + return status; } static bool search_graph_arg_present(const char *args, const char *name) { @@ -5134,9 +5148,9 @@ static char *handle_search_graph(cbm_mcp_server_t *srv, const char *args) { cbm_vector_result_t *vresults = NULL; int vcount = 0; bool sq_present = false; - bool sq_type_error = run_semantic_query_core(args, store, project, semantic_materialize_limit, - &vresults, &vcount, &sq_present); - if (sq_type_error) { + sq_run_status_t sq_status = run_semantic_query_core( + args, store, project, semantic_materialize_limit, &vresults, &vcount, &sq_present); + if (sq_status != SQ_RUN_OK) { if (fields_owner) { yyjson_doc_free(fields_owner); } @@ -5146,11 +5160,17 @@ static char *handle_search_graph(cbm_mcp_server_t *srv, const char *args) { free(qn_pattern); free(file_pattern); free(relationship); + if (sq_status == SQ_RUN_STORE_ERROR) { + return cbm_mcp_text_result( + "semantic search failed: the vector index could not be scanned — see the " + "server log for the SQLite error; reindex the project if it persists.", + true); + } return cbm_mcp_text_result( "semantic_query must be an array of keyword strings, e.g. " - "[\"send\",\"pubsub\",\"publish\"] — not a single string. Split your query " - "into individual keywords; each is scored independently via per-keyword " - "min-cosine.", + "[\"send\",\"pubsub\",\"publish\"] — not a single string, and every element " + "must be a string. Split your query into individual keywords; each is scored " + "independently via per-keyword min-cosine.", true); } diff --git a/src/store/store.c b/src/store/store.c index c36f00b44..fbea20146 100644 --- a/src/store/store.c +++ b/src/store/store.c @@ -10018,25 +10018,42 @@ static int vs_build_keyword_vectors(cbm_store_t *s, const char *project, const c return actual_kw; } +/* L2 norm of every quantized keyword vector, computed once per search + * instead of once per (node, keyword) pair inside the scoring loop. */ +static void vs_keyword_norms(const int8_t (*kw_vecs)[VS_VEC_DIM], int actual_kw, double *kw_norms) { + for (int k = 0; k < actual_kw; k++) { + int32_t ma = 0; + for (int d = 0; d < VS_VEC_DIM; d++) { + ma += (int32_t)kw_vecs[k][d] * (int32_t)kw_vecs[k][d]; + } + kw_norms[k] = sqrt((double)ma); + } +} + /* Compute the per-keyword min cosine score between a node's int8 vector and - * each of the query vectors. Returns 0.0 if the node vector is unavailable - * or mis-sized. */ + * each of the query vectors. `kw_norms[k]` is the precomputed L2 norm of + * kw_vecs[k]; the node norm is computed once per node. The integer sums are + * exact, so factoring the norms out of the pair loop leaves every score + * bit-identical to the fused form. Returns 0.0 if the node vector is + * unavailable or mis-sized. */ static double vs_min_cosine_score(const int8_t *node_vec, int node_vec_len, - const int8_t (*kw_vecs)[VS_VEC_DIM], int actual_kw) { + const int8_t (*kw_vecs)[VS_VEC_DIM], const double *kw_norms, + int actual_kw) { if (!node_vec || node_vec_len != VS_VEC_DIM) { return 0.0; } + int32_t mb = 0; + for (int d = 0; d < VS_VEC_DIM; d++) { + mb += (int32_t)node_vec[d] * (int32_t)node_vec[d]; + } + double node_norm = sqrt((double)mb); double min_score = CBM_STORE_UNIT_POS_D; for (int k = 0; k < actual_kw; k++) { int32_t dot = 0; - int32_t ma = 0; - int32_t mb = 0; for (int d = 0; d < VS_VEC_DIM; d++) { dot += (int32_t)kw_vecs[k][d] * (int32_t)node_vec[d]; - ma += (int32_t)kw_vecs[k][d] * (int32_t)kw_vecs[k][d]; - mb += (int32_t)node_vec[d] * (int32_t)node_vec[d]; } - double denom = sqrt((double)ma) * sqrt((double)mb); + double denom = kw_norms[k] * node_norm; double cos_k = denom > CBM_STORE_DENOM_EPS_D ? (double)dot / denom : 0.0; if (cos_k < min_score) { min_score = cos_k; @@ -10045,35 +10062,69 @@ static double vs_min_cosine_score(const int8_t *node_vec, int node_vec_len, return min_score; } -/* Append one candidate row read from the scan statement into the result - * vector. Grows the results array geometrically on demand. Returns the - * (possibly grown) results pointer, or NULL on allocation failure. */ -static cbm_vector_result_t *vs_append_result(cbm_vector_result_t *results, int *count, int *cap, - sqlite3_stmt *stmt, - const int8_t (*kw_vecs)[VS_VEC_DIM], int actual_kw) { +/* Append one candidate row read from the scan statement into `*results`, + * growing the array geometrically on demand. Returns CBM_STORE_OK, or + * CBM_STORE_ERR on allocation failure — in which case `*results` still owns + * exactly `*count` complete rows (a row is only counted once every string + * copy succeeded), so the caller's usual free path stays valid. */ +static int vs_append_result(cbm_vector_result_t **results, int *count, int *cap, sqlite3_stmt *stmt, + const int8_t (*kw_vecs)[VS_VEC_DIM], const double *kw_norms, + int actual_kw) { if (*count >= *cap) { int nc = *cap < CBM_SZ_16 ? CBM_SZ_16 : *cap * ST_COL_2; - cbm_vector_result_t *grown = realloc(results, (size_t)nc * sizeof(cbm_vector_result_t)); + cbm_vector_result_t *grown = realloc(*results, (size_t)nc * sizeof(cbm_vector_result_t)); if (!grown) { - return NULL; + return CBM_STORE_ERR; } - results = grown; + *results = grown; *cap = nc; } - int idx = (*count)++; - results[idx].node_id = sqlite3_column_int64(stmt, 0); + cbm_vector_result_t *row = &(*results)[*count]; + row->node_id = sqlite3_column_int64(stmt, 0); const char *name = (const char *)sqlite3_column_text(stmt, SKIP_ONE); const char *qn = (const char *)sqlite3_column_text(stmt, ST_COL_2); const char *fp = (const char *)sqlite3_column_text(stmt, ST_COL_3); const char *label = (const char *)sqlite3_column_text(stmt, ST_COL_4); - results[idx].name = name ? strdup(name) : strdup(""); - results[idx].qualified_name = qn ? strdup(qn) : strdup(""); - results[idx].file_path = fp ? strdup(fp) : strdup(""); - results[idx].label = label ? strdup(label) : strdup(""); + row->name = strdup(name ? name : ""); + row->qualified_name = strdup(qn ? qn : ""); + row->file_path = strdup(fp ? fp : ""); + row->label = strdup(label ? label : ""); + if (!row->name || !row->qualified_name || !row->file_path || !row->label) { + free(row->name); + free(row->qualified_name); + free(row->file_path); + free(row->label); + return CBM_STORE_ERR; + } const int8_t *node_vec = (const int8_t *)sqlite3_column_blob(stmt, ST_COL_6); int node_vec_len = sqlite3_column_bytes(stmt, ST_COL_6); - results[idx].score = vs_min_cosine_score(node_vec, node_vec_len, kw_vecs, actual_kw); - return results; + row->score = vs_min_cosine_score(node_vec, node_vec_len, kw_vecs, kw_norms, actual_kw); + (*count)++; + return CBM_STORE_OK; +} + +/* A lean index carries no node_vectors table at all. That is an empty + * semantic universe, not a scan failure, and callers must be able to tell + * the two apart — so probe the schema before preparing the scan. Returns + * CBM_STORE_OK (present), CBM_STORE_NOT_FOUND (absent) or CBM_STORE_ERR. */ +static int vs_probe_node_vectors_table(cbm_store_t *s) { + sqlite3_stmt *probe = NULL; + const char *sql = + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='node_vectors' LIMIT 1;"; + if (sqlite3_prepare_v2(s->db, sql, SQLITE_AUTO_LEN, &probe, NULL) != SQLITE_OK) { + (void)fprintf(stderr, "vector_search: %s\n", sqlite3_errmsg(s->db)); + return CBM_STORE_ERR; + } + int step_rc = sqlite3_step(probe); + sqlite3_finalize(probe); + if (step_rc == SQLITE_ROW) { + return CBM_STORE_OK; + } + if (step_rc == SQLITE_DONE) { + return CBM_STORE_NOT_FOUND; + } + (void)fprintf(stderr, "vector_search: %s\n", sqlite3_errmsg(s->db)); + return CBM_STORE_ERR; } static int vs_ranked_result_cmp(const void *lhs, const void *rhs) { @@ -10097,11 +10148,18 @@ int cbm_store_vector_search(cbm_store_t *s, const char *project, const char **ke return CBM_STORE_ERR; } + int probe_rc = vs_probe_node_vectors_table(s); + if (probe_rc != CBM_STORE_OK) { + return probe_rc; + } + int8_t kw_vecs[VS_MAX_KW][VS_VEC_DIM]; int actual_kw = vs_build_keyword_vectors(s, project, keywords, keyword_count, kw_vecs); if (actual_kw == 0) { return CBM_STORE_OK; } + double kw_norms[VS_MAX_KW]; + vs_keyword_norms(kw_vecs, actual_kw, kw_norms); /* Use the first keyword for a cheap candidate ordering, then score each * materialized candidate by min-cosine across every keyword. */ @@ -10141,13 +10199,11 @@ int cbm_store_vector_search(cbm_store_t *s, const char *project, const char **ke sqlite3_bind_int(stmt, ST_COL_3, fetch_limit); while ((step_rc = sqlite3_step(stmt)) == SQLITE_ROW) { omitted_score_ceiling = sqlite3_column_double(stmt, ST_COL_5); - cbm_vector_result_t *grown = - vs_append_result(results, &count, &cap, stmt, kw_vecs, actual_kw); - if (!grown) { + if (vs_append_result(&results, &count, &cap, stmt, kw_vecs, kw_norms, actual_kw) != + CBM_STORE_OK) { step_rc = SQLITE_NOMEM; break; } - results = grown; } if (step_rc != SQLITE_DONE) { char rc_buf[VS_STR_BUF]; diff --git a/src/store/store.h b/src/store/store.h index 353227f09..b6ce844f7 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -1048,7 +1048,11 @@ typedef struct { /* Search for nodes similar to the given query keywords using stored RI vectors. * Builds a merged query vector from the keywords, then does cosine scan via * the cbm_cosine_i8 SQL function joined with the nodes table. - * Returns results sorted by score DESC. Caller must free with cbm_store_free_vector_results. */ + * Returns CBM_STORE_OK with results sorted by score DESC (possibly zero), + * CBM_STORE_NOT_FOUND when the store carries no node_vectors table (lean + * index — an empty universe, not a fault), or CBM_STORE_ERR when the scan + * itself failed; callers must not render CBM_STORE_ERR as zero matches. + * Caller must free with cbm_store_free_vector_results. */ int cbm_store_vector_search(cbm_store_t *s, const char *project, const char **keywords, int keyword_count, int limit, cbm_vector_result_t **out, int *out_count); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index f97eda09e..ef7823a12 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -4268,6 +4268,125 @@ TEST(tool_search_graph_semantic_pagination_is_lossless_and_independent) { PASS(); } +/* #915 residual: a semantic_query array with a non-string element used to be + * silently narrowed to its string members, so ["publish",42] ran as + * ["publish"] and the caller never learned its input was malformed. Every + * element must be a string; anything else is the same type error as a bare + * string semantic_query. */ +TEST(tool_search_graph_semantic_query_rejects_non_string_elements) { + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + ASSERT_NOT_NULL(srv); + cbm_store_t *store = cbm_mcp_server_store(srv); + ASSERT_NOT_NULL(store); + const char *project = "semantic-element-type"; + cbm_mcp_server_set_project(srv, project); + ASSERT_EQ(cbm_store_upsert_project(store, project, "/tmp/semantic-element-type"), CBM_STORE_OK); + + static const char *const bad_args[] = { + "{\"project\":\"semantic-element-type\",\"semantic_query\":[\"publish\",42]}", + "{\"project\":\"semantic-element-type\",\"semantic_query\":[null]}", + "{\"project\":\"semantic-element-type\",\"semantic_query\":[[\"publish\"]]}", + }; + for (size_t i = 0; i < sizeof(bad_args) / sizeof(bad_args[0]); i++) { + char *response = cbm_mcp_handle_tool(srv, "search_graph", bad_args[i]); + ASSERT_NOT_NULL(response); + ASSERT_NOT_NULL(strstr(response, "\"isError\":true")); + char *inner = extract_text_content(response); + ASSERT_NOT_NULL(inner); + ASSERT_NOT_NULL(strstr(inner, "semantic_query must be an array of keyword strings")); + ASSERT_NULL(strstr(inner, "semantic search failed")); + free(inner); + free(response); + } + + /* An all-string array is still accepted. */ + char *response = cbm_mcp_handle_tool( + srv, "search_graph", + "{\"project\":\"semantic-element-type\",\"semantic_query\":[\"publish\",\"send\"]," + "\"format\":\"json\"}"); + ASSERT_NOT_NULL(response); + ASSERT_NULL(strstr(response, "\"isError\":true")); + free(response); + + cbm_mcp_server_free(srv); + PASS(); +} + +static void mcp_test_semantic_scan_failure(sqlite3_context *ctx, int argc, sqlite3_value **argv) { + (void)argc; + (void)argv; + sqlite3_result_error(ctx, "forced semantic scan failure", -1); +} + +/* #915 residual: a vector scan that FAILS must never be reported as "0 + * semantic matches" — the caller would keep broadening keywords against a + * broken index. A project without a vector table (lean index) is not a + * failure and keeps the moderate/full-index hint. */ +TEST(tool_search_graph_semantic_store_error_fails_closed) { + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + ASSERT_NOT_NULL(srv); + cbm_store_t *store = cbm_mcp_server_store(srv); + ASSERT_NOT_NULL(store); + const char *project = "semantic-store-error"; + cbm_mcp_server_set_project(srv, project); + ASSERT_EQ(cbm_store_upsert_project(store, project, "/tmp/semantic-store-error"), CBM_STORE_OK); + cbm_node_t node = {.project = project, + .label = "Function", + .name = "semantic_error_probe", + .qualified_name = "semantic.error.probe", + .file_path = "src/semantic_error_probe.c", + .start_line = 1, + .end_line = 2}; + int64_t id = cbm_store_upsert_node(store, &node); + ASSERT_GT(id, 0); + + /* No node_vectors table at all: an empty page with the reindex hint. */ + const char *semantic_only_args = + "{\"project\":\"semantic-store-error\",\"semantic_query\":[\"semantic-error-token\"]," + "\"format\":\"json\"}"; + char *response = cbm_mcp_handle_tool(srv, "search_graph", semantic_only_args); + ASSERT_NOT_NULL(response); + ASSERT_NULL(strstr(response, "\"isError\":true")); + ASSERT_NOT_NULL(strstr(response, "moderate/full index")); + free(response); + + /* The table exists and holds a vector, but scoring fails mid-scan. */ + ASSERT_EQ(cbm_store_exec(store, "CREATE TABLE node_vectors(node_id INTEGER PRIMARY KEY," + "project TEXT NOT NULL,vector BLOB NOT NULL);"), + CBM_STORE_OK); + ASSERT_EQ(mcp_test_insert_semantic_vector(store, "node_vectors", project, id, NULL, 127, 0), + CBM_STORE_OK); + sqlite3 *db = cbm_store_get_db(store); + ASSERT_NOT_NULL(db); + ASSERT_EQ(sqlite3_create_function(db, "cbm_cosine_i8", 2, SQLITE_UTF8 | SQLITE_DETERMINISTIC, + NULL, mcp_test_semantic_scan_failure, NULL, NULL), + SQLITE_OK); + + response = cbm_mcp_handle_tool(srv, "search_graph", semantic_only_args); + ASSERT_NOT_NULL(response); + ASSERT_NOT_NULL(strstr(response, "\"isError\":true")); + char *inner = extract_text_content(response); + ASSERT_NOT_NULL(inner); + ASSERT_NOT_NULL(strstr(inner, "semantic search failed")); + ASSERT_NULL(strstr(inner, "moderate/full index")); + free(inner); + free(response); + + /* Combined with a structural filter the whole call fails closed too: no + * structural page pretends the semantic half simply found nothing. */ + response = cbm_mcp_handle_tool( + srv, "search_graph", + "{\"project\":\"semantic-store-error\",\"semantic_query\":[\"semantic-error-token\"]," + "\"name_pattern\":\"semantic_error\",\"format\":\"json\"}"); + ASSERT_NOT_NULL(response); + ASSERT_NOT_NULL(strstr(response, "\"isError\":true")); + ASSERT_NULL(strstr(response, "semantic_error_probe")); + free(response); + + cbm_mcp_server_free(srv); + PASS(); +} + TEST(tool_search_graph_budget_preserves_long_values_and_continuation) { cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); ASSERT_NOT_NULL(srv); @@ -19834,6 +19953,8 @@ SUITE(mcp) { RUN_TEST(tool_search_graph_rejects_bm25_and_semantic_query_together); RUN_TEST(tool_search_graph_semantic_ceiling_never_emits_unusable_continuation); RUN_TEST(tool_search_graph_semantic_pagination_is_lossless_and_independent); + RUN_TEST(tool_search_graph_semantic_query_rejects_non_string_elements); + RUN_TEST(tool_search_graph_semantic_store_error_fails_closed); RUN_TEST(tool_search_graph_budget_preserves_long_values_and_continuation); RUN_TEST(mcp_resource_discovery_methods_return_empty_lists); RUN_TEST(tool_query_graph_basic);