From a154d18a2e90f01037b009ca72f9939f934fedd4 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 01:16:56 +0200 Subject: [PATCH 01/11] fix(cli): stop breaking commands people already type (#1544, #1550) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regressions we shipped ourselves, both with the same shape: a change that was correct in the abstract broke a concrete thing users had working. **`update --ui` became a hard error (#1544).** v0.10.2 removed the ui/standard chooser and deleted the flags that drove it, so a command in scripts, aliases and muscle memory started failing with "unknown update option" where v0.10.1 had printed the install-script handoff. Retiring a CHOICE is fine; retiring the WORDS is a break users get nothing for. Both flags are accepted again, do nothing, and print one note explaining that every build now includes the UI. A genuinely unknown flag is still rejected — the point is compatibility, not silence. Reported by @ehendrix23. **The generated Pi extension could not load (#1550).** Since #1392 the extension has been generated from the tool registry, emitting `export function register(pi)`. Pi loads an extension by calling its DEFAULT export as a factory, so the file failed to load — and a Pi extension that fails to load takes every `pi` command with it, `pi doctor` included, not just cbm's tools. It now default-exports the factory. Reported by @musichen. The existing generator test passed throughout: it asserted WHICH tools the file lists, never that the file Pi loads is loadable. The new test pins the export shape, which is the property that actually matters to the consumer. Signed-off-by: Martin Vogel --- src/cli/cli.c | 14 ++++++++++++++ src/cli/client_adapter.c | 11 +++++++++-- tests/test_agent_clients.c | 19 +++++++++++++++++++ tests/test_cli.c | 19 +++++++++++++++++++ 4 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index 010719a64..f85a55f25 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -11482,17 +11482,31 @@ int cbm_cmd_update(int argc, char **argv) { bool dry_run = false; bool force = false; + bool legacy_variant_flag = false; for (int i = 0; i < argc; i++) { if (strcmp(argv[i], "--dry-run") == 0) { dry_run = true; } else if (strcmp(argv[i], "--force") == 0) { force = true; + } else if (strcmp(argv[i], "--ui") == 0 || strcmp(argv[i], "--standard") == 0) { + /* v0.10.2 deleted the ui/standard chooser and, with it, these flags — + * which turned `update --ui` from a working command into a hard + * error for everyone who had it in a script, an alias, or muscle + * memory (#1544). Removing a CHOICE is fine; removing the WORDS + * people type is a break we owe them nothing for. Accept both, + * ignore them, and say once why they no longer mean anything. */ + legacy_variant_flag = true; } else if (strcmp(argv[i], "-y") != 0 && strcmp(argv[i], "--yes") != 0 && strcmp(argv[i], "-n") != 0 && strcmp(argv[i], "--no") != 0) { (void)fprintf(stderr, "error: unknown update option: %s\n", argv[i]); return CLI_TRUE; } } + if (legacy_variant_flag) { + (void)fprintf(stderr, "note: --ui/--standard are accepted but no longer do anything; since " + "v0.10.0 there is one build per platform and it always includes the " + "graph UI.\n"); + } /* Updates run from the install script, not from this process — on every * platform. diff --git a/src/cli/client_adapter.c b/src/cli/client_adapter.c index c6828ea12..be00cdb33 100644 --- a/src/cli/client_adapter.c +++ b/src/cli/client_adapter.c @@ -155,8 +155,15 @@ char *cbm_client_adapter_pi(const char *binary_path) { "}\n\n"); /* Register every tool the registry advertises — never a hand-picked subset, - * which is the drift this generator exists to prevent. */ - sb_append(&sb, "export function register(pi) {\n"); + * which is the drift this generator exists to prevent. + * + * DEFAULT export, and it must stay that way: Pi loads an extension by + * calling its default export as a factory. A named `register` export made + * the file fail to load, and a Pi extension that fails to load takes EVERY + * pi command down with it — `pi doctor` included — not just cbm's tools + * (#1550). The named binding is kept alongside for anything that imported + * it by name. */ + sb_append(&sb, "export default function (pi) {\n"); for (int i = 0; i < count; i++) { const char *name = cbm_mcp_tool_name(i); if (!name || !name[0]) { diff --git a/tests/test_agent_clients.c b/tests/test_agent_clients.c index 3c164b5cc..edfc3a803 100644 --- a/tests/test_agent_clients.c +++ b/tests/test_agent_clients.c @@ -1096,6 +1096,24 @@ TEST(client_adapter_pi_registers_every_registry_tool) { PASS(); } +/* #1550: Pi loads an extension by calling its DEFAULT export as a factory. The + * generator emitted a named `register` export instead, so the file failed to + * load — and a Pi extension that fails to load takes every pi command with it + * (`pi doctor` included), not just cbm's tools. The blast radius is why this is + * pinned rather than left to the registry-drift test above, which passed the + * whole time this was broken: it checked WHICH tools were listed, never whether + * the file Pi loads is loadable. */ +TEST(client_adapter_pi_default_exports_its_factory_issue1550) { + char *js = cbm_client_adapter_pi("/usr/local/bin/codebase-memory-mcp"); + ASSERT_NOT_NULL(js); + ASSERT_NOT_NULL(strstr(js, "export default function")); + /* The old shape must be gone: a bare `export function register(pi)` is the + * exact form Pi rejects. */ + ASSERT_NULL(strstr(js, "export function register(pi)")); + free(js); + PASS(); +} + /* #616 rejected `"` in its path template but not `\`, so a Windows home like * C:\Users\urs\bin produced `\u` — an invalid unicode escape — and the whole * auto-loaded plugin failed to parse. A broken plugin in an auto-load directory @@ -1185,6 +1203,7 @@ SUITE(agent_clients) { RUN_TEST(agent_clients_continue_uses_owned_yaml_sequence_item); RUN_TEST(agent_clients_continue_refuses_foreign_same_name_and_nonsequence_section); RUN_TEST(client_adapter_pi_registers_every_registry_tool); + RUN_TEST(client_adapter_pi_default_exports_its_factory_issue1550); RUN_TEST(client_adapter_escapes_windows_paths_and_quotes); RUN_TEST(client_adapter_opencode_sends_the_required_hook_event); RUN_TEST(client_adapter_rejects_missing_binary_path); diff --git a/tests/test_cli.c b/tests/test_cli.c index 9baf9eae5..a6628f667 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -12061,6 +12061,24 @@ TEST(cli_sha256_file_matches_known_vector) { PASS(); } +/* #1544: v0.10.2 deleted the ui/standard chooser AND the flags that drove it, + * so `update --ui` — a command people had in scripts and aliases — started + * failing with "unknown update option". Retiring a choice is fine; breaking the + * words people already type is not. Both flags must be accepted, do nothing, + * and say so once. A genuinely unknown flag must still be rejected, or this + * degenerates into ignoring typos. */ +TEST(cli_update_accepts_retired_variant_flags_issue1544) { + char *ui_argv[] = {"--dry-run", "--ui", "--yes"}; + char *standard_argv[] = {"--dry-run", "--standard", "--yes"}; + char *bogus_argv[] = {"--dry-run", "--not-a-flag", "--yes"}; + + ASSERT_EQ(cbm_cmd_update(3, ui_argv), 0); + ASSERT_EQ(cbm_cmd_update(3, standard_argv), 0); + /* Unknown flags stay an error — the point is compatibility, not silence. */ + ASSERT_TRUE(cbm_cmd_update(3, bogus_argv) != 0); + PASS(); +} + #ifdef _WIN32 /* The Windows update contract, asserted with the activation seam OFF so this * takes the exact dispatch a release binary ships: `update` never replaces the @@ -12136,6 +12154,7 @@ SUITE(cli) { RUN_TEST(cli_install_config_failure_keeps_published_binary); RUN_TEST(cli_update_download_failure_does_not_quiesce_sessions); RUN_TEST(cli_update_already_current_does_not_quiesce_sessions); + RUN_TEST(cli_update_accepts_retired_variant_flags_issue1544); RUN_TEST(cli_update_agent_configs_finish_before_guard_release); RUN_TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index); RUN_TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain); From f3d78b1cf7a6ae2686257ac06a01d0cbbb115a12 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 01:29:56 +0200 Subject: [PATCH 02/11] test(store): bind the quarantine verdict that #1545 introduced (#1206, #1037) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1545 fixed the defect that let concurrent instances quarantine each other's healthy databases, but shipped without tests — and this is a data-loss path, so it gets coverage before it ships. Three cases, one per verdict, chosen so a regression in either direction fails: healthy -> OK (the baseline the other two are measured against) bad root_path-> CORRUPT (a verdict that never says CORRUPT would protect broken databases instead of users) no handle -> TRANSIENT (a file we could not open tells us NOTHING about its contents; answering CORRUPT there is how a database nobody had read got renamed and rebuilt) Co-Authored-By: LynxBay Signed-off-by: Martin Vogel --- tests/test_store_nodes.c | 48 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/tests/test_store_nodes.c b/tests/test_store_nodes.c index 64945d1be..d063ea748 100644 --- a/tests/test_store_nodes.c +++ b/tests/test_store_nodes.c @@ -544,8 +544,7 @@ TEST(store_lsp_surface_round_trip) { ASSERT_STR_EQ(rows[0].defs_json, "[{\"qn\":\"pkg.A\",\"sn\":\"A\",\"label\":\"Function\"}]"); ASSERT_STR_EQ(rows[0].config_ctx, "cfg-1"); ASSERT_EQ(rows[0].ref_bloom_len, (int)sizeof(bloom)); - ASSERT_TRUE(rows[0].ref_bloom != NULL && - memcmp(rows[0].ref_bloom, bloom, sizeof(bloom)) == 0); + ASSERT_TRUE(rows[0].ref_bloom != NULL && memcmp(rows[0].ref_bloom, bloom, sizeof(bloom)) == 0); ASSERT_STR_EQ(rows[1].rel_path, "pkg/b.go"); ASSERT_STR_EQ(rows[1].defs_json, "[]"); ASSERT_EQ(rows[1].ref_bloom_len, 0); @@ -1265,6 +1264,48 @@ TEST(store_integrity_corrupt_too_many_rows) { PASS(); } +/* ── Quarantine verdict (#1206, #1037) ────────────────────────────── + * + * The bool check above cannot answer the only question the quarantine path + * actually asks: "is this database damaged, or did I just lose a lock race?" + * Answering "damaged" to the second question is what made concurrent instances + * rename each other's HEALTHY databases to .corrupt (#1206). These bind the + * three-way verdict so that behaviour cannot come back. */ + +TEST(store_integrity_verdict_healthy_is_ok) { + cbm_store_t *s = cbm_store_open_memory(); + ASSERT_NOT_NULL(s); + cbm_store_upsert_project(s, "healthy-proj", "/tmp/healthy"); + ASSERT_EQ(cbm_store_check_integrity_verdict(s), CBM_INTEGRITY_OK); + cbm_store_close(s); + PASS(); +} + +TEST(store_integrity_verdict_real_corruption_is_corrupt) { + /* Structural damage the shallow check can see: node IDs landing in + * root_path. This one MUST be quarantinable — a verdict that never says + * CORRUPT would protect broken databases instead of users. */ + cbm_store_t *s = cbm_store_open_memory(); + ASSERT_NOT_NULL(s); + sqlite3 *db = cbm_store_get_db(s); + sqlite3_exec(db, + "INSERT INTO projects (name, indexed_at, root_path) " + "VALUES ('broken', '2024-01-01', '826');", + NULL, NULL, NULL); + ASSERT_EQ(cbm_store_check_integrity_verdict(s), CBM_INTEGRITY_CORRUPT); + cbm_store_close(s); + PASS(); +} + +TEST(store_integrity_verdict_unopenable_is_transient_not_corrupt) { + /* A handle we could not open tells us NOTHING about the file's contents. + * Reporting CORRUPT here is how a database nobody could read got renamed + * and rebuilt from scratch (#1206) — the destructive answer to a question + * that was never asked. */ + ASSERT_EQ(cbm_store_check_integrity_verdict(NULL), CBM_INTEGRITY_TRANSIENT); + PASS(); +} + TEST(store_integrity_null_check) { /* NULL store should return false (not crash) */ ASSERT_FALSE(cbm_store_check_integrity(NULL)); @@ -2144,6 +2185,9 @@ SUITE(store_nodes) { RUN_TEST(store_integrity_corrupt_bad_path); RUN_TEST(store_integrity_windows_lowercase_drive_issue367); RUN_TEST(store_integrity_corrupt_too_many_rows); + RUN_TEST(store_integrity_verdict_healthy_is_ok); + RUN_TEST(store_integrity_verdict_real_corruption_is_corrupt); + RUN_TEST(store_integrity_verdict_unopenable_is_transient_not_corrupt); RUN_TEST(store_integrity_null_check); RUN_TEST(store_project_crud); RUN_TEST(store_project_update); From 4b886904d70b652ba6804ccb84590f597ac00d14 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 02:11:07 +0200 Subject: [PATCH 03/11] fix(mcp): give format:"json" the evidence columns it advertises (#1542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `trace_path(include_evidence:true)` added `strategy` and `confidence` per hop in the default tree output, and returned `cols: ["name","hop"]` under format:"json" — no error, no warning, just two fields missing from the response shape the schema and `--help` both promise. include_evidence was only ever implemented in bfs_to_tree_table; bfs_to_tree_json never took the flag. So the callers most likely to want the evidence — the ones asking for structured output to consume programmatically — were exactly the ones who silently got none of it. Reported by @asemraza. The json path now emits both columns with the same semantics as the tree path, including the placeholder case: the root hop has no inbound edge and non-CALLS edges record no strategy, so those rows carry nulls rather than dropping fields and leaving callers with ragged rows. Tree says "-", json says null — the same promise in the form each consumer can test. The existing evidence test covered only the tree path, which is why this went unnoticed; it now asserts both formats. Signed-off-by: Martin Vogel --- src/mcp/mcp.c | 42 +++++++++++++++++++++++++++++++++++------- tests/test_mcp.c | 20 ++++++++++++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 2bbbec52d..7cbee0bf9 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -6203,7 +6203,8 @@ static int trace_watermark_index(const cbm_traverse_result_t *tr, int hop, int64 * output — {cols, groups:[{qn_prefix, rows:[[name,hop,...]]}]}. Optional * risk/args columns mirror the flags. */ static yyjson_mut_val *bfs_to_tree_json(yyjson_mut_doc *doc, cbm_traverse_result_t *tr, - bool risk_labels, bool include_tests, bool data_flow) { + bool risk_labels, bool include_tests, bool data_flow, + bool include_evidence) { yyjson_mut_val *leg = yyjson_mut_obj(doc); yyjson_mut_val *cols = yyjson_mut_arr(doc); yyjson_mut_arr_add_str(doc, cols, "name"); @@ -6211,6 +6212,14 @@ static yyjson_mut_val *bfs_to_tree_json(yyjson_mut_doc *doc, cbm_traverse_result if (risk_labels) { yyjson_mut_arr_add_str(doc, cols, "risk"); } + /* #1542: include_evidence was implemented on the tree path only, so + * format:"json" silently returned cols ["name","hop"] while the schema and + * --help promised two more. A structured caller — the one most likely to + * ask for json — got no error, just missing fields. */ + if (include_evidence) { + yyjson_mut_arr_add_str(doc, cols, "strategy"); + yyjson_mut_arr_add_str(doc, cols, "confidence"); + } if (data_flow) { yyjson_mut_arr_add_str(doc, cols, "args"); } @@ -6258,6 +6267,25 @@ static yyjson_mut_val *bfs_to_tree_json(yyjson_mut_doc *doc, cbm_traverse_result yyjson_mut_arr_add_str(doc, row, ""); } } + if (include_evidence) { + const char *ev_class = NULL; + double ev_conf = -1.0; + if (bfs_edge_evidence_for_hop(tr, tr->visited[i].node.id, &ev_class, &ev_conf)) { + yyjson_mut_arr_add_strcpy(doc, row, ev_class ? ev_class : ""); + if (ev_conf >= 0.0) { + yyjson_mut_arr_add_real(doc, row, ev_conf); + } else { + yyjson_mut_arr_add_null(doc, row); + } + } else { + /* The root hop has no inbound edge and non-CALLS edges record + * no strategy. The tree path emits "-" placeholders to keep the + * column count fixed; json says null, which is the same promise + * in a form a structured caller can test. */ + yyjson_mut_arr_add_null(doc, row); + yyjson_mut_arr_add_null(doc, row); + } + } yyjson_mut_arr_add_val(cur_rows, row); } yyjson_mut_obj_add_val(doc, leg, "groups", groups); @@ -6710,15 +6738,15 @@ static char *handle_trace_call_path(cbm_mcp_server_t *srv, const char *args) { } if (do_outbound) { 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)); + yyjson_mut_obj_add_val(doc, root, "callees", + bfs_to_tree_json(doc, &view_out, risk_labels, include_tests, + data_flow, include_evidence)); } if (do_inbound) { 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)); + yyjson_mut_obj_add_val(doc, root, "callers", + bfs_to_tree_json(doc, &view_in, risk_labels, include_tests, + data_flow, include_evidence)); } if (more_rows) { yyjson_mut_obj_add_bool(doc, root, "truncated", true); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index 854330053..b83a8031c 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -3161,6 +3161,26 @@ TEST(tool_trace_path_evidence_is_opt_in_and_class_mapped) { ASSERT_NULL(strstr(ev_txt, "lsp_trait_dispatch")); free(ev_txt); free(ev); + + /* #1542: the same request with format:"json" returned cols ["name","hop"] + * — include_evidence was implemented on the tree path only, so the callers + * most likely to ask for structured output were the ones who silently got + * nothing. The two formats must promise the same fields. */ + char *ev_json = cbm_mcp_server_handle( + srv, "{\"jsonrpc\":\"2.0\",\"id\":93,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"trace_path\",\"arguments\":{\"function_name\":\"caller\"," + "\"project\":\"ev-proj\",\"direction\":\"outbound\",\"include_evidence\":true," + "\"format\":\"json\"}}}"); + ASSERT_NOT_NULL(ev_json); + char *ev_json_txt = extract_text_content(ev_json); + ASSERT_NOT_NULL(ev_json_txt); + ASSERT_NOT_NULL(strstr(ev_json_txt, "\"strategy\"")); + ASSERT_NOT_NULL(strstr(ev_json_txt, "\"confidence\"")); + ASSERT_NOT_NULL(strstr(ev_json_txt, "lsp")); + ASSERT_NOT_NULL(strstr(ev_json_txt, "0.95")); + ASSERT_NULL(strstr(ev_json_txt, "lsp_trait_dispatch")); + free(ev_json_txt); + free(ev_json); cbm_mcp_server_free(srv); PASS(); } From 98aff5b7dc016183f92bb2aac827235e1621646d Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 03:33:54 +0200 Subject: [PATCH 04/11] fix(subprocess): retry a spawn the kernel refused for load, not for cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `posix_spawnp` and `fork` both fail with EAGAIN when the process table or a per-user limit is momentarily full. Both paths treated that as a hard failure, so a burst of process creation turned into CBM_PROC_SPAWN_FAILED — a git probe, an LSP server, or an index worker refusing to run for a reason the user can neither see nor act on, on a machine that is merely busy. This is the production half of a flake we have been re-running for two days: `subprocess_run_clean` failed spawning `/bin/sh -c "exit 0"` on macOS runners in three separate CI runs, always under a loaded release matrix, always green on a quiet rerun. Chasing it as a test race would have been chasing the symptom — the test is right, and a user on a loaded laptop hits the same edge. EAGAIN/ENOMEM now retry three times over ~30ms; everything else keeps failing immediately, and an exhausted system still fails after the backoff rather than spinning. exec-class errors (ENOENT/EACCES/ENOEXEC/...) are untouched, so the documented "missing binary exits 127, spawn_failed means the mechanism failed" contract still holds. The fork fallback gets the same treatment: it fails for the same reason under the same pressure, and the fallback must not be less robust than the primary path. Signed-off-by: Martin Vogel --- src/foundation/subprocess.c | 40 +++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/src/foundation/subprocess.c b/src/foundation/subprocess.c index dfcc12f3f..6e173816f 100644 --- a/src/foundation/subprocess.c +++ b/src/foundation/subprocess.c @@ -882,6 +882,26 @@ static cbm_proc_poll_t cbm_subprocess_poll_win(cbm_subprocess_t *process, cbm_pr #else /* POSIX */ +/* Transient spawn-failure retry (see the EAGAIN note in cbm_posix_spawn_apple). + * Three attempts over ~30ms total: long enough to ride out a burst of process + * creation, short enough that a genuinely exhausted system still fails fast. */ +enum { CBM_SPAWN_RETRY = 2, CBM_SPAWN_RETRY_ATTEMPTS = 3 }; +static const struct timespec CBM_SPAWN_RETRY_DELAY = {0, 10L * 1000L * 1000L}; +#define CBM_SPAWN_RETRY_DELAY_NS (&CBM_SPAWN_RETRY_DELAY) + +/* fork() fails with EAGAIN under the same pressure posix_spawn does, and the + * fallback path must not be less robust than the primary one. */ +static pid_t cbm_fork_with_retry(void) { + for (int attempt = 0; attempt < CBM_SPAWN_RETRY_ATTEMPTS; attempt++) { + pid_t pid = fork(); + if (pid >= 0 || (errno != EAGAIN && errno != ENOMEM)) { + return pid; + } + (void)cbm_nanosleep(CBM_SPAWN_RETRY_DELAY_NS, NULL); + } + return fork(); +} + /* Used by the fork+exec child. posix_spawn performs the same reset * declaratively via SETSIGDEF + SETSIGMASK, but Apple still forks for the * exec-failure fallback below, so this stays compiled everywhere. */ @@ -1010,6 +1030,14 @@ static int cbm_posix_spawn_apple(cbm_subprocess_t *process, int input, int outpu rc == ELOOP || rc == ENAMETOOLONG || rc == ENOTDIR)) { return 1; } + /* EAGAIN/ENOMEM are the kernel saying "not right now", not "never": the + * process table or a per-user limit is momentarily full. Reporting + * spawn_failed for that turns transient load into a user-visible error — + * a git or LSP probe failing on a busy laptop for no reason the user can + * see or act on. Retry briefly. Everything else stays a hard failure. */ + if (configured && (rc == EAGAIN || rc == ENOMEM)) { + return CBM_SPAWN_RETRY; + } return -1; } #endif @@ -1059,13 +1087,21 @@ static int cbm_subprocess_spawn_posix(cbm_subprocess_t *process) { pid_t pid = -1; #ifdef __APPLE__ int spawn_rc = cbm_posix_spawn_apple(process, input, output, &pid); + for (int attempt = 0; spawn_rc == CBM_SPAWN_RETRY && attempt < CBM_SPAWN_RETRY_ATTEMPTS; + attempt++) { + (void)cbm_nanosleep(CBM_SPAWN_RETRY_DELAY_NS, NULL); + spawn_rc = cbm_posix_spawn_apple(process, input, output, &pid); + } + if (spawn_rc == CBM_SPAWN_RETRY) { + spawn_rc = -1; /* still exhausted after backoff: a real failure */ + } if (spawn_rc < 0) { (void)close(input); (void)close(output); return -1; } if (spawn_rc > 0) { /* exec-class failure: reproduce the fork+exec 127 */ - pid = fork(); + pid = cbm_fork_with_retry(); if (pid < 0) { (void)close(input); (void)close(output); @@ -1076,7 +1112,7 @@ static int cbm_subprocess_spawn_posix(cbm_subprocess_t *process) { } } #else - pid = fork(); + pid = cbm_fork_with_retry(); if (pid < 0) { (void)close(input); (void)close(output); From ff853ac14b156acf3f85a9d0fe8732209f5419fb Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 09:46:37 +0200 Subject: [PATCH 05/11] fix(cli): stop blaming sessions for a failure that has none (#1537) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reporter ran `uninstall`, then could not install again: every attempt said "active CBM sessions and operations could not be stopped safely". A reboot proved no such session existed, and the remedy line v0.10.2 added made it worse — it told them to run `codebase-memory-mcp daemon status`, a binary they had just uninstalled. Both halves were wrong for their case: The guard funneled two unrelated outcomes into one message. A cohort that is BUSY means real sessions hold it and closing something is the fix. A reservation that FAILED — lock I/O, leftover coordination state, permissions — means nothing is running at all, and telling that reader to close sessions sends them after processes that do not exist. Same misattribution class as #1416 and #1535: a non-session failure wearing a session costume. They are now separate messages, and the failure one says plainly that nothing needs to be closed. The remedy also has to survive the situation it prints in. Install, and any retry after uninstall, runs with no cbm on PATH; advice that starts with `codebase-memory-mcp ...` is unusable exactly when it is needed. The busy message now leads with the actionable step (close the editor/agent holding it) and mentions the command only as an aid where a binary still exists; the failure message asks for a cache listing, which needs no cbm at all. A recorded transaction refusal still outranks both generic messages, so the #1416 attribution is unchanged. This does NOT yet explain why their reservation fails with nothing running — that needs their environment, which is being requested on the issue. What it does is stop sending them after the wrong thing while we find out. Signed-off-by: Martin Vogel --- src/cli/cli.c | 45 ++++++++++++++++++++++++++++++++++----------- tests/test_cli.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 11 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index f85a55f25..ea86a3024 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -152,17 +152,31 @@ int cbm_cli_exit_status_after_maintenance(int exit_status, bool maintenance_canc return maintenance_cancelled && exit_status == EXIT_SUCCESS ? EXIT_FAILURE : exit_status; } -/* #1537: the refusal is correct, but "sessions could not be stopped" gives the - * reader nothing to act on — which sessions? The daemon already knows every - * client pid and `daemon status` prints them, so name that remedy here rather - * than leaving people to guess (one reporter uninstalled first and still hit - * this, with no way to see what was holding on). */ -static const char CLI_ACTIVATION_REFUSED_MESSAGE[] = +/* #1537. Two very different failures reached this one message: the cohort was + * BUSY (real sessions are running — the reader can close them), or the + * reservation failed outright (lock I/O, stale coordination state, permissions + * — nothing the reader can close, because nothing is running). Reporting both + * as "active sessions" sent someone hunting processes that a reboot proved did + * not exist. + * + * The remedy line must also survive the situation it prints in: an install or + * a retry after `uninstall` has no cbm on PATH, so "run codebase-memory-mcp + * daemon status" was advice the reader could not follow at exactly the moment + * they needed it. Each message now names its own condition and stays runnable. */ +static const char CLI_ACTIVATION_BUSY_MESSAGE[] = "error: active CBM sessions and operations could not be stopped safely; " "no activation was committed.\n" - "error: run 'codebase-memory-mcp daemon status' to list the client " - "processes still holding the daemon (MCP servers started by your editor or " - "agent are the usual holders), close them, then retry."; + "error: something is still using CBM. If an editor or agent is running an " + "MCP server, close it and retry; 'codebase-memory-mcp daemon status' lists " + "the holders when a cbm binary is still installed."; +static const char CLI_ACTIVATION_REFUSED_MESSAGE[] = + "error: activation could not reserve exclusive access; no activation was " + "committed.\n" + "error: this is NOT a running-session problem — the reservation itself " + "failed (coordination lock, leftover state, or permissions). Nothing needs " + "to be closed. Check the errors above, and report this with the output of " + "'ls -la \"${CBM_CACHE_DIR:-$HOME/.cache/codebase-memory-mcp}\"' if it " + "persists."; static const char CLI_ACTIVATION_PARTIAL_MESSAGE[] = "error: activation stopped after one or more agent configuration or " "cleanup operations failed; the published/current executable was kept, " @@ -211,7 +225,11 @@ static void cli_activation_diagnostic(const cbm_cli_activation_ops_t *ops, const * stop/reservation failures, which record no refusal note. */ char attributed[CBM_SZ_1K]; const char *note = cbm_activation_transaction_refusal_note(); - if (diagnostic == CLI_ACTIVATION_REFUSED_MESSAGE && note && note[0]) { + /* A recorded refusal is more specific than EITHER generic message, so it + * wins over both the busy and the reservation-failure wording. */ + if ((diagnostic == CLI_ACTIVATION_REFUSED_MESSAGE || + diagnostic == CLI_ACTIVATION_BUSY_MESSAGE) && + note && note[0]) { (void)snprintf(attributed, sizeof(attributed), "error: activation was refused by a filesystem safety check before any " "change was made: %s\n" @@ -245,7 +263,12 @@ int cbm_cli_activation_guard_with_ops(const cbm_cli_activation_ops_t *ops, if (mutation_lease) { ops->mutation_lease_release(ops->context, mutation_lease); } - cli_activation_diagnostic(ops, CLI_ACTIVATION_REFUSED_MESSAGE); + /* 0 means the cohort was BUSY: real sessions hold it and closing them + * is the remedy. Anything else is the reservation itself failing, and + * telling that reader to close sessions sends them after processes + * that do not exist (#1537). */ + cli_activation_diagnostic(ops, reserve_status == 0 ? CLI_ACTIVATION_BUSY_MESSAGE + : CLI_ACTIVATION_REFUSED_MESSAGE); return CLI_TRUE; } diff --git a/tests/test_cli.c b/tests/test_cli.c index a6628f667..57e37ba69 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -682,6 +682,40 @@ TEST(cli_activation_refusal_note_reaches_diagnostic_issue1416) { PASS(); } +/* #1537: a reservation that FAILED (lock I/O, leftover coordination state, + * permissions) is not a reservation that was BUSY. Reporting both as "active + * CBM sessions could not be stopped" sent a reporter hunting processes that a + * reboot proved did not exist — and the remedy we printed told them to run a + * cbm binary they had just uninstalled. Each condition now names itself. */ +TEST(cli_activation_distinguishes_busy_from_reservation_failure_issue1537) { + cbm_activation_transaction_note_refusal_for_testing(NULL, 0UL); + + /* BUSY (0): real sessions hold the cohort — closing something is the fix. */ + cli_activation_fake_t busy = { + .participants_active = true, + .mutation_reserve_result = 0, + }; + cbm_cli_activation_ops_t ops = { + .context = &busy, + .reserve_for_mutation = cli_activation_fake_reserve_mutation, + .mutation_lease_release = cli_activation_fake_release_mutation, + .visible_diagnostic = cli_activation_fake_diagnostic, + }; + ASSERT_EQ(cbm_cli_activation_guard_with_ops(&ops, cli_activation_fake_mutation, &busy), 1); + ASSERT_NOT_NULL(strstr(busy.diagnostic, "could not be stopped safely")); + + /* FAILURE (-1): nothing is running, so the reader must not be told to close + * sessions — and must not be handed a command that may not be installed. */ + cli_activation_fake_t failed = { + .mutation_reserve_result = -1, + }; + ops.context = &failed; + ASSERT_EQ(cbm_cli_activation_guard_with_ops(&ops, cli_activation_fake_mutation, &failed), 1); + ASSERT_NOT_NULL(strstr(failed.diagnostic, "NOT a running-session problem")); + ASSERT_NULL(strstr(failed.diagnostic, "could not be stopped safely")); + PASS(); +} + TEST(cli_activation_refuses_unsafe_cohort_reservation) { cli_activation_fake_t fake = { .mutation_reserve_result = -1, @@ -12138,6 +12172,7 @@ SUITE(cli) { RUN_TEST(cli_activation_quiesces_active_cohort_before_mutation); RUN_TEST(cli_activation_refuses_when_cohort_does_not_drain); RUN_TEST(cli_activation_refusal_note_reaches_diagnostic_issue1416); + RUN_TEST(cli_activation_distinguishes_busy_from_reservation_failure_issue1537); RUN_TEST(cli_activation_refuses_unsafe_cohort_reservation); RUN_TEST(cli_activation_releases_maintenance_lease_after_success); RUN_TEST(cli_activation_releases_maintenance_lease_when_mutation_fails); From 8d487b7da02ef0cbc5c610dc82307b5d7f8289f7 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 10:59:54 +0200 Subject: [PATCH 06/11] test(daemon): scale the frontend announce backstop with the build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test-msan` failed the two backpressure cases while every other lane was green, and the same suite passes under MSan locally (12/12). The wait that fired is a LIVENESS BACKSTOP, not a race budget: the daemon child either announces on its ready pipe or it does not, and the test asserts the announcement itself, never a timing window. A backstop only does its job if it never fires on a legitimately slow start. 30s was already the second value here — the first was below the daemon's own 12s cohort-claim budget and SIGKILLed healthy startups on oversubscribed runners. Under MemorySanitizer, with instrumented libc++ and origin tracking, startup runs several times slower than that same budget it has to clear, so 30s stopped being a backstop and started being a verdict. Sanitized builds now get 180s, matching the existing sanitizer-aware pattern in test_cs_lsp_bench.c. This costs nothing when the daemon is healthy: a passing run returns the moment the byte arrives, whatever the ceiling. Nothing about the asserted state changed, and the non-sanitized bound is untouched. Signed-off-by: Martin Vogel --- tests/test_daemon_frontend.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/test_daemon_frontend.c b/tests/test_daemon_frontend.c index 2044b30c1..495a15f8d 100644 --- a/tests/test_daemon_frontend.c +++ b/tests/test_daemon_frontend.c @@ -830,9 +830,27 @@ static bool frontend_backpressure_run_isolated(bool maintenance) { * below that budget SIGKILLs a slow-but-legitimate startup — on * oversubscribed CI runners this fired every round (announced=0, * daemon_signal=9) while fast local machines never saw it. Keep the - * deadline comfortably above the daemon's own worst-case budget. */ - bool announced_read = daemon > 0 && frontend_test_read_byte(ready_pipe[0], &announced_marker, - cbm_now_ms() + 30000U); + * deadline comfortably above the daemon's own worst-case budget. + * + * This wait is a LIVENESS BACKSTOP, not a race budget: the daemon either + * announces or it does not, and the test asserts the announcement, never a + * timing window. A backstop is only doing its job if it never fires on a + * legitimately slow start — so it has to scale with the build. Under + * MemorySanitizer (instrumented libc++ and origin tracking) startup runs + * several times slower than the 12s budget it must clear, which is what + * reddened test-msan while every other lane and the same suite under local + * MSan stayed green. Widening the backstop for sanitized builds costs + * nothing when the daemon is healthy: a passing run returns as soon as the + * byte arrives, whatever the ceiling is. */ +#if defined(CBM_SANITIZED_BUILD) || defined(__SANITIZE_ADDRESS__) || \ + defined(__SANITIZE_MEMORY__) || defined(__SANITIZE_THREAD__) + const uint64_t announce_backstop_ms = 180000U; +#else + const uint64_t announce_backstop_ms = 30000U; +#endif + bool announced_read = + daemon > 0 && frontend_test_read_byte(ready_pipe[0], &announced_marker, + cbm_now_ms() + announce_backstop_ms); bool announced = announced_read && announced_marker == 'R'; /* This raw runtime client intentionally owns no cohort lease. It is a From a654acbc88006aeedcae748503c287f94b2e4c92 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 11:57:35 +0200 Subject: [PATCH 07/11] fix(test-infra): grant the build-dir ACL by SID, not by name (#1532 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local Windows leg caught this within hours of merging #1532: on a machine that is not domain-joined, USERDOMAIN is the literal string WORKGROUP, and `WORKGROUP\test` is not a resolvable principal — WARN: build-dir DACL stamp (pre-wave) failed (user=WORKGROUP\test ...): WORKGROUP\test: No mapping between account names and security IDs was done. so the grant fails outright and the tree keeps its inherited `Authenticated Users:(M)`. The guard then correctly reports that cross-account mutation is still possible, and the whole leg stops. Since most Windows machines are not domain-joined, that is the common case, not the exotic one. #1532 was fixing a real bug in the other direction (a bare name resolves against the machine first, so a host named like its user grants to an empty principal). Both failures are name RESOLUTION failures, so this stops resolving names: the account is identified by SID, which is unambiguous everywhere. The SYSTEM and Administrators grants in these same commands have always used that form — the current user was the odd one out. Name lookup survives only as a fallback where PowerShell is unavailable. Applied to all three scripts carrying the stamp (run-tests-parallel, soak-test, memlab), since they share the failure and would otherwise drift apart. Co-Authored-By: Kiborgik Signed-off-by: Martin Vogel --- scripts/memlab.sh | 25 +++++++++++++--- scripts/run-tests-parallel.sh | 25 +++++++++++++--- scripts/soak-test.sh | 25 +++++++++++++--- src/mcp/mcp.c | 10 ++++++- tests/test_mcp.c | 56 +++++++++++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 13 deletions(-) diff --git a/scripts/memlab.sh b/scripts/memlab.sh index ffe5969f7..373742556 100644 --- a/scripts/memlab.sh +++ b/scripts/memlab.sh @@ -38,10 +38,27 @@ if [[ "$BINARY" == *.exe ]] && command -v cygpath >/dev/null 2>&1 && # Qualify with the domain: a bare name from coreutils `whoami` resolves # against the machine first, so on a host whose name equals the user's the # grant lands on an empty principal. - ME="$(whoami | tr -d '\r')" - if [ -n "${USERDOMAIN:-}" ]; then - ME="${USERDOMAIN}\\${ME}" - fi + # Identify the account by SID, never by name. icacls resolves a bare name + # against the machine first, so a host whose name equals the user's grants + # to an empty principal (#1532); and a USERDOMAIN-qualified name is + # UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between + # account names and security IDs was done" — which fails the grant outright + # and silently leaves the tree writable by Authenticated Users. A SID has + # neither ambiguity, and the SYSTEM/Administrators grants below already use + # this form. Name lookup remains only as a fallback where PowerShell is + # unavailable. + ME="$(powershell.exe -NoProfile -NonInteractive -Command \ + '[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null | + tr -d '\r\n')" + case "${ME}" in + S-1-*) ME="*${ME}" ;; + *) + ME="$(whoami | tr -d '\r')" + if [ -n "${USERDOMAIN:-}" ]; then + ME="${USERDOMAIN}\\${ME}" + fi + ;; + esac MSYS2_ARG_CONV_EXCL='*' icacls "$WIN_ROOT_W" /reset /Q >/dev/null 2>&1 || true if ! MSYS2_ARG_CONV_EXCL='*' icacls "$WIN_ROOT_W" /inheritance:r \ /grant:r "${ME}:(OI)(CI)F" '*S-1-5-18:(OI)(CI)F' '*S-1-5-32-544:(OI)(CI)F' \ diff --git a/scripts/run-tests-parallel.sh b/scripts/run-tests-parallel.sh index d7f8c0e8c..d28829540 100644 --- a/scripts/run-tests-parallel.sh +++ b/scripts/run-tests-parallel.sh @@ -64,10 +64,27 @@ stamp_windows_build_dir() { # (COMPUTERNAME=BUILD, user build) the grant lands on an empty principal # (BUILD\) and, combined with /inheritance:r above, locks this script out # of its own log directory. - me="$(whoami | tr -d '\r')" - if [ -n "${USERDOMAIN:-}" ]; then - me="${USERDOMAIN}\\${me}" - fi + # Identify the account by SID, never by name. icacls resolves a bare name + # against the machine first, so a host whose name equals the user's grants + # to an empty principal (#1532); and a USERDOMAIN-qualified name is + # UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between + # account names and security IDs was done" — which fails the grant outright + # and silently leaves the tree writable by Authenticated Users. A SID has + # neither ambiguity, and the SYSTEM/Administrators grants below already use + # this form. Name lookup remains only as a fallback where PowerShell is + # unavailable. + me="$(powershell.exe -NoProfile -NonInteractive -Command \ + '[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null | + tr -d '\r\n')" + case "${me}" in + S-1-*) me="*${me}" ;; + *) + me="$(whoami | tr -d '\r')" + if [ -n "${USERDOMAIN:-}" ]; then + me="${USERDOMAIN}\\${me}" + fi + ;; + esac # Normalize FIRST: some runner images stamp EXPLICIT (non-inherited) # Authenticated-Users ACEs onto the workspace tree, which /inheritance:r # cannot strip and /grant:r does not touch (it replaces only the granted diff --git a/scripts/soak-test.sh b/scripts/soak-test.sh index 12910bbd1..a12029a30 100755 --- a/scripts/soak-test.sh +++ b/scripts/soak-test.sh @@ -80,10 +80,27 @@ soak_stamp_windows_dir() { # Qualify with the domain: a bare name from coreutils `whoami` resolves # against the machine first, so on a host whose name equals the user's the # grant lands on an empty principal. - me="$(whoami | tr -d '\r')" - if [ -n "${USERDOMAIN:-}" ]; then - me="${USERDOMAIN}\\${me}" - fi + # Identify the account by SID, never by name. icacls resolves a bare name + # against the machine first, so a host whose name equals the user's grants + # to an empty principal (#1532); and a USERDOMAIN-qualified name is + # UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between + # account names and security IDs was done" — which fails the grant outright + # and silently leaves the tree writable by Authenticated Users. A SID has + # neither ambiguity, and the SYSTEM/Administrators grants below already use + # this form. Name lookup remains only as a fallback where PowerShell is + # unavailable. + me="$(powershell.exe -NoProfile -NonInteractive -Command \ + '[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null | + tr -d '\r\n')" + case "${me}" in + S-1-*) me="*${me}" ;; + *) + me="$(whoami | tr -d '\r')" + if [ -n "${USERDOMAIN:-}" ]; then + me="${USERDOMAIN}\\${me}" + fi + ;; + esac if ! output=$(MSYS2_ARG_CONV_EXCL='*' icacls "$dir_w" /reset /Q 2>&1); then echo "FAIL: soak DACL normalize failed (dir=$dir_w user=$me)" >&2 printf '%s\n' "$output" >&2 diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 7cbee0bf9..a0d3269f0 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -601,7 +601,7 @@ static const tool_def_t TOOLS[] = { "\"description\":\"Max enriched results per call. Default 10. Response includes " "'total_grep_matches' and 'total_results' so callers can detect truncation. No " "offset parameter — raise limit or narrow with file_pattern / path_filter to see more." - "\",\"default\":10}},\"required\":[\"pattern\",\"project\"]}"}, + "\",\"default\":10,\"minimum\":1}},\"required\":[\"pattern\",\"project\"]}"}, {"list_projects", "List projects", "List all indexed projects", "{\"type\":\"object\",\"properties\":{}}"}, @@ -9634,6 +9634,14 @@ static char *handle_search_code(cbm_mcp_server_t *srv, const char *args) { char *path_filter = cbm_mcp_get_string_arg(args, "path_filter"); char *mode_str = cbm_mcp_get_string_arg(args, "mode"); int limit = cbm_mcp_get_int_arg(args, "limit", MCP_DEFAULT_LIMIT); + /* #1511: a negative limit flowed straight into the result cap and came back + * as the reported count ("results: -5"), which reads to an agent as a real + * answer rather than a rejected argument. The schema now declares + * minimum:1, but a schema is a request to the client, never a guarantee to + * the server — clamp here too. */ + if (limit < 1) { + limit = MCP_DEFAULT_LIMIT; + } int context_lines = cbm_mcp_get_int_arg(args, "context", 0); bool use_regex = cbm_mcp_get_bool_arg(args, "regex"); uint64_t search_t0 = cbm_now_ms(); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index b83a8031c..1d99d71ad 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -4012,6 +4012,60 @@ TEST(tool_search_code_missing_pattern) { PASS(); } +/* #1511 (distilled from @lukiod's #1512): search_code echoed a negative limit + * back as the result count — "results: -5" — which an agent reads as an answer, + * not as a rejected argument. Both halves matter: the schema declares the bound + * so well-behaved clients never send it, and the handler clamps because a + * schema is a request to the client, never a guarantee to the server. */ +TEST(tool_search_code_negative_limit_is_not_echoed_issue1511) { + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + char *resp = + cbm_mcp_server_handle(srv, "{\"jsonrpc\":\"2.0\",\"id\":35,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"search_code\"," + "\"arguments\":{\"pattern\":\"func main\"," + "\"project\":\"nonexistent\",\"limit\":-5}}}"); + ASSERT_NOT_NULL(resp); + ASSERT_NULL(strstr(resp, "results: -5")); + free(resp); + cbm_mcp_server_free(srv); + PASS(); +} + +TEST(tool_search_code_limit_declares_a_minimum_issue1511) { + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + char *resp = cbm_mcp_server_handle( + srv, "{\"jsonrpc\":\"2.0\",\"id\":36,\"method\":\"tools/list\",\"params\":{}}"); + ASSERT_NOT_NULL(resp); + + yyjson_doc *doc = yyjson_read(resp, strlen(resp), 0); + yyjson_val *root = doc ? yyjson_doc_get_root(doc) : NULL; + yyjson_val *result = root ? yyjson_obj_get(root, "result") : NULL; + yyjson_val *tools = result ? yyjson_obj_get(result, "tools") : NULL; + yyjson_val *minimum = NULL; + if (tools && yyjson_is_arr(tools)) { + size_t index, max; + yyjson_val *tool; + yyjson_arr_foreach(tools, index, max, tool) { + yyjson_val *name = yyjson_obj_get(tool, "name"); + if (!name || !yyjson_is_str(name) || strcmp(yyjson_get_str(name), "search_code") != 0) { + continue; + } + yyjson_val *schema = yyjson_obj_get(tool, "inputSchema"); + yyjson_val *props = schema ? yyjson_obj_get(schema, "properties") : NULL; + yyjson_val *limit = props ? yyjson_obj_get(props, "limit") : NULL; + minimum = limit ? yyjson_obj_get(limit, "minimum") : NULL; + break; + } + } + bool declared = minimum && yyjson_is_int(minimum) && yyjson_get_int(minimum) >= 1; + yyjson_doc_free(doc); + free(resp); + cbm_mcp_server_free(srv); + + ASSERT_TRUE(declared); + PASS(); +} + TEST(tool_search_code_no_project) { cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); @@ -10591,6 +10645,8 @@ SUITE(mcp) { RUN_TEST(tool_get_code_snippet_missing_qn); RUN_TEST(tool_get_code_snippet_not_found); RUN_TEST(tool_search_code_missing_pattern); + RUN_TEST(tool_search_code_negative_limit_is_not_echoed_issue1511); + RUN_TEST(tool_search_code_limit_declares_a_minimum_issue1511); RUN_TEST(tool_search_code_no_project); RUN_TEST(search_code_multi_word); RUN_TEST(search_code_scoped_path_with_spaces_issue687); From b91abca173efdb15bdea04b156153c24f9578422 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 12:47:59 +0200 Subject: [PATCH 08/11] fix(daemon): admit a group-writable ancestor, and stop inventing an errno (#1537) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reporter came back with the output that named this: exact executable identity could not be verified (cache-private) - /Users/.../.cache/codebase-memory-mcp: ancestry component validation failed (errno 2) Two separate defects, and the second is why nobody could see the first. **The refusal.** The daemon's private-directory ancestry walk rejected any group-write bit on any ancestor — `(mode & 0022) == 0`. That is the identical rule #1535 removed on the activation side, in the sibling gate the consolidated strictness decision covers but that never got changed. A group-writable ~ or ~/.cache is ordinary: WSL2 ships 0775, so do several distro skeletons and any site with a shared primary group. So the daemon refused to run, permanently, with nothing the reader could close or restart. World-writable is still refused (any local user could swap a path component), and the private directory itself is still chmod'd to 0700 and verified after the walk — which is what makes admitting the ancestor safe: the thing that actually holds data stays owner-private either way. Group-writable ancestors are logged, so the weaker check is never silent. Both directions are tested; a fix that admitted everything would "fix" this by deleting the protection. **The invented errno.** When that check failed, the branch set no detail, so the caller fell back to printing `errno` — which nothing on the path sets. The reporter was handed ENOENT for a permission refusal and went looking for a missing file that was there all along; it cost a round trip to find out the number meant nothing. The branch now names the component and the rule. Reported by @listepo, who ran the diagnostics that turned this from a guess into a one-line root cause. Signed-off-by: Martin Vogel --- src/daemon/ipc.c | 33 ++++++++++++++++- tests/test_daemon_ipc.c | 82 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 4 deletions(-) diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index 92a36b744..d9b1a98e5 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -1383,8 +1383,26 @@ static bool posix_directory_parent_secure(int directory_fd) { !cbm_macos_extended_acl_fd_is_deny_only(directory_fd)) { return false; } - return (status.st_mode & 0022) == 0 || - (status.st_uid == (uid_t)0 && (status.st_mode & S_ISVTX) != 0); + /* #1537: this ANCESTOR check refused any group-write bit, which is the same + * rule #1535 removed on the activation side — and the sibling that decision + * covers but that never got changed. A group-writable ~ or ~/.cache is + * ordinary (WSL2 ships 0775, so do several distro skeletons and any site + * with a shared primary group), and refusing it here made the daemon + * unusable with no way for the reader to see why. + * + * WORLD-writable is still refused: any local user could swap a path + * component. Group-writable is admitted for ancestors only — the private + * directory itself is chmod'd to 0700 and verified after this walk, so the + * thing that actually holds data stays owner-private either way. */ + if ((status.st_mode & 0002) != 0) { + return status.st_uid == (uid_t)0 && (status.st_mode & S_ISVTX) != 0; + } + if ((status.st_mode & 0020) != 0) { + char mode_text[16]; + (void)snprintf(mode_text, sizeof(mode_text), "%04o", (unsigned)(status.st_mode & 07777)); + cbm_log_warn("daemon.private_dir_group_writable_ancestor", "mode", mode_text); + } + return true; } /* Validate a path transition only through the two already-open directory @@ -1435,6 +1453,17 @@ static int private_directory_tree_open(const char *directory_path) { ok = false; } else { ok = posix_directory_parent_secure(current_fd); + if (!ok) { + /* #1537: this branch used to leave the detail empty, so the + * caller fell back to printing errno — which NOTHING here sets. + * A reporter was handed "errno 2" (ENOENT) for a permission + * refusal and went looking for a missing file that existed. + * An unset errno is not a diagnosis; name the component. */ + ipc_validation_detail_set("%s: ancestor '%s' is not a usable private-directory " + "parent (must be owned by you, not world-writable, and " + "carry no allow-ACL)", + directory_path, component); + } bool created = ok && mkdirat(current_fd, component, 0700) == 0; if (!created && errno != EEXIST) { ok = false; diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index 94754230e..977826751 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -1667,8 +1667,7 @@ TEST(daemon_ipc_endpoint_is_namespaced_by_instance_key) { if (a_startup_status == 1 && !cbm_daemon_ipc_startup_lock_prepare_handoff(a_startup)) { a_startup_status = -1; } - if (other_startup_status == 1 && - !cbm_daemon_ipc_startup_lock_prepare_handoff(other_startup)) { + if (other_startup_status == 1 && !cbm_daemon_ipc_startup_lock_prepare_handoff(other_startup)) { other_startup_status = -1; } cbm_daemon_ipc_startup_lock_release(&a_startup); @@ -4712,6 +4711,83 @@ TEST(daemon_ipc_posix_rejects_non_socket_and_symlink_endpoints) { #endif /* !_WIN32 */ +#ifndef _WIN32 +/* #1537: the daemon's private-directory ancestry walk refused ANY group-write + * bit on an ancestor — the same rule #1535 removed on the activation side, and + * the sibling the consolidated strictness decision covers but that never got + * changed. A group-writable ~ or ~/.cache is ordinary (WSL2 ships 0775, so do + * several distro skeletons and any shared-primary-group site), so the daemon + * was unusable there. World-writable must still be refused: any local user + * could swap a path component. And the private directory itself must still end + * up owner-private, which is what makes admitting the ancestor safe. */ +TEST(daemon_ipc_posix_group_writable_ancestor_is_admitted_issue1537) { + char parent[TEST_PATH_CAP]; + char ancestor[TEST_PATH_CAP]; + char cache[TEST_PATH_CAP]; + bool paths_ok = false; + bool ancestor_ready = false; + bool secured = false; + bool leaf_owner_private = false; + + if (ipc_test_parent_new(parent, "posix-group-writable-ancestor")) { + int a = snprintf(ancestor, sizeof(ancestor), "%s/group-writable", parent); + int c = snprintf(cache, sizeof(cache), "%s/cache", ancestor); + paths_ok = a > 0 && a < (int)sizeof(ancestor) && c > 0 && c < (int)sizeof(cache); + } + if (paths_ok) { + ancestor_ready = mkdir(ancestor, 0775) == 0 && chmod(ancestor, 0775) == 0; + } + if (ancestor_ready) { + secured = cbm_daemon_ipc_private_directory_secure(cache); + struct stat leaf; + leaf_owner_private = + stat(cache, &leaf) == 0 && S_ISDIR(leaf.st_mode) && (leaf.st_mode & 07777) == 0700; + } + + (void)rmdir(cache); + (void)rmdir(ancestor); + ipc_test_remove_flat_dir(parent); + + ASSERT_TRUE(paths_ok); + ASSERT_TRUE(ancestor_ready); + ASSERT_TRUE(secured); + ASSERT_TRUE(leaf_owner_private); + PASS(); +} + +/* The other half of the same decision: world-writable stays refused. A rule + * that admitted everything would "fix" #1537 by removing the protection. */ +TEST(daemon_ipc_posix_world_writable_ancestor_still_refused_issue1537) { + char parent[TEST_PATH_CAP]; + char ancestor[TEST_PATH_CAP]; + char cache[TEST_PATH_CAP]; + bool paths_ok = false; + bool ancestor_ready = false; + bool refused = false; + + if (ipc_test_parent_new(parent, "posix-world-writable-ancestor")) { + int a = snprintf(ancestor, sizeof(ancestor), "%s/world-writable", parent); + int c = snprintf(cache, sizeof(cache), "%s/cache", ancestor); + paths_ok = a > 0 && a < (int)sizeof(ancestor) && c > 0 && c < (int)sizeof(cache); + } + if (paths_ok) { + ancestor_ready = mkdir(ancestor, 0777) == 0 && chmod(ancestor, 0777) == 0; + } + if (ancestor_ready) { + refused = !cbm_daemon_ipc_private_directory_secure(cache); + } + + (void)rmdir(cache); + (void)rmdir(ancestor); + ipc_test_remove_flat_dir(parent); + + ASSERT_TRUE(paths_ok); + ASSERT_TRUE(ancestor_ready); + ASSERT_TRUE(refused); + PASS(); +} +#endif /* !_WIN32 */ + SUITE(daemon_ipc) { RUN_TEST(daemon_ipc_pending_timeout_race_returns_completed_io); RUN_TEST(daemon_ipc_pending_wait_failure_cancels_and_drains); @@ -4750,6 +4826,8 @@ SUITE(daemon_ipc) { RUN_TEST(daemon_ipc_windows_startup_release_retains_retry_authority); RUN_TEST(daemon_ipc_frame_timeout_poisons_connection); #ifndef _WIN32 + RUN_TEST(daemon_ipc_posix_group_writable_ancestor_is_admitted_issue1537); + RUN_TEST(daemon_ipc_posix_world_writable_ancestor_still_refused_issue1537); RUN_TEST(daemon_ipc_posix_startup_lock_is_cross_process); RUN_TEST(daemon_ipc_posix_lifetime_reservation_rejects_fork_inheritance); RUN_TEST(daemon_ipc_posix_child_participant_handoff_retains_legacy_bridge); From 42e320a6cbbff4b4dcac1ef522301fbff2405a21 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 12:52:46 +0200 Subject: [PATCH 09/11] docs(pi): name the loader mechanism behind the default-export requirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two contributors independently sent the same fix for #1550 while ours was already in CI (#1553 by @musichen, who also reported the bug, and #1564 by @henri-edh). Rather than close both as duplicates, this takes the part of theirs that ours was missing. @henri-edh's comment names the actual mechanism: Pi loads an extension via jiti.import(path, { default: true }) and rejects anything that is not a function, so a named-only export hands the loader the module namespace object and the install fails with "Extension does not export a valid factory function". Ours said "Pi calls the default export" — true, but it does not tell the next reader why a named export cannot simply be added alongside, which is exactly the change someone would try. Their wording is now ours. The assertion is tightened the same way, from `export default function` to `export default function (pi)`, so a default export with the wrong shape cannot satisfy it. Co-Authored-By: musichen Co-Authored-By: henri-edh Signed-off-by: Martin Vogel --- src/cli/client_adapter.c | 13 +++++++------ tests/test_agent_clients.c | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/cli/client_adapter.c b/src/cli/client_adapter.c index be00cdb33..839b3d87b 100644 --- a/src/cli/client_adapter.c +++ b/src/cli/client_adapter.c @@ -157,12 +157,13 @@ char *cbm_client_adapter_pi(const char *binary_path) { /* Register every tool the registry advertises — never a hand-picked subset, * which is the drift this generator exists to prevent. * - * DEFAULT export, and it must stay that way: Pi loads an extension by - * calling its default export as a factory. A named `register` export made - * the file fail to load, and a Pi extension that fails to load takes EVERY - * pi command down with it — `pi doctor` included — not just cbm's tools - * (#1550). The named binding is kept alongside for anything that imported - * it by name. */ + * DEFAULT export, and it must stay that way. Pi loads an extension via + * jiti.import(path, { default: true }) and rejects anything that is not a + * function, so a named `export function register(pi)` hands the loader the + * module namespace object instead and every install fails with "Extension + * does not export a valid factory function". A Pi extension that fails to + * load takes EVERY pi command down with it — `pi doctor` included — not + * just cbm's tools (#1550). */ sb_append(&sb, "export default function (pi) {\n"); for (int i = 0; i < count; i++) { const char *name = cbm_mcp_tool_name(i); diff --git a/tests/test_agent_clients.c b/tests/test_agent_clients.c index edfc3a803..15794c2b1 100644 --- a/tests/test_agent_clients.c +++ b/tests/test_agent_clients.c @@ -1106,7 +1106,7 @@ TEST(client_adapter_pi_registers_every_registry_tool) { TEST(client_adapter_pi_default_exports_its_factory_issue1550) { char *js = cbm_client_adapter_pi("/usr/local/bin/codebase-memory-mcp"); ASSERT_NOT_NULL(js); - ASSERT_NOT_NULL(strstr(js, "export default function")); + ASSERT_NOT_NULL(strstr(js, "export default function (pi)")); /* The old shape must be gone: a bare `export function register(pi)` is the * exact form Pi rejects. */ ASSERT_NULL(strstr(js, "export function register(pi)")); From 2bb94372a49a76b7acac3bf0dda308f4e3ba2d1b Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 13:11:56 +0200 Subject: [PATCH 10/11] fix(cli): quote the skill description so strict YAML readers accept it (#1554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The installed SKILL.md carried an unquoted `description` whose value contains "Triggers on: ". In YAML a colon-space inside an unquoted scalar is a nested-mapping indicator, so strict readers — js-yaml's `load`, the frontmatter parser in `npx skills` — reject the whole document. The skill does not load wrongly; it does not load at all, and nothing in the install output says why. Quoting the scalar fixes it without touching a word of the text. The test checks the RULE rather than this string: for every skill, any frontmatter value containing ": " must be quoted. Pinning the current wording would let the next skill someone adds reintroduce exactly this bug. Verified by reverting the fix — the test goes red, and green again with it restored. Reported by @xezzon. Signed-off-by: Martin Vogel --- src/cli/cli.c | 9 ++++++-- tests/test_cli.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index ea86a3024..8f6b25adf 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -1255,12 +1255,17 @@ int cbm_replace_binary(const char *path, const unsigned char *data, int len, int static const char skill_content[] = "---\n" "name: codebase-memory\n" - "description: Use the codebase knowledge graph for structural code queries. " + /* #1554: the value contains "Triggers on: " — a colon-space, which YAML + * reads as a nested-mapping indicator inside an unquoted scalar. Strict + * parsers (js-yaml's load, the frontmatter reader in `npx skills`) reject + * the whole file, so the skill silently fails to load rather than loading + * wrongly. Quoting the scalar is the fix; the text itself is unchanged. */ + "description: \"Use the codebase knowledge graph for structural code queries. " "Triggers on: explore the codebase, understand the architecture, what functions exist, " "show me the structure, who calls this function, what does X call, trace the call chain, " "find callers of, show dependencies, impact analysis, dead code, unused functions, " "high fan-out, refactor candidates, code quality audit, graph query syntax, " - "Cypher query examples, edge types, how to use search_graph.\n" + "Cypher query examples, edge types, how to use search_graph.\"\n" "---\n" "\n" "# Codebase Memory — Knowledge Graph Tools\n" diff --git a/tests/test_cli.c b/tests/test_cli.c index 57e37ba69..311bf6397 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -12101,6 +12101,59 @@ TEST(cli_sha256_file_matches_known_vector) { * words people already type is not. Both flags must be accepted, do nothing, * and say so once. A genuinely unknown flag must still be rejected, or this * degenerates into ignoring typos. */ +/* #1554: the skill's `description` contained "Triggers on: " — a colon-space, + * which YAML reads as a nested-mapping indicator inside an UNQUOTED scalar. + * Strict readers (js-yaml's load, the frontmatter parser in `npx skills`) + * reject the whole document, so the installed skill silently fails to load. + * + * The rule is checked for EVERY skill, not just the one that broke: any + * frontmatter scalar whose value contains ": " must be quoted. Pinning only + * the current text would let the next added skill reintroduce it. */ +TEST(cli_skill_frontmatter_scalars_with_colons_are_quoted_issue1554) { + const cbm_skill_t *sk = cbm_get_skills(); + ASSERT_NOT_NULL(sk); + for (int i = 0; i < CBM_SKILL_COUNT; i++) { + const char *body = sk[i].content; + ASSERT_NOT_NULL(body); + /* Frontmatter is the block between the first two "---" lines. */ + ASSERT_TRUE(strncmp(body, "---\n", 4) == 0); + const char *end = strstr(body + 4, "\n---\n"); + ASSERT_NOT_NULL(end); + + const char *line = body + 4; + while (line < end) { + const char *eol = strchr(line, '\n'); + if (!eol || eol > end) { + break; + } + const char *sep = NULL; + for (const char *c = line; c < eol - 1; c++) { + if (c[0] == ':' && c[1] == ' ') { + sep = c; + break; + } + } + if (sep) { + const char *value = sep + 2; + /* A value that itself contains ": " must be quoted, or a + * strict parser treats it as a nested mapping. */ + bool has_inner_colon = false; + for (const char *c = value; c < eol - 1; c++) { + if (c[0] == ':' && c[1] == ' ') { + has_inner_colon = true; + break; + } + } + if (has_inner_colon) { + ASSERT_TRUE(value[0] == '"' || value[0] == '\''); + } + } + line = eol + 1; + } + } + PASS(); +} + TEST(cli_update_accepts_retired_variant_flags_issue1544) { char *ui_argv[] = {"--dry-run", "--ui", "--yes"}; char *standard_argv[] = {"--dry-run", "--standard", "--yes"}; @@ -12189,6 +12242,7 @@ SUITE(cli) { RUN_TEST(cli_install_config_failure_keeps_published_binary); RUN_TEST(cli_update_download_failure_does_not_quiesce_sessions); RUN_TEST(cli_update_already_current_does_not_quiesce_sessions); + RUN_TEST(cli_skill_frontmatter_scalars_with_colons_are_quoted_issue1554); RUN_TEST(cli_update_accepts_retired_variant_flags_issue1544); RUN_TEST(cli_update_agent_configs_finish_before_guard_release); RUN_TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index); From 63f26f4ce1d5eac0249da767721eaa947544c15e Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 13:23:27 +0200 Subject: [PATCH 11/11] fix(cli): forward CBM_CACHE_DIR to the Codex-spawned server (#1562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex sanitizes the environment of stdio MCP subprocesses and passes through only the names listed in `env_vars`. Our generated block had `command` and `args` and nothing else, so a user with a custom CBM_CACHE_DIR got a Codex-spawned server reading the DEFAULT cache while their account daemon used the configured one. The two disagree during initialization, the connection closes, and Codex exposes no cbm tools at all — a total failure that looks like a broken MCP server rather than a missing environment variable. The name is listed unconditionally rather than only when the variable happens to be set during install: `env_vars` names variables to forward IF PRESENT, so listing it costs nothing when unset, and it keeps working for someone who sets CBM_CACHE_DIR after installing — a case install-time detection would silently miss, producing exactly this bug again with no way to tell. Reported by @anton-matosov, who verified the corrected block end-to-end before filing. Signed-off-by: Martin Vogel --- src/cli/cli.c | 16 +++++++++++++++- tests/test_cli.c | 5 +++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index 8f6b25adf..f07c0ca6a 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -3278,8 +3278,22 @@ int cbm_upsert_codex_mcp(const char *binary_path, const char *config_path) { return CLI_ERR; } char block[CLI_BUF_8K]; + /* #1562: Codex sanitizes the environment of stdio MCP subprocesses, passing + * through only the names listed in env_vars. Without CBM_CACHE_DIR the + * Codex-spawned server silently falls back to the DEFAULT cache while the + * account daemon uses the configured one; the two disagree and the + * handshake closes during initialization, so Codex exposes no cbm tools at + * all. + * + * The name is listed unconditionally rather than only when the variable is + * set at install time: env_vars names variables to FORWARD IF PRESENT, so + * listing it costs nothing when unset and keeps working for someone who + * sets it after installing — which install-time detection would silently + * fail to cover. */ int written = snprintf(block, sizeof(block), - CODEX_CMM_SECTION "\ncommand = \"%s\"\nargs = []\n", escaped); + CODEX_CMM_SECTION "\ncommand = \"%s\"\nargs = []\n" + "env_vars = [\"CBM_CACHE_DIR\"]\n", + escaped); if (written < 0 || (size_t)written >= sizeof(block) || cbm_remove_codex_legacy_mcp(config_path) != 0) { return CLI_ERR; diff --git a/tests/test_cli.c b/tests/test_cli.c index 311bf6397..74a065432 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -10122,6 +10122,11 @@ TEST(cli_upsert_codex_mcp_fresh) { ASSERT_NOT_NULL(data); ASSERT(strstr(data, "[mcp_servers.codebase-memory-mcp]") != NULL); ASSERT(strstr(data, "/usr/local/bin/codebase-memory-mcp") != NULL); + /* #1562: Codex passes only the names listed in env_vars into a stdio MCP + * subprocess. Without CBM_CACHE_DIR the spawned server uses the DEFAULT + * cache while the daemon uses the configured one, the two disagree, and the + * handshake closes — Codex then shows no cbm tools at all. */ + ASSERT(strstr(data, "env_vars = [\"CBM_CACHE_DIR\"]") != NULL); test_rmdir_r(tmpdir); PASS();