fix(mcp): flag source drift in get_code_snippet and search_code (#1750) - #1788

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.

Thank you for identifying a correctness problem that can make stale coordinates look authoritative, and for covering both snippet and search paths. I checked current main: get_code_snippet and search_code still consume stored start and end lines for live-file slices without first applying the existing freshness result. That grounds the source-drift failure described here.

There is one mechanical scope blocker before code review. This PR adds 196 lines across seven new openspec/ files, including project configuration, while current main has no openspec/ tree and CONTRIBUTING.md requires prior approval for project configuration. Please remove the entire openspec/ portion and keep the focused MCP change plus tests. I have labeled the underlying problem as a high-priority parsing and correctness bug. Splitting enables review but does not pre-decide the merge outcome. Thank you for the clear reproduction and fail-closed response shape.

The fix is good and I want it. Two things before it can land — one of them is a merge-order hazard you may not have noticed, and it is the more important of the two.

Blocker: this must not merge before #1787

Your D1 makes coverage_path_freshness the single drift oracle:

freshness == "metadata_changed" || "missing" ⇒ drifted → skip the live read

And your own #1787 exists because that oracle returns metadata_changed for every file on Windows, always — coverage_stat_mtime_ns truncates to seconds under #elif defined(_WIN32) while the indexer records FILETIME-derived nanoseconds, so a byte-identical file never matches.

So if this lands first, on Windows:

• get_code_snippet returns the placeholder for every request,
• search_code attaches no source or context to any result.

Both read tools would go dark on that platform. Today the broken oracle is merely uninformative because only check_index_coverage consumes it; this PR makes it load-bearing. That is not a flaw in either change — each is correct alone — it is purely an ordering constraint, and exactly the kind that merges clean and looks fine in review.

#1787 goes first. I have approved it; once it lands I will re-run this one. Worth adding a "depends on #1787" line to the description so nobody merges them the other way round.

Please drop the openspec/ files

Seven of the nine files here — openspec/config.yaml, openspec/specs/…, and the openspec/changes/archive/… tree — are spec-framework scaffolding, about 196 lines against a fix of mcp.c +56/-10 plus its test. openspec/ does not exist on main, no other contributor uses it, and adopting it is a project-level decision rather than something a bug fix should carry in.

We have decided not to adopt it, so please strip openspec/ from this PR and keep the code and tests. Same request on #1789 and #1790.

To be clear about what is being declined: the framework directory, not the thinking. Your D1–D4 decisions, the Non-Goals, and the Risks section are genuinely good analysis — D4 in particular (per-item rather than per-response, so one drifted file cannot suppress healthy results) is the right call and I would not have wanted it decided the other way. That reasoning is welcome in the PR description itself, where reviewers read it. It is the permanent config.yaml and per-change archive tree in a lean C repo that we are saying no to.

On the fix itself

The defect is well chosen. Slicing a live file with index-time coordinates and presenting the result as current source is the failure mode this project should care most about — not an error, not an absence, but plausible wrong text: a snippet of the adjacent function, or a grep hit attributed to the wrong symbol, with nothing in the response to say so.

Three things I checked and agree with:

• Reusing the existing oracle rather than inventing a second freshness notion keeps check_index_coverage and the read tools from ever disagreeing about whether a file is current. Two oracles would drift apart.
• Leaving unavailable / not_tracked on today's behavior. A file with no hash record cannot be judged, and refusing would regress reads of foreign files. Declining to widen the guard is the right instinct.
• Raw grep hits keep their content, because those lines come from live grep output rather than stale index coordinates. That distinction is easy to miss and you got it right.

Strip openspec/, note the #1787 dependency, and this is ready.

Thanks for the review! I've stripped the openspec/ files (the PR now only touches src/mcp/mcp.c and tests/test_mcp.c) and noted the dependency on #1787 in the description. Since #1787 has now merged, this should be ready for another look.

Thank you for the October 22 update and for removing the openspec files while preserving the design explanation. The description now records the merged #1787 dependency and the refreshed tests; the next review is on us. Sorry for the delay—I’ll follow up soon.

Thank you, Ulises, for this fix, for the careful design write-up, and for the October 22 update. Removing the openspec files while keeping the D1 to D4 reasoning in the description was exactly what we hoped for, and we are sorry you then waited on us again.

We are closing this PR because #2600, now merged in 2658a18, fixes the get_code_snippet side of the same defect with a stronger check. Instead of comparing mtime and size, it compares the file against the SHA-256 recorded at index time, which also catches a same-size edit that kept its timestamp, and it slices the snippet from the very bytes it verified. Landing both would have given the snippet path two drift checks that can disagree (for example when only the timestamp changed), so we kept one.

Your work shaped how we judged this. Keeping file_path and the line range on a withheld snippet is how #2600 behaves too, and your #1787 is what made file freshness trustworthy on Windows in the first place.

The search_code half is not covered by #2600: it still attaches source by indexed coordinates. #1750 stays open for it, and your D3 and D4 approach (freshness resolved once per result, raw grep hits keep their content, one drifted file never suppresses healthy results) is a good starting point for that work. If you would like to take that on as a focused PR built on the content check from #2600, it would be very welcome. Thank you again for the persistence and the care.