From 078173de1a828577e95f18361b8f59935812e750 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 27 Jul 2026 12:22:28 +0200 Subject: [PATCH 1/6] chore(tools): Suppress noisy/sensitive paths from fs tool results The `fs_list_files`, `fs_grep_files`, and `fs_read_file` tools gain an `options.suppress` list of `.ignore`-style patterns for paths they may read but must never hand back to the model. `.git/` and `**/target/` are suppressed by default across all of them, so a search or listing no longer floods the model with build output or exposes repository internals it has no business seeing. Read access to those paths is unaffected: `access.fs` still governs what the tool process may touch, which is why `fs_modify_file` can keep running `git status` against a suppressed `.git` to check for uncommitted work. A path skipped for suppression, or because the access policy denies read, is no longer silently dropped from listings and searches. The result now names what was skipped and why, and points at asking the user for the content instead of letting an empty result read as evidence the content does not exist. Explicitly naming a path (e.g. `fs_read_file` on an exact file) still reaches it even when a `.ignore` rule would otherwise prune it from an unscoped listing; only `suppress` and the access policy can withhold an explicitly named path. `fs_list_files` and `fs_grep_files` also correctly handle a prefix naming a file directly, and a prefix naming a directory whose pruning rule lives below the workspace root (e.g. a nested `.ignore`), by walking that subtree as its own root instead of silently returning nothing. Access-denial error messages from the fs tools now list only the paths that grant the specific denied capability, rather than every configured grant path, and point the model at asking the user for access rather than leaving the refusal as a dead end. Signed-off-by: Jean Mertz --- .config/jp/tools/src/fs.rs | 13 + .config/jp/tools/src/fs/delete_file_tests.rs | 40 +- .config/jp/tools/src/fs/grep_files.rs | 52 ++- .config/jp/tools/src/fs/grep_files_tests.rs | 119 ++++- .config/jp/tools/src/fs/list_files.rs | 446 +++++++++++++----- .config/jp/tools/src/fs/list_files_tests.rs | 451 ++++++++++++++++++- .config/jp/tools/src/fs/move_file_tests.rs | 66 ++- .config/jp/tools/src/fs/read_file.rs | 9 +- .config/jp/tools/src/fs/read_file_tests.rs | 100 +++- .config/jp/tools/src/fs/utils.rs | 99 ++-- .config/jp/tools/src/fs/utils_tests.rs | 82 ---- .ignore | 16 +- .jp/mcp/tools/fs/grep_files.toml | 11 +- .jp/mcp/tools/fs/grep_user_docs.toml | 26 +- .jp/mcp/tools/fs/list_files.toml | 11 +- .jp/mcp/tools/fs/read_file.toml | 9 + Cargo.lock | 18 +- crates/jp_tool/src/access.rs | 31 +- 18 files changed, 1289 insertions(+), 310 deletions(-) diff --git a/.config/jp/tools/src/fs.rs b/.config/jp/tools/src/fs.rs index fb531d76a..682e7c23e 100644 --- a/.config/jp/tools/src/fs.rs +++ b/.config/jp/tools/src/fs.rs @@ -1,3 +1,5 @@ +use utils::suppress_matcher; + use crate::{ Context, Tool, to_xml, util::{OneOrMany, ToolResult}, @@ -21,12 +23,20 @@ use move_file::fs_move_file; use read_file::fs_read_file; pub async fn run(ctx: Context, t: Tool) -> ToolResult { + // Paths these tools may read but never return. Honored by the tools that exist + // to hand file contents or paths back; the write tools return confirmations + // rather than content, and what they may touch is the access policy's + // question. + let patterns: Vec = t.option_or("suppress", vec![]); + let suppress = suppress_matcher(&ctx.root, &patterns); + match t.name.trim_start_matches("fs_") { "list_files" => fs_list_files( &ctx.root, ctx.access.as_ref(), t.opt("prefixes")?, t.opt("extensions")?, + &suppress, ) .await .and_then(to_xml) @@ -35,6 +45,7 @@ pub async fn run(ctx: Context, t: Tool) -> ToolResult { "read_file" => { fs_read_file( &ctx, + &suppress, t.req("path")?, t.opt("start_line")?, t.opt("end_line")?, @@ -49,6 +60,7 @@ pub async fn run(ctx: Context, t: Tool) -> ToolResult { t.opt("context")?, t.opt("paths")?, None, + &suppress, ) .await .map(Into::into), @@ -65,6 +77,7 @@ pub async fn run(ctx: Context, t: Tool) -> ToolResult { t.opt("context")?, Some(vec!["docs".to_owned()].into()), Some(vec!["md".to_owned()].into()), + &suppress, ) .await .map(Into::into), diff --git a/.config/jp/tools/src/fs/delete_file_tests.rs b/.config/jp/tools/src/fs/delete_file_tests.rs index 2e5b79298..f1c2a800a 100644 --- a/.config/jp/tools/src/fs/delete_file_tests.rs +++ b/.config/jp/tools/src/fs/delete_file_tests.rs @@ -1,9 +1,20 @@ use camino_tempfile::tempdir; -use jp_tool::Outcome; +use jp_tool::{AccessPolicy, FsRule, Outcome}; use serde_json::Map; use super::*; +/// A policy granting the whole workspace except `denied`. +fn workspace_except(denied: &str) -> AccessPolicy { + AccessPolicy { + fs: vec![ + FsRule::new("").with_read(true).with_write(true), + FsRule::new(denied).with_read(false).with_write(false), + ], + ..AccessPolicy::default() + } +} + fn no_answers() -> Map { Map::new() } @@ -178,6 +189,33 @@ async fn deleting_dangling_symlink_succeeds() { assert!(std::fs::symlink_metadata(root.join("broken")).is_err()); } +#[tokio::test] +async fn refuses_to_delete_a_path_a_deny_rule_closes() { + let dir = tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir(root.join(".git")).unwrap(); + std::fs::write(root.join(".git/HEAD"), "ref: refs/heads/main").unwrap(); + + let result = fs_delete_file( + root, + Some(&workspace_except(".git")), + &no_answers(), + ".git/HEAD".to_owned(), + ) + .await + .unwrap(); + + let Outcome::Error { message, .. } = result else { + panic!("expected a refusal, got: {result:?}"); + }; + assert_eq!( + message, + "Access denied: cannot delete '.git/HEAD'. Paths granting delete: [.]. If required, ask \ + the user for explicit access." + ); + assert!(root.join(".git/HEAD").exists(), "file was deleted anyway"); +} + #[tokio::test] async fn deleting_missing_path_errors() { let dir = tempdir().unwrap(); diff --git a/.config/jp/tools/src/fs/grep_files.rs b/.config/jp/tools/src/fs/grep_files.rs index efb3a7a25..aa8b6bba1 100644 --- a/.config/jp/tools/src/fs/grep_files.rs +++ b/.config/jp/tools/src/fs/grep_files.rs @@ -2,6 +2,7 @@ use camino::{Utf8Path, Utf8PathBuf}; use grep_printer::StandardBuilder; use grep_regex::RegexMatcher; use grep_searcher::SearcherBuilder; +use ignore::gitignore::Gitignore; use jp_tool::AccessPolicy; use super::fs_list_files; @@ -14,6 +15,7 @@ pub(crate) async fn fs_grep_files( context: Option, paths: Option>, extensions: Option>, + suppress: &Gitignore, ) -> std::result::Result { // Resolve the file set via `fs_list_files`, which always walks from the // workspace root. Anchoring the walk there is what makes the root @@ -27,8 +29,10 @@ pub(crate) async fn fs_grep_files( // nothing, and `""`/`.` mean the workspace root. Escape attempts surface // as a hard error from the shared path validation. The access policy is // threaded through so an approved external mount can be searched. - let files: Vec = fs_list_files(root, access, paths.clone(), extensions.clone()) - .await? + let listing = fs_list_files(root, access, paths.clone(), extensions.clone(), suppress).await?; + + let notes = listing.notes(); + let files: Vec = listing .into_files() .into_iter() .map(Utf8PathBuf::from) @@ -63,11 +67,19 @@ pub(crate) async fn fs_grep_files( let matches = String::from_utf8(printer.into_inner().into_inner())?; let lines = matches.lines().count(); - if matches.is_empty() { - Ok("No matches found. Broaden your search to see more.".to_owned()) + let body = if matches.is_empty() { + // A search whose requested paths were skipped finds nothing for a + // completely different reason than a search that came up empty, and the + // notes below say which happened. + if notes.is_empty() { + "No matches found. Broaden your search to see more.".to_owned() + } else { + "No matches found in the paths that were searched.".to_owned() + } } else if lines > 200 && context.is_some() { - Box::pin(fs_grep_files( - root, access, pattern, None, paths, extensions, + // The inner call reproduces the notes, so they are not appended twice. + return Box::pin(fs_grep_files( + root, access, pattern, None, paths, extensions, suppress, )) .await .map(|v| { @@ -75,16 +87,36 @@ pub(crate) async fn fs_grep_files( "{v}\n[Hidden contextual lines due to excessive number of lines returned. Narrow \ down your search to see more.]" ) - }) + }); } else if lines > 100 { - Ok(indoc::formatdoc! {" + indoc::formatdoc! {" {} [Showing 100/{lines} lines of matches... Narrow down your search to see more.] - ", matches.lines().take(100).collect::>().join("\n"),}) + ", matches.lines().take(100).collect::>().join("\n"),} } else { - Ok(matches) + matches + }; + + Ok(append_notes(body, ¬es)) +} + +/// Append the listing's skip notes to a search result. +/// +/// Without them, a search whose requested paths were skipped reports the same +/// empty result as a search that genuinely found nothing. +fn append_notes(body: String, notes: &[String]) -> String { + if notes.is_empty() { + return body; } + + let notes = notes + .iter() + .map(|note| format!("Note: {note}")) + .collect::>() + .join("\n"); + + format!("{body}\n\n{notes}") } #[cfg(test)] diff --git a/.config/jp/tools/src/fs/grep_files_tests.rs b/.config/jp/tools/src/fs/grep_files_tests.rs index cbe25bad7..862eb3eaf 100644 --- a/.config/jp/tools/src/fs/grep_files_tests.rs +++ b/.config/jp/tools/src/fs/grep_files_tests.rs @@ -1,8 +1,9 @@ use std::collections::HashMap; use camino_tempfile::tempdir; +use ignore::gitignore::Gitignore; -use super::*; +use super::{super::utils::suppress_matcher, *}; #[tokio::test] async fn grep_with_restricted_policy_skips_ungranted_files() { @@ -26,6 +27,7 @@ async fn grep_with_restricted_policy_skips_ungranted_files() { None, None, None, + &Gitignore::empty(), ) .await .unwrap() @@ -67,6 +69,7 @@ async fn greps_files_under_approved_external_mount() { None, Some(vec!["fork".to_owned()].into()), None, + &Gitignore::empty(), ) .await .unwrap(); @@ -92,6 +95,7 @@ async fn dot_means_workspace_root() { None, Some(vec![".".to_owned()].into()), None, + &Gitignore::empty(), ) .await .unwrap(); @@ -128,6 +132,7 @@ async fn subdir_scope_respects_root_ignore() { None, Some(vec!["docs".to_owned()].into()), None, + &Gitignore::empty(), ) .await .unwrap() @@ -163,6 +168,7 @@ async fn restricts_to_extensions() { None, Some(vec!["docs".to_owned()].into()), Some(vec!["md".to_owned()].into()), + &Gitignore::empty(), ) .await .unwrap() @@ -182,6 +188,7 @@ async fn rejects_workspace_escape() { None, Some(vec!["../escape".to_owned()].into()), None, + &Gitignore::empty(), ) .await; @@ -192,6 +199,100 @@ async fn rejects_workspace_escape() { ); } +/// Workspace with one `.ignore`d fixture holding the sought text, mirroring +/// `**/fixtures/` in the real workspace `.ignore`. +fn fixtures_workspace(root: &camino::Utf8Path) { + std::fs::write(root.join(".ignore"), "**/fixtures/\n").unwrap(); + std::fs::create_dir_all(root.join("crates/tests/fixtures")).unwrap(); + std::fs::write( + root.join("crates/tests/fixtures/a.snap"), + "context_window: None", + ) + .unwrap(); +} + +#[tokio::test] +async fn ignored_directory_is_searched_when_named() { + // Regression: a sweep scoped to an `.ignore`d directory searched nothing and + // returned the same "no matches" as a real miss, which reads as evidence + // that the pattern is absent from files the tool never opened. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + fixtures_workspace(root); + + let matches = fs_grep_files( + root, + None, + "context_window: None".to_owned(), + None, + Some(vec!["crates/tests/fixtures".to_owned()].into()), + None, + &Gitignore::empty(), + ) + .await + .unwrap() + .replace('\\', "/"); + + assert_eq!( + matches, + "crates/tests/fixtures/a.snap:1:context_window: None\n" + ); +} + +#[tokio::test] +async fn explicitly_named_ignored_file_is_searched() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + fixtures_workspace(root); + + let matches = fs_grep_files( + root, + None, + "context_window".to_owned(), + None, + Some(vec!["crates/tests/fixtures/a.snap".to_owned()].into()), + None, + &Gitignore::empty(), + ) + .await + .unwrap() + .replace('\\', "/"); + + assert_eq!( + matches, + "crates/tests/fixtures/a.snap:1:context_window: None\n" + ); +} + +#[tokio::test] +async fn suppressed_path_is_reported_so_the_caller_can_ask_the_user() { + // The tool will not return this content however it is asked, so the note points + // at the only route left rather than leaving the reader to conclude the text is + // absent. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::write(root.join(".git/HEAD"), "ref: refs/heads/main").unwrap(); + + let matches = fs_grep_files( + root, + None, + "refs/heads".to_owned(), + None, + Some(vec![".git/HEAD".to_owned()].into()), + None, + &suppress_matcher(root, &[".git/".to_owned()]), + ) + .await + .unwrap(); + + assert_eq!( + matches, + "No matches found in the paths that were searched.\n\nNote: '.git/HEAD' is suppressed \ + from this tool's results. If you need it, ask the user to provide it." + ); +} + #[tokio::test] #[test_log::test] async fn test_grep_files() { @@ -305,10 +406,18 @@ async fn test_grep_files() { let paths = (!paths.is_empty()).then_some(paths.into_iter().map(str::to_owned).collect()); - let matches = fs_grep_files(root, None, pattern.to_owned(), Some(5), paths, None) - .await - .unwrap() - .replace('\\', "/"); + let matches = fs_grep_files( + root, + None, + pattern.to_owned(), + Some(5), + paths, + None, + &Gitignore::empty(), + ) + .await + .unwrap() + .replace('\\', "/"); assert_eq!(matches, expected.join(""), "test case: {name}"); } diff --git a/.config/jp/tools/src/fs/list_files.rs b/.config/jp/tools/src/fs/list_files.rs index 4566b7930..3324fffc1 100644 --- a/.config/jp/tools/src/fs/list_files.rs +++ b/.config/jp/tools/src/fs/list_files.rs @@ -1,65 +1,128 @@ use camino::{Utf8Path, Utf8PathBuf}; -use ignore::{WalkBuilder, WalkState}; +use ignore::{IncrementalIgnore, WalkBuilder, WalkState, gitignore::Gitignore}; use jp_tool::{AccessPolicy, Capability}; +use serde::ser::SerializeMap as _; -use super::utils::clean_workspace_path; +use super::utils::{is_suppressed, resolve_workspace_path, suppressed_note}; use crate::{Error, util::OneOrMany}; +/// Outcome of a listing: the files found, plus any requested path that +/// contributed nothing and why. #[derive(Debug)] -pub(crate) enum Files { - Empty, - List(Vec), +pub(crate) struct Files { + files: Vec, + skipped: Vec, } -impl Files { - pub(crate) fn into_files(self) -> Vec { +/// A requested path that produced no results, and the reason. +/// +/// The two reasons carry different remedies, which is why they are not +/// collapsed: only the policy can open a denied path, and only the user can +/// hand over a suppressed one. +#[derive(Debug)] +enum Skipped { + /// The access policy does not grant read on it. + Denied(String), + /// The tool may read it but never returns it. + Suppressed(String), +} + +impl Skipped { + fn note(&self) -> String { match self { - Files::Empty => vec![], - Files::List(files) => files, + Self::Denied(path) => denied_note(path), + Self::Suppressed(path) => suppressed_note(path), } } } +impl Files { + pub(crate) fn into_files(self) -> Vec { + self.files + } + + /// One sentence per requested path that produced no results. + /// + /// Empty for a listing that covered everything it was asked about. + pub(crate) fn notes(&self) -> Vec { + self.skipped.iter().map(Skipped::note).collect() + } +} + impl serde::Serialize for Files { fn serialize(&self, serializer: S) -> Result where S: serde::Serializer, { - match self { - Files::Empty => serializer.serialize_str("No files found."), - Files::List(files) => files.serialize(serializer), + // A listing that reached everything it was asked about serializes as a + // bare list (or a single sentence when nothing matched). The keyed shape + // appears only when something was skipped, so the notes cannot be + // mistaken for part of the file set. + if self.skipped.is_empty() { + return if self.files.is_empty() { + serializer.serialize_str("No files found.") + } else { + self.files.serialize(serializer) + }; } + + let mut map = serializer.serialize_map(Some(2))?; + map.serialize_entry("files", &self.files)?; + map.serialize_entry("notes", &self.notes())?; + map.end() } } +/// Report that a requested path was withheld by the access policy. +/// +/// Reporting an empty result as though the path had been examined is what makes +/// a search read as evidence of absence. +/// The policy is not the reader's to change, so the way forward is the user: +/// naming that here is what lets the reader ask for the contents instead of +/// concluding they do not exist. +fn denied_note(path: &str) -> String { + format!( + "'{path}' is not readable by this tool and was skipped. If you need it, ask the user to \ + provide it." + ) +} + pub(crate) async fn fs_list_files( root: &Utf8Path, access: Option<&AccessPolicy>, prefixes: Option>, extensions: Option>, + suppress: &Gitignore, ) -> std::result::Result { let prefixes = prefixes.unwrap_or(OneOrMany::One(String::new())).into_vec(); + let mut ignore_rules = IgnoreRules::for_workspace(root); - let mut entries = vec![]; + let mut files = vec![]; + let mut skipped = vec![]; for prefix in &prefixes { - let spec = walk_spec(root, prefix, access)?; - entries.extend(collect_files( - &spec.walk_root, - &spec.display_prefix, - spec.path_filter.as_deref(), - extensions.as_ref(), - access, - )); + match resolve_target(root, prefix, access, &mut ignore_rules, suppress)? { + Target::File(path) => files.extend(explicit_file(&path, extensions.as_ref())), + Target::Walk(spec) => { + files.extend(collect_files(&spec, extensions.as_ref(), access, suppress)); + } + Target::Skipped(reason) => skipped.push(reason), + } } - if entries.is_empty() { - return Ok(Files::Empty); - } + files.sort(); + files.dedup(); - entries.sort(); - entries.dedup(); + Ok(Files { files, skipped }) +} - Ok(Files::List(entries)) +/// What a single prefix resolves to. +enum Target { + /// An existing file, read without a walk. + File(Utf8PathBuf), + /// A tree to walk. + Walk(WalkSpec), + /// An existing path that produces no results. + Skipped(Skipped), } /// Where to walk for a prefix, and how to present the results. @@ -71,54 +134,121 @@ struct WalkSpec { display_prefix: Utf8PathBuf, /// Optional partial-prefix filter applied to the display path. path_filter: Option, + /// Whether `.ignore` files prune this walk. + /// + /// Disabled for a subtree the caller named outright and `soft_ignore` opted + /// in: the anchored root patterns do not prune reliably below the workspace + /// root, so leaving them on would prune unpredictably rather than not at + /// all. + apply_ignore: bool, } -/// Resolve a prefix into a [`WalkSpec`]. +/// Resolve a prefix into the work it implies. /// /// An empty prefix or bare `.` walks the whole workspace. -/// A prefix that names an approved external mount walks the mount's canonical +/// +/// A prefix naming an approved external mount walks the mount's canonical /// target (bounded by the approved target) and presents results under the mount /// name. +/// +/// A prefix the access policy withholds, or one the `suppress` list covers, is +/// skipped, so the caller learns its request went unanswered. +/// +/// A prefix naming an existing file reads it directly, and a prefix naming an +/// `.ignore`d directory walks that directory as its own root. +/// Ignore rules govern what traversal *surfaces*, not what the caller may name: +/// a path the caller already knows about is one it can already read, so hiding +/// it here would withhold nothing while breaking a legitimate request. +/// Paths that must stay closed however they are named are the access policy's +/// job, not the ignore rules'. +/// /// Any other prefix scopes the workspace walk with a path filter rather than -/// re-rooting: the walk always starts at the workspace root so the root -/// `.ignore` whitelist applies consistently (its anchored patterns like +/// re-rooting: the walk starts at the workspace root so the root `.ignore` +/// whitelist applies consistently (its anchored patterns like /// `docs/.vitepress/dist/` only prune when the walk is rooted at the `.ignore` /// file). -fn walk_spec( +fn resolve_target( root: &Utf8Path, prefix: &str, access: Option<&AccessPolicy>, -) -> std::result::Result { + ignore_rules: &mut IgnoreRules, + suppress: &Gitignore, +) -> std::result::Result { if prefix.is_empty() || prefix == "." { - return Ok(WalkSpec { + return Ok(Target::Walk(WalkSpec { walk_root: root.to_owned(), display_prefix: Utf8PathBuf::new(), path_filter: None, - }); + apply_ignore: true, + })); + } + + // The canonical workspace-relative form, which is what access rules match + // on: a path reached through an in-workspace symlink is checked against the + // rule for its real location, so a link cannot dodge a rule denying its + // target. External mount paths have no canonical workspace-relative form and + // keep their lexical shape, which is what external rules match. + let cleaned = resolve_workspace_path(root, prefix, access)?.relative; + + // Both checks come before the mount branch below: an approved mount is named + // by its in-workspace path, which is the form access rules and suppress + // patterns both match, so a mount can be withheld the same way any other + // directory can. + // + // Access is checked first because it is the harder boundary — a path the tool + // may not read at all is not merely one it declines to return. + if access.is_some_and(|policy| !policy.permits(Capability::Read, &cleaned)) { + return Ok(Target::Skipped(Skipped::Denied(cleaned.into_string()))); } - let cleaned = clean_workspace_path(root, prefix, access)?; + if is_suppressed(suppress, &cleaned) { + return Ok(Target::Skipped(Skipped::Suppressed(cleaned.into_string()))); + } - // A prefix naming an approved external mount walks the mount's canonical - // target and presents results under the mount name. `follow_links(false)` - // keeps nested symlinks inside the target from escaping the approved - // boundary. + // `follow_links(false)` in `collect_files` keeps nested symlinks inside the + // target from escaping the approved boundary. if let Some(rule) = access.and_then(|policy| policy.matching_fs_rule(&cleaned)) && rule.external() && let Some(target) = rule.approved_target() { - return Ok(WalkSpec { + return Ok(Target::Walk(WalkSpec { walk_root: target.to_owned(), display_prefix: rule.lexical_path().to_owned(), path_filter: Some(prefix_filter(&cleaned, root)), - }); + apply_ignore: true, + })); } - Ok(WalkSpec { + let full = root.join(&cleaned); + let is_dir = full.is_dir(); + let is_file = full.is_file(); + + // Only an existing path can be classified. A partial prefix like `rfd/D` + // names nothing on disk and stays a filter over the workspace walk. + let exists = is_dir || is_file; + + if is_file { + return Ok(Target::File(cleaned)); + } + + // Walking the subtree as its own root is the only way to reach it: the + // anchored root patterns do not prune reliably below the workspace root, so + // scoping to it with a filter would find nothing. + if exists && ignore_rules.prunes(&cleaned, is_dir) { + return Ok(Target::Walk(WalkSpec { + walk_root: full, + display_prefix: cleaned, + path_filter: None, + apply_ignore: false, + })); + } + + Ok(Target::Walk(WalkSpec { walk_root: root.to_owned(), display_prefix: Utf8PathBuf::new(), path_filter: Some(prefix_filter(&cleaned, root)), - }) + apply_ignore: true, + })) } /// Build a partial-prefix filter from a cleaned prefix. @@ -134,89 +264,171 @@ fn prefix_filter(cleaned: &Utf8Path, root: &Utf8Path) -> String { filter } -/// Walk `walk_root` and return display paths that pass the extension, prefix, -/// and read-access filters. +/// Apply the path-filtering configuration shared by the walk and the standalone +/// ignore matcher. +/// +/// Both are configured from here so the matcher's verdict is the walk's +/// behavior: configuring them separately would let a path be reported reachable +/// and then pruned anyway, or the reverse. +fn walk_filters(builder: &mut WalkBuilder, apply_ignore: bool) -> &mut WalkBuilder { + builder + // Include hidden and otherwise ignored files. + .standard_filters(false) + .follow_links(false) + // Respect `.ignore` files (also in parent directories). + .ignore(apply_ignore) + .parents(apply_ignore) +} + +/// The ignore rules the workspace walk applies, queryable one path at a time. +/// +/// Answers what the walk would do with a path without walking to it, so a +/// pruned path can be named in the result instead of silently vanishing. +/// Rules from `.ignore` files in subdirectories count, not just the root one: +/// the matcher loads each queried path's directory chain on demand. +struct IgnoreRules(Option); + +impl IgnoreRules { + fn for_workspace(root: &Utf8Path) -> Self { + let mut builder = WalkBuilder::new(root); + walk_filters(&mut builder, true); + Self(builder.build_matchers().into_iter().next()) + } + + /// Whether the walk prunes `relative`, which must be workspace-relative and + /// free of `..` components. + /// + /// Reports nothing as pruned when the matcher could not be built, so a + /// matcher failure surfaces files rather than hiding them. + fn prunes(&mut self, relative: &Utf8Path, is_dir: bool) -> bool { + self.0 + .as_mut() + .is_some_and(|rules| rules.matched(relative, is_dir).is_ignore()) + } +} + +/// Present an explicitly named file as a listing entry. +/// +/// Bypassing the walk also bypasses its per-entry extension filter, so that is +/// applied here on the same terms. +/// Read access is settled before this point, by the caller. +fn explicit_file(cleaned: &Utf8Path, extensions: Option<&OneOrMany>) -> Option { + if extensions.is_some_and(|extensions| { + cleaned + .extension() + .is_some_and(|ext| !extensions.iter().any(|allowed| allowed == ext)) + }) { + return None; + } + + Some(cleaned.as_str().replace('/', std::path::MAIN_SEPARATOR_STR)) +} + +/// Walk a [`WalkSpec`] and return display paths that pass the extension, +/// prefix, and read-access filters. /// /// Each result is `display_prefix` joined with the entry's path relative to /// `walk_root`, so callers see workspace-relative (or mount-relative) paths. /// When a policy is supplied, only files it grants `read` on are returned. fn collect_files( - walk_root: &Utf8Path, - display_prefix: &Utf8Path, - path_filter: Option<&str>, + spec: &WalkSpec, extensions: Option<&OneOrMany>, access: Option<&AccessPolicy>, + suppress: &Gitignore, ) -> Vec { + let walk_root = &spec.walk_root; let (tx, matches) = crossbeam_channel::unbounded(); - WalkBuilder::new(walk_root) - // Include hidden and otherwise ignored files. - .standard_filters(false) - .follow_links(false) - // Respect `.ignore` files (also in parent directories). - .ignore(true) - .parents(true) - .build_parallel() - .run(|| { - let tx = tx.clone(); - let extensions = extensions.cloned(); - let path_filter = path_filter.map(str::to_owned); - let display_prefix = display_prefix.to_owned(); - Box::new(move |entry| { - // Ignore invalid entries. - let Ok(entry) = entry else { - return WalkState::Continue; - }; - - // Ignore non-files. - if entry.file_type().is_none_or(|ft| !ft.is_file()) { - return WalkState::Continue; - } - - // Ignore files that don't match the extension, if any. - if extensions.as_ref().is_some_and(|extensions| { - entry.path().extension().is_some_and(|ext| { - !extensions.contains(&ext.to_string_lossy().into_owned()) - }) - }) { - return WalkState::Continue; - } - - let Ok(path) = Utf8PathBuf::try_from(entry.into_path()) else { - return WalkState::Continue; - }; - - let Ok(relative) = path.strip_prefix(walk_root) else { - return WalkState::Continue; - }; - - // Present results under the display prefix (mount name, or - // empty for the workspace itself). - let display = if display_prefix.as_str().is_empty() { - relative.to_owned() - } else { - display_prefix.join(relative) - }; - - // Filter by partial prefix if the original prefix wasn't a directory. - if let Some(filter) = &path_filter - && !display.as_str().starts_with(filter.as_str()) - { - return WalkState::Continue; - } - - // Per-entry read enforcement: only list files the policy grants - // read on. An absent policy lists everything (unrestricted). - if let Some(policy) = access - && !policy.permits(Capability::Read, &display) - { - return WalkState::Continue; - } - - let _result = tx.send(display.to_string()); - - WalkState::Continue - }) + let mut builder = WalkBuilder::new(walk_root); + walk_filters(&mut builder, spec.apply_ignore); + + // Prune suppressed paths from traversal too, so one `suppress` entry is enough + // to keep a tree out of results — with no matching `.ignore` entry to keep in + // sync, and no way for an `.ignore` un-ignore rule to let it back in. + if !suppress.is_empty() { + let suppress = suppress.clone(); + let walk_root = walk_root.to_owned(); + let display_prefix = spec.display_prefix.clone(); + builder.filter_entry(move |entry| { + let Some(path) = Utf8Path::from_path(entry.path()) else { + return true; + }; + let Ok(relative) = path.strip_prefix(&walk_root) else { + return true; + }; + // Match the path as the caller sees it, so a pattern covering a mount + // name reaches the mount's contents even though they live outside the + // workspace on disk. + let display = if display_prefix.as_str().is_empty() { + relative.to_owned() + } else { + display_prefix.join(relative) + }; + !is_suppressed(&suppress, &display) }); + } + + builder.build_parallel().run(|| { + let tx = tx.clone(); + let extensions = extensions.cloned(); + let path_filter = spec.path_filter.clone(); + let display_prefix = spec.display_prefix.clone(); + Box::new(move |entry| { + // Ignore invalid entries. + let Ok(entry) = entry else { + return WalkState::Continue; + }; + + // Ignore non-files. + if entry.file_type().is_none_or(|ft| !ft.is_file()) { + return WalkState::Continue; + } + + // Ignore files that don't match the extension, if any. + if extensions.as_ref().is_some_and(|extensions| { + entry + .path() + .extension() + .is_some_and(|ext| !extensions.contains(&ext.to_string_lossy().into_owned())) + }) { + return WalkState::Continue; + } + + let Ok(path) = Utf8PathBuf::try_from(entry.into_path()) else { + return WalkState::Continue; + }; + + let Ok(relative) = path.strip_prefix(walk_root) else { + return WalkState::Continue; + }; + + // Present results under the display prefix (mount name, the + // requested subtree, or empty for the workspace itself). + let display = if display_prefix.as_str().is_empty() { + relative.to_owned() + } else { + display_prefix.join(relative) + }; + + // Filter by partial prefix if the original prefix wasn't a directory. + if let Some(filter) = &path_filter + && !display.as_str().starts_with(filter.as_str()) + { + return WalkState::Continue; + } + + // Per-entry read enforcement: only list files the policy grants + // read on. An absent policy lists everything (unrestricted). + if let Some(policy) = access + && !policy.permits(Capability::Read, &display) + { + return WalkState::Continue; + } + + let _result = tx.send(display.to_string()); + + WalkState::Continue + }) + }); drop(tx); matches.into_iter().collect() diff --git a/.config/jp/tools/src/fs/list_files_tests.rs b/.config/jp/tools/src/fs/list_files_tests.rs index 08c97c586..4689a550e 100644 --- a/.config/jp/tools/src/fs/list_files_tests.rs +++ b/.config/jp/tools/src/fs/list_files_tests.rs @@ -1,14 +1,17 @@ use std::collections::HashMap; -use assert_matches::assert_matches; use camino_tempfile::tempdir; +use jp_tool::{AccessPolicy, FsRule}; -use super::*; +use super::{super::utils::suppress_matcher, *}; + +/// No suppression, for the tests that are not about it. +fn unsuppressed() -> Gitignore { + Gitignore::empty() +} #[tokio::test] async fn restricted_policy_filters_listing_to_readable() { - use jp_tool::{AccessPolicy, FsRule}; - let ws = tempdir().unwrap(); std::fs::create_dir(ws.path().join("src")).unwrap(); std::fs::write(ws.path().join("src/lib.rs"), "").unwrap(); @@ -19,7 +22,7 @@ async fn restricted_policy_filters_listing_to_readable() { fs: vec![FsRule::new("src").with_read(true)], ..AccessPolicy::default() }; - let files: Vec = fs_list_files(ws.path(), Some(&policy), None, None) + let files: Vec = fs_list_files(ws.path(), Some(&policy), None, None, &unsuppressed()) .await .unwrap() .into_files() @@ -63,6 +66,7 @@ async fn lists_files_under_approved_external_mount() { Some(&policy), Some(vec!["fork".to_owned()].into()), None, + &unsuppressed(), ) .await .unwrap() @@ -95,6 +99,7 @@ async fn listing_external_mount_without_grant_is_rejected() { Some(&policy), Some(vec!["fork".to_owned()].into()), None, + &unsuppressed(), ) .await; @@ -195,7 +200,7 @@ async fn test_list_files() { let extensions = (!extensions.is_empty()).then_some(extensions.into_iter().map(str::to_owned).collect()); - let files = fs_list_files(root, None, prefixes, extensions) + let files = fs_list_files(root, None, prefixes, extensions, &unsuppressed()) .await .unwrap(); @@ -221,9 +226,15 @@ async fn dot_prefix_lists_workspace_root() { std::fs::write(root.join("a.txt"), "").unwrap(); std::fs::write(root.join("b.txt"), "").unwrap(); - let files = fs_list_files(root, None, Some(vec![".".to_owned()].into()), None) - .await - .unwrap(); + let files = fs_list_files( + root, + None, + Some(vec![".".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap(); let mut listed = files.into_files(); listed.sort(); @@ -248,25 +259,427 @@ async fn subdir_scope_respects_root_ignore() { std::fs::write(path, "").unwrap(); } - let files = fs_list_files(root, None, Some(vec!["docs".to_owned()].into()), None) - .await - .unwrap() + let files = fs_list_files( + root, + None, + Some(vec!["docs".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap() + .into_files() + .into_iter() + .map(|s| s.replace('\\', "/")) + .collect::>(); + + assert_eq!(files, vec!["docs/getting-started.md".to_owned()]); +} + +/// Workspace with one whitelisted source file and one `.ignore`d fixture tree, +/// mirroring `**/fixtures/` in the real workspace `.ignore`. +fn fixtures_workspace(root: &camino::Utf8Path) { + std::fs::write(root.join(".ignore"), "**/fixtures/\n").unwrap(); + std::fs::create_dir_all(root.join("crates/tests/fixtures")).unwrap(); + std::fs::write(root.join("crates/tests/lib.rs"), "").unwrap(); + std::fs::write(root.join("crates/tests/fixtures/a.snap"), "").unwrap(); +} + +fn listed(files: Files) -> Vec { + files .into_files() .into_iter() - .map(|s| s.replace('\\', "/")) - .collect::>(); + .map(|f| f.replace('\\', "/")) + .collect() +} - assert_eq!(files, vec!["docs/getting-started.md".to_owned()]); +#[tokio::test] +async fn explicitly_named_ignored_file_is_listed() { + // Ignore rules govern what traversal surfaces, not what a caller may name. A + // path the caller already knows about is one it can already read, so + // withholding it here would protect nothing. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + fixtures_workspace(root); + + let files = fs_list_files( + root, + None, + Some(vec!["crates/tests/fixtures/a.snap".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap(); + + assert!(files.notes().is_empty(), "got: {:?}", files.notes()); + assert_eq!(listed(files), vec![ + "crates/tests/fixtures/a.snap".to_owned() + ]); } #[tokio::test] -#[test_log::test] -async fn test_empty_list() { +async fn ignored_directory_is_walked_when_named() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + fixtures_workspace(root); + + let files = fs_list_files( + root, + None, + Some(vec!["crates/tests/fixtures".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap(); + + assert!(files.notes().is_empty(), "got: {:?}", files.notes()); + assert_eq!(listed(files), vec![ + "crates/tests/fixtures/a.snap".to_owned() + ]); +} + +#[tokio::test] +async fn nested_ignore_file_decides_how_a_named_directory_is_reached() { + // A directory the rules exclude can only be reached by walking it as its own + // root, and the rule here lives in `docs/.ignore` rather than at the + // workspace root. Classifying from the root file alone would call this path + // ordinary, scope the workspace walk to it with a filter, and return nothing + // — because traversal prunes it on the way down. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("docs/dist")).unwrap(); + std::fs::write(root.join("docs/.ignore"), "dist/\n").unwrap(); + std::fs::write(root.join("docs/dist/index.html"), "").unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec!["docs/dist".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap(); + + assert_eq!(listed(files), vec!["docs/dist/index.html".to_owned()]); +} + +#[tokio::test] +async fn ignored_tree_stays_out_of_an_unscoped_listing() { + // Naming a tree reaches it; not naming one leaves it pruned. This is the + // anti-bloat property the ignore rules exist for. let tmp = tempdir().unwrap(); let root = tmp.path(); - let files = fs_list_files(root, None, Some(vec!["foo".to_owned()].into()), None) + fixtures_workspace(root); + + let files = fs_list_files(root, None, None, None, &unsuppressed()) .await .unwrap(); - assert_matches!(files, Files::Empty); + assert_eq!(listed(files), vec![ + ".ignore".to_owned(), + "crates/tests/lib.rs".to_owned(), + ]); +} + +#[tokio::test] +async fn path_outside_the_whitelist_is_walked_when_named() { + // The real `.ignore` is a whitelist: `*` then un-ignores. A tree nobody has + // gotten around to un-ignoring is invisible to an unscoped listing but still + // reachable on request, so adding a directory does not make it unreadable + // until someone updates `.ignore`. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::write(root.join(".ignore"), "*\n!src/\n!src/**\n").unwrap(); + std::fs::create_dir_all(root.join("vendor")).unwrap(); + std::fs::write(root.join("vendor/dep.rs"), "").unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec!["vendor".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap(); + + assert_eq!(listed(files), vec!["vendor/dep.rs".to_owned()]); +} + +#[tokio::test] +async fn suppressed_path_is_skipped_with_a_note() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::write(root.join(".git/HEAD"), "").unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec![".git".to_owned()].into()), + None, + &suppress_matcher(root, &[".git/".to_owned()]), + ) + .await + .unwrap(); + + assert_eq!(files.notes(), vec![ + "'.git' is suppressed from this tool's results. If you need it, ask the user to provide \ + it." + .to_owned() + ]); + assert!(listed(files).is_empty()); +} + +#[tokio::test] +async fn suppress_pattern_covers_files_inside_the_named_directory() { + // A pattern naming a directory covers the files under it, or suppression is + // one path component away from being bypassed. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::write(root.join(".git/HEAD"), "ref: refs/heads/main").unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec![".git/HEAD".to_owned()].into()), + None, + &suppress_matcher(root, &[".git/".to_owned()]), + ) + .await + .unwrap(); + + assert_eq!(files.notes(), vec![ + "'.git/HEAD' is suppressed from this tool's results. If you need it, ask the user to \ + provide it." + .to_owned() + ]); + assert!(listed(files).is_empty()); +} + +#[tokio::test] +async fn suppress_patterns_match_at_any_depth() { + // `.ignore` glob syntax, so one pattern covers a name wherever it appears — + // including inside a nested tree that an anchored prefix would miss. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("crates/inner/target/debug")).unwrap(); + std::fs::write(root.join("crates/inner/target/debug/bin"), "").unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec!["crates/inner/target".to_owned()].into()), + None, + &suppress_matcher(root, &["**/target/".to_owned()]), + ) + .await + .unwrap(); + + assert_eq!(files.notes(), vec![ + "'crates/inner/target' is suppressed from this tool's results. If you need it, ask the \ + user to provide it." + .to_owned() + ]); + assert!(listed(files).is_empty()); +} + +#[cfg(unix)] +#[tokio::test] +async fn an_in_workspace_symlink_cannot_dodge_suppression() { + // Matching happens on the canonical form, so naming the link resolves to the + // suppressed target instead of sliding past a pattern keyed on its name. + use std::os::unix::fs::symlink; + + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::write(root.join(".git/HEAD"), "ref: refs/heads/main").unwrap(); + symlink(".git", root.join("gitlink")).unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec!["gitlink/HEAD".to_owned()].into()), + None, + &suppress_matcher(root, &[".git/".to_owned()]), + ) + .await + .unwrap(); + + assert_eq!(files.notes(), vec![ + "'.git/HEAD' is suppressed from this tool's results. If you need it, ask the user to \ + provide it." + .to_owned() + ]); + assert!(listed(files).is_empty()); +} + +#[tokio::test] +async fn suppressed_tree_is_pruned_from_traversal_without_an_ignore_entry() { + // One `suppress` entry is enough on its own: there is no matching `.ignore` + // entry to keep in sync. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("secrets")).unwrap(); + std::fs::write(root.join("secrets/key.pem"), "").unwrap(); + std::fs::write(root.join("main.rs"), "").unwrap(); + + let files = fs_list_files( + root, + None, + None, + None, + &suppress_matcher(root, &["secrets/".to_owned()]), + ) + .await + .unwrap(); + + assert!(files.notes().is_empty(), "got: {:?}", files.notes()); + assert_eq!(listed(files), vec!["main.rs".to_owned()]); +} + +#[cfg(unix)] +#[tokio::test] +async fn suppression_reaches_inside_an_approved_external_mount() { + // The mount's contents live outside the workspace on disk, so pruning matches + // the path as the caller sees it rather than its real location. An anchored + // access rule could not express this at all. + use std::os::unix::fs::symlink; + + let ws = tempdir().unwrap(); + let ext = tempdir().unwrap(); + let ext_canon = ext.path().canonicalize_utf8().unwrap(); + std::fs::create_dir(ext_canon.join(".git")).unwrap(); + std::fs::write(ext_canon.join(".git/HEAD"), "").unwrap(); + std::fs::write(ext_canon.join("a.rs"), "").unwrap(); + symlink(ext.path(), ws.path().join("fork")).unwrap(); + + let policy = AccessPolicy { + fs: vec![ + FsRule::new("fork") + .with_external(true) + .with_approved_target(Some(ext_canon)) + .with_read(true), + ], + ..AccessPolicy::default() + }; + let files = fs_list_files( + ws.path(), + Some(&policy), + Some(vec!["fork".to_owned()].into()), + None, + &suppress_matcher(ws.path(), &["**/.git/".to_owned()]), + ) + .await + .unwrap(); + + assert_eq!(listed(files), vec!["fork/a.rs".to_owned()]); +} + +#[tokio::test] +async fn suppressed_path_does_not_suppress_the_other_requested_paths() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::create_dir_all(root.join("src")).unwrap(); + std::fs::write(root.join(".git/HEAD"), "").unwrap(); + std::fs::write(root.join("src/lib.rs"), "").unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec![".git".to_owned(), "src".to_owned()].into()), + None, + &suppress_matcher(root, &[".git/".to_owned()]), + ) + .await + .unwrap(); + + assert_eq!(files.notes(), vec![ + "'.git' is suppressed from this tool's results. If you need it, ask the user to provide \ + it." + .to_owned() + ]); + assert_eq!(listed(files), vec!["src/lib.rs".to_owned()]); +} + +#[tokio::test] +async fn explicitly_named_file_respects_read_policy() { + // Naming a file outright skips the walk, so the walk's per-entry read check + // never sees it. The grant is settled before the file is read, and an + // ungranted one is reported rather than dropped. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir(root.join("src")).unwrap(); + std::fs::write(root.join("src/lib.rs"), "").unwrap(); + std::fs::write(root.join("secret.txt"), "").unwrap(); + + let policy = AccessPolicy { + fs: vec![FsRule::new("src").with_read(true)], + ..AccessPolicy::default() + }; + let files = fs_list_files( + root, + Some(&policy), + Some(vec!["secret.txt".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap(); + + assert_eq!(files.notes(), vec![ + "'secret.txt' is not readable by this tool and was skipped. If you need it, ask the user \ + to provide it." + .to_owned() + ]); + assert!(listed(files).is_empty(), "ungranted file leaked"); +} + +#[tokio::test] +async fn explicitly_named_file_respects_extension_filter() { + // `grep_user_docs` relies on the extension filter to keep non-prose out of + // documentation searches; an explicit target must not escape it. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + std::fs::create_dir(root.join("docs")).unwrap(); + std::fs::write(root.join("docs/config.mts"), "").unwrap(); + + let files = fs_list_files( + root, + None, + Some(vec!["docs/config.mts".to_owned()].into()), + Some(vec!["md".to_owned()].into()), + &unsuppressed(), + ) + .await + .unwrap(); + + assert!(listed(files).is_empty()); +} + +#[tokio::test] +#[test_log::test] +async fn test_empty_list() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let files = fs_list_files( + root, + None, + Some(vec!["foo".to_owned()].into()), + None, + &unsuppressed(), + ) + .await + .unwrap(); + + // A prefix that names nothing is a filter that matches nothing. There is no + // `.ignore` rule involved, so the result carries no note. + assert!(files.notes().is_empty()); + assert!(files.into_files().is_empty()); } diff --git a/.config/jp/tools/src/fs/move_file_tests.rs b/.config/jp/tools/src/fs/move_file_tests.rs index 64d9103e9..fbb33d051 100644 --- a/.config/jp/tools/src/fs/move_file_tests.rs +++ b/.config/jp/tools/src/fs/move_file_tests.rs @@ -1,5 +1,5 @@ use camino_tempfile::tempdir; -use jp_tool::Outcome; +use jp_tool::{AccessPolicy, FsRule, Outcome}; use serde_json::{Map, Value, json}; use super::*; @@ -9,6 +9,17 @@ fn no_answers() -> Map { Map::new() } +/// A policy granting the whole workspace except `denied`. +fn workspace_except(denied: &str) -> AccessPolicy { + AccessPolicy { + fs: vec![ + FsRule::new("").with_read(true).with_write(true), + FsRule::new(denied).with_read(false).with_write(false), + ], + ..AccessPolicy::default() + } +} + fn answers(pairs: &[(&str, Value)]) -> Map { let mut m = Map::new(); for (k, v) in pairs { @@ -129,6 +140,59 @@ fn moves_directory_creates_target_parents() { assert!(root.join("vendored/upstream/src/foo.rs").exists()); } +#[test] +fn refuses_to_move_out_of_a_path_a_deny_rule_closes() { + let dir = tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir(root.join(".git")).unwrap(); + std::fs::write(root.join(".git/HEAD"), "x").unwrap(); + + let result = fs_move_file_impl( + root, + Some(&workspace_except(".git")), + &no_answers(), + ".git/HEAD", + "head.txt", + &never_git_runner(), + ) + .unwrap(); + + assert_eq!( + unwrap_error(result), + "Access denied: cannot delete '.git/HEAD'. Paths granting delete: [.]. If required, ask \ + the user for explicit access." + ); + assert!(root.join(".git/HEAD").exists()); +} + +#[test] +fn refuses_to_move_into_a_path_a_deny_rule_closes() { + // The target is checked as well as the source: writing into a closed tree is + // no more allowed than reading out of one. + let dir = tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir(root.join(".git")).unwrap(); + std::fs::write(root.join("note.txt"), "x").unwrap(); + + let result = fs_move_file_impl( + root, + Some(&workspace_except(".git")), + &no_answers(), + "note.txt", + ".git/note.txt", + // The refusal lands before any git work is done. + &never_git_runner(), + ) + .unwrap(); + + assert_eq!( + unwrap_error(result), + "Access denied: cannot create '.git/note.txt'. Paths granting create: [.]. If required, \ + ask the user for explicit access." + ); + assert!(root.join("note.txt").exists()); +} + #[test] fn missing_source_errors() { let dir = tempdir().unwrap(); diff --git a/.config/jp/tools/src/fs/read_file.rs b/.config/jp/tools/src/fs/read_file.rs index 76baee4fb..7495bab06 100644 --- a/.config/jp/tools/src/fs/read_file.rs +++ b/.config/jp/tools/src/fs/read_file.rs @@ -1,10 +1,12 @@ +use ignore::gitignore::Gitignore; use jp_tool::{Capability, Context}; -use super::utils::{authorize, resolve_workspace_path}; +use super::utils::{authorize, is_suppressed, resolve_workspace_path, suppressed_note}; use crate::util::{ToolResult, error}; pub(crate) async fn fs_read_file( ctx: &Context, + suppress: &Gitignore, path: String, start_line: Option, end_line: Option, @@ -16,6 +18,11 @@ pub(crate) async fn fs_read_file( if let Err(msg) = authorize(ctx.access.as_ref(), Capability::Read, &resolved.relative) { return error(msg); } + // Matched on the canonical form, so a path reached through an in-workspace + // symlink is checked against the pattern for its real location. + if is_suppressed(suppress, &resolved.relative) { + return error(suppressed_note(resolved.relative.as_str())); + } let absolute_path = resolved.absolute; if !absolute_path.exists() { return error("File not found."); diff --git a/.config/jp/tools/src/fs/read_file_tests.rs b/.config/jp/tools/src/fs/read_file_tests.rs index 2a38fe232..1874f8a76 100644 --- a/.config/jp/tools/src/fs/read_file_tests.rs +++ b/.config/jp/tools/src/fs/read_file_tests.rs @@ -1,7 +1,8 @@ use camino_tempfile::tempdir; +use ignore::gitignore::Gitignore; use jp_tool::{Action, Context, Outcome}; -use super::*; +use super::{super::utils::suppress_matcher, *}; #[tokio::test] async fn test_fs_read_file() { @@ -61,9 +62,15 @@ async fn test_fs_read_file() { workspace_id: "test".into(), conversation_id: "test".into(), }; - let result = fs_read_file(&ctx, "file.txt".to_owned(), start_line, end_line) - .await - .unwrap(); + let result = fs_read_file( + &ctx, + &Gitignore::empty(), + "file.txt".to_owned(), + start_line, + end_line, + ) + .await + .unwrap(); let out = match result { Outcome::Success { content } => content, @@ -106,9 +113,15 @@ async fn reads_through_approved_external_mount() { conversation_id: "test".into(), }; - let result = fs_read_file(&ctx, "fork/lib.rs".to_owned(), None, None) - .await - .unwrap(); + let result = fs_read_file( + &ctx, + &Gitignore::empty(), + "fork/lib.rs".to_owned(), + None, + None, + ) + .await + .unwrap(); let content = match result { Outcome::Success { content } => content, @@ -149,9 +162,15 @@ async fn read_through_internal_symlink_respects_deny_rule() { }; // Direct read of the denied path is rejected. - let direct = fs_read_file(&ctx, "secret/f.txt".to_owned(), None, None) - .await - .unwrap(); + let direct = fs_read_file( + &ctx, + &Gitignore::empty(), + "secret/f.txt".to_owned(), + None, + None, + ) + .await + .unwrap(); assert!( matches!(direct, Outcome::Error { .. }), "direct read should be denied" @@ -159,15 +178,58 @@ async fn read_through_internal_symlink_respects_deny_rule() { // Reaching it through the in-workspace symlink canonicalizes to `secret/` // and must be denied too — the symlink cannot dodge the deny rule. - let via_alias = fs_read_file(&ctx, "alias/f.txt".to_owned(), None, None) - .await - .unwrap(); + let via_alias = fs_read_file( + &ctx, + &Gitignore::empty(), + "alias/f.txt".to_owned(), + None, + None, + ) + .await + .unwrap(); assert!( matches!(via_alias, Outcome::Error { .. }), "symlinked read should be denied" ); } +#[tokio::test] +async fn refuses_a_suppressed_path() { + // Naming a path outright is how the user hands a file to the model, so it is + // normally enough. Suppression is the exception: the tool can read the file + // — no access rule stops it — and declines to return it. + let workspace = tempdir().unwrap(); + std::fs::create_dir(workspace.path().join(".git")).unwrap(); + std::fs::write(workspace.path().join(".git/HEAD"), "ref: refs/heads/main").unwrap(); + + let ctx = Context { + root: workspace.path().to_path_buf(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + }; + + let result = fs_read_file( + &ctx, + &suppress_matcher(workspace.path(), &[".git/".to_owned()]), + ".git/HEAD".to_owned(), + None, + None, + ) + .await + .unwrap(); + + let Outcome::Error { message, .. } = result else { + panic!("expected a refusal, got: {result:?}"); + }; + assert_eq!( + message, + "'.git/HEAD' is suppressed from this tool's results. If you need it, ask the user to \ + provide it." + ); +} + #[cfg(unix)] #[tokio::test] async fn denies_in_workspace_path_with_no_matching_grant() { @@ -189,8 +251,14 @@ async fn denies_in_workspace_path_with_no_matching_grant() { conversation_id: "test".into(), }; - let result = fs_read_file(&ctx, "secret.txt".to_owned(), None, None) - .await - .unwrap(); + let result = fs_read_file( + &ctx, + &Gitignore::empty(), + "secret.txt".to_owned(), + None, + None, + ) + .await + .unwrap(); assert!(matches!(result, Outcome::Error { .. })); } diff --git a/.config/jp/tools/src/fs/utils.rs b/.config/jp/tools/src/fs/utils.rs index 9f52d5f19..041714191 100644 --- a/.config/jp/tools/src/fs/utils.rs +++ b/.config/jp/tools/src/fs/utils.rs @@ -2,6 +2,7 @@ use std::{io, path::PathBuf}; use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; use clean_path::Clean as _; +use ignore::gitignore::{Gitignore, GitignoreBuilder}; use jp_tool::{AccessPolicy, Capability}; use crate::{ @@ -9,6 +10,58 @@ use crate::{ util::runner::{DuctProcessRunner, ProcessOutput, ProcessRunner}, }; +/// Matcher for the `suppress` tool option: paths a tool may read but must not +/// return. +/// +/// Suppression is about disclosure, not reach. +/// A suppressed path stays readable to the tool process — which is what lets +/// `fs_modify_file` run `git status` against a suppressed `.git` to check for +/// uncommitted work — and is kept out of what the tool hands back. +/// Reach is [`AccessPolicy`]'s question. +/// +/// Patterns use `.ignore` syntax, so `**/target/` matches at any depth, and are +/// matched against workspace-relative paths. +/// An unparseable pattern is skipped; the rest of the list still applies. +/// An empty list suppresses nothing, which is the default: what counts as noise +/// or as sensitive is a property of the project, not of a directory's name. +pub fn suppress_matcher(root: &Utf8Path, patterns: &[String]) -> Gitignore { + let mut builder = GitignoreBuilder::new(root); + for pattern in patterns { + let _result = builder.add_line(None, pattern); + } + builder.build().unwrap_or_else(|_| Gitignore::empty()) +} + +/// Whether `suppress` keeps `relative` out of a tool's results. +/// +/// Parents are matched too, so a pattern naming a directory also covers the +/// files inside it — otherwise suppression is one path component away from +/// being bypassed. +/// +/// The path is matched as a directory whatever it is on disk. +/// A pattern written `secrets/` is meant to cover that name, and wrongly +/// suppressing a file that happens to share it costs far less than returning a +/// directory's contents. +pub fn is_suppressed(suppress: &Gitignore, relative: &Utf8Path) -> bool { + suppress + .matched_path_or_any_parents(relative, true) + .is_ignore() +} + +/// Report that a path was suppressed from a tool's results. +/// +/// Reporting an empty result as though the path had been examined is what makes +/// a search read as evidence of absence. +/// The tool will not return this content however it is asked, so the way +/// forward is the user: naming that here is what lets the reader ask for the +/// contents instead of concluding they do not exist. +pub fn suppressed_note(path: &str) -> String { + format!( + "'{path}' is suppressed from this tool's results. If you need it, ask the user to provide \ + it." + ) +} + /// Enforce an access-policy capability on a resolved workspace-relative path. /// /// `relative` must be the resolver's output (`ResolvedPath::relative`), not the @@ -21,8 +74,12 @@ use crate::{ /// /// A `None` policy (or an unrestricted one) permits everything. /// A restricted policy permits only what a matching rule grants; on denial the -/// configured grant paths are listed so the user can see what the tool is -/// allowed to do. +/// paths that do grant the capability are listed, so the reader can see where +/// it may go instead. +/// +/// The refusal names the user as the way forward. +/// Only the policy can open the path, and the policy is the user's to change — +/// without that, a denial reads as a dead end. pub fn authorize( access: Option<&AccessPolicy>, capability: Capability, @@ -34,11 +91,15 @@ pub fn authorize( if policy.permits(capability, relative) { return Ok(()); } - let grants: Vec<&str> = policy.grant_paths().map(Utf8Path::as_str).collect(); + let granting: Vec<&str> = policy + .granting_paths(capability) + .map(Utf8Path::as_str) + .collect(); + let capability = capability.as_str(); Err(format!( - "Access denied: cannot {} '{relative}'. Granted paths: [{}].", - capability.as_str(), - grants.join(", ") + "Access denied: cannot {capability} '{relative}'. Paths granting {capability}: [{}]. If \ + required, ask the user for explicit access.", + granting.join(", ") )) } @@ -292,32 +353,6 @@ pub fn resolve_workspace_entry( Ok(ResolvedPath { absolute, relative }) } -/// Clean a user-supplied path against the workspace root. -/// -/// Returns the lexically-normalized, workspace-relative form, preserving the -/// caller's input shape — symlinks in existing ancestors are *checked* for -/// escape but not *followed* in the returned path. -/// -/// Performs the same input validation as [`resolve_workspace_path`], but -/// doesn't canonicalize the result. -/// Use this when output paths should match what the user supplied (read/search -/// tools). -pub fn clean_workspace_path( - root: &Utf8Path, - path: &str, - access: Option<&AccessPolicy>, -) -> Result { - let ValidatedInput { - cleaned, - canonical_root, - } = validate_workspace_input(root, path)?; - - let candidate = root.join(&cleaned); - check_ancestor_in_root(&candidate, &canonical_root, access, &cleaned)?; - - Ok(cleaned) -} - /// Output of [`validate_workspace_input`]: the cleaned form plus the /// canonicalized workspace root. /// Each public resolver decides how to canonicalize the rest. diff --git a/.config/jp/tools/src/fs/utils_tests.rs b/.config/jp/tools/src/fs/utils_tests.rs index 08b4d72ae..31ccf1af2 100644 --- a/.config/jp/tools/src/fs/utils_tests.rs +++ b/.config/jp/tools/src/fs/utils_tests.rs @@ -492,85 +492,3 @@ mod resolve_workspace_entry { assert!(err.contains("relative"), "unexpected error: {err}"); } } - -mod clean_workspace_path { - use super::*; - - #[test] - fn rejects_absolute_path() { - let dir = tempdir().unwrap(); - let err = clean_workspace_path(dir.path(), "/etc/passwd", None).unwrap_err(); - assert!(err.contains("relative"), "unexpected error: {err}"); - } - - #[test] - fn rejects_escaping_parent_dir() { - let dir = tempdir().unwrap(); - let err = clean_workspace_path(dir.path(), "../../etc/passwd", None).unwrap_err(); - assert!( - err.contains("escape the workspace"), - "unexpected error: {err}" - ); - } - - #[test] - fn rejects_empty_path() { - let dir = tempdir().unwrap(); - let err = clean_workspace_path(dir.path(), "", None).unwrap_err(); - assert!(err.contains("empty"), "unexpected error: {err}"); - } - - #[test] - fn accepts_normal_path_and_returns_cleaned_form() { - let dir = tempdir().unwrap(); - let cleaned = clean_workspace_path(dir.path(), "src/main.rs", None).unwrap(); - assert_eq!(cleaned, Utf8PathBuf::from("src/main.rs")); - } - - #[test] - fn collapses_redundant_components() { - let dir = tempdir().unwrap(); - std::fs::create_dir(dir.path().join("sub")).unwrap(); - let cleaned = clean_workspace_path(dir.path(), "sub/../target.rs", None).unwrap(); - assert_eq!(cleaned, Utf8PathBuf::from("target.rs")); - } - - #[cfg(unix)] - #[test] - fn preserves_symlink_input_shape() { - // Where `resolve_workspace_path` would canonicalize the symlink and - // return `real/foo.rs`, `clean_workspace_path` keeps the user's - // input shape `link/foo.rs` — while still checking the escape. - let workspace = tempdir().unwrap(); - std::fs::create_dir(workspace.path().join("real")).unwrap(); - std::fs::write(workspace.path().join("real/foo.rs"), "").unwrap(); - std::os::unix::fs::symlink( - workspace.path().join("real").as_std_path(), - workspace.path().join("link").as_std_path(), - ) - .unwrap(); - - let cleaned = clean_workspace_path(workspace.path(), "link/foo.rs", None).unwrap(); - assert_eq!(cleaned, Utf8PathBuf::from("link/foo.rs")); - } - - #[cfg(unix)] - #[test] - fn rejects_symlink_escaping_workspace() { - let outside = tempdir().unwrap(); - std::fs::create_dir(outside.path().join("real")).unwrap(); - - let workspace = tempdir().unwrap(); - std::os::unix::fs::symlink( - outside.path().join("real").as_std_path(), - workspace.path().join("linkdir").as_std_path(), - ) - .unwrap(); - - let err = clean_workspace_path(workspace.path(), "linkdir/file.rs", None).unwrap_err(); - assert!( - err.contains("escapes the workspace"), - "unexpected error: {err}" - ); - } -} diff --git a/.ignore b/.ignore index 9578abbd4..d861692ad 100644 --- a/.ignore +++ b/.ignore @@ -20,7 +20,21 @@ !.github/** !justfile -# Then the exclusions (applied within the un-ignored trees) +# `.gitignore` is deliberately not consulted by the `fs_*` tools: with a +# whitelist, honouring it would only narrow results inside trees already +# un-ignored above, and everything it would newly hide is listed below anyway. + +# Then the exclusions (applied within the un-ignored trees). +# +# These govern what the `fs_*` tools *surface*, not what they may open: an +# excluded path stays out of unscoped listings and searches, but a tool call +# naming it outright still reaches it — which is the point, since a path the +# user hands over is one they want read. +# +# Two neighbouring lists cover the rest, in .jp/mcp/tools/fs/*.toml: +# +# options.suppress — read by the tool, never returned to the model +# access.fs — what the tool process may touch at all .jp/conversations/ .jp/local-conversations/ docs/.yarn/ diff --git a/.jp/mcp/tools/fs/grep_files.toml b/.jp/mcp/tools/fs/grep_files.toml index 821266374..87def89cd 100644 --- a/.jp/mcp/tools/fs/grep_files.toml +++ b/.jp/mcp/tools/fs/grep_files.toml @@ -2,6 +2,15 @@ enable = false run = "unattended" +# Paths this tool may read but never returns. Their contents would either flood +# the response or expose something the model has no business seeing. Reading them +# is a separate question, governed by `access.fs` — `.git` in particular stays +# readable, since that is how the write tools check for uncommitted work. +# +# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these +# in step across the `fs_*` tools that return file contents or paths. +options.suppress = [".git/", "**/target/"] + source = "local" command = "just serve-tools {{context}} {{tool}}" summary = "Grep files in the project's local filesystem." @@ -53,5 +62,5 @@ type = "array" items.type = "string" summary = "Optional list of files or directories to search." description = """ -If unspecified, all files in the project will be returned. +If unspecified, all files in the project are searched. """ diff --git a/.jp/mcp/tools/fs/grep_user_docs.toml b/.jp/mcp/tools/fs/grep_user_docs.toml index 01ac9b434..f4a3d5000 100644 --- a/.jp/mcp/tools/fs/grep_user_docs.toml +++ b/.jp/mcp/tools/fs/grep_user_docs.toml @@ -2,6 +2,15 @@ enable = false run = "unattended" +# Paths this tool may read but never returns. Their contents would either flood +# the response or expose something the model has no business seeing. Reading them +# is a separate question, governed by `access.fs` — `.git` in particular stays +# readable, since that is how the write tools check for uncommitted work. +# +# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these +# in step across the `fs_*` tools that return file contents or paths. +options.suppress = [".git/", "**/target/"] + source = "local" command = "just serve-tools {{context}} {{tool}}" summary = "Grep the project's user documentation." @@ -12,9 +21,9 @@ Search for a topic: {"pattern": "tool configuration"} ``` -Return entire matching files: +Search with more surrounding context: ```json -{"pattern": "attachments", "return_entire_file": true} +{"pattern": "attachments", "context": 5} ``` """ @@ -28,12 +37,7 @@ type = "string" required = true summary = "Regular expression to filter the results by." -[conversation.tools.fs_grep_user_docs.parameters.return_entire_file] -type = "boolean" -default = false -summary = "Whether to return the entire file contents." -description = """ -If enabled, the tool will return the entire file contents of any files matching -the pattern. If disabled (the default), only the matching lines and 5 contextual -lines above and below the matching lines will be returned. -""" +[conversation.tools.fs_grep_user_docs.parameters.context] +type = "integer" +default = 0 +summary = "Number of lines of context to include before and after the matching lines." diff --git a/.jp/mcp/tools/fs/list_files.toml b/.jp/mcp/tools/fs/list_files.toml index 2126670ad..0e65b4dc2 100644 --- a/.jp/mcp/tools/fs/list_files.toml +++ b/.jp/mcp/tools/fs/list_files.toml @@ -2,6 +2,15 @@ enable = false run = "unattended" +# Paths this tool may read but never returns. Their contents would either flood +# the response or expose something the model has no business seeing. Reading them +# is a separate question, governed by `access.fs` — `.git` in particular stays +# readable, since that is how the write tools check for uncommitted work. +# +# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these +# in step across the `fs_*` tools that return file contents or paths. +options.suppress = [".git/", "**/target/"] + source = "local" command = "just serve-tools {{context}} {{tool}}" summary = "List files in the project's local filesystem." @@ -33,7 +42,7 @@ type = "array" items.type = "string" summary = "Optional list of path prefixes to filter the results by." description = """ -If unspecified, all files in the project will be returned. +If unspecified, all files in the project are returned. """ [conversation.tools.fs_list_files.parameters.extensions] diff --git a/.jp/mcp/tools/fs/read_file.toml b/.jp/mcp/tools/fs/read_file.toml index f2275e85f..ed492f5cc 100644 --- a/.jp/mcp/tools/fs/read_file.toml +++ b/.jp/mcp/tools/fs/read_file.toml @@ -1,6 +1,15 @@ [conversation.tools.fs_read_file] enable = false run = "unattended" + +# Paths this tool may read but never returns. Their contents would either flood +# the response or expose something the model has no business seeing. Reading them +# is a separate question, governed by `access.fs` — `.git` in particular stays +# readable, since that is how the write tools check for uncommitted work. +# +# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these +# in step across the `fs_*` tools that return file contents or paths. +options.suppress = [".git/", "**/target/"] source = "local" command = "just serve-tools {{context}} {{tool}}" summary = "Read the contents of a file in the project's local filesystem." diff --git a/Cargo.lock b/Cargo.lock index f9870b170..552340901 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1013,7 +1013,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.61.0", + "windows-sys 0.59.0", ] [[package]] @@ -1147,7 +1147,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.0", + "windows-sys 0.52.0", ] [[package]] @@ -1455,9 +1455,9 @@ checksum = "0cc23270f6e1808e30a928bdc84dea0b9b4136a8bc82338574f23baf47bbd280" [[package]] name = "globset" -version = "0.4.16" +version = "0.4.19" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "54a1028dfc5f5df5da8a56a73e6c153c9a9708ec57232470703592a3f18e49f5" +checksum = "e47d37d2ae4464254884b60ab7071be2b876a9c35b696bd018ddcc76847309cd" dependencies = [ "aho-corasick", "bstr", @@ -1905,9 +1905,9 @@ dependencies = [ [[package]] name = "ignore" -version = "0.4.23" +version = "0.4.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6d89fd380afde86567dfba715db065673989d6253f42b88179abd3eae47bda4b" +checksum = "7f8a7b8211e695a1d0cd91cace480d4d0bd57667ab10277cc412c5f7f4884f83" dependencies = [ "crossbeam-deque", "globset", @@ -3735,7 +3735,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.61.0", + "windows-sys 0.52.0", ] [[package]] @@ -4464,7 +4464,7 @@ dependencies = [ "getrandom 0.3.4", "once_cell", "rustix", - "windows-sys 0.61.0", + "windows-sys 0.52.0", ] [[package]] @@ -5290,7 +5290,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.0", + "windows-sys 0.52.0", ] [[package]] diff --git a/crates/jp_tool/src/access.rs b/crates/jp_tool/src/access.rs index 6e11f3d06..6f87efd46 100644 --- a/crates/jp_tool/src/access.rs +++ b/crates/jp_tool/src/access.rs @@ -124,9 +124,34 @@ impl AccessPolicy { } } - /// The workspace-relative grant paths, for building helpful error messages. - pub fn grant_paths(&self) -> impl Iterator { - self.fs.iter().map(FsRule::lexical_path) + /// The workspace-relative paths whose rules grant `capability`, for + /// building helpful error messages. + /// + /// Only granting rules are listed. + /// A rule that denies the capability is not somewhere the caller can go, so + /// naming it as a grant would send the reader straight back into the + /// refusal. + /// + /// The workspace root is reported as `.`, the form it is written in config; + /// its lexical path is empty and would otherwise render as nothing. + pub fn granting_paths(&self, capability: Capability) -> impl Iterator { + self.fs + .iter() + .filter(move |rule| match capability { + Capability::Read => rule.read(), + Capability::Create => rule.create(), + Capability::Update => rule.update(), + Capability::Delete => rule.delete(), + Capability::Execute => rule.execute(), + }) + .map(|rule| { + let path = rule.lexical_path(); + if path.as_str().is_empty() { + Utf8Path::new(".") + } else { + path + } + }) } } From 179b41cd9645e89ec185d6548f11fc83edfba983 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 27 Jul 2026 12:27:49 +0200 Subject: [PATCH 2/6] fixup! chore(tools): Suppress noisy/sensitive paths from fs tool results Signed-off-by: Jean Mertz --- .config/supply-chain/imports.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.config/supply-chain/imports.lock b/.config/supply-chain/imports.lock index fbdbeb75e..5782a23a3 100644 --- a/.config/supply-chain/imports.lock +++ b/.config/supply-chain/imports.lock @@ -213,8 +213,8 @@ user-login = "Byron" user-name = "Sebastian Thiel" [[publisher.globset]] -version = "0.4.16" -when = "2025-02-27" +version = "0.4.19" +when = "2026-07-15" user-id = 189 user-login = "BurntSushi" user-name = "Andrew Gallant" @@ -309,8 +309,8 @@ user-login = "seanmonstar" user-name = "Sean McArthur" [[publisher.ignore]] -version = "0.4.23" -when = "2024-09-09" +version = "0.4.31" +when = "2026-07-20" user-id = 189 user-login = "BurntSushi" user-name = "Andrew Gallant" From c04afcafd5ed47074dd8c140f45deb3ded4866f6 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 27 Jul 2026 13:53:29 +0200 Subject: [PATCH 3/6] review feedback Signed-off-by: Jean Mertz --- .config/jp/tools/src/fs.rs | 12 +++- .config/jp/tools/src/fs/grep_files_tests.rs | 2 +- .config/jp/tools/src/fs/list_files.rs | 16 +++-- .config/jp/tools/src/fs/list_files_tests.rs | 14 ++-- .config/jp/tools/src/fs/read_file.rs | 7 +- .config/jp/tools/src/fs/read_file_tests.rs | 44 +++++++++++- .config/jp/tools/src/fs/utils.rs | 56 +++++++++++++--- .config/jp/tools/src/fs/utils_tests.rs | 35 ++++++++++ .jp/mcp/tools/fs/grep_files.toml | 8 ++- .jp/mcp/tools/fs/grep_user_docs.toml | 8 ++- .jp/mcp/tools/fs/list_files.toml | 8 ++- .jp/mcp/tools/fs/read_file.toml | 16 +++-- crates/jp_tool/src/access.rs | 74 ++++++++++++++++++++- 13 files changed, 254 insertions(+), 46 deletions(-) diff --git a/.config/jp/tools/src/fs.rs b/.config/jp/tools/src/fs.rs index 682e7c23e..a6ee81ae9 100644 --- a/.config/jp/tools/src/fs.rs +++ b/.config/jp/tools/src/fs.rs @@ -27,8 +27,16 @@ pub async fn run(ctx: Context, t: Tool) -> ToolResult { // to hand file contents or paths back; the write tools return confirmations // rather than content, and what they may touch is the access policy's // question. - let patterns: Vec = t.option_or("suppress", vec![]); - let suppress = suppress_matcher(&ctx.root, &patterns); + // + // Parsed strictly rather than through `option_or`: a disclosure control that + // falls back to "suppress nothing" when its configuration is malformed hands + // over the very paths it was told to hold back, and does it silently. + let patterns: Vec = match t.options.get("suppress") { + None => vec![], + Some(value) => serde_json::from_value(value.clone()) + .map_err(|error| format!("Invalid `suppress` option for tool '{}': {error}", t.name))?, + }; + let suppress = suppress_matcher(&ctx.root, &patterns)?; match t.name.trim_start_matches("fs_") { "list_files" => fs_list_files( diff --git a/.config/jp/tools/src/fs/grep_files_tests.rs b/.config/jp/tools/src/fs/grep_files_tests.rs index 862eb3eaf..dcece3d8d 100644 --- a/.config/jp/tools/src/fs/grep_files_tests.rs +++ b/.config/jp/tools/src/fs/grep_files_tests.rs @@ -281,7 +281,7 @@ async fn suppressed_path_is_reported_so_the_caller_can_ask_the_user() { None, Some(vec![".git/HEAD".to_owned()].into()), None, - &suppress_matcher(root, &[".git/".to_owned()]), + &suppress_matcher(root, &[".git/".to_owned()]).unwrap(), ) .await .unwrap(); diff --git a/.config/jp/tools/src/fs/list_files.rs b/.config/jp/tools/src/fs/list_files.rs index 3324fffc1..249a06646 100644 --- a/.config/jp/tools/src/fs/list_files.rs +++ b/.config/jp/tools/src/fs/list_files.rs @@ -136,10 +136,9 @@ struct WalkSpec { path_filter: Option, /// Whether `.ignore` files prune this walk. /// - /// Disabled for a subtree the caller named outright and `soft_ignore` opted - /// in: the anchored root patterns do not prune reliably below the workspace - /// root, so leaving them on would prune unpredictably rather than not at - /// all. + /// Disabled for an `.ignore`d subtree the caller named outright: the + /// anchored root patterns do not prune reliably below the workspace root, + /// so leaving them on would prune unpredictably rather than not at all. apply_ignore: bool, } @@ -188,7 +187,8 @@ fn resolve_target( // rule for its real location, so a link cannot dodge a rule denying its // target. External mount paths have no canonical workspace-relative form and // keep their lexical shape, which is what external rules match. - let cleaned = resolve_workspace_path(root, prefix, access)?.relative; + let resolved = resolve_workspace_path(root, prefix, access)?; + let cleaned = resolved.relative; // Both checks come before the mount branch below: an approved mount is named // by its in-workspace path, which is the form access rules and suppress @@ -201,7 +201,9 @@ fn resolve_target( return Ok(Target::Skipped(Skipped::Denied(cleaned.into_string()))); } - if is_suppressed(suppress, &cleaned) { + // Both forms, so a pattern closes the path whether it names the real location + // or the symlink this request arrived through. + if is_suppressed(suppress, &[&cleaned, &resolved.lexical]) { return Ok(Target::Skipped(Skipped::Suppressed(cleaned.into_string()))); } @@ -363,7 +365,7 @@ fn collect_files( } else { display_prefix.join(relative) }; - !is_suppressed(&suppress, &display) + !is_suppressed(&suppress, &[&display]) }); } diff --git a/.config/jp/tools/src/fs/list_files_tests.rs b/.config/jp/tools/src/fs/list_files_tests.rs index 4689a550e..6862056e3 100644 --- a/.config/jp/tools/src/fs/list_files_tests.rs +++ b/.config/jp/tools/src/fs/list_files_tests.rs @@ -421,7 +421,7 @@ async fn suppressed_path_is_skipped_with_a_note() { None, Some(vec![".git".to_owned()].into()), None, - &suppress_matcher(root, &[".git/".to_owned()]), + &suppress_matcher(root, &[".git/".to_owned()]).unwrap(), ) .await .unwrap(); @@ -448,7 +448,7 @@ async fn suppress_pattern_covers_files_inside_the_named_directory() { None, Some(vec![".git/HEAD".to_owned()].into()), None, - &suppress_matcher(root, &[".git/".to_owned()]), + &suppress_matcher(root, &[".git/".to_owned()]).unwrap(), ) .await .unwrap(); @@ -475,7 +475,7 @@ async fn suppress_patterns_match_at_any_depth() { None, Some(vec!["crates/inner/target".to_owned()].into()), None, - &suppress_matcher(root, &["**/target/".to_owned()]), + &suppress_matcher(root, &["**/target/".to_owned()]).unwrap(), ) .await .unwrap(); @@ -506,7 +506,7 @@ async fn an_in_workspace_symlink_cannot_dodge_suppression() { None, Some(vec!["gitlink/HEAD".to_owned()].into()), None, - &suppress_matcher(root, &[".git/".to_owned()]), + &suppress_matcher(root, &[".git/".to_owned()]).unwrap(), ) .await .unwrap(); @@ -534,7 +534,7 @@ async fn suppressed_tree_is_pruned_from_traversal_without_an_ignore_entry() { None, None, None, - &suppress_matcher(root, &["secrets/".to_owned()]), + &suppress_matcher(root, &["secrets/".to_owned()]).unwrap(), ) .await .unwrap(); @@ -573,7 +573,7 @@ async fn suppression_reaches_inside_an_approved_external_mount() { Some(&policy), Some(vec!["fork".to_owned()].into()), None, - &suppress_matcher(ws.path(), &["**/.git/".to_owned()]), + &suppress_matcher(ws.path(), &["**/.git/".to_owned()]).unwrap(), ) .await .unwrap(); @@ -595,7 +595,7 @@ async fn suppressed_path_does_not_suppress_the_other_requested_paths() { None, Some(vec![".git".to_owned(), "src".to_owned()].into()), None, - &suppress_matcher(root, &[".git/".to_owned()]), + &suppress_matcher(root, &[".git/".to_owned()]).unwrap(), ) .await .unwrap(); diff --git a/.config/jp/tools/src/fs/read_file.rs b/.config/jp/tools/src/fs/read_file.rs index 7495bab06..775bfd334 100644 --- a/.config/jp/tools/src/fs/read_file.rs +++ b/.config/jp/tools/src/fs/read_file.rs @@ -18,9 +18,10 @@ pub(crate) async fn fs_read_file( if let Err(msg) = authorize(ctx.access.as_ref(), Capability::Read, &resolved.relative) { return error(msg); } - // Matched on the canonical form, so a path reached through an in-workspace - // symlink is checked against the pattern for its real location. - if is_suppressed(suppress, &resolved.relative) { + // Matched on both forms: the canonical one so a symlink cannot dodge a + // pattern naming its target, and the caller's own spelling so a pattern + // naming the link closes that route too. + if is_suppressed(suppress, &[&resolved.relative, &resolved.lexical]) { return error(suppressed_note(resolved.relative.as_str())); } let absolute_path = resolved.absolute; diff --git a/.config/jp/tools/src/fs/read_file_tests.rs b/.config/jp/tools/src/fs/read_file_tests.rs index 1874f8a76..c3e9bc0b8 100644 --- a/.config/jp/tools/src/fs/read_file_tests.rs +++ b/.config/jp/tools/src/fs/read_file_tests.rs @@ -212,7 +212,7 @@ async fn refuses_a_suppressed_path() { let result = fs_read_file( &ctx, - &suppress_matcher(workspace.path(), &[".git/".to_owned()]), + &suppress_matcher(workspace.path(), &[".git/".to_owned()]).unwrap(), ".git/HEAD".to_owned(), None, None, @@ -230,6 +230,48 @@ async fn refuses_a_suppressed_path() { ); } +#[cfg(unix)] +#[tokio::test] +async fn a_pattern_naming_a_symlink_closes_that_route() { + // Suppression matches the caller's own spelling as well as the canonical + // form, so a pattern written against the link name is honored when a request + // arrives through it. + use std::os::unix::fs::symlink; + + let workspace = tempdir().unwrap(); + std::fs::create_dir(workspace.path().join("real")).unwrap(); + std::fs::write(workspace.path().join("real/secret.txt"), "shhh").unwrap(); + symlink("real", workspace.path().join("alias")).unwrap(); + + let ctx = Context { + root: workspace.path().to_path_buf(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + }; + let suppress = suppress_matcher(workspace.path(), &["alias/".to_owned()]).unwrap(); + + let via_alias = fs_read_file(&ctx, &suppress, "alias/secret.txt".to_owned(), None, None) + .await + .unwrap(); + assert!( + matches!(via_alias, Outcome::Error { .. }), + "the configured spelling should be refused, got: {via_alias:?}" + ); + + // The other half of the same fact: a pattern naming a link closes that + // spelling and nothing else. Patterns belong on real locations; matching the + // as-written form is a convenience, not a boundary. + let via_real = fs_read_file(&ctx, &suppress, "real/secret.txt".to_owned(), None, None) + .await + .unwrap(); + assert!( + matches!(via_real, Outcome::Success { .. }), + "a pattern on the link name does not cover the target, got: {via_real:?}" + ); +} + #[cfg(unix)] #[tokio::test] async fn denies_in_workspace_path_with_no_matching_grant() { diff --git a/.config/jp/tools/src/fs/utils.rs b/.config/jp/tools/src/fs/utils.rs index 041714191..d3fe06db4 100644 --- a/.config/jp/tools/src/fs/utils.rs +++ b/.config/jp/tools/src/fs/utils.rs @@ -21,15 +21,30 @@ use crate::{ /// /// Patterns use `.ignore` syntax, so `**/target/` matches at any depth, and are /// matched against workspace-relative paths. -/// An unparseable pattern is skipped; the rest of the list still applies. /// An empty list suppresses nothing, which is the default: what counts as noise /// or as sensitive is a property of the project, not of a directory's name. -pub fn suppress_matcher(root: &Utf8Path, patterns: &[String]) -> Gitignore { +/// +/// Patterns should name real locations rather than symlinks pointing at them. +/// A path is matched in both its canonical and its as-written form, so naming a +/// link does close that spelling — but only that one, and any other route to +/// the same target stays open. +/// +/// # Errors +/// +/// Returns an error naming the offending pattern if one cannot be parsed as a +/// glob. +/// A disclosure control that quietly drops the rule it could not read is worse +/// than one that refuses to start. +pub fn suppress_matcher(root: &Utf8Path, patterns: &[String]) -> Result { let mut builder = GitignoreBuilder::new(root); for pattern in patterns { - let _result = builder.add_line(None, pattern); + builder + .add_line(None, pattern) + .map_err(|error| format!("Invalid `suppress` pattern '{pattern}': {error}"))?; } - builder.build().unwrap_or_else(|_| Gitignore::empty()) + builder + .build() + .map_err(|error| format!("Could not compile the `suppress` patterns: {error}")) } /// Whether `suppress` keeps `relative` out of a tool's results. @@ -42,10 +57,14 @@ pub fn suppress_matcher(root: &Utf8Path, patterns: &[String]) -> Gitignore { /// A pattern written `secrets/` is meant to cover that name, and wrongly /// suppressing a file that happens to share it costs far less than returning a /// directory's contents. -pub fn is_suppressed(suppress: &Gitignore, relative: &Utf8Path) -> bool { - suppress - .matched_path_or_any_parents(relative, true) - .is_ignore() +/// +/// Pass [`ResolvedPath::relative`] and [`ResolvedPath::lexical`] so a pattern +/// matches whether it names the real location or the symlink a request arrived +/// through. +pub fn is_suppressed(suppress: &Gitignore, forms: &[&Utf8Path]) -> bool { + forms + .iter() + .any(|form| suppress.matched_path_or_any_parents(form, true).is_ignore()) } /// Report that a path was suppressed from a tool's results. @@ -252,6 +271,15 @@ pub struct ResolvedPath { /// Path relative to the canonical workspace root. pub relative: Utf8PathBuf, + + /// The caller's own spelling, lexically normalized and workspace-relative. + /// + /// Equal to `relative` unless the request arrived through an in-workspace + /// symlink, in which case this keeps the link name and `relative` holds the + /// target's real location. + /// Rules match on `relative`; this exists for checks that should also honor + /// the name the caller used. + pub lexical: Utf8PathBuf, } /// Resolve a user-supplied path against the workspace root, following symlinks @@ -297,7 +325,11 @@ pub fn resolve_workspace_path( let relative = workspace_relative(&absolute, &canonical_root, &cleaned); - Ok(ResolvedPath { absolute, relative }) + Ok(ResolvedPath { + absolute, + relative, + lexical: cleaned, + }) } /// Resolve a user-supplied path as a directory entry, canonicalizing only the @@ -350,7 +382,11 @@ pub fn resolve_workspace_entry( let relative = workspace_relative(&absolute, &canonical_root, &cleaned); - Ok(ResolvedPath { absolute, relative }) + Ok(ResolvedPath { + absolute, + relative, + lexical: cleaned, + }) } /// Output of [`validate_workspace_input`]: the cleaned form plus the diff --git a/.config/jp/tools/src/fs/utils_tests.rs b/.config/jp/tools/src/fs/utils_tests.rs index 31ccf1af2..0faa64491 100644 --- a/.config/jp/tools/src/fs/utils_tests.rs +++ b/.config/jp/tools/src/fs/utils_tests.rs @@ -4,6 +4,41 @@ use camino_tempfile::tempdir; use super::*; use crate::util::runner::MockProcessRunner; +mod suppress_matcher { + use super::*; + + #[test] + fn rejects_an_unparseable_pattern_naming_it() { + // A disclosure control that drops the rule it could not read hands over + // the very paths it was configured to hold back. + let dir = tempdir().unwrap(); + let err = + super::super::suppress_matcher(dir.path(), &["{unclosed".to_owned()]).unwrap_err(); + + assert!(err.contains("{unclosed"), "pattern not named: {err}"); + assert!( + err.starts_with("Invalid `suppress` pattern"), + "unexpected error: {err}" + ); + } + + #[test] + fn one_bad_pattern_rejects_the_whole_list() { + let dir = tempdir().unwrap(); + let patterns = [".git/".to_owned(), "{unclosed".to_owned()]; + + assert!(super::super::suppress_matcher(dir.path(), &patterns).is_err()); + } + + #[test] + fn accepts_an_empty_list() { + let dir = tempdir().unwrap(); + let matcher = super::super::suppress_matcher(dir.path(), &[]).unwrap(); + + assert!(!is_suppressed(&matcher, &[Utf8Path::new("anything")])); + } +} + #[test] fn test_is_file_dirty_modified() { let dir = tempdir().unwrap(); diff --git a/.jp/mcp/tools/fs/grep_files.toml b/.jp/mcp/tools/fs/grep_files.toml index 87def89cd..ff1422bd6 100644 --- a/.jp/mcp/tools/fs/grep_files.toml +++ b/.jp/mcp/tools/fs/grep_files.toml @@ -7,8 +7,12 @@ run = "unattended" # is a separate question, governed by `access.fs` — `.git` in particular stays # readable, since that is how the write tools check for uncommitted work. # -# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these -# in step across the `fs_*` tools that return file contents or paths. +# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. +# +# Disclosure blocks like `.git/` belong on every tool that returns contents, so +# keep those in step. `**/target/` is a flood block and lives only on the tools +# that enumerate in bulk; `fs_read_file` omits it deliberately, so an exact build +# log stays retrievable (issue #626). options.suppress = [".git/", "**/target/"] source = "local" diff --git a/.jp/mcp/tools/fs/grep_user_docs.toml b/.jp/mcp/tools/fs/grep_user_docs.toml index f4a3d5000..c55117e5c 100644 --- a/.jp/mcp/tools/fs/grep_user_docs.toml +++ b/.jp/mcp/tools/fs/grep_user_docs.toml @@ -7,8 +7,12 @@ run = "unattended" # is a separate question, governed by `access.fs` — `.git` in particular stays # readable, since that is how the write tools check for uncommitted work. # -# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these -# in step across the `fs_*` tools that return file contents or paths. +# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. +# +# Disclosure blocks like `.git/` belong on every tool that returns contents, so +# keep those in step. `**/target/` is a flood block and lives only on the tools +# that enumerate in bulk; `fs_read_file` omits it deliberately, so an exact build +# log stays retrievable (issue #626). options.suppress = [".git/", "**/target/"] source = "local" diff --git a/.jp/mcp/tools/fs/list_files.toml b/.jp/mcp/tools/fs/list_files.toml index 0e65b4dc2..badc41db0 100644 --- a/.jp/mcp/tools/fs/list_files.toml +++ b/.jp/mcp/tools/fs/list_files.toml @@ -7,8 +7,12 @@ run = "unattended" # is a separate question, governed by `access.fs` — `.git` in particular stays # readable, since that is how the write tools check for uncommitted work. # -# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these -# in step across the `fs_*` tools that return file contents or paths. +# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. +# +# Disclosure blocks like `.git/` belong on every tool that returns contents, so +# keep those in step. `**/target/` is a flood block and lives only on the tools +# that enumerate in bulk; `fs_read_file` omits it deliberately, so an exact build +# log stays retrievable (issue #626). options.suppress = [".git/", "**/target/"] source = "local" diff --git a/.jp/mcp/tools/fs/read_file.toml b/.jp/mcp/tools/fs/read_file.toml index ed492f5cc..7ce910599 100644 --- a/.jp/mcp/tools/fs/read_file.toml +++ b/.jp/mcp/tools/fs/read_file.toml @@ -2,14 +2,16 @@ enable = false run = "unattended" -# Paths this tool may read but never returns. Their contents would either flood -# the response or expose something the model has no business seeing. Reading them -# is a separate question, governed by `access.fs` — `.git` in particular stays -# readable, since that is how the write tools check for uncommitted work. +# Paths this tool may read but never returns. Reading them is a separate +# question, governed by `access.fs` — `.git` in particular stays readable, since +# that is how the write tools check for uncommitted work. # -# Patterns use `.ignore` syntax, so `**/target/` matches at any depth. Keep these -# in step across the `fs_*` tools that return file contents or paths. -options.suppress = [".git/", "**/target/"] +# Shorter than the list/grep tools on purpose. Those carry `**/target/` because +# bulk enumeration of build output floods a response; an exact path with a line +# range cannot, and issue #626 asks that build logs stay retrievable when someone +# knows the file they want. Disclosure blocks like `.git/` belong on every tool +# that returns contents; flood blocks belong only where flooding is possible. +options.suppress = [".git/"] source = "local" command = "just serve-tools {{context}} {{tool}}" summary = "Read the contents of a file in the project's local filesystem." diff --git a/crates/jp_tool/src/access.rs b/crates/jp_tool/src/access.rs index 6f87efd46..ba3a80141 100644 --- a/crates/jp_tool/src/access.rs +++ b/crates/jp_tool/src/access.rs @@ -132,11 +132,29 @@ impl AccessPolicy { /// naming it as a grant would send the reader straight back into the /// refusal. /// + /// Rules sharing a lexical path are collapsed to the last one declared, + /// matching how [`AccessPolicy::permits`] breaks equal-specificity ties. + /// Appending config layers routinely produces such duplicates, and a grant + /// a later layer has overridden is not a place the caller can go either. + /// Shadowing across *different* paths is left alone: a root grant is still + /// worth naming when a deeper rule denies one subtree. + /// /// The workspace root is reported as `.`, the form it is written in config; /// its lexical path is empty and would otherwise render as nothing. pub fn granting_paths(&self, capability: Capability) -> impl Iterator { - self.fs - .iter() + let mut effective: Vec<&FsRule> = vec![]; + for rule in &self.fs { + match effective + .iter_mut() + .find(|kept| kept.lexical_path() == rule.lexical_path()) + { + Some(kept) => *kept = rule, + None => effective.push(rule), + } + } + + effective + .into_iter() .filter(move |rule| match capability { Capability::Read => rule.read(), Capability::Create => rule.create(), @@ -710,6 +728,58 @@ mod tests { assert!(rule.read()); } + #[test] + fn granting_paths_drops_a_grant_a_later_rule_shadows() { + // Append-merged config layers routinely produce two rules on one path. + // `permits` breaks the tie toward the last, so the earlier grant is dead + // and naming it in a refusal points the reader back at what was refused. + let policy = AccessPolicy { + fs: vec![ + FsRule::new("src").with_read(true), + FsRule::new("src").with_read(false), + ], + ..AccessPolicy::default() + }; + + assert!(!policy.permits(Capability::Read, Utf8Path::new("src/lib.rs"))); + assert_eq!(policy.granting_paths(Capability::Read).count(), 0); + } + + #[test] + fn granting_paths_keeps_a_grant_a_later_rule_restores() { + let policy = AccessPolicy { + fs: vec![ + FsRule::new("src").with_read(false), + FsRule::new("src").with_read(true), + ], + ..AccessPolicy::default() + }; + + assert!(policy.permits(Capability::Read, Utf8Path::new("src/lib.rs"))); + assert_eq!( + policy.granting_paths(Capability::Read).collect::>(), + vec![Utf8Path::new("src")] + ); + } + + #[test] + fn granting_paths_keeps_a_broader_grant_a_deeper_rule_denies() { + // Cross-path shadowing is not collapsed: the root is still reachable + // everywhere except the one subtree the deeper rule closes. + let policy = AccessPolicy { + fs: vec![ + FsRule::new("").with_read(true), + FsRule::new("src").with_read(false), + ], + ..AccessPolicy::default() + }; + + assert_eq!( + policy.granting_paths(Capability::Read).collect::>(), + vec![Utf8Path::new(".")] + ); + } + #[test] fn write_alias_expands() { let rule = FsRule::new("x").with_write(true); From 0ab0e47a5fecae8158e8d6b0226fadf7d86fcc96 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 27 Jul 2026 15:22:19 +0200 Subject: [PATCH 4/6] review feedback Signed-off-by: Jean Mertz --- .config/jp/tools/src/fs.rs | 4 + .config/jp/tools/src/fs/delete_file_tests.rs | 4 +- .config/jp/tools/src/fs/fs_tests.rs | 85 ++++++++++++++++++++ .config/jp/tools/src/fs/grep_files_tests.rs | 4 +- .config/jp/tools/src/fs/list_files_tests.rs | 22 +++-- .config/jp/tools/src/fs/move_file_tests.rs | 6 +- .config/jp/tools/src/fs/read_file_tests.rs | 4 +- .ignore | 4 + 8 files changed, 122 insertions(+), 11 deletions(-) create mode 100644 .config/jp/tools/src/fs/fs_tests.rs diff --git a/.config/jp/tools/src/fs.rs b/.config/jp/tools/src/fs.rs index a6ee81ae9..d1d0d34e9 100644 --- a/.config/jp/tools/src/fs.rs +++ b/.config/jp/tools/src/fs.rs @@ -124,3 +124,7 @@ pub async fn run(ctx: Context, t: Tool) -> ToolResult { _ => Err(format!("Unknown tool '{}'", t.name).into()), } } + +#[cfg(test)] +#[path = "fs/fs_tests.rs"] +mod tests; diff --git a/.config/jp/tools/src/fs/delete_file_tests.rs b/.config/jp/tools/src/fs/delete_file_tests.rs index f1c2a800a..b1d7fd42d 100644 --- a/.config/jp/tools/src/fs/delete_file_tests.rs +++ b/.config/jp/tools/src/fs/delete_file_tests.rs @@ -208,8 +208,10 @@ async fn refuses_to_delete_a_path_a_deny_rule_closes() { let Outcome::Error { message, .. } = result else { panic!("expected a refusal, got: {result:?}"); }; + // Normalized: a resolved path carries native separators, so the message reads + // `.git\HEAD` on Windows. assert_eq!( - message, + message.replace('\\', "/"), "Access denied: cannot delete '.git/HEAD'. Paths granting delete: [.]. If required, ask \ the user for explicit access." ); diff --git a/.config/jp/tools/src/fs/fs_tests.rs b/.config/jp/tools/src/fs/fs_tests.rs new file mode 100644 index 000000000..428a384a7 --- /dev/null +++ b/.config/jp/tools/src/fs/fs_tests.rs @@ -0,0 +1,85 @@ +use camino_tempfile::tempdir; +use jp_tool::{Action, Context}; +use serde_json::{Map, Value, json}; + +use super::*; + +/// A `list_files` invocation carrying the given `suppress` option value. +fn list_files_with_suppress(root: &camino::Utf8Path, suppress: Value) -> (Context, Tool) { + let ctx = Context { + root: root.to_path_buf(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + }; + let tool = Tool { + name: "fs_list_files".to_owned(), + arguments: Map::new(), + answers: Map::new(), + options: Map::from_iter([("suppress".to_owned(), suppress)]), + }; + + (ctx, tool) +} + +#[tokio::test] +async fn a_non_array_suppress_option_fails_the_invocation() { + // The strict parse replaced `option_or`, which turned any unreadable value + // into an empty list and handed over the very paths the option named. This + // drives the dispatcher itself, so a regression back to the lenient read is + // caught here rather than by nobody. + let tmp = tempdir().unwrap(); + std::fs::write(tmp.path().join("a.txt"), "").unwrap(); + + let (ctx, tool) = list_files_with_suppress(tmp.path(), json!("oops")); + let err = run(ctx, tool).await.expect_err("expected a hard failure"); + + assert!( + err.to_string() + .starts_with("Invalid `suppress` option for tool 'fs_list_files'"), + "unexpected error: {err}" + ); +} + +#[tokio::test] +async fn an_object_suppress_option_fails_the_invocation() { + let tmp = tempdir().unwrap(); + std::fs::write(tmp.path().join("a.txt"), "").unwrap(); + + let (ctx, tool) = list_files_with_suppress(tmp.path(), json!({"paths": [".git/"]})); + let err = run(ctx, tool).await.expect_err("expected a hard failure"); + + assert!( + err.to_string() + .starts_with("Invalid `suppress` option for tool 'fs_list_files'"), + "unexpected error: {err}" + ); +} + +#[tokio::test] +async fn an_unparseable_suppress_pattern_fails_the_invocation() { + // The value deserializes but the glob does not compile: the other half of + // the same "never silently suppress nothing" guarantee, checked through the + // dispatcher rather than against `suppress_matcher` directly. + let tmp = tempdir().unwrap(); + std::fs::write(tmp.path().join("a.txt"), "").unwrap(); + + let (ctx, tool) = list_files_with_suppress(tmp.path(), json!(["{unclosed"])); + let err = run(ctx, tool).await.expect_err("expected a hard failure"); + + assert!( + err.to_string().contains("{unclosed"), + "pattern not named: {err}" + ); +} + +#[tokio::test] +async fn a_valid_suppress_option_is_accepted() { + let tmp = tempdir().unwrap(); + std::fs::write(tmp.path().join("a.txt"), "").unwrap(); + + let (ctx, tool) = list_files_with_suppress(tmp.path(), json!([".git/", "**/target/"])); + + assert!(run(ctx, tool).await.is_ok()); +} diff --git a/.config/jp/tools/src/fs/grep_files_tests.rs b/.config/jp/tools/src/fs/grep_files_tests.rs index dcece3d8d..36f778bb9 100644 --- a/.config/jp/tools/src/fs/grep_files_tests.rs +++ b/.config/jp/tools/src/fs/grep_files_tests.rs @@ -286,8 +286,10 @@ async fn suppressed_path_is_reported_so_the_caller_can_ask_the_user() { .await .unwrap(); + // Normalized: a resolved path carries native separators, so the note reads + // `.git\HEAD` on Windows. assert_eq!( - matches, + matches.replace('\\', "/"), "No matches found in the paths that were searched.\n\nNote: '.git/HEAD' is suppressed \ from this tool's results. If you need it, ask the user to provide it." ); diff --git a/.config/jp/tools/src/fs/list_files_tests.rs b/.config/jp/tools/src/fs/list_files_tests.rs index 6862056e3..c4ba4a04a 100644 --- a/.config/jp/tools/src/fs/list_files_tests.rs +++ b/.config/jp/tools/src/fs/list_files_tests.rs @@ -285,6 +285,16 @@ fn fixtures_workspace(root: &camino::Utf8Path) { std::fs::write(root.join("crates/tests/fixtures/a.snap"), "").unwrap(); } +/// Notes with native separators normalized, so exact assertions hold on Windows +/// where a resolved path carries `\`. +fn notes(files: &Files) -> Vec { + files + .notes() + .iter() + .map(|note| note.replace('\\', "/")) + .collect() +} + fn listed(files: Files) -> Vec { files .into_files() @@ -426,7 +436,7 @@ async fn suppressed_path_is_skipped_with_a_note() { .await .unwrap(); - assert_eq!(files.notes(), vec![ + assert_eq!(notes(&files), vec![ "'.git' is suppressed from this tool's results. If you need it, ask the user to provide \ it." .to_owned() @@ -453,7 +463,7 @@ async fn suppress_pattern_covers_files_inside_the_named_directory() { .await .unwrap(); - assert_eq!(files.notes(), vec![ + assert_eq!(notes(&files), vec![ "'.git/HEAD' is suppressed from this tool's results. If you need it, ask the user to \ provide it." .to_owned() @@ -480,7 +490,7 @@ async fn suppress_patterns_match_at_any_depth() { .await .unwrap(); - assert_eq!(files.notes(), vec![ + assert_eq!(notes(&files), vec![ "'crates/inner/target' is suppressed from this tool's results. If you need it, ask the \ user to provide it." .to_owned() @@ -511,7 +521,7 @@ async fn an_in_workspace_symlink_cannot_dodge_suppression() { .await .unwrap(); - assert_eq!(files.notes(), vec![ + assert_eq!(notes(&files), vec![ "'.git/HEAD' is suppressed from this tool's results. If you need it, ask the user to \ provide it." .to_owned() @@ -600,7 +610,7 @@ async fn suppressed_path_does_not_suppress_the_other_requested_paths() { .await .unwrap(); - assert_eq!(files.notes(), vec![ + assert_eq!(notes(&files), vec![ "'.git' is suppressed from this tool's results. If you need it, ask the user to provide \ it." .to_owned() @@ -633,7 +643,7 @@ async fn explicitly_named_file_respects_read_policy() { .await .unwrap(); - assert_eq!(files.notes(), vec![ + assert_eq!(notes(&files), vec![ "'secret.txt' is not readable by this tool and was skipped. If you need it, ask the user \ to provide it." .to_owned() diff --git a/.config/jp/tools/src/fs/move_file_tests.rs b/.config/jp/tools/src/fs/move_file_tests.rs index fbb33d051..40a20c3ea 100644 --- a/.config/jp/tools/src/fs/move_file_tests.rs +++ b/.config/jp/tools/src/fs/move_file_tests.rs @@ -157,8 +157,10 @@ fn refuses_to_move_out_of_a_path_a_deny_rule_closes() { ) .unwrap(); + // Normalized: a resolved path carries native separators, so the message reads + // `.git\HEAD` on Windows. assert_eq!( - unwrap_error(result), + unwrap_error(result).replace('\\', "/"), "Access denied: cannot delete '.git/HEAD'. Paths granting delete: [.]. If required, ask \ the user for explicit access." ); @@ -186,7 +188,7 @@ fn refuses_to_move_into_a_path_a_deny_rule_closes() { .unwrap(); assert_eq!( - unwrap_error(result), + unwrap_error(result).replace('\\', "/"), "Access denied: cannot create '.git/note.txt'. Paths granting create: [.]. If required, \ ask the user for explicit access." ); diff --git a/.config/jp/tools/src/fs/read_file_tests.rs b/.config/jp/tools/src/fs/read_file_tests.rs index c3e9bc0b8..ae3387822 100644 --- a/.config/jp/tools/src/fs/read_file_tests.rs +++ b/.config/jp/tools/src/fs/read_file_tests.rs @@ -223,8 +223,10 @@ async fn refuses_a_suppressed_path() { let Outcome::Error { message, .. } = result else { panic!("expected a refusal, got: {result:?}"); }; + // Normalized: a resolved path carries native separators, so the note reads + // `.git\HEAD` on Windows. assert_eq!( - message, + message.replace('\\', "/"), "'.git/HEAD' is suppressed from this tool's results. If you need it, ask the user to \ provide it." ); diff --git a/.ignore b/.ignore index d861692ad..be3fdec5c 100644 --- a/.ignore +++ b/.ignore @@ -37,6 +37,8 @@ # access.fs — what the tool process may touch at all .jp/conversations/ .jp/local-conversations/ +.jp/mcp/state/ +.jp/**/QUERY_MESSAGE.md docs/.yarn/ docs/.vitepress/cache/ docs/.vitepress/dist/ @@ -48,3 +50,5 @@ docs/yarn.lock **/fixtures/ docs2/ lcov.info +*.log +/rustc-ice-* From 876bd3116f1181bbfc1b4d9574094eaf2d5c38af Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 27 Jul 2026 15:39:00 +0200 Subject: [PATCH 5/6] review feedback Signed-off-by: Jean Mertz --- .config/jp/tools/src/fs/fs_tests.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.config/jp/tools/src/fs/fs_tests.rs b/.config/jp/tools/src/fs/fs_tests.rs index 428a384a7..189e0a905 100644 --- a/.config/jp/tools/src/fs/fs_tests.rs +++ b/.config/jp/tools/src/fs/fs_tests.rs @@ -25,10 +25,12 @@ fn list_files_with_suppress(root: &camino::Utf8Path, suppress: Value) -> (Contex #[tokio::test] async fn a_non_array_suppress_option_fails_the_invocation() { - // The strict parse replaced `option_or`, which turned any unreadable value - // into an empty list and handed over the very paths the option named. This - // drives the dispatcher itself, so a regression back to the lenient read is - // caught here rather than by nobody. + // A malformed `suppress` value fails the invocation at the dispatcher rather + // than degrading to "suppress nothing", which would hand over the very paths + // the option names. + // `Tool::option_or` degrades exactly that way, so reading this option through + // it satisfies every other test while breaking the guarantee; only driving + // `run` itself catches that. let tmp = tempdir().unwrap(); std::fs::write(tmp.path().join("a.txt"), "").unwrap(); From 98a5d3b6e8298e02d50b6950706b238059701f5b Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 27 Jul 2026 16:39:57 +0200 Subject: [PATCH 6/6] review feedback Signed-off-by: Jean Mertz --- .config/jp/tools/src/fs.rs | 2 +- .config/jp/tools/src/{fs => }/fs_tests.rs | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename .config/jp/tools/src/{fs => }/fs_tests.rs (100%) diff --git a/.config/jp/tools/src/fs.rs b/.config/jp/tools/src/fs.rs index d1d0d34e9..5106bb3c7 100644 --- a/.config/jp/tools/src/fs.rs +++ b/.config/jp/tools/src/fs.rs @@ -126,5 +126,5 @@ pub async fn run(ctx: Context, t: Tool) -> ToolResult { } #[cfg(test)] -#[path = "fs/fs_tests.rs"] +#[path = "fs_tests.rs"] mod tests; diff --git a/.config/jp/tools/src/fs/fs_tests.rs b/.config/jp/tools/src/fs_tests.rs similarity index 100% rename from .config/jp/tools/src/fs/fs_tests.rs rename to .config/jp/tools/src/fs_tests.rs