Skip to content

fix(mcp): reject stale AST coordinates after source changes - #2216

Closed
lorenzozanee wants to merge 1 commit into
DeusData:mainfrom
lorenzozanee:restore/pr-2105
Closed

lorenzozanee wants to merge 1 commit into
DeusData:mainfrom
lorenzozanee:restore/pr-2105

Conversation

@lorenzozanee

@lorenzozanee lorenzozanee commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1750. Source edits can invalidate AST line ranges, so this update uses the canonical file metadata on Windows before exposing stored coordinates, rejects stale snippet reads, and preserves raw search matches without stale AST attribution.

Supersedes #2105(原PR分支已删,按原提交重建)

@github-actions

Copy link
Copy Markdown

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 DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for carrying the stale-range work into this focused replacement. Current source still uses indexed source ranges, with freshness checks forming a separate contract. Review needs to retain the supplied boundary cases and check the Windows metadata and untracked-store behavior explicitly. The queue is busy, so it may take a little time to complete that review.

@DeusData

Copy link
Copy Markdown
Owner

Thank you — rejecting stale AST coordinates after the source has changed is exactly the kind of correctness fix this project cares about. Returning a snippet keyed to bytes that have moved is worse than returning nothing.

Two blockers, both small.

1. The memory-core linter (this is what lint / lint is failing on, not clang-format):

memory-core linter FAILED: raw allocator use grew.
  src/mcp/mcp.c: grew by 1 (814 -> 815); latest sites: 18147:free

One raw free at src/mcp/mcp.c:18147. Every allocation here routes through src/foundation/mem_core.h rather than the ~800 raw sites this codebase used to have, because the memory accounting, the budget and the spill machinery all depend on seeing each one — a raw free is invisible to them. The linter ratchets, so existing sites are tolerated and new ones are not.

Use cbm_free (and cbm_alloc / cbm_calloc / cbm_realloc / cbm_mem_strdup for the rest of the family). Keep the pairing consistent: memory that came from cbm_alloc must go to cbm_free, and memory from a raw malloc elsewhere must still go to raw free. If line 18147 releases something allocated by code you did not touch, tell me rather than guessing and we will sort it out.

make -f Makefile.cbm lint-ci reproduces it locally.

2. The dco check is red. Your commits have no Signed-off-by trailer — the message is a headline with an empty body, so there is nothing for the check to parse. (Not the blank-line trap, where the sign-off sits outside the trailer block and git's parser skips it while grep still sees it — here there is no trailer at all.)

git commit --amend -s          # single commit
git rebase --signoff origin/main   # several
git push --force-with-lease

#2241 and #2242 merged cleanly today with correct sign-offs, so the habit is already there — these branches just predate it.

aspenas added a commit to aspenas/candlefish-memory-mcp that referenced this pull request Sep 23, 2026
Treat explicit project names as literals and reject conflicting roots before
staging a replacement index. Preserve coverage rows and imports into a fresh
clone. Recognize Swift test files and capture Git HEAD when a watcher registers
so commits made before the first poll are detected, including in subdirectories.

Adapt snippet and code-search freshness checks from upstream PR DeusData#2216,
commit 729c3ac.

Signed-off-by: Patrick Smith <25066580+aspenas@users.noreply.github.com>
@DeusData

Copy link
Copy Markdown
Owner

Thanks again for the stale-coordinate fix. Following up on the existing review: the allocator ownership/pairing and commit sign-offs remain the recorded requests. If the freed buffer comes from an untouched raw-allocation API, please flag that ownership so the next review can resolve it correctly.

@DeusData

Copy link
Copy Markdown
Owner

Thank you again for the stale-coordinate guard. The allocator-pairing and sign-off requests remain recorded; please flag any buffer owned by an unchanged allocation API rather than changing only its deallocator. We also need to reconcile this with the updated #1788 before further integration work.

@DeusData

Copy link
Copy Markdown
Owner

Thank you for the stale-coordinate guard, and for rebuilding it here from #2105 after the original branch was gone. Rejecting a snippet whose bytes have moved, rather than returning plausible wrong code, is exactly the right instinct for this project.

We are closing this PR because #2600, now merged in 2658a18, fixes the get_code_snippet side of the same defect (#1750). It compares the file against the SHA-256 recorded at index time instead of its metadata, so it also catches a same-size edit that kept its timestamp, and it slices the snippet from the exact bytes it verified. With that in place, the sign-off and allocator items from the earlier review no longer need any work from you.

Keeping raw search matches while dropping stale attribution, as you did for search_code, is the part #2600 does not cover. #1750 stays open for it, and your approach is a useful reference for that work. Thank you again for caring about this correctness problem.

@DeusData DeusData closed this Oct 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Stale AST line coordinates in get_code_snippet / search_code after file modification

2 participants