refactor: simplify internals and shell configuration

[max-sixty]

This removes duplicated internal paths while making shell configuration changes safer across the confirmation prompt.

• consolidate config-state clearing and reporting
• inline one-use PR/ref adapters and remove test-only production APIs
• make shell rc and Fish completion writes atomic or preimage-checked, so concurrent edits are preserved
• keep recognizing historical shell-integration lines while applying only the cleanup the user previewed
• retarget tests to the canonical APIs and add PTY coverage for confirmation-window changes

The protected CLI and configuration interfaces are unchanged. Production code remains substantially smaller; the added test coverage carries the shell-safety and shared-cache proofs explicitly.

Tested with cargo run -- hook pre-merge --yes: 4,659 tests passed, with Clippy, docs, doctests, snapshots, formatting, and repository policy checks also clean.

This was written by Codex on behalf of max-sixty

[worktrunk-bot]

The preimage checks are a real improvement, and the split_inclusive rewrite in uninstall_previewed_lines incidentally fixes the old lines().join("\n") path, which normalised a CRLF rc file to LF on every uninstall. Folding the completion preview into scan_shell_configs is also what lets the first-run offer name the fish completion write, which it previously didn't. Two findings and a coverage note.

Uninstall reports "Removed" for lines it didn't remove — inline below. This is the one I'd want fixed before merge; it's a user-visible regression in the same command the rest of the PR is making safer.

Deleting tests/integration_tests/cache_sharing.rs drops more than the shares_cache_with removal required. Three of the five tests there — test_cache_shared_between_clones, test_merge_base_cache_shared, test_parallel_tasks_share_cache — never called shares_cache_with. They cover the Arc<RepoCache> contract documented under # Caching in src/git/repository/mod.rs: merge-base results visible across clones, and the clone-per-thread pattern wt list depends on. The replacement assertion in branches.rs covers Arc::ptr_eq plus default_branch_sha, so the merge-base and cross-thread cases lose their only coverage. Only test_repository_clone_shares_cache and test_separate_repositories_have_separate_caches were actually blocked by removing the method; I'd re-home the other three rather than delete them.

codecov/patch is red — 94.82%, 24 of 463 patch lines missed, against a 98.12% base, so the auto target won't clear. Most of the misses are new I/O error arms with no deterministic trigger: remove_config_file's NotFound and read-error branches, verify_completion_preimage's read-error branch, preview_shell_completions's read-error branch. state.rs:1680 ((false, None) => bail!) is a pre-existing miss the match rewrite pulled into the patch — same lines, same behaviour, just relocated. The one that looks genuinely testable is configure_shell.rs:1110, the AlreadyExists short-circuit in apply_shell_completions; a unit test in the style of the two new ones beside it would close it.

Not approving, on two counts. The mechanical one: codecov/patch is failing, and CLAUDE.md under Coverage requires explicit approval before merging over it. The judgement one: this edits the shell-config deletion path — remove_config_file's fs::remove_file over ~/.config/fish/functions/wt.fish, conf.d/wt.fish, nushell autoload wrappers and fish completions, plus the in-place rewrite of ~/.zshrc, ~/.bashrc and PowerShell profiles. .claude/skills/running-tend/references/review-pr.md → Data-Loss Surface: Hold for Human Review says a diff that edits a file containing std::fs::remove_file isn't mine to approve. You're the human it defers to, so this is a COMMENT rather than a block.

Also checked

• must_resolve, available_branches, is_gitlab, local_branch_name, repo_url_from_ref_url, repo_info_from_ref_url, HookLog::internal/shared, approve_command, shares_cache_with — no remaining references anywhere in the tree, so the removals are clean.
• config_paths / completion_path / legacy_fish_conf_d_path narrowing to pub(super): the inherent Shell methods at src/shell/mod.rs:142, :164, :152 already wrapped them, so the one external caller in detection.rs routing through Shell::Nushell.config_paths(cmd) is a straight dedup.
• The RefBaseConflict check moving up into resolve_switch_target keeps the same ordering both pr: and mr: had before (base rejected before any forge CLI runs), and --base pr:N still routes through choose_pr_provider.
• configure_shell_file switching from OpenOptions::append to read + write_atomically: this is what CLAUDE.md → Data Safety ("Replace files, never truncate them") asks for on rc files, and the new symlink/mode assertions in test_configure_shell_with_yes pin the properties that matters for. The CRLF trim_end_matches('\r') in the already-installed check also makes install idempotent on a CRLF rc file, which it wasn't.
• Overlapping open PRs: #1312 also touches src/commands/configure_shell.rs, src/shell/detection.rs and src/shell/mod.rs, and #3791 touches src/commands/picker/mod.rs and src/output/handlers.rs. No duplicated fix in either — #1312 will need a rebase around the UninstallAction removal.

[worktrunk-bot]

The new tests land on the arms the previous review named — remove_config_file's NotFound and read-error branches, verify_completion_preimage's and preview_shell_completions's read-error branches, and state.rs's (false, None) => bail!. codecov/patch hasn't reported on this head yet. One finding, inline.

test_preimage_writes_leave_files_intact_on_io_error is missing the root probe. It sets the tempdir to 0o500 and then unwrap_err()s both results. Under euid 0 the mode bits are ignored, so fs::remove_file succeeds, remove_result is Ok(()), and the test panics on unwrap_err — and write_atomically succeeds too, so the Failed to write assertion goes the same way. tests/CLAUDE.md names this as a property the suite already has ("Tests that need an unprivileged uid skip automatically when running as root, which both this environment and Codex Cloud do"), and every other permission test in the tree carries the probe: test_remove_dir_with_progress_skips_unreadable_subtree in src/remove_dir.rs, test_configure_shell_fish_legacy_remove_failure_warns in tests/integration_tests/configure_shell.rs, and the copy_approved_commands_to_approvals_file test in src/config/deprecation.rs. CI runs unprivileged so this stays green there; it only breaks the Claude Code web / Codex Cloud / Docker-as-root path.

Same gap in test_uninstall_from_file_leaves_rc_intact_when_write_fails, the other 0o500 test in this module — pre-existing rather than yours, but it's the same insert if you want both.

The other two new unit tests are correctly left un-gated: test_preimage_verification_rejects_directories asserts only the message prefix, and Windows reaches the same read-error arm (File::open on a directory fails with ERROR_ACCESS_DENIED, not NotFound), while test_configure_shell_rejects_invalid_utf8_before_modifying_file hits str::from_utf8 on the byte scan on every platform. test_configure_shell_rejects_completion_directory needs no probe either — EISDIR is 21 on both Linux and macOS and root doesn't bypass it — and the abort lands in the preview scan, before apply_confirmed_shell_config writes anything, so the snapshot's empty stdout is the real thing.

Still a COMMENT rather than an approval, for the data-loss-surface reason in the previous review.

[worktrunk-bot]

The reporting fix is the right shape: having UninstallResult::apply return what it actually changed, and letting the printer consume only the survivors, makes Removed … mean what it says — and it fixes the restart hint and the shell_was_removed suppression of the completion_not_found warning for free, since both read the same post-apply results. codecov/patch also passes on this head, so the mechanical half of the previous hold is gone. Resolving both inline threads; the verify_completion_preimage rationale (narrowing the window before the atomic replace, not defending against create_dir_all itself) is a fair call.

Two things on the incremental.

bc4d18674 trades real coverage for a root fragility it doesn't actually remove. test_preimage_writes_leave_files_intact_on_io_error was the only test reaching remove_config_file's Failed to remove {}: {error} arm and apply_shell_completions's Failed to write {}: {e} arm, and the only assertion that both leave the user's bytes byte-identical when the write fails — which is the property the preimage/write_atomically machinery exists to guarantee. Nothing else in mod tests reaches either: test_remove_config_file_requires_matching_preimage covers the mismatch, success and NotFound returns, test_preimage_verification_rejects_directories covers the read-error arm, and test_apply_shell_completions_verifies_before_creating_parent stops at the first preimage check. Deleting it also doesn't make the module root-safe — test_uninstall_from_file_leaves_rc_intact_when_write_fails, in the same mod tests, still sets the tempdir to 0o500 and asserts result.is_err(), which euid 0 ignores, so Docker-as-root / Codex Cloud still breaks there. Net effect: the coverage went, the fragility stayed. The six-line probe from the previous review keeps both. Happy to push a commit restoring the test with the probe and adding the same probe to the surviving one, if you want it.

A filtered-out entry is silent unless it's the only entry. print_shell_uninstall_result reaches No shell integration found to remove only when total_changes == 0, which the new PTY test hits because zsh is the sole result. Run uninstall with no shell filter against a machine with both ~/.bashrc and ~/.zshrc configured, replace the zsh line during the confirmation window, and the output is Removed shell extension & completions for bash plus Removed integration from 1 shell — zsh, which the preview named a few lines earlier and the user just approved, produces no line at all. It's dropped from results and it never entered not_found, since it was present at scan time. Honest by omission rather than a false claim, so a milder form of the same problem, but a per-result fallback (the No worktrunk lines left in ~/.zshrc shape) would close it and would also carry the single-entry case more precisely than the generic "found nothing" message does.

Still a COMMENT rather than an approval, and now on my own judgment rather than the coverage gate: this edits the shell-config deletion path — fs::remove_file over the fish/nushell wrappers and fish completions, plus in-place rewrites of ~/.zshrc, ~/.bashrc and PowerShell profiles — which .claude/skills/running-tend/references/review-pr.md → Data-Loss Surface: Hold for Human Review puts outside what I approve. You're the human it defers to.

Also checked

