mirror of
https://github.com/instructkr/claw-code.git
synced 2026-10-18 00:00:11 +08:00
fix: page large read_file results instead of returning whole files
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q9McEEH32ZtyM62GSXVSyJ
(cherry picked from commit c796d7e5eedc66b0cb37bb6823ec772f8ceef9cf)
This commit is contained in:
1 parent
95f797f5e2
commit
4361e98998
3 files changed
+141
-33
No files matched your search
@@ -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<usize>,
|
||||
}
|
||||
|
||||
/// 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::<Vec<_>>()
|
||||
.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");
|
||||
|
||||
@@ -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<ConversationMessag
|
||||
|
||||
let mut obsolete_indices: BTreeSet<usize> = 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<ConversationMessag
|
||||
|
||||
if let Some(last_write) = last_write_idx {
|
||||
for op in ops {
|
||||
if op.op_type == FileOp::Read && op.index < last_write {
|
||||
obsolete_indices.insert(op.index);
|
||||
} else if (op.op_type == FileOp::Write || op.op_type == FileOp::Edit)
|
||||
&& op.index < last_write
|
||||
if op.index < last_write
|
||||
&& matches!(op.op_type, FileOp::Read | FileOp::Write | FileOp::Edit)
|
||||
{
|
||||
obsolete_indices.insert(op.index);
|
||||
}
|
||||
@@ -325,7 +308,7 @@ fn stage2_collapse(
|
||||
usage: None,
|
||||
});
|
||||
} else {
|
||||
result.extend(buffer.drain(..));
|
||||
result.append(&mut buffer);
|
||||
}
|
||||
buffer.clear();
|
||||
result.push(msg.clone());
|
||||
@@ -476,7 +459,7 @@ fn stage3_cluster(
|
||||
}
|
||||
|
||||
let total_clustered: usize = cluster_assignments.len();
|
||||
let clusters_found = cluster_id as usize;
|
||||
let clusters_found = cluster_id;
|
||||
|
||||
let mut result: Vec<ConversationMessage> = Vec::new();
|
||||
let mut cluster_buffers: BTreeMap<usize, Vec<usize>> = 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,
|
||||
));
|
||||
}
|
||||
|
||||
@@ -506,13 +506,17 @@ pub fn mvp_tool_specs() -> Vec<ToolSpec> {
|
||||
},
|
||||
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
|
||||
|
||||
Reference in new issue
Block a user