trending/codebase-memory-mcp
1
0
Fork 0
mirror of https://github.com/DeusData/codebase-memory-mcp.git synced 2026-11-09 09:11:00 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(mcp): review polish — honest totals, complete counts, quoted cells

Browse Source 
Fresh-eyes review of the phase-2/3 work; five defects, each against our own
stated principles (no silent caps, exact counts, same model in both
encodings):

- trace callees_total/callers_total counted test-file rows even when
  include_tests=false hides them from the table — the scalar disagreed with
  the enumerable set (a field-eval agent read callers_total=175 against a
  handful of visible rows and stopped trusting the tool). Totals now apply
  the same filter as the emitters; description documents the transitive
  (within-depth) semantics. Guard test: filtered=1 vs include_tests=2.
- cycles clamped the SCC list at 100 silently, losing the true count.
  cycles_total is now always exact and cycles_omitted is emitted when the
  display clamp engages (both encodings).
- detect_changes accepted a 'limit' param (referenced by its own omitted-
  hint) that the schema never declared — undiscoverable. Declared.
- detect_changes format:json lacked the impacted_modules rollup the tree
  encoding carries — model parity restored via a shared rollup helper.
- tree search extra-field cells were appended raw: a signature with spaces
  shifted every following column. Cells now route through the shared
  emitters (space-triggered quoting, '-' placeholder); the properties test
  pins the quoted form.

