Skip to content

Commit 9235a23

Browse files
fix(mcp): close the two seed-scoping recall gaps from review
Three items from the #1372 review. The Windows shard failed in the fixture, not the fix: `system("cd '<dir>' && git init …")` used POSIX single quotes, which cmd.exe cannot parse. Switched to the portable `git -C "<dir>"` shape already used by wt_git in tests/test_watcher.c, with identity, default branch and signing passed via -c so the fixture no longer depends on the machine's global git config. A changed file with hunks but no overlapping definition now keeps whole-file seeding instead of dropping out. Import-only edits, module-level constants and anything above the first definition land outside every definition's line range, so scoping alone would have removed the file from the seed set entirely — strictly worse recall than the behavior being replaced. The overlap probe has to apply the same label filter as the seeding loop: a Module node spans lines 1..EOF, so probing raw nodes reports an overlap for every hunk and defeats the fallback. Extracting detect_is_seedable_label lets both sites share one definition. The new regression test caught exactly this — it failed with seed_symbols 0 until the probe was filtered. A filled hunk buffer is now treated as a truncated diff: hunks past HUNK_CAP are gone, so files captured only partially would still look scoped and silently under-seed. Scoping is dropped for the whole request and the decision logged, rather than under-reporting a large refactor. Documented the mixed coordinate systems in the fetch block: base...HEAD hunks are HEAD-side lines, worktree hunks are worktree-side, node lines come from the indexed snapshot. They agree while the index is fresh; a stale index plus earlier-in-file insertions can mis-scope, bounded by the zero-overlap fallback. Signed-off-by: lishixiang <lishixiang@gmail.com>
1 parent 2d94282 commit 9235a23

2 files changed

Lines changed: 143 additions & 16 deletions

File tree

src/mcp/mcp.c

