fix(shell): reclaim the legacy wrapper paths instead of inspecting them - #3602
Merged
Conversation
`wt config shell install` decided whether the file at a legacy location was
worktrunk's by reading it — a substring test for the wrapper header, falling back
to "every code line looks like an integration line". Anything that didn't match
was left in place. Fish sources `conf.d` at startup, so whatever
`conf.d/{cmd}.fish` defines is already loaded by the time fish would autoload the
`functions/{cmd}.fish` the install just wrote — the stale definition wins and the
new wrapper may never load.
Ownership is now the path. `conf.d/{cmd}.fish` and the stranded nushell `{cmd}.nu`
candidates are paths worktrunk itself computes for the command name being
installed, so install takes them back whole without reading them. Only that exact
filename is touched: a neighbour under another name is not worktrunk's, and each
removal is reported.
`wt config shell uninstall` still reads the header, because it takes no `--cmd`
and so genuinely doesn't know the name — it lists the shell-owned directories and
has to tell our `{cmd}.fish` from the user's own files beside it. That is the one
place ownership can't come from the path, and it prompts and previews every file
before removing it, which install does neither of.
The two tests that pinned the old conservatism now pin the new boundary: a
headerless file at the reclaimed path goes, a differently-named neighbour stays.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
Collaborator
There was a problem hiding this comment.
Holding this for a human on the deletion surface, not because I found a problem — the diff removes the content guards in front of fs::remove_file in cleanup_legacy_fish_conf_d and cleanup_stranded_nushell, which is exactly the class of change the repo's review policy says an agent shouldn't sign off on. I traced it and the blast radius holds up:
- Bounded to the exact filename. Both cleanups only ever touch a path worktrunk itself computes for the install command —
legacy_fish_conf_d_path(cmd)→conf.d/{cmd}.fish, andNushell.config_paths(cmd)→{autoload_dir}/{cmd}.nu. A neighbour under any other name in the same directory is never a candidate, and the new tests (aliases.fish/helpers.nu) pin that. - Uninstall recognition is unaffected. Dropping
is_worktrunk_managed_nushellis safe — it had no other callers, and uninstall's nushell scan goes throughis_worktrunk_managed_content, whoseWRAPPER_MARKERbranch ("worktrunk shell integration for") already matches the nushell header, so the marker-based recognition it removed is subsumed. - The report path is real. On success each removal surfaces via
output/shell_integration.rsasRemoved … (deprecated; now using …), which is what the newstderr.contains("deprecated")assertion keys on.
The one thing genuinely worth a human's eye is the deliberate tradeoff the PR already names: a user-authored conf.d/{cmd}.fish — named exactly after the command being installed, no worktrunk marker — is now deleted rather than preserved. The shadowing argument for it is sound (a conf.d definition wins over the new functions/ autoload, so leaving it breaks the install), and running wt config shell install for wt is a reasonable point to claim the wt filename. That's a defensible call, just one for the maintainer to own rather than me. No code changes requested.
max-sixty
added a commit
that referenced
this pull request
Jul 27, 2026
…#3608) ## What prompted this Getting #3605 green ran into codecov reporting a `base_commit` three commits older than the real merge-base. This audits whether our config causes that. ## The cause Codecov picks a PR's base by walking back to the newest ancestor that has a coverage report. It used the real merge-base for PRs #3480, #3532 and #3602, and a stale one for #3603 and #3605. The difference is whether the merge-base uploaded a report. **29 of the last 40 main commits did not.** `ci` had one concurrency group for main pushes, and GitHub cancels the *pending* run in a group whenever a newer one joins, even with `cancel-in-progress: false`. So the question is how long a run holds the group, and a run isn't done until its slowest job is: | job | duration on main | |-----|------------------| | `fast-checks` | 2 min | | `code-coverage` | 3-4 min | | `test (windows)` | 11 min | | `collect affected coverage (windows)` | 110-129 min | Each main run held the group for ~2 hours, so nearly every subsequent main push was cancelled while queued, taking the 4-minute coverage job with it. Every cancelled main run's `updated_at` lands within a second of the next push's `created_at`. The 2 hours is real work, not queue: 2-5s from `created_at` to `started_at`, then 108 minutes inside `cargo affected collect` — 4181 tests under `-C instrument-coverage` with a per-test LLVM profile, ~5 GB of profraw. ## The fix: one workflow per cadence The three groups of jobs have incompatible needs, and one group was serving all of them. | workflow | cadence on main | why | |----------|-----------------|-----| | `ci` | every commit, ~11 min | required gate + fast checks | | `coverage` | every commit, keyed per-sha | a skipped upload leaves later PRs on a stale base | | `affected` | sampled, ~2 h | a DB a few commits old still anchors a correct superset | `affected` keeps exactly the grouping it has today, so its sampling is unchanged and deliberate. It just no longer drags the other two along. ### Scope of the impact The posted `codecov/patch` check scopes to the PR's own GitHub diff, so a stale base did **not** score PRs against other people's lines. On #3605 the posted 91.66% is exactly `github.rs`'s 11/12, while the stale-base compare object reported 64/65 across 13 files. What a stale base costs: - `codecov/project` reports "compared to \<stale sha\>" - the patch `auto` target is the stale base's project coverage (0.02pp here) - the compare API object widens to `base..head`, which is what made the investigation look like silence Separately, `test`/`lint`/`fast-checks` also stopped completing on main. Nothing load-bearing rode on that (they already ran on the PR), but it left `tend-ci-fix` with nothing to watch, since it doesn't fire on cancelled runs. ## Two smaller fixes - `ignore: "**/tests/**"` compiles to `.*/tests/.*` (confirmed against codecov's validator), which needs a leading directory and so never matched `tests/` itself. Inert today since `cargo llvm-cov` reports only `src/` (verified against a downloaded `cobertura.xml`), but now correct if that changes. Now `tests/**`. - `fail_ci_if_error` gated on `github.repository_owner`, which is the *base* repo's owner on a fork PR too, so the soft-fail its comment describes never applied. It keys off the head repo now. ## Docs The API behaviour was ours to misuse, not codecov's to explain. Three traps, all confirmed against the live API: - `file_report/<path>/` 404s with `coverage info not found` because the route swallows the trailing slash into the path. Without it the endpoint returns `line_coverage`. - `?pullid=N` always compares the PR's **current** head. `?base=&head=` asks about an earlier commit. - the compare response has no `patch_totals` key, and `.name` is `{base, head}` rather than a string, so a filename lookup silently matches nothing. A working recipe already existed in `running-tend`, but that skill is scoped to CI. `tests/CLAUDE.md` owns coverage investigation, so the queries go there and `running-tend` points at them instead of keeping a second copy. Re-running the corrected query against #3605's failing commit reproduces the miss exactly: `src/git/remote_ref/github.rs:164`, the `gh repo set-default` hint, matching what the session eventually found by hand. ## This PR demonstrates it It changes no Rust at all, only YAML and markdown. Codecov still reported a **10-file, 111-line patch** on its first commit, because it based the comparison on `203603909` rather than the real merge-base `32f380a27`. Every main commit in between has no report: | commit | ci run | report | |--------|--------|--------| | `32f380a27` | queued | no | | `9645e3e13` | cancelled | no | | `bcd1ffdfd` | cancelled | no | | `8865f20ab` | cancelled | no | Every one of those 111 patch lines belongs to somebody else's merged commit. It passed at 100% only because those commits are well covered. > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
Merged
max-sixty
added a commit
that referenced
this pull request
Jul 29, 2026
Cuts 0.70.0 — version bump plus the changelog for the 45 commits since v0.69.2. **Minor bump, not patch.** `cargo semver-checks` reports 6 breaking library changes (`set_command_timeout` and `ListConfig::task_timeout_ms` removed, `WorkingTree::stage` arity changed, three enum variants added to exhaustive enums). Worktrunk ships breaking library changes freely, but semver still puts a break at minor while pre-1.0. ## Release validation - Local gate green on the release commit: 4631 tests, lints, doctests. - `nightly` dispatched on the cut-from tip (`a27cbd42`) for the full cross-platform suite — `full-tests` green on linux, macOS, and Windows, plus minimal-versions, nix-flake, crate-build, and the release targets. ## Data-loss surface review The cumulative diff was audited against the deletion surface. One deliberate widening, signed off for this release with follow-ups to file: - **#3602** removes the content check from `wt config shell install`'s legacy cleanup, so `conf.d/{cmd}.fish` and the stranded nushell `{cmd}.nu` are now deleted by path, unread. Only that exact filename is touched and each removal is reported, but the deletion is absent from both `--dry-run` and the confirmation prompt, and the already-configured path skips the prompt entirely. Also noted, none blocking: - The `!path.exists()` check precedes the lock guard on the `Path`/`Current` removal arm, so a locked worktree whose directory is absent loses its branch. This already governed the branch-targeted route in v0.69.2; #3533 unified the other arms onto it. The FAQ's "Neither `git worktree remove` nor `wt remove` (even with `--force`) will delete them" is absolute where the behavior isn't. - On the default background removal path, `ensure_clean` and `stop_fsmonitor_daemon` swapped order, so the safety gate is now answered by the live fsmonitor daemon rather than a full re-stat. Bounded by trash staging with 24-hour retention, and the foreground and picker paths were already daemon-served. Net *improvements* to the same surface: shared-branch retention across remove/prune/merge (#3533), outcome-accurate removal reporting (#3633, #3637), and the removal of the thread-local command timeout that could kill in-flight git commands on worker threads (#3615). ## Changelog accuracy Entries were verified against the actual diffs rather than commit messages, which corrected several drafts: the prune figures were one PR stale (~2.9 s → the real ~0.6 s), `wt merge` takes no worktree argument so it only gained the retention half of #3533, the Azure DevOps report is behind `--full`, #3608 never touched `nightly.yaml`, and #3601 inverted what the FAQ change actually said. Two omissions were added — the `install-statusline` foreign-statusline fix (#3595) and the shipped `/wt-switch-create` skill change (#3636). > _This was written by Claude Code on behalf of Maximilian_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wt config shell installdecided whether the file at a legacy location was worktrunk's by reading it — a substring test for the wrapper header, falling back to "every code line looks like an integration line". Anything that didn't match was left in place. Fish sourcesconf.dat startup, so whateverconf.d/{cmd}.fishdefines is already loaded by the time fish would autoload thefunctions/{cmd}.fishthe install just wrote — the stale definition wins and the new wrapper may never load.Ownership is now the path.
conf.d/{cmd}.fishand the stranded nushell{cmd}.nucandidates are paths worktrunk itself computes for the command name being installed, so install takes them back whole without reading them. Only that exact filename is touched: a neighbour under another name is not worktrunk's, and each removal is still reported.wt config shell uninstallstill reads the header, because it takes no--cmdand so genuinely doesn't know the name — it lists the shell-owned directories and has to tell our{cmd}.fishfrom the user's own files beside it. That is the one place ownership can't come from the path, and it prompts and previews every file before removing it, which install does neither of.is_worktrunk_managed_content's docstring now says so;is_worktrunk_managed_nushellexisted only to gate the install-time deletion and is gone.Audited the rest of the surface for the same principle: the OpenCode plugin (
~/.config/opencode/plugins/worktrunk.ts) is already removed by path with no inspection, the Claude plugin delegates toclaude plugin uninstall, Gemini installs throughgemini extensions installand leaves nothing of ours on disk, and fish is the only shell with a separate completion file — which uninstall already covers. No other gaps.Data safety
This deliberately widens what install deletes, so it's worth naming: two tests asserted the old conservatism and now assert the new boundary —
test_configure_shell_fish_reclaims_conf_d_path(was..._preserves_user_conf_d_file, from #3589) andtest_nushell_install_reclaims_only_the_command_name(was..._keeps_unmanaged_legacy_file, from #2992). Both put a neighbour file in the same directory to pin that only{cmd}.{ext}is taken. The FAQ's "What can Worktrunk delete?" inventory is updated to describe path-based ownership rather than the old content-marker rule.