diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 75ff8e396c..fefecb3a12 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -15521,6 +15521,48 @@ static bool detect_add_changed_path(char ***files, int *file_count, int *file_ca return true; } +/* Git status porcelain always reports paths relative to the Git ROOT, while + * graph file_paths are relative to the indexed project root, which may be a + * subdirectory of the repository (#1951). `prefix` is that subdirectory as + * `git rev-parse --show-prefix` prints it ("" at the root, else "dir/"). + * Returns the project-relative tail, or NULL for a path outside the project. */ +static const char *detect_project_relative_path(const char *git_path, const char *prefix) { + size_t prefix_length = strlen(prefix); + if (strncmp(git_path, prefix, prefix_length) != 0) { + return NULL; + } + return git_path + prefix_length; +} + +/* A `--show-prefix` answer: empty at the repository root, otherwise a + * '/'-terminated relative directory. */ +static bool detect_valid_git_prefix(const char *value) { + size_t length = strlen(value); + return length == 0 || value[length - 1] == '/'; +} + +/* Read one '\n'-terminated `git rev-parse` record (a trailing '\r' dropped) + * into out when it is terminated, fits, and passes `valid`. Anything else is + * a malformed answer and fails the request closed. */ +static bool detect_read_rev_parse_line(FILE *stream, bool *oom, char *out, size_t out_size, + bool (*valid)(const char *)) { + bool terminated = false; + char *record = detect_read_record(stream, '\n', oom, &terminated); + if (!record) { + return false; + } + size_t length = strlen(record); + if (length > 0 && record[length - 1] == '\r') { + record[--length] = '\0'; + } + bool ok = terminated && length < out_size && valid(record); + if (ok) { + memcpy(out, record, length + 1U); + } + free(record); + return ok; +} + static int detect_changed_path_compare(const void *left, const void *right) { const char *const *left_path = left; const char *const *right_path = right; @@ -15871,15 +15913,20 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { * HEAD advance cannot mix revisions within one answer. */ char head_oid[65] = ""; char base_oid[65] = ""; + /* The project root may be a subdirectory of the Git worktree (#1951); + * --show-prefix names it so every changed path can be translated from the + * Git-root coordinate system into the graph's project-relative one. */ + char git_prefix[CBM_SZ_4K] = ""; + bool git_prefix_valid = false; char resolve_cmd[CBM_SZ_2K]; #ifdef _WIN32 snprintf(resolve_cmd, sizeof(resolve_cmd), - "git -C \"%s\" rev-parse \"HEAD^{commit}\" \"%s^{commit}\" 2>NUL", root_path, - base_branch); + "git -C \"%s\" rev-parse \"HEAD^{commit}\" \"%s^{commit}\" --show-prefix 2>NUL", + root_path, base_branch); #else snprintf(resolve_cmd, sizeof(resolve_cmd), - "git -C '%s' rev-parse 'HEAD^{commit}' '%s^{commit}' 2>/dev/null", root_path, - base_branch); + "git -C '%s' rev-parse 'HEAD^{commit}' '%s^{commit}' --show-prefix 2>/dev/null", + root_path, base_branch); #endif char resolve_output_path[CBM_SZ_2K] = {0}; cbm_proc_result_t resolve_result = {0}; @@ -15887,43 +15934,24 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { mcp_run_shell_command_cancellable(srv, resolve_cmd, resolve_output_path, &resolve_result); bool resolve_cancelled = resolve_result.cancellation_requested || mcp_request_cancelled(srv); bool resolve_oom = false; - char *resolved_head = NULL; - char *resolved_base = NULL; FILE *resolve_fp = resolve_run == 0 && resolve_result.exit_code == 0 && !resolve_cancelled ? cbm_fopen(resolve_output_path, "rb") : NULL; if (resolve_fp) { - bool head_terminated = false; - bool base_terminated = false; - resolved_head = detect_read_record(resolve_fp, '\n', &resolve_oom, &head_terminated); - resolved_base = detect_read_record(resolve_fp, '\n', &resolve_oom, &base_terminated); - if (resolved_head) { - size_t length = strlen(resolved_head); - if (length > 0 && resolved_head[length - 1] == '\r') { - resolved_head[--length] = '\0'; - } - if (head_terminated && detect_valid_object_id(resolved_head)) { - memcpy(head_oid, resolved_head, length + 1U); - } - } - if (resolved_base) { - size_t length = strlen(resolved_base); - if (length > 0 && resolved_base[length - 1] == '\r') { - resolved_base[--length] = '\0'; - } - if (base_terminated && detect_valid_object_id(resolved_base)) { - memcpy(base_oid, resolved_base, length + 1U); - } - } + /* Records in argument order: HEAD, base, then the --show-prefix line. */ + (void)detect_read_rev_parse_line(resolve_fp, &resolve_oom, head_oid, sizeof(head_oid), + detect_valid_object_id); + (void)detect_read_rev_parse_line(resolve_fp, &resolve_oom, base_oid, sizeof(base_oid), + detect_valid_object_id); + git_prefix_valid = detect_read_rev_parse_line(resolve_fp, &resolve_oom, git_prefix, + sizeof(git_prefix), detect_valid_git_prefix); (void)fclose(resolve_fp); } - free(resolved_head); - free(resolved_base); if (resolve_output_path[0]) { (void)cbm_unlink(resolve_output_path); } if (resolve_cancelled || resolve_run != 0 || resolve_result.exit_code != 0 || resolve_oom || - !head_oid[0] || !base_oid[0]) { + !head_oid[0] || !base_oid[0] || !git_prefix_valid) { free(direction); free(root_path); free(project); @@ -15936,6 +15964,12 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { return cbm_mcp_text_result( "git revision resolution failed: the contained command could not complete", true); } + if (head_oid[0] && base_oid[0] && !resolve_oom) { + return cbm_mcp_text_result( + "git revision resolution failed: the project root's path inside the Git " + "worktree could not be determined", + true); + } return cbm_mcp_text_result( "git revision resolution failed: base_branch or HEAD is not a commit", true); } @@ -16035,7 +16069,9 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { free(record); break; } - if (!detect_add_changed_path(&files, &file_count, &file_cap, record)) { + const char *project_path = detect_project_relative_path(record, git_prefix); + if (project_path && + !detect_add_changed_path(&files, &file_count, &file_cap, project_path)) { changed_path_oom = true; free(record); break; @@ -16110,7 +16146,10 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { free(record); break; } - if (!detect_add_changed_path(&files, &file_count, &file_cap, record + PAIR_LEN + 1U)) { + const char *project_path = + detect_project_relative_path(record + PAIR_LEN + 1U, git_prefix); + if (project_path && + !detect_add_changed_path(&files, &file_count, &file_cap, project_path)) { changed_path_oom = true; free(record); break; @@ -16227,20 +16266,22 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { * index combined with insertions earlier in the file shifts the node lines * relative to the hunks and can mis-scope. The failure is bounded by * detect_collect_seeds' zero-overlap fallback: a file whose definitions all - * miss reverts to whole-file seeding rather than dropping out. */ + * miss reverts to whole-file seeding rather than dropping out. + * --relative keeps hunk paths in the same project-relative coordinates as + * `files` when the project root is a repository subdirectory (#1951). */ cbm_changed_hunk_t *hunks = NULL; int hunk_count = 0; if (want_symbols) { char hunk_cmd[CBM_SZ_2K]; #ifdef _WIN32 snprintf(hunk_cmd, sizeof(hunk_cmd), - "git -C \"%s\" diff --unified=0 \"%s\" \"%s\" -- 2>NUL && " - "git -C \"%s\" diff --unified=0 -- 2>NUL", + "git -C \"%s\" diff --relative --unified=0 \"%s\" \"%s\" -- 2>NUL && " + "git -C \"%s\" diff --relative --unified=0 -- 2>NUL", root_path, merge_base, head_oid, root_path); #else snprintf(hunk_cmd, sizeof(hunk_cmd), - "git -C '%s' diff --unified=0 '%s' '%s' -- 2>/dev/null && " - "git -C '%s' diff --unified=0 -- 2>/dev/null", + "git -C '%s' diff --relative --unified=0 '%s' '%s' -- 2>/dev/null && " + "git -C '%s' diff --relative --unified=0 -- 2>/dev/null", root_path, merge_base, head_oid, root_path); #endif char hunk_output_path[CBM_SZ_2K] = {0}; diff --git a/tests/test_mcp.c b/tests/test_mcp.c index ffa9ea3a3b..d1a78ab31c 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -14127,6 +14127,146 @@ TEST(tool_detect_changes_finds_nested_untracked_file_and_impact_seed) { PASS(); } +/* Issue #1951: the indexed project root is a SUBDIRECTORY of a normal Git + * repository. Git reports changed paths relative to the Git root + * ("game/src/math.c"), while graph file_paths are relative to the project + * root ("src/math.c"). detect_changes must translate Git-root paths into the + * project's coordinate system (diff, hunk and status records alike) and leave + * changes outside the project out, or the seed lookup finds nothing. */ +TEST(tool_detect_changes_subdirectory_project_translates_git_root_paths_issue1951) { + char repo[CBM_SZ_4K]; + snprintf(repo, sizeof(repo), "%s/cbm-detect-subdir-XXXXXX", cbm_tmpdir()); + ASSERT_NOT_NULL(cbm_mkdtemp(repo)); + char cache[CBM_SZ_4K]; + snprintf(cache, sizeof(cache), "%s/cbm-detect-subdir-cache-XXXXXX", cbm_tmpdir()); + ASSERT_NOT_NULL(cbm_mkdtemp(cache)); + + char project_root[CBM_SZ_4K]; + snprintf(project_root, sizeof(project_root), "%s/game", repo); + ASSERT_EQ(cbm_mkdir(project_root), 0); + char source_dir[CBM_SZ_4K]; + snprintf(source_dir, sizeof(source_dir), "%s/src", project_root); + ASSERT_EQ(cbm_mkdir(source_dir), 0); + char math_source[CBM_SZ_4K]; + snprintf(math_source, sizeof(math_source), "%s/math.c", source_dir); + ASSERT_EQ(th_write_file(math_source, "int add_one(int x) { return x + 1; }\n" + "int untouched(void) { return 0; }\n"), + 0); + char use_source[CBM_SZ_4K]; + snprintf(use_source, sizeof(use_source), "%s/use.c", source_dir); + ASSERT_EQ( + th_write_file(use_source, "int add_one(int x);\nint use_it(void) { return add_one(4); }\n"), + 0); + char outside_source[CBM_SZ_4K]; + snprintf(outside_source, sizeof(outside_source), "%s/outside.c", repo); + ASSERT_EQ(th_write_file(outside_source, "int outside(void) { return 0; }\n"), 0); + + const char *const init_args[] = {"init", "-q", NULL}; + const char *const add_args[] = {"add", "-A", NULL}; + const char *const commit_args[] = { + "-c", "user.name=cbm-test", + "-c", "user.email=cbm-test@example.invalid", + "-c", "commit.gpgsign=false", + "commit", "-q", + "-m", "fixture", + NULL, + }; + ASSERT_EQ(mcp_test_git(repo, init_args), 0); + ASSERT_EQ(mcp_test_git(repo, add_args), 0); + ASSERT_EQ(mcp_test_git(repo, commit_args), 0); + + /* Worktree edits: a tracked change inside the project (diff + hunk path), + * an untracked file inside it (status path) and a change outside it. */ + ASSERT_EQ(th_write_file(math_source, "int add_one(int x) { return x + 2; }\n" + "int untouched(void) { return 0; }\n"), + 0); + char fresh_source[CBM_SZ_4K]; + snprintf(fresh_source, sizeof(fresh_source), "%s/fresh.c", source_dir); + ASSERT_EQ(th_write_file(fresh_source, "int fresh(void) { return 1; }\n"), 0); + ASSERT_EQ(th_write_file(outside_source, "int outside(void) { return 1; }\n"), 0); + + const char *saved_cache = getenv("CBM_CACHE_DIR"); + char *saved_cache_copy = saved_cache ? strdup(saved_cache) : NULL; + ASSERT_EQ(cbm_setenv("CBM_CACHE_DIR", cache, 1), 0); + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + ASSERT_NOT_NULL(srv); + cbm_store_t *store = cbm_mcp_server_store(srv); + ASSERT_NOT_NULL(store); + const char *project = "detect-subdir-project"; + ASSERT_EQ(cbm_store_upsert_project(store, project, project_root), CBM_STORE_OK); + cbm_mcp_server_set_project(srv, project); + + cbm_node_t seed = {.project = project, + .label = "Function", + .name = "add_one", + .qualified_name = "fixture.src.math.add_one", + .file_path = "src/math.c", + .start_line = 1, + .end_line = 1}; + int64_t seed_id = cbm_store_upsert_node(store, &seed); + ASSERT_GT(seed_id, 0); + /* Same file, untouched line: only hunk scoping (project-relative hunk + * paths) keeps it out of the seeds; whole-file fallback would add it. */ + cbm_node_t untouched = {.project = project, + .label = "Function", + .name = "untouched", + .qualified_name = "fixture.src.math.untouched", + .file_path = "src/math.c", + .start_line = 2, + .end_line = 2}; + ASSERT_GT(cbm_store_upsert_node(store, &untouched), 0); + cbm_node_t caller = {.project = project, + .label = "Function", + .name = "use_it", + .qualified_name = "fixture.src.use.use_it", + .file_path = "src/use.c", + .start_line = 2, + .end_line = 2}; + int64_t caller_id = cbm_store_upsert_node(store, &caller); + ASSERT_GT(caller_id, 0); + cbm_edge_t edge = { + .project = project, .source_id = caller_id, .target_id = seed_id, .type = "CALLS"}; + ASSERT_GT(cbm_store_insert_edge(store, &edge), 0); + + char *response = + cbm_mcp_handle_tool(srv, "detect_changes", + "{\"project\":\"detect-subdir-project\",\"base_branch\":\"HEAD\"," + "\"scope\":\"impact\",\"depth\":2,\"max_output_tokens\":10000," + "\"format\":\"json\"}"); + char *inner = extract_text_content(response); + yyjson_doc *doc = inner ? yyjson_read(inner, strlen(inner), 0) : NULL; + yyjson_val *root = doc ? yyjson_doc_get_root(doc) : NULL; + yyjson_val *changed_files = root ? yyjson_obj_get(root, "changed_files") : NULL; + yyjson_val *first_path = changed_files ? yyjson_arr_get(changed_files, 0) : NULL; + yyjson_val *second_path = changed_files ? yyjson_arr_get(changed_files, 1) : NULL; + yyjson_val *impacted = root ? yyjson_obj_get(root, "impacted") : NULL; + yyjson_val *first_impact = impacted ? yyjson_arr_get(impacted, 0) : NULL; + bool project_relative_paths = + root && yyjson_get_int(yyjson_obj_get(root, "changed_total")) == 2 && first_path && + second_path && strcmp(yyjson_get_str(first_path), "src/fresh.c") == 0 && + strcmp(yyjson_get_str(second_path), "src/math.c") == 0; + bool seed_found = root && yyjson_get_int(yyjson_obj_get(root, "seed_symbols")) == 1; + bool impact_found = first_impact && strcmp(yyjson_get_str(yyjson_obj_get(first_impact, "qn")), + "fixture.src.use.use_it") == 0; + if (!project_relative_paths || !seed_found || !impact_found) { + fprintf(stderr, " issue1951 response: %s\n", inner ? inner : "(null)"); + } + + yyjson_doc_free(doc); + free(inner); + free(response); + cbm_mcp_server_free(srv); + restore_cache_dir(saved_cache_copy); + free(saved_cache_copy); + ASSERT_EQ(th_rmtree(cache), 0); + ASSERT_EQ(th_rmtree(repo), 0); + + ASSERT_TRUE(project_relative_paths); + ASSERT_TRUE(seed_found); + ASSERT_TRUE(impact_found); + PASS(); +} + TEST(tool_detect_changes_escapes_newline_path_in_tree_and_round_trips_json) { #ifdef _WIN32 /* Win32 rejects control characters in filenames, so Windows cannot create @@ -20697,6 +20837,7 @@ SUITE(mcp) { RUN_TEST(tool_detect_changes_invalid_base_is_an_error); RUN_TEST(tool_detect_changes_preserves_utf8_git_path_and_impact_seed); RUN_TEST(tool_detect_changes_finds_nested_untracked_file_and_impact_seed); + RUN_TEST(tool_detect_changes_subdirectory_project_translates_git_root_paths_issue1951); RUN_TEST(tool_detect_changes_escapes_newline_path_in_tree_and_round_trips_json); RUN_TEST(tool_detect_changes_staged_rename_uses_exact_destination_record); RUN_TEST(tool_detect_changes_contained_commands_clean_up_error_and_success);