lane: require verified managed-worktree identity before TTL cleanup (#5824) - #5854
Conversation
…5824) TTL cleanup passed the persisted worktree_path straight to recursive deletion: worktree_details() was consulted for git cleanup, but when it could not identify the path, cleanup still fell through to fs::remove_dir_all. A stale or malformed LaneRecord pointing at an existing unrelated directory could therefore be recursively deleted the moment its TTL hit zero. Cleanup now requires successful managed-worktree identity before any removal (#5824): - worktree_details() also records every worktree the owning repository lists, and is_managed_worktree() verifies that the candidate path canonicalizes to one of those entries (still resolves to the identified worktree) and that its .git file names registration data beneath the owning repository's .git/worktrees/ (a linked, managed worktree of that repo — not a repo root or an unrelated subdirectory). - An unverified path short-circuits: no git worktree remove, no prune, no branch deletion, no recursive delete. - The fs::remove_dir_all fallback re-verifies identity immediately before running, closing the window where the directory was swapped between identification and removal. Missing paths, TTL evaluation, git worktree removal, pruning, and the merged-branch deletion policy are unchanged; nothing was deleted by the old code path that git verified, so no existing test changed. Fixes #5824 Gates (all pass): cargo fmt --all -- --check; clippy -p codewhale-lane --all-targets --all-features --locked -D warnings; nextest -p codewhale-lane --lib --profile ci: 67 tests run: 67 passed, 0 failed.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0b59c68b-cf45-4482-82db-5bffc7194f49) |
|
Claude finished @Hmbown's task in 2m 59s —— View job Review: lane: require verified managed-worktree identity before TTL cleanup (#5824)
SummaryThis closes a real bug in
Test coverage is solid: unrelated plain directory, unrelated repo subdirectory, repo root, path swapped after provisioning, and a direct unit test of FindingsOne non-blocking observation left as an inline comment on No correctness bugs, provider/model-routing concerns (not applicable to this file), or security issues found beyond the note above. No dead code or needless duplication — the double Notes on unverified gatesI could not execute |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if !is_managed_worktree(worktree_path, &details) { | ||
| tracing::debug!( | ||
| "skipped TTL cleanup of {}: git does not identify it as a managed worktree", | ||
| worktree_path.display() | ||
| ); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Best-effort: git worktree remove --force, then rm -rf. | ||
| let removed = details.as_ref().is_some_and(|details| { | ||
| Command::new("git") | ||
| .current_dir(&details.repo_root) | ||
| .args([ | ||
| "worktree", | ||
| "remove", | ||
| "--force", | ||
| &worktree_path.to_string_lossy(), | ||
| ]) | ||
| .status() | ||
| .is_ok_and(|status| status.success()) | ||
| }); | ||
| if worktree_path.exists() { | ||
| let removed = Command::new("git") | ||
| .current_dir(&details.repo_root) | ||
| .args([ | ||
| "worktree", | ||
| "remove", | ||
| "--force", | ||
| &worktree_path.to_string_lossy(), | ||
| ]) | ||
| .status() | ||
| .is_ok_and(|status| status.success()); |
There was a problem hiding this comment.
Minor/non-blocking: is_managed_worktree is (correctly) re-checked right before the fs::remove_dir_all fallback (line 135), but not right before the primary git worktree remove --force call (line 125-134). There's still a small TOCTOU window between the check at line 116 and that Command::new("git") invocation where the path could theoretically be swapped. In practice this relies on git's own worktree-remove path validation as a second layer, and the realistic threat here is a stale/malformed record (which this PR fixes), not an active local attacker racing the filesystem — so I wouldn't block on it, just flagging for awareness given the "verified identity" framing in the PR title.
There was a problem hiding this comment.
🟡 Changes recommended
New logging/contexts expose raw paths and the .git parsing reads unbounded data from a potentially malformed path, both of which should be tightened before landing a safety-critical cleanup change.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens lane TTL cleanup to prevent destructive recursive deletion of arbitrary directories when worktree_path is stale, corrupted, or externally modified, by requiring Git-verified “managed linked worktree” identity before any remove_dir_all fallback.
Changes:
- Gate TTL cleanup on a new
is_managed_worktreecheck (worktree must be listed bygit worktree listand have a linked-worktree.gitregistration under<repo>/.git/worktrees/). - Extend
worktree_detailsto capture the full worktree list to support identity verification. - Add focused tests covering plain directories, repo roots/subdirs, and swapped paths after provisioning.
File summaries
| File | Description |
|---|---|
| crates/lane/src/worktree.rs | Adds Git-verified managed-worktree identity checks before TTL cleanup deletion and introduces new regression tests for #5824 safety cases. |
Review details
Suppressed comments (1)
crates/lane/src/worktree.rs:139
- This error context string also formats the raw
worktree_pathwithdisplay(). Using the lane redaction helper keeps absolute$HOMEpaths out of errors that may be surfaced to operators/logs.
fs::remove_dir_all(worktree_path)
.with_context(|| format!("remove worktree {}", worktree_path.display()))?;
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| tracing::debug!( | ||
| "skipped TTL cleanup of {}: git does not identify it as a managed worktree", | ||
| worktree_path.display() | ||
| ); |
| let registrations = repo_root.join(".git").join("worktrees"); | ||
| let registration = fs::read_to_string(worktree_path.join(".git")) | ||
| .ok() | ||
| .and_then(|dot_git| { | ||
| dot_git | ||
| .lines() | ||
| .find_map(|line| line.strip_prefix("gitdir: ")) | ||
| .map(str::trim) | ||
| .filter(|gitdir| !gitdir.is_empty()) | ||
| .map(PathBuf::from) | ||
| }); |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
The PR hardens lane TTL cleanup by requiring the path to be a git-managed linked worktree before git worktree remove or remove_dir_all, and adds tests for plain directories, repo roots/subdirs, and a swapped plain directory. The direction is correct, but identity is still derived only from the path's current repository, so a stale lane path swapped with a different repository's valid worktree would still be deleted.
Findings
- [ERROR] Identity gate is not bound to the lane record's repository (
crates/lane/src/worktree.rs)
remove_worktree_if_expired derives WorktreeDetails by running git worktree list from worktree_path. If a stale lane path has been replaced with a valid linked worktree of a different repository, worktree_details returns that other repository, is_managed_worktree returns true for it, and cleanup deletes what is actually another repository's worktree. The new swapped-path test only replaces the path with a plain directory, so this case is not covered. The expected repository/worktree identity should come from the lane record or caller. - [WARNING] No positive test for the remove_dir_all fallback (
crates/lane/src/worktree.rs)
The new fallback logic deletes the path via fs::remove_dir_all only when git worktree remove fails and is_managed_worktree still passes. The added tests cover refusal cases, but no test shows a valid managed worktree is actually removed through this fallback. A too-strict gate could silently break TTL cleanup for dirty or locked worktrees. - [INFO] Registrations directory should be canonicalized before prefix comparison (
crates/lane/src/worktree.rs:224)
registrations is built as repo_root.join(".git").join("worktrees"), where only repo_root is canonicalized. If .git is a symlink or a separate gitdir, canonicalized registration paths may not start with the non-canonical registrations prefix, causing valid linked worktrees to be rejected. This is a false negative, not data loss.
Suggestions
-
crates/lane/src/worktree.rs:224— Canonicalize the registrations directory before the starts_with check so symlinked .git directories or separate gitdirs do not cause false negatives.let Ok(registrations) = fs::canonicalize(repo_root.join(".git").join("worktrees")) else { return false; };
Assessment
The change is a meaningful safety improvement and the new negative tests are valuable. It should not be merged as-is without addressing the self-referential identity gap: a path swapped with another repository's managed worktree is still deleted. A positive fallback test is also needed.
Advisory review by Codewhale (codewhale review --pr 5854 --post, head a02571347d933d2b3855af554cfdb678b4a67053). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| let Ok(repo_root) = fs::canonicalize(&details.repo_root) else { | ||
| return false; | ||
| }; | ||
| let registrations = repo_root.join(".git").join("worktrees"); |
There was a problem hiding this comment.
[INFO] Registrations directory should be canonicalized before prefix comparison
registrations is built as repo_root.join(".git").join("worktrees"), where only repo_root is canonicalized. If .git is a symlink or a separate gitdir, canonicalized registration paths may not start with the non-canonical registrations prefix, causing valid linked worktrees to be rejected. This is a false negative, not data loss.
| let Ok(repo_root) = fs::canonicalize(&details.repo_root) else { | ||
| return false; | ||
| }; | ||
| let registrations = repo_root.join(".git").join("worktrees"); |
There was a problem hiding this comment.
Canonicalize the registrations directory before the starts_with check so symlinked .git directories or separate gitdirs do not cause false negatives.
| let registrations = repo_root.join(".git").join("worktrees"); | |
| let Ok(registrations) = fs::canonicalize(repo_root.join(".git").join("worktrees")) else { | |
| return false; | |
| }; |
Closes #5824. Rebased clean onto current tip. Gates: fmt clean, dead-code PASS at 425. Full matrix via CI.
Note
Medium Risk
Touches the destructive TTL cleanup path (
remove_dir_all); the new gates reduce data-loss risk but must not block legitimate lane worktree removal.Overview
Fixes #5824: lane TTL cleanup could run
git worktree removeand thenfs::remove_dir_allon any path that still looked like a git worktree, so a stale or wrong record could wipe unrelated directories (plain folders, repo roots, or subdirs).remove_worktree_if_expirednow bails out unlessis_managed_worktreesays git treats the path as a linked worktree of the owning repo: canonical path must match an entry fromgit worktree list, and the worktree’s.gitfile must point at a registration under<repo>/.git/worktrees/. The same check runs again immediately before the recursive-delete fallback ifgit worktree removefails.worktree_detailscollects the full porcelain worktree list (not just repo root) to support that gate. New tests cover non-worktrees, repo roots/subdirs, and paths swapped after provisioning.Reviewed by Cursor Bugbot for commit a025713. Bugbot is set up for automated code reviews on this repo. Configure here.