llm_runner: add streaming text boundary helpers by mergennachin · Pull Request #20242 · pytorch/executorch

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

mergennachin · 2026-06-12T20:12:58Z

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress.

This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling.

The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

#20001

pytorch-bot · 2026-06-12T20:13:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20242

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 6 Pending

As of commit ee1ea74 with merge base d7ca5db ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-06-12T20:13:43Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Pull request overview

Adds runner-level helper utilities to make incremental text streaming safer by (1) avoiding emission of incomplete UTF-8 sequences and (2) avoiding emission of prefixes that could still be part of a stop-string match across chunk boundaries. This supports the broader goal of making ExecuTorch’s LLM serving stack streaming-correct without duplicating fragile string logic across server paths.

Changes:

Introduces utf8_complete_prefix_len() to compute a UTF-8-safe emit prefix length.
Introduces stop_safe_prefix_len() to compute a stop-string-safe emit prefix length (with UTF-8 boundary snapping) and a stop_hit signal.
Adds focused unit tests for both helpers.

Reviewed changes

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

File	Description
extension/llm/runner/util.h	Adds UTF-8 boundary and stop-string boundary helper functions for streaming-safe text emission.
extension/llm/runner/test/test_util.cpp	Adds unit tests covering UTF-8 prefix handling and stop-string-safe prefix handling.

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

mergennachin · 2026-06-12T20:52:11Z

@claude Review this PR

claude · 2026-06-12T20:52:53Z

Claude finished @mergennachin's task in 2m 15s —— View job

Review: `llm_runner: add streaming text boundary helpers`

Read changed files (util.h, test_util.cpp)
Analyze helper correctness & edge cases
Check whether prior Copilot feedback is addressed
Post review

Nice, focused change. The helpers are pure, well-documented, and the test coverage hits the important boundary cases. I read this as good to merge modulo the minor notes below.

Prior Copilot feedback — addressed ✅

Both earlier comments referred to an out-of-bounds text[end] access and a missing hold == 0 test. The current revision resolves both:

Snapping is now delegated to a dedicated utf8_safe_prefix_len() (util.h:117) that backs up over continuation bytes and validates the lead byte's expected length, rather than indexing text[end]. No exclusive-offset read remains.
SingleByteStopMissEmitsEverything (test_util.cpp:96, stops={"Z"} → hold == 0) and HoldZeroDoesNotEmitDanglingUtf8LeadByte (test_util.cpp:138) cover the no-hold-back path, including the dangling-lead-byte case.

Correctness

The three functions look correct:

utf8_complete_prefix_len — forward scan, invalid lead byte treated as len 1 (emit-and-replace) so output never stalls. Good.
utf8_safe_prefix_len — backward snap, returns len only when a complete char ends exactly at len, else lead_pos. Never exceeds the requested length. Good.
stop_safe_prefix_len — earliest-occurrence wins, conservative max_len - 1 hold-back, snapped down to a UTF-8 boundary. The text.size() <= hold → 0 guard is correct.

Minor observations (non-blocking)

Stop offset isn't UTF-8-snapped (util.h:182-185). When a stop is found, earliest is returned directly. This is correct given the documented precondition (input is complete-UTF-8 and stops are real text). But a stop that is a bare continuation byte (e.g. "\x82") could split a multi-byte char. Worth a one-line comment noting the snap is intentionally skipped because the precondition makes a mid-char match impossible — or a defensive utf8_safe_prefix_len(text, earliest) if you want belt-and-suspenders.
Hold-back is coarser than necessary (util.h:186). Holding back max_len - 1 bytes unconditionally is always safe, but it holds back bytes even when the trailing text can't be a prefix of any stop (e.g. "hi ST" with stop "STOP" holds back " ST", not just "ST"). This only adds streaming latency, never incorrectness — fine to leave, but a comment clarifying the conservative choice would help future readers.
Minor duplication.utf8_complete_prefix_len and utf8_safe_prefix_len each carry their own lead-byte length table (forward vs. backward). Two uses is borderline per the repo's "no trivial single-use helpers" guidance, so I'd leave it — just flagging that the two tables must stay in sync.

Test suggestions (optional)

A stop that straddles into a held-back multi-byte tail, e.g. text ending in a partial multi-byte char where hold lands mid-character — confirms the snap interacts correctly with hold-back beyond the existing HoldBackSnapsToUtf8Boundary case.
Multiple stops of differing lengths where the longer one drives max_len but the shorter one is the earliest match — confirms earliest/max_len are tracked independently.
· branch llm-pr-b-runner-streaming-helpers

The serving stack streams decoded text incrementally, but token boundaries do not guarantee user-visible text boundaries. A chunk can end in the middle of a UTF-8 sequence, or immediately before bytes that complete a configured stop string. Handling that ad hoc in each server path would make streaming correctness easy to regress. This adds small, model-agnostic runner helpers for computing UTF-8-safe and stop-string-safe prefixes before emitting text. Keeping the logic in the runner utility layer gives both generic and model-specific workers one tested implementation instead of duplicating fragile string handling. The change is intentionally narrow: it introduces pure helper functions and focused unit coverage only. It does not change runner generation behavior on its own.

Copilot

Pull request overview

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

mergennachin · 2026-06-12T21:09:13Z

@claude Validate the last copilot comments, wdyt?

claude · 2026-06-12T21:10:05Z

Claude finished @mergennachin's task in 1m 15s —— View job