• uninstall_previewed_lines's new removed vector is normalised the same way scan_rc_file_for_uninstall builds matched_lines (line.trim()), and both are in file order, so the post-apply matched_lines replacement is a strict subset in the same shape — no formatting drift between the preview block and the confirmation block.
• Duplicate previewed lines still remove one file occurrence each: remaining.remove(position) pops per match, so a concurrent writer that appends a second copy leaves it in place and the report names only the one that went. The new [integration, integration] assertion in the PowerShell CRLF test pins that.
• filter_map(|result| result.apply().transpose()).collect::<Result<Vec<_>, _>>() short-circuits on the first Err and leaves later entries unapplied, matching the ?-in-a-loop behaviour it replaced.
• The remaining post-apply readers of results — shell_count, the completion_not_found skip, and current_shell_affected — all get more accurate under the filtering, not less.
• The pre-existing Removed integration from 0 shells case (fish completions present, wrapper absent) is reachable from scan_for_uninstall independently of this change, so the filtering doesn't introduce it.

[worktrunk-bot]

Both findings from the last review are addressed. UninstallResult::apply now hands its None case back as a (shell, path) pair that lands in not_found / completion_not_found, so a previewed entry that goes stale during the confirmation window gets its own line instead of vanishing from the report — that closes the multi-shell case (Removed … for bash with zsh dropped silently) along with the single-entry one the PTY snapshot pins. test_preimage_writes_leave_files_intact_on_io_error is back with the root probe, and make_directory_unwritable carries the same probe into test_uninstall_from_file_leaves_rc_intact_when_write_fails, so the module no longer breaks under Docker-as-root / Codex Cloud. The re-homed cache test covers more than the three that went: the deleted test_parallel_tasks_share_cache only called default_branch(), this one puts merge_base through the clone and thread boundary.

One inline suggestion, and one observation on the error path.

An Err mid-apply still leaves already-removed rc lines unreported. The previous review noted the short-circuit leaves later entries unapplied, which is behaviour-preserving; the other half is that it also discards the report for the earlier ones. UninstallScanResult::apply propagates through ?, so handle_unconfigure_shell returns Err and main.rs never reaches print_shell_uninstall_result — the whole result is dropped, filtered or not. Shell::all() puts bash before zsh, so: run uninstall with no shell filter on a machine with both ~/.bashrc and ~/.zshrc configured, delete ~/.zshrc during the confirmation window, and bash's lines are gone from the user's rc file while the only output is Failed to read ~/.zshrc: No such file or directory. That contradicts the invariant the matched_lines doc comment states — "Shown before removal and again after, so no line leaves a user's rc file unseen" — and it's the same class this commit is closing, reached through the error arm rather than the None arm. Pre-existing, and it needs the rc file to disappear or become unreadable rather than merely change (a content change is what the previewed-lines rewrite absorbs by design), so not something I'd hold on — just noting the hole is still open after the fix.

Still a COMMENT rather than an approval, on the same judgment as the previous three: this edits the shell-config deletion path — fs::remove_file over the fish/nushell wrappers and fish completions, plus in-place rewrites of ~/.zshrc, ~/.bashrc and PowerShell profiles — which .claude/skills/running-tend/references/review-pr.md → Data-Loss Surface: Hold for Human Review puts outside what I approve. You're the human it defers to. test (linux|macos|windows), code-coverage and the advisory affected-tests matrix are all still in flight on this head, so I haven't seen a codecov/patch verdict either.

Also checked

• Post-apply not_found entries can't duplicate scan-time ones: scan_for_uninstall pushes to not_found only when a shell produced no results at all, so the two sets are disjoint by construction. Ordering is scan-time entries first, then the stale ones, which reads fine in both the warning and hint forms.
• The completion_not_found skip (shell_was_removed) and current_shell_affected both read the filtered results, so a stale wrapper no longer suppresses the completions warning or the restart hint — and shell_count / total_changes count only what actually went.
• The new loop is otherwise equivalent to the filter_map(…).transpose() it replaced: Ok(None) filtered, Err short-circuiting, order preserved.
• uninstall_previewed_lines early-returns before write_atomically when nothing matched, so the new None arm can't be reached via a no-op rewrite that replaced the file's inode.
• make_directory_unwritable leaves the directory at 0o500 only on the unprivileged path; both callers restore 0o700 before asserting, and the root path restores before returning false. The probe file is removed on the path that creates it, and never created on the other. The helper and both its call sites are #[cfg(unix)], so it doesn't trip the "never used" dead-code failure on test (windows) that tests/CLAUDE.md warns about.
• In the new integration test the four threads all hit Entry::Occupied on the merge_base DashMap (the main thread primes it computing expected_base), so nothing forks git while holding a shard lock; default_branch is a OnceCell, so exactly one thread runs detection and the worktrunk.default-branch config write can't race.
• Cache sharing itself — Arc::ptr_eq on a clone, and a fresh Repository::at getting its own cache — is still pinned by default_branch_sha_is_snapshot_at_first_scan in src/git/repository/branches.rs, so the integration test doesn't need to re-assert what test_repository_clone_shares_cache and test_separate_repositories_have_separate_caches covered.
• #1312 still overlaps on src/commands/configure_shell.rs; no duplicated fix, same as the previous reviews.