diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index af2092a82..4eeebf45a 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -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); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index 8db63b1ed..dc7716b14 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -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 ── */