Repository navigation
fix(config): reach the marketplace when the plugin is already gone - #4034
Conversation
`handle_claude_uninstall` returned early on `!is_plugin_installed()`, so an uninstall that removed the plugin and then failed on the marketplace could not be finished: re-running printed `Plugin not installed` and exited 0 with the marketplace still configured. The early return now needs both halves gone, and takes only a confident `Some(false)` for the marketplace, matching what `run_plugin_removal` trusts. The plugin step becomes conditional and the `?` preview lists it only when it will run, so the preview stays equal to what the uninstall spawns. The marketplace removal still runs unconditionally, leaving its own tolerance to decide whether an absent marketplace is a failure — which is what keeps the existing tolerance tests meaningful. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
The fix and its test read right, and the reasoning in the body for keeping the marketplace removal unconditional is convincing — gating it would make run_plugin_removal's tolerance unreachable in exactly the tests that exist to exercise it. One doc gap, and one question about the sibling harness.
wt config plugins claude uninstall --help now contradicts the code. src/cli/config.rs:445 promises the opposite of the new behavior:
Skips gracefully if the plugin is not installed, leaving the marketplace in place.
After this PR, plugin-gone-but-marketplace-present is precisely the state the command no longer skips — it removes the marketplace, which is the whole point. Nothing catches this: the string appears in no snapshot and no generated mirror, so test_docs_are_in_sync and the --help snapshots stay green while the sentence describes behavior that's gone. Per CLAUDE.md → Documentation, "does --help still describe what the code does?"
It's outside the diff, so no inline suggestion — the line, with a proposed replacement:
after_long_help = r#"Uninstalls the Worktrunk plugin from Claude Code and removes its marketplace. Skips gracefully only when both are already gone; when one half is left — what a partly-failed uninstall leaves behind — it removes that half. Equivalent to:Happy to push that as a commit if you'd like it.
Does the Codex uninstall have the same stranded state? handle_codex_uninstall in src/commands/config/codex.rs runs codex plugin remove worktrunk@worktrunk through plain run_plugin_cli — unconditionally, with no tolerance wrapper and no Codex counterpart to is_plugin_installed. So if codex plugin remove exits non-zero on an absent plugin the way both harnesses' marketplace remove does (per run_plugin_removal's docstring), a Codex uninstall whose second step failed can't be finished either: the re-run dies at the first step and the marketplace stays. Same shape as the bug here, reached from the other end. I can't determine Codex's exit behavior from CI, so this is a question rather than a claim — if it does exit non-zero, it'd want its own fix rather than being folded in here.
… pages `wt config plugins claude uninstall --help` promised the opposite of what the command now does: "Skips gracefully if the plugin is not installed, leaving the marketplace in place" describes precisely the state it no longer skips. Nothing caught it. The string sits in no generated mirror and no snapshot, so `test_docs_are_in_sync` and the help snapshots both stayed green while the sentence described behavior that was gone. Both uninstall pages are now snapshotted, since what each one says about skipping depends on what the harness's config still holds and is the part most likely to drift again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both taken, in 89df894. The help string. You're right that it promised the opposite, and right that nothing caught it. I wrote my own phrasing rather than the suggestion, keeping the
The "nothing catches this" half seemed worth closing too, so both uninstall pages are now The Codex question — no, it doesn't have the same stranded state, and it's measurable rather than a judgment call. I probed the real CLI earlier in a throwaway
So Same measurement is what established that |
|
Confirmed against 89df894 — both resolved, and the new help wording matches the handler: the early return needs Thanks for the exit-code table — that closes the Codex question in the direction I couldn't measure from CI. One thing that follows from it and holds up on the code: the Codex uninstall page makes no skip claim at all, which is the right page for a command whose both steps tolerate an absent target, so the new snapshot has nothing to drift away from there. |
worktrunk-bot
left a comment
There was a problem hiding this comment.
The help string now matches the guard, and the Codex measurement settles the sibling question cleanly — codex plugin remove exiting 0 on an absent plugin is exactly why that handler has no stranded state to fix, which is a better answer than the symmetry I was guessing at. Snapshotting both uninstall pages closes the "nothing catches this" half.
One optional suggestion inline, on the line this PR rewrites.
It was the only one of the four `config plugins` install/uninstall pages not to, though `handle_claude_uninstall` opens with `require_claude_cli()?` like the rest, so a user without the CLI got the same bail with no warning on the page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Version bump to 0.77.0 plus the changelog section for this release. **Bump level**: minor. `cargo semver-checks` reports 8 breaking library changes (removed `DeprecationInfo::has_deprecated_patterns`, `Repository::recent_commit_subjects`, `Repository::diff_stats_summary`, `config::detect_deprecations`; arity changes on `compute_migrated_content` and `Progress::record`). Pre-1.0, so a breaking change takes a minor bump. `wt list --format=json` defaulting to schema 2 is a user-facing break on its own. **Validation** - `cargo run -- hook pre-merge --yes` — 4831 tests passed, 1 skipped; clippy, fmt, doc-sync, snapshot and lockfile checks green - Nightly dispatched on the cut-from tip (https://github.com/max-sixty/worktrunk/actions/runs/34272636821): full-tests on linux, macos, windows and git-2.43 all green, alongside release-target ×3, nix-flake, minimal-versions, crate-build, unused-deps and link-check **Changelog** 29 entries. Each was verified against its commit's diff by a separate pass, which caught two uncovered fixes (#4033/#4034 uninstall idempotency), one entry in the wrong section (#4018 is a fix, not an improvement), and three unmentioned machine-visible breaks (`wt list` JSON `marker`/`state`, `copy-ignored --dry-run` dropping `files`/`bytes`, the hidden-column footer wording). One correction to the previous release's notes: the `wt config update --output` entry was filed under `## 0.76.0` by #4021, which landed after v0.76.0 was tagged. It moves to 0.77.0, where it actually ships. **Data-loss surface review** Four independent finders swept the cumulative diff. The destructive core is untouched — no changes in `src/commands/process.rs`, `src/git/remove.rs`, `src/utils.rs`, or `src/commands/remove.rs`. This release removes a loss path: #3984 deletes the fork-PR `git branch -D` rollback that force-deleted a pre-existing branch, and its reflog, when the branch name was already taken. Three follow-ups, none blocking: `run_plugin_removal` reporting success over a failed marketplace removal; the documented-not-fixed `post-commit` window in `wt merge`; and `merge_args_into_command`'s `args` precondition, now dead code since `drop_unsupported_keys` removes the key regardless. > _This was written by Claude Code on behalf of max-sixty_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_0134XuJoGCzwoZ21XXbWujdY Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk 0.77.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Release Notes ### Improved - **`wt list` sizes and aligns its columns to the terminal width**: one long branch name no longer sizes the Branch column for every row — it caps at 32 characters and elides, so a narrow terminal keeps its other columns instead of degenerating into a branch list. `Remote⇅` stops holding blank space open in a repo with no remote, and alignment now follows the value type. `--format=json` still carries the whole name. ([#3998](max-sixty/worktrunk#3998)) - **`wt list` names what it hid and marks detached worktrees**: the summary reads `hidden: Path, Commit` instead of counting columns, a detached worktree shows `⊘` rather than borrowing `⚑`, and a prunable row leaves its cells blank instead of showing loading dots that never resolve. ([#3998](max-sixty/worktrunk#3998)) - **`wt list --format=json` gains a `marker` field and stops reporting `detached` twice**: the branch marker set by `wt config state marker` is now readable without parsing it out of `symbols`, and `state` no longer reports a detached worktree as `branch_worktree_mismatch` — the sibling `detached` boolean carries it. ([#3998](max-sixty/worktrunk#3998)) - **`wt list --format=json` now defaults to schema 2**: callers get the envelope with repository metadata and orthogonal per-item facts without configuring `[list] json-schema`. `wt list statusline --format=json` follows the same key. Set `json-schema = 1` to retain the original bare-array format. (Breaking.) ([#4038](max-sixty/worktrunk#4038)) - **`wt config show` fails on a broken config**: it exits 1 on an unreadable or invalid config, an invalid `[list] columns`, or an invalid `approvals.toml`. Unknown keys and deprecations stay warnings and still exit 0. A new `APPROVALS` section counts project commands awaiting approval. (Breaking: it always exited 0 before.) ([#3999](max-sixty/worktrunk#3999)) - **Config migration output names its destination**: `wt config update --output <path>` writes the migration artifact to that file instead of applying it in place, and `--output=-` writes it to stdout. Output mode includes project config when run from a linked worktree. (Breaking: `--print` was removed.) ([#4021](max-sixty/worktrunk#4021)) - **Pi joins the agent integrations**: `wt config plugins pi install` writes an activity hook to `~/.omp/agent/hooks/pre/worktrunk.ts`, so Pi sessions show 🤖/💬 markers in `wt list` like Claude Code, Codex, OpenCode, and Gemini. `$PI_CONFIG_DIR`, `$PI_CODING_AGENT_DIR`, and named `$OMP_PROFILE`/`$PI_PROFILE` profiles are honored. [Docs](https://worktrunk.dev/claude-code/) ([#3594](max-sixty/worktrunk#3594), thanks @adity982, and @ortonomy for the request) - **`wt config plugins codex install` installs the plugin too**: it previously registered the marketplace and left you to run `/plugins` in Codex yourself. Uninstall follows for both Codex and Claude Code, removing the plugin and then the marketplace, so the two commands are inverses. (Breaking: Codex uninstall previously left an installed plugin alone; Claude uninstall now also removes the marketplace.) ([#4019](max-sixty/worktrunk#4019)) - **`wt step copy-ignored` says whether it reflinked or copied**: the summary now reads `(reflinked, no extra disk)`, `(full copy)`, or a partial count, so the same `29.5 GB` line distinguishes a free copy from one that actually wrote the bytes. `--format=json` gains `reflinked` and `written`; a `--dry-run` plan no longer reports `files`/`bytes`. ([#4025](max-sixty/worktrunk#4025)) - **`wt config update` removes keys its destination cannot hold**: migrating `[select]` or `[commit-generation]` drops keys the new section has no field for, reporting each one. Otherwise the key landed at a path the user never typed and warned on every command, with no way to clear it. ([#3994](max-sixty/worktrunk#3994)) ### Fixed - **A failed `wt switch pr:<n>` against a fork no longer deletes a branch it didn't create**: the rollback force-deleted the branch on any setup error, including the one case where the branch wasn't its own — a `pre-switch` hook or a concurrent session claiming the name in the window after the forge answered. Branch and worktree are now created in one `git worktree add -b`, which writes nothing when the name is taken. ([#3984](max-sixty/worktrunk#3984)) - **Codex on Windows: the activity hooks no longer fail on every event**: each hook led with a bare `bash`, which `cmd.exe` resolves to the WSL launcher rather than Git Bash, so every event raised a `Hook failed` banner. Hooks now run `wt.sh` through a shim that finds Git Bash by path. ([#4008](max-sixty/worktrunk#4008), fixes [#4007](max-sixty/worktrunk#4007), thanks @McNultyyy for reporting and diagnosing) - **Templates leave `branch` unset in a detached worktree**: aliases and hooks rendered `{{ branch }}` as the literal `HEAD`, which git resolves, so an unguarded template ran against the wrong ref — the reported case ended in `git push origin --delete HEAD`, which git refused. `base` and `target` follow. (Breaking: an unguarded `{{ branch }}` now errors.) ([#4010](max-sixty/worktrunk#4010), fixes [#4009](max-sixty/worktrunk#4009)) - **The OpenCode plugin loads under OpenCode 2**: OpenCode 2's loader dropped the old bare-function export silently, so activity markers stopped appearing. The plugin now exports one object carrying both the v2 `setup` and v1 `server` entry points, and spawns `wt` through `node:child_process` rather than the Bun shell. Re-run `wt config plugins opencode install` to pick it up; the one file covers OpenCode 2 and OpenCode 1.16 or later. ([#4018](max-sixty/worktrunk#4018), thanks @pragmaticivan for the request) - **`wt step commit --branch` builds its prompt from the worktree being committed**: prompt-building git plumbing ran in the invoking worktree, so with nothing staged there the generated message described none of the changes it committed. `wt step relocate --commit` had the same bug, and `--dry-run`/`--show-prompt` now preview the worktree `--branch` names. ([#3996](max-sixty/worktrunk#3996)) - **LLM commit messages keep diffs for quoted paths**: a file whose name git quotes in the `diff --git` header — any non-ASCII name under the default `core.quotePath` — had its whole diff section dropped from the prompt once the diff exceeded the 100 KB budget. ([#4041](max-sixty/worktrunk#4041)) - **An unparsable user config is reported, not a panic**: `wt config show --format=json` and `--full`, plus `wt step prune`, `relocate`, `eval`, and `for-each`, panicked on a debug build and printed a bare `✗ Command failed` on a release one. They now report `✗ Failed to load config` with the parser's caret diagram. ([#4002](max-sixty/worktrunk#4002)) - **`wt switch --execute` names the directory the program actually starts in**: the `Executing (--execute) @ …` header rendered the worktree the background hooks run in, not the program's own directory — which differs under `--no-cd` or a switch from a subdirectory. The path is omitted when the shell already stands there. ([#4043](max-sixty/worktrunk#4043), fixes [#4042](max-sixty/worktrunk#4042), thanks @yajo for reporting) - **`wt config plugins claude|codex uninstall` is safe to re-run**: removing an already-absent marketplace reported a failure, and a Claude uninstall that removed the plugin and then failed left the marketplace with no way to clear it. Both now finish a half-done uninstall and succeed when there is nothing left to remove. ([#4033](max-sixty/worktrunk#4033), [#4034](max-sixty/worktrunk#4034)) - **`wt config state marker` set and clear no-op outside a git repository**: they exited 1 with a git error, so an agent plugin running outside a repository printed one every turn (the hook's `|| true` kept the session going). ([#3981](max-sixty/worktrunk#3981), thanks @mahirhir) - **`wt list` produces output in a bare repo with no worktrees**: it printed nothing at all and exited 0. It now reports `○ No worktrees` with a hint, `--branches` lists the branches, and `--format json` emits the envelope with an empty `items`. ([#3992](max-sixty/worktrunk#3992)) - **`wt list` warns when it can't parse the project config** rather than ignoring it silently; the listing still succeeds. ([#3993](max-sixty/worktrunk#3993)) - **Relayed command output no longer prints ahead of the delayed-stream progress line**, and delayed-stream output routes through anstream so color is stripped consistently when redirected. ([#4037](max-sixty/worktrunk#4037), [#3995](max-sixty/worktrunk#3995)) - **A hook filter that matches nothing names the unconfigured source**, rather than reporting no match without saying which source was empty. ([#3997](max-sixty/worktrunk#3997)) ### Documentation - **`wt hook` says that `post-*` hooks from the two sources run concurrently**, so dependent commands belong in one source; `wt merge` records that `post-commit` can't run when the merge removes the worktree it anchors on. ([#4020](max-sixty/worktrunk#4020), [#4026](max-sixty/worktrunk#4026)) - **`wt step` help pages describe what the commands actually do**: `wt step prune` says it removes br See merge request: Harmonybrew/homebrew-core!18763
wt config plugins claude uninstallcannot finish a half-done uninstall. It returns early on!is_plugin_installed(), so once the plugin is gone the command stops there — and the state where the plugin is gone but the marketplace is not is exactly what a first run leaves behind when the plugin uninstall succeeds and the marketplace removal then fails. Re-running printsPlugin not installedand exits 0 with the marketplace still configured, and nothing inwtwill remove it.The early return now requires both halves to be gone, and takes only a confident
Some(false)for the marketplace, matching whatrun_plugin_removalis willing to trust. The plugin step becomes conditional, and the?preview lists it only when it will run, so the preview stays equal to the set of commands the uninstall spawns — the propertytest_plugins_claude_prompt_previews_commandspins.The marketplace removal still runs unconditionally. Gating it on the config reading
Some(false)was the first thing I tried, and it is wrong: it makesrun_plugin_removal's tolerance unreachable in the cases that exercise it, sotest_plugins_claude_uninstall_tolerates_absent_marketplaceand its siblings would pass while never attempting the removal they exist to tolerate. Leaving the removal unconditional keeps that tolerance the thing that decides whether an absent marketplace is a failure.test_plugins_claude_uninstall_removes_marketplace_left_without_plugincovers the recovered state: noinstalled_plugins.json, aknown_marketplaces.jsonholding worktrunk, and a run that skips the plugin step and removes the marketplace. Every other uninstall snapshot is unchanged, which is what confirms this reaches only the state that was previously unreachable.wt config plugins claude uninstall --helppromised the opposite of the new behavior — "Skips gracefully if the plugin is not installed, leaving the marketplace in place" is precisely the state the command no longer skips — so it now states the real condition. Nothing caught that: the string is in no generated mirror and no snapshot, sotest_docs_are_in_syncand the help snapshots both stayed green while the sentence described behavior that was gone. Both uninstall pages aretest_helpcases now, since what each says about skipping depends on what the harness's config still holds.The Codex uninstall has no equivalent stranded state, which is measurable rather than a judgment call:
codex plugin removeexits 0 on an absent plugin, so its first step never blocks the re-run from reaching the marketplace step.claude plugin uninstallexits 1 there, which is what made theis_plugin_installedguard necessary on that side and gave it a state to strand.Raised as a non-blocking observation on #4033. It is pre-existing rather than introduced there, but it is in the function that PR was editing.
🤖 Generated with Claude Code