Full suite 6,373 passed / 0 failed; lint clean.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
Martin Vogel
2026-07-18 00:40:54 +02:00
parent 130a82bd10
commit 47e4d25dac
2 changed files with 202 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/mcp/mcp.c
+129 -53
View File
@@ -459,7 +459,9 @@ static const tool_def_t TOOLS[] = {
"Use INSTEAD OF grep for callers, dependencies, impact analysis, or data flow tracing. "
"RESPONSE: prefix-grouped tree rows — callees/callers grouped under their shared "
"qn-prefix, `name hop` per row (full qn = group prefix + dot + name); exact "
"callees_total/callers_total on every page; risk/args flags use a flat table. "
"callees_total/callers_total on every page = ALL nodes reachable within depth (transitive, "
"not just direct; test files excluded unless include_tests). risk/args flags use a flat "
"table. "
"`truncated: true` + `next` = more rows — pass next back as cursor. "
"format=\"json\" returns the SAME tree model as structured JSON.",
"{\"type\":\"object\",\"properties\":{\"function_name\":{\"type\":\"string\"},\"project\":{"
@@ -614,7 +616,10 @@ static const tool_def_t TOOLS[] = {
"the "
"changed symbols. outbound = what the changed code depends on. both = union.\"},"
"\"depth\":{\"type\":\"integer\",\"default\":2,\"description\":\"Max traversal hops from the "
"changed symbols.\"},\"base_branch\":{\"type\":"
"changed symbols.\"},\"limit\":{\"type\":\"integer\",\"default\":200,\"maximum\":5000,"
"\"description\":\"Per-symbol impacted rows shown (nearest hops first). impacted_total is "
"always exact and the impacted_modules rollup always complete regardless.\"},"
"\"base_branch\":{\"type\":"
"\"string\",\"default\":\"main\"},\"since\":{\"type\":\"string\",\"description\":"
"\"Git ref or tag to compare from (e.g. HEAD~5, v0.5.0). Diffs <ref>...HEAD.\"},"
"\"format\":{\"type\":\"string\",\"enum\":[\"tree\",\"json\"],\"default\":\"tree\"}},"
@@ -2800,8 +2805,10 @@ static void emit_search_results_tree(cbm_sb_t *sb, cbm_search_output_t *out, int
snprintf(row, sizeof(row), " %s %s %s %d %d", shortname,
sr->node.label ? sr->node.label : "", lines, sr->in_degree, sr->out_degree);
cbm_sb_append(sb, row);
/* Extra property columns (fields param), space-delimited; missing
* values emit as "-" so column positions stay stable. */
/* Extra property columns (fields param). Routed through the shared
* cell emitters so values with spaces (signatures, docstrings) are
* QUOTED — a raw append would shift every following column. Missing
* values emit as "-" (the emitter's empty-cell placeholder). */
if (nfields > 0) {
yyjson_doc *pd =
(sr->node.properties_json && sr->node.properties_json[0])
@@ -2810,19 +2817,17 @@ static void emit_search_results_tree(cbm_sb_t *sb, cbm_search_output_t *out, int
yyjson_val *pr = pd ? yyjson_doc_get_root(pd) : NULL;
for (int f = 0; f < nfields; f++) {
yyjson_val *v = (pr && yyjson_is_obj(pr)) ? yyjson_obj_get(pr, fields[f]) : NULL;
char cell[CBM_SZ_256];
if (v && yyjson_is_str(v)) {
snprintf(cell, sizeof(cell), " %s", yyjson_get_str(v));
cbm_tree_cell_str(sb, yyjson_get_str(v), false);
} else if (v && yyjson_is_int(v)) {
snprintf(cell, sizeof(cell), " %lld", (long long)yyjson_get_int(v));
cbm_tree_cell_int(sb, yyjson_get_int(v), false);
} else if (v && yyjson_is_real(v)) {
snprintf(cell, sizeof(cell), " %.3g", yyjson_get_real(v));
cbm_tree_cell_real(sb, yyjson_get_real(v), false);
} else if (v && yyjson_is_bool(v)) {
snprintf(cell, sizeof(cell), " %s", yyjson_get_bool(v) ? "true" : "false");
cbm_tree_cell_bool(sb, yyjson_get_bool(v), false);
} else {
snprintf(cell, sizeof(cell), " -");
cbm_tree_cell_str(sb, "", false); /* emits "-" */
}
cbm_sb_append(sb, cell);
}
if (pd) {
yyjson_doc_free(pd);
@@ -4287,11 +4292,12 @@ static void arch_join_list(char *buf, size_t sz, const char **items, int n) {
enum { ARCH_SCC_MAX_EDGES = 400000, ARCH_SCC_MAX_CYCLES = 100, ARCH_SCC_MEMBERS_SHOWN = 20 };
static int arch_compute_cycles(cbm_store_t *store, const char *project, int64_t ***out_members,
int **out_sizes, int *out_ncycles, int *scanned_edges,
bool *edges_truncated) {
int **out_sizes, int *out_ncycles, int *out_total_cycles,
int *scanned_edges, bool *edges_truncated) {
*out_members = NULL;
*out_sizes = NULL;
*out_ncycles = 0;
*out_total_cycles = 0;
*scanned_edges = 0;
*edges_truncated = false;
@@ -4340,6 +4346,7 @@ static int arch_compute_cycles(cbm_store_t *store, const char *project, int64_t
ncyc++;
}
}
*out_total_cycles = ncyc; /* the TRUE count, before the display clamp */
if (ncyc > ARCH_SCC_MAX_CYCLES) {
ncyc = ARCH_SCC_MAX_CYCLES;
}
@@ -4714,17 +4721,25 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
int64_t **members = NULL;
int *sizes = NULL;
int ncyc = 0;
int total_cyc = 0;
int scanned = 0;
bool etrunc = false;
if (arch_compute_cycles(store, project, &members, &sizes, &ncyc, &scanned, &etrunc) ==
CBM_STORE_OK) {
if (arch_compute_cycles(store, project, &members, &sizes, &ncyc, &total_cyc, &scanned,
&etrunc) == CBM_STORE_OK) {
cbm_tree_scalar_int(&sb, "call_edges_scanned", scanned);
cbm_tree_scalar_int(&sb, "cycles_total", total_cyc);
if (etrunc) {
cbm_tree_scalar_bool(&sb, "cycles_partial", true);
cbm_tree_scalar_str(&sb, "cycles_hint",
"call graph exceeded the scan budget; cycle list may be "
"incomplete");
}
if (total_cyc > ncyc) {
char omit[CBM_SZ_128];
snprintf(omit, sizeof(omit), "cycles_omitted: %d (showing the first %d)\n",
total_cyc - ncyc, ncyc);
cbm_sb_append(&sb, omit);
}
char hdr[CBM_SZ_128];
snprintf(hdr, sizeof(hdr),
"cycles: %d (rows: size members; circular CALLS dependencies)\n", ncyc);
@@ -4998,11 +5013,13 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
int64_t **members = NULL;
int *sizes = NULL;
int ncyc = 0;
int total_cyc = 0;
int scanned = 0;
bool etrunc = false;
if (arch_compute_cycles(store, project, &members, &sizes, &ncyc, &scanned, &etrunc) ==
CBM_STORE_OK) {
if (arch_compute_cycles(store, project, &members, &sizes, &ncyc, &total_cyc, &scanned,
&etrunc) == CBM_STORE_OK) {
yyjson_mut_obj_add_int(doc, root, "call_edges_scanned", scanned);
yyjson_mut_obj_add_int(doc, root, "cycles_total", total_cyc);
yyjson_mut_obj_add_bool(doc, root, "cycles_partial", etrunc);
yyjson_mut_val *cyc = yyjson_mut_arr(doc);
for (int c = 0; c < ncyc; c++) {
@@ -5867,6 +5884,24 @@ static char *handle_trace_call_path(cbm_mcp_server_t *srv, const char *args) {
view_in.visited += in_start;
view_in.visited_count = in_len;
/* Totals must count what the caller can actually enumerate: when
* include_tests=false the tables hide test-file rows, so raw
* visited_count overstated the reachable set (a field-eval agent read
* callers_total=175 against a handful of visible rows and distrusted
* the tool). Count with the same filter the emitters apply. */
int out_total = 0;
for (int i = 0; i < tr_out.visited_count; i++) {
if (include_tests || !is_test_file(tr_out.visited[i].node.file_path)) {
out_total++;
}
}
int in_total = 0;
for (int i = 0; i < tr_in.visited_count; i++) {
if (include_tests || !is_test_file(tr_in.visited[i].node.file_path)) {
in_total++;
}
}
char *json = NULL;
if (!trace_legacy_json) {
cbm_sb_t sb;
@@ -5880,7 +5915,7 @@ static char *handle_trace_call_path(cbm_mcp_server_t *srv, const char *args) {
* table (extra columns) in the same tree syntax. */
bool flat_trace = risk_labels || data_flow;
if (do_outbound) {
cbm_tree_scalar_int(&sb, "callees_total", tr_out.visited_count);
cbm_tree_scalar_int(&sb, "callees_total", out_total);
if (flat_trace) {
bfs_to_toon_table(&sb, "callees", &view_out, risk_labels, include_tests, data_flow);
} else {
@@ -5888,7 +5923,7 @@ static char *handle_trace_call_path(cbm_mcp_server_t *srv, const char *args) {
}
}
if (do_inbound) {
cbm_tree_scalar_int(&sb, "callers_total", tr_in.visited_count);
cbm_tree_scalar_int(&sb, "callers_total", in_total);
if (flat_trace) {
bfs_to_toon_table(&sb, "callers", &view_in, risk_labels, include_tests, data_flow);
} else {
@@ -5915,13 +5950,13 @@ static char *handle_trace_call_path(cbm_mcp_server_t *srv, const char *args) {
yyjson_mut_obj_add_str(doc, root, "mode", mode);
}
if (do_outbound) {
yyjson_mut_obj_add_int(doc, root, "callees_total", tr_out.visited_count);
yyjson_mut_obj_add_int(doc, root, "callees_total", out_total);
yyjson_mut_obj_add_val(
doc, root, "callees",
bfs_to_tree_json(doc, &view_out, risk_labels, include_tests, data_flow));
}
if (do_inbound) {
yyjson_mut_obj_add_int(doc, root, "callers_total", tr_in.visited_count);
yyjson_mut_obj_add_int(doc, root, "callers_total", in_total);
yyjson_mut_obj_add_val(
doc, root, "callers",
bfs_to_tree_json(doc, &view_in, risk_labels, include_tests, data_flow));
@@ -8551,6 +8586,39 @@ static void detect_module_of(const char *file, char *out, size_t outsz) {
out[len] = '\0';
}
/* Aggregate the impact set into the 2-segment module rollup. Fills up to
* DETECT_MODCAP (module, count) pairs; symbols beyond the cap land in
* *overflow (surfaced as "(other)", never silently dropped). Shared by the
* tree and json emitters so both encodings carry the same model. */
enum { DETECT_MODCAP = 256 };
static int detect_module_rollup(const cbm_traverse_result_t *impact, char mods[][CBM_SZ_128],
int *mcnt, int *overflow) {
int nmods = 0;
*overflow = 0;
for (int i = 0; i < impact->visited_count; i++) {
char m[CBM_SZ_128];
detect_module_of(impact->visited[i].node.file_path, m, sizeof(m));
int j = 0;
for (; j < nmods; j++) {
if (strcmp(mods[j], m) == 0) {
mcnt[j]++;
break;
}
}
if (j == nmods) {
if (nmods < DETECT_MODCAP) {
snprintf(mods[nmods], CBM_SZ_128, "%s", m);
mcnt[nmods] = 1;
nmods++;
} else {
(*overflow)++;
}
}
}
return nmods;
}
/* Emit the impacted set as a grouped tree leg: rows grouped under their shared
* (qn-prefix, file), `name label hop` per row. At most `limit` rows are listed
* (the visited array is hop-ordered, so the closest — highest-signal — impact
@@ -8847,41 +8915,24 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
/* module rollup: a quotient view of the blast radius */
if (impact.visited_count > 0) {
cbm_sb_append(&sb, "impacted_modules: (rows: module count)\n");
enum { MODCAP = 256 };
char mods[MODCAP][CBM_SZ_128];
int mcnt[MODCAP];
int nmods = 0;
int overflow = 0; /* symbols whose module didn't fit MODCAP */
for (int i = 0; i < impact.visited_count; i++) {
char m[CBM_SZ_128];
detect_module_of(impact.visited[i].node.file_path, m, sizeof(m));
int j = 0;
for (; j < nmods; j++) {
if (strcmp(mods[j], m) == 0) {
mcnt[j]++;
break;
}
char (*mods)[CBM_SZ_128] = malloc(DETECT_MODCAP * CBM_SZ_128);
int *mcnt = malloc(DETECT_MODCAP * sizeof(int));
if (mods && mcnt) {
int overflow = 0;
int nmods = detect_module_rollup(&impact, mods, mcnt, &overflow);
for (int j = 0; j < nmods; j++) {
char mrow[CBM_SZ_256];
snprintf(mrow, sizeof(mrow), " %s %d\n", mods[j], mcnt[j]);
cbm_sb_append(&sb, mrow);
}
if (j == nmods) {
if (nmods < MODCAP) {
snprintf(mods[nmods], sizeof(mods[nmods]), "%s", m);
mcnt[nmods] = 1;
nmods++;
} else {
overflow++; /* never silently dropped — surfaced below */
}
if (overflow > 0) {
char orow[CBM_SZ_128];
snprintf(orow, sizeof(orow), " (other) %d\n", overflow);
cbm_sb_append(&sb, orow);
}
}
for (int j = 0; j < nmods; j++) {
char mrow[CBM_SZ_256];
snprintf(mrow, sizeof(mrow), " %s %d\n", mods[j], mcnt[j]);
cbm_sb_append(&sb, mrow);
}
if (overflow > 0) {
char orow[CBM_SZ_128];
snprintf(orow, sizeof(orow), " (other) %d\n", overflow);
cbm_sb_append(&sb, orow);
}
free(mods);
free(mcnt);
}
if (truncated) {
cbm_tree_scalar_bool(&sb, "truncated", true);
@@ -8925,6 +8976,31 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
yyjson_mut_arr_add_val(imp, o);
}
yyjson_mut_obj_add_val(doc, root_obj, "impacted", imp);
/* Model parity with the tree encoding: the complete module rollup. */
if (impact.visited_count > 0) {
char (*mods)[CBM_SZ_128] = malloc(DETECT_MODCAP * CBM_SZ_128);
int *mcnt = malloc(DETECT_MODCAP * sizeof(int));
if (mods && mcnt) {
int overflow = 0;
int nmods = detect_module_rollup(&impact, mods, mcnt, &overflow);
yyjson_mut_val *rollup = yyjson_mut_arr(doc);
for (int j = 0; j < nmods; j++) {
yyjson_mut_val *o = yyjson_mut_obj(doc);
yyjson_mut_obj_add_strcpy(doc, o, "module", mods[j]);
yyjson_mut_obj_add_int(doc, o, "count", mcnt[j]);
yyjson_mut_arr_add_val(rollup, o);
}
if (overflow > 0) {
yyjson_mut_val *o = yyjson_mut_obj(doc);
yyjson_mut_obj_add_strcpy(doc, o, "module", "(other)");
yyjson_mut_obj_add_int(doc, o, "count", overflow);
yyjson_mut_arr_add_val(rollup, o);
}
yyjson_mut_obj_add_val(doc, root_obj, "impacted_modules", rollup);
}
free(mods);
free(mcnt);
}
yyjson_mut_obj_add_bool(doc, root_obj, "truncated", truncated);
if (is_error) {
char hint_buf[CBM_SZ_256];
tests/test_mcp.c
+73
View File
@@ -1090,6 +1090,76 @@ static cbm_mcp_server_t *setup_snippet_server(char *tmp_dir, size_t tmp_sz);
static void cleanup_snippet_dir(const char *tmp_dir);
static char *extract_text_content(const char *mcp_result);
/* callers_total/callees_total must count what the caller can enumerate: with
* include_tests=false (default) test-file rows are hidden from the table, so
* the totals must apply the same filter — a raw visited_count overstated the
* set (field-eval agent read callers_total=175 against 2 visible rows and
* distrusted the tool). */
TEST(tool_trace_totals_respect_test_filter) {
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
ASSERT_NOT_NULL(srv);
cbm_store_t *st = cbm_mcp_server_store(srv);
const char *proj = "totproj";
cbm_mcp_server_set_project(srv, proj);
cbm_store_upsert_project(st, proj, "/tmp/tot");
cbm_node_t tgt = {.project = proj,
.label = "Function",
.name = "tgt",
.qualified_name = "totproj.a.tgt",
.file_path = "a.c",
.start_line = 1,
.end_line = 5};
int64_t tid = cbm_store_upsert_node(st, &tgt);
ASSERT_GT(tid, 0);
cbm_node_t prod = {.project = proj,
.label = "Function",
.name = "prod_caller",
.qualified_name = "totproj.a.prod_caller",
.file_path = "a.c",
.start_line = 10,
.end_line = 15};
int64_t pid = cbm_store_upsert_node(st, &prod);
ASSERT_GT(pid, 0);
cbm_node_t tst = {.project = proj,
.label = "Function",
.name = "test_caller",
.qualified_name = "totproj.t.test_caller",
.file_path = "tests/test_x.c",
.start_line = 1,
.end_line = 5};
int64_t xid = cbm_store_upsert_node(st, &tst);
ASSERT_GT(xid, 0);
cbm_edge_t e1 = {.project = proj, .source_id = pid, .target_id = tid, .type = "CALLS"};
ASSERT_GT(cbm_store_insert_edge(st, &e1), 0);
cbm_edge_t e2 = {.project = proj, .source_id = xid, .target_id = tid, .type = "CALLS"};
ASSERT_GT(cbm_store_insert_edge(st, &e2), 0);
char *resp = cbm_mcp_server_handle(
srv, "{\"jsonrpc\":\"2.0\",\"id\":90,\"method\":\"tools/call\",\"params\":{"
"\"name\":\"trace_call_path\",\"arguments\":{\"project\":\"totproj\","
"\"function_name\":\"tgt\",\"direction\":\"inbound\"}}}");
ASSERT_NOT_NULL(resp);
char *inner = extract_text_content(resp);
free(resp);
ASSERT_NOT_NULL(inner);
ASSERT_NOT_NULL(strstr(inner, "callers_total: 1")); /* test row filtered */
free(inner);
resp = cbm_mcp_server_handle(
srv, "{\"jsonrpc\":\"2.0\",\"id\":91,\"method\":\"tools/call\",\"params\":{"
"\"name\":\"trace_call_path\",\"arguments\":{\"project\":\"totproj\","
"\"function_name\":\"tgt\",\"direction\":\"inbound\",\"include_tests\":true}}}");
ASSERT_NOT_NULL(resp);
inner = extract_text_content(resp);
free(resp);
ASSERT_NOT_NULL(inner);
ASSERT_NOT_NULL(strstr(inner, "callers_total: 2")); /* both visible now */
free(inner);
cbm_mcp_server_free(srv);
PASS();
}
/* SCC condensation (get_architecture aspect "cycles"): a 3-function CALLS
* cycle A->B->C->A must be reported as one circular dependency of size 3 with
* all three members; a separate acyclic chain (D->E) must NOT appear. The
@@ -1255,6 +1325,8 @@ TEST(tool_search_graph_includes_node_properties) {
inner = extract_text_content(resp);
ASSERT_NOT_NULL(inner);
ASSERT_NOT_NULL(strstr(inner, "(rows: name label lines in out signature;"));
/* values with spaces are QUOTED so column positions survive */
ASSERT_NOT_NULL(strstr(inner, "\"func HandleRequest() error\""));
ASSERT_NOT_NULL(strstr(inner, "func HandleRequest"));
free(inner);
free(resp);
@@ -6936,6 +7008,7 @@ SUITE(mcp) {
RUN_TEST(tool_get_graph_schema_empty);
RUN_TEST(tool_unknown_tool);
RUN_TEST(tool_search_graph_basic);
RUN_TEST(tool_trace_totals_respect_test_filter);
RUN_TEST(tool_get_architecture_cycles_detects_scc);
RUN_TEST(tool_get_code_snippet_clips_whole_file_node);
RUN_TEST(tool_search_graph_includes_node_properties);