diff --git a/src/tools/files.rs b/src/tools/files.rs index e67d687..ecec5c1 100644 --- a/src/tools/files.rs +++ b/src/tools/files.rs @@ -9,10 +9,22 @@ use tokio::{fs, io::AsyncWriteExt}; /// tells the agent to narrow the path/pattern. const MAX_SHELL_OUTPUT_BYTES: usize = 64 * 1024; +/// Largest single line accumulated while streaming a `read`. `read_until` would +/// buffer a whole line before any trimming, so a no-newline multi-GB file could OOM +/// the service. We keep at most this many bytes per line and drop the rest to the +/// next newline — the agent still sees the line's head, memory stays bounded. +const MAX_READ_LINE_BYTES: usize = 64 * 1024; + /// Read a file, paginated by line AND bounded by bytes (via the shared job-log /// paginator) so neither a huge file nor a single pathological line can flood the /// agent context. +/// +/// Streams line by line, holding at most the requested window (plus the line in +/// flight) — never the whole file. Slurping first and paginating after bounded +/// the reply but not server memory: paging 200 lines of a multi-GB log would +/// have materialized all of it and could OOM the service. pub async fn read(path: &str, cursor: usize, limit: usize) -> Result { + use tokio::io::{AsyncBufReadExt, BufReader}; // A directory can't be read as text: surface a useful redirect to `list` // instead of a raw "Is a directory" errno. match fs::metadata(path).await { @@ -24,25 +36,77 @@ pub async fn read(path: &str, cursor: usize, limit: usize) -> Result {} Err(e) => return Err(e.to_string()), } - // Binary-safe: replace non-UTF-8 bytes with U+FFFD rather than hard-erroring. - let bytes = fs::read(path).await.map_err(|e| e.to_string())?; - let owned = String::from_utf8_lossy(&bytes).into_owned(); - let lines: Vec<&str> = owned.lines().collect(); + let limit = limit.max(1); + let file = fs::File::open(path).await.map_err(|e| e.to_string())?; + let mut reader = BufReader::new(file); + let mut total = 0usize; + let mut window: Vec = Vec::new(); + // The line in flight, capped at MAX_READ_LINE_BYTES: fill_buf/consume lets us + // stop accumulating a pathological no-newline line instead of read_until slurping + // the whole file into one buffer. + let mut line: Vec = Vec::new(); + loop { + let chunk = reader.fill_buf().await.map_err(|e| e.to_string())?; + if chunk.is_empty() { + // EOF: emit a final line that had no terminating newline. + if !line.is_empty() { + if total >= cursor && window.len() < limit { + window.push(String::from_utf8_lossy(&line).into_owned()); + } + total += 1; + } + break; + } + match chunk.iter().position(|&b| b == b'\n') { + Some(nl) => { + append_capped(&mut line, &chunk[..nl]); + reader.consume(nl + 1); + if line.last() == Some(&b'\r') { + line.pop(); + } + if total >= cursor && window.len() < limit { + // Binary-safe: replace non-UTF-8 bytes with U+FFFD, like `lines()`. + window.push(String::from_utf8_lossy(&line).into_owned()); + } + total += 1; + line.clear(); + } + None => { + let n = chunk.len(); + append_capped(&mut line, chunk); + reader.consume(n); + } + } + } // Forward pagination from the top (cursor 0 = first line): a file is read // start-to-end. (`job poll` instead reads newest-first — a live log's latest - // output matters most.) Same byte/line ceilings via `paginate`. - let page = crate::jobs::paginate(&lines, cursor, limit); + // output matters most.) Same byte/line ceilings via `paginate`, applied to + // the pre-cut window (so the byte cap can shorten the page further). + let refs: Vec<&str> = window.iter().map(String::as_str).collect(); + let page = crate::jobs::paginate(&refs, 0, limit); let body = page.lines.join("\n"); - if page.has_more { + let next = cursor.min(total) + page.lines.len(); + if next < total { Ok(format!( - "{body}\n[lines {cursor}..{} of {}; next_cursor={}]", - page.next_cursor, page.total_lines, page.next_cursor + "{body}\n[lines {cursor}..{next} of {total}; next_cursor={next}]" )) } else { Ok(body) } } +/// Append `bytes` to the in-flight line without letting it grow past +/// `MAX_READ_LINE_BYTES`. Overflow bytes are dropped (the line is already longer +/// than any page will show), so one pathological no-newline line can't grow the +/// buffer without bound. +fn append_capped(line: &mut Vec, bytes: &[u8]) { + let room = MAX_READ_LINE_BYTES.saturating_sub(line.len()); + if room == 0 { + return; + } + line.extend_from_slice(&bytes[..room.min(bytes.len())]); +} + pub async fn write(path: &str, content: &str) -> Result { ensure_parent(path).await?; fs::write(path, content).await.map_err(|e| e.to_string())?; @@ -66,7 +130,13 @@ pub async fn append(path: &str, content: &str) -> Result { } pub async fn delete(path: &str) -> Result { - let meta = fs::metadata(path).await.map_err(|e| e.to_string())?; + // symlink_metadata, not metadata: a symlink must be unlinked, never followed. + // Following classified a dir-symlink as a directory (remove_dir_all refuses + // the top-level link) and made a dangling symlink undeletable (ENOENT on the + // stat before any removal was attempted). + let meta = fs::symlink_metadata(path) + .await + .map_err(|e| e.to_string())?; let r = if meta.is_dir() { fs::remove_dir_all(path).await } else { @@ -99,28 +169,76 @@ async fn ensure_parent(path: &str) -> Result<(), String> { pub async fn list(path: &str, recursive: bool) -> Result { if recursive { - sh("find", &[path]).await + // `find` has no `--`; anchor a leading-dash relative path with `./` so + // it can't be parsed as an expression. + let path = if path.starts_with('-') { + format!("./{path}") + } else { + path.to_string() + }; + sh("find", &[&path]).await.map_err(|e| e.to_string()) } else { - sh("ls", &["-la", path]).await + sh("ls", &["-la", "--", path]) + .await + .map_err(|e| e.to_string()) } } pub async fn grep(pattern: &str, path: &str, recursive: bool) -> Result { let flag = if recursive { "-rn" } else { "-n" }; - sh("grep", &[flag, pattern, path]).await + // `--` so a pattern like `->` or `-r` is a pattern, not an option: without + // it, grepping Rust code for `->` errored, and `-r` silently recursed with + // the *path* as the pattern — wrong results, not even an error. + match sh("grep", &[flag, "--", pattern, path]).await { + Ok(s) => Ok(s), + // Exit 1 is grep's "no line matched" — a legitimate empty result. The + // marker keeps it distinguishable from matching an empty line. + Err(ShError::Status { code: 1, out }) if out.is_empty() => { + Ok("[grep: no matches]".to_string()) + } + Err(e) => Err(e.to_string()), + } } -async fn sh(prog: &str, args: &[&str]) -> Result { +/// A shelled-out command that didn't produce a clean result. Exit status and +/// combined output stay separate so callers can special-case a status (grep's +/// exit-1-means-no-matches) without parsing message text. +#[derive(Debug, thiserror::Error)] +enum ShError { + #[error("{0}")] + Spawn(String), + #[error("exit status {code}: {out}")] + Status { code: i32, out: String }, + // A signal death carries no exit code: its own variant keeps it distinct from a + // real status so the grep-exit-1 path can never accidentally match it. + #[error("killed by signal: {out}")] + Signal { out: String }, +} + +/// Run `prog` and return its combined stdout+stderr — as `Ok` only on exit 0. +/// A failed command must surface as an error, not as a success whose body +/// happens to contain `ls: cannot access ...`. +async fn sh(prog: &str, args: &[&str]) -> Result { let out = tokio::process::Command::new(prog) .args(args) .output() .await - .map_err(|e| e.to_string())?; + .map_err(|e| ShError::Spawn(e.to_string()))?; let mut s = String::from_utf8_lossy(&out.stdout).into_owned(); if !out.stderr.is_empty() { s.push_str(&String::from_utf8_lossy(&out.stderr)); } - Ok(cap_bytes(s, MAX_SHELL_OUTPUT_BYTES)) + let s = cap_bytes(s, MAX_SHELL_OUTPUT_BYTES); + if out.status.success() { + Ok(s) + } else { + // No exit code means a signal killed the process — model it as its own + // variant rather than folding it into a sentinel status. + Err(match out.status.code() { + Some(code) => ShError::Status { code, out: s }, + None => ShError::Signal { out: s }, + }) + } } /// Bound a non-paginated listing to `max` bytes, cut on a UTF-8 boundary, with a @@ -142,142 +260,4 @@ fn cap_bytes(mut s: String, max: usize) -> String { } #[cfg(test)] -mod tests { - use super::*; - - #[tokio::test] - async fn write_read_paginate_append_move_delete() { - let dir = tempfile::tempdir().unwrap(); - let a = dir.path().join("a.txt"); - let a = a.to_str().unwrap(); - - write(a, "l1\nl2\nl3").await.unwrap(); - let page = read(a, 0, 2).await.unwrap(); - assert!(page.contains("l1") && page.contains("l2") && !page.contains("l3")); - assert!(page.contains("next_cursor=2")); - - append(a, "\nl4").await.unwrap(); - assert!(read(a, 0, 100).await.unwrap().contains("l4")); - - let b = dir.path().join("b.txt"); - let b = b.to_str().unwrap(); - rename(a, b).await.unwrap(); - assert!(read(a, 0, 10).await.is_err()); - - delete(b).await.unwrap(); - assert!(read(b, 0, 10).await.is_err()); - } - - #[tokio::test] - async fn write_creates_missing_parent_dirs() { - let dir = tempfile::tempdir().unwrap(); - // Two levels that don't exist yet — write must `mkdir -p` them. - let nested = dir.path().join("a/b/c.txt"); - let nested = nested.to_str().unwrap(); - write(nested, "hi").await.unwrap(); - assert_eq!(read(nested, 0, 10).await.unwrap(), "hi"); - - // append to a fresh path under a new dir works too. - let ap = dir.path().join("x/y/z.log"); - let ap = ap.to_str().unwrap(); - append(ap, "one\n").await.unwrap(); - assert!(read(ap, 0, 10).await.unwrap().contains("one")); - } - - #[tokio::test] - async fn read_on_directory_redirects_to_list() { - let dir = tempfile::tempdir().unwrap(); - let err = read(dir.path().to_str().unwrap(), 0, 10).await.unwrap_err(); - assert!( - err.contains("is a directory") && err.contains("list"), - "dir read should redirect to list: {err}" - ); - } - - #[tokio::test] - async fn grep_finds_match() { - let dir = tempfile::tempdir().unwrap(); - let c = dir.path().join("c.txt"); - let c = c.to_str().unwrap(); - write(c, "alpha\nbeta\ngamma").await.unwrap(); - assert!(grep("beta", c, false).await.unwrap().contains("beta")); - } - - #[tokio::test] - async fn list_non_recursive_shows_top_level_only() { - let dir = tempfile::tempdir().unwrap(); - let sub = dir.path().join("sub"); - tokio::fs::create_dir(&sub).await.unwrap(); - let f_top = dir.path().join("top.txt"); - let f_nested = sub.join("nested.txt"); - write(f_top.to_str().unwrap(), "top").await.unwrap(); - write(f_nested.to_str().unwrap(), "nested").await.unwrap(); - - let out = list(dir.path().to_str().unwrap(), false).await.unwrap(); - assert!(out.contains("top.txt"), "should list top-level file: {out}"); - assert!(out.contains("sub"), "should list sub dir: {out}"); - assert!(!out.contains("nested.txt"), "should not recurse: {out}"); - } - - #[tokio::test] - async fn list_recursive_finds_nested_files() { - let dir = tempfile::tempdir().unwrap(); - let sub = dir.path().join("sub"); - tokio::fs::create_dir(&sub).await.unwrap(); - let f_nested = sub.join("deep.txt"); - write(f_nested.to_str().unwrap(), "deep").await.unwrap(); - - let out = list(dir.path().to_str().unwrap(), true).await.unwrap(); - assert!( - out.contains("deep.txt"), - "recursive find should reach nested file: {out}" - ); - } - - #[tokio::test] - async fn delete_directory_removes_entire_tree() { - let dir = tempfile::tempdir().unwrap(); - let sub = dir.path().join("to_delete"); - tokio::fs::create_dir(&sub).await.unwrap(); - let f = sub.join("file.txt"); - write(f.to_str().unwrap(), "content").await.unwrap(); - - delete(sub.to_str().unwrap()).await.unwrap(); - assert!( - !sub.exists(), - "directory and its contents should be removed" - ); - } - - #[tokio::test] - async fn grep_recursive_finds_match_in_subdirs() { - let dir = tempfile::tempdir().unwrap(); - let sub = dir.path().join("sub"); - tokio::fs::create_dir(&sub).await.unwrap(); - let f = sub.join("d.txt"); - write(f.to_str().unwrap(), "alpha\nbeta\ngamma") - .await - .unwrap(); - - let out = grep("beta", dir.path().to_str().unwrap(), true) - .await - .unwrap(); - assert!( - out.contains("beta"), - "recursive grep should find pattern in subdir: {out}" - ); - } - - #[tokio::test] - async fn binary_file_reads_as_lossy_utf8_not_error() { - let dir = tempfile::tempdir().unwrap(); - let p = dir.path().join("bin.dat"); - // Write bytes that are not valid UTF-8. - tokio::fs::write(&p, b"hello\xff\xfeworld\n").await.unwrap(); - let result = read(p.to_str().unwrap(), 0, 100).await; - assert!(result.is_ok(), "binary read should not hard-error"); - let content = result.unwrap(); - assert!(content.contains("hello"), "ASCII prefix should survive"); - assert!(content.contains("world"), "ASCII suffix should survive"); - } -} +mod tests; diff --git a/src/tools/files/tests.rs b/src/tools/files/tests.rs new file mode 100644 index 0000000..34f6fcb --- /dev/null +++ b/src/tools/files/tests.rs @@ -0,0 +1,231 @@ +use super::*; + +#[tokio::test] +async fn write_read_paginate_append_move_delete() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("a.txt"); + let a = a.to_str().unwrap(); + + write(a, "l1\nl2\nl3").await.unwrap(); + let page = read(a, 0, 2).await.unwrap(); + assert!(page.contains("l1") && page.contains("l2") && !page.contains("l3")); + assert!(page.contains("next_cursor=2")); + + append(a, "\nl4").await.unwrap(); + assert!(read(a, 0, 100).await.unwrap().contains("l4")); + + let b = dir.path().join("b.txt"); + let b = b.to_str().unwrap(); + rename(a, b).await.unwrap(); + assert!(read(a, 0, 10).await.is_err()); + + delete(b).await.unwrap(); + assert!(read(b, 0, 10).await.is_err()); +} + +#[tokio::test] +async fn write_creates_missing_parent_dirs() { + let dir = tempfile::tempdir().unwrap(); + // Two levels that don't exist yet — write must `mkdir -p` them. + let nested = dir.path().join("a/b/c.txt"); + let nested = nested.to_str().unwrap(); + write(nested, "hi").await.unwrap(); + assert_eq!(read(nested, 0, 10).await.unwrap(), "hi"); + + // append to a fresh path under a new dir works too. + let ap = dir.path().join("x/y/z.log"); + let ap = ap.to_str().unwrap(); + append(ap, "one\n").await.unwrap(); + assert!(read(ap, 0, 10).await.unwrap().contains("one")); +} + +#[tokio::test] +async fn read_on_directory_redirects_to_list() { + let dir = tempfile::tempdir().unwrap(); + let err = read(dir.path().to_str().unwrap(), 0, 10).await.unwrap_err(); + assert!( + err.contains("is a directory") && err.contains("list"), + "dir read should redirect to list: {err}" + ); +} + +#[tokio::test] +async fn oversized_no_newline_line_is_capped_not_slurped() { + // A multi-MiB file with no newline must not be slurped into one buffer: the + // per-line cap plus paginate's clamp keep the reply (and memory) bounded. + let dir = tempfile::tempdir().unwrap(); + let p = dir.path().join("huge.txt"); + let big = vec![b'a'; 2 * 1024 * 1024]; + tokio::fs::write(&p, &big).await.unwrap(); + let out = read(p.to_str().unwrap(), 0, 100).await.unwrap(); + assert!( + out.len() <= 16 * 1024, + "oversized no-newline line must be bounded, got {} bytes", + out.len() + ); + assert!( + out.starts_with("aaa"), + "the head of the line should survive" + ); +} + +#[tokio::test] +async fn grep_finds_match() { + let dir = tempfile::tempdir().unwrap(); + let c = dir.path().join("c.txt"); + let c = c.to_str().unwrap(); + write(c, "alpha\nbeta\ngamma").await.unwrap(); + assert!(grep("beta", c, false).await.unwrap().contains("beta")); +} + +#[tokio::test] +async fn grep_pattern_starting_with_dash_is_a_pattern_not_an_option() { + // An agent grepping Rust code for `->` (or worse, `-r`) must get matches, + // not `grep: invalid option` or a silent argument shift. + let dir = tempfile::tempdir().unwrap(); + let c = dir.path().join("code.rs"); + let c = c.to_str().unwrap(); + write(c, "fn f() -> i32 { 0 }").await.unwrap(); + let out = grep("->", c, false).await.unwrap(); + assert!(out.contains("-> i32"), "dash pattern must match: {out}"); + let out = grep("-r", c, false).await.unwrap(); + assert!( + out.contains("no matches"), + "`-r` is a pattern with no hits, not a flag: {out}" + ); +} + +#[tokio::test] +async fn grep_no_matches_is_ok_and_distinguishable() { + let dir = tempfile::tempdir().unwrap(); + let c = dir.path().join("c.txt"); + let c = c.to_str().unwrap(); + write(c, "alpha").await.unwrap(); + let out = grep("zzz", c, false).await.unwrap(); + assert!( + out.contains("no matches"), + "zero matches is a result, not an error or a blank page: {out}" + ); +} + +#[tokio::test] +async fn failed_shell_command_is_an_error_not_a_success() { + // `ls` on a missing path exits non-zero; that must surface as Err, not as + // an Ok body containing the error text. + assert!(list("/does/not/exist-mcp-ssh-test", false).await.is_err()); + assert!(list("/does/not/exist-mcp-ssh-test", true).await.is_err()); + assert!( + grep("x", "/does/not/exist-mcp-ssh-test", false) + .await + .is_err() + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn delete_unlinks_symlinks_instead_of_following() { + let dir = tempfile::tempdir().unwrap(); + // Symlink to a directory: delete must unlink the link, keep the target. + let target = dir.path().join("target"); + tokio::fs::create_dir(&target).await.unwrap(); + write(target.join("keep.txt").to_str().unwrap(), "keep") + .await + .unwrap(); + let link = dir.path().join("link"); + tokio::fs::symlink(&target, &link).await.unwrap(); + delete(link.to_str().unwrap()).await.unwrap(); + assert!(!link.exists(), "the link itself must be gone"); + assert!( + target.join("keep.txt").exists(), + "the target must be untouched" + ); + + // Dangling symlink: previously undeletable (stat followed it to ENOENT). + let dangling = dir.path().join("dangling"); + tokio::fs::symlink(dir.path().join("nope"), &dangling) + .await + .unwrap(); + delete(dangling.to_str().unwrap()).await.unwrap(); + assert!( + tokio::fs::symlink_metadata(&dangling).await.is_err(), + "dangling link must be removed" + ); +} + +#[tokio::test] +async fn list_non_recursive_shows_top_level_only() { + let dir = tempfile::tempdir().unwrap(); + let sub = dir.path().join("sub"); + tokio::fs::create_dir(&sub).await.unwrap(); + let f_top = dir.path().join("top.txt"); + let f_nested = sub.join("nested.txt"); + write(f_top.to_str().unwrap(), "top").await.unwrap(); + write(f_nested.to_str().unwrap(), "nested").await.unwrap(); + + let out = list(dir.path().to_str().unwrap(), false).await.unwrap(); + assert!(out.contains("top.txt"), "should list top-level file: {out}"); + assert!(out.contains("sub"), "should list sub dir: {out}"); + assert!(!out.contains("nested.txt"), "should not recurse: {out}"); +} + +#[tokio::test] +async fn list_recursive_finds_nested_files() { + let dir = tempfile::tempdir().unwrap(); + let sub = dir.path().join("sub"); + tokio::fs::create_dir(&sub).await.unwrap(); + let f_nested = sub.join("deep.txt"); + write(f_nested.to_str().unwrap(), "deep").await.unwrap(); + + let out = list(dir.path().to_str().unwrap(), true).await.unwrap(); + assert!( + out.contains("deep.txt"), + "recursive find should reach nested file: {out}" + ); +} + +#[tokio::test] +async fn delete_directory_removes_entire_tree() { + let dir = tempfile::tempdir().unwrap(); + let sub = dir.path().join("to_delete"); + tokio::fs::create_dir(&sub).await.unwrap(); + let f = sub.join("file.txt"); + write(f.to_str().unwrap(), "content").await.unwrap(); + + delete(sub.to_str().unwrap()).await.unwrap(); + assert!( + !sub.exists(), + "directory and its contents should be removed" + ); +} + +#[tokio::test] +async fn grep_recursive_finds_match_in_subdirs() { + let dir = tempfile::tempdir().unwrap(); + let sub = dir.path().join("sub"); + tokio::fs::create_dir(&sub).await.unwrap(); + let f = sub.join("d.txt"); + write(f.to_str().unwrap(), "alpha\nbeta\ngamma") + .await + .unwrap(); + + let out = grep("beta", dir.path().to_str().unwrap(), true) + .await + .unwrap(); + assert!( + out.contains("beta"), + "recursive grep should find pattern in subdir: {out}" + ); +} + +#[tokio::test] +async fn binary_file_reads_as_lossy_utf8_not_error() { + let dir = tempfile::tempdir().unwrap(); + let p = dir.path().join("bin.dat"); + // Write bytes that are not valid UTF-8. + tokio::fs::write(&p, b"hello\xff\xfeworld\n").await.unwrap(); + let result = read(p.to_str().unwrap(), 0, 100).await; + assert!(result.is_ok(), "binary read should not hard-error"); + let content = result.unwrap(); + assert!(content.contains("hello"), "ASCII prefix should survive"); + assert!(content.contains("world"), "ASCII suffix should survive"); +}