Skip to content

test(mcp): over-long grep records never invent matches (#2011) - #2066

Open
kavish-19 wants to merge 1 commit into
DeusData:mainfrom
kavish-19:fix-grep-record-alignment
Open

test(mcp): over-long grep records never invent matches (#2011)#2066
kavish-19 wants to merge 1 commit into
DeusData:mainfrom
kavish-19:fix-grep-record-alignment

Conversation

@kavish-19

@kavish-19 kavish-19 commented Sep 5, 2026

Copy link
Copy Markdown

Regression test for #2011. The production fix already landed via #1597 (3c7427e) — this PR is now the test only.

What #2011 was

A grep record longer than the read buffer was split across two reads, and the continuation was parsed as a fresh file:line:content record: a fragment of matched content became the file, and the text after the next delimiter became the line number. An agent reading that result called get_code_snippet on a path that was never in the repository. The same split could also lose a real match, when the tail held fewer than two delimiters and the record was dropped by a continue instead.

Why this is test-only now

search_code no longer reads into a fixed buffer. On current main, scan_and_classify_grep_matches reads each record with cbm_getline into a growable buffer:

ssize_t line_length = cbm_getline(&line, &line_capacity, fp);

so a record cannot split, and collect_grep_matches is gone. I confirmed that before rebasing — the only remaining mention of the old function on main is a word in a comment.

That closed #2011 as a side effect of the lean-output work rather than as a targeted fix, so nothing on main guards the shape today. A future rewrite of the read loop that reintroduced a fixed buffer would restore the bug silently. Hence the test.

The test

tests/test_mcp.csearch_code_long_line_does_not_invent_matches, beside the existing search_code_path_filter_* tests and registered in the suite. It writes a >2 KiB colon-laden line plus a normal one, calls search_code through cbm_mcp_server_handle, and asserts:

  • total_grep_matches == 2 — the repository's two real matches, not the three the split produced
  • no row carries line: 0, the value the fabricated record used to yield

Against the original buffered implementation it failed exactly as #2011 describes (grep_matches == 3, expected 2); against main's cbm_getline version it passes, as you confirmed on your side.

Changes from the first version of this PR

  • Rebased onto current main; the src/mcp/mcp.c drain hunk is dropped entirely — main's file taken as-is, per your resolution.
  • Retitled from fix(mcp): … to test(mcp): …, since there is no production change left.
  • Commit re-authored under kavish-19 <63698788+kavish-19@users.noreply.github.com>; the earlier push used the wrong email of mine, which I have corrected on fix(store): report a failed COUNT read instead of returning zero #2065 too. Sign-off matches the author on both.

Diff against main is tests/test_mcp.c only.

@kavish-19
kavish-19 requested a review from DeusData as a code owner September 5, 2026 07:13
@github-actions

github-actions Bot commented Sep 5, 2026

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 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 5, 2026
@DeusData

DeusData commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Thanks — this is a correct fix for the tree it was written against, and I checked it the hard way: at your base (ceba4272), reverting only your src/mcp/mcp.c hunk makes your new test fail exactly as #2011 describes (tests/test_mcp.c:5402: grep_matches == 3, expected 2), and restoring it passes; len == sizeof(line)-1 && line[len-1] != '\n' is precisely fgets's truncation signal and the drain sits before every continue. Nicely done.

What changed underneath you: #1597 merged on main this morning (3c7427ef) and replaced collect_grep_matches altogether — search_code now reads each grep record with cbm_getline (unbounded, doubling buffer) in scan_and_classify_grep_matches, so an over-long record can no longer split. I ran your test against main's production code: it passes, and the whole mcp suite is green with it (310 passed). So the production half of this PR is already on main, by a side effect nobody guarded — which is exactly why your test is still valuable: nothing on main protects the >2 KiB record shape today, and #2011 (which I've closed with the fixing SHA) deserves a regression test that names it.

Could you rebase and keep just the test? The one conflict is delete-vs-modify on src/mcp/mcp.c; the resolution is to take main's version as-is:

git fetch upstream main && git rebase upstream/main
# conflict in src/mcp/mcp.c: main already reads each record with cbm_getline,
# so your drain hunk has nothing left to fix — take main's file
git restore --source=upstream/main src/mcp/mcp.c && git add src/mcp/mcp.c
git rebase --continue            # tests/test_mcp.c merges on its own

Then retitle to something like test(mcp): over-long grep records never invent matches (#2011) and note in the body that the production fix landed via #1597. Keep the sign-off, and I'll merge it on green. Welcome aboard — and #2065 is being reviewed alongside.

A grep record longer than the read buffer used to be split across two
reads, and the continuation was parsed as a fresh file:line:content
record — a fragment of matched content became a file path and the text
after the next delimiter became a line number. An agent reading that
result called get_code_snippet on a path that was never in the
repository. The same split could also lose a real match, when the tail
held fewer than two delimiters and the record was dropped instead.

The production fix landed via DeusData#1597 (3c7427e): search_code now reads each
record with cbm_getline into a growable buffer, so a record can no longer
split. That closed DeusData#2011 as a side effect, and nothing on main guards the
shape — the buffer could be reintroduced by a future rewrite of the read
loop with no test to catch it.

Add the regression test the fix never got. It writes a >2 KiB colon-laden
line plus a normal one, calls search_code through cbm_mcp_server_handle,
and asserts the repository's two real matches are reported as two — not
three — and that no row carries the line 0 the fabricated record used to
produce.

Signed-off-by: kavish-19 <63698788+kavish-19@users.noreply.github.com>
@kavish-19
kavish-19 force-pushed the fix-grep-record-alignment branch from 4b73a51 to c4bec4a Compare September 5, 2026 17:25
@kavish-19 kavish-19 changed the title fix(mcp): keep grep parsing aligned on record boundaries test(mcp): over-long grep records never invent matches (#2011) Sep 5, 2026
@kavish-19

Copy link
Copy Markdown
Author

Thank you — and for checking it at my base rather than just against main; that made the situation unambiguous.

Verified your account of main before rebasing: scan_and_classify_grep_matches reads each record with cbm_getline(&line, &line_capacity, fp) into a growable buffer, so the split cannot happen, and collect_grep_matches survives only as a word in a comment at mcp.c:12327. My drain hunk has nothing left to fix.

Rebased exactly as you set out — git restore --source=upstream/main src/mcp/mcp.c, and tests/test_mcp.c merged on its own. The diff against main is now that one test file, +68/-0. Retitled, and the body says the production fix landed via #1597 (3c7427e) and explains why the shape is still worth guarding: it was closed as a side effect, so nothing on main protects it if the read loop is ever rewritten with a fixed buffer again.

One correction on my side, unprompted: the earlier pushes on this branch and on #2065 were authored under the wrong email of mine. Both are re-authored under kavish-19 <63698788+kavish-19@users.noreply.github.com>, with the sign-off matching the author. Content is byte-identical — I diffed the new commits against the old ones to be sure only the identity changed.

Same caveat as on #2065 about what my "verified" covers: scripts/build.sh fails on this machine at internal/cbm/preprocessor.cpp (fatal error: 'cctype' file not found — the C++ standard-library headers are missing from its Command Line Tools), and that blocks the test runner, so I have not executed the test myself. You have, which is the run that counts here. If CI disagrees I will fix it promptly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working 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.

search_code invents file paths and line numbers: fgets splits a >2047-byte grep record and the continuation is parsed as a fresh file:line:content

2 participants