From e8a6b4913a5ccbb8bf4c2f16c8fa3d5ba8de7540 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Fri, 28 Aug 2026 01:52:53 +0530 Subject: [PATCH] fix(sources): resolve a relative folder path against the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FolderReader` was the only reader in this crate that ignored the workspace it is handed — `_workspace`, with `PathBuf::from(base_path)` used verbatim. A relative path therefore resolved against the host process's working directory, which is whatever directory the process happened to start 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 for a source that could work. Relative paths now anchor on the workspace, matching `ConversationReader`'s `workspace.join(...)` — the in-crate precedent. Absolute paths are taken verbatim, so every source configured today resolves exactly where it does now; `an_absolute_path_ignores_the_workspace` pins that against a workspace sharing no prefix with the source. Both halves move together. `read_item` had the same `_workspace` and built its path from the raw configured string, so fixing only `list_items` would have been worse than the bug: the reader would walk the workspace and then read back from the CWD, failing on every item it had just listed. `ensure_within_base` now receives the resolved base too. It was being handed the raw configured string while `file_path` was built from it separately; with a relative base those canonicalise against different roots, so the containment check was not comparing the file against the base it was actually joined onto. The error also says where the reader looked: folder does not exist: docs (resolved to /.../app/src-tauri/docs) Reporting only the configured string is what made this cost a source-read and an `lsof` of the running process to diagnose. The suffix is appended only when the resolved path differs, so an absolute source does not echo itself. Refs tinyhumansai/openhuman#5830 --- .../tinymemory-sources/src/readers/folder.rs | 66 +++++++-- .../src/readers/folder_tests.rs | 125 ++++++++++++++++++ 2 files changed, 183 insertions(+), 8 deletions(-) diff --git a/crates/tinymemory-sources/src/readers/folder.rs b/crates/tinymemory-sources/src/readers/folder.rs index a057f594..9c886cfe 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 b87583f4..f38e7860 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}" + ); +}