feat(cli): add csp clear orphans to remove cached indexes whose source is gone - #102
Conversation
…rce is gone (#87) Port of upstream semble#243 (`_clear_orphans`). `clear_orphan_indexes` walks `<home>/index/*`, reads each manifest, and removes entries whose `sourceId` is a local path that no longer exists. An entry is trusted only when `resolve_cache_dir(sourceId, content)` reproduces its directory name, so git-URL leaves (URL + ref keyed, built from a temp clone), malformed manifests, and key mismatches are left untouched. The root guard is shared with `clear_index_cache` via `guarded_index_root`. `orphans` is its own choice — `clear all` does not include it, matching upstream. Closes #87
…source The key-reproduction guard re-derived the cache key from the manifest and required it to match the entry directory name. It never matched for the documented invocation (`csp search "q" ."`): `load_or_build_index` keys on the raw CLI argument while `from_path` records `std::path::absolute` of it, so the sweep was a no-op for local sources. Upstream can afford that guard because its `cache_key` resolves the path; csp normalizes only lexically. Replace it with the checks that actually hold: - the leaf directory name has the cache-key shape (32 lowercase hex, mirroring upstream `_SHA_256_REGEX`), so stray directories are never swept; - `is_git_url(sourceId)` excludes remote indexes, which `from_git` re-roots to the URL; - the `sourceId` is absolute, since a relative one would be judged against the caller's cwd; - the source is a genuine `NotFound`, not merely unreachable — `Path::exists` collapses the two and would delete live caches for an unmounted volume or an unreadable parent. Read only `sourceId` out of the manifest instead of a full `read_manifest` parse, which also deserializes the per-file entry map; `read_manifest` goes back to private. Refs #87
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 36 |
| Duplication | 4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the clear orphans command, which identifies and removes cached indexes whose local source paths no longer exist. The changes span the CLI interface, cache management logic, documentation, and unit tests. The feedback suggests a performance optimization in read_manifest_source_id to deserialize only the sourceId field using a dedicated lightweight struct, avoiding the overhead of parsing the entire manifest into a generic JSON value.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds the
Confidence Score: 4/5The PR is not safe to merge until orphan cleanup stops deleting caches for sources that are temporarily absent because their filesystem is unmounted. The sweep treats every metadata Files Needing Attention: crates/csp/src/indexing/cache.rs, README.md, README.ko.md, .please/docs/references/semble.md
|
| Filename | Overview |
|---|---|
| crates/csp/src/indexing/cache.rs | Adds guarded orphan discovery and deletion, but conflates unmounted subpaths with deleted sources and suppresses per-entry traversal errors. |
| crates/csp/src/bin/csp/main.rs | Adds the orphans clear choice, output, exit handling, and an end-to-end-style command test. |
| README.md | Documents the new command but promises retention of unmounted sources that the implementation cannot guarantee. |
| README.ko.md | Adds synchronized Korean documentation, including the same unsupported unmounted-source guarantee. |
| .please/docs/references/semble.md | Records upstream parity and key-recomputation drift, but also overstates the protection provided by checking only NotFound. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[csp clear orphans] --> B[Resolve and guard index root]
B --> C[Walk cache-key-shaped leaves]
C --> D[Read manifest sourceId]
D --> E{Absolute local path?}
E -->|No| F[Keep cache entry]
E -->|Yes| G[Call filesystem metadata]
G -->|Exists or non-NotFound error| F
G -->|NotFound| H[Remove cache entry]
I[Unmounted source below mount point] -->|May also return NotFound| H
Prompt To Fix All With AI
### Issue 1
crates/csp/src/indexing/cache.rs:358
**Unmounted Sources Lose Caches**
If a repository is below a mount point and that volume or network share is unmounted, metadata on the recorded subpath can return `NotFound`. This code treats that result as proof that the source was deleted and removes its cache, despite the documented promise that unmounted sources are retained. The existing test only produces `PermissionDenied` by changing Unix permissions, so it does not cover this case.
### Issue 2
crates/csp/src/indexing/cache.rs:377-379
**Traversal Errors Are Hidden**
`flatten()` silently discards directory-entry errors, while `is_ok_and` silently drops entries whose file type cannot be read. A transient filesystem error can therefore leave an orphan untouched while the command still reports success and may print `No orphaned indexes found`, making an incomplete sweep indistinguishable from a complete one.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(cli): make `csp clear orphans` sweep..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Deserialize only `sourceId` from the manifest via a typed struct instead of building a full `serde_json::Value` tree (gemini-code-assist). - Propagate directory-traversal and file-type errors from the index root instead of silently skipping entries, so an incomplete sweep cannot report "No orphaned indexes found" (greptile P2). - Stop claiming that sources on an unmounted volume are retained: their path is simply absent and is swept like upstream. Docs, READMEs, and the drift reference now describe the actual guard, which is `PermissionDenied`/other I/O errors, not unmounts (greptile P1).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new clear orphans command to remove cached indexes whose local source paths no longer exist. It implements the clear_orphan_indexes function in cache.rs, integrates it into the CLI command dispatcher, updates the documentation, and adds comprehensive unit tests. The feedback suggests improving the error context when traversing the index root fails by including the path of the index root in the error message.
Wrap the `read_dir` failure on the index root with its path, matching the per-entry errors introduced in the previous commit (gemini-code-assist).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the csp clear orphans command, which removes cached indexes whose recorded local source paths no longer exist. It includes comprehensive unit tests and documentation updates. The review feedback suggests optimizing the manifest parsing in read_manifest_source_id by streaming the JSON directly from a file reader instead of reading the entire file into memory, which prevents potential memory spikes on large manifest files.
|
1 participant
Summary
Port of upstream semble#243
_clear_orphans: a newcsp clear orphanssubcommand that removes cached indexes whose local source directory no longer exists.clear_orphan_indexesincrates/csp/src/indexing/cache.rswalks~/.csp/index/*and removes an entry only when all of the following hold:sourceIdis an absolute, non-git local path,std::fs::metadataon that path returnsNotFound.The sweep uses
std::fs::metadatarather thanPath::existson purpose:Path::existscollapses every error intofalse, so an unreachable volume or a permission error would look like a deleted source and get swept. Matching onNotFoundkeeps those entries.The index-root guard is shared with
clear indexviaguarded_index_root.orphansis its own choice and is deliberately not part ofclear all, matching upstream.Design note: no cache-key recomputation
The issue's task list asked to recompute the cache key from the manifest and compare it against the directory name, as upstream does. That was implemented first and then dropped in the second commit: csp keys the cache on the raw CLI source string (
.), while the manifest records the absolute path, so the recomputed key never matched a real entry and the sweep skipped everything. csp instead relies onis_git_urlto exclude git entries, which is sound becausefrom_gitre-roots the manifestsourceIdto the URL. Follow-up #100 tracks canonicalizing the source before keying, which would let the stricter upstream check come back.Changes
crates/csp/src/indexing/cache.rs: addclear_orphan_indexes, reuseguarded_index_rootwithclear index.orphansas aclearchoice (not included inclear all).README.md/README.ko.md: document the new subcommand (bilingual, kept in sync)..please/docs/references/semble.md: record the upstream parity mapping and the deviation above.Test plan
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo test --workspace— 343 pass., delete the repo directory, runcsp clear orphans— it reports the entry cleared and leaves a live source untouchedRelated issue
Closes #87
Related follow-up: #100 (canonicalize the source before keying the cache)
Checklist
Summary by cubic
Adds
csp clear orphansto remove cached indexes whose recorded local source directory no longer exists. Live sources, git-URL entries, and sources that merely fail to stat (e.g., permission denied) are left untouched; sources on an unmounted volume are swept because their path is simply absent.New Features
~/.csp/index/and removes only entries with a cache-key-shaped directory name, an absolute localsourceIdthat is a genuineNotFound, and no git URL; traversal errors fail the sweep instead of reporting "no orphans found".clear index;orphansis a separate choice and is not part ofclear all.Written for commit aeb128c. Summary will update on new commits.