Skip to content

Commit 2d94282

Browse files
fix(mcp): scope detect_changes seed detection to changed line ranges
detect_collect_seeds treated every definition in a changed file as a BFS seed, regardless of which lines actually changed. A one-line edit inside a single method seeded every other definition in the file too, producing an impact report an order of magnitude larger than what the edit actually touched. Fetch `git diff --unified=0` hunks (cbm_parse_hunks already existed in pass_gitdiff.c but had no caller) alongside the existing file list, and scope seeds to definitions whose line range overlaps a hunk. Any failure fetching hunks (new/untracked files, a transient git error) falls back to the previous whole-file behavior, so this is a precision improvement with no new failure mode. Verified on microsoft/qlib (an unrelated third-party repo): a same- line-count edit inside one method now seeds 2 symbols (the method + its containing class) instead of 25 (every definition in the 24-def file), matching the equivalent output of a different code-graph tool (GitNexus) on the identical scenario. Reproduced on a second file with a different definition count (42 defs -> seeds 43 before the fix, 2 after) to rule out coincidence. cbm_changed_hunk_t / cbm_parse_hunks move from pipeline_internal.h (explicitly not a public header) to pipeline.h, since detect_changes now needs them from src/mcp/mcp.c. cbm_detect_node_in_hunks is exposed non-static via mcp_internal.h, matching this file's existing white-box test-hook pattern, so the overlap logic has a direct unit test independent of the git/subprocess/index plumbing around it. Fixes #1363 Signed-off-by: lishixiang <lishixiang@gmail.com>
1 parent 08f6d1d commit 2d94282

5 files changed

Lines changed: 226 additions & 14 deletions

File tree

src/mcp/mcp.c

Lines changed: 92 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9736,19 +9736,54 @@ static int mcp_run_shell_command_cancellable(cbm_mcp_server_t *srv, const char *
97369736
return contained ? 0 : -1;
97379737
}
97389738

