Skip to content

Commit db23d56

Browse files
author
Martin Vogel
committed
fix(mcp,ci): harden get_architecture serialization + CodeQL gate
Follow-up to #281 (handle_get_architecture) plus a CodeQL workflow upgrade developed in parallel; bundling into one commit because the test suite had to land alongside both. mcp.c — handle_get_architecture - NULL-coerce every const-char* field in the architecture sections via `x ? x : ""`, matching the rest of mcp.c (search_graph, etc.). Without this, a NULL field becomes a missing JSON key instead of an empty string; yyjson_mut_obj_add_str returns false on NULL and silently no-ops, so an inconsistent omission could surprise callers. - Serialize two more architecture aspects that #281 left on the floor: services (cbm_service_link_t: from/to/type/count) and clusters (cbm_cluster_info_t: id/label/members/cohesion plus the top_nodes / packages / edge_types string arrays). The store-side computation populates these for aspects=["all"] / explicit names, so dropping them in the serializer was data loss. tests/test_mcp.c - New tool_get_architecture_emits_populated_sections regression test. Uses a minimal inline fixture (single Function node tagged with "is_entry_point": true) since arch_entry_points reads that flag out of properties_json. Asserts the response contains both an "entry_points" array and the function name — neither would appear before #281 because handle_get_architecture never called cbm_store_get_architecture. - extract_text_content drilled too shallow: it pulled "content" only from the JSON root, so it worked for cbm_mcp_handle_tool but silently fell through to the raw response for cbm_mcp_server_handle (where content lives under .result.content). Added a fallback that checks .result.content; both unwrappers tested by existing fixtures. ci(codeql) - Run on pull_request to main, not just push; surfaces findings on the PR instead of after merge. - Pull custom queries from ./codeql via `queries: +./codeql`. - Capture SARIF output and fail the job on any error-level finding, using jq to enumerate rule id, file:line, and message text in the GitHub Actions error annotation. Warnings are still reported as before; only errors block. Full suite: 2842 passed, 0 failed.
1 parent 96b49aa commit db23d56

3 files changed

Lines changed: 164 additions & 18 deletions

File tree

.github/workflows/codeql.yml

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,40 @@ jobs:
1515
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
1616

1717
- name: Install build dependencies
18-
run: sudo apt-get update && sudo apt-get install -y zlib1g-dev
18+
run: sudo apt-get update && sudo apt-get install -y zlib1g-dev jq
1919

2020
- name: Initialize CodeQL
2121
uses: github/codeql-action/init@95e58e9a2cdfd71adc6e0353d5c52f41a045d225 # v4
2222
with:
2323
languages: c-cpp
2424
build-mode: manual
25+
queries: +./codeql
2526

2627
- name: Build for CodeQL analysis
2728
run: scripts/build.sh
2829

