From f804dbb08c0dd398ad0c39b1d8a98e80189955f4 Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 21:24:50 -0500 Subject: [PATCH 1/5] fix(core): tell the model what file tools skipped and accept numbers written as strings grep passed over files above 1.5 MB and files that are not UTF-8 without a word, so a search whose answer sat in one of them read as "no matches". read_file refused a file above 1.5 MB before it looked at offset and limit, while the refusal told the model to retry with offset and limit, and it reported a directory or a call with no path as a file that is not UTF-8. glob and grep read .gitignore only inside a git repository, so in a project without one they walked node_modules and build output. offset, limit, timeout_secs and replace_all were read as JSON numbers and booleans only, so "5" or "true" was dropped and the default ran. grep now counts the files it skipped by reason in one closing line. read_file streams the requested window of a large file line by line, holding only the window and a bounded prefix of each line, and renders it exactly as a file read whole; a directory, and a missing path in any file tool, get errors that name them. glob and grep share one walker that applies .gitignore outside a repository and keeps the default inside one, so a .gitignore above the repository root still cannot hide the project. One helper reads those four arguments from their JSON type or from a string. The tool schemas are unchanged. --- crates/core/src/tools.rs | 521 +++++++++++++++++++++++++++++++++------ 1 file changed, 452 insertions(+), 69 deletions(-) diff --git a/crates/core/src/tools.rs b/crates/core/src/tools.rs index a027750b..5b9dd549 100644 --- a/crates/core/src/tools.rs +++ b/crates/core/src/tools.rs @@ -367,6 +367,14 @@ fn rel_display(root: &Path, path: &Path) -> String { .unwrap_or_else(|_| path.to_string_lossy().to_string()) } +/// The argument `key` read by `native`, or parsed from a string. Models often +/// write a number or a boolean as a string (`"5"`, `"true"`), and reading the +/// JSON type alone dropped it without a word and ran the default instead. +fn arg(args: &Value, key: &str, native: fn(&Value) -> Option) -> Option { + let value = &args[key]; + native(value).or_else(|| value.as_str()?.trim().to_ascii_lowercase().parse().ok()) +} + pub async fn execute( name: &str, args: &Value, @@ -460,68 +468,181 @@ fn list_dir(root: &Path, args: &Value) -> ToolOutcome { } fn read_file(root: &Path, args: &Value) -> ToolOutcome { - let rel = args["path"].as_str().unwrap_or_default(); + let Some(rel) = args["path"].as_str() else { + return ToolOutcome::err("missing required argument: path"); + }; let path = match resolve(root, rel) { Ok(p) => p, Err(e) => return ToolOutcome::err(e), }; - match std::fs::metadata(&path) { - Ok(m) if m.len() > MAX_FILE_BYTES => { + let offset = arg(args, "offset", Value::as_u64); + let limit = arg(args, "limit", Value::as_u64); + let asked_for_window = offset.is_some() || limit.is_some(); + let mut window = ReadWindow { + offset: offset.unwrap_or(1).max(1) as usize, + limit: limit.unwrap_or(MAX_READ_LINES as u64).min(MAX_READ_LINES as u64) as usize, + out: String::new(), + cut_at: None, + }; + let read_error = |e: std::io::Error| match e.kind() { + std::io::ErrorKind::InvalidData => ToolOutcome::err(format!("{rel} is not a UTF-8 text file")), + _ => ToolOutcome::err(format!("cannot read {rel}: {e}")), + }; + let total = match std::fs::metadata(&path) { + // A directory fails to read like any unreadable path; calling it a + // file that is not UTF-8 sends the model after an encoding problem. + Ok(m) if m.is_dir() => { + return ToolOutcome::err(format!("{rel} is a directory; use list_dir to see its entries")) + } + // The cap keeps a whole-file read of a huge file out of memory. A + // window of one streams instead: this refusal tells the model to + // ask for one, and refusing that too leaves it nothing to retry. + Ok(m) if m.len() > MAX_FILE_BYTES && !asked_for_window => { return ToolOutcome::err(format!("file too large ({} bytes); use grep or read with offset/limit", m.len())) } + Ok(m) if m.len() > MAX_FILE_BYTES => match stream_window(&path, &mut window) { + Ok(total) => total, + Err(e) => return read_error(e), + }, + Ok(_) => { + let text = match std::fs::read_to_string(&path) { + Ok(t) => t, + Err(e) => return read_error(e), + }; + for (i, line) in text.lines().enumerate().skip(window.offset - 1).take(window.limit) { + if !window.push(i + 1, line, line.len()) { + break; + } + } + text.lines().count() + } Err(e) => return ToolOutcome::err(format!("cannot read {rel}: {e}")), - _ => {} - } - let text = match std::fs::read_to_string(&path) { - Ok(t) => t, - Err(_) => return ToolOutcome::err(format!("{rel} is not a UTF-8 text file")), }; - let offset = args["offset"].as_u64().unwrap_or(1).max(1) as usize; - let limit = args["limit"].as_u64().unwrap_or(MAX_READ_LINES as u64) as usize; - let limit = limit.min(MAX_READ_LINES); - let total = text.lines().count(); - // An offset past the end must not read like an empty file: the model - // would conclude the content is gone rather than that its offset is - // stale. - if offset > total && total > 0 { - return ToolOutcome::err(format!( - "offset {offset} is past the end of {rel} ({total} lines); retry with a smaller offset" - )); + window.finish(rel, total) +} + +/// The numbered lines one read_file call returns. +struct ReadWindow { + /// First line to show, 1-based. + offset: usize, + /// Most lines to show. + limit: usize, + out: String, + /// The first line that did not fit under `MAX_READ_BYTES`, if one did not. + cut_at: Option, +} + +impl ReadWindow { + /// Whether line `n` (1-based) is still to be shown. + fn wants(&self, n: usize) -> bool { + self.cut_at.is_none() && n >= self.offset && n - self.offset < self.limit } - let mut out = String::new(); - let mut stopped_by_bytes = false; - let mut byte_cap_line = 0usize; - for (i, line) in text.lines().enumerate().skip(offset - 1).take(limit) { + + /// Show line `n`, whose full length is `len` bytes; `line` may hold only + /// the first `MAX_LINE_CHARS` bytes of a longer one. False once the byte + /// cap is reached and the read should stop. + fn push(&mut self, n: usize, line: &str, len: usize) -> bool { // A clipped line must say so: silently dropping its tail sends the // model into edit_file with an old_string that can never match. - let formatted = if line.len() > MAX_LINE_CHARS { + let formatted = if len > MAX_LINE_CHARS { let end = floor_char(line, MAX_LINE_CHARS); - format!("{:>5} {}… [line clipped; {} more bytes]\n", i + 1, &line[..end], line.len() - end) + format!("{n:>5} {}… [line clipped; {} more bytes]\n", &line[..end], len - end) } else { - format!("{:>5} {}\n", i + 1, line) + format!("{n:>5} {line}\n") }; - if out.len() + formatted.len() > MAX_READ_BYTES { - stopped_by_bytes = true; - byte_cap_line = i + 1; - break; + if self.out.len() + formatted.len() > MAX_READ_BYTES { + self.cut_at = Some(n); + return false; } - out.push_str(&formatted); - } - if stopped_by_bytes { - // `byte_cap_line` is the first line that did NOT fit, so the - // continuation resumes exactly there. The former `+ 1` skipped one - // line per capped read, and pointed past EOF when the cap landed on - // the final line. - out.push_str(&format!( - "… output limit reached at line {byte_cap_line} (file has {total} lines; continue with offset={byte_cap_line})\n" - )); - } else if total > offset - 1 + limit { - out.push_str(&format!("… {} more lines (file has {total} lines; continue with offset={})\n", total - (offset - 1 + limit), offset + limit)); + self.out.push_str(&formatted); + true } - if out.is_empty() { - out = "(empty file)".into(); + + /// The result for a file of `total` lines. + fn finish(self, rel: &str, total: usize) -> ToolOutcome { + let Self { offset, limit, mut out, cut_at } = self; + // An offset past the end must not read like an empty file: the model + // would conclude the content is gone rather than that its offset is + // stale. + if offset > total && total > 0 { + return ToolOutcome::err(format!( + "offset {offset} is past the end of {rel} ({total} lines); retry with a smaller offset" + )); + } + if let Some(cut_at) = cut_at { + // `cut_at` is the first line that did NOT fit, so the + // continuation resumes exactly there. The former `+ 1` skipped + // one line per capped read, and pointed past EOF when the cap + // landed on the final line. + out.push_str(&format!( + "… output limit reached at line {cut_at} (file has {total} lines; continue with offset={cut_at})\n" + )); + } else if total > offset - 1 + limit { + out.push_str(&format!("… {} more lines (file has {total} lines; continue with offset={})\n", total - (offset - 1 + limit), offset + limit)); + } + if out.is_empty() { + out = "(empty file)".into(); + } + ToolOutcome::ok(out) + } +} + +/// Fill `window` from a file too large to load whole, and return its line +/// count. Lines are read one at a time and only those in the window are +/// decoded, each held to the prefix that `ReadWindow::push` can show, so +/// memory stays bounded however long the file or any of its lines runs. +fn stream_window(path: &Path, window: &mut ReadWindow) -> std::io::Result { + let mut reader = std::io::BufReader::new(std::fs::File::open(path)?); + let mut kept = Vec::new(); + let mut n = 0; + loop { + let keep = if window.wants(n + 1) { MAX_LINE_CHARS } else { 0 }; + let Some(len) = next_line(&mut reader, &mut kept, keep)? else { + return Ok(n); + }; + n += 1; + if keep == 0 { + continue; + } + let line = match std::str::from_utf8(&kept) { + Ok(line) => line, + // The kept prefix of a clipped line can end inside a character. + Err(e) if e.error_len().is_none() && len > kept.len() => { + std::str::from_utf8(&kept[..e.valid_up_to()]).unwrap_or_default() + } + Err(e) => return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, e)), + }; + window.push(n, line, len); + } +} + +/// Read the next line into `kept`, keeping at most its first `keep` bytes, +/// and return its full length; None at the end of the file. Lines end as +/// `str::lines` ends them: at `\n`, with one `\r` before it dropped. +fn next_line(reader: &mut impl std::io::BufRead, kept: &mut Vec, keep: usize) -> std::io::Result> { + kept.clear(); + let mut len = 0; + let mut last = None; + loop { + let chunk = reader.fill_buf()?; + if chunk.is_empty() { + return Ok((len > 0).then_some(len)); + } + let newline = chunk.iter().position(|&b| b == b'\n'); + let body = &chunk[..newline.unwrap_or(chunk.len())]; + kept.extend_from_slice(&body[..body.len().min(keep.saturating_sub(kept.len()))]); + len += body.len(); + last = body.last().copied().or(last); + let used = newline.map_or(chunk.len(), |i| i + 1); + reader.consume(used); + if newline.is_some() { + if last == Some(b'\r') { + len -= 1; + kept.truncate(len); + } + return Ok(Some(len)); + } } - ToolOutcome::ok(out) } fn floor_char(s: &str, mut idx: usize) -> usize { @@ -586,7 +707,9 @@ fn diff_strings(rel: &str, old: &str, new: &str) -> (DiffInfo, bool) { } fn write_file(root: &Path, args: &Value) -> ToolOutcome { - let rel = args["path"].as_str().unwrap_or_default(); + let Some(rel) = args["path"].as_str() else { + return ToolOutcome::err("missing required argument: path"); + }; let Some(content) = args["content"].as_str() else { return ToolOutcome::err("missing required argument: content"); }; @@ -712,7 +835,9 @@ fn to_crlf(text: &str) -> String { } fn edit_file(root: &Path, args: &Value) -> ToolOutcome { - let rel = args["path"].as_str().unwrap_or_default(); + let Some(rel) = args["path"].as_str() else { + return ToolOutcome::err("missing required argument: path"); + }; let (Some(old_string), Some(new_string)) = (args["old_string"].as_str(), args["new_string"].as_str()) else { return ToolOutcome::err("missing required arguments: old_string and new_string"); }; @@ -727,7 +852,7 @@ fn edit_file(root: &Path, args: &Value) -> ToolOutcome { if old_string == new_string { return ToolOutcome::err("old_string and new_string are identical"); } - let replace_all = args["replace_all"].as_bool().unwrap_or(false); + let replace_all = arg(args, "replace_all", Value::as_bool).unwrap_or(false); let path = match resolve(root, rel) { Ok(p) => p, Err(e) => return ToolOutcome::err(e), @@ -786,18 +911,32 @@ fn edit_file(root: &Path, args: &Value) -> ToolOutcome { changed_file("edited", root, &path, &old, &new) } +/// The walk glob and grep share, from `walk_root`. +/// /// Hidden files are searchable: the agent's own extension surface lives in /// dot-directories (`.openmax/tools`, `.agents`, `.github`), and a walker /// that skips them makes the agent blind to the capabilities it wrote. /// `.git` alone is excluded by name; gitignore rules still apply. -fn project_walk(root: &Path) -> ignore::Walk { - ignore::WalkBuilder::new(root) +/// +/// The walker reads .gitignore only inside a repository by default, which in +/// a project without one sent glob and grep through the node_modules and +/// build output it ignores. Outside a repository the rules apply regardless. +/// Inside one the default stays, because only the default stops reading them +/// at the repository's root: without it a .gitignore above the root, such as +/// a home directory kept in git that ignores `*`, would hide the project. +fn project_walker(walk_root: &Path) -> ignore::WalkBuilder { + // The markers and the canonical ancestors the walker itself checks. + let canon = walk_root.canonicalize().unwrap_or_else(|_| walk_root.to_path_buf()); + let in_repository = canon.ancestors().any(|dir| dir.join(".git").exists() || dir.join(".jj").exists()); + let mut walker = ignore::WalkBuilder::new(walk_root); + walker .hidden(false) .filter_entry(|e| e.file_name() != std::ffi::OsStr::new(".git")) .git_ignore(true) .git_global(true) - .max_depth(Some(24)) - .build() + .require_git(in_repository) + .max_depth(Some(24)); + walker } /// The subtree a glob can possibly match: its literal prefix up to the last @@ -901,7 +1040,7 @@ fn glob_tool(root: &Path, args: &Value) -> ToolOutcome { } } let mut hits: Vec<(std::time::SystemTime, String)> = Vec::new(); - for entry in project_walk(&walk_root).flatten() { + for entry in project_walker(&walk_root).build().flatten() { let path = entry.path(); if !path.is_file() || !link_stays_in_root(&entry, &root_canon) { continue; @@ -965,18 +1104,16 @@ fn grep_tool(root: &Path, args: &Value) -> ToolOutcome { // Full-corpus scans (rare or no matches) dominate this tool's latency, so // walk and scan in parallel. Hits are collected per file and sorted before // the cap so the output order is deterministic across runs. - use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; let hits: std::sync::Mutex> = std::sync::Mutex::new(Vec::new()); let enough = AtomicBool::new(false); + // Files passed over, by reason: a skipped file can hold the match, so a + // search that skips any must say so rather than answer "no matches". + let too_large = AtomicUsize::new(0); + let not_text = AtomicUsize::new(0); + let unreadable = AtomicUsize::new(0); let threads = std::thread::available_parallelism().map(|n| n.get()).unwrap_or(4).min(12); - ignore::WalkBuilder::new(&search_root) - // Same visibility contract as `project_walk`: hidden files are - // searchable, `.git` alone is excluded by name. - .hidden(false) - .filter_entry(|e| e.file_name() != std::ffi::OsStr::new(".git")) - .git_ignore(true) - .git_global(true) - .max_depth(Some(24)) + project_walker(&search_root) .threads(threads) .build_parallel() .run(|| { @@ -997,11 +1134,22 @@ fn grep_tool(root: &Path, args: &Value) -> ToolOutcome { return WalkState::Continue; } } - if entry.metadata().map(|m| m.len() > MAX_FILE_BYTES).unwrap_or(true) { - return WalkState::Continue; - } - let Ok(text) = std::fs::read_to_string(path) else { - return WalkState::Continue; + // The size of what the read below gets: through a link, its + // target, not the link itself. + let text = match std::fs::metadata(path) { + Ok(m) if m.len() > MAX_FILE_BYTES => Err(&too_large), + Ok(_) => std::fs::read_to_string(path).map_err(|e| match e.kind() { + std::io::ErrorKind::InvalidData => ¬_text, + _ => &unreadable, + }), + Err(_) => Err(&unreadable), + }; + let text = match text { + Ok(text) => text, + Err(skipped) => { + skipped.fetch_add(1, Ordering::Relaxed); + return WalkState::Continue; + } }; let rel = rel_display(root, path); let mut file_hits = Vec::new(); @@ -1021,9 +1169,13 @@ fn grep_tool(root: &Path, args: &Value) -> ToolOutcome { }) }); + let skipped = skipped_note(too_large.into_inner(), not_text.into_inner(), unreadable.into_inner()); let mut hits = hits.into_inner().unwrap(); if hits.is_empty() { - return ToolOutcome::ok("no matches".into()); + return ToolOutcome::ok(match skipped { + Some(note) => format!("no matches\n{note}"), + None => "no matches".into(), + }); } hits.sort(); let capped = hits.len() >= MAX_GREP_RESULTS; @@ -1035,9 +1187,30 @@ fn grep_tool(root: &Path, args: &Value) -> ToolOutcome { if capped { out.push_str("… result limit reached; refine the pattern\n"); } + if let Some(note) = skipped { + out.push_str(¬e); + out.push('\n'); + } ToolOutcome::ok(out) } +/// One line counting the files grep passed over, by reason, or None when it +/// searched every file it walked. +fn skipped_note(too_large: usize, not_text: usize, unreadable: usize) -> Option { + let files = |n: usize| if n == 1 { "1 file".to_string() } else { format!("{n} files") }; + let mut reasons = Vec::new(); + if too_large > 0 { + reasons.push(format!("{} over {} MB", files(too_large), MAX_FILE_BYTES as f64 / 1e6)); + } + if not_text > 0 { + reasons.push(format!("{} not UTF-8 text", files(not_text))); + } + if unreadable > 0 { + reasons.push(format!("{} unreadable", files(unreadable))); + } + (!reasons.is_empty()).then(|| format!("… not searched: {}", reasons.join(", "))) +} + /// The tail of `text` within `max_bytes`, starting at a line boundary when /// one is close by. fn kept_tail(text: &str, max_bytes: usize) -> &str { @@ -1158,7 +1331,7 @@ async fn bash_tool( let Some(command) = args["command"].as_str() else { return ToolOutcome::err("missing required argument: command"); }; - let timeout_secs = args["timeout_secs"].as_u64().unwrap_or(60).clamp(1, 300); + let timeout_secs = arg(args, "timeout_secs", Value::as_u64).unwrap_or(60).clamp(1, 300); let request = ProcessRequest { program: bash_shell().into(), args: vec!["-c".into(), command.into()], @@ -1533,6 +1706,76 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + /// A file grep passes over can hold the match, so a search that skipped + /// it must say so instead of answering "no matches". + #[test] + fn grep_counts_the_files_it_could_not_search() { + let root = temp_project(); + let filler = "padding line without the word\n"; + let big = filler.repeat(MAX_FILE_BYTES as usize / filler.len() + 1) + "needle\n"; + std::fs::write(root.join("docs/huge.log"), big).unwrap(); + std::fs::write(root.join("docs/latin1.txt"), b"needle caf\xe9\n").unwrap(); + + let out = grep_tool(&root, &json!({"pattern": "needle"})); + assert!(out.ok, "{}", out.output); + assert_eq!( + out.output, + "no matches\n… not searched: 1 file over 1.5 MB, 1 file not UTF-8 text", + "skipped files are counted by reason" + ); + + // The note follows the hits, and counts every file of a reason. + std::fs::write(root.join("docs/latin1-2.txt"), b"caf\xe9\n").unwrap(); + std::fs::write(root.join("src/needle.rs"), "// needle\n").unwrap(); + let out = grep_tool(&root, &json!({"pattern": "needle"})); + assert!(out.ok, "{}", out.output); + assert_eq!( + out.output, + "src/needle.rs:1: // needle\n… not searched: 1 file over 1.5 MB, 2 files not UTF-8 text\n" + ); + + // A search that read every candidate carries no note. + let out = grep_tool(&root, &json!({"pattern": "needle", "path": "src"})); + assert_eq!(out.output, "src/needle.rs:1: // needle\n"); + let _ = std::fs::remove_dir_all(root); + } + + /// The ignore crate reads .gitignore only inside a git repository by + /// default, so in a project without one grep and glob walked + /// node_modules and build output the project had ignored. + #[test] + fn grep_and_glob_honor_gitignore_without_a_git_repository() { + let root = temp_project(); + std::fs::write(root.join(".gitignore"), "node_modules/\nbuild/\n").unwrap(); + std::fs::create_dir_all(root.join("node_modules/pkg")).unwrap(); + std::fs::write(root.join("node_modules/pkg/index.js"), "alpha_vendored\n").unwrap(); + std::fs::create_dir_all(root.join("build")).unwrap(); + std::fs::write(root.join("build/out.js"), "alpha_vendored\n").unwrap(); + std::fs::write(root.join("src/app.js"), "alpha_vendored\n").unwrap(); + + let out = glob_tool(&root, &json!({"pattern": "**/*.js"})); + assert_eq!(out.output, "src/app.js", "ignored directories are not listed"); + let out = grep_tool(&root, &json!({"pattern": "alpha_vendored"})); + assert_eq!(out.output, "src/app.js:1: alpha_vendored\n", "ignored directories are not searched"); + + // Inside a repository the rules stop at its root as before: a + // .gitignore above it, such as a home directory's "*", must not hide + // the project. + let outer = outside_dir(); + std::fs::create_dir_all(outer.join(".git")).unwrap(); + std::fs::write(outer.join(".gitignore"), "*\n").unwrap(); + let project = outer.join("project"); + std::fs::create_dir_all(project.join(".git")).unwrap(); + std::fs::create_dir_all(project.join("src")).unwrap(); + std::fs::write(project.join("src/lib.rs"), "fn alpha() {}\n").unwrap(); + let out = glob_tool(&project, &json!({"pattern": "**/*.rs"})); + assert_eq!(out.output, "src/lib.rs"); + let out = grep_tool(&project, &json!({"pattern": "alpha"})); + assert_eq!(out.output, "src/lib.rs:1: fn alpha() {}\n"); + let _ = std::fs::remove_dir_all(root); + let _ = std::fs::remove_dir_all(outer); + } + #[test] fn grep_caps_results_with_notice() { let root = temp_project(); @@ -2301,6 +2544,146 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + /// A file over the size cap was refused before offset and limit were + /// read, while the refusal told the model to retry with offset and + /// limit: a retry that could never work. A window of it streams, and + /// shows exactly what the same lines show in a file read whole. + #[test] + fn read_file_reads_a_window_of_a_file_too_large_to_load_whole() { + let root = temp_project(); + let mut lines: Vec = Vec::new(); + let mut bytes = 0; + while bytes <= MAX_FILE_BYTES as usize { + let line = format!("row {} of a large log\n", lines.len() + 1); + bytes += line.len(); + lines.push(line); + } + let total = lines.len(); + let offset = total / 2; + // In the window: a long line whose clip point falls inside a + // three-byte character, and a CRLF line. + lines[offset] = format!("{}\n", "€".repeat(400)); + lines[offset + 1] = "carriage return\r\n".into(); + std::fs::write(root.join("big.log"), lines.concat()).unwrap(); + assert!(std::fs::metadata(root.join("big.log")).unwrap().len() > MAX_FILE_BYTES); + + let out = read_file(&root, &json!({"path": "big.log", "offset": offset, "limit": 4})); + assert!(out.ok, "a window of a large file must be readable: {}", out.output); + let shown: Vec<&str> = out.output.lines().collect(); + assert_eq!(shown.len(), 5, "{}", out.output); + assert_eq!( + shown[4], + format!("… {} more lines (file has {total} lines; continue with offset={})", total - (offset + 3), offset + 4) + ); + // The same lines in a file small enough to read whole render the same. + std::fs::write(root.join("small.log"), lines[..offset + 3].concat()).unwrap(); + let small = read_file(&root, &json!({"path": "small.log", "offset": offset, "limit": 4})); + assert!(small.ok, "{}", small.output); + assert_eq!(shown[..4], small.output.lines().collect::>()[..], "{}", out.output); + assert!(shown[1].contains("[line clipped; 702 more bytes]"), "{}", shown[1]); + assert!(shown[2].ends_with(" carriage return"), "{:?}", shown[2]); + + // The final line is reachable, and an offset past it says so. + let out = read_file(&root, &json!({"path": "big.log", "offset": total})); + assert_eq!(out.output, format!("{total:>5} row {total} of a large log\n")); + let out = read_file(&root, &json!({"path": "big.log", "offset": total + 1})); + assert!(!out.ok && out.output.contains(&format!("past the end of big.log ({total} lines)")), "{}", out.output); + // A whole-file read is still refused, with advice that now works. + let out = read_file(&root, &json!({"path": "big.log"})); + assert!(!out.ok && out.output.contains("file too large"), "{}", out.output); + let _ = std::fs::remove_dir_all(root); + } + + /// A streamed window must number and split lines exactly as a file read + /// whole does, wherever the reader's buffer boundaries fall. + #[test] + fn streamed_lines_split_like_str_lines() { + for text in ["", "a", "a\n", "a\n\n", "a\r\nb", "a\r", "a\r\r\nb\n", "\r\n", "a\rb\n", "héllo\r\nwörld\n"] { + for capacity in [1, 2, 3, 64] { + let mut reader = std::io::BufReader::with_capacity(capacity, text.as_bytes()); + let mut kept = Vec::new(); + let mut lines = Vec::new(); + while let Some(len) = next_line(&mut reader, &mut kept, usize::MAX).unwrap() { + assert_eq!(len, kept.len(), "{text:?} at capacity {capacity}"); + lines.push(String::from_utf8(kept.clone()).unwrap()); + } + assert_eq!(lines, text.lines().collect::>(), "{text:?} at capacity {capacity}"); + } + } + // A long line keeps only its prefix but reports its whole length. + let mut reader = std::io::BufReader::with_capacity(2, "abcdef\r\nxy".as_bytes()); + let mut kept = Vec::new(); + assert_eq!(next_line(&mut reader, &mut kept, 2).unwrap(), Some(6)); + assert_eq!(kept, b"ab"); + assert_eq!(next_line(&mut reader, &mut kept, 0).unwrap(), Some(2)); + assert!(kept.is_empty()); + assert_eq!(next_line(&mut reader, &mut kept, 2).unwrap(), None); + } + + /// read_file discarded the read error and called every failure "not a + /// UTF-8 text file", so a directory or a call with no path sent the + /// model looking for an encoding problem. + #[test] + fn file_tools_name_a_directory_and_a_missing_path() { + let root = temp_project(); + let out = read_file(&root, &json!({"path": "src"})); + assert!(!out.ok, "{}", out.output); + assert_eq!(out.output, "src is a directory; use list_dir to see its entries"); + let out = read_file(&root, &json!({"path": "."})); + assert!(!out.ok && out.output.starts_with(". is a directory"), "{}", out.output); + + let out = read_file(&root, &json!({})); + assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path")); + let out = write_file(&root, &json!({"content": "x\n"})); + assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path")); + let out = edit_file(&root, &json!({"old_string": "a", "new_string": "b"})); + assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path")); + + // A file that is not UTF-8 keeps its own message. + std::fs::write(root.join("latin1.txt"), b"caf\xe9\n").unwrap(); + let out = read_file(&root, &json!({"path": "latin1.txt"})); + assert_eq!((out.ok, out.output.as_str()), (false, "latin1.txt is not a UTF-8 text file")); + let _ = std::fs::remove_dir_all(root); + } + + /// Models often write a number or a boolean as a string. Those + /// arguments were read as JSON numbers and booleans only, so `"5"` and + /// `"true"` were dropped without a word and the defaults ran instead. + #[tokio::test] + async fn numeric_and_boolean_arguments_written_as_strings_are_honored() { + let root = temp_project(); + std::fs::write(root.join("three.txt"), "one\ntwo\nthree\n").unwrap(); + let out = read_file(&root, &json!({"path": "three.txt", "offset": "2", "limit": " 1 "})); + assert!(out.ok, "{}", out.output); + assert!(out.output.starts_with(" 2 two\n"), "offset as a string: {}", out.output); + assert!(!out.output.contains("three\n"), "limit as a string: {}", out.output); + + std::fs::write(root.join("twice.txt"), "x\nx\n").unwrap(); + let out = edit_file(&root, &json!({"path": "twice.txt", "old_string": "x", "new_string": "y", "replace_all": "false"})); + assert!(!out.ok && out.output.contains("matches 2 times"), "{}", out.output); + let out = edit_file(&root, &json!({"path": "twice.txt", "old_string": "x", "new_string": "y", "replace_all": "true"})); + assert!(out.ok, "{}", out.output); + assert_eq!(std::fs::read_to_string(root.join("twice.txt")).unwrap(), "y\ny\n"); + + let out = bash_tool( + &root.join("data"), + &root, + &json!({"command": "sleep 30", "timeout_secs": "2"}), + OutputCaps::default(), + Arc::new(CancelToken::default()), + ) + .await; + assert!(!out.ok && out.output.contains("timed out after 2s"), "{}", out.output); + + // The helper's own shape: anything else still falls back. + assert_eq!(arg(&json!({"n": 7}), "n", Value::as_u64), Some(7)); + assert_eq!(arg(&json!({"n": "True"}), "n", Value::as_bool), Some(true)); + for unusable in [json!({"n": "-1"}), json!({"n": "five"}), json!({"n": null}), json!({})] { + assert_eq!(arg(&unusable, "n", Value::as_u64), None, "{unusable}"); + } + let _ = std::fs::remove_dir_all(root); + } + #[test] fn list_dir_caps_entries() { let root = std::env::temp_dir().join(format!("openmax-listdir-{}", uuid::Uuid::new_v4())); From 36e796bd4e853ef06ca6a1de0278fcd7ff6bd38d Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 21:45:59 -0500 Subject: [PATCH 2/5] fix(core): stop a .gitignore above a project without git from hiding it Outside a repository nothing marks where git's rules stop, so glob and grep read every .gitignore up to the filesystem root. git applies none of them there, and a parent directory's .gitignore that ignores "*" made the whole project look empty. Outside a repository the walker now reads no ignore file above the walk root, and a filter applies the project's own .gitignore files from its root down plus the .ignore files it always read, so a glob or grep scoped below the root keeps the root's rules. read_file's refusal of a file over 1.5 MB still pointed at grep, which skips files that large; it now names only a range read with offset and limit. A blank path in read_file, write_file or edit_file resolved to the project root and produced an error that named nothing; it is now reported as a missing path. --- crates/core/src/tools.rs | 126 ++++++++++++++++++++++++++++++++++----- 1 file changed, 110 insertions(+), 16 deletions(-) diff --git a/crates/core/src/tools.rs b/crates/core/src/tools.rs index 5b9dd549..fc6d1aba 100644 --- a/crates/core/src/tools.rs +++ b/crates/core/src/tools.rs @@ -375,6 +375,12 @@ fn arg(args: &Value, key: &str, native: fn(&Value) -> Opti native(value).or_else(|| value.as_str()?.trim().to_ascii_lowercase().parse().ok()) } +/// The file a read, write or edit names. A blank path names none: resolved, +/// it reached the project root, and the error that followed named nothing. +fn file_path_arg(args: &Value) -> Option<&str> { + args["path"].as_str().filter(|p| !p.trim().is_empty()) +} + pub async fn execute( name: &str, args: &Value, @@ -468,7 +474,7 @@ fn list_dir(root: &Path, args: &Value) -> ToolOutcome { } fn read_file(root: &Path, args: &Value) -> ToolOutcome { - let Some(rel) = args["path"].as_str() else { + let Some(rel) = file_path_arg(args) else { return ToolOutcome::err("missing required argument: path"); }; let path = match resolve(root, rel) { @@ -496,9 +502,13 @@ fn read_file(root: &Path, args: &Value) -> ToolOutcome { } // The cap keeps a whole-file read of a huge file out of memory. A // window of one streams instead: this refusal tells the model to - // ask for one, and refusing that too leaves it nothing to retry. + // ask for one, and refusing that too leaves it nothing to retry. It + // names nothing else, because grep skips a file this large. Ok(m) if m.len() > MAX_FILE_BYTES && !asked_for_window => { - return ToolOutcome::err(format!("file too large ({} bytes); use grep or read with offset/limit", m.len())) + return ToolOutcome::err(format!( + "file too large ({} bytes); read a range of lines with offset and limit", + m.len() + )) } Ok(m) if m.len() > MAX_FILE_BYTES => match stream_window(&path, &mut window) { Ok(total) => total, @@ -707,7 +717,7 @@ fn diff_strings(rel: &str, old: &str, new: &str) -> (DiffInfo, bool) { } fn write_file(root: &Path, args: &Value) -> ToolOutcome { - let Some(rel) = args["path"].as_str() else { + let Some(rel) = file_path_arg(args) else { return ToolOutcome::err("missing required argument: path"); }; let Some(content) = args["content"].as_str() else { @@ -835,7 +845,7 @@ fn to_crlf(text: &str) -> String { } fn edit_file(root: &Path, args: &Value) -> ToolOutcome { - let Some(rel) = args["path"].as_str() else { + let Some(rel) = file_path_arg(args) else { return ToolOutcome::err("missing required argument: path"); }; let (Some(old_string), Some(new_string)) = (args["old_string"].as_str(), args["new_string"].as_str()) else { @@ -911,7 +921,8 @@ fn edit_file(root: &Path, args: &Value) -> ToolOutcome { changed_file("edited", root, &path, &old, &new) } -/// The walk glob and grep share, from `walk_root`. +/// The walk glob and grep share, from `walk_root` in the project whose +/// canonical root is `root_canon`. /// /// Hidden files are searchable: the agent's own extension surface lives in /// dot-directories (`.openmax/tools`, `.agents`, `.github`), and a walker @@ -920,21 +931,52 @@ fn edit_file(root: &Path, args: &Value) -> ToolOutcome { /// /// The walker reads .gitignore only inside a repository by default, which in /// a project without one sent glob and grep through the node_modules and -/// build output it ignores. Outside a repository the rules apply regardless. -/// Inside one the default stays, because only the default stops reading them -/// at the repository's root: without it a .gitignore above the root, such as -/// a home directory kept in git that ignores `*`, would hide the project. -fn project_walker(walk_root: &Path) -> ignore::WalkBuilder { +/// build output it ignores. Inside a repository the default stays: it reads +/// them up to the repository's root and no further, so a .gitignore above +/// it, such as a home directory kept in git that ignores `*`, cannot hide +/// the project. Outside one nothing marks where to stop: told to read them +/// anyway, the walker reads every .gitignore up to the filesystem root, +/// ones git never applies there. So outside a repository it reads no ignore +/// file above the walk root, and the filter applies the ones that count: +/// the project's .gitignore files from its root down, and .ignore files +/// from every ancestor, as the walker always has. +fn project_walker(root_canon: &Path, walk_root: &Path) -> ignore::WalkBuilder { // The markers and the canonical ancestors the walker itself checks. let canon = walk_root.canonicalize().unwrap_or_else(|_| walk_root.to_path_buf()); let in_repository = canon.ancestors().any(|dir| dir.join(".git").exists() || dir.join(".jj").exists()); + // Nearest first, and .ignore ahead of .gitignore, as the walker ranks them. + let mut above: Vec = Vec::new(); + if !in_repository { + let ancestors = || canon.ancestors().skip(1); + let ignores = ancestors().map(|dir| dir.join(".ignore")); + let gitignores = ancestors().take_while(|dir| dir.starts_with(root_canon)).map(|dir| dir.join(".gitignore")); + above = ignores + .chain(gitignores) + .map(|file| ignore::gitignore::Gitignore::new(file).0) + .filter(|rules| !rules.is_empty()) + .collect(); + } + let walk_from = walk_root.to_path_buf(); let mut walker = ignore::WalkBuilder::new(walk_root); walker .hidden(false) - .filter_entry(|e| e.file_name() != std::ffi::OsStr::new(".git")) + .filter_entry(move |e| { + if e.file_name() == std::ffi::OsStr::new(".git") { + return false; + } + if above.is_empty() { + return true; + } + // The rules are rooted at canonical directories, as the walker's own are. + let path = canon.join(e.path().strip_prefix(&walk_from).unwrap_or(e.path())); + let is_dir = e.file_type().is_some_and(|t| t.is_dir()); + let verdict = above.iter().map(|rules| rules.matched(&path, is_dir)).find(|m| !m.is_none()); + !verdict.is_some_and(|m| m.is_ignore()) + }) .git_ignore(true) .git_global(true) .require_git(in_repository) + .parents(in_repository) .max_depth(Some(24)); walker } @@ -1040,7 +1082,7 @@ fn glob_tool(root: &Path, args: &Value) -> ToolOutcome { } } let mut hits: Vec<(std::time::SystemTime, String)> = Vec::new(); - for entry in project_walker(&walk_root).build().flatten() { + for entry in project_walker(&root_canon, &walk_root).build().flatten() { let path = entry.path(); if !path.is_file() || !link_stays_in_root(&entry, &root_canon) { continue; @@ -1113,7 +1155,7 @@ fn grep_tool(root: &Path, args: &Value) -> ToolOutcome { let not_text = AtomicUsize::new(0); let unreadable = AtomicUsize::new(0); let threads = std::thread::available_parallelism().map(|n| n.get()).unwrap_or(4).min(12); - project_walker(&search_root) + project_walker(&root_canon, &search_root) .threads(threads) .build_parallel() .run(|| { @@ -1758,6 +1800,17 @@ mod tests { let out = grep_tool(&root, &json!({"pattern": "alpha_vendored"})); assert_eq!(out.output, "src/app.js:1: alpha_vendored\n", "ignored directories are not searched"); + // A walk that starts below the root still applies the root's rules, + // and a path named outright is searched even though they ignore it. + std::fs::create_dir_all(root.join("src/build")).unwrap(); + std::fs::write(root.join("src/build/gen.js"), "alpha_vendored\n").unwrap(); + let out = glob_tool(&root, &json!({"pattern": "src/**/*.js"})); + assert_eq!(out.output, "src/app.js", "a scoped glob applies the root's .gitignore"); + let out = grep_tool(&root, &json!({"pattern": "alpha_vendored", "path": "src"})); + assert_eq!(out.output, "src/app.js:1: alpha_vendored\n", "a scoped grep applies the root's .gitignore"); + let out = grep_tool(&root, &json!({"pattern": "alpha_vendored", "path": "build"})); + assert_eq!(out.output, "build/out.js:1: alpha_vendored\n", "a named path is searched"); + // Inside a repository the rules stop at its root as before: a // .gitignore above it, such as a home directory's "*", must not hide // the project. @@ -1776,6 +1829,33 @@ mod tests { let _ = std::fs::remove_dir_all(outer); } + /// Outside a repository nothing marks where git's rules stop, so a walker + /// told to read .gitignore there read every one up to the filesystem + /// root. git applies none of them, and one that ignores `*` (a home + /// directory whose dotfiles are tracked from elsewhere) hid the whole + /// project: glob and grep answered as if it were empty. + #[test] + fn a_gitignore_above_a_project_without_git_cannot_hide_it() { + let outer = outside_dir(); + std::fs::write(outer.join(".gitignore"), "*\n").unwrap(); + // A .ignore file applies from any ancestor, as it always has. + std::fs::write(outer.join(".ignore"), "*.secret\n").unwrap(); + let project = outer.join("project"); + std::fs::create_dir_all(project.join("src")).unwrap(); + std::fs::write(project.join("src/lib.rs"), "fn alpha() {}\n").unwrap(); + std::fs::write(project.join("src/key.secret"), "alpha\n").unwrap(); + + for pattern in ["**/*", "src/*"] { + let out = glob_tool(&project, &json!({"pattern": pattern})); + assert_eq!(out.output, "src/lib.rs", "glob {pattern}"); + } + for path in [".", "src"] { + let out = grep_tool(&project, &json!({"pattern": "alpha", "path": path})); + assert_eq!(out.output, "src/lib.rs:1: fn alpha() {}\n", "grep in {path}"); + } + let _ = std::fs::remove_dir_all(outer); + } + #[test] fn grep_caps_results_with_notice() { let root = temp_project(); @@ -2588,9 +2668,13 @@ mod tests { assert_eq!(out.output, format!("{total:>5} row {total} of a large log\n")); let out = read_file(&root, &json!({"path": "big.log", "offset": total + 1})); assert!(!out.ok && out.output.contains(&format!("past the end of big.log ({total} lines)")), "{}", out.output); - // A whole-file read is still refused, with advice that now works. + // A whole-file read is still refused, and the refusal names only the + // window above: grep skips a file this large, so pointing the model + // at it cost a turn on a search that never looked inside. + let len = std::fs::metadata(root.join("big.log")).unwrap().len(); let out = read_file(&root, &json!({"path": "big.log"})); - assert!(!out.ok && out.output.contains("file too large"), "{}", out.output); + let advice = format!("file too large ({len} bytes); read a range of lines with offset and limit"); + assert_eq!((out.ok, out.output.as_str()), (false, advice.as_str())); let _ = std::fs::remove_dir_all(root); } @@ -2638,6 +2722,16 @@ mod tests { assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path")); let out = edit_file(&root, &json!({"old_string": "a", "new_string": "b"})); assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path")); + // A blank path names no file either; resolved, it reached the + // project root and produced an error about a nameless directory. + for blank in ["", " "] { + let out = read_file(&root, &json!({"path": blank})); + assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path"), "{blank:?}"); + let out = write_file(&root, &json!({"path": blank, "content": "x\n"})); + assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path"), "{blank:?}"); + let out = edit_file(&root, &json!({"path": blank, "old_string": "a", "new_string": "b"})); + assert_eq!((out.ok, out.output.as_str()), (false, "missing required argument: path"), "{blank:?}"); + } // A file that is not UTF-8 keeps its own message. std::fs::write(root.join("latin1.txt"), b"caf\xe9\n").unwrap(); From 6cc6054fdf25f7bd55a2f85d54322df74cff2339 Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 22:06:29 -0500 Subject: [PATCH 3/5] fix(core): stop glob and grep from hiding files git would keep Outside a repository the walker reads .gitignore regardless, and that also turned on the user's global excludes, which git applies only inside a repository. A project without git lost every file they name, such as *.log or .env, from glob and grep without a word. Global excludes now apply only inside a repository. In a project without git, a glob or grep scoped below the root pruned any entry a .gitignore above the walk root ignored, even when a nearer .gitignore inside the walk whitelisted it, so the scoped search answered "no matches" for a file the whole-project search found. When a rule from above the walk root would prune an entry, the filter now ranks it with the ignore files from the entry's directory up, nearest first and .ignore ahead of .gitignore, as the walker ranks the files it reads. --- crates/core/src/tools.rs | 144 ++++++++++++++++++++++++++++++++++----- 1 file changed, 128 insertions(+), 16 deletions(-) diff --git a/crates/core/src/tools.rs b/crates/core/src/tools.rs index fc6d1aba..fcaf301a 100644 --- a/crates/core/src/tools.rs +++ b/crates/core/src/tools.rs @@ -936,23 +936,32 @@ fn edit_file(root: &Path, args: &Value) -> ToolOutcome { /// it, such as a home directory kept in git that ignores `*`, cannot hide /// the project. Outside one nothing marks where to stop: told to read them /// anyway, the walker reads every .gitignore up to the filesystem root, -/// ones git never applies there. So outside a repository it reads no ignore -/// file above the walk root, and the filter applies the ones that count: -/// the project's .gitignore files from its root down, and .ignore files -/// from every ancestor, as the walker always has. +/// ones git never applies there, and the user's global excludes, which git +/// applies only inside a repository. So outside a repository it reads no +/// global excludes and no ignore file above the walk root, and the filter +/// applies the ones that count: the project's .gitignore files from its +/// root down, and .ignore files from every ancestor, as the walker always +/// has. +/// +/// The walker ranks the files it reads nearest first, every .ignore ahead +/// of every .gitignore, and the filter ranks the files above the walk root +/// with them. It sees only what the walker passed, though: a .ignore above +/// the walk root that whitelists what a .gitignore inside it ignores +/// cannot bring that entry back. fn project_walker(root_canon: &Path, walk_root: &Path) -> ignore::WalkBuilder { + use ignore::gitignore::Gitignore; // The markers and the canonical ancestors the walker itself checks. let canon = walk_root.canonicalize().unwrap_or_else(|_| walk_root.to_path_buf()); let in_repository = canon.ancestors().any(|dir| dir.join(".git").exists() || dir.join(".jj").exists()); - // Nearest first, and .ignore ahead of .gitignore, as the walker ranks them. - let mut above: Vec = Vec::new(); + let read = |dir: &Path, name: &str| Gitignore::new(dir.join(name)).0; + // The files above the walk root that count, nearest first. + let (mut above_ignore, mut above_gitignore): (Vec, Vec) = Default::default(); if !in_repository { let ancestors = || canon.ancestors().skip(1); - let ignores = ancestors().map(|dir| dir.join(".ignore")); - let gitignores = ancestors().take_while(|dir| dir.starts_with(root_canon)).map(|dir| dir.join(".gitignore")); - above = ignores - .chain(gitignores) - .map(|file| ignore::gitignore::Gitignore::new(file).0) + above_ignore = ancestors().map(|dir| read(dir, ".ignore")).filter(|rules| !rules.is_empty()).collect(); + above_gitignore = ancestors() + .take_while(|dir| dir.starts_with(root_canon)) + .map(|dir| read(dir, ".gitignore")) .filter(|rules| !rules.is_empty()) .collect(); } @@ -964,23 +973,49 @@ fn project_walker(root_canon: &Path, walk_root: &Path) -> ignore::WalkBuilder { if e.file_name() == std::ffi::OsStr::new(".git") { return false; } - if above.is_empty() { + if above_ignore.is_empty() && above_gitignore.is_empty() { return true; } - // The rules are rooted at canonical directories, as the walker's own are. + // Canonical, like the directories the rules above are rooted at. let path = canon.join(e.path().strip_prefix(&walk_from).unwrap_or(e.path())); let is_dir = e.file_type().is_some_and(|t| t.is_dir()); - let verdict = above.iter().map(|rules| rules.matched(&path, is_dir)).find(|m| !m.is_none()); - !verdict.is_some_and(|m| m.is_ignore()) + let above = |files: &[Gitignore]| files.iter().find_map(|rules| ignore_verdict(rules, &path, is_dir)); + if above(&above_ignore).or_else(|| above(&above_gitignore)) != Some(true) { + return true; + } + // The walker passed the entry, so no file it read ignores it, but + // one may whitelist it, and a nearer file outranks a farther one. + // Pruning on the rule above alone made a glob or grep scoped + // below the root miss a file the unscoped one found. Rank the + // files the walker read, from the entry's directory up to the + // walk root, with the ones above it. This runs only for an entry + // about to be pruned. + let below = |name: &str| { + let dirs = path.parent().into_iter().flat_map(Path::ancestors); + dirs.take_while(|dir| dir.starts_with(&canon)) + .find_map(|dir| ignore_verdict(&read(dir, name), &path, is_dir)) + }; + let ignored = below(".ignore") + .or_else(|| above(&above_ignore)) + .or_else(|| below(".gitignore")) + .or_else(|| above(&above_gitignore)); + ignored != Some(true) }) .git_ignore(true) - .git_global(true) + .git_global(in_repository) .require_git(in_repository) .parents(in_repository) .max_depth(Some(24)); walker } +/// Whether `rules` ignore `path` (`Some(true)`), whitelist it +/// (`Some(false)`), or say nothing about it (`None`). +fn ignore_verdict(rules: &ignore::gitignore::Gitignore, path: &Path, is_dir: bool) -> Option { + let matched = rules.matched(path, is_dir); + (!matched.is_none()).then(|| matched.is_ignore()) +} + /// The subtree a glob can possibly match: its literal prefix up to the last /// '/' before the first metacharacter (`src/**/*.rs` → `src/`). Agent-issued /// globs are almost always prefix-scoped, and walking only that subtree @@ -1856,6 +1891,83 @@ mod tests { let _ = std::fs::remove_dir_all(outer); } + /// The walker passes an entry when no ignore file it read ignores it, + /// though one may whitelist it, and a nearer file outranks a farther + /// one. A rule from above a scoped walk pruned such an entry on its own, + /// so a glob or grep scoped below the root answered "no matches" for a + /// file the unscoped one found. + #[test] + fn a_scoped_walk_ranks_ignore_files_as_a_whole_project_walk_does() { + let outer = outside_dir(); + // Every .ignore outranks every .gitignore, however near. + std::fs::write(outer.join(".ignore"), "*.secret\n").unwrap(); + let project = outer.join("project"); + std::fs::create_dir_all(project.join("src/gen")).unwrap(); + std::fs::write(project.join(".gitignore"), "*.gen\n").unwrap(); + // drop.gen stays ignored: nothing nearer than the root's rule names it. + std::fs::write(project.join("src/.gitignore"), "!keep.gen\n!key.secret\n").unwrap(); + for file in ["keep.gen", "drop.gen", "key.secret"] { + std::fs::write(project.join("src/gen").join(file), "needle\n").unwrap(); + } + + for pattern in ["**/*.{gen,secret}", "src/**/*.{gen,secret}", "src/gen/*.{gen,secret}"] { + let out = glob_tool(&project, &json!({"pattern": pattern})); + assert_eq!(out.output, "src/gen/keep.gen", "glob {pattern}"); + } + for path in [".", "src", "src/gen"] { + let out = grep_tool(&project, &json!({"pattern": "needle", "path": path})); + assert_eq!(out.output, "src/gen/keep.gen:1: needle\n", "grep in {path}"); + } + let _ = std::fs::remove_dir_all(outer); + } + + /// git applies the user's global excludes only inside a repository. A + /// walker told to read .gitignore outside one applied them there too, so + /// a project without git lost every file they name (`*.log`, `.env`) + /// from glob and grep without a word. + #[cfg(unix)] + #[test] + fn global_git_excludes_apply_only_inside_a_repository() { + const CHILD: &str = "OPENMAX_TEST_GLOBAL_EXCLUDES"; + if std::env::var_os(CHILD).is_none() { + // The excludes file is found through HOME and XDG_CONFIG_HOME, + // which every test in this process shares: run this test in a + // child whose HOME and XDG_CONFIG_HOME lead to one naming *.log. + let home = outside_dir(); + std::fs::create_dir_all(home.join(".config/git")).unwrap(); + std::fs::write(home.join(".config/git/ignore"), "*.log\n").unwrap(); + let child = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "tools::tests::global_git_excludes_apply_only_inside_a_repository"]) + .env(CHILD, "1") + .env("HOME", &home) + .env("XDG_CONFIG_HOME", home.join(".config")) + .output() + .unwrap(); + let _ = std::fs::remove_dir_all(&home); + let stdout = String::from_utf8_lossy(&child.stdout); + // "1 passed" rules out a filter that matched nothing and exited 0. + assert!( + child.status.success() && stdout.contains("1 passed"), + "{stdout}{}", + String::from_utf8_lossy(&child.stderr) + ); + return; + } + let project = outside_dir(); + std::fs::write(project.join("notes.log"), "alpha\n").unwrap(); + let out = glob_tool(&project, &json!({"pattern": "**/*.log"})); + assert_eq!(out.output, "notes.log", "glob outside a repository"); + let out = grep_tool(&project, &json!({"pattern": "alpha"})); + assert_eq!(out.output, "notes.log:1: alpha\n", "grep outside a repository"); + // Inside one they apply, as git applies them. + std::fs::create_dir_all(project.join(".git")).unwrap(); + let out = glob_tool(&project, &json!({"pattern": "**/*.log"})); + assert_eq!(out.output, "no files matched", "glob inside a repository"); + let out = grep_tool(&project, &json!({"pattern": "alpha"})); + assert_eq!(out.output, "no matches", "grep inside a repository"); + let _ = std::fs::remove_dir_all(project); + } + #[test] fn grep_caps_results_with_notice() { let root = temp_project(); From 4162385f45b5800365f4e24ba50a0013ee2a1b93 Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 22:41:56 -0500 Subject: [PATCH 4/5] fix(core): let a .ignore whitelist outrank a .gitignore in every walk In a project without git the walker read the .gitignore files inside the walk itself, so it pruned an entry they ignored before the filter could rank it against a .ignore above the walk root that whitelists it. A glob or grep scoped below the root then missed a file the whole-project search found, and a .ignore above the project root lost to the project's own .gitignore. The filter also parsed the ignore files of every directory above an entry once for each entry it was about to prune, so a scoped search over many such entries reread the same files thousands of times. Outside a repository the walker now reads only .ignore files, from every ancestor, and the filter applies the project's .gitignore files from its root down, ranked below every .ignore. Each directory's rules are read once per walk and shared by every entry below it. --- crates/core/src/tools.rs | 184 ++++++++++++++++++++++++++------------- 1 file changed, 124 insertions(+), 60 deletions(-) diff --git a/crates/core/src/tools.rs b/crates/core/src/tools.rs index fcaf301a..8b27df2c 100644 --- a/crates/core/src/tools.rs +++ b/crates/core/src/tools.rs @@ -25,6 +25,7 @@ use crate::execution::{ }; use crate::state::CancelToken; +use ignore::gitignore::Gitignore; use serde_json::{json, Value}; use similar::{ChangeTag, DiffTag, TextDiff}; @@ -937,81 +938,114 @@ fn edit_file(root: &Path, args: &Value) -> ToolOutcome { /// the project. Outside one nothing marks where to stop: told to read them /// anyway, the walker reads every .gitignore up to the filesystem root, /// ones git never applies there, and the user's global excludes, which git -/// applies only inside a repository. So outside a repository it reads no -/// global excludes and no ignore file above the walk root, and the filter -/// applies the ones that count: the project's .gitignore files from its -/// root down, and .ignore files from every ancestor, as the walker always -/// has. -/// -/// The walker ranks the files it reads nearest first, every .ignore ahead -/// of every .gitignore, and the filter ranks the files above the walk root -/// with them. It sees only what the walker passed, though: a .ignore above -/// the walk root that whitelists what a .gitignore inside it ignores -/// cannot bring that entry back. +/// applies only inside a repository. So outside a repository the walker +/// reads .ignore files alone, from every ancestor as it always has, and +/// `ProjectGitignores` applies the project's .gitignore files. fn project_walker(root_canon: &Path, walk_root: &Path) -> ignore::WalkBuilder { - use ignore::gitignore::Gitignore; - // The markers and the canonical ancestors the walker itself checks. + // The markers the walker itself checks, on the canonical ancestors. let canon = walk_root.canonicalize().unwrap_or_else(|_| walk_root.to_path_buf()); let in_repository = canon.ancestors().any(|dir| dir.join(".git").exists() || dir.join(".jj").exists()); - let read = |dir: &Path, name: &str| Gitignore::new(dir.join(name)).0; - // The files above the walk root that count, nearest first. - let (mut above_ignore, mut above_gitignore): (Vec, Vec) = Default::default(); - if !in_repository { - let ancestors = || canon.ancestors().skip(1); - above_ignore = ancestors().map(|dir| read(dir, ".ignore")).filter(|rules| !rules.is_empty()).collect(); - above_gitignore = ancestors() - .take_while(|dir| dir.starts_with(root_canon)) - .map(|dir| read(dir, ".gitignore")) - .filter(|rules| !rules.is_empty()) - .collect(); - } - let walk_from = walk_root.to_path_buf(); + let gitignores = (!in_repository).then(|| ProjectGitignores { + root: root_canon.to_path_buf(), + walk_root: walk_root.to_path_buf(), + canon, + dirs: Default::default(), + }); let mut walker = ignore::WalkBuilder::new(walk_root); walker .hidden(false) .filter_entry(move |e| { - if e.file_name() == std::ffi::OsStr::new(".git") { - return false; - } - if above_ignore.is_empty() && above_gitignore.is_empty() { - return true; - } - // Canonical, like the directories the rules above are rooted at. - let path = canon.join(e.path().strip_prefix(&walk_from).unwrap_or(e.path())); - let is_dir = e.file_type().is_some_and(|t| t.is_dir()); - let above = |files: &[Gitignore]| files.iter().find_map(|rules| ignore_verdict(rules, &path, is_dir)); - if above(&above_ignore).or_else(|| above(&above_gitignore)) != Some(true) { - return true; - } - // The walker passed the entry, so no file it read ignores it, but - // one may whitelist it, and a nearer file outranks a farther one. - // Pruning on the rule above alone made a glob or grep scoped - // below the root miss a file the unscoped one found. Rank the - // files the walker read, from the entry's directory up to the - // walk root, with the ones above it. This runs only for an entry - // about to be pruned. - let below = |name: &str| { - let dirs = path.parent().into_iter().flat_map(Path::ancestors); - dirs.take_while(|dir| dir.starts_with(&canon)) - .find_map(|dir| ignore_verdict(&read(dir, name), &path, is_dir)) - }; - let ignored = below(".ignore") - .or_else(|| above(&above_ignore)) - .or_else(|| below(".gitignore")) - .or_else(|| above(&above_gitignore)); - ignored != Some(true) + e.file_name() != std::ffi::OsStr::new(".git") && !gitignores.as_ref().is_some_and(|g| g.drops(e)) }) - .git_ignore(true) + .git_ignore(in_repository) + .git_exclude(in_repository) .git_global(in_repository) - .require_git(in_repository) - .parents(in_repository) .max_depth(Some(24)); walker } +/// The .gitignore files of a project outside a repository, applied to what +/// the walker passes. +/// +/// They apply from the project's root down, and rank as the walker ranks the +/// files it reads: the nearest file that names an entry decides, and every +/// .ignore, from any ancestor, outranks every .gitignore. The walker has +/// applied the .ignore files before an entry gets here, so none of them +/// ignores it, but one may whitelist it, and then no .gitignore drops it. +/// Pruning on a .gitignore the walker read itself, before any .ignore above +/// the walk root was consulted, made a scoped glob or grep miss a file the +/// unscoped one found. +struct ProjectGitignores { + /// The project's canonical root; no .gitignore above it applies. + root: PathBuf, + /// The walk root as the walker reports its entries. + walk_root: PathBuf, + /// The walk root, canonical. + canon: PathBuf, + /// Each directory's rules by canonical path, read once per walk: every + /// entry asks about every directory above it. + dirs: std::sync::RwLock>>, +} + +/// One directory's ignore files, and its parent's. +struct DirRules { + dir: PathBuf, + /// Empty above the project's root. + gitignore: Gitignore, + /// Read only once a .gitignore drops an entry below it. + ignore: OnceLock, + parent: Option>, +} + +impl ProjectGitignores { + /// Whether the project's .gitignore files drop `entry`. + fn drops(&self, entry: &ignore::DirEntry) -> bool { + // Canonical, like the directories the rules are rooted at, as the + // walker's own paths already are when it starts from a canonical root. + let path: std::borrow::Cow = if self.walk_root == self.canon { + entry.path().into() + } else { + self.canon.join(entry.path().strip_prefix(&self.walk_root).unwrap_or(entry.path())).into() + }; + let Some(dir) = path.parent() else { + return false; + }; + let is_dir = entry.file_type().is_some_and(|t| t.is_dir()); + let rules = self.rules(dir); + let chain = || std::iter::successors(Some(&*rules), |d| d.parent.as_deref()); + // A .ignore that whitelists the entry outranks every .gitignore. + let whitelisted = || { + let verdict = |d: &DirRules| { + ignore_verdict(d.ignore.get_or_init(|| read_rules(&d.dir, ".ignore")), &path, is_dir) + }; + chain().find_map(verdict) == Some(false) + }; + chain().find_map(|d| ignore_verdict(&d.gitignore, &path, is_dir)) == Some(true) && !whitelisted() + } + + /// The rules in `dir` and in every directory above it. + fn rules(&self, dir: &Path) -> Arc { + if let Some(rules) = self.dirs.read().unwrap().get(dir) { + return rules.clone(); + } + let rules = Arc::new(DirRules { + dir: dir.to_path_buf(), + gitignore: if dir.starts_with(&self.root) { read_rules(dir, ".gitignore") } else { Gitignore::empty() }, + ignore: OnceLock::new(), + parent: dir.parent().map(|parent| self.rules(parent)), + }); + self.dirs.write().unwrap().entry(dir.to_path_buf()).or_insert(rules).clone() + } +} + +/// The rules in the ignore file `name` in `dir`; none when it is missing. +fn read_rules(dir: &Path, name: &str) -> Gitignore { + Gitignore::new(dir.join(name)).0 +} + /// Whether `rules` ignore `path` (`Some(true)`), whitelist it /// (`Some(false)`), or say nothing about it (`None`). -fn ignore_verdict(rules: &ignore::gitignore::Gitignore, path: &Path, is_dir: bool) -> Option { +fn ignore_verdict(rules: &Gitignore, path: &Path, is_dir: bool) -> Option { let matched = rules.matched(path, is_dir); (!matched.is_none()).then(|| matched.is_ignore()) } @@ -1921,6 +1955,36 @@ mod tests { let _ = std::fs::remove_dir_all(outer); } + /// A .ignore that whitelists an entry outranks a .gitignore that ignores + /// it, wherever each file sits. The walker pruned on a .gitignore inside + /// the walk before a .ignore above the walk root could keep the entry, + /// so a scoped glob or grep missed a file the unscoped one found, and a + /// .ignore above the project root lost to the project's own .gitignore. + #[test] + fn an_ignore_file_above_the_walk_outranks_a_gitignore_inside_it() { + let outer = outside_dir(); + std::fs::write(outer.join(".ignore"), "!keep.out\n").unwrap(); + let project = outer.join("project"); + std::fs::create_dir_all(project.join("src")).unwrap(); + std::fs::write(project.join(".ignore"), "!keep.gen\n").unwrap(); + std::fs::write(project.join("src/.gitignore"), "*.gen\n*.out\n").unwrap(); + for file in ["keep.gen", "drop.gen", "keep.out", "drop.out"] { + std::fs::write(project.join("src").join(file), format!("{file}\n")).unwrap(); + } + + for ext in ["gen", "out"] { + for pattern in [format!("**/*.{ext}"), format!("src/*.{ext}")] { + let out = glob_tool(&project, &json!({"pattern": pattern})); + assert_eq!(out.output, format!("src/keep.{ext}"), "glob {pattern}"); + } + for path in [".", "src"] { + let out = grep_tool(&project, &json!({"pattern": format!("^(keep|drop)\\.{ext}$"), "path": path})); + assert_eq!(out.output, format!("src/keep.{ext}:1: keep.{ext}\n"), "grep {ext} in {path}"); + } + } + let _ = std::fs::remove_dir_all(outer); + } + /// git applies the user's global excludes only inside a repository. A /// walker told to read .gitignore outside one applied them there too, so /// a project without git lost every file they name (`*.log`, `.env`) From d23cf6d9a08c649fb5c9b24925a64be592c2ab96 Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 22:42:49 -0500 Subject: [PATCH 5/5] fix(core): stop a cancelled read of a large file from reading on A range read of a file over 1.5 MB streams the file to its end to count its lines. Cancelling the call stopped the caller from waiting, but the read ran on in the background through the rest of the file, however large, spending disk and CPU on a result nothing would use. The streamed read now checks the call's cancel token before each buffer fill and stops once the call is cancelled. --- crates/core/src/tools.rs | 51 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/crates/core/src/tools.rs b/crates/core/src/tools.rs index 8b27df2c..cfbdaf8a 100644 --- a/crates/core/src/tools.rs +++ b/crates/core/src/tools.rs @@ -404,9 +404,10 @@ pub async fn execute( let name = name.to_string(); let args = args.clone(); let root = root.to_path_buf(); + let task_cancel = cancel.clone(); let task = tokio::task::spawn_blocking(move || match name.as_str() { "list_dir" => list_dir(&root, &args), - "read_file" => read_file(&root, &args), + "read_file" => read_file(&root, &args, &task_cancel), "write_file" => write_file(&root, &args), "edit_file" => edit_file(&root, &args), "glob" => glob_tool(&root, &args), @@ -474,7 +475,7 @@ fn list_dir(root: &Path, args: &Value) -> ToolOutcome { ToolOutcome::ok(output) } -fn read_file(root: &Path, args: &Value) -> ToolOutcome { +fn read_file(root: &Path, args: &Value, cancel: &CancelToken) -> ToolOutcome { let Some(rel) = file_path_arg(args) else { return ToolOutcome::err("missing required argument: path"); }; @@ -511,8 +512,9 @@ fn read_file(root: &Path, args: &Value) -> ToolOutcome { m.len() )) } - Ok(m) if m.len() > MAX_FILE_BYTES => match stream_window(&path, &mut window) { + Ok(m) if m.len() > MAX_FILE_BYTES => match stream_window(&path, &mut window, cancel) { Ok(total) => total, + Err(_) if cancel.is_cancelled() => return ToolOutcome::err("tool cancelled by user"), Err(e) => return read_error(e), }, Ok(_) => { @@ -602,8 +604,12 @@ impl ReadWindow { /// count. Lines are read one at a time and only those in the window are /// decoded, each held to the prefix that `ReadWindow::push` can show, so /// memory stays bounded however long the file or any of its lines runs. -fn stream_window(path: &Path, window: &mut ReadWindow) -> std::io::Result { - let mut reader = std::io::BufReader::new(std::fs::File::open(path)?); +/// +/// The count reads to the end of a file of any size, so the read stops once +/// the call is cancelled: the caller has stopped waiting for it by then. +fn stream_window(path: &Path, window: &mut ReadWindow, cancel: &CancelToken) -> std::io::Result { + let file = Cancellable { file: std::fs::File::open(path)?, cancel }; + let mut reader = std::io::BufReader::new(file); let mut kept = Vec::new(); let mut n = 0; loop { @@ -627,6 +633,21 @@ fn stream_window(path: &Path, window: &mut ReadWindow) -> std::io::Result } } +/// A file whose reads fail once the call reading it is cancelled. +struct Cancellable<'a> { + file: std::fs::File, + cancel: &'a CancelToken, +} + +impl std::io::Read for Cancellable<'_> { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + if self.cancel.is_cancelled() { + return Err(std::io::Error::other("tool cancelled by user")); + } + std::io::Read::read(&mut self.file, buf) + } +} + /// Read the next line into `kept`, keeping at most its first `keep` bytes, /// and return its full length; None at the end of the file. Lines end as /// `str::lines` ends them: at `\n`, with one `\r` before it dropped. @@ -1730,6 +1751,11 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + /// read_file in a call nothing cancels. + fn read_file(root: &Path, args: &Value) -> ToolOutcome { + super::read_file(root, args, &CancelToken::default()) + } + fn temp_project() -> PathBuf { let dir = std::env::temp_dir().join(format!("openmax-tools-{}", uuid::Uuid::new_v4())); std::fs::create_dir_all(&dir).unwrap(); @@ -2854,6 +2880,21 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + /// The caller stops waiting for a read once the call is cancelled, but a + /// window of a large file went on reading to the end of the file to count + /// its lines, however large the file. + #[test] + fn a_cancelled_read_of_a_large_file_stops_reading() { + let root = temp_project(); + let line = "row of a large log\n"; + std::fs::write(root.join("big.log"), line.repeat(MAX_FILE_BYTES as usize / line.len() + 1)).unwrap(); + let cancel = CancelToken::default(); + cancel.cancel(); + let out = super::read_file(&root, &json!({"path": "big.log", "offset": 1, "limit": 1}), &cancel); + assert_eq!((out.ok, out.output.as_str()), (false, "tool cancelled by user")); + let _ = std::fs::remove_dir_all(root); + } + /// A streamed window must number and split lines exactly as a file read /// whole does, wherever the reader's buffer boundaries fall. #[test]