Lines changed: 53 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9762,26 +9762,49 @@ bool cbm_detect_node_in_hunks(const cbm_node_t *node, const cbm_changed_hunk_t *
97629762
* "before" state, or the hunk fetch failed/was skipped), every non-container
97639763
* definition in the file is a seed — the previous, whole-file behavior — so
97649764
* this is a precision improvement, not a new failure mode. */
9765+
/* Structural container labels carry no CALLS edges and span the whole file, so
9766+
* they are never seeds. Shared by the seeding loop and the overlap probe. */
9767+
static bool detect_is_seedable_label(const char *lb) {
9768+
return lb && strcmp(lb, "File") != 0 && strcmp(lb, "Folder") != 0 &&
9769+
strcmp(lb, "Project") != 0 && strcmp(lb, "Module") != 0 && strcmp(lb, "Package") != 0 &&
9770+
strcmp(lb, "Section") != 0;
9771+
}
9772+
97659773
static void detect_collect_seeds(cbm_store_t *store, const char *project, const char *file,
97669774
const cbm_changed_hunk_t *hunks, int hunk_count, int64_t **seeds,
97679775
int *n, int *cap) {
97689776
cbm_node_t *nodes = NULL;
97699777
int ncount = 0;
97709778
cbm_store_find_nodes_by_file(store, project, file, &nodes, &ncount);
9771-
bool has_hunks_for_file = false;
9779+
bool scope_to_hunks = false;
97729780
for (int h = 0; h < hunk_count; h++) {
97739781
if (strcmp(hunks[h].path, file) == 0) {
9774-
has_hunks_for_file = true;
9782+
scope_to_hunks = true;
97759783
break;
97769784
}
97779785
}
9786+
/* A file can have hunks yet no SEEDABLE definition overlapping any of them:
9787+
* an import-only edit, a module-level constant, or a change above the first
9788+
* definition all land outside every definition's line range. Scoping would
9789+
* then drop the file from the seed set entirely — strictly worse recall
9790+
* than the whole-file behavior this replaces. Probe for an overlap first
9791+
* and keep whole-file seeding for that file when there is none.
9792+
*
9793+
* The probe must apply the same label filter as the seeding loop below:
9794+
* container nodes span the whole file (a Module node is lines 1..EOF), so
9795+
* counting them would report an overlap for every hunk and defeat the
9796+
* fallback entirely. */
9797+
if (scope_to_hunks) {
9798+
bool any_overlap = false;
9799+
for (int i = 0; i < ncount && !any_overlap; i++) {
9800+
any_overlap = detect_is_seedable_label(nodes[i].label) &&
9801+
cbm_detect_node_in_hunks(&nodes[i], hunks, hunk_count, file);
9802+
}
9803+
scope_to_hunks = any_overlap;
9804+
}
97789805
for (int i = 0; i < ncount; i++) {
9779-
const char *lb = nodes[i].label;
9780-
if (lb && strcmp(lb, "File") != 0 && strcmp(lb, "Folder") != 0 &&
9781-
strcmp(lb, "Project") != 0 && strcmp(lb, "Module") != 0 && strcmp(lb, "Package") != 0 &&
9782-
strcmp(lb, "Section") != 0) {
9783-
if (has_hunks_for_file &&
9784-
!cbm_detect_node_in_hunks(&nodes[i], hunks, hunk_count, file)) {
9806+
if (detect_is_seedable_label(nodes[i].label)) {
9807+
if (scope_to_hunks && !cbm_detect_node_in_hunks(&nodes[i], hunks, hunk_count, file)) {
97859808
continue;
97869809
}
97879810
if (*n >= *cap) {
@@ -10075,7 +10098,16 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
1007510098
* (see detect_collect_seeds). Best-effort: any failure here just leaves
1007610099
* `hunks` empty and every file falls back to its previous whole-file
1007710100
* seeding — this is a precision improvement, not a correctness
10078-
* dependency, so it is never treated as a request-level failure. */
10101+
* dependency, so it is never treated as a request-level failure.
10102+
*
10103+
* Coordinate systems: `base...HEAD` hunks carry HEAD-side line numbers,
10104+
* the worktree diff carries worktree-side ones, and node line ranges come
10105+
* from the indexed snapshot. These agree while the index is fresh — the
10106+
* watcher reindexes on HEAD movement and on a dirty tree — but a stale
10107+
* index combined with insertions earlier in the file shifts the node lines
10108+
* relative to the hunks and can mis-scope. The failure is bounded by
10109+
* detect_collect_seeds' zero-overlap fallback: a file whose definitions all
10110+
* miss reverts to whole-file seeding rather than dropping out. */
1007910111
cbm_changed_hunk_t *hunks = NULL;
1008010112
int hunk_count = 0;
1008110113
if (want_symbols) {
@@ -10109,6 +10141,18 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) {
1010910141
enum { HUNK_CAP = 4096 };
1011010142
hunks = safe_realloc(NULL, (size_t)HUNK_CAP * sizeof(cbm_changed_hunk_t));
1011110143
hunk_count = cbm_parse_hunks(hbuf, hunks, HUNK_CAP);
10144+
/* A filled buffer means the diff was truncated: the hunks
10145+
* past the cap are gone, so files captured only partially
10146+
* would still look scoped and silently under-seed. Drop
10147+
* scoping for the whole request rather than under-report a
10148+
* large refactor — whole-file seeding is the safe side. */
10149+
if (hunk_count >= HUNK_CAP) {
10150+
cbm_log_info("detect_changes.hunks", "action", "scoping_disabled", "reason",
10151+
"hunk_cap_reached");
10152+
free(hunks);
10153+
hunks = NULL;
10154+
hunk_count = 0;
10155+
}
1011210156
free(hbuf);
1011310157
}
1011410158
}

tests/test_mcp.c

Lines changed: 90 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6192,15 +6192,22 @@ TEST(detect_changes_seeds_only_touched_symbol_issue1363) {
61926192
" return y\n"),
61936193
0);
61946194

6195+
/* `git -C` with double quotes, not `cd '<dir>' &&`: single quotes are not
6196+
* quoting characters for cmd.exe, and identity/branch/signing come from -c
6197+
* so the fixture does not depend on the machine's global git config. The
6198+
* assertions below read `base: main`, so pin init.defaultBranch. */
6199+
#define DC1363_GITCFG \
6200+
"-c user.name=t -c user.email=t@t.io -c init.defaultBranch=main -c commit.gpgsign=false"
61956201
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");
6202+
const char *steps[] = {"init -q", "add -A", "commit -q -m init"};
6203+
for (size_t s = 0; s < sizeof(steps) / sizeof(steps[0]); s++) {
6204+
snprintf(cmd, sizeof(cmd), "git -C \"%s\" " DC1363_GITCFG " %s", repo, steps[s]);
6205+
if (system(cmd) != 0) {
6206+
th_rmtree(repo);
6207+
FAIL("git fixture setup failed");
6208+
}
62036209
}
6210+
#undef DC1363_GITCFG
62046211

62056212
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
62066213
char idx_args[700];
@@ -6239,6 +6246,81 @@ TEST(detect_changes_seeds_only_touched_symbol_issue1363) {
62396246
PASS();
62406247
}
62416248

6249+
/* Recall guard for the zero-overlap case (#1363 review): an import-only edit
6250+
* changes lines that lie outside every definition's range. Scoping alone would
6251+
* drop the file from the seed set — worse recall than the whole-file behavior
6252+
* being replaced — so detect_collect_seeds falls back to whole-file seeding
6253+
* when a changed file has hunks but no definition overlapping any of them. */
6254+
TEST(detect_changes_zero_overlap_falls_back_issue1363) {
6255+
char repo[512];
6256+
snprintf(repo, sizeof(repo), "%s/cbm-detect-zero-overlap-XXXXXX", cbm_tmpdir());
6257+
if (!cbm_mkdtemp(repo)) {
6258+
FAIL("cbm_mkdtemp failed");
6259+
}
6260+
6261+
char src[600];
6262+
snprintf(src, sizeof(src), "%s/mod.py", repo);
6263+
/* Import on line 1 sits above both definitions. */
6264+
ASSERT_EQ(th_write_file(src, "import os\n"
6265+
"\n"
6266+
"\n"
6267+
"def foo():\n"
6268+
" return 1\n"
6269+
"\n"
6270+
"\n"
6271+
"def bar():\n"
6272+
" return 2\n"),
6273+
0);
6274+
6275+
#define DC1363B_GITCFG \
6276+
"-c user.name=t -c user.email=t@t.io -c init.defaultBranch=main -c commit.gpgsign=false"
6277+
char cmd[1200];
6278+
const char *steps[] = {"init -q", "add -A", "commit -q -m init"};
6279+
for (size_t s = 0; s < sizeof(steps) / sizeof(steps[0]); s++) {
6280+
snprintf(cmd, sizeof(cmd), "git -C \"%s\" " DC1363B_GITCFG " %s", repo, steps[s]);
6281+
if (system(cmd) != 0) {
6282+
th_rmtree(repo);
6283+
FAIL("git fixture setup failed");
6284+
}
6285+
}
6286+
#undef DC1363B_GITCFG
6287+
6288+
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
6289+
char idx_args[700];
6290+
snprintf(idx_args, sizeof(idx_args), "{\"repo_path\":\"%s\",\"mode\":\"full\"}", repo);
6291+
char *idx_resp = cbm_mcp_handle_tool(srv, "index_repository", idx_args);
6292+
ASSERT_NOT_NULL(idx_resp);
6293+
free(idx_resp);
6294+
6295+
/* Edit ONLY the import line — outside every definition's line range. */
6296+
ASSERT_EQ(th_write_file(src, "import os, sys\n"
6297+
"\n"
6298+
"\n"
6299+
"def foo():\n"
6300+
" return 1\n"
6301+
"\n"
6302+
"\n"
6303+
"def bar():\n"
6304+
" return 2\n"),
6305+
0);
6306+
6307+
char *project = cbm_project_name_from_path(repo);
6308+
ASSERT_NOT_NULL(project);
6309+
char dc_args[700];
6310+
snprintf(dc_args, sizeof(dc_args), "{\"project\":\"%s\",\"depth\":1}", project);
6311+
char *dc_resp = cbm_mcp_handle_tool(srv, "detect_changes", dc_args);
6312+
ASSERT_NOT_NULL(dc_resp);
6313+
/* Both definitions must survive: zero overlaps means no scoping for this
6314+
* file, not an empty seed set. */
6315+
ASSERT_NOT_NULL(strstr(dc_resp, "seed_symbols: 2\\n"));
6316+
6317+
free(dc_resp);
6318+
free(project);
6319+
cbm_mcp_server_free(srv);
6320+
th_rmtree(repo);
6321+
PASS();
6322+
}
6323+
62426324
TEST(tool_ingest_traces_basic) {
62436325
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
62446326

@@ -9959,6 +10041,7 @@ SUITE(mcp) {
995910041
RUN_TEST(tool_detect_changes_contained_commands_clean_up_error_and_success);
996010042
RUN_TEST(detect_changes_node_in_hunks_overlap_issue1363);
996110043
RUN_TEST(detect_changes_seeds_only_touched_symbol_issue1363);
10044+
RUN_TEST(detect_changes_zero_overlap_falls_back_issue1363);
996210045
RUN_TEST(tool_ingest_traces_basic);
996310046
RUN_TEST(tool_ingest_traces_empty);
996410047

0 commit comments

Comments
 (0)