From e17cc655888ce374379c3f7204d9ec0d1f385719 Mon Sep 17 00:00:00 2001 From: Yogthos Date: Thu, 21 May 2026 01:02:32 -0400 Subject: [PATCH] fix(F19+F20): strip UTF-8 BOM in read; bound interject channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two F-MEDIUM fixes from ROADMAP.md. ## F19 — UTF-8 BOM strip Old Windows-saved files start with U+FEFF (0xEF 0xBB 0xBF). Before this, the BOM survived `read_line` and ended up as an invisible prefix on the first line shown to the LLM. opencode's `Bom.readFile()` does the same strip on its side. `read.rs` line loop now tracks `first_line` and strips a leading `'\u{FEFF}'` from line 1 only. Mid-file BOMs (rare but possible when concatenating files) stay as regular chars. ## F20 — Bounded interject channel `AgentRunner::interject_tx: mpsc::UnboundedSender<()>` could grow without bound if the user hammered the interject keybind while the runner was in a long LLM call. Each press allocated an entry even though all but the first were redundant (the runner drains via `try_recv()` after the first wakeup). - Switch to `mpsc::Sender<()>` with capacity 64. - UI uses `try_send(())` instead of `send(())`: if the channel is full, no-op (we already have a wakeup queued). - Updated all 9 ownership transfer sites + the runner-side receiver type. ## Tests F19: two new tests in `agent::tools::read::tests`: - `read_strips_utf8_bom_from_first_line`: writes a file starting with `\u{FEFF}`, asserts the output has no BOM byte anywhere. - `read_only_strips_bom_at_start_of_file`: BOM in line 2 stays intact — guards against the strip applying to wrong lines. F20: no new tests (capacity-bound is a runtime contract; the existing interjection-queue tests in ui already exercise the try_send path). 681 pass (was 679). All build profiles clean. --- src/agent/runner.rs | 14 +++++++-- src/agent/tools/read.rs | 69 +++++++++++++++++++++++++++++++++++++++++ src/ui/mod.rs | 12 +++++-- 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/src/agent/runner.rs b/src/agent/runner.rs index 926bbb03..aa5c3595 100644 --- a/src/agent/runner.rs +++ b/src/agent/runner.rs @@ -119,7 +119,14 @@ pub struct AgentRunner { /// emits `AgentEvent::Interjected` with whatever assistant text had /// streamed so far, and the UI is responsible for queueing the next /// user turn. Unbounded because the signal payload is just `()`. - pub interject_tx: mpsc::UnboundedSender<()>, + /// F20: bounded so a user who hammers the interject keybind + /// can't fill an unbounded queue while the runner is in a long + /// LLM call. Only the FIRST signal needs to be received — all + /// subsequent ones are noise (the runner drains via + /// `try_recv()` after the first wakeup). 64 is generous; if + /// the channel is full, `try_send` silently no-ops (we already + /// have one queued). + pub interject_tx: mpsc::Sender<()>, } pub fn convert_history(session: &Session) -> Vec { @@ -220,7 +227,7 @@ async fn run_stream( prompt: &str, history: Vec, event_tx: &mpsc::Sender, - interject_rx: &mut mpsc::UnboundedReceiver<()>, + interject_rx: &mut mpsc::Receiver<()>, ) -> StreamOutcome where M: CompletionModel + 'static, @@ -361,7 +368,8 @@ where { cache.clear(); let (event_tx, event_rx) = mpsc::channel::(256); - let (interject_tx, mut interject_rx) = mpsc::unbounded_channel::<()>(); + // F20: bounded channel. See `AgentRunner::interject_tx` doc. + let (interject_tx, mut interject_rx) = mpsc::channel::<()>(64); let task = tokio::spawn(async move { let policy = RecoveryPolicy::default(); diff --git a/src/agent/tools/read.rs b/src/agent/tools/read.rs index 54b3dde3..3d19e6d3 100644 --- a/src/agent/tools/read.rs +++ b/src/agent/tools/read.rs @@ -121,8 +121,21 @@ impl Tool for ReadTool { let mut total_lines = 0usize; let mut excerpt_lines: Vec<(usize, String)> = Vec::with_capacity(limit); let want_end = offset.saturating_add(limit); + let mut first_line = true; while let Some(line) = lines.next_line().await.transpose() { let mut line = line?; + // F19: strip UTF-8 BOM from the FIRST line only. Old + // Windows-saved files start with U+FEFF (0xEF 0xBB 0xBF); + // when present, the BOM ended up as a leading 3-byte + // invisible-character prefix in the LLM context. + // opencode `read.ts` uses `Bom.readFile()` for the same + // reason. + if first_line { + if let Some(stripped) = line.strip_prefix('\u{FEFF}') { + line = stripped.to_string(); + } + first_line = false; + } if line.len() > MAX_LINE_BYTES { // Truncate by byte index — careful to land on a UTF-8 // boundary. Drop bytes until we find one. @@ -276,4 +289,60 @@ mod tests { let body_lines: Vec<&str> = out.lines().skip(2).collect(); assert_eq!(body_lines.len(), 5); } + + /// F19: UTF-8 BOM (U+FEFF, bytes 0xEF 0xBB 0xBF) at the start + /// of a file is stripped before the line reaches the LLM. The + /// raw 3-byte prefix would otherwise render as an + /// invisible-character at the start of line 1. + #[tokio::test] + async fn read_strips_utf8_bom_from_first_line() { + let path = temp_path("bom"); + let bom = "\u{FEFF}"; + std::fs::write(&path, format!("{bom}first\nsecond")).unwrap(); + + let tool = ReadTool::new(None, None); + let out = tool + .call(ReadArgs { + path: path.to_string_lossy().into_owned(), + offset: None, + limit: None, + }) + .await + .unwrap(); + let _ = std::fs::remove_file(&path); + + // Body lines (after the "File: …" header + blank line). + let body: Vec<&str> = out.lines().skip(2).collect(); + assert_eq!(body, vec!["1: first", "2: second"]); + // No BOM byte anywhere in the output. + assert!( + !out.contains('\u{FEFF}'), + "BOM should be stripped: {:?}", + out, + ); + } + + /// F19: only the FIRST line gets BOM-stripped. A mid-file BOM + /// (extremely rare but possible) is preserved as a regular + /// character. + #[tokio::test] + async fn read_only_strips_bom_at_start_of_file() { + let path = temp_path("bom-mid"); + let bom = "\u{FEFF}"; + std::fs::write(&path, format!("first\n{bom}second")).unwrap(); + + let tool = ReadTool::new(None, None); + let out = tool + .call(ReadArgs { + path: path.to_string_lossy().into_owned(), + offset: None, + limit: None, + }) + .await + .unwrap(); + let _ = std::fs::remove_file(&path); + + // The mid-file BOM stays. + assert!(out.contains('\u{FEFF}')); + } } diff --git a/src/ui/mod.rs b/src/ui/mod.rs index 26847509..cb5b85df 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -457,7 +457,10 @@ pub async fn run_interactive( // Sender into the running agent's interjection channel. The UI signals // (unit-only payload) when a user-typed interjection is queued; the // runner honors it at the next tool-result boundary. - let mut agent_interject: Option> = None; + // F20: bounded mpsc::Sender. Multiple interject signals while + // the runner is mid-call get coalesced — only the first wakeup + // matters since the runner drains via try_recv() after waking. + let mut agent_interject: Option> = None; let mut agent_line_started = false; let mut response_buf = String::new(); // Count of `AgentEvent::ToolCall` events observed during the @@ -1385,7 +1388,12 @@ pub async fn run_interactive( // (race with Done) — harmless, queue still // drains on the Done handler. if let Some(tx) = agent_interject.as_ref() { - let _ = tx.send(()); + // F20: try_send so a full channel + // (already-queued wakeup) is a + // no-op rather than blocking the + // UI thread. We only need ONE + // wakeup queued at a time. + let _ = tx.try_send(()); } for line in text.lines() { let safe_line = sanitize_output(line);