Skip to content

fix(dotfiles): tolerate a stale directory the deeper walk already removed - #12697

Merged
jdx merged 1 commit into
jdx:mainfrom
vladbisceanu:fix/dotfiles-prune-nested-stale-dir
Sep 2, 2026
Merged

fix(dotfiles): tolerate a stale directory the deeper walk already removed#12697
jdx merged 1 commit into
jdx:mainfrom
vladbisceanu:fix/dotfiles-prune-nested-stale-dir

Conversation

@vladbisceanu

@vladbisceanu vladbisceanu commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem

A symlink-each entry fails with No such file or directory (os error 2) on the apply that removes a source directory whose links sat at two depths, e.g. a skill directory holding SKILL.md and references/doc.md. The links are removed, the directories are removed, but the apply aborts, and every stale link after that entry in the same apply is left behind. The next apply succeeds.

Reproduces on 2026.9.0 and main:

[dotfiles]
"/tmp/target" = { source = "skills", mode = "symlink-each", manifest = "git" }
  1. skills/nested/SKILL.md and skills/nested/references/doc.md exist, apply: clean.
  2. Delete skills/nested/, commit, apply: No such file or directory (os error 2), exit 1.
  3. Apply again: all files are applied.

Deleting a flat directory (only SKILL.md) is fine, so the trigger is the second depth.

Cause

prune_stale_links walks each stale link's parent deepest first and climbs while the directory is empty. The walk from references/doc.md removes references/ and then nested/. The loop then visits nested/ again as the parent of SKILL.md and calls read_dir on the directory it just removed.

Fix

Break when the directory is already gone, using the same !dir.is_dir() || guard the two sibling walks in this file (cleanup_reconciled_directories and the unapply cleanup) already use.

The added e2e case in test_dotfiles_files mirrors the scenario: two links at two depths under one directory, the directory removed, apply succeeds and the directory is gone. It fails on main with the error above and passes with the fix. cli/test_dotfiles_files passes locally.

Summary by CodeRabbit

  • Bug Fixes
    • Improved cleanup of stale dotfile links when nested directories are removed.
    • Prevented cleanup checks from failing when an expected directory has already been removed.
    • Preserved unrelated links during stale-link cleanup.
  • Tests
    • Added coverage for removing nested source files and verifying successful status checks afterward.

…oved

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Team

Run ID: c3c2c705-8f5f-48b0-9aa5-fd0485908f53

📥 Commits

Reviewing files that changed from the base of the PR and between 8fe6385 and 39eae81.

📒 Files selected for processing (2)
  • e2e/cli/test_dotfiles_files
  • src/system/files.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The cleanup loop now checks that each path is a directory before reading it. A new end-to-end test covers removal of nested stale links while preserving an unrelated link.

Changes

Dotfiles cleanup

Layer / File(s) Summary
Handle nested stale-link cleanup
src/system/files.rs, e2e/cli/test_dotfiles_files
prune_stale_links stops when a cleanup path is no longer a directory. The end-to-end test validates removal of nested links, preservation of an unrelated link, and successful status --missing execution.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 39eae

The change prevents stale-directory cleanup from aborting when a deeper walk has already removed that directory, allowing the apply to finish and remove all stale links. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: jdx, jambalaya56562

Poem

A rabbit found a vanished door
And checked the path before the floor
Nested links were swept away
While one old link chose to stay
Clean dotfiles hopped through the day

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main fix: handling a stale directory that deeper cleanup already removed during dotfiles processing.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR prevents symlink-each apply from failing when a deeper stale-link cleanup has already removed a directory later encountered by a shallower cleanup walk.

  • Treats an already-absent stale-link parent as a terminal cleanup condition.
  • Adds an end-to-end regression case with stale links at two directory depths.

Confidence Score: 5/5

The PR appears safe to merge because the guard narrowly handles an already-removed directory and the regression test exercises the previously failing cleanup order.

The changed cleanup preserves existing empty-directory behavior, avoids the known second visit to a removed parent, and is consistent with sibling cleanup implementations; no actionable regression remains.

Important Files Changed

Filename Overview
src/system/files.rs Aligns stale-link directory pruning with sibling cleanup walks by stopping when a previously removed parent is no longer a directory.
e2e/cli/test_dotfiles_files Adds deterministic Linux end-to-end coverage for successful cleanup of stale links at two depths and verifies final convergence.

Reviews (1): Last reviewed commit: "fix(dotfiles): tolerate a stale director..." | Re-trigger Greptile

@jdx
jdx merged commit 6ff7b1a into jdx:main Sep 2, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants