mirror of
https://github.com/DeusData/codebase-memory-mcp.git
synced 2026-11-10 09:13:53 +08:00
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>
This commit is contained in:
+92
-5
@@ -9736,19 +9736,54 @@ 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. */
|
||||
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 has_hunks_for_file = false;
|
||||
for (int h = 0; h < hunk_count; h++) {
|
||||
if (strcmp(hunks[h].path, file) == 0) {
|
||||
has_hunks_for_file = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
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 (has_hunks_for_file &&
|
||||
!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 +10070,55 @@ 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. */
|
||||
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);
|
||||
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 +10161,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 +10208,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 +10366,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);
|
||||
|
||||
@@ -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
|
||||
|
||||
+18
-1
@@ -19,7 +19,8 @@
|
||||
#include <stdint.h>
|
||||
#include <stdatomic.h>
|
||||
|
||||
#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 */
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -6136,6 +6136,109 @@ 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);
|
||||
|
||||
char cmd[1200];
|
||||
snprintf(cmd, sizeof(cmd),
|
||||
"cd '%s' && git init -q && git config user.email t@t.com && "
|
||||
"git config user.name t && git add -A && git commit -q -m init",
|
||||
repo);
|
||||
if (system(cmd) != 0) {
|
||||
th_rmtree(repo);
|
||||
FAIL("git fixture setup failed");
|
||||
}
|
||||
|
||||
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();
|
||||
}
|
||||
|
||||
TEST(tool_ingest_traces_basic) {
|
||||
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
|
||||
|
||||
@@ -9854,6 +9957,8 @@ 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(tool_ingest_traces_basic);
|
||||
RUN_TEST(tool_ingest_traces_empty);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user