From bf2d52a9b86a0a30b6f12f33775aa9d725d9852e Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Sat, 5 Sep 2026 00:57:59 +0900 Subject: [PATCH 1/3] fix(cache): absolutize local sources before computing the index cache key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolve_cache_dir` hashed the raw source string the CLI/MCP received after a purely lexical normalize, while `from_path` recorded `std::path::absolute` in the manifest. Every `csp search` run with the default `.` therefore shared one cache key across all repos, forcing a full rebuild on each switch, and the same repo reached via different spellings got separate entries. `normalize_source` now makes local paths absolute (`std::path::absolute`, no filesystem access — the same call the MCP in-memory key and the manifest use) before path-normalizing, so `.`, `./r/../r`, and `/abs/r` share one leaf and the keyed form equals the manifest `sourceId`. Git URLs stay verbatim. Mirrors upstream `cache_key`'s `Path.resolve()`; ADR-0002 already specified this. Entries keyed from the old relative form become unreachable and are cleaned by `clear index` / `clear orphans`. Closes #100 --- .please/docs/references/semble.md | 5 +++ crates/csp/src/indexing/cache.rs | 66 ++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/.please/docs/references/semble.md b/.please/docs/references/semble.md index 0b11a75..f57e3fd 100644 --- a/.please/docs/references/semble.md +++ b/.please/docs/references/semble.md @@ -294,6 +294,11 @@ Ported faithfully (`LazyLock` for the static patterns, `RefCell` - Cache home `$HOME/.csp` (override via `CacheLocation`), index root `/index`, per-source leaf `/index/`. `ensure_cache_dir` creates the chain with **0700** perms (NFR-003), tightening pre-existing dirs on Unix. +- **Cache key source identity** (`normalize_source`): local paths are made absolute with + `std::path::absolute` and then path-normalized before hashing, so `.`, `./r/../r`, and + `/abs/r` share one leaf and two repos searched with the default `.` no longer collide + (#100; upstream `cache_key` uses `Path.resolve()`). The keyed form equals the manifest + `sourceId` that `from_path` records. Git URLs stay verbatim. - `clear_index_cache` removes only the index dir — never the `~/.csp` home (which also holds `savings.jsonl`). - `clear_orphan_indexes` (← upstream `cli.py::_clear_orphans`, #243) removes per-source leaves diff --git a/crates/csp/src/indexing/cache.rs b/crates/csp/src/indexing/cache.rs index ef5e6e1..4f28f92 100644 --- a/crates/csp/src/indexing/cache.rs +++ b/crates/csp/src/indexing/cache.rs @@ -136,14 +136,24 @@ fn normalize_posix(path: &str) -> String { joined } -/// Normalize a source identity: local paths are path-normalized, URLs (scheme:// -/// or scp-style `git@`) kept verbatim. +/// Normalize a source identity: URLs (scheme:// or scp-style `git@`) are kept +/// verbatim; local paths are made absolute against the current directory and +/// then path-normalized, so `.`, `./repo/../repo`, and `/abs/repo` all key the +/// same entry (mirrors upstream `cache_key`'s `Path.resolve()`). Absolutizing +/// here — rather than only at the CLI edge — is what keeps every caller of +/// `resolve_cache_dir` (CLI, MCP, SDK) on one key, and keeps the keyed form +/// equal to the `sourceId` that `from_path` records in the manifest. +/// `std::path::absolute` does not touch the filesystem, matching the in-memory +/// MCP cache key and the manifest, so a path that does not exist yet still +/// keys deterministically. fn normalize_source(source: &str) -> String { if is_url_scheme(source) || source.starts_with("git@") { - source.to_string() - } else { - normalize_posix(source) + return source.to_string(); } + let absolute = std::path::absolute(source) + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_else(|_| source.to_string()); + normalize_posix(&absolute.replace('\\', "/")) } #[derive(Serialize)] @@ -485,6 +495,52 @@ mod tests { assert_ne!(a, b); } + #[test] + fn cache_dir_relative_and_absolute_source_share_a_key() { + let base = Path::new("/h/.csp"); + let cwd = std::env::current_dir().unwrap(); + let dot = resolve_cache_dir(".", &[ContentType::Code], &loc(base)); + let abs = resolve_cache_dir(&cwd.to_string_lossy(), &[ContentType::Code], &loc(base)); + assert_eq!(dot, abs); + + // Lexical detours resolve to the same absolute form. + let sub = cwd.join("sub"); + let detour = resolve_cache_dir("./sub/../sub", &[ContentType::Code], &loc(base)); + let direct = resolve_cache_dir(&sub.to_string_lossy(), &[ContentType::Code], &loc(base)); + assert_eq!(detour, direct); + assert_ne!(dot, detour); + } + + #[test] + fn cache_dir_relative_sources_key_by_their_absolute_form() { + // Two different relative names must not collide, and each must equal + // the key its absolute form produces — this is what makes `csp search` + // from two repos with the default `.` land in two cache entries. + let base = Path::new("/h/.csp"); + let cwd = std::env::current_dir().unwrap(); + let a = resolve_cache_dir("repo-a", &[ContentType::Code], &loc(base)); + let b = resolve_cache_dir("repo-b", &[ContentType::Code], &loc(base)); + assert_ne!(a, b); + let a_abs = cwd.join("repo-a"); + assert_eq!( + a, + resolve_cache_dir(&a_abs.to_string_lossy(), &[ContentType::Code], &loc(base)) + ); + } + + #[test] + fn cache_dir_keeps_git_urls_verbatim() { + let base = Path::new("/h/.csp"); + let a = resolve_cache_dir("https://x/r.git", &[ContentType::Code], &loc(base)); + let b = resolve_cache_dir("git@github.com:x/r.git", &[ContentType::Code], &loc(base)); + assert_ne!(a, b); + // Re-resolving must not absolutize a URL against the cwd. + assert_eq!( + a, + resolve_cache_dir("https://x/r.git", &[ContentType::Code], &loc(base)) + ); + } + #[test] fn cache_dir_differs_by_source() { let base = Path::new("/h/.csp"); From 770ee0b37bf7a261f9996ccc13bce03ba3d2626b Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Sat, 5 Sep 2026 03:25:22 +0900 Subject: [PATCH 2/3] fix(cache): share the source normalizer with MCP and keep `..` sources sweepable - Route `IndexCache::compute_key` through `cache::normalize_source` so `.`, `./r/../r`, and `/abs/r` name one in-memory session entry as well as one on-disk cache key (no duplicate `Arc` per spelling, `evict` can't miss). - Fold `\` into `/` only on Windows: on Unix a backslash is an ordinary filename byte, so `/repos/a\b` and `/repos/a/b` must keep distinct keys and `..\y` must not synthesize a `..` segment. - `source_is_gone` now requires both the recorded `sourceId` and its lexically normalized form to be `NotFound`: `from_path` records `std::path::absolute`, which keeps `..`, so deleting only an intermediate directory must not make a still-present source look orphaned. --- crates/csp/src/indexing/cache.rs | 96 ++++++++++++++++++++++++++------ crates/csp/src/mcp.rs | 9 +-- 2 files changed, 83 insertions(+), 22 deletions(-) diff --git a/crates/csp/src/indexing/cache.rs b/crates/csp/src/indexing/cache.rs index 4f28f92..fbaf844 100644 --- a/crates/csp/src/indexing/cache.rs +++ b/crates/csp/src/indexing/cache.rs @@ -141,19 +141,29 @@ fn normalize_posix(path: &str) -> String { /// then path-normalized, so `.`, `./repo/../repo`, and `/abs/repo` all key the /// same entry (mirrors upstream `cache_key`'s `Path.resolve()`). Absolutizing /// here — rather than only at the CLI edge — is what keeps every caller of -/// `resolve_cache_dir` (CLI, MCP, SDK) on one key, and keeps the keyed form -/// equal to the `sourceId` that `from_path` records in the manifest. -/// `std::path::absolute` does not touch the filesystem, matching the in-memory -/// MCP cache key and the manifest, so a path that does not exist yet still -/// keys deterministically. -fn normalize_source(source: &str) -> String { +/// `resolve_cache_dir` (CLI, MCP, SDK) on one key. +/// `std::path::absolute` does not touch the filesystem, so a path that does not +/// exist yet still keys deterministically. +/// +/// This is *not* byte-identical to the manifest `sourceId`: `from_path` records +/// bare `std::path::absolute(root)`, which keeps `..` segments this function +/// collapses. Anything comparing the two (notably [`source_is_gone`]) must +/// normalize both sides rather than assume they already agree. +pub(crate) fn normalize_source(source: &str) -> String { if is_url_scheme(source) || source.starts_with("git@") { return source.to_string(); } let absolute = std::path::absolute(source) .map(|p| p.to_string_lossy().into_owned()) .unwrap_or_else(|_| source.to_string()); - normalize_posix(&absolute.replace('\\', "/")) + // Only Windows spells its separator `\`. On Unix a backslash is an ordinary + // filename byte, so rewriting it would fold `/home/u/a\b` onto the unrelated + // `/home/u/a/b` (and `..\x` into a `..` segment `normalize_posix` then + // collapses) — two distinct repos sharing one cache leaf, thrashing each + // other's index on every search. + #[cfg(windows)] + let absolute = absolute.replace('\\', "/"); + normalize_posix(&absolute) } #[derive(Serialize)] @@ -372,11 +382,26 @@ fn read_manifest_source_id(dir: &Path) -> Option { /// under a directory whose permissions temporarily deny traversal. A source /// below an *unmounted* volume is not distinguishable from a deleted one at /// this level — its path is simply absent — and is swept, as upstream does. +/// +/// Both the recorded spelling and its lexically normalized form must be +/// `NotFound`: `from_path` records `std::path::absolute`, which does **not** +/// collapse `..`, and the kernel resolves `..` component-by-component. So +/// `csp search "q" ../b` records `/cwd/../b`, and deleting `/cwd` alone makes +/// that spelling `NotFound` while `/b` — the actual source — is still there. +/// Requiring both to be absent keeps the sweep conservative: a lexical +/// collapse that crosses a symlink can only ever *keep* a cache entry. fn source_is_gone(source_id: &str) -> bool { - matches!( - std::fs::metadata(source_id), - Err(e) if e.kind() == std::io::ErrorKind::NotFound - ) + fn is_not_found(path: &str) -> bool { + matches!( + std::fs::metadata(path), + Err(e) if e.kind() == std::io::ErrorKind::NotFound + ) + } + if !is_not_found(source_id) { + return false; + } + let normalized = normalize_source(source_id); + normalized == source_id || is_not_found(&normalized) } /// Remove cached indexes whose local source directory no longer exists (port of @@ -528,6 +553,21 @@ mod tests { ); } + /// On Unix `\\` is an ordinary filename byte, so two distinct repos must not + /// collapse onto one cache leaf. + #[cfg(unix)] + #[test] + fn cache_dir_keeps_unix_backslash_paths_distinct() { + let base = Path::new("/h/.csp"); + let escaped = resolve_cache_dir("/repos/a\\b", &[ContentType::Code], &loc(base)); + let nested = resolve_cache_dir("/repos/a/b", &[ContentType::Code], &loc(base)); + assert_ne!(escaped, nested); + // A `..` must not be synthesizable out of a backslash either. + let literal = resolve_cache_dir("/repos/x/..\\y", &[ContentType::Code], &loc(base)); + let traversed = resolve_cache_dir("/repos/y", &[ContentType::Code], &loc(base)); + assert_ne!(literal, traversed); + } + #[test] fn cache_dir_keeps_git_urls_verbatim() { let base = Path::new("/h/.csp"); @@ -760,9 +800,9 @@ mod tests { } /// Write a cache entry whose directory is keyed on `key_source` but whose - /// manifest records `manifest_source`. Production splits the two: - /// `load_or_build_index` keys on the raw CLI argument while `from_path` - /// records `std::path::absolute` of it. + /// manifest records `manifest_source`. Production can split the two: the key + /// also folds in `CacheLocation::git_ref`, which the manifest never records, + /// and `from_path` keeps the `..` segments `normalize_source` collapses. fn write_entry_keyed( key_source: &str, manifest_source: &str, @@ -800,10 +840,9 @@ mod tests { assert!(resolve_index_root(&loc(&base)).exists()); } - /// The production shape: `csp search "q" .` keys the entry on the raw `"."` - /// while the manifest records the absolutized source. Regression test — a - /// guard that re-derived the key from the manifest never fired here, which - /// made `clear orphans` a no-op for the documented invocation. + /// Regression test for a sweep that filters on the entry directory's shape + /// rather than on a re-derived key: the manifest's `sourceId` is the only + /// thing consulted, so an entry stays sweepable however its key was spelled. #[test] fn orphans_removes_entry_keyed_by_a_relative_source() { let tmp = tempdir().unwrap(); @@ -943,6 +982,27 @@ mod tests { } } + /// `from_path` records `std::path::absolute`, which keeps `..` segments, and + /// the kernel resolves `..` component-by-component. Deleting only the + /// intermediate directory must not make the still-present source look gone. + #[test] + fn orphans_keeps_entry_whose_source_id_traverses_a_deleted_parent() { + let tmp = tempdir().unwrap(); + let base = tmp.path().join(".csp"); + let via = tmp.path().join("via"); + let live = tmp.path().join("live-repo"); + std::fs::create_dir_all(&via).unwrap(); + std::fs::create_dir_all(&live).unwrap(); + // The shape `csp search "q" ../live-repo` records from inside `via`. + let recorded = via.join("..").join("live-repo"); + let dir = write_entry(&recorded.to_string_lossy(), &loc(&base)); + std::fs::remove_dir_all(&via).unwrap(); + + assert!(std::fs::metadata(&recorded).is_err(), "precondition"); + assert!(clear_orphan_indexes(&loc(&base)).unwrap().is_empty()); + assert!(dir.join("manifest.json").exists()); + } + #[test] fn orphans_reports_nothing_without_index_root() { let tmp = tempdir().unwrap(); diff --git a/crates/csp/src/mcp.rs b/crates/csp/src/mcp.rs index 40865cb..68da638 100644 --- a/crates/csp/src/mcp.rs +++ b/crates/csp/src/mcp.rs @@ -196,10 +196,11 @@ impl IndexCache { _ => source.to_string(), } } else { - // Absolutize without requiring existence (matches `path.resolve`). - std::path::absolute(source) - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_else(|_| source.to_string()) + // Local paths go through the same normalizer as the on-disk cache + // key, so `.`, `./r/../r`, and `/abs/r` name one session entry too — + // a second spelling of the same repo must not cost a duplicate + // `Arc` and an LRU slot, or make `evict` silently miss. + crate::indexing::cache::normalize_source(source) }; CacheKey { source, From 221a6dbd25deef28580027d0565bb8ed549e011b Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Sat, 5 Sep 2026 03:30:37 +0900 Subject: [PATCH 3/3] fix(cache): keep every `is_git_url` remote spelling verbatim in the cache key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `normalize_source` exempted only `git@…` prefixes, so a non-`git` scp remote such as `deploy@host:org/repo.git` — which `is_git_url` accepts and `load_or_build_index` clones — was absolutized against the caller's cwd and keyed differently from every directory. Route the check through the shared classifier, and correct the semble drift note that claimed the keyed form equals the manifest `sourceId`. --- .please/docs/references/semble.md | 6 ++++-- crates/csp/src/indexing/cache.rs | 26 ++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/.please/docs/references/semble.md b/.please/docs/references/semble.md index f57e3fd..f98b7ab 100644 --- a/.please/docs/references/semble.md +++ b/.please/docs/references/semble.md @@ -297,8 +297,10 @@ Ported faithfully (`LazyLock` for the static patterns, `RefCell` - **Cache key source identity** (`normalize_source`): local paths are made absolute with `std::path::absolute` and then path-normalized before hashing, so `.`, `./r/../r`, and `/abs/r` share one leaf and two repos searched with the default `.` no longer collide - (#100; upstream `cache_key` uses `Path.resolve()`). The keyed form equals the manifest - `sourceId` that `from_path` records. Git URLs stay verbatim. + (#100; upstream `cache_key` uses `Path.resolve()`). Anything `is_git_url` accepts (scheme + URLs and scp-style remotes) stays verbatim. The keyed form is **not** byte-identical to the + manifest `sourceId`: `from_path` records bare `std::path::absolute`, which keeps `..` + segments the key collapses, so `source_is_gone` normalizes both sides before comparing. - `clear_index_cache` removes only the index dir — never the `~/.csp` home (which also holds `savings.jsonl`). - `clear_orphan_indexes` (← upstream `cli.py::_clear_orphans`, #243) removes per-source leaves diff --git a/crates/csp/src/indexing/cache.rs b/crates/csp/src/indexing/cache.rs index fbaf844..d5f0bee 100644 --- a/crates/csp/src/indexing/cache.rs +++ b/crates/csp/src/indexing/cache.rs @@ -136,7 +136,8 @@ fn normalize_posix(path: &str) -> String { joined } -/// Normalize a source identity: URLs (scheme:// or scp-style `git@`) are kept +/// Normalize a source identity: URLs (any `scheme://`, or anything `is_git_url` +/// accepts, including scp-style `user@host:path`) are kept /// verbatim; local paths are made absolute against the current directory and /// then path-normalized, so `.`, `./repo/../repo`, and `/abs/repo` all key the /// same entry (mirrors upstream `cache_key`'s `Path.resolve()`). Absolutizing @@ -150,7 +151,11 @@ fn normalize_posix(path: &str) -> String { /// collapses. Anything comparing the two (notably [`source_is_gone`]) must /// normalize both sides rather than assume they already agree. pub(crate) fn normalize_source(source: &str) -> String { - if is_url_scheme(source) || source.starts_with("git@") { + // `is_git_url` is the same classifier `load_or_build_index` uses to pick + // `from_git`, so every remote spelling it accepts — including non-`git` + // scp-style ones like `deploy@host:org/repo.git` — stays verbatim instead + // of being absolutized against the caller's cwd. + if is_url_scheme(source) || is_git_url(source) { return source.to_string(); } let absolute = std::path::absolute(source) @@ -568,6 +573,23 @@ mod tests { assert_ne!(literal, traversed); } + /// Every remote spelling `is_git_url` accepts must stay verbatim — not only + /// `git@…` — or the same remote keys differently from each cwd. + #[test] + fn normalize_source_keeps_scp_remotes_verbatim() { + for remote in [ + "deploy@host:org/repo.git", + "user@host:repo", + "git@github.com:x/r.git", + ] { + assert_eq!(normalize_source(remote), remote); + } + // `user@host:/abs` is not scp syntax; it is a local path and gets keyed as one. + let local = normalize_source("user@host:/abs"); + assert_ne!(local, "user@host:/abs"); + assert!(Path::new(&local).is_absolute()); + } + #[test] fn cache_dir_keeps_git_urls_verbatim() { let base = Path::new("/h/.csp");