[umi008]

What does this PR do?

Fixes #1750.

get_code_snippet and search_code sliced the live file on disk using line coordinates recorded at index time. After the file was edited without re-indexing those ranges are stale, and the tools returned shifted source text — a grep hit misattributed to an adjacent function, or a snippet of the wrong symbol — presented as though it were current source.

Both tools now consult the same freshness oracle check_index_coverage already exposes. When the recorded metadata no longer matches the file on disk (metadata_changed/missing), they skip the stale live slice and report source_drift: true plus the freshness state instead. The healthy path (metadata_match) is unchanged; re-indexing remains the remedy.

Depends on #1787 (merged). This change makes coverage_path_freshness load-bearing for the read tools, so it must not land on a tree where the oracle still reports metadata_changed for every file on Windows. Rebased onto current main, which includes #1787.

Design decisions

• D1 — single drift oracle. Reuse coverage_path_freshness: metadata_changed / missing ⇒ drifted (skip the live read, emit source_drift + freshness); metadata_match ⇒ today's behavior. unavailable / not_tracked / outside_project keep today's behavior: a file with no hash record cannot be judged, and containment is already enforced separately.
• D2 — snippet path. resolve_snippet_source gains bool read_allowed; only the read_file_lines call is gated. On drift the response keeps file_path / start_line / end_line (they describe the node asked about) and states that no source is available because the file changed after indexing. Outline and PDF paths are untouched.
• D3 — search path. Freshness is resolved once per result in handle_search_code and carried on search_result_t, so attach_result_source needs no extra store/project plumbing through the paged/budgeted renderers. A drifted item gets neither source nor context. Raw (unattributed) grep hits keep their content, since those lines come from live grep output rather than index coordinates.
• D4 — per item, not per response. One drifted file cannot suppress healthy results. Cost: one stat + one indexed hash-row read per result file.

Changes since the first revision: dropped the openspec/ tree as requested, rebased onto main (post-#1787 / budgeted search renderers), moved the search freshness lookup to handle_search_code per D3 above, and the tests now record the fixture hash with cbm_path_info_utf8 (same mtime source as the indexer, per #1714) instead of struct stat.

Tests

Two reproduce-first regression tests in tests/test_mcp.c:

• tool_get_code_snippet_reports_drift_after_file_change — fresh metadata serves source with no drift marker; after appending to the file, the response carries source_drift: true, freshness: metadata_changed, and no stale text.
• tool_search_code_marks_drifted_file_without_source — same for search_code in full mode.

Checklist

• Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
• Tests pass locally (scripts/test.sh --suites mcp: 382 passed, 4 skipped). Note: search_code_scan_live_child_deadline_is_bounded_and_fails_closed (100 ms deadline) alternated pass/fail on the same binary under ASan on a 2-core box; it fails before the code this PR touches.
• Lint passes (scripts/lint.sh). cppcheck reports nothing on the changed lines; I could not run the project's pinned clang-format/clang-tidy build locally.
• 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]

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.

[DeusData]

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.

[umi008]

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.

[DeusData]

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.