Skip to content

fix(git): anchor Repository::current_worktree() to discovery path - #2652

Merged
max-sixty merged 2 commits into
mainfrom
cwd-audit
May 9, 2026
Merged

max-sixty merged 2 commits into
mainfrom
cwd-audit

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

Summary

Repository::current_worktree() resolved against the process-global base_path() (CWD or -C), not against the Repository's own discovery_path. The two coincide for Repository::current() (the common case), but Repository::at(p) callers — output handlers, picker, run_pipeline, tests, recovery — silently got a WorkingTree at the process CWD instead of at p.

PR #2625 patched one symptom inline (infer_default_branch_locally was calling current_worktree().is_linked() and reaching for the wrong path). This change fixes the helper itself, which subsumes that workaround and the sibling sites identified in the audit (Pass B from the structural audit done in PR #2649's session): project_config_path, project_config, require_current_branch, resolve_worktree_name("@"), resolve_worktree("@").

Why structural over per-site

Five sibling sites use self.current_worktree() with the same intent: "operate on the worktree this Repository represents." Patching each individually would leave the helper as a footgun — the next sibling added would re-introduce the bug. Fixing current_worktree() itself matches what its name has always promised. Audited production callers; none rely on the old "use process CWD even though self was constructed elsewhere" semantics.

What's in this diff

  • src/git/repository/mod.rs: current_worktree() → worktree_at(self.discovery_path.clone()). Docstring explains the dual semantics.
  • src/git/repository/config.rs: revert PR fix(git): tolerate non-git paths in WorkingTree::is_linked #2625's per-method workaround (now redundant).
  • src/git/repository/working_tree.rs: update WorkingTree::path docstring that referenced base_path().
  • src/git/repository/tests.rs: regression test pins both directions — matches discovery_path(), not process CWD.
  • src/commands/worktree/switch.rs: gate the source-worktree capture on !is_recovered. Without this, current_worktree() in recovery mode would resolve to the recovered ancestor (typically the main worktree) and expand {{ base }} / {{ base_worktree_path }} to main's identity instead of empty strings. Mirrors the existing !is_recovered gate on pre-switch hooks at line 1264. Caught by code review.
  • tests/integration_tests/repository.rs: remove three stale "use worktree_at, not current_worktree() which uses base_path()" comments.

Test plan

🤖 Generated with Claude Code

`Repository::current_worktree()` resolved against the process-global
`base_path()` (CWD or `-C`) instead of the Repository's own
`discovery_path`. The two coincide for `Repository::current()`, but
`Repository::at(p)` callers (output handlers, picker, run_pipeline,
tests, recovery) silently got a WorkingTree at the process CWD instead
of `p`. PR #2625 patched one symptom of this — `infer_default_branch_locally`
calling `current_worktree().is_linked()` reaching for the wrong path —
with an inline `worktree_at(self.discovery_path().to_path_buf())`. This
change fixes the helper itself, which subsumes that workaround and the
sibling sites identified in the audit (`project_config_path`,
`project_config`, `require_current_branch`, `resolve_worktree_name("@")`,
`resolve_worktree("@")`).

Recovery preserves prior behavior: `current_or_recover()` returns a
Repository whose `discovery_path` is the recovered ancestor, so without
gating, post-switch hooks would expand `{{ base }}` to main's branch
instead of empty. `run_switch` now skips the source capture when
`is_recovered`, mirroring the existing `!is_recovered` gate on
pre-switch hooks.

Adds a regression test that pins both directions: `current_worktree()`
matches `discovery_path()`, and does NOT match the process CWD when
they differ.

Co-authored-by: Claude <noreply@anthropic.com>
worktrunk-bot
worktrunk-bot previously approved these changes May 9, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structural fix is well-motivated — the docstring update on current_worktree() makes the dual semantics explicit, and the regression test pins down both directions (matches discovery path, not process CWD). The is_recovered gate in switch.rs is the right call: before this PR the empty source_branch/source_path came from current_worktree().root() failing on the deleted CWD; after the fix, those calls would resolve to the recovered ancestor and silently report main's identity as the user's "base".

One stale comment outside the diff worth a follow-up touch:

src/commands/worktree/switch.rs:1355-1356

// When recovered from a deleted worktree, current_dir() and current_worktree().root()
// both fail — fall back to repo_path() (the main worktree root).

After this PR's structural fix, current_worktree().root() no longer fails in recovery — it resolves to the recovered ancestor (the main worktree, since recover_from_path only returns repos with a .git directory), which is the same path repo_path() returns. Behavior is preserved, but the comment now misleads. Suggested rewording:

// When recovered from a deleted worktree, current_dir() fails — fall back to
// repo_path() (the main worktree root).

Happy to push the touch-up if you'd like.

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI on linux/macos/windows is failing for a real regression introduced by this PR — the new docstring on current_worktree() adds an intra-doc link to [`Repository::at(p)`], which rustdoc can't resolve (the (p) argument list isn't valid in a path expression). With RUSTDOCFLAGS='-Dwarnings', this fails the doc step in wt hook pre-merge:

error: unresolved link to `Repository::at(p)`
   --> src/git/repository/mod.rs:898:24
    |
898 |     /// meaning. For [`Repository::at(p)`] the discovery path is `p`, and this
    |                        ^^^^^^^^^^^^^^^^^ the struct `Repository` has no field or associated item named `at(p)`

Mechanical fix — drop the (p) from the link target (the p is still introduced as a code-span on the same line). Inline suggestion below.


codecov/patch also flags the is_recovered = true arm at src/commands/worktree/switch.rs:1302 ((String::new(), String::new())) as uncovered. The test_remove_current_worktree_fires_post_switch_hook test exercises recovery but the implicit switch-back doesn't appear to flow through run_switch's new branch, so the empty-string contract isn't pinned down. A targeted test asserting that {{ base }} and {{ base_worktree_path }} expand to empty strings in a recovered switch would both close the codecov gap and serve as a regression guard for the gate. Happy to push both fixes if you'd prefer not to do them yourself.

Comment thread src/git/repository/mod.rs Outdated
@worktrunk-bot
worktrunk-bot dismissed their stale review May 9, 2026 16:46

CI failed — rustdoc broken intra-doc link in new docstring (and codecov gap on the is_recovered branch). See follow-up review.

- Drop `(p)` from `[Repository::at(p)]` intra-doc link target — rustdoc
  can't resolve the parenthesised form, breaking `cargo doc -Dwarnings`
  on linux/macos/windows.
- Update the stale `current_dir() and current_worktree().root() both
  fail` comment near the `source_root` fallback. After this PR's fix,
  `current_worktree().root()` resolves against the recovered ancestor
  and succeeds; only `current_dir()` still fails on a deleted CWD.
- Extract the source-capture branch in `run_switch` into
  `capture_switch_source(repo, is_recovered)`, with unit tests covering
  both branches. Keeps codecov/patch green on the recovered-arm return.

Co-authored-by: Claude <noreply@anthropic.com>
@max-sixty
max-sixty merged commit 881b14d into main May 9, 2026
34 checks passed
@max-sixty
max-sixty deleted the cwd-audit branch May 9, 2026 17:01
@max-sixty max-sixty mentioned this pull request May 10, 2026
3 tasks done
max-sixty added a commit that referenced this pull request May 10, 2026
Bump worktrunk to **0.49.0** — minor bump (semver-checks reports a
breaking change in `GitError::UncommittedChanges`, which gained a
`dirty_files: Vec<String>` field).

## Highlights

**Improved**
- New `codename(n)` template filter — deterministic friendly worktree
names from a ~1.26M-combo pool (`{{ branch | codename(2) }}` →
`malleable-opah`).
[#2641](#2641), thanks
@endigma
- Picker preview disk cache for Log / BranchDiff / UpstreamDiff, with
background refresh for stale Log decorations.
[#2628](#2628),
[#2646](#2646)
- Picker shares one `default_branch_sha` lookup across BranchDiff items
instead of forking N rev-parse subprocesses.
[#2658](#2658)
- "X has uncommitted changes" errors now list the dirty files inline.
[#2653](#2653)

**Fixed**
- `wt switch` integrates with `cd` aliases like zoxide (uses `builtin
cd`). [#2644](#2644), thanks
@xkumiyu for reporting
- Empty hook tables no longer panic during `wt switch --create`.
[#2635](#2635), thanks @topit
for reporting
- `Repository::current_worktree()` and five sibling helpers no longer
silently leak the process CWD when called via `Repository::at(p)`.
[#2652](#2652)
- `WorkingTree::is_linked` tolerates non-git CWDs (e.g. Nix sandbox).
[#2625](#2625), thanks
@DArtagan for reporting
- Picker no longer overlays `collect` warnings on the active TUI; clears
its frame on exit in inline mode.
[#2627](#2627),
[#2626](#2626)
- Failed `git log` no longer poisons the picker's preview disk cache.
[#2651](#2651)
- `wt step prune` says "removing branch" for branch-only candidates.
[#2619](#2619)
- Claude Code Windows integration uses a `wt.sh` wrapper to avoid
colliding with Windows Terminal's `wt.exe`.
[#1754](#1754), thanks
@lucaspimentel

**Documentation**
- cmux workspace integration recipe.
[#1907](#1907), thanks
@alvistar

See
[CHANGELOG.md](https://github.com/max-sixty/worktrunk/blob/release/CHANGELOG.md)
for the full list including internal CI / test-isolation changes.

## Test plan

- [x] `cargo run -- hook pre-merge --yes` (tests + lints clean)
- [x] `cargo semver-checks check-release -p worktrunk` (one breaking
change confirmed → minor bump)
- [x] CHANGELOG entries verified by subagent against actual diffs

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants