[JacobStephens2]

Summary

A failed Skill or Review checkout acquisition no longer leaves an empty thirdshift-skill-* or thirdshift-review-* directory in the temp root.

 SkillWorktree::checkout / on_pull_request
-  path = tempdir().keep()
-  acquisition::add(...)?                # error returns; empty dir stays
+  acquire_in_temp(prefix, |path| acquisition::add(...))
+    record the directory's dev/ino, then keep()
+    on error: remove_dir(path) only if it is still that same directory
+              (remove_dir refuses non-empty dirs, so retained checkouts and hook work survive)
+    return acquisition's original error

Confirmed cause: SkillWorktree released the TempDir guard with keep() before calling acquisition::add / add_review. Acquisition counts that directory as pre-existing (path_existed), so its recovery is allowed to leave the directory in place. And because no SkillWorktree was returned, nothing removed it. Keeping TempDir's recursive cleanup during acquisition would be unsafe, because a failing hook can leave work that ADR 0014 requires us to retain. So only the still-empty directory is removed.

The new tests are in src/worktree/skill_tests.rs. Each runs in a child process with its own TMPDIR, so the test can count leftover directories:

• an invalid commit through SkillWorktree::at and through on_pull_request
• the Review branch occupied by another worktree between preflight and git worktree add, using a Git wrapper; the occupier's checkout and the branch stay intact
• controls: refusal before allocation when the branch is already occupied, hook-created work retained on both paths, and normal cleanup after success

Evidence

Execution evidence

• Before (regression tests at 2efb0d0 with the skill.rs fix not applied): cargo test --bin thirdshift worktree::skill_tests

  test worktree::skill_tests::a_failed_skill_checkout_leaves_no_allocation_behind ... FAILED
    left: ["thirdshift-skill-QV1pA8"]
   right: []
  test worktree::skill_tests::a_failed_review_checkout_leaves_no_allocation_behind ... FAILED
    left: ["thirdshift-review-MjxtLP"]
   right: []
  test worktree::skill_tests::a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation ... FAILED
    left: ["thirdshift-review-Dzbpar"]
   right: []
  test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 1763 filtered out
  

  After: cargo test --bin thirdshift worktree::skill_tests

  test worktree::skill_tests::a_branch_occupied_before_review_is_refused_before_allocation ... ok
  test worktree::skill_tests::a_failed_skill_checkout_leaves_no_allocation_behind ... ok
  test worktree::skill_tests::a_failed_review_checkout_leaves_no_allocation_behind ... ok
  test worktree::skill_tests::a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation ... ok
  test worktree::skill_tests::hook_work_from_a_failed_skill_or_review_checkout_survives ... ok
  test worktree::skill_tests::successful_skill_and_review_checkouts_are_cleaned_up ... ok
  test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 1763 filtered out
  

• scripts/affected-tests origin/main selected the full suite: PASS cargo test (846.99s). That run was on the first commit. The second commit only renames things and tightens the tests, and the focused module above passes on it. cargo clippy --all-targets reports no warnings.

Merge Danger

Door: two-way

Blast Radius: narrow. The change only affects failed Skill and Review acquisitions: the one directory this attempt allocated is removed if it is still empty and still the same directory instance.

Review coverage

Standards and Spec reviewers read every changed file (src/worktree.rs, src/worktree/skill.rs, src/worktree/skill_tests.rs). Fixed after review: Spec 1 (hook-work control now covers on_pull_request), Spec 3 (the occupation test now asserts the error names the branch), Standards 4–6 (renamed allocate → acquire_in_temp, isolated → ran_in_isolated_child, MISSING → MISSING_COMMIT).

Unaddressed findings

Standards

• 1. Possible duplicated directory identity check (skill.rs acquire_in_temp). Declined. The cited checks (ownership.rs:735 verify, review_recovery.rs:225 verify_file) verify pinned ownership evidence in other modules. This one is a single comparison that guards remove_dir, so sharing a helper would couple unrelated checks.
• 2. Hand-rolled Git shim instead of test_support::git_fault. Declined. git_fault cannot set TMPDIR, and these tests need an isolated temp root to count leftover directories. Widening a shared helper used by other suites is outside this fix.
• 3. Child re-run near-duplicates assert_test_without_warnings. Declined. That helper fails on any warning: line, and the launch_directory fixture's empty clone prints one. The comment in ran_in_isolated_child records this.

Spec

• 2. Tests are not in the existing acquisition tests. Declined, as the Agent Brief directs: that enum's Review variant uses ReviewWorktree and does not reach these allocation paths. SkillWorktree::at has no branch, so its regression uses an invalid commit.
• 4. Reviewer could not confirm scripts/affected-tests ran. Addressed by the run above: scripts/affected-tests origin/main → PASS cargo test. Local main was stale, so origin/main was used.

Closes #1975

Built with claude 2.1.314 · opus · default effort

Security review outcome

No unaddressed introduced findings.