diff --git a/CHANGELOG.md b/CHANGELOG.md index 38c65bf..b7dc029 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,7 @@ adheres to [Semantic Versioning](https://semver.org/). - **Responses / Azure, encrypted reasoning:** a reasoning item whose following output item has no id is no longer stored or replayed (warned) — it would 400 every later request — and an unparseable reasoning `output_item.done` ends the previous item's range. - **yoagent-rutis: a TypeScript/Python handler's `on_event` delivery no longer fails silently.** A panic during delivery is caught, and a delivery task that ended (seen on `send` or `flush`) is recorded; either is the handler's failure (a required extension fails the run, an advisory one logs it). Before, the remaining events were dropped and the run never failed. - **A cancelled run no longer blames an extension for a withheld tool result.** When the run is cancelled while an extension's `after_tool` is still working, the result is still withheld (a redactor may not have run), but the text now says the run was cancelled before the extension finished, instead of "extension '…' did not finish processing it", which reads like the extension failed. +- **`list_files` reports what it couldn't read** (#260, found by yoyo). `find` skips an unreadable subdirectory, prints the error on stderr and exits non-zero; the tool ignored both, so a partial listing read as complete. The listed files are still returned, now with the errors under `Warnings` in the text and in `details.warnings` (capped); a listing with no files and an error fails, as `search` does. ## 0.24.3 diff --git a/CLAUDE.md b/CLAUDE.md index e3da8fb..86cf1d7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -239,7 +239,7 @@ Tests that mutate the global layers or assert on logs live in their own serializ - `PathSandbox::resolve` anchors relative paths at `current_dir()` first, keeps `..` components, re-resolves a path that climbs through a missing directory, and follows a dangling symlink (up to `MAX_SYMLINK_HOPS`; `None` means refused). `tests/sandbox_cwd_test.rs` changes the cwd, so it is its own binary. - Sandboxed tools do their I/O on `PathSandbox::io_path` (the checked path); unsandboxed tools use the caller's spelling. - `search` passes `--regexp=` / `-e`, then `--`, then the path. It has no per-file `--max-count`; it reads lines until `max_results` and kills the search at the next one (`details.truncated`). An exit 2 (some file errored) with matches returns the matches plus stderr under `Warnings:` (`details.warnings` = that text, capped, or null; kept on the early-stop path too); only an error with no matches fails. - - `list_files` guards `find` with `not_an_option`. + - `list_files` guards `find` with `not_an_option`. `find`'s stderr is read: errors (an unreadable subdirectory) are appended to the listing under `Warnings` and in `details.warnings` (capped), so a partial listing never reads as complete; only an error with no files fails (#260). - `bash` spawns with `kill_on_drop`, which kills the `bash` process only, and with `stdin(Stdio::null())` (`spawn` inherits stdin, while `output()` did not; `search` sets it too). Each stream goes through `Capture` (bytes kept ≤ `max_output_bytes`, the rest drained; a cut flag and a read error survive a timeout), and output is decoded after cutting. - Tool `execute` is built and awaited inside `catch_unwind` in `execute_single_tool`, the same as middleware. A panic is an error result carrying the payload text, logged at `error!` with the `tool_call_id`. - The Gemini key goes in `x-goog-api-key`, trimmed, never the URL. It is skipped when the user's headers set `x-goog-api-key`, or when the key is empty. An `Authorization` header doesn't count. diff --git a/docs/reference/tools.md b/docs/reference/tools.md index 1a662a3..4161c53 100644 --- a/docs/reference/tools.md +++ b/docs/reference/tools.md @@ -107,7 +107,7 @@ pub struct ListFilesTool { } ``` -Uses `find`, skipping `target/`, `.git/` and `node_modules/`. +Uses `find`, skipping `target/`, `.git/` and `node_modules/`. Paths `find` can't read (a subdirectory without permission) don't fail the listing: the files it did find are returned, with the errors under `Warnings` and in `details.warnings`, so a partial listing is never presented as complete. A listing with no files and an error is a failure. ## SearchTool diff --git a/src/tools/list.rs b/src/tools/list.rs index 036afec..74209ec 100644 --- a/src/tools/list.rs +++ b/src/tools/list.rs @@ -132,13 +132,29 @@ impl AgentTool for ListFilesTool { let mut lines: Vec<&str> = stdout.lines().collect(); lines.sort(); + // `find` keeps walking past what it can't read (a subdirectory without + // permission, a file removed mid-walk), reports it on stderr and exits + // non-zero. The files it did list are the answer, with those errors as + // warnings, so a partial listing never reads as complete. Only a + // listing with nothing to show is a failure. + let stderr = String::from_utf8_lossy(&result.stderr); + let stderr = stderr.trim(); + // Capped on both paths: a wide walk can produce thousands of lines. + let warnings = (!stderr.is_empty()).then(|| match stderr.char_indices().nth(2000) { + Some((cut, _)) => format!("{}\n... (more warnings not shown)", &stderr[..cut]), + None => stderr.to_string(), + }); + if let (false, true, Some(w)) = (result.status.success(), lines.is_empty(), &warnings) { + return Err(ToolError::Failed(format!("Listing error: {w}"))); + } + let total = lines.len(); let truncated = total > self.max_results; if truncated { lines.truncate(self.max_results); } - let text = if lines.is_empty() { + let mut text = if lines.is_empty() { format!("No files found in {}", path) } else if truncated { format!( @@ -151,9 +167,19 @@ impl AgentTool for ListFilesTool { format!("{}\n\n({} files)", lines.join("\n"), total) }; + if let Some(w) = &warnings { + text.push_str(&format!( + "\nWarnings from find (the listing may be incomplete):\n{w}" + )); + } + Ok(ToolResult { content: vec![Content::Text { text }], - details: serde_json::json!({ "total": total, "truncated": truncated }), + details: serde_json::json!({ + "total": total, + "truncated": truncated, + "warnings": warnings, + }), }) } } diff --git a/tests/tools_test.rs b/tests/tools_test.rs index ce3cd84..dd3918e 100644 --- a/tests/tools_test.rs +++ b/tests/tools_test.rs @@ -584,6 +584,62 @@ async fn test_list_files_tool() { let _ = std::fs::remove_dir_all(tmp_dir); } +/// An unreadable subdirectory doesn't make a partial listing look complete +/// (#260): the readable files are listed, and `find`'s error is reported as a +/// warning in the text and in `details.warnings`. Control: a fully readable +/// tree has no warnings. +#[cfg(unix)] +#[tokio::test] +async fn list_files_reports_unreadable_subdirectories() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a.rs"), "").unwrap(); + let locked = dir.path().join("locked"); + std::fs::create_dir(&locked).unwrap(); + std::fs::write(locked.join("hidden.rs"), "").unwrap(); + let list = |path: std::path::PathBuf| async move { + ListFilesTool::new() + .execute( + serde_json::json!({"path": path.to_str().unwrap()}), + ctx("list_files"), + ) + .await + .unwrap() + }; + + // Control: everything readable, no warnings. + let ok = list(dir.path().to_path_buf()).await; + assert!(ok.details["warnings"].is_null(), "{:?}", ok.details); + + // Restores the permissions however the test ends, so the temp dir can be removed. + struct Unlock(std::path::PathBuf); + impl Drop for Unlock { + fn drop(&mut self) { + let _ = std::fs::set_permissions(&self.0, std::fs::Permissions::from_mode(0o755)); + } + } + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000)).unwrap(); + let _unlock = Unlock(locked.clone()); + if std::fs::read_dir(&locked).is_ok() { + // Running as root: permissions don't apply, nothing to test. + eprintln!("skipped: permissions are not enforced for this user"); + return; + } + let result = list(dir.path().to_path_buf()).await; + let text = match &result.content[0] { + Content::Text { text } => text.clone(), + _ => panic!("expected text"), + }; + assert!(text.contains("a.rs"), "{text}"); + assert!(text.contains("Warnings"), "{text}"); + assert!(text.contains("locked"), "{text}"); + assert!( + result.details["warnings"].is_string(), + "{:?}", + result.details + ); +} + #[tokio::test] async fn test_read_file_line_numbers() { let tmp = std::env::temp_dir().join("yoagent-test-lineno2.txt");