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.
This commit is contained in:
@@ -15,18 +15,40 @@ jobs:
|
||||
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
|
||||
|
||||
- name: Install build dependencies
|
||||
run: sudo apt-get update && sudo apt-get install -y zlib1g-dev
|
||||
run: sudo apt-get update && sudo apt-get install -y zlib1g-dev jq
|
||||
|
||||
- name: Initialize CodeQL
|
||||
uses: github/codeql-action/init@95e58e9a2cdfd71adc6e0353d5c52f41a045d225 # v4
|
||||
with:
|
||||
languages: c-cpp
|
||||
build-mode: manual
|
||||
queries: +./codeql
|
||||
|
||||
- name: Build for CodeQL analysis
|
||||
run: scripts/build.sh
|
||||
|
||||
- name: Perform CodeQL Analysis
|
||||
id: analyze
|
||||
uses: github/codeql-action/analyze@95e58e9a2cdfd71adc6e0353d5c52f41a045d225 # v4
|
||||
with:
|
||||
category: "/language:c-cpp"
|
||||
output: codeql-results
|
||||
|
||||
- name: Fail on CodeQL error-level findings
|
||||
run: |
|
||||
sarif=$(find codeql-results -name '*.sarif' | head -1)
|
||||
if [[ -z "$sarif" ]]; then
|
||||
echo "::error::No SARIF output found"; exit 1
|
||||
fi
|
||||
err=$(jq '[.runs[].results[]? | select((.level // "warning") == "error")] | length' "$sarif")
|
||||
warn=$(jq '[.runs[].results[]? | select((.level // "warning") == "warning")] | length' "$sarif")
|
||||
echo "CodeQL findings: $err error(s), $warn warning(s)"
|
||||
if [[ "$err" -gt 0 ]]; then
|
||||
jq -r '
|
||||
.runs[].results[]?
|
||||
| select((.level // "warning") == "error")
|
||||
| "\(.locations[0].physicalLocation.artifactLocation.uri):\(.locations[0].physicalLocation.region.startLine) [\(.ruleId)] \(.message.text)"
|
||||
' "$sarif"
|
||||
echo "::error::$err CodeQL error-level finding(s) — failing build."
|
||||
exit 1
|
||||
fi
|
||||
|
||||
+83
-17
@@ -1860,7 +1860,8 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
|
||||
yyjson_mut_val *langs = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.language_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "language", arch.languages[i].language);
|
||||
yyjson_mut_obj_add_str(doc, item, "language",
|
||||
arch.languages[i].language ? arch.languages[i].language : "");
|
||||
yyjson_mut_obj_add_int(doc, item, "file_count", arch.languages[i].file_count);
|
||||
yyjson_mut_arr_add_val(langs, item);
|
||||
}
|
||||
@@ -1872,7 +1873,8 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
|
||||
yyjson_mut_val *pkgs = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.package_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "name", arch.packages[i].name);
|
||||
yyjson_mut_obj_add_str(doc, item, "name",
|
||||
arch.packages[i].name ? arch.packages[i].name : "");
|
||||
yyjson_mut_obj_add_int(doc, item, "node_count", arch.packages[i].node_count);
|
||||
yyjson_mut_obj_add_int(doc, item, "fan_in", arch.packages[i].fan_in);
|
||||
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) {
|
||||
yyjson_mut_val *eps = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.entry_point_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "name", arch.entry_points[i].name);
|
||||
yyjson_mut_obj_add_str(doc, item, "name",
|
||||
arch.entry_points[i].name ? arch.entry_points[i].name : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "qualified_name",
|
||||
arch.entry_points[i].qualified_name);
|
||||
yyjson_mut_obj_add_str(doc, item, "file", arch.entry_points[i].file);
|
||||
arch.entry_points[i].qualified_name
|
||||
? arch.entry_points[i].qualified_name
|
||||
: "");
|
||||
yyjson_mut_obj_add_str(doc, item, "file",
|
||||
arch.entry_points[i].file ? arch.entry_points[i].file : "");
|
||||
yyjson_mut_arr_add_val(eps, item);
|
||||
}
|
||||
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) {
|
||||
yyjson_mut_val *routes = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.route_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "method", arch.routes[i].method);
|
||||
yyjson_mut_obj_add_str(doc, item, "path", arch.routes[i].path);
|
||||
yyjson_mut_obj_add_str(doc, item, "handler", arch.routes[i].handler);
|
||||
yyjson_mut_obj_add_str(doc, item, "method",
|
||||
arch.routes[i].method ? arch.routes[i].method : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "path",
|
||||
arch.routes[i].path ? arch.routes[i].path : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "handler",
|
||||
arch.routes[i].handler ? arch.routes[i].handler : "");
|
||||
yyjson_mut_arr_add_val(routes, item);
|
||||
}
|
||||
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) {
|
||||
yyjson_mut_val *hotspots = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.hotspot_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "name", arch.hotspots[i].name);
|
||||
yyjson_mut_obj_add_str(doc, item, "name",
|
||||
arch.hotspots[i].name ? arch.hotspots[i].name : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "qualified_name",
|
||||
arch.hotspots[i].qualified_name);
|
||||
arch.hotspots[i].qualified_name
|
||||
? arch.hotspots[i].qualified_name
|
||||
: "");
|
||||
yyjson_mut_obj_add_int(doc, item, "fan_in", arch.hotspots[i].fan_in);
|
||||
yyjson_mut_arr_add_val(hotspots, item);
|
||||
}
|
||||
@@ -1927,34 +1939,88 @@ static char *handle_get_architecture(cbm_mcp_server_t *srv, const char *args) {
|
||||
yyjson_mut_val *boundaries = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.boundary_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "from", arch.boundaries[i].from);
|
||||
yyjson_mut_obj_add_str(doc, item, "to", arch.boundaries[i].to);
|
||||
yyjson_mut_obj_add_str(doc, item, "from",
|
||||
arch.boundaries[i].from ? arch.boundaries[i].from : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "to",
|
||||
arch.boundaries[i].to ? arch.boundaries[i].to : "");
|
||||
yyjson_mut_obj_add_int(doc, item, "call_count", arch.boundaries[i].call_count);
|
||||
yyjson_mut_arr_add_val(boundaries, item);
|
||||
}
|
||||
yyjson_mut_obj_add_val(doc, root, "boundaries", boundaries);
|
||||
}
|
||||
|
||||
/* Cross-service links (HTTP/async between services) */
|
||||
if (arch.service_count > 0) {
|
||||
yyjson_mut_val *services = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.service_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "from",
|
||||
arch.services[i].from ? arch.services[i].from : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "to",
|
||||
arch.services[i].to ? arch.services[i].to : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "type",
|
||||
arch.services[i].type ? arch.services[i].type : "");
|
||||
yyjson_mut_obj_add_int(doc, item, "count", arch.services[i].count);
|
||||
yyjson_mut_arr_add_val(services, item);
|
||||
}
|
||||
yyjson_mut_obj_add_val(doc, root, "services", services);
|
||||
}
|
||||
|
||||
/* Package layers */
|
||||
if (arch.layer_count > 0) {
|
||||
yyjson_mut_val *layers = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.layer_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "name", arch.layers[i].name);
|
||||
yyjson_mut_obj_add_str(doc, item, "layer", arch.layers[i].layer);
|
||||
yyjson_mut_obj_add_str(doc, item, "reason", arch.layers[i].reason);
|
||||
yyjson_mut_obj_add_str(doc, item, "name",
|
||||
arch.layers[i].name ? arch.layers[i].name : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "layer",
|
||||
arch.layers[i].layer ? arch.layers[i].layer : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "reason",
|
||||
arch.layers[i].reason ? arch.layers[i].reason : "");
|
||||
yyjson_mut_arr_add_val(layers, item);
|
||||
}
|
||||
yyjson_mut_obj_add_val(doc, root, "layers", layers);
|
||||
}
|
||||
|
||||
/* Clusters (community detection) */
|
||||
if (arch.cluster_count > 0) {
|
||||
yyjson_mut_val *clusters = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.cluster_count; i++) {
|
||||
const cbm_cluster_info_t *c = &arch.clusters[i];
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_int(doc, item, "id", c->id);
|
||||
yyjson_mut_obj_add_str(doc, item, "label", c->label ? c->label : "");
|
||||
yyjson_mut_obj_add_int(doc, item, "members", c->members);
|
||||
yyjson_mut_obj_add_real(doc, item, "cohesion", c->cohesion);
|
||||
yyjson_mut_val *top = yyjson_mut_arr(doc);
|
||||
for (int j = 0; j < c->top_node_count; j++) {
|
||||
yyjson_mut_arr_add_str(doc, top, c->top_nodes[j] ? c->top_nodes[j] : "");
|
||||
}
|
||||
yyjson_mut_obj_add_val(doc, item, "top_nodes", top);
|
||||
yyjson_mut_val *pkgs = yyjson_mut_arr(doc);
|
||||
for (int j = 0; j < c->package_count; j++) {
|
||||
yyjson_mut_arr_add_str(doc, pkgs, c->packages[j] ? c->packages[j] : "");
|
||||
}
|
||||
yyjson_mut_obj_add_val(doc, item, "packages", pkgs);
|
||||
yyjson_mut_val *etypes = yyjson_mut_arr(doc);
|
||||
for (int j = 0; j < c->edge_type_count; j++) {
|
||||
yyjson_mut_arr_add_str(doc, etypes, c->edge_types[j] ? c->edge_types[j] : "");
|
||||
}
|
||||
yyjson_mut_obj_add_val(doc, item, "edge_types", etypes);
|
||||
yyjson_mut_arr_add_val(clusters, item);
|
||||
}
|
||||
yyjson_mut_obj_add_val(doc, root, "clusters", clusters);
|
||||
}
|
||||
|
||||
/* File tree */
|
||||
if (arch.file_tree_count > 0) {
|
||||
yyjson_mut_val *file_tree = yyjson_mut_arr(doc);
|
||||
for (int i = 0; i < arch.file_tree_count; i++) {
|
||||
yyjson_mut_val *item = yyjson_mut_obj(doc);
|
||||
yyjson_mut_obj_add_str(doc, item, "path", arch.file_tree[i].path);
|
||||
yyjson_mut_obj_add_str(doc, item, "type", arch.file_tree[i].type);
|
||||
yyjson_mut_obj_add_str(doc, item, "path",
|
||||
arch.file_tree[i].path ? arch.file_tree[i].path : "");
|
||||
yyjson_mut_obj_add_str(doc, item, "type",
|
||||
arch.file_tree[i].type ? arch.file_tree[i].type : "");
|
||||
yyjson_mut_obj_add_int(doc, item, "children", arch.file_tree[i].children);
|
||||
yyjson_mut_arr_add_val(file_tree, item);
|
||||
}
|
||||
|
||||
@@ -525,6 +525,56 @@ TEST(tool_get_architecture_empty) {
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* Regression for #281: handle_get_architecture must actually call
|
||||
* cbm_store_get_architecture and surface its sections. Before the fix
|
||||
* only label/edge histograms were emitted regardless of which aspects
|
||||
* were requested. The store-side arch_entry_points query reads
|
||||
* properties.is_entry_point on Function nodes, so we tag one node and
|
||||
* assert the resulting JSON surfaces an "entry_points" array containing
|
||||
* the tagged function — which is impossible without the wiring. */
|
||||
TEST(tool_get_architecture_emits_populated_sections) {
|
||||
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
|
||||
ASSERT_NOT_NULL(srv);
|
||||
|
||||
cbm_store_t *st = cbm_mcp_server_store(srv);
|
||||
ASSERT_NOT_NULL(st);
|
||||
|
||||
const char *proj = "arch-test";
|
||||
cbm_mcp_server_set_project(srv, proj);
|
||||
cbm_store_upsert_project(st, proj, "/tmp/arch-test");
|
||||
|
||||
cbm_node_t main_fn = {0};
|
||||
main_fn.project = proj;
|
||||
main_fn.label = "Function";
|
||||
main_fn.name = "main";
|
||||
main_fn.qualified_name = "arch-test.cmd.main";
|
||||
main_fn.file_path = "cmd/main.go";
|
||||
main_fn.start_line = 1;
|
||||
main_fn.end_line = 3;
|
||||
main_fn.properties_json = "{\"is_entry_point\":true}";
|
||||
ASSERT_GT(cbm_store_upsert_node(st, &main_fn), 0);
|
||||
|
||||
char *resp = cbm_mcp_server_handle(
|
||||
srv, "{\"jsonrpc\":\"2.0\",\"id\":91,\"method\":\"tools/call\","
|
||||
"\"params\":{\"name\":\"get_architecture\","
|
||||
"\"arguments\":{\"project\":\"arch-test\",\"aspects\":[\"all\"]}}}");
|
||||
ASSERT_NOT_NULL(resp);
|
||||
char *inner = extract_text_content(resp);
|
||||
ASSERT_NOT_NULL(inner);
|
||||
|
||||
/* The handler always emits node/edge counts and schema histograms;
|
||||
* those existed before #281. The "entry_points" array only appears
|
||||
* when cbm_store_get_architecture is actually called and its result
|
||||
* is serialized — which is exactly what #281 wires up. */
|
||||
ASSERT_NOT_NULL(strstr(inner, "\"entry_points\""));
|
||||
ASSERT_NOT_NULL(strstr(inner, "main"));
|
||||
|
||||
free(inner);
|
||||
free(resp);
|
||||
cbm_mcp_server_free(srv);
|
||||
PASS();
|
||||
}
|
||||
|
||||
TEST(tool_query_graph_missing_query) {
|
||||
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
|
||||
|
||||
@@ -1001,6 +1051,13 @@ static char *extract_text_content(const char *mcp_result) {
|
||||
return strdup(mcp_result); /* fallback */
|
||||
yyjson_val *root = yyjson_doc_get_root(doc);
|
||||
yyjson_val *content = yyjson_obj_get(root, "content");
|
||||
if (!content) {
|
||||
/* Handle JSON-RPC wrapper: {"jsonrpc":...,"result":{"content":[...]}} */
|
||||
yyjson_val *rpc_result = yyjson_obj_get(root, "result");
|
||||
if (rpc_result) {
|
||||
content = yyjson_obj_get(rpc_result, "content");
|
||||
}
|
||||
}
|
||||
if (!content || !yyjson_is_arr(content)) {
|
||||
yyjson_doc_free(doc);
|
||||
return strdup(mcp_result);
|
||||
@@ -1739,6 +1796,7 @@ SUITE(mcp) {
|
||||
RUN_TEST(tool_trace_missing_function_name);
|
||||
RUN_TEST(tool_delete_project_not_found);
|
||||
RUN_TEST(tool_get_architecture_empty);
|
||||
RUN_TEST(tool_get_architecture_emits_populated_sections);
|
||||
RUN_TEST(tool_query_graph_missing_query);
|
||||
|
||||
/* Pipeline-dependent tool handlers */
|
||||
|
||||
Reference in New Issue
Block a user