Validating the latest Copilot comments

I read util.h and test_util.cpp at the current revision. All three of the latest Copilot comments are factually correct, but they're documentation/edge-case nits rather than real bugs. My take on each:

1. `util.h:108` — invalid lead bytes `0xC0/0xC1` and `0xF5–0xF7` treated as multi-byte ✅ correct, low impact

The bit tests are the "simple" UTF-8 length classification:

(c >> 5) == 0x6 → 0xC0–0xDF (len 2), which includes0xC0/0xC1 (only ever produce overlong encodings → always invalid).
(c >> 3) == 0x1E → 0xF0–0xF7 (len 4), which includes0xF5–0xF7 (would encode > U+10FFFF → always invalid).

So the comment "An invalid lead byte counts as length 1" is only true for continuation bytes (0x80–0xBF) and 0xF8–0xFF; the four byte values above are instead consumed as multi-byte starts. Copilot is right that this contradicts the stated contract.

Real-world impact is negligible — a sane byte-level tokenizer won't emit these — but there is a theoretical streaming stall: utf8_complete_prefix_len("\xc0") returns 0 (holds the byte waiting for a 2nd that may never be part of the same char) instead of 1. Since the whole point of the helper is "emit-and-replace rather than stall," I'd lean toward making the code match the comment. Tightening the classification is cheap:

} elseif (c >= 0xC2 && c <= 0xDF) { // exclude 0xC0/0xC1
len = 2;
} elseif ((c >> 4) == 0xE) {
len = 3;
} elseif (c >= 0xF0 && c <= 0xF4) { // exclude 0xF5-0xF7
len = 4;
} else {
len = 1;
}

Alternatively, if you'd rather keep the simple bit-twiddling, just soften the comment to say "structurally-complete lead byte" instead of "valid." Either resolves the mismatch.

2. `util.h:163` — contract says complete-UTF-8 but tests feed incomplete tails ✅ correct

HoldBackWithIncompleteUtf8TailSnapsToBoundary ("ab\xe2\x82") and HoldZeroDoesNotEmitDanglingUtf8LeadByte ("ab\xc3") deliberately pass truncated trailing codepoints, and stop_safe_prefix_len handles them correctly via utf8_safe_prefix_len's back-snap. So the function is actually more robust than its documented contract. Worth a one-line tweak noting the back-snap defensively handles an incomplete trailing codepoint, so the "expected complete-UTF-8" line isn't misread as a hard precondition. Purely a doc fix.

3. `test_util.cpp:88` — no coverage for invalid lead-byte ranges ✅ correct, and coupled to #1

The tests only exercise an invalid continuation byte (0x80). Whatever you decide for #1, lock it in with assertions, e.g.:

EXPECT_EQ(utf8_complete_prefix_len("\xc0"), 1u); // overlong leadEXPECT_EQ(utf8_complete_prefix_len("\xf5"), 1u); // > U+10FFFF leadEXPECT_EQ(utf8_complete_prefix_len("\xf8"), 1u); // already len 1 today

Note "\xc0" / "\xf5" will only pass with the #1 code fix; with the current code they'd return 0. So this test is the natural way to decide between "fix code" vs. "soften comment."

Bottom line: none of these are correctness/safety bugs — no OOB reads, output never stalls in practice. They're a real but minor comment-vs-code mismatch (#1), a doc clarification (#2), and the missing test that pins it down (#3). I'd resolve #1 by tightening the classification and add the #3 assertions; #2 is a one-liner. Happy to push those changes if you want — just say the word.
· branch llm-pr-b-runner-streaming-helpers

CopilotAI review requested due to automatic review settings June 12, 2026 20:12

mergennachin requested a review from larryliu0820 as a code owner June 12, 2026 20:12

meta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 12, 2026

mergennachin temporarily deployed to cadence June 12, 2026 20:13 — with GitHub Actions Inactive

mergennachin requested a review from kirklandsign June 12, 2026 20:13

Copilotstarted reviewing on behalf of mergennachin June 12, 2026 20:13 View session

CopilotAI reviewed Jun 12, 2026
View reviewed changes

Comment threadextension/llm/runner/util.h Outdated
Comment threadextension/llm/runner/test/test_util.cpp

mergennachin requested a review from psiddh June 12, 2026 20:30

mergennachin force-pushed the llm-pr-b-runner-streaming-helpers branch from 2ceae8c to f0bc5c8Compare June 12, 2026 20:44

mergennachin had a problem deploying to cadence June 12, 2026 20:44 — with GitHub Actions Error

CopilotAI review requested due to automatic review settings June 12, 2026 21:03

mergennachin force-pushed the llm-pr-b-runner-streaming-helpers branch from f0bc5c8 to ee1ea74Compare June 12, 2026 21:03

Copilotstarted reviewing on behalf of mergennachin June 12, 2026 21:04 View session

mergennachin temporarily deployed to cadence June 12, 2026 21:05 — with GitHub Actions Inactive

CopilotAI reviewed Jun 12, 2026
View reviewed changes

Comment threadextension/llm/runner/util.h
Comment threadextension/llm/runner/util.h
Comment threadextension/llm/runner/test/test_util.cpp

Gasoonjia approved these changes Jun 12, 2026
View reviewed changes

mergennachin merged commit 06143cb into mainJun 15, 2026
187 checks passed

mergennachin deleted the llm-pr-b-runner-streaming-helpers branch June 15, 2026 14:17