Skip to content

fix: preserve UTF-8 in search_code source - #1253

Open
anfedoro wants to merge 8 commits into
DeusData:mainfrom
anfedoro:agent/fix-search-code-utf8
Open

fix: preserve UTF-8 in search_code source#1253
anfedoro wants to merge 8 commits into
DeusData:mainfrom
anfedoro:agent/fix-search-code-utf8

Conversation

@anfedoro

Copy link
Copy Markdown

Summary

  • Preserve valid UTF-8 in search_code full results and raw match content.
  • Replace the ASCII-only sanitizer that converted every byte >= 0x80 to ? with the existing UTF-8-safe lossy sanitizer.
  • Add a regression test using Russian Markdown content.

Root cause

search_code constructed result fields through sanitize_ascii(). Because UTF-8 characters are encoded as bytes >= 0x80, the sanitizer replaced their individual bytes with ? after the file and line match had already been found correctly.

Scope

This PR addresses result serialization after a successful match. It intentionally does not address the separate Windows CP_ACP pattern-matching issue discussed in #1209.

Related to #1209

Validation

  • make -f Makefile.cbm test-focused TEST_SUITES=mcp — 179 passed, 2 skipped.
  • Production build via scripts/build.sh on macOS arm64.
  • Manual search_code --mode full smoke test confirmed Cyrillic is preserved in source.

@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges ux/behavior Display bugs, docs, adoption UX labels Jul 24, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 24, 2026
@DeusData DeusData added the priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. label Jul 24, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for opening the focused split requested in #1209. The exact patch stays in post-match serialization, reuses the existing sanitize_utf8_lossy implementation, and does not claim to fix Windows CP_ACP matching. No dependency, workflow, network, prompt, or raw-file-I/O surface is added. The Russian full-source fixture is a useful RED guard. Before exact-head approval, please add coverage for the other behavior claimed by the PR: raw match content, attached context, and one invalid UTF-8 sequence proving lossy replacement still yields valid JSON. Both commits currently lack visible DCO sign-offs, so the branch also needs a signed-off commit history and the complete CI result.

Signed-off-by: anfedoro <anfedoro@gmail.com>
@anfedoro
anfedoro force-pushed the agent/fix-search-code-utf8 branch from 57032ca to 05574ff Compare July 24, 2026 14:53
@anfedoro
anfedoro marked this pull request as ready for review July 29, 2026 07:17
@anfedoro
anfedoro requested a review from DeusData as a code owner July 29, 2026 07:17
@DeusData

Copy link
Copy Markdown
Owner

A quick note so this does not sit here looking like your problem: your red CI is not caused by your change — and a rebase will not fix it either.

The failure is tests/test_daemon_version.c:604 ASSERT(all_succeeded), a Windows concurrent conflict-log append test. It is a live flake on main itself, still failing as of yesterday — so it is ours to fix, not something you can clear by rebasing.

I have flagged it internally as a production-race candidate rather than a test to tune: under our own testing doctrine a test whose verdict depends on scheduler timing is a defect, and the right response is a deterministic repro plus a fix, not a wider budget.

So: nothing is needed from you on the CI front. Your PR is queued for a proper content review and I will come back to you on the change itself. Apologies that a failure of ours has been sitting on your PR looking like yours.

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.

2 participants