From 4361e9899875855286b8cb01748f281ccb82ac3a Mon Sep 17 00:00:00 2001 From: reacechalmers Date: Thu, 13 Aug 2026 14:10:23 -0600 Subject: [PATCH] fix: page large read_file results instead of returning whole files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unbounded read_file returned the entire file, which on a small-context backend overflows the model's window. The backend then rejects the whole request (llama-server: "request (N tokens) exceeds the available context size"), which kills the turn — so reading everything at once is precisely what makes the session stop. Observed live: one 100 KB source file is ~25k tokens against a 32k window with a ~6k-token prefix, and a session read the same file three times and died on six 400s. read_file now returns a page and says so: - absent `limit` yields DEFAULT_READ_LINES (2000) rather than EOF - MAX_READ_CHARS (64 KB) bounds the payload even when `limit` is explicit, so minified or generated files can't defeat the line cap - `truncated` and `nextOffset` tell the caller it has a partial view and exactly where to resume; the tool description explains both MAX_READ_SIZE (10 MB) is untouched: it is a process guard against slurping huge or binary blobs, not a context guard, and at ~2.5M tokens it never fires on the files that actually overflow a window. Also clears pre-existing clippy failures in trident.rs that blocked -D warnings on every edit to this crate. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q9McEEH32ZtyM62GSXVSyJ (cherry picked from commit c796d7e5eedc66b0cb37bb6823ec772f8ceef9cf) --- rust/crates/runtime/src/file_ops.rs | 127 +++++++++++++++++++++++++++- rust/crates/runtime/src/trident.rs | 37 +++----- rust/crates/tools/src/lib.rs | 10 ++- 3 files changed, 141 insertions(+), 33 deletions(-) diff --git a/rust/crates/runtime/src/file_ops.rs b/rust/crates/runtime/src/file_ops.rs index 7a62447a..f22c9b87 100644 --- a/rust/crates/runtime/src/file_ops.rs +++ b/rust/crates/runtime/src/file_ops.rs @@ -11,8 +11,28 @@ use serde::{Deserialize, Serialize}; use walkdir::{DirEntry, WalkDir}; /// Maximum file size that can be read (10 MB). +/// +/// This is a *process* guard — it stops us slurping a huge or binary blob into memory. It is +/// not a context guard: 10 MB is on the order of 2.5 M tokens, so it never fires on the files +/// that actually blow a model's context window. `DEFAULT_READ_LINES` / `MAX_READ_CHARS` below +/// are the context guard. const MAX_READ_SIZE: u64 = 10 * 1024 * 1024; +/// Lines returned by `read_file` when the caller does not pass an explicit `limit`. +/// +/// This is a *paging* size, not a refusal. An unbounded read is the easiest way for a model to +/// destroy its own context: one 100 KB source file is ~25 k tokens, which does not fit alongside +/// a system prompt in a 32 k window. When that happens the backend rejects the whole request and +/// the session dies — so reading everything at once is what makes it stop. Returning a page plus +/// `nextOffset` means the read always succeeds and the model can keep going. +const DEFAULT_READ_LINES: usize = 2000; + +/// Hard ceiling on the characters `read_file` returns, applied even when `limit` is explicit. +/// +/// `DEFAULT_READ_LINES` alone does not bound the payload: minified bundles, JSON blobs and +/// generated sources routinely put megabytes on a handful of lines. ~64 KB is ~16 k tokens. +const MAX_READ_CHARS: usize = 64 * 1024; + /// Maximum file size that can be written (10 MB). const MAX_WRITE_SIZE: usize = 10 * 1024 * 1024; @@ -80,6 +100,17 @@ pub struct TextFilePayload { pub start_line: usize, #[serde(rename = "totalLines")] pub total_lines: usize, + /// True when the file has more lines past this page. Signals the caller that it has NOT seen + /// the whole file, so it does not silently reason over a partial view. + #[serde(default)] + pub truncated: bool, + /// Line index to pass back as `offset` to continue reading. Absent once the file is exhausted. + #[serde( + rename = "nextOffset", + default, + skip_serializing_if = "Option::is_none" + )] + pub next_offset: Option, } /// Output envelope for the `read_file` tool. @@ -228,10 +259,27 @@ pub fn read_file( let content = fs::read_to_string(&absolute_path)?; let lines: Vec<&str> = content.lines().collect(); let start_index = offset.unwrap_or(0).min(lines.len()); - let end_index = limit.map_or(lines.len(), |limit| { - start_index.saturating_add(limit).min(lines.len()) - }); + // An absent `limit` pages rather than reading to EOF: see DEFAULT_READ_LINES. + let requested_end = start_index + .saturating_add(limit.unwrap_or(DEFAULT_READ_LINES)) + .min(lines.len()); + + // Apply the character ceiling on top of the line window, so a file with very long lines + // cannot blow the budget the line cap was meant to enforce. Always yield at least one line, + // otherwise an oversized single line would return nothing and the caller could not advance. + let mut end_index = start_index; + let mut chars = 0usize; + for line in &lines[start_index..requested_end] { + let next = chars + line.chars().count() + 1; + if next > MAX_READ_CHARS && end_index > start_index { + break; + } + chars = next; + end_index += 1; + } + let selected = lines[start_index..end_index].join("\n"); + let truncated = end_index < lines.len(); Ok(ReadFileOutput { kind: String::from("text"), @@ -241,6 +289,8 @@ pub fn read_file( num_lines: end_index.saturating_sub(start_index), start_line: start_index.saturating_add(1), total_lines: lines.len(), + truncated, + next_offset: truncated.then_some(end_index), }, }) } @@ -810,10 +860,14 @@ mod tests { use std::path::PathBuf; use std::time::{SystemTime, UNIX_EPOCH}; + use super::{ + component_contains_glob, derive_glob_walk_root, edit_file, expand_braces, glob_search, use super::{ component_contains_glob, derive_glob_walk_root, edit_file, expand_braces, glob_search, grep_search, is_symlink_escape, normalize_for_comparison, read_file, read_file_in_workspace, write_file, write_file_in_workspace, GrepSearchInput, + DEFAULT_READ_LINES, MAX_READ_CHARS, + }; MAX_WRITE_SIZE, }; @@ -837,6 +891,73 @@ mod tests { assert_eq!(read_output.file.content, "two"); } + #[test] + fn unbounded_read_of_a_large_file_pages_instead_of_returning_everything() { + // Regression: an unbounded read used to return the whole file, which on a small-context + // backend overflows the window and the request is rejected outright — the session dies. + let path = temp_path("paged-read.txt"); + let total = DEFAULT_READ_LINES + 500; + let body = (0..total) + .map(|i| format!("line {i}")) + .collect::>() + .join("\n"); + write_file(path.to_string_lossy().as_ref(), &body).expect("write should succeed"); + + let first = read_file(path.to_string_lossy().as_ref(), None, None).expect("first page"); + assert_eq!(first.file.num_lines, DEFAULT_READ_LINES); + assert_eq!(first.file.total_lines, total); + assert!(first.file.truncated, "caller must be told it is partial"); + assert_eq!(first.file.next_offset, Some(DEFAULT_READ_LINES)); + assert!(first.file.content.starts_with("line 0\n")); + + // The advertised next_offset must actually resume where the last page stopped. + let second = read_file( + path.to_string_lossy().as_ref(), + first.file.next_offset, + None, + ) + .expect("second page"); + assert_eq!(second.file.num_lines, 500); + assert!(!second.file.truncated, "the tail is the final page"); + assert_eq!(second.file.next_offset, None); + assert!(second + .file + .content + .starts_with(&format!("line {DEFAULT_READ_LINES}\n"))); + } + + #[test] + fn char_ceiling_bounds_files_whose_lines_are_enormous() { + // A line cap alone does not bound the payload: minified bundles put megabytes on a few + // lines, so the byte ceiling has to apply too — and must still yield forward progress. + let path = temp_path("long-lines.txt"); + let huge = "x".repeat(MAX_READ_CHARS); + let body = format!("{huge}\n{huge}\n{huge}"); + write_file(path.to_string_lossy().as_ref(), &body).expect("write should succeed"); + + let page = read_file(path.to_string_lossy().as_ref(), None, None).expect("first page"); + assert_eq!(page.file.num_lines, 1, "one oversized line still advances"); + assert!(page.file.truncated); + assert_eq!(page.file.next_offset, Some(1)); + + // An explicit limit must not be able to defeat the ceiling. + let greedy = read_file(path.to_string_lossy().as_ref(), None, Some(3)).expect("greedy"); + assert_eq!(greedy.file.num_lines, 1); + assert!(greedy.file.truncated); + } + + #[test] + fn small_files_are_returned_whole_and_not_marked_truncated() { + let path = temp_path("small-read.txt"); + write_file(path.to_string_lossy().as_ref(), "one\ntwo\nthree").expect("write"); + + let output = read_file(path.to_string_lossy().as_ref(), None, None).expect("read"); + assert_eq!(output.file.content, "one\ntwo\nthree"); + assert_eq!(output.file.num_lines, 3); + assert!(!output.file.truncated); + assert_eq!(output.file.next_offset, None); + } + #[test] fn edits_file_contents() { let path = temp_path("edit.txt"); diff --git a/rust/crates/runtime/src/trident.rs b/rust/crates/runtime/src/trident.rs index 2346a4ea..07576822 100644 --- a/rust/crates/runtime/src/trident.rs +++ b/rust/crates/runtime/src/trident.rs @@ -29,7 +29,7 @@ impl Default for TridentConfig { } /// Statistics from a Trident compaction run. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct TridentStats { pub superseded_count: usize, pub collapsed_chains: usize, @@ -41,21 +41,6 @@ pub struct TridentStats { pub final_message_count: usize, } -impl Default for TridentStats { - fn default() -> Self { - Self { - superseded_count: 0, - collapsed_chains: 0, - messages_collapsed: 0, - clusters_found: 0, - messages_clustered: 0, - tokens_saved_estimate: 0, - original_message_count: 0, - final_message_count: 0, - } - } -} - impl TridentStats { pub fn format_report(&self) -> String { let compression = if self.final_message_count > 0 { @@ -192,7 +177,7 @@ fn stage1_supersede(messages: &[ConversationMessage]) -> (Vec = BTreeSet::new(); - for (_path, ops) in &file_ops { + for ops in file_ops.values() { if ops.len() < 2 { continue; } @@ -205,10 +190,8 @@ fn stage1_supersede(messages: &[ConversationMessage]) -> (Vec = Vec::new(); let mut cluster_buffers: BTreeMap> = BTreeMap::new(); @@ -677,7 +660,7 @@ fn truncate_text(text: &str, max_chars: usize) -> String { mod tests { use super::*; use crate::compact::CompactionConfig; - use crate::session::{ContentBlock, ConversationMessage, MessageRole, Session}; + use crate::session::{ContentBlock, ConversationMessage, Session}; #[test] fn stage1_removes_obsolete_file_reads() { @@ -736,7 +719,7 @@ mod tests { fn stage2_collapses_chatty_messages() { let mut messages = vec![]; for i in 0..6 { - messages.push(ConversationMessage::user_text(&format!("ok {i}"))); + messages.push(ConversationMessage::user_text(format!("ok {i}"))); messages.push(ConversationMessage::assistant(vec![ContentBlock::Text { text: format!("got {i}"), }])); @@ -767,9 +750,9 @@ mod tests { }, ])); messages.push(ConversationMessage::tool_result( - &format!("read_{i}"), + format!("read_{i}"), "read_file", - &format!(r#"{{"path":"src/{i}.rs","content":"data {i}"}}"#), + format!(r#"{{"path":"src/{i}.rs","content":"data {i}"}}"#), false, )); } diff --git a/rust/crates/tools/src/lib.rs b/rust/crates/tools/src/lib.rs index 2ad7926a..dd90949e 100644 --- a/rust/crates/tools/src/lib.rs +++ b/rust/crates/tools/src/lib.rs @@ -506,13 +506,17 @@ pub fn mvp_tool_specs() -> Vec { }, ToolSpec { name: "read_file", - description: "Read a text file from the workspace.", + description: "Read a text file from the workspace. Large files come back one page at \ + a time: if the response has \"truncated\": true you have NOT seen the \ + whole file, and \"nextOffset\" is the line to pass as `offset` to \ + continue. Prefer grep_search to locate what you need over paging \ + through a big file, and never re-read a page you already have.", input_schema: json!({ "type": "object", "properties": { "path": { "type": "string" }, - "offset": { "type": "integer", "minimum": 0 }, - "limit": { "type": "integer", "minimum": 1 } + "offset": { "type": "integer", "minimum": 0, "description": "First line to read (0-based). Pass the previous response's nextOffset to continue." }, + "limit": { "type": "integer", "minimum": 1, "description": "Maximum lines to return. Capped by a byte ceiling regardless." } }, "required": ["path"], "additionalProperties": false