fix(mcp): scope detect_changes seed detection to changed line ranges

[lishixiang0705]

What does this PR do?

Fixes #1363: detect_changes scoped its BFS seed set to every definition
in a changed file, 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.

cbm_parse_hunks (in pass_gitdiff.c) already existed to parse
git diff --unified=0 hunks but had no caller. This PR fetches those hunks
alongside the existing changed-file list and scopes 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.

cbm_changed_hunk_t / cbm_parse_hunks move from pipeline_internal.h
(explicitly not a public header) to pipeline.h, since detect_changes
(in src/mcp/mcp.c) now needs them. 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.

Verified against v0.9.1-rc.1 — the RC's tree-format rewrite (this
release's "real detect_changes impact reporting") is a different, earlier
part of this same path: it made the BFS traversal itself real instead of a
placeholder, but the seed set feeding that traversal was still whole-file.
I confirmed the bug still reproduces on the v0.9.1-rc.1 release binary
before opening this PR:

$ codebase-memory-mcp cli --json detect_changes '{"project":"...","depth":1}'
...
seed_symbols: 2   # foo() and bar(), only foo() was touched

Also 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.

This branch is rebased onto current main (on top of v0.9.1-rc.1 plus the
subsequent commits); local scripts/test.sh SANITIZE= and
scripts/lint.sh --ci both pass clean.

Checklist

• Every commit is signed off (git commit -s) — required, CI rejects
  unsigned commits (DCO, see CONTRIBUTING.md)
• Tests pass locally (make -f Makefile.cbm test)
• Lint passes (make -f Makefile.cbm lint-ci)
• New behavior is covered by a test (reproduce-first for bug fixes)

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[DeusData]

Reviewed in full, and this is a good find. The bug is real and I verified it on main: cbm_parse_hunks sits in src/pipeline/pass_gitdiff.c:154 with zero production callers — the only references outside its own declaration are in tests/test_pipeline.c. Meanwhile detect_collect_seeds seeds every non-container node in a changed file. Dormant machinery plus an over-approximating caller is exactly the kind of archaeology that is worth doing, and your reproduce-first e2e test (seed_symbols 2 → 1) plus the v0.9.1-rc.1 and qlib cross-checks made verifying it straightforward. Thank you.

On the direction: narrowing to changed line ranges is noise removal rather than a findability cut, which is the important question for us — the seeds you drop are definitions in a changed file whose lines did not change. Deletion safety is handled correctly too: parse_hunk_line rejects start <= 0, so a deleted file emits no hunks and falls back to whole-file seeding.

One CI red is genuinely yours, and it is a small fix. The Windows shard fails at tests/test_mcp.c:6202: git fixture setup failed — "The filename, directory name, or volume label syntax is incorrect." The fixture uses system("cd '%s' && git init …") with POSIX single quotes, which cmd.exe cannot parse. The repo already has the portable pattern: wt_git() in tests/test_watcher.c:34 (used at :1183), which shells git -C "<dir>" …. Switch to that shape and the shard should go green. Everything else — 27 checks including both TSan trios and the other Windows shard — passes.

Two recall edge cases I would like closed before merge, both inside the shape you already have:

1. A file with hunks but zero overlapping definitions now contributes no seeds. Import-only or other module-level edits land outside every definition's line range, so that file drops out entirely where main seeded all of its definitions. Suggest: when the overlap count for a file is 0, fall back to whole-file seeding for that file.
2. HUNK_CAP = 4096 truncates silently. On a large refactor the diff can exceed the cap, and files whose hunks fell past it under-seed while has_hunks_for_file may still read true for partially-captured files. Suggest: if cbm_parse_hunks returns exactly HUNK_CAP, disable scoping for the whole request and seed as before. At this repo's scale a >4096-hunk diff is not hypothetical.

And one worth a comment rather than a fix: the coordinate systems are mixed — base…HEAD hunks are HEAD-side lines, worktree hunks are worktree-side, and node lines come from the indexed snapshot. That is correct for a freshly refreshed index, but a stale index plus earlier-in-file insertions will mis-scope. A note in the code (or preferring the worktree diff when it is non-empty) would cover it.