2930
- name: Perform CodeQL Analysis
31+
id: analyze
3032
uses: github/codeql-action/analyze@95e58e9a2cdfd71adc6e0353d5c52f41a045d225 # v4
3133
with:
3234
category: "/language:c-cpp"
35+
output: codeql-results
36+
37+
- name: Fail on CodeQL error-level findings
38+
run: |
39+
sarif=$(find codeql-results -name '*.sarif' | head -1)
40+
if [[ -z "$sarif" ]]; then
41+
echo "::error::No SARIF output found"; exit 1
42+
fi
43+
err=$(jq '[.runs[].results[]? | select((.level // "warning") == "error")] | length' "$sarif")
44+
warn=$(jq '[.runs[].results[]? | select((.level // "warning") == "warning")] | length' "$sarif")
45+
echo "CodeQL findings: $err error(s), $warn warning(s)"
46+
if [[ "$err" -gt 0 ]]; then
47+
jq -r '
48+
.runs[].results[]?
49+
| select((.level // "warning") == "error")
50+
| "\(.locations[0].physicalLocation.artifactLocation.uri):\(.locations[0].physicalLocation.region.startLine) [\(.ruleId)] \(.message.text)"
51+
' "$sarif"
52+
echo "::error::$err CodeQL error-level finding(s) — failing build."
53+
exit 1
54+
fi

src/mcp/mcp.c

Lines changed: 83 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1860,7 +1860,8 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
18601860
yyjson_mut_val *langs = yyjson_mut_arr(doc);
18611861
for (int i = 0; i < arch.language_count; i++) {
18621862
yyjson_mut_val *item = yyjson_mut_obj(doc);
1863-
yyjson_mut_obj_add_str(doc, item, "language", arch.languages[i].language);
1863+
yyjson_mut_obj_add_str(doc, item, "language",
1864+
arch.languages[i].language ? arch.languages[i].language : "");
18641865
yyjson_mut_obj_add_int(doc, item, "file_count", arch.languages[i].file_count);
18651866
yyjson_mut_arr_add_val(langs, item);
18661867
}
@@ -1872,7 +1873,8 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
18721873
yyjson_mut_val *pkgs = yyjson_mut_arr(doc);
18731874
for (int i = 0; i < arch.package_count; i++) {
18741875
yyjson_mut_val *item = yyjson_mut_obj(doc);
1875-
yyjson_mut_obj_add_str(doc, item, "name", arch.packages[i].name);
1876+
yyjson_mut_obj_add_str(doc, item, "name",
1877+
arch.packages[i].name ? arch.packages[i].name : "");
18761878
yyjson_mut_obj_add_int(doc, item, "node_count", arch.packages[i].node_count);
18771879
yyjson_mut_obj_add_int(doc, item, "fan_in", arch.packages[i].fan_in);
18781880
yyjson_mut_obj_add_int(doc, item, "fan_out", arch.packages[i].fan_out);
@@ -1886,10 +1888,14 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
18861888
yyjson_mut_val *eps = yyjson_mut_arr(doc);
18871889
for (int i = 0; i < arch.entry_point_count; i++) {
18881890
yyjson_mut_val *item = yyjson_mut_obj(doc);
1889-
yyjson_mut_obj_add_str(doc, item, "name", arch.entry_points[i].name);
1891+
yyjson_mut_obj_add_str(doc, item, "name",
1892+
arch.entry_points[i].name ? arch.entry_points[i].name : "");
18901893
yyjson_mut_obj_add_str(doc, item, "qualified_name",
1891-
arch.entry_points[i].qualified_name);
1892-
yyjson_mut_obj_add_str(doc, item, "file", arch.entry_points[i].file);
1894+
arch.entry_points[i].qualified_name
1895+
? arch.entry_points[i].qualified_name
1896+
: "");
1897+
yyjson_mut_obj_add_str(doc, item, "file",
1898+
arch.entry_points[i].file ? arch.entry_points[i].file : "");
18931899
yyjson_mut_arr_add_val(eps, item);
18941900
}
18951901
yyjson_mut_obj_add_val(doc, root, "entry_points", eps);
@@ -1900,9 +1906,12 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
19001906
yyjson_mut_val *routes = yyjson_mut_arr(doc);
19011907
for (int i = 0; i < arch.route_count; i++) {
19021908
yyjson_mut_val *item = yyjson_mut_obj(doc);
1903-
yyjson_mut_obj_add_str(doc, item, "method", arch.routes[i].method);
1904-
yyjson_mut_obj_add_str(doc, item, "path", arch.routes[i].path);
1905-
yyjson_mut_obj_add_str(doc, item, "handler", arch.routes[i].handler);
1909+
yyjson_mut_obj_add_str(doc, item, "method",
1910+
arch.routes[i].method ? arch.routes[i].method : "");
1911+
yyjson_mut_obj_add_str(doc, item, "path",
1912+
arch.routes[i].path ? arch.routes[i].path : "");
1913+
yyjson_mut_obj_add_str(doc, item, "handler",
1914+
arch.routes[i].handler ? arch.routes[i].handler : "");
19061915
yyjson_mut_arr_add_val(routes, item);
19071916
}
19081917
yyjson_mut_obj_add_val(doc, root, "routes", routes);
@@ -1913,9 +1922,12 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
19131922
yyjson_mut_val *hotspots = yyjson_mut_arr(doc);
19141923
for (int i = 0; i < arch.hotspot_count; i++) {
19151924
yyjson_mut_val *item = yyjson_mut_obj(doc);
1916-
yyjson_mut_obj_add_str(doc, item, "name", arch.hotspots[i].name);
1925+
yyjson_mut_obj_add_str(doc, item, "name",
1926+
arch.hotspots[i].name ? arch.hotspots[i].name : "");
19171927
yyjson_mut_obj_add_str(doc, item, "qualified_name",
1918-
arch.hotspots[i].qualified_name);
1928+
arch.hotspots[i].qualified_name
1929+
? arch.hotspots[i].qualified_name
1930+
: "");
19191931
yyjson_mut_obj_add_int(doc, item, "fan_in", arch.hotspots[i].fan_in);
19201932
yyjson_mut_arr_add_val(hotspots, item);
19211933
}
@@ -1927,34 +1939,88 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
19271939
yyjson_mut_val *boundaries = yyjson_mut_arr(doc);
19281940
for (int i = 0; i < arch.boundary_count; i++) {
19291941
yyjson_mut_val *item = yyjson_mut_obj(doc);
1930-
yyjson_mut_obj_add_str(doc, item, "from", arch.boundaries[i].from);
1931-
yyjson_mut_obj_add_str(doc, item, "to", arch.boundaries[i].to);
1942+
yyjson_mut_obj_add_str(doc, item, "from",
1943+
arch.boundaries[i].from ? arch.boundaries[i].from : "");
1944+
yyjson_mut_obj_add_str(doc, item, "to",
1945+
arch.boundaries[i].to ? arch.boundaries[i].to : "");
19321946
yyjson_mut_obj_add_int(doc, item, "call_count", arch.boundaries[i].call_count);
19331947
yyjson_mut_arr_add_val(boundaries, item);
19341948
}
19351949
yyjson_mut_obj_add_val(doc, root, "boundaries", boundaries);
19361950
}
19371951

