fix(chunker): remove empty parent dirs on download delete (#21)

[dubadub]

Summary

Closes #21. When the syncer downloads a file-delete event, parent
directories that become empty as a result are now also removed —
fixing ghost folders left behind on receiving devices.

• Chunker::delete walks parents upward after fs::remove_file and
  calls fs::remove_dir (which natively fails on non-empty dirs,
  giving strict-empty semantics). Stops at the storage root and on
  any error (ENOTEMPTY is the normal terminating condition).
• Four unit tests pin the behavior: happy path, stop-at-non-empty
  ancestor, storage-root preservation, sibling preservation.
• // SAFETY: comment documents the trusted-server assumption
  behind the lexical dir == base_path guard. trace! log on the
  swallowed remove_dir error path aids field debugging.
• Spec: docs/superpowers/specs/2026-05-18-empty-folder-cleanup-design.md
• Plan: docs/superpowers/plans/2026-05-18-empty-folder-cleanup.md

Test Plan

• cargo test -p cooklang-sync-client — 125 passed, 0 failed
• New: empty parent directories get removed
• New: stop at first non-empty ancestor (sibling preservation)
• New: storage root never removed
• New: same-dir siblings preserved
• No regression in pre-existing chunker / indexer / syncer tests

[claude]

Code Review

Overview

This PR correctly fixes issue #21 by walking parent directories after fs::remove_file and removing them when empty, using remove_dir's built-in ENOTEMPTY semantics as the stop condition. The core logic is minimal and sound. A few items worth discussing:

---

Issues to address before merging

1. Use starts_with instead of == for the base-path guard

The SAFETY comment itself flags this risk: if full_path ever escapes base_path (e.g., a server-sent path with .. components), the dir == self.base_path guard won't trigger and the walk will continue above the storage root, making remove_dir calls on unrelated directories until ENOTEMPTY stops it.

remove_dir won't remove a non-empty ancestor, so this isn't catastrophic, but the intent is clearly "never go above base_path." Encoding that intent directly is safer and self-documenting:

while let Some(dir) = parent {
    if !dir.starts_with(&self.base_path) || dir == self.base_path {
        break;
    }
    // ...
}

Or simply:

if dir == self.base_path || !dir.starts_with(&self.base_path) {
    break;
}

This aligns with the SAFETY comment's own recommendation and removes the need for the trust-assumption paragraph.

---

2. The docs/superpowers/ planning documents should not ship in the repo

docs/superpowers/plans/2026-05-18-empty-folder-cleanup.md (345 lines) and docs/superpowers/specs/2026-05-18-empty-folder-cleanup-design.md (147 lines) are internal AI-agent planning artifacts — the plan file even opens with a directive aimed at agentic workers. They describe the "how we arrived here" process, not anything a future contributor or user would look up. Shipping them:

• Bloats git log / git blame for docs/ with scaffolding noise.
• Leaks internal tooling conventions into a public repo.
• Sets a precedent of date-stamped plan files accumulating in the tree.

Consider dropping both files from this PR (the relevant design rationale is already captured in the SAFETY comment and the PR description).

---

Suggestions (non-blocking)

3. Trim the in-code comment block

The // Walk parents upward… comment + // SAFETY: block runs to 11 lines for ~8 lines of code. The "why remove_dir is the right primitive" and "why ENOTEMPTY is the normal exit" are the non-obvious parts and worth keeping short; the rest reads like the spec doc. Something like:

// Remove now-empty parent directories up to (but not including) the
// storage root. `remove_dir` fails atomically on non-empty dirs
// (ENOTEMPTY is the normal exit, not an error to propagate).
//
// SAFETY: path comparison is lexical. Sound because `full_path` is
// built with `base_path.push(path)` and server paths are trusted
// not to contain `..` (see indexer's WalkDir usage). Use
// `starts_with` if that contract ever changes.

4. Missing test: file-not-found on the file itself returns an error

The four tests cover the happy path and all three stop conditions for the walk. They don't cover the case where remove_file itself fails (e.g., file already gone). Existing behavior is to propagate as SyncError::from_io_error, which is correct — a brief test pinning that would complete the picture, though this is low priority given the pre-existing test delete_removes_file_from_storage_dir.

---

What's working well

• Using remove_dir instead of read_dir + count is the right call — atomic, no TOCTOU window, one syscall per level.
• The trace! on the swallowed error is a good debug aid.
• Four focused tests with clear names and comments explaining why each case matters (especially the non-empty ancestor test guarding against accidental recursive deletion).
• The existing syncer.rs call site is correctly left untouched.

---

Summary: The implementation logic is correct. The two blocking items are (1) using starts_with for a defense-in-depth guard and (2) dropping the planning docs from the changeset. Everything else is polish.