Skip to content

fix(extract): snap docstring truncation to UTF-8 codepoint boundary (#1017)#1094

Open
harshitaajoshi wants to merge 2 commits into
DeusData:mainfrom
harshitaajoshi:fix/utf8-docstring-truncation-1017
Open

fix(extract): snap docstring truncation to UTF-8 codepoint boundary (#1017)#1094
harshitaajoshi wants to merge 2 commits into
DeusData:mainfrom
harshitaajoshi:fix/utf8-docstring-truncation-1017

Conversation

@harshitaajoshi

@harshitaajoshi harshitaajoshi commented Jul 15, 2026

Copy link
Copy Markdown

Fixes #1017

What does this PR do?

Docstring/comment truncation at MAX_COMMENT_LEN (500 bytes) in extract_comment_text() cuts at a raw byte offset, which can split a multi-byte UTF-8 character and leave an incomplete byte sequence in the stored nodes.properties JSON. This breaks any strict UTF-8 consumer reading the SQLite database (e.g. Python's sqlite3 module).

This PR snaps the truncation point back to the last complete UTF-8 codepoint boundary in extract_comment_text().

A regression test (docstring_utf8_truncation_boundary_issue1017) crafts a Go comment with 3-byte CJK characters straddling the 500-byte boundary and validates every byte of the extracted docstring is well-formed UTF-8.

Checklist

  • Every commit is signed off (git commit -s) -- required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
    ASan hangs on macOS Tahoe 26.5 (Apple Clang 17) for any binary, including a trivial int main(){return 0;}. All 6294 tests pass when built without sanitizers (SANITIZE="").
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@harshitaajoshi harshitaajoshi requested a review from DeusData as a code owner July 15, 2026 01:29
@DeusData DeusData added the bug Something isn't working label Jul 15, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 15, 2026
@DeusData DeusData added 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. labels Jul 15, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for the reproduce-first fix. The extract_comment_text boundary change is narrow and memory-safe, and the new test is red on the old raw-byte truncation path. I have triaged this with #1017 as a high-priority 0.9.1-rc parsing bug. The diff adds no dependency, network, installer, or workflow surface.

Two review changes are needed:

  1. The test's byte-offset explanation is inaccurate. After "// " plus 495 ASCII bytes, the first CJK codepoint starts at offset 498, so offset 500 is its final continuation byte. The pre-fix string retains the lead plus one continuation byte; it does not end with a lone lead byte from the third codepoint. Please correct the comments so the guard documents the actual red failure.
  2. The regression exercises extract_comment_text only, but the PR also changes two independent fixed-buffer JSON serializers. Either add red-on-main boundary coverage for both the sequential and parallel serializer paths, including 2-, 3-, and 4-byte final codepoints, or remove those defense-in-depth hunks from this focused PR.

The existing full CI is green; these asks are about keeping every behavior change proved and reviewable.

…eusData#1017)

Signed-off-by: Harshita Joshi <j.harshitaa06@gmail.com>
@harshitaajoshi harshitaajoshi force-pushed the fix/utf8-docstring-truncation-1017 branch from 619b301 to 3ebcf48 Compare July 15, 2026 21:27
@harshitaajoshi

Copy link
Copy Markdown
Author

Thanks for the review and for catching the byte-offset math. You're right that offset 500 is the final byte of the first kanji, so the pre-fix string ends with the lead byte plus one continuation (an incomplete 2-of-3 sequence), not a lone lead byte from the third codepoint. I've corrected the test comment to document the actual red failure accurately.

For the second point, I opted to keep this PR tightly focused on extract_comment_text() only, so I removed the append_json_string defense-in-depth hunks from pass_definitions.c and pass_parallel.c. Those can go in a follow-up PR with their own red-on-main boundary coverage for 2-, 3-, and 4-byte final codepoints across both serializer paths.

I force-pushed the amended commit and updated the PR description to reflect the narrower scope. Ready for another look when you get a chance.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docstring truncation splits multi-byte UTF-8 characters, producing invalid UTF-8 in nodes.properties (non-ASCII comments, e.g. Japanese)

2 participants