test(mcp): over-long grep records never invent matches (#2011) - #2066
test(mcp): over-long grep records never invent matches (#2011)#2066kavish-19 wants to merge 1 commit into
Conversation
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
|
Thanks — this is a correct fix for the tree it was written against, and I checked it the hard way: at your base ( What changed underneath you: #1597 merged on main this morning ( Could you rebase and keep just the test? The one conflict is delete-vs-modify on Then retitle to something like |
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>
4b73a51 to
c4bec4a
Compare
|
Thank you — and for checking it at my base rather than just against Verified your account of Rebased exactly as you set out — 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 Same caveat as on #2065 about what my "verified" covers: |
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:contentrecord: a fragment of matched content became the file, and the text after the next delimiter became the line number. An agent reading that result calledget_code_snippeton 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 acontinueinstead.Why this is test-only now
search_codeno longer reads into a fixed buffer. On currentmain,scan_and_classify_grep_matchesreads each record withcbm_getlineinto a growable buffer:so a record cannot split, and
collect_grep_matchesis gone. I confirmed that before rebasing — the only remaining mention of the old function onmainis 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
mainguards 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.c→search_code_long_line_does_not_invent_matches, beside the existingsearch_code_path_filter_*tests and registered in the suite. It writes a >2 KiB colon-laden line plus a normal one, callssearch_codethroughcbm_mcp_server_handle, and asserts:total_grep_matches == 2— the repository's two real matches, not the three the split producedline: 0, the value the fabricated record used to yieldAgainst the original buffered implementation it failed exactly as #2011 describes (
grep_matches == 3, expected 2); againstmain'scbm_getlineversion it passes, as you confirmed on your side.Changes from the first version of this PR
main; thesrc/mcp/mcp.cdrain hunk is dropped entirely —main's file taken as-is, per your resolution.fix(mcp): …totest(mcp): …, since there is no production change left.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
mainistests/test_mcp.conly.