diff --git a/crates/tinymemory-sources/src/readers/folder.rs b/crates/tinymemory-sources/src/readers/folder.rs index a057f59..9c886cf 100644 --- a/crates/tinymemory-sources/src/readers/folder.rs +++ b/crates/tinymemory-sources/src/readers/folder.rs @@ -29,6 +29,49 @@ use super::SourceReader; /// Default glob applied when a folder source does not specify one. const DEFAULT_GLOB: &str = "**/*.md"; +/// Resolve a folder source's configured path against the workspace. +/// +/// An absolute path is taken verbatim, so every source configured today keeps +/// resolving exactly where it does now. A **relative** path is anchored on the +/// workspace, matching [`ConversationReader`]'s `workspace.join(…)` — the +/// in-crate precedent — instead of being resolved against the process working +/// directory. +/// +/// The CWD is not a defensible root for this. It is whatever directory the host +/// process happens to have been started in; for the OpenHuman desktop app that +/// is the Tauri build directory, so a source configured as `docs` looked in +/// `…/app/src-tauri/docs`, found nothing, and failed on every sync cycle +/// forever (tinyhumansai/openhuman#5830). The workspace is the root the rest of +/// this crate already treats as authoritative. +fn resolve_base(base_path: &str, workspace: &Path) -> PathBuf { + let configured = Path::new(base_path); + if configured.is_absolute() { + configured.to_path_buf() + } else { + workspace.join(configured) + } +} + +/// Build the "folder does not exist" error so it always says **where the reader +/// looked**, not merely what it was configured with. +/// +/// Reporting the configured string alone is what made openhuman#5830 cost a +/// source-read and an `lsof` of the running process to diagnose: the log said +/// `folder does not exist: docs` and nothing in it revealed the root that had +/// been joined on. The resolved path is appended only when it differs from the +/// configured one, so an absolute source does not get a redundant echo of +/// itself. +fn missing_folder_error(base_path: &str, resolved: &Path) -> MemoryError { + let resolved = resolved.display().to_string(); + if resolved == base_path { + MemoryError::NotFound(format!("folder does not exist: {base_path}")) + } else { + MemoryError::NotFound(format!( + "folder does not exist: {base_path} (resolved to {resolved})" + )) + } +} + /// A reader over a local folder of files. pub struct FolderReader; @@ -41,7 +84,7 @@ impl SourceReader for FolderReader { async fn list_items( &self, source: &MemorySourceEntry, - _workspace: &std::path::Path, + workspace: &std::path::Path, ) -> SourceResult> { let base_path = source .path @@ -49,11 +92,9 @@ impl SourceReader for FolderReader { .ok_or_else(|| MemoryError::Invalid("folder source requires a path".to_string()))?; let pattern = source.glob.as_deref().unwrap_or(DEFAULT_GLOB); - let base = PathBuf::from(base_path); + let base = resolve_base(base_path, workspace); if !base.exists() { - return Err(MemoryError::NotFound(format!( - "folder does not exist: {base_path}" - ))); + return Err(missing_folder_error(base_path, &base)); } let matcher = glob_to_regex(pattern)?; @@ -107,7 +148,7 @@ impl SourceReader for FolderReader { &self, source: &MemorySourceEntry, item_id: &str, - _workspace: &std::path::Path, + workspace: &std::path::Path, ) -> SourceResult { let base_path = source .path @@ -123,7 +164,11 @@ impl SourceReader for FolderReader { ))); } - let file_path = Path::new(base_path).join(item_id); + // Resolve through the same rule `list_items` used, so a relative source + // reads back the files it listed. Splitting these would be worse than + // the bug: list would walk the workspace while read looked in the CWD. + let base = resolve_base(base_path, workspace); + let file_path = base.join(item_id); if !file_path.exists() { return Err(MemoryError::NotFound(format!( "file not found: {}", @@ -133,7 +178,12 @@ impl SourceReader for FolderReader { // Canonicalize and verify the resolved file stays within the folder // root — defends against `..` traversal and symlink escapes. - let canonical_file = ensure_within_base(Path::new(base_path), &file_path)?; + // Containment is checked against the *resolved* base. Passing the raw + // configured string here would canonicalise a relative base against the + // CWD while `file_path` sits under the workspace, so the two roots + // would not correspond — the check has to see the same base the file + // was joined onto. + let canonical_file = ensure_within_base(&base, &file_path)?; // Apply the same size cap as list_items so a huge file can't blow up // the renderer or the chunker. diff --git a/crates/tinymemory-sources/src/readers/folder_tests.rs b/crates/tinymemory-sources/src/readers/folder_tests.rs index b87583f..f38e786 100644 --- a/crates/tinymemory-sources/src/readers/folder_tests.rs +++ b/crates/tinymemory-sources/src/readers/folder_tests.rs @@ -247,3 +247,128 @@ async fn symlinks_cannot_escape_the_configured_folder() { .unwrap_err(); assert!(matches!(error, MemoryError::PathEscape(_)), "got {error:?}"); } + +// ── openhuman#5830: relative paths resolve against the workspace ───────────── + +/// Build a workspace containing `relative_docs/note.md` and return both paths. +/// +/// The subdirectory name is deliberately distinctive: a relative path is +/// resolved against the process CWD before this fix, and the CWD under `cargo +/// test` is the crate directory — a common name like `docs` could accidentally +/// exist there and make the pre-fix run pass for the wrong reason. +fn workspace_with_relative_folder() -> TempDir { + let tmp = TempDir::new().unwrap(); + let nested = tmp.path().join("relative_docs"); + fs::create_dir_all(&nested).unwrap(); + fs::write(nested.join("note.md"), "# note").unwrap(); + tmp +} + +/// A relative folder path must be anchored on the workspace, not on whatever +/// directory the host process happens to have been started in. +/// +/// This is openhuman#5830: the desktop app's CWD is the Tauri build directory, +/// so a source configured as `docs` looked in `…/app/src-tauri/docs` and failed +/// on every sync cycle, permanently, for a source that could work. +#[tokio::test] +async fn list_items_resolves_a_relative_path_against_the_workspace() { + let tmp = workspace_with_relative_folder(); + let source = folder_source("relative_docs"); + let reader = FolderReader; + + let items = reader + .list_items(&source, tmp.path()) + .await + .expect("a relative folder path must resolve against the workspace, not the process CWD"); + + assert_eq!( + items.len(), + 1, + "the workspace-relative folder holds exactly one .md file" + ); + assert_eq!(items[0].id, "note.md"); +} + +/// `read_item` must resolve by the same rule as `list_items`. +/// +/// Fixing only the listing half would be worse than the original bug: the +/// reader would walk the workspace and then read back from the CWD, so every +/// item it just listed would fail to load. +#[tokio::test] +async fn read_item_resolves_a_relative_path_against_the_workspace() { + let tmp = workspace_with_relative_folder(); + let source = folder_source("relative_docs"); + let reader = FolderReader; + + let content = reader + .read_item(&source, "note.md", tmp.path()) + .await + .expect("read_item must resolve a relative path against the workspace, like list_items"); + + assert_eq!(content.body, "# note"); +} + +/// The error has to say **where the reader looked**, not only what it was +/// configured with. `folder does not exist: docs` is what cost a source-read +/// and an `lsof` of the running process to diagnose. +#[tokio::test] +async fn a_missing_relative_folder_error_names_the_resolved_path() { + let tmp = TempDir::new().unwrap(); + let source = folder_source("relative_docs"); + let reader = FolderReader; + + let err = reader + .list_items(&source, tmp.path()) + .await + .expect_err("a missing folder is still an error") + .to_string(); + + assert!( + err.contains("resolved to"), + "the error must name the resolved path, not only the configured one: {err}" + ); + assert!( + err.contains(&tmp.path().join("relative_docs").display().to_string()), + "the resolved path must be the workspace-anchored one: {err}" + ); +} + +/// An absolute path keeps working exactly as before, and the workspace must not +/// influence it — otherwise this fix would break every source already +/// configured with an absolute path. +#[tokio::test] +async fn an_absolute_path_ignores_the_workspace() { + let tmp = TempDir::new().unwrap(); + fs::write(tmp.path().join("note.md"), "# note").unwrap(); + let source = folder_source(&tmp.path().to_string_lossy()); + let reader = FolderReader; + + // A workspace that does not exist and shares no prefix with the source: if + // the absolute path were being joined onto it, nothing would resolve. + let bogus_workspace = std::path::Path::new("/nonexistent/workspace/root"); + let items = reader + .list_items(&source, bogus_workspace) + .await + .expect("an absolute folder path must resolve without consulting the workspace"); + + assert_eq!(items.len(), 1, "the absolute folder still lists its file"); +} + +/// For an absolute path the configured string *is* the resolved path, so the +/// error must not echo it twice. +#[tokio::test] +async fn an_absolute_missing_folder_error_does_not_echo_itself() { + let source = folder_source("/nonexistent/path/xyz"); + let reader = FolderReader; + + let err = reader + .list_items(&source, std::path::Path::new("/some/workspace")) + .await + .expect_err("a missing folder is still an error") + .to_string(); + + assert!( + !err.contains("resolved to"), + "an absolute path is already resolved; the error must not repeat it: {err}" + ); +}