Security review is clean: no new dependencies, no CI changes, cbm_fopen/cbm_unlink used correctly, and the new hunk-fetch command interpolates only root_path and base_branch — both already validated upstream and quoted identically to the existing name-only command at mcp.c:9830. No new injection surface.

Ping me when the fixture and the two edge cases are in and I will pick it straight back up.

[lishixiang0705]

Thanks — the two edge cases were both real, and the first one was sharper than I
realised. All three items are in 9235a23e.

Windows fixture. Switched to the wt_git shape you pointed at: git -C "<dir>" with identity, init.defaultBranch and commit.gpgsign passed via
-c, so the fixture no longer reads the machine's global git config either.

Zero-overlap fallback — and a bug in my first attempt at it. Implemented as
suggested: probe the file for an overlapping definition, and keep whole-file
seeding when there is none. The first version still returned seed_symbols: 0
for an import-only edit, because the probe walked raw nodes while the seeding
loop applies a label filter. A Module node spans lines 1..EOF, so it overlaps
every hunk — the probe always reported an overlap and the fallback could never
fire. Extracted detect_is_seedable_label() so the probe and the seeding loop
share one definition of "seedable".

Worth recording that the regression test is what caught it: it asserted
seed_symbols: 2 for a one-line import edit above two functions and failed at 0
until the probe was filtered. Without the test I would have shipped a fallback
that was structurally dead.

HUNK_CAP truncation. hunk_count >= HUNK_CAP now drops scoping for the
whole request and logs detect_changes.hunks action=scoping_disabled reason=hunk_cap_reached, rather than letting partially-captured files look
scoped and under-seed.

Coordinate systems. Documented in the fetch block rather than changing
behavior, per your suggestion — HEAD-side vs worktree-side vs indexed-snapshot
lines, when they agree, and that the failure mode is bounded by the zero-overlap
fallback (a file whose definitions all miss reverts to whole-file seeding rather
than dropping out). Happy to switch to preferring the worktree diff when it is
non-empty if you would rather have the behavioral fix.

Local: 6792 passed / 0 failed, scripts/lint.sh --ci clean.

[DeusData]

Merged as 27893016b. All three items verified, 29/29 green.

The bug you caught in your own fix is the part worth dwelling on. I confirmed it independently: extract_defs.c:6817 gives every file a Module node spanning start_line = FIRST_LINE to the end of the root node — lines 1..EOF. An unfiltered probe therefore overlaps every hunk in every file, so the zero-overlap fallback would have been structurally dead code and import-only edits would still have seeded nothing. Your description of it was accurate rather than post-hoc narrative, and finding it required noticing that the probe and the seeding loop disagreed about what counts as a node — which is exactly the kind of mismatch that survives review.

The fix is also structurally right, not just correct: the probe and the seeding loop now call the same detect_is_seedable_label() and the same cbm_detect_node_in_hunks(). There is no second copy of the condition that can drift apart later. That is what stops this bug recurring, and it is why I am not asking for anything else here.

The other two hold up too. The Windows fixture passing identity, init.defaultBranch and commit.gpgsign via -c means it no longer reads the developer's global git config — a fixture that depends on that is a latent flake, so thank you for going past what was asked. Both Windows shards are green, which is the venue that caught the original problem. And the HUNK_CAP guard drops scoping for the whole request and logs the decision, which is the right side to fail on: legitimate-zero-hunks and cap-truncated must behave identically, so collapsing them to hunk_count = 0 with a log line for observability is the correct encoding.

Two small things for whenever you are next in this code — neither blocking, neither worth a PR on its own:

A comment overstates a bound. mcp.c:10108-10110 says the mis-scoping failure "is bounded by detect_collect_seeds' zero-overlap fallback". That holds only for a total miss. If a stale index leaves some definitions overlapping the shifted hunks, scoping stays on and the wrong definitions get seeded — the fallback does not bound that case. Worth a clause so a future reader does not over-trust it.

A rationale that does not match. test_mcp.c:6198 says "the assertions below read base: main, so pin init.defaultBranch". There is no such assertion in either test. Pinning the branch is still right — I am only flagging the stated reason.

One acknowledged gap: the HUNK_CAP guard has no binding test, because the cap is a function-local enum and nothing can produce 4096 hunks in a fixture. It fails safe in both directions, so I did not hold the PR on it.

Thank you — this was a good find made better by a careful second pass on your own work.