From 7f865aa59dddb88a21b0001e1350023fe9bc9e3f Mon Sep 17 00:00:00 2001 From: OceanLi <122793010+ohdearquant@users.noreply.github.com> Date: Mon, 22 Jun 2026 13:58:27 -0400 Subject: [PATCH 1/3] feat(inference): UTF-8-safe streaming detokenization + generate_streaming Add generate_streaming() for token-by-token output via a delta callback, and make incremental detokenization UTF-8-boundary-safe: byte-level BPE can split a multi-byte codepoint across tokens, so only complete-UTF-8 deltas are flushed while partial bytes buffer until the next token completes them. Without this, CJK and emoji streams surface U+FFFD replacement characters. Consumed by the Lattice Studio Chat surface (chat_metal --json streaming). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../inference/src/model/qwen35/detokenize.rs | 153 ++++++++++++++++-- .../inference/src/model/qwen35/generation.rs | 128 ++++++++++++++- 2 files changed, 270 insertions(+), 11 deletions(-) diff --git a/crates/inference/src/model/qwen35/detokenize.rs b/crates/inference/src/model/qwen35/detokenize.rs index a05e9e513b..a21c7f9161 100644 --- a/crates/inference/src/model/qwen35/detokenize.rs +++ b/crates/inference/src/model/qwen35/detokenize.rs @@ -1,29 +1,115 @@ use crate::tokenizer::bpe::BpeTokenizer; use std::collections::HashMap; -/// Decode token IDs back to text using the BPE tokenizer's reverse lookup. -pub(crate) fn decode_tokens(tokenizer: &BpeTokenizer, ids: &[u32]) -> String { +/// Build the GPT-2 byte-level reverse map (unicode placeholder char → original byte). +pub(crate) fn build_byte_decoder() -> HashMap { let byte_encoder = bytes_to_unicode(); let mut byte_decoder: HashMap = HashMap::new(); for (byte_val, &ch) in byte_encoder.iter().enumerate() { byte_decoder.insert(ch, byte_val as u8); } + byte_decoder +} - let mut bytes = Vec::new(); - for &id in ids { - if let Some(token_str) = tokenizer.token_for_id(id) { - for ch in token_str.chars() { - if let Some(&b) = byte_decoder.get(&ch) { - bytes.push(b); - } - // Skip characters not in byte_decoder (e.g., special tokens) +/// Append the raw decoded bytes for a single token id onto `out`. +/// +/// Byte-level BPE decoding is context-free per token, so concatenating the +/// per-token byte strings equals decoding the whole id sequence at once. +fn append_token_bytes( + tokenizer: &BpeTokenizer, + id: u32, + byte_decoder: &HashMap, + out: &mut Vec, +) { + if let Some(token_str) = tokenizer.token_for_id(id) { + for ch in token_str.chars() { + if let Some(&b) = byte_decoder.get(&ch) { + out.push(b); } + // Skip characters not in byte_decoder (e.g., special tokens) } } +} +/// Decode token IDs back to text using the BPE tokenizer's reverse lookup. +pub(crate) fn decode_tokens(tokenizer: &BpeTokenizer, ids: &[u32]) -> String { + let byte_decoder = build_byte_decoder(); + let mut bytes = Vec::new(); + for &id in ids { + append_token_bytes(tokenizer, id, &byte_decoder, &mut bytes); + } String::from_utf8_lossy(&bytes).to_string() } +/// Incremental, UTF-8-boundary-safe detokenizer for streaming generation. +/// +/// Byte-level BPE frequently splits a single multibyte codepoint (CJK, emoji, +/// accented Latin) across two or more tokens. Decoding the running token list +/// with `from_utf8_lossy` after every token would substitute `U+FFFD` for the +/// incomplete trailing bytes and stream that replacement char to the caller, +/// who cannot retract it. This type instead buffers raw bytes and emits only +/// the longest *complete* UTF-8 prefix after each token, holding incomplete +/// trailing bytes until a later token completes the codepoint. The concatenation +/// of every `push` delta plus the final `finish` equals `decode_tokens` over the +/// same ids. +pub(crate) struct IncrementalDetokenizer { + byte_decoder: HashMap, + bytes: Vec, + flushed: usize, +} + +impl IncrementalDetokenizer { + pub(crate) fn new() -> Self { + Self { + byte_decoder: build_byte_decoder(), + bytes: Vec::new(), + flushed: 0, + } + } + + /// Feed one token id; return the complete-UTF-8 text it reveals (which may be + /// empty when the token only contributed continuation bytes of a codepoint + /// still being assembled). + pub(crate) fn push(&mut self, tokenizer: &BpeTokenizer, id: u32) -> String { + append_token_bytes(tokenizer, id, &self.byte_decoder, &mut self.bytes); + self.flush_complete() + } + + fn flush_complete(&mut self) -> String { + let valid = match std::str::from_utf8(&self.bytes[self.flushed..]) { + Ok(s) => s.len(), + Err(e) => e.valid_up_to(), + }; + if valid == 0 { + return String::new(); + } + // [flushed .. flushed + valid] is guaranteed valid UTF-8 by the check + // above, so from_utf8_lossy performs no substitution here. + let delta = + String::from_utf8_lossy(&self.bytes[self.flushed..self.flushed + valid]).into_owned(); + self.flushed += valid; + delta + } + + /// Flush any trailing incomplete bytes lossily. Call once after the final + /// token so the concatenated stream matches `decode_tokens` exactly even when + /// a generation is truncated mid-codepoint. + pub(crate) fn finish(&mut self) -> String { + if self.flushed < self.bytes.len() { + let tail = String::from_utf8_lossy(&self.bytes[self.flushed..]).into_owned(); + self.flushed = self.bytes.len(); + tail + } else { + String::new() + } + } + + /// The full decoded text so far (equivalent to `decode_tokens` over every fed id). + pub(crate) fn text(&self) -> String { + String::from_utf8_lossy(&self.bytes).to_string() + } +} + /// GPT-2 byte-to-unicode mapping (same as in bpe.rs). pub fn bytes_to_unicode() -> Vec { let mut bs = Vec::new(); @@ -48,3 +134,50 @@ pub fn bytes_to_unicode() -> Vec { } table } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn incremental_flush_reconstructs_split_codepoints() { + // 好 = E5 A5 BD, 😀 = F0 9F 98 80 — split across simulated token boundaries + // so several intermediate buffers end mid-codepoint. + let chunks: &[&[u8]] = &[&[0xE5, 0xA5], &[0xBD], &[0xF0, 0x9F], &[0x98, 0x80], b"ok"]; + let mut d = IncrementalDetokenizer::new(); + let mut out = String::new(); + for c in chunks { + d.bytes.extend_from_slice(c); + out.push_str(&d.flush_complete()); + } + out.push_str(&d.finish()); + assert_eq!(out, "好😀ok"); + assert_eq!(out, d.text()); + } + + #[test] + fn incremental_flush_truncated_mid_codepoint_matches_lossy() { + // Generation truncated after only the first 2 of 好's 3 bytes: finish() + // flushes the incomplete tail lossily, matching decode_tokens' behavior. + let mut d = IncrementalDetokenizer::new(); + let mut out = String::new(); + d.bytes.extend_from_slice(&[b'h', b'i', 0xE5, 0xA5]); + out.push_str(&d.flush_complete()); + out.push_str(&d.finish()); + assert_eq!(out, String::from_utf8_lossy(&[b'h', b'i', 0xE5, 0xA5])); + assert_eq!(out, d.text()); + } + + #[test] + fn incremental_flush_ascii_is_exact_per_chunk() { + // Pure-ASCII path: every chunk is whole codepoints, so each flush emits it all. + let mut d = IncrementalDetokenizer::new(); + let mut out = String::new(); + for c in [b"He".as_slice(), b"llo", b"!"] { + d.bytes.extend_from_slice(c); + out.push_str(&d.flush_complete()); + } + out.push_str(&d.finish()); + assert_eq!(out, "Hello!"); + } +} diff --git a/crates/inference/src/model/qwen35/generation.rs b/crates/inference/src/model/qwen35/generation.rs index 77be4a89c8..ed1aafb4da 100644 --- a/crates/inference/src/model/qwen35/generation.rs +++ b/crates/inference/src/model/qwen35/generation.rs @@ -1,5 +1,5 @@ use super::cache::{ForwardScratch, KvCache}; -use super::detokenize::decode_tokens; +use super::detokenize::{IncrementalDetokenizer, decode_tokens}; use super::model::Qwen35Model; use super::sampling::sample_token; use crate::attention::gdn::GatedDeltaNetState; @@ -85,6 +85,132 @@ impl Qwen35Model { generated_tokens: generated_ids.len(), }) } + + /// Streaming variant of [`generate`] — identical token sequence, but invokes + /// `on_token` with incremental text deltas after each generated token. + /// + /// # Parity safety + /// + /// The body below is a deliberate copy of `generate` rather than a refactor of + /// the shared path. This ensures that no change here can silently alter the + /// non-streaming `generate` path, which is pinned by the e2e-parity CI gate + /// (greedy token match vs HF transformers). `on_token` is the only addition. + pub fn generate_streaming( + &self, + prompt: &str, + gen_cfg: &GenerateConfig, + mut on_token: impl FnMut(&str), + ) -> Result { + let cfg = &self.config; + + let mut rng_state = initial_rng_state(gen_cfg.seed); + + let input = self.tokenizer.tokenize(prompt); + let prompt_ids: Vec = input.input_ids[..input.real_length].to_vec(); + let prompt_len = prompt_ids.len(); + + if prompt_len == 0 { + return Err(InferenceError::Inference("empty prompt".into())); + } + + let num_linear = cfg.num_linear_attention_layers(); + let num_full = cfg.num_full_attention_layers(); + let mut gdn_states: Vec = (0..num_linear) + .map(|_| GatedDeltaNetState::new(cfg)) + .collect(); + let mut kv_cache = KvCache::new(num_full); + let mut scratch = ForwardScratch::new(); + + let mut generated_ids: Vec = Vec::with_capacity(gen_cfg.max_new_tokens); + let mut all_ids = prompt_ids.clone(); + + prefill_tokens( + self, + &prompt_ids, + &mut gdn_states, + &mut kv_cache, + &mut scratch, + ); + kv_cache.seq_len = prompt_len; + + let next_id = sample_token( + &scratch.logits[..cfg.vocab_size], + gen_cfg, + &all_ids, + &mut rng_state, + ); + + if should_stop_token(cfg, gen_cfg, next_id) { + return Ok(GenerateOutput { + text: String::new(), + token_ids: vec![], + prompt_tokens: prompt_len, + generated_tokens: 0, + }); + } + + generated_ids.push(next_id); + all_ids.push(next_id); + + // Incremental detokenization: emit only complete-UTF-8 text deltas. A + // byte-level BPE codepoint can span several tokens, so we buffer raw bytes + // and never stream a partial codepoint (see IncrementalDetokenizer). + let mut detok = IncrementalDetokenizer::new(); + let delta = detok.push(&self.tokenizer, next_id); + if !delta.is_empty() { + on_token(&delta); + } + + // Decode loop (mirrors decode_loop free function exactly). + for _ in 1..gen_cfg.max_new_tokens { + let pos = kv_cache.seq_len; + let last_token = *all_ids + .last() + .expect("invariant: prompt or prior generation seeded all_ids"); + + self.forward_step( + last_token, + pos, + &mut gdn_states, + &mut kv_cache, + &mut scratch, + ); + kv_cache.seq_len += 1; + + let next_id = sample_token( + &scratch.logits[..cfg.vocab_size], + gen_cfg, + &all_ids, + &mut rng_state, + ); + + if should_stop_token(cfg, gen_cfg, next_id) { + break; + } + + generated_ids.push(next_id); + all_ids.push(next_id); + + let delta = detok.push(&self.tokenizer, next_id); + if !delta.is_empty() { + on_token(&delta); + } + } + + // Flush any trailing incomplete bytes (generation truncated mid-codepoint) + // so the streamed deltas concatenate to exactly the returned text. + let tail = detok.finish(); + if !tail.is_empty() { + on_token(&tail); + } + + Ok(GenerateOutput { + text: detok.text(), + token_ids: generated_ids.clone(), + prompt_tokens: prompt_len, + generated_tokens: generated_ids.len(), + }) + } } fn initial_rng_state(seed: Option) -> u64 { From c40b55efef25461cf3ca3e994b8a8fddd6bd0129 Mon Sep 17 00:00:00 2001 From: OceanLi <122793010+ohdearquant@users.noreply.github.com> Date: Mon, 22 Jun 2026 14:49:04 -0400 Subject: [PATCH 2/3] fix(inference): address codex review on streaming detok - flush_complete: branch on Utf8Error::error_len() so a malformed byte emits U+FFFD and advances immediately instead of stalling the stream until finish(); add two invalid-byte regression tests - generate_streaming: guard max_new_tokens == 0 (return before sampling, so no unrequested token is emitted) - generate_streaming: replace .expect() on all_ids.last() with a let-else returning InferenceError (no expect/unwrap in library code) Co-Authored-By: Claude Opus 4.8 (1M context) --- codex_review_pr196.md | 54 ++++++++++++ .../inference/src/model/qwen35/detokenize.rs | 82 ++++++++++++++++--- .../inference/src/model/qwen35/generation.rs | 17 +++- 3 files changed, 138 insertions(+), 15 deletions(-) create mode 100644 codex_review_pr196.md diff --git a/codex_review_pr196.md b/codex_review_pr196.md new file mode 100644 index 0000000000..86357a7d50 --- /dev/null +++ b/codex_review_pr196.md @@ -0,0 +1,54 @@ +> _Comment authored by Codex (OpenAI agent) on behalf of @ohdearquant._ + +## Review of PR #196 + +### Verdict: REQUEST CHANGES + +This PR is close on the main CJK/emoji split-codepoint goal: the normal incomplete-tail path buffers bytes across token boundaries, avoids premature `U+FFFD`, and `finish()` flushes the final tail. I found two correctness/policy issues that should be fixed before merge. + +### Findings + +1. [crates/inference/src/model/qwen35/detokenize.rs:78](crates/inference/src/model/qwen35/detokenize.rs#L78) Invalid UTF-8 is treated as incomplete and can stall the stream until EOS. + - **Current**: `Err(e) => e.valid_up_to()` + - **Issue**: `valid_up_to()` alone cannot distinguish an incomplete trailing codepoint from a complete invalid byte sequence. Rust exposes that distinction through `Utf8Error::error_len()`: `None` means the stream ended mid-codepoint, while `Some(len)` means an invalid sequence should be skipped after inserting `U+FFFD`. With the current code, generated bytes like `[0x80]` followed by ASCII keep returning `valid == 0`, so the callback emits nothing until `finish()`, delaying both the replacement character and later valid text. Byte-level BPE can sample arbitrary byte tokens, so this is a real streaming failure mode, not just malformed input theory. + - **Fix**: In `flush_complete`, loop over the unflushed slice and branch on `error_len()`: buffer only the `None` incomplete-tail case; for `Some(len)`, emit the valid prefix plus `U+FFFD`, advance past the invalid sequence, and continue so following valid bytes are not stuck behind it. The Rust std docs show this exact lossy incremental pattern: . + - **Test**: Add a unit test with chunks `[0x80]`, `b"A"` asserting the first flush emits `"\u{FFFD}"`, the second emits `"A"`, and a valid split `[0xE5, 0xA5]`, `[0xBD]` still emits no replacement before completion. + - **Confidence**: Derived from `std::str::Utf8Error::{valid_up_to,error_len}` semantics and the current `flush_complete` state machine. + +2. [crates/inference/src/model/qwen35/generation.rs:167](crates/inference/src/model/qwen35/generation.rs#L167) New streaming path adds `expect()` in library code. + - **Current**: `.expect("invariant: prompt or prior generation seeded all_ids")` + - **Issue**: `AGENTS.md` explicitly forbids `unwrap()` / `expect()` in library code. This PR adds a second copy of the existing decode-loop panic into the new public `generate_streaming` path. The invariant is currently intended to hold after the prompt-empty check and first sample, but the policy is still no panicking assertions in library generation code. + - **Fix**: Replace the `expect()` with non-panicking control flow, e.g. `let Some(&last_token) = all_ids.last() else { return Err(InferenceError::Inference("empty generation state".into())); };`, or share a helper that returns `Result`. + - **Test**: Add a static regression check or focused test that the new streaming implementation contains no `expect()` / `unwrap()` in non-test library code. + - **Confidence**: Observed in the added PR diff and checked against `AGENTS.md`. + +3. [crates/inference/src/model/qwen35/generation.rs:136](crates/inference/src/model/qwen35/generation.rs#L136) `max_new_tokens = 0` still samples and can emit one token. + - **Current**: the method samples `next_id` before entering `for _ in 1..gen_cfg.max_new_tokens`. + - **Issue**: `GenerateConfig::max_new_tokens` is a public `usize` with no lower-bound invariant in [crates/inference/src/model/qwen35_config.rs:515](crates/inference/src/model/qwen35_config.rs#L515). The new streaming entry point therefore violates the token cap for zero by sampling, appending, and potentially calling `on_token` once. The HTTP server rejects zero earlier, but the library API does not. + - **Fix**: Return an empty `GenerateOutput` before prefill/sampling when `gen_cfg.max_new_tokens == 0`, or make the config invariant explicit and enforce it before generation. The same inherited behavior exists in `generate`, so fix both paths or factor generation to avoid drift. + - **Test**: Add a `max_new_tokens_zero_returns_empty_without_callback` regression for `generate_streaming` using a tiny/fake model harness if available; otherwise document and validate the config before both generation entry points. + - **Confidence**: Derived from the control flow and the unconstrained public config field. + +### What I Checked And Is Fine + +- Valid split CJK/emoji buffering path: `flush_complete` buffers `error_len() == None` tails by returning empty until later bytes complete the codepoint. +- Tail flushing: `finish()` emits the unflushed tail lossily, so truncation at the end of generation does not silently drop bytes. +- Normal `max_new_tokens >= 1` token/KV flow: the streaming loop mirrors `decode_loop`; the first sampled token is appended before the loop, then forwarded at `pos = kv_cache.seq_len` on the next iteration. +- EOS/stop tokens: sampled stop IDs are not appended or streamed, matching the non-streaming `generate` behavior. + +### Commands Run + +- `git diff origin/main...HEAD --check` passed. +- `cargo test -p lattice-inference incremental_flush` passed: 3 tests. +- `cargo test -p lattice-inference test_decode_tokens_roundtrip` passed: 1 test. +- `cargo test -p lattice-inference test_should_stop_token_includes_im_end` passed: 1 test. + +### What I Did Not Check + +- I did not run full `cargo clippy --workspace -- -D warnings`, full workspace tests, e2e parity, or `make bench-compare`. +- I did not run a real Qwen model generation session or benchmark per-token detokenization throughput. +- I did not verify the PR body's integrated-tree binary build claim. + +Domain utility: SKIPPED — khive memory recall returned only adjacent review heuristics, and `knowledge.suggest` was unavailable because the ANN index was still warming. + +Resume id: 019ef098-2989-7aa0-88aa-3642626ef70a diff --git a/crates/inference/src/model/qwen35/detokenize.rs b/crates/inference/src/model/qwen35/detokenize.rs index a21c7f9161..1fa149a328 100644 --- a/crates/inference/src/model/qwen35/detokenize.rs +++ b/crates/inference/src/model/qwen35/detokenize.rs @@ -76,19 +76,46 @@ impl IncrementalDetokenizer { } fn flush_complete(&mut self) -> String { - let valid = match std::str::from_utf8(&self.bytes[self.flushed..]) { - Ok(s) => s.len(), - Err(e) => e.valid_up_to(), - }; - if valid == 0 { - return String::new(); + let mut out = String::new(); + loop { + match std::str::from_utf8(&self.bytes[self.flushed..]) { + // Entire remaining buffer is valid UTF-8: emit it all. + Ok(s) => { + out.push_str(s); + self.flushed = self.bytes.len(); + return out; + } + Err(e) => { + let valid = e.valid_up_to(); + if valid > 0 { + // [flushed .. flushed + valid] is guaranteed valid by + // valid_up_to, so from_utf8_lossy substitutes nothing. + out.push_str( + String::from_utf8_lossy( + &self.bytes[self.flushed..self.flushed + valid], + ) + .as_ref(), + ); + self.flushed += valid; + } + match e.error_len() { + // None: the error is an incomplete trailing codepoint, + // not a malformed one. A later token may complete it, so + // keep the tail buffered and emit no replacement char. + None => return out, + // Some(len): a genuinely invalid sequence of `len` bytes + // that no future token can repair. Emit one U+FFFD (matching + // from_utf8_lossy's maximal-subpart substitution), skip past + // it, and continue — without this the stream would stall on + // the bad byte until finish(). + Some(len) => { + out.push('\u{FFFD}'); + self.flushed += len; + } + } + } + } } - // [flushed .. flushed + valid] is guaranteed valid UTF-8 by the check - // above, so from_utf8_lossy performs no substitution here. - let delta = - String::from_utf8_lossy(&self.bytes[self.flushed..self.flushed + valid]).into_owned(); - self.flushed += valid; - delta } /// Flush any trailing incomplete bytes lossily. Call once after the final @@ -168,6 +195,37 @@ mod tests { assert_eq!(out, d.text()); } + #[test] + fn incremental_flush_invalid_byte_does_not_stall_stream() { + // A lone continuation byte (0x80) is a malformed sequence no later token + // can complete. flush_complete must emit U+FFFD for it immediately and + // keep going, not buffer it until finish() (which would stall the stream). + let mut d = IncrementalDetokenizer::new(); + d.bytes.extend_from_slice(&[0x80]); + let first = d.flush_complete(); + assert_eq!(first, "\u{FFFD}", "invalid byte must flush immediately"); + d.bytes.extend_from_slice(b"A"); + let second = d.flush_complete(); + assert_eq!(second, "A", "valid byte after invalid one must flush"); + // Concatenated stream equals from_utf8_lossy over the whole byte buffer. + assert_eq!( + format!("{first}{second}"), + String::from_utf8_lossy(&[0x80, b'A']) + ); + } + + #[test] + fn incremental_flush_invalid_between_valid_matches_lossy() { + // Valid · invalid · valid in one buffer: one U+FFFD for the bad run, both + // valid runs verbatim — byte-for-byte equal to from_utf8_lossy. + let raw = [b'h', b'i', 0xFF, b'y', b'o']; + let mut d = IncrementalDetokenizer::new(); + d.bytes.extend_from_slice(&raw); + let out = d.flush_complete(); + assert_eq!(out, String::from_utf8_lossy(&raw)); + assert_eq!(out, d.text()); + } + #[test] fn incremental_flush_ascii_is_exact_per_chunk() { // Pure-ASCII path: every chunk is whole codepoints, so each flush emits it all. diff --git a/crates/inference/src/model/qwen35/generation.rs b/crates/inference/src/model/qwen35/generation.rs index ed1aafb4da..20e1b443dd 100644 --- a/crates/inference/src/model/qwen35/generation.rs +++ b/crates/inference/src/model/qwen35/generation.rs @@ -113,6 +113,17 @@ impl Qwen35Model { return Err(InferenceError::Inference("empty prompt".into())); } + // max_new_tokens == 0 means "generate nothing": return before sampling so + // we never emit a token the caller did not ask for. + if gen_cfg.max_new_tokens == 0 { + return Ok(GenerateOutput { + text: String::new(), + token_ids: vec![], + prompt_tokens: prompt_len, + generated_tokens: 0, + }); + } + let num_linear = cfg.num_linear_attention_layers(); let num_full = cfg.num_full_attention_layers(); let mut gdn_states: Vec = (0..num_linear) @@ -164,9 +175,9 @@ impl Qwen35Model { // Decode loop (mirrors decode_loop free function exactly). for _ in 1..gen_cfg.max_new_tokens { let pos = kv_cache.seq_len; - let last_token = *all_ids - .last() - .expect("invariant: prompt or prior generation seeded all_ids"); + let Some(&last_token) = all_ids.last() else { + return Err(InferenceError::Inference("empty generation state".into())); + }; self.forward_step( last_token, From 299aa523e0c730be5c3c5f6dec28259089762315 Mon Sep 17 00:00:00 2001 From: OceanLi <122793010+ohdearquant@users.noreply.github.com> Date: Mon, 22 Jun 2026 14:56:40 -0400 Subject: [PATCH 3/3] chore: remove stray codex review artifact from PR branch codex_review_pr196.md is a local review note that leaked into the branch via `git add -A`; it is not product code. The PR diff should contain only the streaming-detokenization source. Codex round-2 confirmed the three code findings are resolved; this removes the only remaining REQUEST CHANGES item. Co-Authored-By: Claude Opus 4.8 (1M context) --- codex_review_pr196.md | 54 ------------------------------------------- 1 file changed, 54 deletions(-) delete mode 100644 codex_review_pr196.md diff --git a/codex_review_pr196.md b/codex_review_pr196.md deleted file mode 100644 index 86357a7d50..0000000000 --- a/codex_review_pr196.md +++ /dev/null @@ -1,54 +0,0 @@ -> _Comment authored by Codex (OpenAI agent) on behalf of @ohdearquant._ - -## Review of PR #196 - -### Verdict: REQUEST CHANGES - -This PR is close on the main CJK/emoji split-codepoint goal: the normal incomplete-tail path buffers bytes across token boundaries, avoids premature `U+FFFD`, and `finish()` flushes the final tail. I found two correctness/policy issues that should be fixed before merge. - -### Findings - -1. [crates/inference/src/model/qwen35/detokenize.rs:78](crates/inference/src/model/qwen35/detokenize.rs#L78) Invalid UTF-8 is treated as incomplete and can stall the stream until EOS. - - **Current**: `Err(e) => e.valid_up_to()` - - **Issue**: `valid_up_to()` alone cannot distinguish an incomplete trailing codepoint from a complete invalid byte sequence. Rust exposes that distinction through `Utf8Error::error_len()`: `None` means the stream ended mid-codepoint, while `Some(len)` means an invalid sequence should be skipped after inserting `U+FFFD`. With the current code, generated bytes like `[0x80]` followed by ASCII keep returning `valid == 0`, so the callback emits nothing until `finish()`, delaying both the replacement character and later valid text. Byte-level BPE can sample arbitrary byte tokens, so this is a real streaming failure mode, not just malformed input theory. - - **Fix**: In `flush_complete`, loop over the unflushed slice and branch on `error_len()`: buffer only the `None` incomplete-tail case; for `Some(len)`, emit the valid prefix plus `U+FFFD`, advance past the invalid sequence, and continue so following valid bytes are not stuck behind it. The Rust std docs show this exact lossy incremental pattern: . - - **Test**: Add a unit test with chunks `[0x80]`, `b"A"` asserting the first flush emits `"\u{FFFD}"`, the second emits `"A"`, and a valid split `[0xE5, 0xA5]`, `[0xBD]` still emits no replacement before completion. - - **Confidence**: Derived from `std::str::Utf8Error::{valid_up_to,error_len}` semantics and the current `flush_complete` state machine. - -2. [crates/inference/src/model/qwen35/generation.rs:167](crates/inference/src/model/qwen35/generation.rs#L167) New streaming path adds `expect()` in library code. - - **Current**: `.expect("invariant: prompt or prior generation seeded all_ids")` - - **Issue**: `AGENTS.md` explicitly forbids `unwrap()` / `expect()` in library code. This PR adds a second copy of the existing decode-loop panic into the new public `generate_streaming` path. The invariant is currently intended to hold after the prompt-empty check and first sample, but the policy is still no panicking assertions in library generation code. - - **Fix**: Replace the `expect()` with non-panicking control flow, e.g. `let Some(&last_token) = all_ids.last() else { return Err(InferenceError::Inference("empty generation state".into())); };`, or share a helper that returns `Result`. - - **Test**: Add a static regression check or focused test that the new streaming implementation contains no `expect()` / `unwrap()` in non-test library code. - - **Confidence**: Observed in the added PR diff and checked against `AGENTS.md`. - -3. [crates/inference/src/model/qwen35/generation.rs:136](crates/inference/src/model/qwen35/generation.rs#L136) `max_new_tokens = 0` still samples and can emit one token. - - **Current**: the method samples `next_id` before entering `for _ in 1..gen_cfg.max_new_tokens`. - - **Issue**: `GenerateConfig::max_new_tokens` is a public `usize` with no lower-bound invariant in [crates/inference/src/model/qwen35_config.rs:515](crates/inference/src/model/qwen35_config.rs#L515). The new streaming entry point therefore violates the token cap for zero by sampling, appending, and potentially calling `on_token` once. The HTTP server rejects zero earlier, but the library API does not. - - **Fix**: Return an empty `GenerateOutput` before prefill/sampling when `gen_cfg.max_new_tokens == 0`, or make the config invariant explicit and enforce it before generation. The same inherited behavior exists in `generate`, so fix both paths or factor generation to avoid drift. - - **Test**: Add a `max_new_tokens_zero_returns_empty_without_callback` regression for `generate_streaming` using a tiny/fake model harness if available; otherwise document and validate the config before both generation entry points. - - **Confidence**: Derived from the control flow and the unconstrained public config field. - -### What I Checked And Is Fine - -- Valid split CJK/emoji buffering path: `flush_complete` buffers `error_len() == None` tails by returning empty until later bytes complete the codepoint. -- Tail flushing: `finish()` emits the unflushed tail lossily, so truncation at the end of generation does not silently drop bytes. -- Normal `max_new_tokens >= 1` token/KV flow: the streaming loop mirrors `decode_loop`; the first sampled token is appended before the loop, then forwarded at `pos = kv_cache.seq_len` on the next iteration. -- EOS/stop tokens: sampled stop IDs are not appended or streamed, matching the non-streaming `generate` behavior. - -### Commands Run - -- `git diff origin/main...HEAD --check` passed. -- `cargo test -p lattice-inference incremental_flush` passed: 3 tests. -- `cargo test -p lattice-inference test_decode_tokens_roundtrip` passed: 1 test. -- `cargo test -p lattice-inference test_should_stop_token_includes_im_end` passed: 1 test. - -### What I Did Not Check - -- I did not run full `cargo clippy --workspace -- -D warnings`, full workspace tests, e2e parity, or `make bench-compare`. -- I did not run a real Qwen model generation session or benchmark per-token detokenization throughput. -- I did not verify the PR body's integrated-tree binary build claim. - -Domain utility: SKIPPED — khive memory recall returned only adjacent review heuristics, and `knowledge.suggest` was unavailable because the ANN index was still warming. - -Resume id: 019ef098-2989-7aa0-88aa-3642626ef70a