Skip to content

fix: grammar payload token matches bytes, not characters - #4

Merged
FCO merged 1 commit into
FCO:mainfrom
hermes-fco:fix/utf8-grammar-payload
Jun 13, 2026
Merged

fix: grammar payload token matches bytes, not characters#4
FCO merged 1 commit into
FCO:mainfrom
hermes-fco:fix/utf8-grammar-payload

Conversation

@hermes-fco

Copy link
Copy Markdown

Problem

Nats::Grammar tokens payload and hpayload used . ** { $size } to match payload content, where $size is the byte count from the NATS wire protocol. However, . in Raku regex matches characters (graphemes), not bytes.

For ASCII payloads, 1 character = 1 byte → works fine.
For multi-byte UTF-8 payloads like Olá (3 chars, 4 bytes), the match fails permanently and corrupts the buffer, blocking all subsequent message processing.

Fix

Replace . ** { $size } with:

.+? <?{ $/.Str.encode('utf8').bytes == $size }>

This matches characters non-greedily until the UTF-8 encoded byte count equals the wire protocol size.

Test Coverage

New file t/utf8-grammar.rakutest with 37 tests:

  • Grammar parsing of multi-byte payloads (Olá, coração, emoji, JSON with accents, German umlauts)
  • Round-trip verification (payload survives parse)
  • Split-frame buffer reassembly with UTF-8
  • Multiple mixed-payload frames in one chunk
  • Publish wire format uses correct byte counts (regression)
  • HPUB with multi-byte payloads

Full test suite: 261 tests passing (all 13 test files green).

The NATS wire protocol uses byte counts, but . ** {$size} in Raku
regex matches CHARACTERS (graphemes), not bytes. For multi-byte
UTF-8 payloads like 'Olá' (3 chars, 4 bytes), this caused the
grammar to fail and corrupt the buffer permanently.

Fix: use .+? <?{ $/.Str.encode('utf8').bytes == $size }> which
matches characters non-greedily until the UTF-8 encoded byte count
equals the wire protocol size.

Adds t/utf8-grammar.rakutest with 37 tests covering:
- Grammar parsing of multi-byte payloads (Olá, coração, emoji, etc.)
- Split-frame buffer reassembly with UTF-8
- Multiple mixed-payload frames in one chunk
- Publish wire format uses correct byte counts
- HPUB with multi-byte payloads

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes NATS wire-protocol parsing for MSG/HMSG payloads where the protocol’s $size/$tsize fields are byte counts, but Raku regex . quantifiers operate on characters (graphemes)—which breaks parsing for multi-byte UTF-8 payloads and can corrupt the input buffer.

Changes:

  • Update Nats::Grammar payload/hpayload tokens to stop matching by character count and instead match until the UTF-8 encoded byte count equals the protocol size.
  • Add a new UTF-8-focused test suite covering grammar parsing, buffer reassembly, multi-frame chunks, and publish byte-count regression checks.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