1952+
/* Cross-service links (HTTP/async between services) */
1953+
if (arch.service_count > 0) {
1954+
yyjson_mut_val *services = yyjson_mut_arr(doc);
1955+
for (int i = 0; i < arch.service_count; i++) {
1956+
yyjson_mut_val *item = yyjson_mut_obj(doc);
1957+
yyjson_mut_obj_add_str(doc, item, "from",
1958+
arch.services[i].from ? arch.services[i].from : "");
1959+
yyjson_mut_obj_add_str(doc, item, "to",
1960+
arch.services[i].to ? arch.services[i].to : "");
1961+
yyjson_mut_obj_add_str(doc, item, "type",
1962+
arch.services[i].type ? arch.services[i].type : "");
1963+
yyjson_mut_obj_add_int(doc, item, "count", arch.services[i].count);
1964+
yyjson_mut_arr_add_val(services, item);
1965+
}
1966+
yyjson_mut_obj_add_val(doc, root, "services", services);
1967+
}
1968+
19381969
/* Package layers */
19391970
if (arch.layer_count > 0) {
19401971
yyjson_mut_val *layers = yyjson_mut_arr(doc);
19411972
for (int i = 0; i < arch.layer_count; i++) {
19421973
yyjson_mut_val *item = yyjson_mut_obj(doc);
1943-
yyjson_mut_obj_add_str(doc, item, "name", arch.layers[i].name);
1944-
yyjson_mut_obj_add_str(doc, item, "layer", arch.layers[i].layer);
1945-
yyjson_mut_obj_add_str(doc, item, "reason", arch.layers[i].reason);
1974+
yyjson_mut_obj_add_str(doc, item, "name",
1975+
arch.layers[i].name ? arch.layers[i].name : "");
1976+
yyjson_mut_obj_add_str(doc, item, "layer",
1977+
arch.layers[i].layer ? arch.layers[i].layer : "");
1978+
yyjson_mut_obj_add_str(doc, item, "reason",
1979+
arch.layers[i].reason ? arch.layers[i].reason : "");
19461980
yyjson_mut_arr_add_val(layers, item);
19471981
}
19481982
yyjson_mut_obj_add_val(doc, root, "layers", layers);
19491983
}
19501984

1985+
/* Clusters (community detection) */
1986+
if (arch.cluster_count > 0) {
1987+
yyjson_mut_val *clusters = yyjson_mut_arr(doc);
1988+
for (int i = 0; i < arch.cluster_count; i++) {
1989+
const cbm_cluster_info_t *c = &arch.clusters[i];
1990+
yyjson_mut_val *item = yyjson_mut_obj(doc);
1991+
yyjson_mut_obj_add_int(doc, item, "id", c->id);
1992+
yyjson_mut_obj_add_str(doc, item, "label", c->label ? c->label : "");
1993+
yyjson_mut_obj_add_int(doc, item, "members", c->members);
1994+
yyjson_mut_obj_add_real(doc, item, "cohesion", c->cohesion);
1995+
yyjson_mut_val *top = yyjson_mut_arr(doc);
1996+
for (int j = 0; j < c->top_node_count; j++) {
1997+
yyjson_mut_arr_add_str(doc, top, c->top_nodes[j] ? c->top_nodes[j] : "");
1998+
}
1999+
yyjson_mut_obj_add_val(doc, item, "top_nodes", top);
2000+
yyjson_mut_val *pkgs = yyjson_mut_arr(doc);
2001+
for (int j = 0; j < c->package_count; j++) {
2002+
yyjson_mut_arr_add_str(doc, pkgs, c->packages[j] ? c->packages[j] : "");
2003+
}
2004+
yyjson_mut_obj_add_val(doc, item, "packages", pkgs);
2005+
yyjson_mut_val *etypes = yyjson_mut_arr(doc);
2006+
for (int j = 0; j < c->edge_type_count; j++) {
2007+
yyjson_mut_arr_add_str(doc, etypes, c->edge_types[j] ? c->edge_types[j] : "");
2008+
}
2009+
yyjson_mut_obj_add_val(doc, item, "edge_types", etypes);
2010+
yyjson_mut_arr_add_val(clusters, item);
2011+
}
2012+
yyjson_mut_obj_add_val(doc, root, "clusters", clusters);
2013+
}
2014+
19512015
/* File tree */
19522016
if (arch.file_tree_count > 0) {
19532017
yyjson_mut_val *file_tree = yyjson_mut_arr(doc);
19542018
for (int i = 0; i < arch.file_tree_count; i++) {
19552019
yyjson_mut_val *item = yyjson_mut_obj(doc);
1956-
yyjson_mut_obj_add_str(doc, item, "path", arch.file_tree[i].path);
1957-
yyjson_mut_obj_add_str(doc, item, "type", arch.file_tree[i].type);
2020+
yyjson_mut_obj_add_str(doc, item, "path",
2021+
arch.file_tree[i].path ? arch.file_tree[i].path : "");
2022+
yyjson_mut_obj_add_str(doc, item, "type",
2023+
arch.file_tree[i].type ? arch.file_tree[i].type : "");
19582024
yyjson_mut_obj_add_int(doc, item, "children", arch.file_tree[i].children);
19592025
yyjson_mut_arr_add_val(file_tree, item);
19602026
}

tests/test_mcp.c

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,56 @@ TEST(tool_get_architecture_empty) {
525525
PASS();
526526
}
527527

528+
/* Regression for #281: handle_get_architecture must actually call
529+
* cbm_store_get_architecture and surface its sections. Before the fix
530+
* only label/edge histograms were emitted regardless of which aspects
531+
* were requested. The store-side arch_entry_points query reads
532+
* properties.is_entry_point on Function nodes, so we tag one node and
533+
* assert the resulting JSON surfaces an "entry_points" array containing
534+
* the tagged function — which is impossible without the wiring. */
535+
TEST(tool_get_architecture_emits_populated_sections) {
536+
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
537+
ASSERT_NOT_NULL(srv);
538+
539+
cbm_store_t *st = cbm_mcp_server_store(srv);
540+
ASSERT_NOT_NULL(st);
541+
542+
const char *proj = "arch-test";
543+
cbm_mcp_server_set_project(srv, proj);
544+
cbm_store_upsert_project(st, proj, "/tmp/arch-test");
545+
546+
cbm_node_t main_fn = {0};
547+
main_fn.project = proj;
548+
main_fn.label = "Function";
549+
main_fn.name = "main";
550+
main_fn.qualified_name = "arch-test.cmd.main";
551+
main_fn.file_path = "cmd/main.go";
552+
main_fn.start_line = 1;
553+
main_fn.end_line = 3;
554+
main_fn.properties_json = "{\"is_entry_point\":true}";
555+
ASSERT_GT(cbm_store_upsert_node(st, &main_fn), 0);
556+
557+
char *resp = cbm_mcp_server_handle(
558+
srv, "{\"jsonrpc\":\"2.0\",\"id\":91,\"method\":\"tools/call\","
559+
"\"params\":{\"name\":\"get_architecture\","
560+
"\"arguments\":{\"project\":\"arch-test\",\"aspects\":[\"all\"]}}}");
561+
ASSERT_NOT_NULL(resp);
562+
char *inner = extract_text_content(resp);
563+
ASSERT_NOT_NULL(inner);
564+
565+
/* The handler always emits node/edge counts and schema histograms;
566+
* those existed before #281. The "entry_points" array only appears
567+
* when cbm_store_get_architecture is actually called and its result
568+
* is serialized — which is exactly what #281 wires up. */
569+
ASSERT_NOT_NULL(strstr(inner, "\"entry_points\""));
570+
ASSERT_NOT_NULL(strstr(inner, "main"));
571+
572+
free(inner);
573+
free(resp);
574+
cbm_mcp_server_free(srv);
575+
PASS();
576+
}
577+
528578
TEST(tool_query_graph_missing_query) {
529579
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
530580

@@ -1001,6 +1051,13 @@ static char *extract_text_content(const char *mcp_result) {
10011051
return strdup(mcp_result); /* fallback */
10021052
yyjson_val *root = yyjson_doc_get_root(doc);
10031053
yyjson_val *content = yyjson_obj_get(root, "content");
1054+
if (!content) {
1055+
/* Handle JSON-RPC wrapper: {"jsonrpc":...,"result":{"content":[...]}} */
1056+
yyjson_val *rpc_result = yyjson_obj_get(root, "result");
1057+
if (rpc_result) {
1058+
content = yyjson_obj_get(rpc_result, "content");
1059+
}
1060+
}
10041061
if (!content || !yyjson_is_arr(content)) {
10051062
yyjson_doc_free(doc);
10061063
return strdup(mcp_result);
@@ -1739,6 +1796,7 @@ SUITE(mcp) {
17391796
RUN_TEST(tool_trace_missing_function_name);
17401797
RUN_TEST(tool_delete_project_not_found);
17411798
RUN_TEST(tool_get_architecture_empty);
1799+
RUN_TEST(tool_get_architecture_emits_populated_sections);
17421800
RUN_TEST(tool_query_graph_missing_query);
17431801

17441802
/* Pipeline-dependent tool handlers */

0 commit comments

Comments
 (0)