diff --git a/src/memory/sources/readers/folder.rs b/src/memory/sources/readers/folder.rs index 4dbf688..971801d 100644 --- a/src/memory/sources/readers/folder.rs +++ b/src/memory/sources/readers/folder.rs @@ -29,6 +29,47 @@ 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 memory 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 +/// [`MemoryConfig::workspace`] — the root this crate already treats as +/// authoritative — 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 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 (tinyhumansai/openhuman#5830). +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 echo 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 +82,7 @@ impl SourceReader for FolderReader { async fn list_items( &self, source: &MemorySourceEntry, - _config: &MemoryConfig, + config: &MemoryConfig, ) -> MemoryEngineResult> { let base_path = source .path @@ -49,11 +90,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, &config.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 +146,7 @@ impl SourceReader for FolderReader { &self, source: &MemorySourceEntry, item_id: &str, - _config: &MemoryConfig, + config: &MemoryConfig, ) -> MemoryEngineResult { let base_path = source .path @@ -123,7 +162,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, &config.workspace); + let file_path = base.join(item_id); if !file_path.exists() { return Err(MemoryError::NotFound(format!( "file not found: {}", @@ -133,7 +176,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/src/memory/sources/readers/folder_tests.rs b/src/memory/sources/readers/folder_tests.rs index 831de23..0e02b0f 100644 --- a/src/memory/sources/readers/folder_tests.rs +++ b/src/memory/sources/readers/folder_tests.rs @@ -149,3 +149,131 @@ async fn read_item_missing_file_errors() { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("not found")); } + +// ── openhuman#5830: relative paths resolve against the workspace ───────────── + +/// Build a workspace containing `relative_docs/note.md`, and a `MemoryConfig` +/// rooted at it. +/// +/// The subdirectory name is deliberately distinctive: a relative path resolves +/// 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, MemoryConfig) { + 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(); + let cfg = MemoryConfig::new(tmp.path()); + (tmp, cfg) +} + +/// A relative folder path must be anchored on the memory workspace, not on +/// whatever directory the host process happened to start 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, cfg) = workspace_with_relative_folder(); + let source = folder_source("relative_docs"); + let reader = FolderReader; + + let items = reader + .list_items(&source, &cfg) + .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, cfg) = workspace_with_relative_folder(); + let source = folder_source("relative_docs"); + let reader = FolderReader; + + let content = reader + .read_item(&source, "note.md", &cfg) + .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 cfg = MemoryConfig::new(tmp.path()); + let source = folder_source("relative_docs"); + let reader = FolderReader; + + let err = reader + .list_items(&source, &cfg) + .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 = MemoryConfig::new("/nonexistent/workspace/root"); + let items = reader + .list_items(&source, &bogus) + .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, &config()) + .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}" + ); +}