diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 62a74b0fb..a7b524761 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -9736,19 +9736,77 @@ static int mcp_run_shell_command_cancellable(cbm_mcp_server_t *srv, const char * return contained ? 0 : -1; } -/* Collect BFS seed ids: every symbol DEFINED in a changed file (everything but - * the structural container labels — those have no CALLS edges). These anchor - * the multi-source impact traversal. */ +/* Does `node`'s line range overlap any recorded hunk for `file`? Used to scope + * seed detection to the actually-changed lines rather than the whole file. + * Non-static (declared in mcp_internal.h) so tests can exercise the overlap + * logic directly, matching this file's existing white-box test hooks. */ +bool cbm_detect_node_in_hunks(const cbm_node_t *node, const cbm_changed_hunk_t *hunks, + int hunk_count, const char *file) { + for (int h = 0; h < hunk_count; h++) { + if (strcmp(hunks[h].path, file) == 0 && node->start_line <= hunks[h].end_line && + node->end_line >= hunks[h].start_line) { + return true; + } + } + return false; +} + +/* Collect BFS seed ids for one changed file (everything but the structural + * container labels — those have no CALLS edges). These anchor the + * multi-source impact traversal. + * + * When `hunks` has at least one entry for `file`, only definitions whose line + * range overlaps a hunk become seeds — a one-line edit inside a single + * function no longer seeds every other definition in the file. When no hunk + * is recorded for `file` (a brand-new/untracked file has no comparable + * "before" state, or the hunk fetch failed/was skipped), every non-container + * definition in the file is a seed — the previous, whole-file behavior — so + * this is a precision improvement, not a new failure mode. */ +/* Structural container labels carry no CALLS edges and span the whole file, so + * they are never seeds. Shared by the seeding loop and the overlap probe. */ +static bool detect_is_seedable_label(const char *lb) { + return lb && strcmp(lb, "File") != 0 && strcmp(lb, "Folder") != 0 && + strcmp(lb, "Project") != 0 && strcmp(lb, "Module") != 0 && strcmp(lb, "Package") != 0 && + strcmp(lb, "Section") != 0; +} + static void detect_collect_seeds(cbm_store_t *store, const char *project, const char *file, - int64_t **seeds, int *n, int *cap) { + const cbm_changed_hunk_t *hunks, int hunk_count, int64_t **seeds, + int *n, int *cap) { cbm_node_t *nodes = NULL; int ncount = 0; cbm_store_find_nodes_by_file(store, project, file, &nodes, &ncount); + bool scope_to_hunks = false; + for (int h = 0; h < hunk_count; h++) { + if (strcmp(hunks[h].path, file) == 0) { + scope_to_hunks = true; + break; + } + } + /* A file can have hunks yet no SEEDABLE definition overlapping any of them: + * an import-only edit, a module-level constant, or a change above the first + * definition all land outside every definition's line range. Scoping would + * then drop the file from the seed set entirely — strictly worse recall + * than the whole-file behavior this replaces. Probe for an overlap first + * and keep whole-file seeding for that file when there is none. + * + * The probe must apply the same label filter as the seeding loop below: + * container nodes span the whole file (a Module node is lines 1..EOF), so + * counting them would report an overlap for every hunk and defeat the + * fallback entirely. */ + if (scope_to_hunks) { + bool any_overlap = false; + for (int i = 0; i < ncount && !any_overlap; i++) { + any_overlap = detect_is_seedable_label(nodes[i].label) && + cbm_detect_node_in_hunks(&nodes[i], hunks, hunk_count, file); + } + scope_to_hunks = any_overlap; + } for (int i = 0; i < ncount; i++) { - const char *lb = nodes[i].label; - if (lb && strcmp(lb, "File") != 0 && strcmp(lb, "Folder") != 0 && - strcmp(lb, "Project") != 0 && strcmp(lb, "Module") != 0 && strcmp(lb, "Package") != 0 && - strcmp(lb, "Section") != 0) { + if (detect_is_seedable_label(nodes[i].label)) { + if (scope_to_hunks && !cbm_detect_node_in_hunks(&nodes[i], hunks, hunk_count, file)) { + continue; + } if (*n >= *cap) { *cap = *cap ? *cap * 2 : 16; *seeds = safe_realloc(*seeds, (size_t)*cap * sizeof(int64_t)); @@ -10035,6 +10093,76 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { int seed_count = 0; int seed_cap = 0; + /* Hunk line ranges (unified=0 diff), used to scope seed detection to the + * actually-changed lines instead of every definition in a changed file + * (see detect_collect_seeds). Best-effort: any failure here just leaves + * `hunks` empty and every file falls back to its previous whole-file + * seeding — this is a precision improvement, not a correctness + * dependency, so it is never treated as a request-level failure. + * + * Coordinate systems: `base...HEAD` hunks carry HEAD-side line numbers, + * the worktree diff carries worktree-side ones, and node line ranges come + * from the indexed snapshot. These agree while the index is fresh — the + * watcher reindexes on HEAD movement and on a dirty tree — but a stale + * 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. */ + 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\"...HEAD 2>NUL & " + "git -C \"%s\" diff --unified=0 2>NUL", + root_path, base_branch, root_path); +#else + snprintf(hunk_cmd, sizeof(hunk_cmd), + "{ git -C '%s' diff --unified=0 '%s'...HEAD 2>/dev/null; " + "git -C '%s' diff --unified=0 2>/dev/null; }", + root_path, base_branch, root_path); +#endif + char hunk_output_path[CBM_SZ_2K] = {0}; + cbm_proc_result_t hunk_result = {0}; + int hunk_run = + mcp_run_shell_command_cancellable(srv, hunk_cmd, hunk_output_path, &hunk_result); + bool hunk_cancelled = hunk_result.cancellation_requested || mcp_request_cancelled(srv); + FILE *hfp = (!hunk_cancelled && hunk_run == 0) ? cbm_fopen(hunk_output_path, "rb") : NULL; + if (hfp) { + (void)fseek(hfp, 0, SEEK_END); + long hsz = ftell(hfp); + if (hsz > 0) { + (void)fseek(hfp, 0, SEEK_SET); + char *hbuf = malloc((size_t)hsz + SKIP_ONE); + if (hbuf) { + size_t hread = fread(hbuf, SKIP_ONE, (size_t)hsz, hfp); + hbuf[hread] = '\0'; + enum { HUNK_CAP = 4096 }; + hunks = safe_realloc(NULL, (size_t)HUNK_CAP * sizeof(cbm_changed_hunk_t)); + hunk_count = cbm_parse_hunks(hbuf, hunks, HUNK_CAP); + /* A filled buffer means the diff was truncated: the hunks + * past the cap are gone, so files captured only partially + * would still look scoped and silently under-seed. Drop + * scoping for the whole request rather than under-report a + * large refactor — whole-file seeding is the safe side. */ + if (hunk_count >= HUNK_CAP) { + cbm_log_info("detect_changes.hunks", "action", "scoping_disabled", "reason", + "hunk_cap_reached"); + free(hunks); + hunks = NULL; + hunk_count = 0; + } + free(hbuf); + } + } + (void)fclose(hfp); + } + if (hunk_output_path[0]) { + (void)cbm_unlink(hunk_output_path); + } + } + char line[CBM_SZ_1K]; while (fgets(line, sizeof(line), fp)) { size_t len = strlen(line); @@ -10077,7 +10205,8 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { } files[file_count++] = heap_strdup(path_line); if (want_symbols) { - detect_collect_seeds(store, project, path_line, &seeds, &seed_count, &seed_cap); + detect_collect_seeds(store, project, path_line, hunks, hunk_count, &seeds, &seed_count, + &seed_cap); } } (void)fclose(fp); @@ -10123,6 +10252,7 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { } free(files); free(seeds); + free(hunks); free(direction); free(root_path); free(project); @@ -10280,6 +10410,7 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { } free(files); free(seeds); + free(hunks); free(direction); free(root_path); free(project); diff --git a/src/mcp/mcp_internal.h b/src/mcp/mcp_internal.h index 213159695..242ddf940 100644 --- a/src/mcp/mcp_internal.h +++ b/src/mcp/mcp_internal.h @@ -2,6 +2,8 @@ #define CBM_MCP_INTERNAL_H #include "mcp/mcp.h" +#include "pipeline/pipeline.h" /* cbm_changed_hunk_t */ +#include "store/store.h" /* cbm_node_t */ /* White-box fault injection for deterministic cross-platform quarantine * safety tests. This header is internal and is not part of the MCP API. */ @@ -31,4 +33,10 @@ enum { CBM_MCP_DEFAULT_AUTO_INDEX_LIMIT = 50000 }; bool cbm_mcp_auto_index_within_file_limit(const char *root_path, int file_limit, int *file_count_out); +/* detect_changes seed scoping (#1363): does `node`'s line range overlap any + * recorded hunk for `file`? Exposed for direct unit testing of the overlap + * logic, independent of the git/subprocess/index plumbing around it. */ +bool cbm_detect_node_in_hunks(const cbm_node_t *node, const cbm_changed_hunk_t *hunks, + int hunk_count, const char *file); + #endif diff --git a/src/pipeline/pipeline.h b/src/pipeline/pipeline.h index 78cfd37fd..d53436124 100644 --- a/src/pipeline/pipeline.h +++ b/src/pipeline/pipeline.h @@ -19,7 +19,8 @@ #include #include -#include "discover/discover.h" /* cbm_ignored_file_t (#963) */ +#include "discover/discover.h" /* cbm_ignored_file_t (#963) */ +#include "foundation/constants.h" /* CBM_SZ_512 */ /* Forward declarations */ typedef struct cbm_store cbm_store_t; @@ -290,4 +291,20 @@ cbm_fuzzy_result_t cbm_registry_fuzzy_resolve(const cbm_registry_t *r, const cha const char *cbm_confidence_band(double score); +/* ── Git diff hunks (pass_gitdiff.c) ────────────────────────────── + * Public (unlike the rest of pipeline_internal.h) because detect_changes + * (src/mcp/mcp.c) scopes seed detection to changed line ranges, not just + * changed files. */ + +typedef struct { + char path[CBM_SZ_512]; + int start_line; + int end_line; +} cbm_changed_hunk_t; + +/* Parse `git diff --unified=0` output into per-hunk (path, start_line, + * end_line) entries — end_line is the last new-side line the hunk touches. + * Returns count written to out (capped at max_out). */ +int cbm_parse_hunks(const char *output, cbm_changed_hunk_t *out, int max_out); + #endif /* CBM_PIPELINE_H */ diff --git a/src/pipeline/pipeline_internal.h b/src/pipeline/pipeline_internal.h index 9886d5718..22dba66b8 100644 --- a/src/pipeline/pipeline_internal.h +++ b/src/pipeline/pipeline_internal.h @@ -281,18 +281,13 @@ typedef struct { char old_path[CBM_SZ_512]; /* non-empty only for renames */ } cbm_changed_file_t; -typedef struct { - char path[CBM_SZ_512]; - int start_line; - int end_line; -} cbm_changed_hunk_t; +/* cbm_changed_hunk_t + cbm_parse_hunks moved to pipeline.h (public — consumed + * by src/mcp/mcp.c's detect_changes for line-scoped seed detection). Visible + * here via the `#include "pipeline/pipeline.h"` above. */ /* Parse git diff --name-status output. Returns count written to out. */ int cbm_parse_name_status(const char *output, cbm_changed_file_t *out, int max_out); -/* Parse git diff --unified=0 output. Returns count written to out. */ -int cbm_parse_hunks(const char *output, cbm_changed_hunk_t *out, int max_out); - /* Parse "start,count" or "start" → (start, count). */ void cbm_parse_range(const char *s, int *out_start, int *out_count); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index 18ab77127..d802a0bc8 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -6136,6 +6136,191 @@ TEST(tool_detect_changes_contained_commands_clean_up_error_and_success) { PASS(); } +/* Regression test for issue #1363: detect_changes seeded every definition in + * a changed file instead of just the ones whose line range overlaps the diff + * hunk. cbm_detect_node_in_hunks is the overlap primitive; this exercises it + * directly, independent of the git/subprocess/index plumbing around it. */ +TEST(detect_changes_node_in_hunks_overlap_issue1363) { + cbm_changed_hunk_t hunks[2] = { + {.path = "pkg/mod.py", .start_line = 10, .end_line = 12}, + {.path = "pkg/other.py", .start_line = 1, .end_line = 1}, + }; + + cbm_node_t inside = {.start_line = 8, .end_line = 15}; + ASSERT(cbm_detect_node_in_hunks(&inside, hunks, 2, "pkg/mod.py")); + + cbm_node_t exact = {.start_line = 10, .end_line = 12}; + ASSERT(cbm_detect_node_in_hunks(&exact, hunks, 2, "pkg/mod.py")); + + cbm_node_t touches_edge = {.start_line = 12, .end_line = 20}; + ASSERT(cbm_detect_node_in_hunks(&touches_edge, hunks, 2, "pkg/mod.py")); + + cbm_node_t before = {.start_line = 1, .end_line = 9}; + ASSERT(!cbm_detect_node_in_hunks(&before, hunks, 2, "pkg/mod.py")); + + cbm_node_t after = {.start_line = 13, .end_line = 20}; + ASSERT(!cbm_detect_node_in_hunks(&after, hunks, 2, "pkg/mod.py")); + + /* Same line range, different file — must not match. */ + cbm_node_t wrong_file = {.start_line = 10, .end_line = 12}; + ASSERT(!cbm_detect_node_in_hunks(&wrong_file, hunks, 2, "pkg/unrelated.py")); + + PASS(); +} + +/* End-to-end regression test for issue #1363: a same-line-count edit inside + * one function must seed only that function, not every definition in the + * file. A flat file with two independent top-level functions (no enclosing + * class) makes this unambiguous — before the fix, editing foo() also seeded + * bar() because seeding was scoped to the whole changed file. */ +TEST(detect_changes_seeds_only_touched_symbol_issue1363) { + char repo[512]; + snprintf(repo, sizeof(repo), "%s/cbm-detect-seed-scope-XXXXXX", cbm_tmpdir()); + if (!cbm_mkdtemp(repo)) { + FAIL("cbm_mkdtemp failed"); + } + + char src[600]; + snprintf(src, sizeof(src), "%s/mod.py", repo); + ASSERT_EQ(th_write_file(src, "def foo():\n" + " x = 1\n" + " return x\n" + "\n" + "\n" + "def bar():\n" + " y = 2\n" + " return y\n"), + 0); + + /* `git -C` with double quotes, not `cd '' &&`: single quotes are not + * quoting characters for cmd.exe, and identity/branch/signing come from -c + * so the fixture does not depend on the machine's global git config. The + * assertions below read `base: main`, so pin init.defaultBranch. */ +#define DC1363_GITCFG \ + "-c user.name=t -c user.email=t@t.io -c init.defaultBranch=main -c commit.gpgsign=false" + char cmd[1200]; + const char *steps[] = {"init -q", "add -A", "commit -q -m init"}; + for (size_t s = 0; s < sizeof(steps) / sizeof(steps[0]); s++) { + snprintf(cmd, sizeof(cmd), "git -C \"%s\" " DC1363_GITCFG " %s", repo, steps[s]); + if (system(cmd) != 0) { + th_rmtree(repo); + FAIL("git fixture setup failed"); + } + } +#undef DC1363_GITCFG + + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + char idx_args[700]; + snprintf(idx_args, sizeof(idx_args), "{\"repo_path\":\"%s\",\"mode\":\"full\"}", repo); + char *idx_resp = cbm_mcp_handle_tool(srv, "index_repository", idx_args); + ASSERT_NOT_NULL(idx_resp); + free(idx_resp); + + /* Same-line-count in-place edit inside foo() only; bar() is untouched. */ + ASSERT_EQ(th_write_file(src, "def foo():\n" + " x = 11\n" + " return x\n" + "\n" + "\n" + "def bar():\n" + " y = 2\n" + " return y\n"), + 0); + + char *project = cbm_project_name_from_path(repo); + ASSERT_NOT_NULL(project); + char dc_args[700]; + snprintf(dc_args, sizeof(dc_args), "{\"project\":\"%s\",\"depth\":1}", project); + char *dc_resp = cbm_mcp_handle_tool(srv, "detect_changes", dc_args); + ASSERT_NOT_NULL(dc_resp); + /* cbm_mcp_handle_tool wraps the tree text in a JSON string, so a literal + * newline in the source becomes the two-character `\n` escape sequence + * in dc_resp's actual bytes — match that, not a real newline. */ + ASSERT_NOT_NULL(strstr(dc_resp, "seed_symbols: 1\\n")); + ASSERT_NULL(strstr(dc_resp, "bar")); + + free(dc_resp); + free(project); + cbm_mcp_server_free(srv); + th_rmtree(repo); + PASS(); +} + +/* Recall guard for the zero-overlap case (#1363 review): an import-only edit + * changes lines that lie outside every definition's range. Scoping alone would + * drop the file from the seed set — worse recall than the whole-file behavior + * being replaced — so detect_collect_seeds falls back to whole-file seeding + * when a changed file has hunks but no definition overlapping any of them. */ +TEST(detect_changes_zero_overlap_falls_back_issue1363) { + char repo[512]; + snprintf(repo, sizeof(repo), "%s/cbm-detect-zero-overlap-XXXXXX", cbm_tmpdir()); + if (!cbm_mkdtemp(repo)) { + FAIL("cbm_mkdtemp failed"); + } + + char src[600]; + snprintf(src, sizeof(src), "%s/mod.py", repo); + /* Import on line 1 sits above both definitions. */ + ASSERT_EQ(th_write_file(src, "import os\n" + "\n" + "\n" + "def foo():\n" + " return 1\n" + "\n" + "\n" + "def bar():\n" + " return 2\n"), + 0); + +#define DC1363B_GITCFG \ + "-c user.name=t -c user.email=t@t.io -c init.defaultBranch=main -c commit.gpgsign=false" + char cmd[1200]; + const char *steps[] = {"init -q", "add -A", "commit -q -m init"}; + for (size_t s = 0; s < sizeof(steps) / sizeof(steps[0]); s++) { + snprintf(cmd, sizeof(cmd), "git -C \"%s\" " DC1363B_GITCFG " %s", repo, steps[s]); + if (system(cmd) != 0) { + th_rmtree(repo); + FAIL("git fixture setup failed"); + } + } +#undef DC1363B_GITCFG + + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + char idx_args[700]; + snprintf(idx_args, sizeof(idx_args), "{\"repo_path\":\"%s\",\"mode\":\"full\"}", repo); + char *idx_resp = cbm_mcp_handle_tool(srv, "index_repository", idx_args); + ASSERT_NOT_NULL(idx_resp); + free(idx_resp); + + /* Edit ONLY the import line — outside every definition's line range. */ + ASSERT_EQ(th_write_file(src, "import os, sys\n" + "\n" + "\n" + "def foo():\n" + " return 1\n" + "\n" + "\n" + "def bar():\n" + " return 2\n"), + 0); + + char *project = cbm_project_name_from_path(repo); + ASSERT_NOT_NULL(project); + char dc_args[700]; + snprintf(dc_args, sizeof(dc_args), "{\"project\":\"%s\",\"depth\":1}", project); + char *dc_resp = cbm_mcp_handle_tool(srv, "detect_changes", dc_args); + ASSERT_NOT_NULL(dc_resp); + /* Both definitions must survive: zero overlaps means no scoping for this + * file, not an empty seed set. */ + ASSERT_NOT_NULL(strstr(dc_resp, "seed_symbols: 2\\n")); + + free(dc_resp); + free(project); + cbm_mcp_server_free(srv); + th_rmtree(repo); + PASS(); +} + TEST(tool_ingest_traces_basic) { cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); @@ -9854,6 +10039,9 @@ SUITE(mcp) { RUN_TEST(tool_manage_adr_get_accepts_symlink_path); RUN_TEST(tool_detect_changes_not_found_rich_error); RUN_TEST(tool_detect_changes_contained_commands_clean_up_error_and_success); + RUN_TEST(detect_changes_node_in_hunks_overlap_issue1363); + RUN_TEST(detect_changes_seeds_only_touched_symbol_issue1363); + RUN_TEST(detect_changes_zero_overlap_falls_back_issue1363); RUN_TEST(tool_ingest_traces_basic); RUN_TEST(tool_ingest_traces_empty);