From 7a674af80ac25d49a75af5fefcdaff42cd85ceb1 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Sat, 25 Jul 2026 07:17:26 +0000 Subject: [PATCH] fix(mcp): resolve index_repository's repo_path from a known project name handle_index_repository requires repo_path and errors with "repo_path is required" if it is absent. list_projects only ever advertises the project NAME, never repo_path, and every read tool accepts that name back through get_project_arg's project/project_name/ project_id/projectName aliases -- but index_repository did not, so re-indexing an already-indexed project by the name list_projects gave you had no resolution path and failed immediately. Add resolved_repo_path_from_project_arg(): a query-only lookup of the project's own stored root_path, tried when repo_path is absent before giving up. It never touches srv->store or srv->current_project, so it cannot disturb whatever project the server already has cached, and an unresolvable name still falls through to the original, correct "repo_path is required" error. Verified in a clean ubuntu:24.04 container (TEST_SUITES=mcp make -f Makefile.cbm test-focused): the new tool_index_repository_resolves_root_path_from_project_name_issue1211 fails on unpatched main (strstr(resp, "repo_path is required") is not NULL) and passes with the fix; the companion tool_index_repository_unknown_project_name_still_requires_repo_path guards the fallback stays exact for a name that was never indexed. Full mcp suite: 179 passed on both sides, with one pre-existing, unrelated failure (tool_detect_changes_contained_commands_clean_up_error_and_success, test_mcp.c:5814) reproducing identically on unmodified origin/main. scripts/lint.sh --ci and scripts/security-audit.sh both clean. Fixes #1211 Signed-off-by: Amir Fathi --- src/mcp/mcp.c | 35 ++++++++++++++++++ tests/test_mcp.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index fb5a750ec..92ba60562 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -7566,12 +7566,47 @@ static char *index_args_with_repo_path(const char *args, const char *canonical_r return rewritten; } +/* #1211: index_repository requires repo_path, but list_projects only ever + * advertises the project NAME, and every read tool accepts that name back via + * get_project_arg's "project"/"project_name"/"project_id"/"projectName" + * aliases (mcp.c:1385). Re-indexing an already-indexed project by that same + * name had no resolution path and fell straight to "repo_path is required". + * Look up the project's own stored root_path (list_projects proves it's on + * file) before giving up. Query-only open, always closed here: this never + * creates a store or touches srv->store/srv->current_project, so it cannot + * disturb whatever project the server has cached. */ +static char *resolved_repo_path_from_project_arg(const char *args) { + char *project = get_project_arg(args); + if (!project) { + return NULL; + } + char db_path[CBM_SZ_1K]; + project_db_path(project, db_path, sizeof(db_path)); + cbm_store_t *store = db_path[0] ? cbm_store_open_path_query(db_path) : NULL; + char *root_path = NULL; + if (store) { + cbm_project_t proj = {0}; + if (cbm_store_get_project(store, project, &proj) == CBM_STORE_OK) { + root_path = proj.root_path ? heap_strdup(proj.root_path) : NULL; + cbm_project_free_fields(&proj); + } + cbm_store_close(store); + } + free(project); + return root_path; +} + static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { char *repo_path = cbm_mcp_get_string_arg(args, "repo_path"); char *mode_str = cbm_mcp_get_string_arg(args, "mode"); char *name_override = cbm_mcp_get_string_arg(args, "name"); cbm_normalize_path_sep(repo_path); + if (!repo_path) { + repo_path = resolved_repo_path_from_project_arg(args); + cbm_normalize_path_sep(repo_path); + } + if (!repo_path) { free(mode_str); free(name_override); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index f1809dccd..37b3403cd 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -5349,6 +5349,97 @@ TEST(tool_index_repository_reports_store_backed_adr) { PASS(); } +/* #1211: list_projects only ever advertises the project NAME, never the + * repo_path, but re-indexing by that same name (the natural next call) used + * to fall straight to "repo_path is required" because nothing resolved the + * name back to its stored root_path. Index once by repo_path, then re-index + * by project name alone and confirm it actually indexes instead of erroring. */ +TEST(tool_index_repository_resolves_root_path_from_project_name_issue1211) { + char tmp_dir[256]; + snprintf(tmp_dir, sizeof(tmp_dir), "/tmp/cbm-index-byname-test-XXXXXX"); + if (!cbm_mkdtemp(tmp_dir)) { + PASS(); + } + char cache[256]; + snprintf(cache, sizeof(cache), "/tmp/cbm-index-byname-cache-XXXXXX"); + if (!cbm_mkdtemp(cache)) { + cbm_rmdir(tmp_dir); + PASS(); + } + + const char *saved = getenv("CBM_CACHE_DIR"); + char *saved_copy = saved ? strdup(saved) : NULL; + cbm_setenv("CBM_CACHE_DIR", cache, 1); + + char src_path[512]; + snprintf(src_path, sizeof(src_path), "%s/main.py", tmp_dir); + FILE *fp = fopen(src_path, "w"); + ASSERT_NOT_NULL(fp); + fputs("def main():\n return 'ok'\n", fp); + fclose(fp); + + char *project = cbm_project_name_from_path(tmp_dir); + ASSERT_NOT_NULL(project); + + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + ASSERT_NOT_NULL(srv); + + char index_args[1024]; + snprintf(index_args, sizeof(index_args), "{\"repo_path\":\"%s\",\"mode\":\"fast\"}", tmp_dir); + char *resp = cbm_mcp_handle_tool(srv, "index_repository", index_args); + ASSERT_NOT_NULL(resp); + ASSERT(response_contains_json_fragment(resp, "\"status\":\"indexed\"")); + free(resp); + + char by_name_args[512]; + snprintf(by_name_args, sizeof(by_name_args), "{\"project\":\"%s\",\"mode\":\"fast\"}", project); + resp = cbm_mcp_handle_tool(srv, "index_repository", by_name_args); + ASSERT_NOT_NULL(resp); + ASSERT_NULL(strstr(resp, "repo_path is required")); + ASSERT(response_contains_json_fragment(resp, "\"status\":\"indexed\"")); + free(resp); + + cbm_mcp_server_free(srv); + cleanup_project_db(cache, project); + restore_cache_dir(saved_copy); + free(saved_copy); + free(project); + remove(src_path); + cbm_rmdir(cache); + cbm_rmdir(tmp_dir); + PASS(); +} + +/* Same gap, opposite outcome: a project name that was never indexed has no + * stored root_path to resolve, so it must still fail with the same clear + * "repo_path is required" error rather than a resolver crash or silent + * no-op. Guards the fallback path the fix above added. */ +TEST(tool_index_repository_unknown_project_name_still_requires_repo_path) { + char cache[256]; + snprintf(cache, sizeof(cache), "/tmp/cbm-index-byname-unknown-cache-XXXXXX"); + if (!cbm_mkdtemp(cache)) { + PASS(); + } + const char *saved = getenv("CBM_CACHE_DIR"); + char *saved_copy = saved ? strdup(saved) : NULL; + cbm_setenv("CBM_CACHE_DIR", cache, 1); + + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + ASSERT_NOT_NULL(srv); + + char *resp = cbm_mcp_handle_tool(srv, "index_repository", + "{\"project\":\"never-indexed-project\"}"); + ASSERT_NOT_NULL(resp); + ASSERT_NOT_NULL(strstr(resp, "repo_path is required")); + free(resp); + + cbm_mcp_server_free(srv); + restore_cache_dir(saved_copy); + free(saved_copy); + cbm_rmdir(cache); + PASS(); +} + TEST(tool_index_repository_dot_uses_absolute_project_key_and_preserves_adr) { char tmp_dir[256]; snprintf(tmp_dir, sizeof(tmp_dir), "/tmp/cbm-index-dot-adr-test-XXXXXX"); @@ -9424,6 +9515,8 @@ SUITE(mcp) { RUN_TEST(tool_manage_adr_get_with_existing_adr); RUN_TEST(tool_manage_adr_unified_backend_issue256); RUN_TEST(tool_index_repository_reports_store_backed_adr); + RUN_TEST(tool_index_repository_resolves_root_path_from_project_name_issue1211); + RUN_TEST(tool_index_repository_unknown_project_name_still_requires_repo_path); RUN_TEST(tool_index_repository_dot_uses_absolute_project_key_and_preserves_adr); RUN_TEST(index_repository_relative_path_uses_explicit_session_root); RUN_TEST(index_repository_supervisor_uses_canonical_session_path);