9739-
/* Collect BFS seed ids: every symbol DEFINED in a changed file (everything but
9740-
* the structural container labels — those have no CALLS edges). These anchor
9741-
* the multi-source impact traversal. */
9739+
/* Does `node`'s line range overlap any recorded hunk for `file`? Used to scope
9740+
* seed detection to the actually-changed lines rather than the whole file.
9741+
* Non-static (declared in mcp_internal.h) so tests can exercise the overlap
9742+
* logic directly, matching this file's existing white-box test hooks. */
9743+
bool cbm_detect_node_in_hunks(const cbm_node_t *node, const cbm_changed_hunk_t *hunks,
9744+
int hunk_count, const char *file) {
9745+
for (int h = 0; h < hunk_count; h++) {
9746+
if (strcmp(hunks[h].path, file) == 0 && node->start_line <= hunks[h].end_line &&
9747+
node->end_line >= hunks[h].start_line) {
9748+
return true;
9749+
}
9750+
}
9751+
return false;
9752+
}
9753+
9754+
/* Collect BFS seed ids for one changed file (everything but the structural
9755+
* container labels — those have no CALLS edges). These anchor the
9756+
* multi-source impact traversal.
9757+
*
9758+
* When `hunks` has at least one entry for `file`, only definitions whose line
9759+
* range overlaps a hunk become seeds — a one-line edit inside a single
9760+
* function no longer seeds every other definition in the file. When no hunk
9761+
* is recorded for `file` (a brand-new/untracked file has no comparable
9762+
* "before" state, or the hunk fetch failed/was skipped), every non-container
9763+
* definition in the file is a seed — the previous, whole-file behavior — so
9764+
* this is a precision improvement, not a new failure mode. */
97429765
static void detect_collect_seeds(cbm_store_t *store, const char *project, const char *file,
9743-
int64_t **seeds, int *n, int *cap) {
9766+
const cbm_changed_hunk_t *hunks, int hunk_count, int64_t **seeds,
9767+
int *n, int *cap) {
97449768
cbm_node_t *nodes = NULL;
97459769
int ncount = 0;
97469770
cbm_store_find_nodes_by_file(store, project, file, &nodes, &ncount);
9771+
bool has_hunks_for_file = false;
9772+
for (int h = 0; h < hunk_count; h++) {
9773+
if (strcmp(hunks[h].path, file) == 0) {
9774+
has_hunks_for_file = true;
9775+
break;
9776+
}
9777+
}
97479778
for (int i = 0; i < ncount; i++) {
97489779
const char *lb = nodes[i].label;
97499780
if (lb && strcmp(lb, "File") != 0 && strcmp(lb, "Folder") != 0 &&
97509781
strcmp(lb, "Project") != 0 && strcmp(lb, "Module") != 0 && strcmp(lb, "Package") != 0 &&
97519782
strcmp(lb, "Section") != 0) {
9783+
if (has_hunks_for_file &&
9784+
!cbm_detect_node_in_hunks(&nodes[i], hunks, hunk_count, file)) {
9785+
continue;
9786+
}
97529787
if (*n >= *cap) {
97539788
*cap = *cap ? *cap * 2 : 16;
97549789
*seeds = safe_realloc(*seeds, (size_t)*cap * sizeof(int64_t));
@@ -10035,6 +10070,55 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
1003510070
int seed_count = 0;
1003610071
int seed_cap = 0;
1003710072

10073+
/* Hunk line ranges (unified=0 diff), used to scope seed detection to the
10074+
* actually-changed lines instead of every definition in a changed file
10075+
* (see detect_collect_seeds). Best-effort: any failure here just leaves
10076+
* `hunks` empty and every file falls back to its previous whole-file
10077+
* seeding — this is a precision improvement, not a correctness
10078+
* dependency, so it is never treated as a request-level failure. */
10079+
cbm_changed_hunk_t *hunks = NULL;
10080+
int hunk_count = 0;
10081+
if (want_symbols) {
10082+
char hunk_cmd[CBM_SZ_2K];
10083+
#ifdef _WIN32
10084+
snprintf(hunk_cmd, sizeof(hunk_cmd),
10085+
"git -C \"%s\" diff --unified=0 \"%s\"...HEAD 2>NUL & "
10086+
"git -C \"%s\" diff --unified=0 2>NUL",
10087+
root_path, base_branch, root_path);
10088+
#else
10089+
snprintf(hunk_cmd, sizeof(hunk_cmd),
10090+
"{ git -C '%s' diff --unified=0 '%s'...HEAD 2>/dev/null; "
10091+
"git -C '%s' diff --unified=0 2>/dev/null; }",
10092+
root_path, base_branch, root_path);
10093+
#endif
10094+
char hunk_output_path[CBM_SZ_2K] = {0};
10095+
cbm_proc_result_t hunk_result = {0};
10096+
int hunk_run =
10097+
mcp_run_shell_command_cancellable(srv, hunk_cmd, hunk_output_path, &hunk_result);
10098+
bool hunk_cancelled = hunk_result.cancellation_requested || mcp_request_cancelled(srv);
10099+
FILE *hfp = (!hunk_cancelled && hunk_run == 0) ? cbm_fopen(hunk_output_path, "rb") : NULL;
10100+
if (hfp) {
10101+
(void)fseek(hfp, 0, SEEK_END);
10102+
long hsz = ftell(hfp);
10103+
if (hsz > 0) {
10104+
(void)fseek(hfp, 0, SEEK_SET);
10105+
char *hbuf = malloc((size_t)hsz + SKIP_ONE);
10106+
if (hbuf) {
10107+
size_t hread = fread(hbuf, SKIP_ONE, (size_t)hsz, hfp);
10108+
hbuf[hread] = '\0';
10109+
enum { HUNK_CAP = 4096 };
10110+
hunks = safe_realloc(NULL, (size_t)HUNK_CAP * sizeof(cbm_changed_hunk_t));
10111+
hunk_count = cbm_parse_hunks(hbuf, hunks, HUNK_CAP);
10112+
free(hbuf);
10113+
}
10114+
}
10115+
(void)fclose(hfp);
10116+
}
10117+
if (hunk_output_path[0]) {
10118+
(void)cbm_unlink(hunk_output_path);
10119+
}
10120+
}
10121+
1003810122
char line[CBM_SZ_1K];
1003910123
while (fgets(line, sizeof(line), fp)) {
1004010124
size_t len = strlen(line);
@@ -10077,7 +10161,8 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
1007710161
}
1007810162
files[file_count++] = heap_strdup(path_line);
1007910163
if (want_symbols) {
10080-
detect_collect_seeds(store, project, path_line, &seeds, &seed_count, &seed_cap);
10164+
detect_collect_seeds(store, project, path_line, hunks, hunk_count, &seeds, &seed_count,
10165+
&seed_cap);
1008110166
}
1008210167
}
1008310168
(void)fclose(fp);
@@ -10123,6 +10208,7 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
1012310208
}
1012410209
free(files);
1012510210
free(seeds);
10211+
free(hunks);
1012610212
free(direction);
1012710213
free(root_path);
1012810214
free(project);
@@ -10280,6 +10366,7 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
1028010366
}
1028110367
free(files);
1028210368
free(seeds);
10369+
free(hunks);
1028310370
free(direction);
1028410371
free(root_path);
1028510372
free(project);

src/mcp/mcp_internal.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
#define CBM_MCP_INTERNAL_H
33

44
#include "mcp/mcp.h"
5+
#include "pipeline/pipeline.h" /* cbm_changed_hunk_t */
6+
#include "store/store.h" /* cbm_node_t */
57

68
/* White-box fault injection for deterministic cross-platform quarantine
79
* 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 };
3133
bool cbm_mcp_auto_index_within_file_limit(const char *root_path, int file_limit,
3234
int *file_count_out);
3335

36+
/* detect_changes seed scoping (#1363): does `node`'s line range overlap any
37+
* recorded hunk for `file`? Exposed for direct unit testing of the overlap
38+
* logic, independent of the git/subprocess/index plumbing around it. */
39+
bool cbm_detect_node_in_hunks(const cbm_node_t *node, const cbm_changed_hunk_t *hunks,
40+
int hunk_count, const char *file);
41+
3442
#endif

src/pipeline/pipeline.h

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919
#include <stdint.h>
2020
#include <stdatomic.h>
2121

22-
#include "discover/discover.h" /* cbm_ignored_file_t (#963) */
22+
#include "discover/discover.h" /* cbm_ignored_file_t (#963) */
23+
#include "foundation/constants.h" /* CBM_SZ_512 */
2324

2425
/* Forward declarations */
2526
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
290291

291292
const char *cbm_confidence_band(double score);
292293

294+
/* ── Git diff hunks (pass_gitdiff.c) ──────────────────────────────
295+
* Public (unlike the rest of pipeline_internal.h) because detect_changes
296+
* (src/mcp/mcp.c) scopes seed detection to changed line ranges, not just
297+
* changed files. */
298+
299+
typedef struct {
300+
char path[CBM_SZ_512];
301+
int start_line;
302+
int end_line;
303+
} cbm_changed_hunk_t;
304+
305+
/* Parse `git diff --unified=0` output into per-hunk (path, start_line,
306+
* end_line) entries — end_line is the last new-side line the hunk touches.
307+
* Returns count written to out (capped at max_out). */
308+
int cbm_parse_hunks(const char *output, cbm_changed_hunk_t *out, int max_out);
309+
293310
#endif /* CBM_PIPELINE_H */

src/pipeline/pipeline_internal.h

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -281,18 +281,13 @@ typedef struct {
281281
char old_path[CBM_SZ_512]; /* non-empty only for renames */
282282
} cbm_changed_file_t;
283283

284-
typedef struct {
285-
char path[CBM_SZ_512];
286-
int start_line;
287-
int end_line;
288-
} cbm_changed_hunk_t;
284+
/* cbm_changed_hunk_t + cbm_parse_hunks moved to pipeline.h (public — consumed
285+
* by src/mcp/mcp.c's detect_changes for line-scoped seed detection). Visible
286+
* here via the `#include "pipeline/pipeline.h"` above. */
289287

290288
/* Parse git diff --name-status output. Returns count written to out. */
291289
int cbm_parse_name_status(const char *output, cbm_changed_file_t *out, int max_out);
292290

293-
/* Parse git diff --unified=0 output. Returns count written to out. */
294-
int cbm_parse_hunks(const char *output, cbm_changed_hunk_t *out, int max_out);
295-
296291
/* Parse "start,count" or "start" → (start, count). */
297292
void cbm_parse_range(const char *s, int *out_start, int *out_count);
298293

tests/test_mcp.c

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6136,6 +6136,109 @@ TEST(tool_detect_changes_contained_commands_clean_up_error_and_success) {
61366136
PASS();
61376137
}
61386138

6139+
/* Regression test for issue #1363: detect_changes seeded every definition in
6140+
* a changed file instead of just the ones whose line range overlaps the diff
6141+
* hunk. cbm_detect_node_in_hunks is the overlap primitive; this exercises it
6142+
* directly, independent of the git/subprocess/index plumbing around it. */
6143+
TEST(detect_changes_node_in_hunks_overlap_issue1363) {
6144+
cbm_changed_hunk_t hunks[2] = {
6145+
{.path = "pkg/mod.py", .start_line = 10, .end_line = 12},
6146+
{.path = "pkg/other.py", .start_line = 1, .end_line = 1},
6147+
};
6148+
6149+
cbm_node_t inside = {.start_line = 8, .end_line = 15};
6150+
ASSERT(cbm_detect_node_in_hunks(&inside, hunks, 2, "pkg/mod.py"));
6151+
6152+
cbm_node_t exact = {.start_line = 10, .end_line = 12};
6153+
ASSERT(cbm_detect_node_in_hunks(&exact, hunks, 2, "pkg/mod.py"));
6154+
6155+
cbm_node_t touches_edge = {.start_line = 12, .end_line = 20};
6156+
ASSERT(cbm_detect_node_in_hunks(&touches_edge, hunks, 2, "pkg/mod.py"));
6157+
6158+
cbm_node_t before = {.start_line = 1, .end_line = 9};
6159+
ASSERT(!cbm_detect_node_in_hunks(&before, hunks, 2, "pkg/mod.py"));
6160+
6161+
cbm_node_t after = {.start_line = 13, .end_line = 20};
6162+
ASSERT(!cbm_detect_node_in_hunks(&after, hunks, 2, "pkg/mod.py"));
6163+
6164+
/* Same line range, different file — must not match. */
6165+
cbm_node_t wrong_file = {.start_line = 10, .end_line = 12};
6166+
ASSERT(!cbm_detect_node_in_hunks(&wrong_file, hunks, 2, "pkg/unrelated.py"));
6167+
6168+
PASS();
6169+
}
6170+
6171+
/* End-to-end regression test for issue #1363: a same-line-count edit inside
6172+
* one function must seed only that function, not every definition in the
6173+
* file. A flat file with two independent top-level functions (no enclosing
6174+
* class) makes this unambiguous — before the fix, editing foo() also seeded
6175+
* bar() because seeding was scoped to the whole changed file. */
6176+
TEST(detect_changes_seeds_only_touched_symbol_issue1363) {
6177+
char repo[512];
6178+
snprintf(repo, sizeof(repo), "%s/cbm-detect-seed-scope-XXXXXX", cbm_tmpdir());
6179+
if (!cbm_mkdtemp(repo)) {
6180+
FAIL("cbm_mkdtemp failed");
6181+
}
6182+
6183+
char src[600];
6184+
snprintf(src, sizeof(src), "%s/mod.py", repo);
6185+
ASSERT_EQ(th_write_file(src, "def foo():\n"
6186+
" x = 1\n"
6187+
" return x\n"
6188+
"\n"
6189+
"\n"
6190+
"def bar():\n"
6191+
" y = 2\n"
6192+
" return y\n"),
6193+
0);
6194+
6195+
char cmd[1200];
6196+
snprintf(cmd, sizeof(cmd),
6197+
"cd '%s' && git init -q && git config user.email t@t.com && "
6198+
"git config user.name t && git add -A && git commit -q -m init",
6199+
repo);
6200+
if (system(cmd) != 0) {
6201+
th_rmtree(repo);
6202+
FAIL("git fixture setup failed");
6203+
}
6204+
6205+
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
6206+
char idx_args[700];
6207+
snprintf(idx_args, sizeof(idx_args), "{\"repo_path\":\"%s\",\"mode\":\"full\"}", repo);
6208+
char *idx_resp = cbm_mcp_handle_tool(srv, "index_repository", idx_args);
6209+
ASSERT_NOT_NULL(idx_resp);
6210+
free(idx_resp);
6211+
6212+
/* Same-line-count in-place edit inside foo() only; bar() is untouched. */
6213+
ASSERT_EQ(th_write_file(src, "def foo():\n"
6214+
" x = 11\n"
6215+
" return x\n"
6216+
"\n"
6217+
"\n"
6218+
"def bar():\n"
6219+
" y = 2\n"
6220+
" return y\n"),
6221+
0);
6222+
6223+
char *project = cbm_project_name_from_path(repo);
6224+
ASSERT_NOT_NULL(project);
6225+
char dc_args[700];
6226+
snprintf(dc_args, sizeof(dc_args), "{\"project\":\"%s\",\"depth\":1}", project);
6227+
char *dc_resp = cbm_mcp_handle_tool(srv, "detect_changes", dc_args);
6228+
ASSERT_NOT_NULL(dc_resp);
6229+
/* cbm_mcp_handle_tool wraps the tree text in a JSON string, so a literal
6230+
* newline in the source becomes the two-character `\n` escape sequence
6231+
* in dc_resp's actual bytes — match that, not a real newline. */
6232+
ASSERT_NOT_NULL(strstr(dc_resp, "seed_symbols: 1\\n"));
6233+
ASSERT_NULL(strstr(dc_resp, "bar"));
6234+
6235+
free(dc_resp);
6236+
free(project);
6237+
cbm_mcp_server_free(srv);
6238+
th_rmtree(repo);
6239+
PASS();
6240+
}
6241+
61396242
TEST(tool_ingest_traces_basic) {
61406243
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
61416244

@@ -9854,6 +9957,8 @@ SUITE(mcp) {
98549957
RUN_TEST(tool_manage_adr_get_accepts_symlink_path);
98559958
RUN_TEST(tool_detect_changes_not_found_rich_error);
98569959
RUN_TEST(tool_detect_changes_contained_commands_clean_up_error_and_success);
9960+
RUN_TEST(detect_changes_node_in_hunks_overlap_issue1363);
9961+
RUN_TEST(detect_changes_seeds_only_touched_symbol_issue1363);
98579962
RUN_TEST(tool_ingest_traces_basic);
98589963
RUN_TEST(tool_ingest_traces_empty);
98599964

0 commit comments

Comments
 (0)