fix(git): close hidden status safety gaps

[Duang777]

Follow-up to #4111.

Problem

The original fix protected untracked files hidden by status.showUntrackedFiles, but related Git display settings and race windows could still produce unsafe or misleading decisions:

• submodule dirt could be hidden by submodule.<name>.ignore or diff.ignoreSubmodules;
• --stage=tracked and --stage=none could make merge commit and cleanup decisions from state they would not actually stage;
• a file created between the pre-stage status query and git add -A could be committed without disclosure;
• cleanup eligibility could change after the target ref moved, including a pre-remove hook locking the worktree;
• porcelain rename/copy parsing only recognized index-side R / C statuses.

Fix

• Override user display configuration in destructive dirty checks and staged-gitlink detection.
• Make merge auto-commit and cleanup decisions match StageMode::{All, Tracked, None} exactly.
• Derive auto-staged additions from the before/after index delta, with bounded terminal-safe output that preserves non-UTF-8 bytes as \xNN.
• Freeze the removal disposition before updating the target ref and preserve a worktree locked by pre-remove.
• Parse rename/copy markers from both porcelain status columns.
• Route merge, step commit, step squash, and relocate through the shared staging disclosure path.

Testing

Added red/green coverage for hidden untracked files, dirty submodules, staged gitlinks, all stage modes, approval and staging TOCTOU windows, worktree lock transitions, terminal control characters, non-UTF-8 paths, warning truncation, relocate disclosure, and worktree-side rename/copy records.

cargo run -- hook pre-merge --yes
4876 tests passed, 1 skipped
lockfile, formatting, Clippy, doctest, rustdoc, and snapshot checks passed

Review focus

This changes destructive cleanup and index-mutation safety paths. A focused data-preservation review of merge/remove race handling and StageMode semantics would be useful.

[Duang777]

@max-sixty Could you take a focused data-preservation review of this follow-up to #4111, especially the pre-ref cleanup gate, StageMode semantics, and pre-remove lock handling? The full local pre-merge gate passed; GitHub CI and automated review are now running.

[worktrunk-bot]

This changes the worktree-removal decision path (src/commands/worktree/finish.rs, src/output/handlers.rs) and what wt merge refuses before deleting a worktree. That is the data-loss surface this repo's review guidance holds for a human reviewer regardless of how the diff reads, so this is a comment rather than an approval — the verdict is @max-sixty's, not mine. Adding them as a reviewer to match the ask above.

codecov/patch is red — 97.61% of diff hit against a 98.29% target — and per CLAUDE.md → Coverage that gates the merge. From the cobertura.xml in the code-coverage run for 1afa1262d6cdd21dbf655346d4364aabf0280243, eight added lines are unhit:

File	Line	Code
src/commands/merge.rs	162	StageMode::None => unreachable!(),
src/commands/worktree/finish.rs	201	None => "Worktree preserved (locked)".into(),
src/commands/worktree/finish.rs	194, 269	the ? on execute_pre_remove_hook / handle_remove_output_after_pre_remove
src/commands/commit.rs	39, 40, 61	error arms in stage_with_untracked_warning / staged_added_paths
src/git/repository/working_tree.rs	1041	the non-0/1 exit arm of has_staged_changes

Two of those are cheap to close. merge.rs:162 disappears entirely with the restructure suggested inline. finish.rs:201 is reachable from a pre-remove hook that runs git worktree lock without --reason — test_merge_pre_remove_lock_preserves_worktree currently exercises only the with-reason branch. The remaining six are subprocess-failure arms with no deterministic trigger; if closing the two above doesn't clear the target, that's the arithmetic worth handing over rather than contorting tests around.

The finish.rs module docstring no longer matches the function. It still says removal is blocked by "Five conditions … --no-remove, on-target, primary-worktree, locked, and default-branch" and that "handle_remove_output performs it". This PR adds a sixth blocker — a lock created by the pre-remove hook, checked after the hook returns — and routes the removal through handle_remove_output_after_pre_remove instead. CLAUDE.md → Code Quality asks that system docstrings stay current, and this one is the map a reader uses for exactly the ordering the PR changed. It sits outside the diff so I can't attach a suggestion to it; I'm happy to push that edit if you'd rather not hand-apply it.

The remaining inline comments are the unreachable!() restructure, a --no-relative hardening on the new staged-additions query, and two readability suggestions in parse.rs.

[Duang777]

@worktrunk-bot Thanks; ccfb7c0 addresses all four inline suggestions, updates the finish.rs lifecycle documentation, and adds coverage for no-reason hook locks plus post-stage inspection failure. The full local gate now reports 4,875 passed and 1 skipped; the new GitHub checks are in progress.

[worktrunk-bot]

Confirmed — all four inline suggestions landed verbatim at ccfb7c084, and the coverage gate is now green.

codecov/patch reports 99.08% of diff hit against the 98.29% target, up from the 97.61% that was failing on 1afa1262. Both lines I flagged as cheap to close are gone: the unreachable!() arm no longer exists (should_auto_commit now matches StageMode once and early-returns has_staged_changes() for None), and the new #[case]("git worktree lock .", "Worktree preserved (locked)") exercises the no-reason branch in finish.rs. test_step_commit_preserves_staged_changes_when_post_stage_inspection_fails also reaches one of the six subprocess-failure arms I'd written off as having no deterministic trigger — the argv-matching wrapper script is a better lever for those than I gave it credit for.

Two verification notes on the rest:

• The --no-relative hardening updated both exact-argv case patterns in the test wrappers alongside the query, so the wrappers still intercept what they intend to rather than silently falling through to real git.
• The finish.rs module docstring now describes the frozen disposition, the pre-existing-lock vs. hook-created-lock split, and handle_remove_output_after_pre_remove — it matches the function again.

test (linux), test (macos), and test (windows) all pass on this head. The merge verdict on the data-loss surface is still @max-sixty's.

[worktrunk-bot]

Two findings on ccfb7c084, both in code this PR adds.

The pre-update refusal names paths that don't block it. dirty_files() returns every porcelain record, so under --stage=tracked a modified tracked file is listed beside the untracked one — but --stage=tracked would have committed that file, and only the untracked path blocks the removal. The hint that follows ("Commit or stash changes first") then points at a path where committing changes nothing. test_merge_non_all_stage_modes_refuse_hidden_untracked_before_update has only an untracked file in its fixture, so nothing pins the listing.

escape_filename_for_terminal lets Unicode bidi controls through. char::is_control() is general category Cc only; U+202E and the rest of the Cf directional overrides and isolates reorder the rendered line without being control codes, so an auto-staged path can print as something other than the path being committed — the spoofing the ESC escaping already blocks, by a different mechanism. -z is what makes it reachable here: it turns off git's own core.quotePath, which octal-escapes those bytes.

[worktrunk-bot]

One new finding on this head, inline in finish.rs: a pre-remove hook that
locks the worktree now means two different things depending on which command
ran it. Merge treats it as a request to preserve; wt remove still fails the
removal with WorktreeLocked.

The verdict stays with @max-sixty, per the earlier pass on this PR.

[worktrunk-bot]

Two findings on d8c7f2dff, both in code this PR adds, plus a note on the PR's shape.

wt switch --prs can lose every PR row to a concurrent alt-x. The new publication_is_current() gates the --prs append on the row-mutation generation as well as the spawn generation, but the two aren't interchangeable: dropping a superseded spawn's batch is free because the alt-r that superseded it starts a --prs thread that refetches everything, while a completed alt-x removal starts no new stream and stream_open_prs runs once per refresh. Press alt-x on a worktree row while gh pr list is still in flight and the whole batch is discarded — no rows, no stashed warning, and the "loading…" marker clears anyway, so the picker just has no PRs in it until the user hits alt-r. Mechanism and a narrower guard inline.

The deferred-removal wait also gates post-switch. Waiting for the detached fallback is right for post-remove, whose template variables describe a worktree that has to be gone. post-switch is anchored at the destination and has no such dependency, but it is registered before the checkpoint scope closes, so it inherits the same condition: a dev server started from post-switch now blocks until the old directory disappears, and doesn't start at all if that removal never succeeds — the Windows-file-locking and permissions cases the legacy fallback exists to cover. Moving the wait_for_worktree_removal_since call above the post-switch registration scopes it to post-remove; details inline, and I'm happy to push it.

On the PR's shape: the picker's refresh-race reconciliation (row_mutation_generation, reconcile_successful_drop / reconcile_successful_morph, ~900 changed lines in picker/mod.rs) and the post-remove wait are both independent of the status-visibility subject this PR is named for, and neither appears in the PR body or the CHANGELOG entry. Both fix races that predate this branch — a refresh can publish a pre-removal snapshot whenever a pre-remove hook is slow, lock change or not. Split out, each could be judged on its own evidence; bundled into a 3,400-line diff over the removal surface, they make the data-safety change harder to review than it needs to be.

The verdict stays with @max-sixty, per the earlier pass on this PR.

[worktrunk-bot]

codecov/patch is red on d8c7f2dff — 97.77% of diff hit against the 98.29% target — and per CLAUDE.md → Coverage that gates the merge. This landed after the review above, so folding the numbers in here. From the codecov compare API, 28 added lines are unhit:

File	Lines	What
src/commands/run_pipeline.rs	168-174, 180-182	the symlink_metadata error arm and the REMOVAL_WAIT_TIMEOUT bail in wait_for_worktree_removal
src/commands/picker/mod.rs	401, 652-654, 671, 4649-4652	do_removal's kept-branch arm, the morph path's Err arm, and the deadline loop inside the new test
src/output/handlers.rs	1925, 1936, 2049, 2084	three ? arms plus None => "Worktree preserved (locked)"
src/commands/remove.rs	273, 282, 290	execute_removal_plan's branch-only fallthrough, the hook-failure ?, and the --force arm — all under --reap
src/commands/worktree/finish.rs	197, 275	? arms
src/commands/merge.rs, src/git/repository/working_tree.rs	181, 1041	command-failure arms
src/utils.rs	105	the '\u{2066}'..='\u{2069}' isolate range

Roughly five of those are cheap: handlers.rs:2084 is the no-reason lock under wt remove, which a second #[case] on test_pre_remove_hook_lock_preserves_worktree reaches the same way last round's "git worktree lock ." case reached the merge one; remove.rs:273/290 are wt remove --reap against a branch-only target and against --force; and utils.rs:105 wants one isolate character added to escape_text_for_terminal_escapes_controls_and_bidi. Clearing the target needs about seven, so that alone is tight.

The largest single block is the seven lines in wait_for_worktree_removal, and its timeout arm has no trigger short of a 300-second wait. That is the same machinery the review flags as independent of this PR's subject: split out, it carries its own coverage argument instead of costing this patch a third of its misses, and what remains here is mostly ? propagation arms with no deterministic trigger — the arithmetic worth handing over rather than contorting tests around.

[max-sixty]

@Duang777 please can we radically reduce the size of the PRs? I very much appreciate the recent one, thank you; but these +3K lines are really difficult to review... I would much prefer 10 PRs each with 100 (non-test / comments) lines than 1 PR with 800 lines...

[max-sixty]

@worktrunk-bot rather than add all these flags to the porcelain git commands, are there git commands that answer the question more literally, don't defer to user preferences?

[worktrunk-bot]

Answering @max-sixty's question first, then two findings on the five commits pushed since the last pass.

For the two index queries, yes — git diff-index is the literal form and drops both display flags. For submodules, no. For untracked, not for free. The plumbing diff commands (diff-index, diff-files, diff-tree) read git's basic diff config rather than the UI config git diff reads, so they never consult diff.relative or diff.renames. Swapping git diff --cached for git diff-index --cached <tree-ish> in staged_added_paths and has_staged_changes therefore makes --no-relative and --no-renames unnecessary: the property stops being a flag each new call site has to remember and becomes a property of the command. Two real costs — diff-index requires an explicit tree-ish and exits 128 on an unborn HEAD where git diff --cached falls back to the empty tree, so it needs head_sha() (a fresh rev-parse per call, ~5 ms by its own doc comment) plus an empty-tree fallback for the None case.

--ignore-submodules=none is load-bearing in both families and should stay. diff.ignoreSubmodules is UI config that plumbing ignores, but submodule.<name>.ignore is read by plumbing too, and it hides a staged gitlink from diff-index exactly as it does from git diff.

For the untracked half there is a literal command but no free win. git ls-files --others --exclude-standard asks precisely "which files are untracked" and reads no display config at all — but it trades the config dependency for a cwd dependency: it reports only the cwd subtree, cwd-relative, unless you add --full-name -- :/, where git status --porcelain is whole-repo and repo-root-relative by definition regardless of status.relativePaths. And is_dirty / dirty_files want tracked modifications, untracked paths and submodule dirt in one record stream from one subprocess; splitting that into diff-index + diff-files + ls-files --others is three calls and three parsers. --untracked-files=normal on status --porcelain is the better answer there.

git 2.55 transcripts behind the three claims

diff.relative truncates the porcelain listing and is invisible to the plumbing form. a/new1.txt and b/new2.txt staged, run from a/:

$ git -c diff.relative=true diff --cached --name-only --diff-filter=A
new1.txt
$ git -c diff.relative=true diff-index --cached --name-only --diff-filter=A HEAD
a/new1.txt
a/renamed.txt
b/new2.txt

diff.renames folds an addition into a rename for the porcelain form only, so --diff-filter=A still sees the new path under plumbing with no --no-renames:

$ git -c diff.renames=true diff --cached --name-status
A	a/new1.txt
R100	a/tracked.txt	a/renamed.txt
A	b/new2.txt
$ git -c diff.renames=true diff-index --cached --name-status HEAD
A	a/new1.txt
A	a/renamed.txt
D	a/tracked.txt
A	b/new2.txt

submodule.<name>.ignore reaches plumbing, including for a staged gitlink change, so the explicit flag is the only override:

$ git -c submodule.sub.ignore=all diff-index --cached --name-only HEAD
$ git -c submodule.sub.ignore=all diff-index --cached --name-only --ignore-submodules=none HEAD
sub

ls-files --others ignores status.showUntrackedFiles but answers only for the cwd subtree. Run from a/, with a/tracked.txt modified and a/untracked.txt present:

$ git -c status.showUntrackedFiles=no status --porcelain
 M a/tracked.txt
$ git -c status.showUntrackedFiles=no ls-files --others --exclude-standard
untracked.txt
$ git -c status.showUntrackedFiles=no ls-files --others --exclude-standard --full-name -- :/
a/untracked.txt

That leads into the first finding, inline: has_staged_changes is the one index query in this PR that didn't get --no-relative, and because it reads an exit code rather than a path list, the leak inverts its answer instead of shortening a listing.

The second finding, also inline: the new completion marker leaves a file under .git/wt/removal-markers/ that nothing reclaims when the detached removal fails — the case the fallback exists for — and that no doc lists. Git already maintains a per-worktree signal with the path-reuse property the marker's docstring gives as its reason.

Both threads from the last pass are addressed; resolving them. The post-switch reorder and the mutation-replay rewrite both land where they were asked to, and collect_published_branches correctly keeps a dropped row's branch out of the dedup set so its PR stays visible.

[worktrunk-bot]

staged_added_paths in commit.rs carries --no-relative for the same reason this array carries --ignore-submodules=none, but this query doesn't — and because it reads an exit code rather than a path list, diff.relative doesn't shorten the answer, it inverts it. diff.relative = true drops every path outside the cwd, so --quiet reports a clean index. With the only staged change in b/, run from a/ on git 2.55:

$ git diff --cached --quiet --exit-code --ignore-submodules=none
exit=1
$ git -c diff.relative=true diff --cached --quiet --exit-code --ignore-submodules=none
exit=0

should_auto_commit returns this call directly for StageMode::None, so a wt merge --stage=none would read a staged index as empty and skip the commit it was asked to make. Every caller today passes a worktree root, so it's masked — but WorkingTree::run_command runs in self.path, and path()'s own doc notes that for current_worktree() that is the discovery path, i.e. the user's cwd. So this is the same latent shape the --no-relative on staged_added_paths was added to close, and it should close the same way:

Suggested change

	"--ignore-submodules=none",
	"--no-relative",
	"--ignore-submodules=none",

(git diff-index --cached --quiet <tree-ish> needs neither flag — see the review body.)

[worktrunk-bot]

Every deferred removal now leaves a file here, and the && rm -f -- … that reclaims it runs only after git worktree remove exits 0 — so it survives exactly the failures the legacy fallback exists to cover (Windows file locking, a permission problem). Nothing else reclaims it: sweep_stale_trash walks only wt_trash_dir(), and it selects entries by the embedded timestamp that these pending-<random> names don't carry. So .git/wt/removal-markers/ accumulates one zero-byte file per failed fallback for the life of the repo, and it's a file worktrunk creates on the user's behalf that isn't in the What files does Worktrunk create? table that CLAUDE.md → Data Safety asks new code on this surface to be reviewed against.

Git already maintains a signal with the property the wait_for_removal_completion docstring gives as the marker's reason ("remains correct if another process creates a new directory or worktree at the same path"). git worktree remove deletes .git/worktrees/<name>, and a directory reappearing at the old path does not recreate it — only another git worktree add of that name would:

$ ls .git/worktrees
linked
$ git worktree remove ../linked && ls .git/worktrees
ls: cannot access '.git/worktrees': No such file or directory

WorkingTree::git_dir() already returns that path — the new has_staged_changes_surfaces_git_errors test uses it — so waiting on its absence instead drops the marker file, the leak, the rm -f tail in the detached command, and the new fallible step that create_removal_completion_marker's ? adds to execute_instant_removal_or_fallback, which on the merge path runs after the target ref has already advanced.

If the marker stays, it wants a sweep and a FAQ row. Happy to push either shape — the git_dir() swap or the sweep plus docs — whichever you'd prefer.

[worktrunk-bot]

Retracting the coverage gate I left standing: codecov/patch is green on 21b5bb210 — 98.83% of diff hit against the 98.29% target, up from the 97.77% that was failing on d8c7f2dff, with codecov/project at 98.31% (+0.01%). The two blocks I'd written off as having no deterministic trigger are covered from the other side after all: removal_completion_does_not_depend_on_the_old_path and removal_completion_reports_marker_inspection_errors reach the wait loop's poll and error arms, and test_do_removal_branch_only_keep_skips_deletion plus test_apply_reverts_morph_when_removal_fails reach the picker arms. So the arithmetic I handed over is moot — no coverage decision is outstanding.

test (linux), test (macos), and test (windows) all pass on this head. The merge decision is unaffected by any check; it rests on the review and on the size question above.