feat: add daft repo remove and daft repo subcommand category

[avihut]

Summary

• Adds daft repo remove [<path>] — removes a Git repo (bare git dir + every checked-out worktree), running worktree-pre-remove/worktree-post-remove hooks for each worktree before tearing it down. Works on any Git repo, daft-managed or vanilla; resolves the bare via git rev-parse --git-common-dir (CLI) or gix::discover (gitoxide backend) so it accepts a path to the project, a worktree, or a child directory.
• Introduces a new daft repo subcommand category for repo-level operations (groundwork for the catalog work in Repo Catalog #357).
• Updates the development sandbox: new mise-tasks/sandbox/clean-repos walks the sandbox test/ tree and tears down every git repo it finds via daft repo remove, so test repos with side effects (docker containers/volumes, mounts) are cleaned up properly rather than rm -rf'd.

Other notable bits delivered alongside:

• Hook failure policy: best-effort per-worktree, warned-only runs exit 0, abort-mode failures bump exit code (matches daft prune).
• Layout-safe parent walk: never removes the directory containing the project root, even when empty (regression fix from field testing).
• Inline TUI for parallel multi-worktree removal; sequential output for single-task and empty-worktree cases (avoids the empty-table-with-headers visual glitch).
• Pre-push cargo check --all-targets lefthook hook so post-rebase compile breakage gets caught before push.

Coverage

• 1525 unit tests passing, including state-machine pinning of the full event flow, gix/CLI parity tests for both backends, parent-preservation guard, head-metadata population.
• 11 bats integration tests in tests/integration/test_repo_remove.sh covering basic, dry-run, hooks, non-git rejection, run-from-various-paths, parent preservation (data-loss regression), bare-only repo, and the sandbox clean-repos helper.
• 7 YAML manual scenarios in tests/manual/scenarios/repo/.
• Man page, VitePress CLI reference page, help-text categorization, shell completions for bash/zsh/fish/Fig.

Test Plan

• mise run test:unit passes
• mise run clippy clean
• mise run test:integration passes (bats + YAML)
• mise run man:verify and mise run docs:cli:verify clean
• Manual: daft repo remove <path> from outside the repo removes everything cleanly
• Manual: daft repo remove (no arg) from inside a worktree triggers DAFT_CD_FILE redirect so the shell cds out before its cwd disappears
• Manual: daft repo remove --dry-run prints the plan without touching anything
• Manual: bare-only repo (cloned with --no-checkout) reports (bare): removed rather than rendering an empty TUI
• Manual: a worktree-pre-remove hook that tears down a docker container fires for every worktree before removal
• Manual: parent of project root is preserved even when it would otherwise be empty after removal

Fixes #421
Related: #443 (clone -n validation), #444 (daft list empty state), #446 (YAML fail_mode for pre-remove), #357 (catalog → jobs lookup for removed repos).

🤖 Generated with Claude Code

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 3m 26s —— View job

---

Code Review — daft repo remove + daft repo subcommand category

This is a well-structured, deeply tested PR. The core logic is sound, the state-machine design is coherent, and the regression tests are thorough. The following are the actionable findings, roughly ordered by severity.

---

Findings

1. run() argv slicing is fragile — medium severity

src/commands/repo/remove.rs:48-51

let argv: Vec<String> = std::iter::once("git-daft-repo-remove".to_string())
    .chain(std::env::args().skip(3))
    .collect();

skip(3) hardcodes the assumption that the user typed daft repo remove …. If invoked via a future shortcut alias (e.g. daft rr … — one fewer word in argv[0]), or via git daft repo remove (one extra word), the skip offset is wrong and arguments will be silently dropped or misinterpreted. Every other multi-word subcommand in the codebase that pattern-matches on args[1]/args[2] re-slices at parse time via clap, but repo remove bypasses that by rebuilding argv manually.

The setup shortcuts path uses the same trick, so this is an accepted pattern for now — but it should at least assert the invariant (debug_assert_eq!(args[1], "repo"); debug_assert_eq!(args[2], "remove");) or extract a shared helper. Fix this →

---

2. Hardcoded "origin" remote in hook context — medium severity

src/commands/sync_shared.rs:459

let ctx = HookContext::new(
    hook_type,
    "repo-remove",
    &target.project_root,
    &target.bare_git_dir,
    "origin",        // ← hardcoded
    &target.project_root,
    &entry.path,
    …
)

Users who have configured remote = "upstream" in their daft config will see $DAFT_REMOTE_NAME=origin in their pre/post-remove hooks. Since execute_remove_worktree_task accepts only HooksConfig (not DaftSettings), the remote name isn't threaded through. The HooksConfig::default() construction at both the sequential and TUI call sites also drops the remote. A hook that uses $DAFT_REMOTE_NAME (e.g. to prune a remote tracking branch) will silently target the wrong remote. Fix this →

---

3. source_worktree in HookContext is set to project_root, not the main worktree — minor

src/commands/sync_shared.rs:457

let ctx = HookContext::new(
    …
    &target.project_root,  // source_worktree
    &entry.path,           // worktree_path
    …
)

For a daft-managed bare-layout repo, project_root is the directory that contains .git/ and worktree siblings (e.g. /sandbox/test/myrepo). The "source worktree" (main worktree) lives at e.g. /sandbox/test/myrepo/main. Hooks that cd "$DAFT_SOURCE_WORKTREE" and run git commands will land in a non-git working directory and fail with a confusing error. This should be set to the main worktree path, which is the first entry in enumerate_worktrees or can be derived from git rev-parse --show-toplevel off the bare.

---

4. hook_viewport_budget — unchecked usize → u16 cast — low severity

src/commands/repo/remove.rs:323-325

fn hook_viewport_budget(worktrees_len: usize) -> u16 {
    5 + (worktrees_len as u16) * 4
}

Panics in debug mode when worktrees_len > 16383 (overflow in as u16 * 4). In release mode it silently wraps. Saturating arithmetic is the right fix:

fn hook_viewport_budget(worktrees_len: usize) -> u16 {
    let rows = worktrees_len.saturating_mul(4).saturating_add(5);
    rows.min(u16::MAX as usize) as u16
}

Fix this →

---

5. post-remove hook receives an already-deleted worktree path — worth documenting

src/commands/sync_shared.rs:400-419

run_remove_hook_best_effort(…, HookType::PreRemove, …);
remove_repo::remove_worktree_filesystem(target, &entry.path);
// entry.path is now gone ↓
run_remove_hook_best_effort(…, HookType::PostRemove, …);

By design, but the $DAFT_WORKTREE_PATH env var passed to the post-remove hook points to a directory that no longer exists. Hooks that cd "$DAFT_WORKTREE_PATH" or stat that path will fail. This matches the lifecycle semantics used by daft prune's post-remove, but since repo-remove is new it should be explicitly called out in the long_about text and the VitePress CLI reference page so hook authors know not to rely on the directory being present. Fix this →

---

6. HookExecutor::new constructs (and loads the trust DB) twice per worktree — performance note

src/commands/sync_shared.rs:441-483

run_remove_hook_best_effort calls HookExecutor::new(hooks_config.clone()) independently for PreRemove and PostRemove. If HookExecutor::new loads the trust DB from disk each time, this is 2 × n_worktrees disk reads. For a repo with 20 worktrees that's 40 trust-DB reads. The same pattern exists in the prune path so this is consistent, but it would be worth coalescing at the execute_remove_worktree_task level if trust-DB load time becomes a bottleneck.

---

7. parse_worktree_list_porcelain — duplicated flush block — style

src/core/worktree/remove_repo.rs:261-295

The same "flush current entry, reset state variables" block appears identically on blank line AND on worktree prefix. This is correct and tested, but extracting it into an inline closure or a helper keeps the parser DRY and reduces the chance of the two copies diverging. Not blocking.

---

8. No test for non-interactive stdin rejection — minor gap

src/commands/repo/remove.rs:80-86

if !std::io::IsTerminal::is_terminal(&std::io::stdin()) {
    bail!("Refusing to run without --force in non-interactive mode");
}

There is no integration test for echo "" | daft repo remove <path> → non-zero exit. The error message is important enough that a bats one-liner would be worthwhile. Not blocking.

---

Strengths

• Regression test for parent-directory preservation (test_repo_remove_preserves_empty_parent_directory, unit tests in remove_repo.rs) is excellent — exactly the kind of regression test CLAUDE.md requires for every bug fix.
• Dual-backend parity (for_each_backend) — running every core test against both CLI and gitoxide backends is the right call.
• check_tui_failures_strict + unit tests — the asymmetry between tolerant (prune) and strict (repo-remove) exit-code semantics is clearly modeled and tested.
• DaftSettings::load_global() — the explicit explanation of why load() can't be used here is a good comment that prevents future regression.
• lefthook.yml pre-push cargo check --all-targets — catches post-rebase compile breakage before tests run; a worthwhile addition.
• TUI viewport budget always allocated — the comment about hooks doing critical teardown (docker, mounts) even at verbose=0 is correct and the implementation matches the intent.

---

Summary

No blockers, but items 1 (argv fragility), 2 (hardcoded "origin" in hook env), and 3 (source_worktree pointing to non-git dir) are worth addressing before merge since they affect hook correctness for real users. Items 4 and 5 are low-risk quality improvements. Items 6–8 are non-blocking suggestions.

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 6m 31s —— View job

---

Code Review — daft repo remove + daft repo subcommand category

Tasks

• Read CLAUDE.md guidelines
• Read core implementation files
• Read test files
• Analyze correctness, idioms, test coverage, hot-path constraints, security, and performance
• Post review feedback

---

This is a well-engineered, deeply-tested PR. The prior review's four actionable items are all correctly resolved. Findings below are ordered by severity.

---

What the Prior Review Flagged → Now Addressed

Prior Finding	Status
1. skip(3) argv fragility	✅ debug_assert! added (remove.rs:59-63)
2. Hardcoded "origin" remote	✅ remote_name: &str threaded through from settings.remote
3. source_worktree → non-git dir	✅ main_worktree_path() finds first non-bare, non-detached entry
4. u16 overflow in hook_viewport_budget	✅ Saturating arithmetic with .try_into().unwrap_or(u16::MAX)
5. worktree-post-remove path-gone documentation	✅ In long_about (remove.rs:20-22) and in the VitePress doc
7. Duplicate flush block in parser	⏸ Still present — non-blocking
8. No test for stdin non-interactive rejection	⏸ Still missing — non-blocking

---

New Findings

1. HooksConfig::default() silently drops user-configured hook settings — minor correctness

src/commands/repo/remove.rs:217, src/commands/repo/remove.rs:435

Both the sequential and TUI paths construct HooksConfig::default() instead of loading from settings. This drops user-configured hook behavior: daft.hooks.timeout, daft.hooks.output.quiet, daft.hooks.output.tailLines, per-hook failMode, etc.

// sequential path
let hooks_config = crate::hooks::HooksConfig::default();

// TUI path
let hooks_arc = Arc::new(crate::hooks::HooksConfig::default());

DaftSettings::load_global() is already called at line 81, but there is no corresponding load_hooks_config_global() call. Since prune.rs:145 has the same gap, this is a pre-existing pattern, not a regression introduced here. Still worth flagging because users with custom hook timeouts (e.g. daft.hooks.timeout = 600 for long-running docker teardown hooks) will silently time out at the 300-second default during repo remove. Consider calling crate::core::settings::load_hooks_config_global() and using the result here — same argument as load_global() for use_gitoxide. Fix this →

---

2. run_sequential double-counts failure from the same task — minor redundancy

src/commands/repo/remove.rs:234-244

let line = match &msg {
    TaskMessage::Failed(e) => {
        any_failed = true;   // ← set here when msg is Failed
        e.clone()
    }
    _ => "removed".to_string(),
};
if matches!(status, TaskStatus::Failed) {
    any_failed = true;       // ← set again; status == Failed iff msg == Failed(e)
}

execute_remove_worktree_task only returns (TaskStatus::Failed, TaskMessage::Failed(...)) as a pair — they're always in sync. The if matches!(status, TaskStatus::Failed) check is unreachable without already having set any_failed in the match arm above. Not a correctness bug (double-setting a bool to true is harmless), but the redundancy could mask a future refactor where the two fall out of sync. Either the match arm or the if matches! check should be removed.

---

3. !success || warned condition adds successful warn-mode hooks to the summary — theoretical edge case

src/commands/repo/remove.rs:270

if !success || warned {
    hook_summaries.push(HookSummary { ... });
}

If warned = true while success = true (hook ran in FailMode::Warn and succeeded), this pushes to hook_summaries and renders it as "warned" in the output. The condition should likely be if !success (summarise failures only), with the sequential any_failed check already gated on !success && !warned. In practice, the hook execution stack likely never emits success=true, warned=true, but the invariant is implicit rather than enforced. Fix this →

---

4. repo/mod.rs — --version falls into unknown-subcommand handler — minor UX

src/commands/repo/mod.rs:20

daft repo --version hits handle_unknown_subcommand("daft repo", "--version", ...), which suggests "remove" as a typo correction. The --help/-h cases are handled but --version is not. Low-frequency but surprising. Suggest adding "--version" to the handled cases:

"" | "--help" | "-h" | "--version" | "-V" => {
    print_help();
    Ok(())
}

---

5. enumerate_worktrees_gix silently skips stale linked worktrees — documentation gap

src/core/worktree/remove_repo.rs:145-148

let path = match proxy.base() {
    Ok(p) => std::fs::canonicalize(&p).unwrap_or(p),
    Err(_) => continue,  // stale worktree silently skipped
};

A linked worktree whose base path can't be resolved (broken symlink, manually rm -rf'd without git worktree prune) is silently skipped. The CLI path doesn't have this behavior — git worktree list --porcelain still emits the entry with its recorded path (even if it no longer exists on disk). This gix/CLI asymmetry means the gix backend reports fewer worktrees to remove than the CLI backend for repos with stale entries. The bare git dir's worktrees/ directory will still contain the stale admin entries after the bare is removed (though that's moot since the whole directory is rm -rf'd). Worth a comment explaining why the skip is safe: the bare dir removal via fs::remove_dir_all destroys any remaining admin entries regardless.

---

6. Sequential path rx channel not drained after the bare-removal task

src/commands/repo/remove.rs:285-294

After execute_remove_bare_task(target) there is no rx.try_recv() drain. execute_remove_bare_task doesn't run hooks so no events are emitted, making this safe today. But if a future refactor adds a post-bare-removal hook (e.g., a repo-level post-remove hook distinct from per-worktree hooks), the events would be silently dropped. A defensive drain at line 294 (or after execute_remove_bare_task) would future-proof the sequential path. Non-blocking.

---

Strengths

• All four actionable prior-review items resolved correctly. The remote name threading, main_worktree_path function, debug assert, and saturating viewport budget are clean fixes.
• post-remove hook lifecycle documented in both long_about and the VitePress CLI reference page — exactly what hook authors need.
• Dual-backend parity tests via for_each_backend — every core assertion runs under both the CLI and gitoxide backends.
• Parent-preservation regression test (remove_bare_directory_does_not_delete_empty_parent_of_project_root) satisfies the CLAUDE.md requirement that every bug fix include a regression test.
• debug_assert! on argv shape — correctly surfaces future dispatcher mismatches in debug builds without overhead in release.
• HooksConfig::default() consistency — matches the existing prune command pattern so behavior is uniform across removal paths.
• Shell wrapper integration — daft repo added to the layout|repo case in BASH_ZSH_WRAPPER with proper DAFT_CD_FILE semantics; daft → command daft repo remove doesn't lose cd support.

---

Summary

No blockers. Item 1 (hooks config from settings) is the most likely to bite real users (long docker-teardown hooks timing out at the 300s default). Items 2 and 3 are low-risk cleanup. Items 4–6 are minor or documentary. Safe to merge after the CLAUDE.md pre-merge checklist passes.