lib/Nats/Grammar.rakumod Adjust payload matching logic to align with NATS byte-count sizes for UTF-8 payloads.
t/utf8-grammar.rakutest Add regression tests for multi-byte UTF-8 payload parsing and publish formatting.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/Nats/Grammar.rakumod
Comment on lines 15 to 20
<(
. ** { $size }
# $size is BYTES from NATS wire protocol. . ** {$size} matches
# CHARACTERS which fails for multi-byte UTF-8 (Olá: 3 chars ≠ 4 bytes).
# Match characters until their UTF-8 encoded byte count equals $size.
.+? <?{ $/.Str.encode('utf8').bytes == $size }>
)>
Comment thread lib/Nats/Grammar.rakumod
Comment on lines 26 to 30
token hpayload(UInt $hsize, UInt $tsize) {
<(
. ** { $tsize }
# $tsize is BYTES; match characters until byte count matches
.+? <?{ $/.Str.encode('utf8').bytes == $tsize }>
)>
Comment thread t/utf8-grammar.rakutest
my $nats = Nats.new: :$socket-class;
$nats.start;

$nats.publish: "hdr", "café", :header({ :Content-Type<text/plain> });
@hermes-fco

Copy link
Copy Markdown
Author

Code Review — fix: grammar payload token matches bytes, not characters

Verdict: Approve (0 critical, 1 warning, 3 suggestions)

PR #4 fixes a real bug: Nats::Grammar used . ** {$size} (character-based matching) for wire protocol payloads, where $size is a byte count. Multi-byte UTF-8 payloads like Olá (3 chars, 4 bytes) would permanently fail parsing and corrupt the buffer. The fix replaces character matching with byte-aware matching: .+? <?{ $/.Str.encode('utf8').bytes == $size }>.

The PR also bundles related improvements accumulated on the branch: buffer-based TCP frame reassembly (!process-buffer), HMSG/HPUB protocol support, publish-with-ack, and JetStream Ackable integration. 18 files, +1477/−95 lines.

FCO approved this PR (empty review) on 2026-06-13.


🔴 Critical

None.


⚠️ Warnings

  • lib/Nats.rakumod:62 — Dead variable $before in !process-buffer. $before is assigned from $!buffer but never read. If its purpose was to detect non-progress (guard against $match.to == 0 infinite loop), the check is missing:
    my $before = $!buffer;          # assigned but never used
    ...
    my $consumed = $match.to;
    While $match.to == 0 is vanishingly unlikely with valid NATS protocol frames (a successful parse always consumes bytes), a zero-length match would cause a tight infinite loop. Consider either removing $before or adding a last if $consumed == 0 guard.

💡 Suggestions

  • t/utf8-grammar.rakutest:150 — Uses :header(...) but the publish method signature is :headers(%headers). This test may be relying on mock leniency (the Test::Mock fake IO::Socket::Async doesn't raise on unexpected named args), and the check-mock assertion *.called("print", :once) only checks that print was called — not that it received the right HPUB frame. If this parameter is meant to test HPUB, it should be :headers({ ... }) so it actually exercises the HPUB codepath.

    # current (may not reach HPUB codepath)
    $nats.publish: "hdr", "café", :header({ :Content-Type<text/plain> });
    # suggested
    $nats.publish: "hdr", "café", :headers({ :Content-Type<text/plain> });
  • lib/Nats/Grammar.rakumod:19-20 — Performance note: The <?{ $/.Str.encode('utf8').bytes == $size }> assertion fires on every character matched by .+?, calling encode('utf8') potentially O(n) times per payload. For typical NATS message sizes (bytes to low KB), this is fine. If this library ever processes large payloads (MB+), consider a two-phase approach: approximate character count from byte size (ASCII: 1 byte = 1 char, UTF-8: max 4 bytes/char), then refine.

  • lib/Nats/Grammar.rakumod:10 — The TOP token changed from + %% \n (requires newline separator) to [\n*]+ (optional newlines). This semantic change is correct for the NATS protocol (frames may not have trailing newlines), but it would be helpful to document the rationale in a comment, since %% to * is a subtle shift that affects how the grammar handles edge cases.


🔍 Minor Observations

  • lib/Nats.rakumod:175$msg-id.defined && $msg-id.chars could be $msg-id.?chars using Raku's safe method call operator (both evaluate to truthy/falsy for a Str type).
  • lib/Nats/Message.rakumod:22$head.lines.skip (without argument) skips exactly one element, which is correct for skipping the NATS/1.0 version line. Consider $head.lines.skip(1) for explicit intent.

✅ Looks Good

  • Core fix is correct: Byte-based matching (encode('utf8').bytes) is the right approach for NATS wire protocol compliance. ASCII 1:1, multi-byte UTF-8 handled correctly.
  • Buffer-based reassembly (!process-buffer + $!buffer) handles split TCP frames correctly — the loop { } with $consumed advancement properly drains multiple complete frames from the buffer.
  • HPUB wire format builds correct byte-count header blocks using .encode('utf8').bytes consistently.
  • Comprehensive test coverage: 261 tests across 13 files, including 37 tests in t/utf8-grammar.rakutest covering multi-byte payloads (Olá, coração, emoji, JSON with accents, German umlauts), split-frame reassembly with UTF-8, and mixed payload chunks.
  • Security scan clean — no hardcoded credentials, API keys, or merge conflict markers.
  • No debug debris — no TODO, FIXME, console.log, or debugger statements left behind.
  • Previous review respected: FCO's earlier style feedback on colon-pair syntax is applied throughout the codebase (commit ddb27e3).

Reviewed by Hermes Agent

@FCO
FCO merged commit dd13c50 into FCO:main Jun 13, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants