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

Merge pull request #1871 from LazyXuan/fix/query-tools-readonly-annotations

Browse Source 
fix(mcp): non-mutating query-only store resolve; annotate the ten read-only tools
This commit is contained in:
Martin Vogel
2026-09-03 11:46:06 +02:00
committed by GitHub
parent 6cca768816 adb49b3a28
commit f37a47eb01
2 changed files with 149 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/mcp/mcp.c
+91 -26
View File
@@ -754,23 +754,30 @@ typedef struct {
} tool_annotation_def_t;
/* Tool annotations are deliberately explicit. All tools operate on the local
* repository/index domain, so none cross an open-world trust boundary. */
* repository/index domain, so none cross an open-world trust boundary.
*
* The ten pure query tools resolve their store through resolve_store(), whose
* query-only path is strictly non-mutating: a corrupt database is reported and
* left in place, never quarantined or rebuilt — quarantine/rebuild is reserved
* for write-side opens (index_repository, manage_adr writes). That is what
* makes readOnlyHint=true honest for them and lets plan-mode clients expose
* them (the "read-only analysis tools" surface described in #1100). */
static const tool_annotation_def_t TOOL_ANNOTATIONS[] = {
{"index_repository", false, false, true, false},
{"search_graph", false, true, true, false},
{"query_graph", false, true, true, false},
{"trace_path", false, true, true, false},
{"get_code_snippet", false, true, true, false},
{"search_graph", true, false, true, false},
{"query_graph", true, false, true, false},
{"trace_path", true, false, true, false},
{"get_code_snippet", true, false, true, false},
{"get_file_outline", false, true, true, false},
{"get_graph_schema", false, true, true, false},
{"get_graph_schema", true, false, true, false},
{"compare_graphs", true, false, true, false},
{"get_architecture", false, true, true, false},
{"search_code", false, true, true, false},
{"get_architecture", true, false, true, false},
{"search_code", true, false, true, false},
{"list_projects", true, false, true, false},
{"delete_project", false, true, true, false},
{"index_status", false, true, true, false},
{"check_index_coverage", false, true, true, false},
{"detect_changes", false, true, true, false},
{"index_status", true, false, true, false},
{"check_index_coverage", true, false, true, false},
{"detect_changes", true, false, true, false},
{"manage_adr", false, true, false, false},
{"ingest_traces", false, false, false, false},
};
@@ -1624,6 +1631,12 @@ struct cbm_mcp_server {
bool owns_store; /* true if we opened the store */
char *current_project; /* which project store is open for (heap) */
time_t store_last_used; /* last time resolve_store was called for a named project */
/* Set by a query-only resolve that hit a confirmed-corrupt database and
* left it in place (no quarantine, no rebuild). Error builders read this
* so the tool reply names the corruption instead of a misleading
* "project not found". */
bool readonly_resolve_hit_corrupt;
char readonly_corrupt_project[CBM_SZ_1K];
/* Session + auto-index state */
char session_root[CBM_SZ_1K]; /* detected project root path */
@@ -2217,14 +2230,21 @@ typedef enum {
STORE_RECOVERY_NONE,
STORE_RECOVERY_BUSY,
STORE_RECOVERY_TRY_GUARD_UNAVAILABLE,
/* Query-only resolve: a confirmed-corrupt database was reported and left
* in place. Not an error state of the recovery machinery — the caller is
* expected to answer with the corruption, not retry. */
STORE_RECOVERY_CORRUPT,
} store_recovery_status_t;
static cbm_store_t *resolve_store_internal(cbm_mcp_server_t *srv, const char *project,
bool mutation_already_held, bool nonblocking_recovery,
store_recovery_status_t *recovery_status) {
store_recovery_status_t *recovery_status,
bool allow_autorecovery) {
if (recovery_status) {
*recovery_status = STORE_RECOVERY_NONE;
}
srv->readonly_resolve_hit_corrupt = false;
srv->readonly_corrupt_project[0] = '\0';
if (!project) {
return NULL; /* project is required — no implicit fallback */
}
@@ -2254,6 +2274,38 @@ static cbm_store_t *resolve_store_internal(cbm_mcp_server_t *srv, const char *pr
project_db_path(project, path, sizeof(path));
srv->store = path[0] ? cbm_store_open_path_query(path) : NULL;
if (srv->store) {
/* Query-only resolve: classify a failed integrity check without any
* mutation — no lease, no quarantine. A corrupt database is reported
* and left in place for a write-side open (index_repository) to
* quarantine and rebuild; that is what keeps the ten read-only
* tools' readOnlyHint=true honest. A transient failure (lock, IO, or
* the shallow check's own prepare-error-as-corrupt blind spot) is
* answered as busy and retried by the next resolve. */
if (!allow_autorecovery && !cbm_store_check_integrity(srv->store)) {
cbm_store_close(srv->store);
srv->store = NULL;
srv->store = cbm_store_open_path_query(path);
cbm_integrity_verdict_t verdict = srv->store
? cbm_store_check_integrity_verdict(srv->store)
: CBM_INTEGRITY_TRANSIENT;
if (srv->store) {
cbm_store_close(srv->store);
srv->store = NULL;
}
if (verdict == CBM_INTEGRITY_CORRUPT) {
srv->readonly_resolve_hit_corrupt = true;
snprintf(srv->readonly_corrupt_project, sizeof(srv->readonly_corrupt_project), "%s",
project);
cbm_log_warn("store.corrupt_readonly", "project", project, "path", path, "action",
"left in place for a write-side rebuild");
if (recovery_status) {
*recovery_status = STORE_RECOVERY_CORRUPT;
}
} else if (recovery_status) {
*recovery_status = STORE_RECOVERY_BUSY;
}
return NULL;
}
/* Check DB integrity — back up (never silently delete) a corrupt DB */
if (!cbm_store_check_integrity(srv->store)) {
cbm_store_close(srv->store);
@@ -2358,7 +2410,8 @@ static cbm_store_t *resolve_store_internal(cbm_mcp_server_t *srv, const char *pr
}
static cbm_store_t *resolve_store(cbm_mcp_server_t *srv, const char *project) {
return resolve_store_internal(srv, project, false, false, NULL);
/* Query-only callers (every read tool): strictly non-mutating resolve. */
return resolve_store_internal(srv, project, false, false, NULL, false);
}
/* Forward decl — definition lives below alongside list_projects. */
@@ -2489,16 +2542,28 @@ static char *build_no_store_error(const char *project) {
: build_missing_project_error();
}
/* Same contract as build_no_store_error, but when the query-only resolve
* confirmed a corrupt database and left it in place, name that instead of a
* misleading "project not found". */
static char *build_no_store_error_checked(cbm_mcp_server_t *srv, const char *project) {
if (srv->readonly_resolve_hit_corrupt && project &&
strcmp(project, srv->readonly_corrupt_project) == 0) {
return build_project_list_error(
"project store is corrupt (left untouched); run index_repository to rebuild it");
}
return build_no_store_error(project);
}
/* Bail with the right error when no store is available. */
#define REQUIRE_STORE(store, project) \
do { \
if (!(store)) { \
char *_err = build_no_store_error(project); \
char *_res = cbm_mcp_text_result(_err, true); \
free(_err); \
free(project); \
return _res; \
} \
#define REQUIRE_STORE(store, project) \
do { \
if (!(store)) { \
char *_err = build_no_store_error_checked(srv, project); \
char *_res = cbm_mcp_text_result(_err, true); \
free(_err); \
free(project); \
return _res; \
} \
} while (0)
static bool project_has_adr(cbm_store_t *store, const char *project, const char *root_path) {
@@ -11627,7 +11692,7 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
char *root_path = get_project_root(srv, project);
if (!root_path) {
char *err = build_no_store_error(project);
char *err = build_no_store_error_checked(srv, project);
char *res = cbm_mcp_text_result(err, true);
free(err);
free(project);
@@ -12397,7 +12462,7 @@ static char *handle_manage_adr(cbm_mcp_server_t *srv, const char *args) {
* the UI are visible to each other (#256). */
store_recovery_status_t recovery_status = STORE_RECOVERY_NONE;
cbm_store_t *resolved =
resolve_store_internal(srv, project, mutation_held, !write_request, &recovery_status);
resolve_store_internal(srv, project, mutation_held, !write_request, &recovery_status, true);
if (!resolved) {
char *res = NULL;
if (recovery_status == STORE_RECOVERY_BUSY) {
@@ -12406,7 +12471,7 @@ static char *handle_manage_adr(cbm_mcp_server_t *srv, const char *args) {
res =
cbm_mcp_text_result("project recovery requires a nonblocking mutation guard", true);
} else {
char *err = build_no_store_error(project);
char *err = build_no_store_error_checked(srv, project);
res = cbm_mcp_text_result(err, true);
free(err);
}
@@ -12458,7 +12523,7 @@ static char *handle_manage_adr(cbm_mcp_server_t *srv, const char *args) {
invalidate_cached_store(srv);
resolved = NULL;
store = NULL;
resolved = resolve_store_internal(srv, project, true, false, NULL);
resolved = resolve_store_internal(srv, project, true, false, NULL, true);
}
if (resolved) {
store = open_adr_store_for_write(srv, resolved, &owned_rw);
tests/test_mcp.c
+58 -39
View File
@@ -934,23 +934,27 @@ TEST(mcp_tools_have_behavior_annotations) {
bool open_world;
} expected[] = {
{"index_repository", false, false, true, false},
/* These query tools can reach resolve_store(), whose corrupt-store
* recovery quarantines/removes database files. Keep the annotations
* conservative until query resolution is strictly non-mutating. */
{"search_graph", false, true, true, false},
{"query_graph", false, true, true, false},
{"trace_path", false, true, true, false},
{"get_code_snippet", false, true, true, false},
/* The ten query tools resolve their store through the strictly
* non-mutating query-only path: a corrupt database is reported and
* left in place, never quarantined or rebuilt. Quarantine/rebuild is
* a write-side job (index_repository, manage_adr writes), so the
* read-only annotations are honest and plan-mode clients can expose
* these tools. get_file_outline arrived after this split and keeps
* its upstream conservative annotation. */
{"search_graph", true, false, true, false},
{"query_graph", true, false, true, false},
{"trace_path", true, false, true, false},
{"get_code_snippet", true, false, true, false},
{"get_file_outline", false, true, true, false},
{"get_graph_schema", false, true, true, false},
{"get_graph_schema", true, false, true, false},
{"compare_graphs", true, false, true, false},
{"get_architecture", false, true, true, false},
{"search_code", false, true, true, false},
{"get_architecture", true, false, true, false},
{"search_code", true, false, true, false},
{"list_projects", true, false, true, false},
{"delete_project", false, true, true, false},
{"index_status", false, true, true, false},
{"check_index_coverage", false, true, true, false},
{"detect_changes", false, true, true, false},
{"index_status", true, false, true, false},
{"check_index_coverage", true, false, true, false},
{"detect_changes", true, false, true, false},
{"manage_adr", false, true, false, false},
{"ingest_traces", false, false, false, false},
};
@@ -7553,9 +7557,11 @@ TEST(tool_cross_repo_honors_source_name_override) {
}
/* Corrupt-store quarantine renames/unlinks the project DB and sidecars, so it
* is a mutation even when resolve_store() was reached by a query tool. Generic
* queries use a blocking guard for that recovery, while manage_adr reads must
* use one nonblocking acquisition and never nest a blocking lease. */
* is a mutation — and exactly the mutation a query-only resolve must never
* perform. A query tool that meets a corrupt store reports the corruption and
* leaves every file in place, taking no mutation lease; the quarantine path
* belongs to write-side opens, where manage_adr reads must use one nonblocking
* acquisition and never nest a blocking lease. */
TEST(tool_corrupt_store_cleanup_guard_is_balanced_and_not_nested) {
char cache[256];
snprintf(cache, sizeof(cache), "%s/cbm-mcp-corrupt-guard-XXXXXX", cbm_tmpdir());
@@ -7581,6 +7587,7 @@ TEST(tool_corrupt_store_cleanup_guard_is_balanced_and_not_nested) {
char *resp =
cbm_mcp_handle_tool(query_srv, "search_graph",
"{\"project\":\"guard-corrupt-project\",\"name_pattern\":\".*\"}");
bool query_reports_corruption = resp && strstr(resp, "corrupt") != NULL;
free(resp);
cbm_mcp_server_free(query_srv);
char query_backup_path[CBM_SZ_1K];
@@ -7588,6 +7595,11 @@ TEST(tool_corrupt_store_cleanup_guard_is_balanced_and_not_nested) {
mcp_find_corrupt_backups(cache, project, query_backup_path, sizeof(query_backup_path));
bool query_quarantined =
!cbm_file_exists(db_path) && query_backup_count == 1 && query_backup_path[0] != '\0';
/* Snapshot the on-disk state HERE: the manage_adr branch below replants
* and then quarantines this same path, and the suite's cleanup unlinks
* it before the assertions run — a later cbm_file_exists() would observe
* that teardown, not the query-only resolve this check pins. */
bool query_db_left_in_place = cbm_file_exists(db_path);
/* Replant the same deterministic corruption to exercise manage_adr's
* already-held lease independently from the query server above. */
@@ -7617,13 +7629,14 @@ TEST(tool_corrupt_store_cleanup_guard_is_balanced_and_not_nested) {
free(saved_cache_copy);
cbm_rmdir(cache);
ASSERT_TRUE(query_quarantined);
ASSERT_EQ(query_probe.begin_count, 1);
ASSERT_EQ(query_probe.end_count, 1);
ASSERT_STR_EQ(query_probe.begin_projects[0], project);
ASSERT_STR_EQ(query_probe.end_projects[0], project);
ASSERT_TRUE(query_probe.db_exists_at_begin);
ASSERT_FALSE(query_probe.db_exists_at_end);
/* A query-only resolve never repairs: no lease is taken, the corrupt DB
* and its sidecars stay in place, and the reply names the corruption
* instead of "project not found". */
ASSERT_FALSE(query_quarantined);
ASSERT_TRUE(query_db_left_in_place);
ASSERT_TRUE(query_reports_corruption);
ASSERT_EQ(query_probe.begin_count, 0);
ASSERT_EQ(query_probe.end_count, 0);
ASSERT_TRUE(adr_quarantined);
ASSERT_EQ(adr_probe.begin_count, 0);
ASSERT_EQ(adr_probe.try_begin_count, 1);
@@ -7636,8 +7649,9 @@ TEST(tool_corrupt_store_cleanup_guard_is_balanced_and_not_nested) {
}
/* Integrity is checked before the lease is requested, but quarantine itself
* must fail closed when that lease is denied. In particular, a rejected query
* may not remove either a recoverable DB generation or its committed WAL. */
* must fail closed when that lease is denied. A query-only resolve never
* reaches that point at all: it asks for no lease, touches no file, and
* leaves both a recoverable DB generation and its committed WAL untouched. */
TEST(tool_corrupt_store_cleanup_guard_denial_preserves_db_and_wal) {
char cache[256];
snprintf(cache, sizeof(cache), "%s/cbm-mcp-corrupt-denied-XXXXXX", cbm_tmpdir());
@@ -7695,9 +7709,9 @@ TEST(tool_corrupt_store_cleanup_guard_denial_preserves_db_and_wal) {
free(saved_cache_copy);
cbm_rmdir(cache);
ASSERT_EQ(begin_count, 1);
ASSERT_EQ(begin_count, 0);
ASSERT_EQ(end_count, 0);
ASSERT_TRUE(guarded_project);
ASSERT_FALSE(guarded_project);
ASSERT_TRUE(db_unchanged);
ASSERT_TRUE(wal_unchanged);
ASSERT_EQ(backup_count, 0);
@@ -7801,10 +7815,11 @@ TEST(tool_manage_adr_corrupt_store_missing_try_guard_reports_configuration) {
PASS();
}
/* Another session may publish a good generation while this query waits for
* the mutation lease. Cleanup must re-open and re-check the path after lease
* acquisition; quarantining based on the stale pre-wait handle loses the new
* generation and returns a false "not indexed" result. */
/* A write-side open must trust only the generation that is current after its
* mutation lease is held: another session may have published a good
* generation while this request waited for the lease. Quarantining based on
* a stale pre-wait handle loses the new generation and returns a false
* "not indexed" result. */
TEST(tool_corrupt_store_cleanup_rechecks_generation_after_guard_wait) {
char cache[256];
snprintf(cache, sizeof(cache), "%s/cbm-mcp-corrupt-recheck-XXXXXX", cbm_tmpdir());
@@ -7832,7 +7847,8 @@ TEST(tool_corrupt_store_cleanup_rechecks_generation_after_guard_wait) {
cbm_mcp_server_set_project_mutation_guard(srv, mcp_replacing_mutation_guard_begin,
mcp_replacing_mutation_guard_end, &replacement);
char *resp = cbm_mcp_handle_tool(
srv, "search_graph", "{\"project\":\"guard-corrupt-recheck\",\"name_pattern\":\".*\"}");
srv, "manage_adr",
"{\"project\":\"guard-corrupt-recheck\",\"mode\":\"update\",\"content\":\"# ADR\\n\\nPending replacement.\"}");
bool response_used_replacement =
resp && !response_contains_json_fragment(resp, "\"isError\":true");
free(resp);
@@ -7908,7 +7924,8 @@ TEST(tool_corrupt_store_cleanup_preserves_existing_backup_and_uses_unique_name)
cbm_mcp_server_set_project_mutation_guard(srv, mcp_mutation_guard_probe_begin,
mcp_mutation_guard_probe_end, &probe);
char *resp = cbm_mcp_handle_tool(
srv, "search_graph", "{\"project\":\"guard-corrupt-unique\",\"name_pattern\":\".*\"}");
srv, "manage_adr",
"{\"project\":\"guard-corrupt-unique\",\"mode\":\"update\",\"content\":\"# ADR\\n\\nPending quarantine.\"}");
free(resp);
cbm_mcp_server_free(srv);
@@ -7981,9 +7998,9 @@ TEST(tool_corrupt_store_cleanup_publish_failure_preserves_db_and_wal) {
mcp_mutation_guard_probe_end, &guard);
mcp_quarantine_hook_probe_t hook = {.deny_step = "before_snapshot_publish"};
cbm_mcp_server_set_quarantine_test_hook(srv, mcp_quarantine_hook_probe, &hook);
char *resp =
cbm_mcp_handle_tool(srv, "search_graph",
"{\"project\":\"guard-corrupt-publish-fail\",\"name_pattern\":\".*\"}");
char *resp = cbm_mcp_handle_tool(
srv, "manage_adr",
"{\"project\":\"guard-corrupt-publish-fail\",\"mode\":\"update\",\"content\":\"# ADR\\n\\nPending publish.\"}");
bool db_unchanged = mcp_file_matches_snapshot(db_path, db_before, db_len);
bool wal_unchanged = mcp_file_matches_snapshot(wal_path, wal_before, wal_len);
@@ -8056,8 +8073,8 @@ TEST(tool_corrupt_store_cleanup_publishes_complete_wal_snapshot_before_delete) {
mcp_quarantine_hook_probe_t hook = {.deny_step = "after_snapshot_publish"};
cbm_mcp_server_set_quarantine_test_hook(srv, mcp_quarantine_hook_probe, &hook);
char *resp = cbm_mcp_handle_tool(
srv, "search_graph",
"{\"project\":\"guard-corrupt-after-publish\",\"name_pattern\":\".*\"}");
srv, "manage_adr",
"{\"project\":\"guard-corrupt-after-publish\",\"mode\":\"update\",\"content\":\"# ADR\\n\\nPending publish.\"}");
bool db_unchanged = mcp_file_matches_snapshot(db_path, db_before, db_len);
bool wal_unchanged = mcp_file_matches_snapshot(wal_path, wal_before, wal_len);
@@ -11071,12 +11088,14 @@ TEST(tool_resolve_store_by_internal_name_issue704) {
free(q_alpha);
/* ── D: the 0-byte ghost is NOT resolvable ─────────────────────── */
/* A query-only resolve reports the corrupt generation and leaves it in
* place — the reply names the corruption instead of "not found". */
char *q_ghost = cbm_mcp_server_handle(
srv, "{\"jsonrpc\":\"2.0\",\"id\":4,\"method\":\"tools/call\","
"\"params\":{\"name\":\"search_graph\",\"arguments\":{"
"\"project\":\"ghost704\",\"name_pattern\":\".*\",\"limit\":5}}}");
ASSERT_NOT_NULL(q_ghost);
ASSERT_NOT_NULL(strstr(q_ghost, "not found"));
ASSERT_NOT_NULL(strstr(q_ghost, "corrupt"));
free(q_ghost);
/* ── E: addressing the drifted db by its FILENAME stays not-found ── */