From 9235a23e61ef80d6b7702db8b3f81bb059e4d70b Mon Sep 17 00:00:00 2001 From: lishixiang Date: Fri, 31 Jul 2026 20:51:55 +0800 Subject: [PATCH] fix(mcp): close the two seed-scoping recall gaps from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three items from the #1372 review. The Windows shard failed in the fixture, not the fix: `system("cd '' && git init …")` used POSIX single quotes, which cmd.exe cannot parse. Switched to the portable `git -C ""` 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 --- src/mcp/mcp.c | 62 ++++++++++++++++++++++++++----- tests/test_mcp.c | 97 ++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 143 insertions(+), 16 deletions(-) diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 980296bdd..a7b524761 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -9762,26 +9762,49 @@ bool cbm_detect_node_in_hunks(const cbm_node_t *node, const cbm_changed_hunk_t * * "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, 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; + bool scope_to_hunks = false; for (int h = 0; h < hunk_count; h++) { if (strcmp(hunks[h].path, file) == 0) { - has_hunks_for_file = true; + 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 (has_hunks_for_file && - !cbm_detect_node_in_hunks(&nodes[i], hunks, hunk_count, file)) { + 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) { @@ -10075,7 +10098,16 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { * (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. */ + * 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) { @@ -10109,6 +10141,18 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { 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); } } diff --git a/tests/test_mcp.c b/tests/test_mcp.c index b132f2417..d802a0bc8 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -6192,15 +6192,22 @@ TEST(detect_changes_seeds_only_touched_symbol_issue1363) { " 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]; - 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"); + 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]; @@ -6239,6 +6246,81 @@ TEST(detect_changes_seeds_only_touched_symbol_issue1363) { 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); @@ -9959,6 +10041,7 @@ SUITE(mcp) { 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);