diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 6bb0f5e37..768a6c232 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -1702,6 +1702,8 @@ struct cbm_mcp_server { #ifdef CBM_ENABLE_TEST_SEAMS cbm_mcp_auto_index_count_test_hook_fn auto_index_count_test_hook; void *auto_index_count_test_context; + cbm_mcp_snapshot_read_test_hook_fn snapshot_read_test_hook; + void *snapshot_read_test_context; #endif size_t search_output_limit_override; const char *search_scan_command_override; @@ -2035,6 +2037,17 @@ void cbm_mcp_server_set_search_scan_timeout_for_test(cbm_mcp_server_t *srv, uint srv->search_scan_timeout_override_set = override_set; } } +#ifdef CBM_ENABLE_TEST_SEAMS +void cbm_mcp_server_set_snapshot_read_test_hook(cbm_mcp_server_t *srv, + cbm_mcp_snapshot_read_test_hook_fn hook, + void *context) { + if (!srv) { + return; + } + srv->snapshot_read_test_hook = hook; + srv->snapshot_read_test_context = context; +} +#endif /* ── Cache dir + project DB path helpers ───────────────────────── */ @@ -14889,19 +14902,19 @@ static void detect_snapshot_add_field(cbm_sha256_ctx *hash, const char *value) { cbm_sha256_update(hash, "|", 1); } -static void detect_snapshot_add_changed_file(cbm_sha256_ctx *hash, const char *root_path, - const char *relative_path) { +static bool detect_snapshot_add_changed_file(cbm_mcp_server_t *srv, cbm_sha256_ctx *hash, + const char *root_path, const char *relative_path) { detect_snapshot_add_field(hash, relative_path); size_t root_len = strlen(root_path); size_t relative_len = strlen(relative_path); if (root_len > (size_t)-1 - relative_len - 2U) { detect_snapshot_add_field(hash, "path_overflow"); - return; + return false; } char *absolute = malloc(root_len + relative_len + 2U); if (!absolute) { detect_snapshot_add_field(hash, "path_oom"); - return; + return false; } memcpy(absolute, root_path, root_len); absolute[root_len] = '/'; @@ -14909,33 +14922,59 @@ static void detect_snapshot_add_changed_file(cbm_sha256_ctx *hash, const char *r cbm_path_info_t info = {0}; if (cbm_path_info_utf8(absolute, &info) != 0) { - detect_snapshot_add_field(hash, "absent"); + /* A missing path can be a legitimate deletion, but the portable + * metadata API cannot distinguish it from permission and transient + * inspection failures on every supported platform. Hash the observed + * state for diagnostics, but do not issue a cursor from incomplete + * evidence. Offset paging remains available. */ + detect_snapshot_add_field(hash, "metadata_unavailable"); free(absolute); - return; + return false; } char metadata[160]; snprintf(metadata, sizeof(metadata), "r%d:d%d:l%d:s%lld:m%lld", info.is_regular ? 1 : 0, info.is_directory ? 1 : 0, info.is_symlink ? 1 : 0, (long long)info.size, (long long)info.mtime_ns); detect_snapshot_add_field(hash, metadata); + bool complete = true; if (info.is_regular) { - FILE *file = cbm_fopen(absolute, "rb"); + bool allow_open = true; +#ifdef CBM_ENABLE_TEST_SEAMS + if (srv && srv->snapshot_read_test_hook) { + allow_open = srv->snapshot_read_test_hook(srv->snapshot_read_test_context, absolute); + } +#else + (void)srv; +#endif + FILE *file = allow_open ? cbm_fopen(absolute, "rb") : NULL; if (file) { unsigned char buffer[64 * 1024]; size_t count; while ((count = fread(buffer, 1, sizeof(buffer), file)) > 0) { cbm_sha256_update(hash, buffer, count); } - detect_snapshot_add_field(hash, ferror(file) ? "read_error" : "read_complete"); + bool read_error = ferror(file) != 0; + detect_snapshot_add_field(hash, read_error ? "read_error" : "read_complete"); + complete = !read_error; (void)fclose(file); } else { detect_snapshot_add_field(hash, "open_error"); + complete = false; } + } else if (info.is_symlink) { + /* lstat-style metadata identifies the link object but not its target + * bytes. The cross-platform metadata API intentionally does not expose + * a readlink/reparse payload, so fail closed instead of pretending the + * cursor is bound to the changed link identity. */ + detect_snapshot_add_field(hash, "link_identity_unavailable"); + complete = false; } free(absolute); + return complete; } -static void detect_snapshot_fingerprint(const char *root_path, const char *head_oid, +static bool detect_snapshot_fingerprint(cbm_mcp_server_t *srv, const char *root_path, + const char *head_oid, const char *base_oid, const char *merge_base, const char *generation, char **files, int file_count, const cbm_traverse_result_t *impact, @@ -14948,8 +14987,11 @@ static void detect_snapshot_fingerprint(const char *root_path, const char *head_ detect_snapshot_add_field(&hash, base_oid); detect_snapshot_add_field(&hash, merge_base); detect_snapshot_add_field(&hash, generation); + bool complete = true; for (int i = 0; i < file_count; i++) { - detect_snapshot_add_changed_file(&hash, root_path, files[i]); + if (!detect_snapshot_add_changed_file(srv, &hash, root_path, files[i])) { + complete = false; + } } if (impact) { for (int i = 0; i < impact->visited_count; i++) { @@ -14980,6 +15022,7 @@ static void detect_snapshot_fingerprint(const char *root_path, const char *head_ out[i * 2 + 1] = hex[digest[i] & 0x0f]; } out[32] = '\0'; + return complete; } static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { @@ -15522,13 +15565,20 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { char generation[96] = "unknown"; (void)cbm_store_generation(store, generation, sizeof(generation)); char detect_snapshot[33]; - detect_snapshot_fingerprint(root_path, head_oid, base_oid, merge_base, generation, files, - file_count, &impact, modules, nmods, module_overflow, - detect_snapshot); + bool detect_snapshot_complete = detect_snapshot_fingerprint( + srv, root_path, head_oid, base_oid, merge_base, generation, files, file_count, &impact, + modules, nmods, module_overflow, detect_snapshot); const char *cursor_error = NULL; detect_cursor_t decoded_cursor = {0}; - if (changed_cursor_arg && changed_cursor_arg[0]) { + bool cursor_supplied = (changed_cursor_arg && changed_cursor_arg[0]) || + (impact_cursor_arg && impact_cursor_arg[0]) || + (module_cursor_arg && module_cursor_arg[0]); + if (cursor_supplied && !detect_snapshot_complete) { + cursor_error = "snapshot_unavailable: changed file bytes could not be fingerprinted — " + "rerun without the cursor after the files are readable"; + } + if (!cursor_error && changed_cursor_arg && changed_cursor_arg[0]) { if (changed_offset != 0) { cursor_error = "cursor_params_mismatch: changed_cursor cannot be combined with a " "nonzero changed_offset"; @@ -15621,6 +15671,9 @@ render_detect_output: if (output_budget_floor_exceeded) { cbm_tree_scalar_bool(&sb, "output_budget_floor_exceeded", true); } + if (!detect_snapshot_complete) { + cbm_tree_scalar_bool(&sb, "snapshot_cursor_unavailable", true); + } /* Changed files are independently pageable: traversal still used every * file, so paging affects presentation only, never the graph answer. */ cbm_tree_scalar_int(&sb, "changed_total", file_count); @@ -15630,9 +15683,11 @@ render_detect_output: if (changed_has_more && changed_returned > 0) { cbm_tree_scalar_int(&sb, "changed_next_offset", changed_start + changed_returned); char cursor[80]; - detect_cursor_encode('c', detect_snapshot, detect_qhash, - changed_start + changed_returned, cursor); - cbm_tree_scalar_str(&sb, "changed_next_cursor", cursor); + if (detect_snapshot_complete) { + detect_cursor_encode('c', detect_snapshot, detect_qhash, + changed_start + changed_returned, cursor); + cbm_tree_scalar_str(&sb, "changed_next_cursor", cursor); + } } else if (changed_has_more) { cbm_tree_scalar_bool(&sb, changed_limit == 0 ? "changed_continuation_requires_positive_limit" @@ -15659,7 +15714,8 @@ render_detect_output: cbm_tree_scalar_int(&sb, "seed_symbols", seed_count); if (want_symbols) { detect_emit_impacted_tree(&sb, &impact, imp_start, imp_returned, truncated); - if (imp_start + imp_returned < impact.visited_count && imp_returned > 0) { + if (detect_snapshot_complete && imp_start + imp_returned < impact.visited_count && + imp_returned > 0) { char cursor[80]; detect_cursor_encode('i', detect_snapshot, detect_qhash, imp_start + imp_returned, cursor); @@ -15675,9 +15731,11 @@ render_detect_output: if (module_has_more && module_returned > 0) { cbm_tree_scalar_int(&sb, "module_next_offset", module_start + module_returned); char cursor[80]; - detect_cursor_encode('m', detect_snapshot, detect_qhash, - module_start + module_returned, cursor); - cbm_tree_scalar_str(&sb, "module_next_cursor", cursor); + if (detect_snapshot_complete) { + detect_cursor_encode('m', detect_snapshot, detect_qhash, + module_start + module_returned, cursor); + cbm_tree_scalar_str(&sb, "module_next_cursor", cursor); + } } else if (module_has_more) { cbm_tree_scalar_bool(&sb, module_limit == 0 @@ -15735,6 +15793,9 @@ render_detect_output: if (output_budget_floor_exceeded) { yyjson_mut_obj_add_bool(doc, root_obj, "output_budget_floor_exceeded", true); } + if (!detect_snapshot_complete) { + yyjson_mut_obj_add_bool(doc, root_obj, "snapshot_cursor_unavailable", true); + } yyjson_mut_obj_add_int(doc, root_obj, "changed_total", file_count); yyjson_mut_obj_add_int(doc, root_obj, "changed_returned", changed_returned); bool changed_has_more = changed_start + changed_returned < file_count; @@ -15743,9 +15804,11 @@ render_detect_output: yyjson_mut_obj_add_int(doc, root_obj, "changed_next_offset", changed_start + changed_returned); char cursor[80]; - detect_cursor_encode('c', detect_snapshot, detect_qhash, - changed_start + changed_returned, cursor); - yyjson_mut_obj_add_strcpy(doc, root_obj, "changed_next_cursor", cursor); + if (detect_snapshot_complete) { + detect_cursor_encode('c', detect_snapshot, detect_qhash, + changed_start + changed_returned, cursor); + yyjson_mut_obj_add_strcpy(doc, root_obj, "changed_next_cursor", cursor); + } } else if (changed_has_more) { yyjson_mut_obj_add_bool(doc, root_obj, changed_limit == 0 @@ -15781,10 +15844,12 @@ render_detect_output: yyjson_mut_obj_add_bool(doc, root_obj, "impacted_has_more", impacted_has_more); if (impacted_has_more && imp_returned > 0) { yyjson_mut_obj_add_int(doc, root_obj, "impacted_next_offset", imp_start + imp_returned); - char cursor[80]; - detect_cursor_encode('i', detect_snapshot, detect_qhash, imp_start + imp_returned, - cursor); - yyjson_mut_obj_add_strcpy(doc, root_obj, "impacted_next_cursor", cursor); + if (detect_snapshot_complete) { + char cursor[80]; + detect_cursor_encode('i', detect_snapshot, detect_qhash, imp_start + imp_returned, + cursor); + yyjson_mut_obj_add_strcpy(doc, root_obj, "impacted_next_cursor", cursor); + } } else if (impacted_has_more) { yyjson_mut_obj_add_bool(doc, root_obj, "impacted_continuation_requires_higher_budget", true); @@ -15800,10 +15865,12 @@ render_detect_output: if (module_has_more && module_returned > 0) { yyjson_mut_obj_add_int(doc, root_obj, "module_next_offset", module_start + module_returned); - char cursor[80]; - detect_cursor_encode('m', detect_snapshot, detect_qhash, - module_start + module_returned, cursor); - yyjson_mut_obj_add_strcpy(doc, root_obj, "module_next_cursor", cursor); + if (detect_snapshot_complete) { + char cursor[80]; + detect_cursor_encode('m', detect_snapshot, detect_qhash, + module_start + module_returned, cursor); + yyjson_mut_obj_add_strcpy(doc, root_obj, "module_next_cursor", cursor); + } } else if (module_has_more) { yyjson_mut_obj_add_bool(doc, root_obj, module_limit == 0 diff --git a/src/mcp/mcp_internal.h b/src/mcp/mcp_internal.h index a9e67d832..bba4305c3 100644 --- a/src/mcp/mcp_internal.h +++ b/src/mcp/mcp_internal.h @@ -76,6 +76,14 @@ void cbm_search_code_build_grep_cmd(char *cmd, size_t cmd_sz, bool use_regex, bo const char *file_pattern, const char *tmpfile, const char *filelist, const char *root_path); #ifdef CBM_ENABLE_TEST_SEAMS +/* Reject opening one changed file while detect_changes fingerprints its live + * snapshot. This makes otherwise platform-specific permission/read races + * deterministic without changing production filesystem behavior. */ +typedef bool (*cbm_mcp_snapshot_read_test_hook_fn)(void *context, const char *absolute_path); +void cbm_mcp_server_set_snapshot_read_test_hook(cbm_mcp_server_t *srv, + cbm_mcp_snapshot_read_test_hook_fn hook, + void *context); + /* Filesystem PATH_MAX prevents a portable end-to-end fixture for multi-KiB * stored paths. This seam drives the normal search result ownership, * directory aggregation, and renderer pipeline with synthetic identities. */ diff --git a/tests/test_mcp.c b/tests/test_mcp.c index bfb597a3e..4fcb14690 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -266,6 +266,10 @@ static bool mcp_search_cache_close(mcp_search_cache_t *cache) { cache->saved_cache = NULL; return th_rmtree(cache->path) == 0; } +static bool mcp_snapshot_read_hook_probe(void *context, const char *absolute_path) { + const char *rejected_fragment = context; + return !rejected_fragment || !absolute_path || !strstr(absolute_path, rejected_fragment); +} typedef struct { const char *name; @@ -13327,6 +13331,37 @@ TEST(tool_detect_changes_pages_changed_files_and_honors_semantic_budget) { free(inner); free(response); + /* A cursor is only safe when every changed regular file contributed its + * bytes. Force a portable open failure after Git has listed the file: the + * first page must stay usable but cannot advertise snapshot-bound cursors, + * and an already-issued cursor must fail closed while the snapshot cannot + * be re-established. */ + cbm_mcp_server_set_snapshot_read_test_hook(srv, mcp_snapshot_read_hook_probe, seed_file); + response = cbm_mcp_handle_tool( + srv, "detect_changes", + "{\"project\":\"detect-pages-project\",\"base_branch\":\"HEAD\"," + "\"scope\":\"files\",\"format\":\"json\"}"); + ASSERT_NOT_NULL(response); + inner = extract_text_content(response); + ASSERT_NOT_NULL(inner); + doc = yyjson_read(inner, strlen(inner), 0); + ASSERT_NOT_NULL(doc); + root = yyjson_doc_get_root(doc); + ASSERT_TRUE(yyjson_get_bool(yyjson_obj_get(root, "snapshot_cursor_unavailable"))); + ASSERT_NULL(yyjson_obj_get(root, "changed_next_cursor")); + yyjson_doc_free(doc); + free(inner); + free(response); + + response = cbm_mcp_handle_tool(srv, "detect_changes", cursor_args); + ASSERT_NOT_NULL(response); + inner = extract_text_content(response); + ASSERT_NOT_NULL(inner); + ASSERT_NOT_NULL(strstr(inner, "snapshot_unavailable")); + free(inner); + free(response); + cbm_mcp_server_set_snapshot_read_test_hook(srv, NULL, NULL); + /* The same path set with changed bytes is a different live snapshot. The * old opaque cursor must fail loudly instead of applying offset 20 to the * new ordering/impact model. */ @@ -13340,6 +13375,30 @@ TEST(tool_detect_changes_pages_changed_files_and_honors_semantic_budget) { ASSERT_NOT_NULL(strstr(inner, "snapshot_changed")); free(inner); free(response); + +#ifndef _WIN32 + /* lstat metadata is not the symlink's identity: different target bytes can + * have the same length and timestamp. Until those bytes are fingerprinted, + * keep the otherwise complete answer but withhold snapshot cursors. */ + char changed_link[CBM_SZ_4K]; + snprintf(changed_link, sizeof(changed_link), "%s/changed-link.c", repo); + ASSERT_EQ(symlink("first-target", changed_link), 0); + response = cbm_mcp_handle_tool( + srv, "detect_changes", + "{\"project\":\"detect-pages-project\",\"base_branch\":\"HEAD\"," + "\"scope\":\"files\",\"format\":\"json\"}"); + ASSERT_NOT_NULL(response); + inner = extract_text_content(response); + ASSERT_NOT_NULL(inner); + doc = yyjson_read(inner, strlen(inner), 0); + ASSERT_NOT_NULL(doc); + root = yyjson_doc_get_root(doc); + ASSERT_TRUE(yyjson_get_bool(yyjson_obj_get(root, "snapshot_cursor_unavailable"))); + ASSERT_NULL(yyjson_obj_get(root, "changed_next_cursor")); + yyjson_doc_free(doc); + free(inner); + free(response); +#endif free(changed_cursor); cbm_mcp_server_free(srv);