mirror of
https://github.com/max-sixty/worktrunk.git
synced 2026-09-14 20:00:38 +08:00
codex/remove-codex-cloud-specific-tests
2388 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Maximilian Roos
|
0d3ce4b14c | chore: use native Codex Cloud environment | ||
|
Worktrunk Bot
|
aa4e527f35 |
refactor(git): drop Branch::push_remote, unused since the @{push} fix (#3833)
Nightly sweep finding. `Branch::push_remote()` has had no callers — production or test — since [#769](https://github.com/max-sixty/worktrunk/pull/769) (Jan 2026), which replaced its only caller in the CI-status path with `push_remote_url()`. This drops it, and fixes a stale doc comment that named it. ## Why it went unnoticed It's `pub` on a public lib type, so rustc's `dead_code` lint never fires on it. `git log -S` puts the last caller's removal in `c4f1c0730` (#769): `gh pr checkout` sets `branch.<name>.pushremote` to a URL rather than a remote name, and `@{push}` — which is what `push_remote()` resolves through — fails in that case. `push_remote_url()` uses `%(push:remotename)` instead, which handles both. So the method isn't merely unused; its docstring still advertises the `@{push}` resolution chain that #769 established is wrong for the case the codebase actually hits, which makes it read as a live alternative to the function that superseded it. ## The stale comment `setup_push_tracking` in `tests/integration_tests/default_branch.rs` was documented as existing so `branch.push_remote()` and `github_push_url()` work. The first is what this PR deletes; the second has never existed anywhere in the tree (`grep` finds that comment as its only occurrence). Its four call sites all call `push_remote_url()`, so the comment now names that. ## Verification No regression test accompanies this — there's no behavior to pin, since the deleted method had no callers to change the behavior of. The proof is negative and the compiler carries it: `cargo build --all-targets` and `cargo clippy --all-targets -- -D warnings` both pass, which they could not if any call site remained. Also ran the suites covering the touched area: `cargo test --test integration default_branch` (63 passed) and `cargo test --lib git::repository` (178 passed). <details><summary>Confirming there are no callers</summary> Every mention of the bare identifier in the tree before this change: ``` src/git/repository/branch.rs:151: pub fn push_remote(&self) -> Option<String> { # the definition src/git/repository/branch.rs:186: let push_remote = self # local var in push_remote_url src/git/repository/branch.rs:197: if push_remote.contains("://") ... # same local src/git/repository/branch.rs:198: Some(push_remote) # same local src/git/repository/branch.rs:200: self.repo.effective_remote_url(...) # same local tests/integration_tests/default_branch.rs:439: /// ... `branch.push_remote()` ... # the stale comment ``` The lines in `push_remote_url` are a local binding of the same name, not calls. `switch.rs:808` writes the `branch.<name>.pushRemote` git-config key and is unrelated. </details> --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Maximilian Roos
|
18b408ce98 |
Add shared Codex Cloud environment setup (#3810)
## Summary - add a repository-owned Codex Cloud Taskfile, exposed through root setup and maintenance tasks - share setup and maintenance preparation in one task instead of two scripts - document concise, checksum-gated environment commands - preserve the proven UID 1000, `tini`, pinned-tool, and retry behavior - keep tool pins, archive checksums, Task version, and launcher digests synchronized by test and maintenance guidance ## Why Worktrunk's full suite needs dependencies and process/permission semantics beyond the stock universal image. The working configuration previously lived only in one saved environment, where other contributors could neither review nor reuse it. The dedicated Taskfile sits beside the project Taskfile without making unrelated task edits invalidate the Cloud environment hash. Root wrapper tasks launch it as a new Task process so its repository-relative paths retain their own Taskfile context. ## Security Codex checks out the task branch before setup or maintenance. Each saved launcher verifies the dedicated Taskfile's fixed SHA-256 digest before executing it as root. `MISE_NO_CONFIG=1` also prevents branch-controlled mise configuration from running before the verified Taskfile. Approved changes require updating the digest in environment settings, which invalidates the cache. Repository-sensitive Rustup, pre-commit, and Cargo work runs as the image's UID 1000 `ubuntu` user; root is limited to the verified system and ownership preparation. ## Validation - root and direct Taskfile discovery expose `setup-codex` and `maintain-codex` - YAML parsing and extracted Bash syntax pass - warning-level ShellCheck passes - applicable pre-commit hooks pass - positive and negative checksum checks pass - the launcher-sync integration test passes - independent adversarial, abstraction-level, and current-head code reviews are clean - exact Taskfile validation passed in Codex Cloud task `task_e_6a7e53a87e908325bf50ea3413ed521c` - setup and maintenance launchers passed - root and direct Task discovery passed - Cargo identity probe returned UID 1000 - `cargo run -- hook pre-merge --yes` exited 0 - 4,601/4,601 tests passed; all gate components passed - HEAD remained `288953dcb18466f64b8e5355192ae297f4862240` and the checkout remained clean - final-head Cloud task `task_e_6a8089197ecc8325afd723d41beb5c50` reached `READY` with no diff after about 38 minutes on `dab4a5dcd5f343c3e5ea0a1183e9fcc5d8271a08`; its transcript was unavailable, so no finer-grained result is claimed - all 18 applicable final-head checks pass on Linux, macOS, and Windows, including coverage, `codecov/patch`, and current-head tend review > _This was written by Codex on behalf of @max-sixty_ |
||
|
Worktrunk Bot
|
246c6bd919 |
fix(list): keep [list] columns out of the --format json plan (#3812)
Closes the `[list] columns` half of #3787, per the call in [this
comment](https://github.com/max-sixty/worktrunk/issues/3787#issuecomment-5273942067):
JSON always emits the same shape, and `list.columns` only affects the
actual columns.
Before, `--format json` planned `all_columns` (source `Default`)
*unioned* with the selection's forced-on columns, so the selection
reached JSON in one direction only — it couldn't narrow the emitted
fields, but a listed `ci` did force the forge fetch on without `--full`.
That made a presentation setting decide whether a machine-readable call
talks to GitHub, which is the thing the Neovim plugin in #3787 had to
pin `--config-set 'list.columns=[…]'` against. Now the JSON branch plans
`all_columns` alone; `--full` is the only switch for the gated data, and
it's the one a caller controls.
The table and the `wt switch` picker are untouched — a listed `ci` still
renders the CI column without `--full`, and the picker still unions the
selection in so its table matches `wt list`'s.
Only `ci` and `summary` are affected: every other column is ungated, so
`full_plan()` already covered them, and custom columns require no
background task.
**For the release note — this changes schema 1 too.** A caller with
`[list] columns = […, "ci"]` and no `--full` used to get the `ci` object
in schema-1 JSON and now won't; schema 1 has no `collected` envelope to
say why. The schema-1 `ci` row already documented `` `--full` only ``,
so the docs get *more* accurate, but the observable output changes for
anyone who was relying on the forcing path. Schema 2 reports the same
narrowing through `collected.ci`.
Docs updated in `after_long_help` (the `[list] columns` section plus the
schema-2 `pr`, `summary`, and `checks` rows — `summary` now names
`--full` alongside `[list] summary = true`, and `checks` names the
`--full` gate it shares with `pr`), with the generated mirrors,
`dev/config.example.toml`, and the `--help` snapshots regenerated. The
`CLAUDE.md` network inventory and the `collect` planning comment now
record the exemption too.
<details><summary>Test</summary>
`test_list_json_columns_selection_does_not_force_ci` in
`tests/integration_tests/list_config.rs` asserts schema 2's
`collected.ci` across three configs: unset (false), `columns =
["branch", "ci"]` without `--full` (false — the regression this fixes),
and the same with `--full` (true). `collected` records what the plan
requested rather than what a fetch returned, so the test needs no forge
and no `gh` on PATH. It sits next to
`test_list_json_ignores_columns_selection`, which owns the narrowing
direction, and `test_list_config_listed_column_overrides_full_gate`,
which owns the table's forcing behaviour and still passes unchanged.
Ran locally: full `cargo test --test integration` and `cargo test --lib
--bins`, plus `cargo clippy --all-targets` and `cargo fmt --check`. One
unrelated failure,
`test_copy_ignored_preserves_file_executable_permissions`, is a umask
artifact of this sandbox (expects `0644`, the runner's `umask 002`
produces `0664`); it touches no code in this diff.
The docs-row follow-up in
|
||
|
Maximilian Roos
|
92dfb686bb |
feat(approvals): let wt config approvals add --yes record approvals without a TTY (#3819)
`wt config approvals add` refused every non-interactive run — even with `--yes`, whose hint then suggested the flag already passed — so there was no way to pre-approve a project's commands unattended. An orchestrator (tend's Codex Cloud container was the motivating case) had to hand-write `approvals.toml` from `wt config approvals list --format=json` output, a third-party reimplementation of `add` that breaks whenever the schema changes. The `wt config approvals` docs already promised "`--yes` to bypass prompts in CI" and described `stale` entries as "what `--yes` would silently re-approve"; behavior now matches them. The two `--yes` meanings stay distinct: on a command that runs project commands it grants consent for that run alone and records nothing (unchanged), while on `add` — whose product is the record — it lists what it trusts and writes it. `add` no longer routes through `approve_command_batch` (the execution gate) for this: it prompts or announces, then saves itself, which also makes a failed `approvals.toml` write fail the command instead of warning behind a `✓ saved` line and exit 0 — an orchestrator reading only the exit code would otherwise walk into the prompt it just paid to avoid. The non-interactive hint's pre-approval suggestion now carries `--yes` (`run wt config approvals add --yes`), since a hint reached in CI must name a command that runs there. Per the existing `list --format=json` docs, `add --yes` re-approves templates edited since an earlier approval without comment; the `add` help now says so and points at the `stale` field for reading them first, and the worktrunk skill's escalation rule tells agents not to reach for it on a user's behalf. > _This was written by Claude Code on behalf of max-sixty_ |
||
|
Caleb Cox
|
aa9d8c43df |
feat: add remote_repo variable (#3745)
Add a `remote_repo` variable that returns the repo name from the remote URL. Unlike `repo`, it stays consistent even if the clone was renamed. Feel free to reject, or suggest other names for the variable. But this change would improve my workflow. I hope you don't mind my submitting a PR before opening an issue. Thanks for an amazing developer tool! AI Disclosure 🤖: I used Claude Code to generate the changes, but reviewed every line and made adjustments. --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Worktrunk Bot
|
96c6c846f7 |
fix(shell): register completions under the --cmd name, not clap's (#3817)
## Problem
`wt config shell init <shell> --cmd <name>` renames the shell wrapper
and its lazy completion loader, but the registration that loader evals
comes from clap, which derives every identifier in it from its own
compile-time `Command` name (`wt`) — not from `argv[0]` and not from
`--cmd`. The two halves never agreed:
```console
$ wt config shell init zsh --cmd wot | grep _clap
if ! (( $+functions[_clap_dynamic_completer_wot] )); then
_clap_dynamic_completer_wot "$@"
$ COMPLETE=zsh wt | grep -oE '_clap_dynamic_completer_[a-z_]*' | sort -u
_clap_dynamic_completer_wt
```
Nothing completed, and because the guard never became true the
completion script was regenerated and re-evaluated on *every* TAB. Same
shape in bash (`_clap_complete_*`); PowerShell emitted
`Register-ArgumentCompleter -Native -CommandName wt`, so the `--cmd`
name was never registered at all. The documented `--cmd=git-wt` case
(the Windows Terminal conflict) was broken too — including for a binary
genuinely installed under that name, since clap's name comes from the
declaration rather than `argv[0]`.
There is a second, sharper edge: zsh's registration ends with `compdef
<completer> <cmd>`, so the first TAB on `wot` also bound worktrunk's
completer to plain `wt` — handing completions to the *other* `wt` that
`--cmd` exists to step around.
fish and nushell were unaffected. Both register a completer that shells
out to the binary rather than depending on a clap-emitted identifier, so
the reporter's "unverified" row for fish is a pass.
## Solution
The bash, zsh, and PowerShell loaders now pass the name they bind in
`WORKTRUNK_COMPLETE_NAME`, and `registration_name()` in
`src/completion.rs` emits the registration under that name (validated
through the same `validate_shell_command_name` guard `--cmd` uses, since
the value lands verbatim in generated shell code). The fallback is
`binary_name()`, which covers a binary installed as `git-wt` and invoked
directly. The templates apply clap's own `-` → `_` escaping to the
function they call, so `--cmd git-wt` guards on `_clap_complete_git_wt`
rather than the invalid `_clap_complete_git-wt`.
That fixes all four shells and the stray `compdef` in one place, rather
than pinning the templates to clap's internal naming:
```console
$ WORKTRUNK_COMPLETE_NAME=wot COMPLETE=zsh wt | grep -oE '_clap_dynamic_completer_[a-z_]*|compdef .*' | sort -u
_clap_dynamic_completer_wot
compdef _clap_dynamic_completer_wot wot
```
## Testing
Two reproduction tests in `tests/integration_tests/completion.rs`, both
failing before the change:
- `test_init_custom_cmd_defines_clap_completer_in_bash` drives the whole
chain through a real bash — generate the init script with `--cmd`, call
the loader it defines, then assert clap's completer function exists
afterwards. Printed `MISSING` before, `DEFINED` after. Cases for `wot`
and `git-wt`.
- `test_completion_registration_uses_shell_integration_cmd_name` covers
zsh and PowerShell, which CI can't drive: the identifier the init script
references must be the one the registration defines, and the `compdef` /
`-CommandName` target must be the `--cmd` name.
`cargo test --lib --bins` and `cargo test --test integration` are
otherwise green (one unrelated failure locally,
`test_copy_ignored_preserves_file_executable_permissions`, from this
sandbox's `umask 0002`), and `cargo clippy --all-targets --all-features`
/ `cargo fmt --check` are clean.
---
Closes #3816 — automated triage
---------
Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
|
||
|
Worktrunk Bot
|
bdce107d91 |
fix(config): rank env vars and --config-set above project entries (#3790)
Fixes #3788. Layer and specificity were separate steps. `load_with_warnings` flattened system config → user config → `WORKTRUNK_*` env vars → `--config-set` into one document, and the accessors then resolved specificity on that document, so a `[projects."<id>"]` entry answered for the global key of the same name whichever layer set it. `WORKTRUNK_WORKTREE_PATH` could therefore not override a project's `worktree-path`, and a global `--config-set` hit the same wall. Per @max-sixty in the issue thread — "env vars should indeed take precedence over the user project config, we should fix this throughout" — the two invocation layers now cross the axes: they're typed for one run, so they outrank a project entry as well as the global key. Load applies them at both scopes (`apply_invocation_layer_over_projects`, the last step before `finalize`): whatever the layer set is dropped from every project entry, leaving the global key it also set to answer for it. Two kinds of key are held back: - **Keys the layer restates under `projects."<name>"`** — `--config-set 'projects."github.com/owner/repo".worktree-path = …'` is both the highest layer *and* the most specific key, so it still wins over the same layer's global key. - **Composing keys** — hooks, aliases, and `step.copy-ignored.exclude` — whose project-scoped values append to the global ones rather than replacing them. Both already apply, so an env-set hook was never outranked, and dropping the project's copy would silently stop it running. Hook names come from `HooksConfig`'s schema, so a new hook can't be forgotten. Two sections have to go as a unit rather than leaf by leaf. `[commit.generation]`'s mutually exclusive pairs: `template` and `template-file` clear one another in `merge_with` *and* are rejected together by `validate`, so overriding either has to displace both at project scope — otherwise the project's partner would still win the merge. `exclusive_sibling` names those pairs. And `[list.custom-columns]`, which `ListConfig::merge_with` extends per whole column, so a partial removal leaves the project's column replacing the global one anyway — and `ListColumnConfig::template` is required, so it can also strand a column that no longer deserializes. `is_atomic_section` names that table. Both are enumerations, so the pass degrades as a unit behind them: the removals land on a candidate, kept only if it still deserializes and validates. That is the guarantee the env and `--config-set` layers already have, and without it the next required field would answer a stranded leaf with `UserConfig::default()` — costing the user their whole config for that invocation rather than one project entry's precedence. The precedence table now reads: | Source of `worktree-path` | Loses to | |---|---| | `--config-set 'worktree-path = …'` | — | | `WORKTRUNK_WORKTREE_PATH` | `--config-set` | | `[projects."github.com/owner/repo"]` in a config file | either invocation layer | | global `worktree-path` in a config file | all of the above | ## Docs The help text had no precedence section at all — the gap that made this read as a bug — so this adds one under **Environment variables**, plus a pointer from **User project-specific settings**. That supersedes #3789, which documented the old behavior; I'll close it in favour of this. ## Testing Nine unit tests in `src/config/user/tests.rs` cover the table-level rule (both layers, pattern entries, restated project-scoped overrides, untouched sibling keys, composing keys, the exclusive pair, the atomic custom column, a rolled-back layer, and the no-override no-op), and `test_switch_create_invocation_layers_outrank_project_worktree_path` proves it end-to-end — a real process is the only thing that reads `WORKTRUNK_WORKTREE_PATH` off the environment. That test keeps a control showing the project entry still beats the config file's own global key, so it can't pass by project entries having stopped applying. The reproduction from the issue now lands where it says it should: ```console $ WORKTRUNK_CONFIG_PATH="$tmp/wt.toml" WORKTRUNK_WORKTREE_PATH="$tmp/from-environment" \ wt switch --create feature --no-cd --no-hooks --yes --format=json {"action":"created","branch":"feature","path":"/tmp/tmp.jQiTAuNPFd/from-environment",…} ``` <details><summary>Local suite</summary> `cargo test --lib --bins` and `cargo test --test integration` are green apart from `test_copy_ignored_preserves_file_executable_permissions`, which fails in this sandbox because its umask is `0002` (file created `0664`, test expects `0644`) — unrelated to this change and not reproducible on a `0022` runner. `cargo fmt --check` and `cargo clippy --all-targets --all-features` are clean. </details> --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Maximilian Roos <m@maxroos.com> |
||
|
Maximilian Roos
|
7aba380f0c |
fix(remove): gate removal on the registration, not the repository (#3808)
`wt remove --force` deleted a live worktree of this same repository, uncommitted work included, whenever that worktree had been moved onto another worktree's registered path. The guard added in #3785 asks which *repository* the occupant answers to: a linked worktree's git dir sits under `<common>/worktrees/`, the main worktree's *is* the common dir, anything else is someone else's. A sibling worktree moved onto the path satisfies that — its git dir sits under `<common>/worktrees/` like any worktree of this repo — so it passed, and the fast path renamed the directory into trash and handed the `rm -rf` to a detached process. It is not prunable either: its gitdir file points at a location that exists, so the `is_prunable` arm from the same PR doesn't catch it. Git's own validation is one level finer. `validate_worktree` requires the directory to point back at *this registration*, and refuses this removal with `--force`: ```console $ git -C repo worktree remove --force ../repo.feature fatal: validation failed, cannot remove working tree: '.../repo.feature' does not point back to '.git/worktrees/repo.feature' ``` <details> <summary>Reproducer, verified against a build of main</summary> The occupant has to be *moved* onto the path rather than created there — `git worktree add` refuses a registered path, which is what leaves a plain `mv` as the way this state arises. ```console $ git -C repo worktree add ../repo.feature -b feature $ git -C repo worktree add ../repo.bar -b bar $ rm -rf ../repo.feature && mv ../repo.bar ../repo.feature $ echo PRECIOUS > ../repo.feature/precious.txt $ wt remove --force --yes feature ◎ Removing feature worktree (--force) & branch in background (same commit as main, _) $ ls ../repo.feature ls: ../repo.feature: No such file or directory ``` </details> ## The fix The gate is now git's comparison at git's granularity: the directory's `.git` must name *this* registration, and that registration's `gitdir` file must name the directory back. Repository-level ownership stays as the weaker half of the conjunction — it is what rejects a `.git` file pointing at another repository — and the main worktree is the same test where there is no registration to point back at. `ensure_belongs_to_repo` becomes `ensure_holds_this_worktree`, since it no longer merely asks about repository membership. Resolution moves to `Repository::git_dir_at`, the fs-only resolver the `wt list` prewarm already used (`derive_worktree_git_dir`), generalized to answer for a directory rather than for a known worktree of this repo: its main-worktree branch returned `git_common_dir()` on trust, and now canonicalizes the `.git` it actually found. It also never walks up to a parent, which is what git reads too — `git rev-parse --git-dir` in an emptied worktree can resolve the *enclosing* repository. That settles a second thing the old docstring got wrong. It claimed the plan→rename window was "narrower than `ensure_clean`'s"; in fact `ensure_clean` re-runs `git status` while this gate answered from `GIT_DIRS`, memoized process-wide, so the second call was vacuous and the window — which contains the approval prompt and the `pre-remove` hook — was unguarded. `git_dir_at` reads the filesystem on every call, so the check at the rename now re-decides. The refusal was `Directory @ … is not this repository's worktree`, which is false in the sibling case: it *is* one of this repository's worktrees, just not the one registered there. Its hint didn't fit either — "move the directory aside, then run `git worktree prune`" is a repo-wide prune, and with a sibling moved aside *both* registrations are prunable, so following it clears both and leaves a live checkout that has stopped being a worktree: ```console $ mv ../repo.feature ../repo.aside && git worktree prune -v Removing worktrees/repo.feature: gitdir file points to non-existent location Removing worktrees/repo.other: gitdir file points to non-existent location $ git -C ../repo.aside status fatal: not a git repository: (null) ``` So the error carries where the occupant's own registration records it, and each case gets the remedy that fits. Moving it back to that path leaves prune with only the stale entry to clear: ```console $ wt remove --force --yes feature ✗ Directory @ ../repo.feature does not hold the worktree registered there ↳ Removing it could destroy the worktree registered @ ../repo.other; move the directory back there, then run git worktree prune ``` That path is read through `canonicalize_with_parents`, because a relative `gitdir` entry resolves against `<common>/worktrees/<id>` and would otherwise reach the hint with the `..` chain still in it — and plain canonicalization can't normalize a directory that no longer exists, which is the case the arm is reached for. Normalizing there also makes `crate::path::paths_match`, the crate's canonicalizing comparison over that same helper, the right test for the gate, so there is no second comparison beside it. The gate's fail-closed behavior now rests on that helper resolving `..` through the filesystem rather than collapsing it lexically — across a symlink the two readings name different directories — so `src/path.rs` records the constraint where a lexical rewrite would otherwise read as a tidy-up. The FAQ's "What can Worktrunk delete?" paragraph carried the same "a *different* repository" framing and is corrected. ## Scope Pre-existing, and 0.73.0 already narrowed it — 0.72.0 had no ownership check at all and deleted foreign clones too. The guard has two call sites (`prepare_worktree_removal` at planning, `stage_worktree_removal` at the rename), so this reaches `wt merge --remove`, `wt step prune`, and picker removal, not only `wt remove`. One incidental tightening: `wt remove <bare-repo-path>` previously passed the guard (a bare root's git dir *is* the common dir) and was stopped only by the dirty check, which `--force` skips. It now refuses at the guard. <details> <summary>One residual, left alone</summary> `git worktree repair <path>` after the `mv` produces a *double registration*: both `worktrees/repo.bar/gitdir` and `worktrees/repo.feature/gitdir` come to record the same path, and `git worktree list` reports two worktrees there. In that state the new gate accepts the removal — the occupant does point at the `feature` registration, and that registration does point back — while git refuses, because its path→worktree lookup happens to match the `bar` entry first. Closing it means knowing the registration id at the gate, or scanning every `worktrees/*/gitdir` for duplicate claims. Unchanged by this PR, and reachable only via `mv` followed by `repair`. </details> ## Testing Five new tests, each confirmed to fail with the line it covers reverted and to leave the others passing. Three drive the binary: - **the sibling case** — follows #3785's data-safety model: asserts the filesystem afterwards, not just the exit code, since removal stages by rename and deletes in a detached process. Snapshots the refusal, so the hint and the path it names are pinned. Fails with the pointer-back conjunct removed, while the foreign-repo test still passes without it — the two cover different halves. - **the re-check at the rename** — a `pre-remove` hook repoints the worktree's `.git` at a sibling's registration after planning has already cleared it, which is what makes the second gate's freshness observable. Fails when resolution routes back through the `GIT_DIRS`-cached `git_dir()`. - **a relative `gitdir` entry** — removal succeeds, and git reads the rewritten entry back, which is what makes it the form git itself writes. Rewriting the entry rather than setting `worktree.useRelativePaths` keeps the test independent of the git version that introduced the option. Two sit at the gate, where the CLI can't reach: - **both worktree shapes are accepted** — including the main worktree, whose git dir *is* the common dir. `wt remove` rejects the main worktree well upstream of this gate and a bare repository's worktrees are all linked, so nothing through the CLI would notice that arm inverting. - **the refusal names a normalized path** — asserted against `Diagnostic::render`, since the path is in the hint and `Display` carries only the title. Local gate green: 4607 tests, lints, doctests, rustdoc under `-Dwarnings`. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
667c6efaf0 |
docs(switch): note that Alt-x never forces (#3811)
Requested by @max-sixty in [#3809](https://github.com/max-sixty/worktrunk/issues/3809#issuecomment-5273484502) — a couple of words clarifying that `Alt-x`'s removal is safe-only. The keybinding table read `Remove selected worktree/branch`, which doesn't say the removal never forces; that's what sent the reporter looking for a force-remove that isn't there. The picker hardcodes the safe path — [`prepare_removal`](https://github.com/max-sixty/worktrunk/blob/7a2a3e003e7eed138ff5f2dcd2296f6bbd8e86d4/src/commands/picker/mod.rs#L323-L330) passes `BranchDeletionMode::SafeDelete` and `force_worktree: false`. Deliberately scoped to the table cell, per the "(only)" in the request. The bigger question — whether `Alt-x` should ever pass `-D` — is still open on the issue and isn't touched here. Primary source is `after_long_help` in `src/cli/mod.rs`; the three mirrors and the `--help` snapshot are regenerated. Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Maximilian Roos
|
0cb3cc1ef7 |
test(remove): cover the reap guard's spared branch deterministically (#3805)
The controlling-terminal guard's sparing branch had no CI coverage.
`test_remove_reap_kills_process` branches on the suite's own terminal —
none in CI, so CI only ever exercised the kill side, and the branch that
protects an interactive process from `--reap` ran only on dev boxes.
`test_remove_reap_spares_terminal_process` gives the child its own PTY
instead of reading the suite's, so the spared branch runs everywhere.
`portable_pty` makes the child a session leader with that PTY as its
controlling terminal, which is exactly the property the guard reads. A
spared process and an undiscovered one produce identical output ("No
processes to reap"), so the test proves discovery sees the child first,
then pins the guard's verdict in-process, then asserts the child
survives removal.
The reap phase prints before `handle_remove_output` runs the removal, so
both tests now wait for the worktree to actually disappear — without it,
a `wt` that gave up after the reap phase would still satisfy every
output assertion. Both presence polls also move onto `wait_for`, the
existing presence-poll helper, instead of hand-rolled deadline loops.
`tests/CLAUDE.md` picks up two entries this work surfaced. The first is
the slow-timeout triage signature: a kill at the 180s bound is a
duration symptom with two causes, and the durations around it tell CPU
starvation (a sibling worktree's build) from a blocked call inside the
test. It records `threads-required` as starvation's only lever and why
it stays unset. The second distinguishes a bounded poll on one
identified `ErrorKind` from a retry, so `pin_test_binary` and
`forward_with_etxtbsy_retry` don't read as violations of the No Retries
doctrine — the opening sentence is narrowed to the re-run it actually
bans.
> _This was written by Claude Code on behalf of max-sixty_
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Worktrunk Bot
|
1636b78ddf | fix(gitlab): forward glab's verdict when the project lookup fails (#3799) | ||
|
Worktrunk Bot
|
b688e8142b | test(docs): drop the command-page sync test its docs file outlived (#3802) | ||
|
Maximilian Roos
|
897f7d7193 |
fix(tests): extend the spawn pin to benches; correct the pin's cost note (#3792)
Follow-up to #3784, prompted by a history audit of the spawn-flake family. Benches still spawned `env!("CARGO_BIN_EXE_wt")` — the uplifted path the suite stopped spawning — so a concurrent build could fail a bench run's spawns; all 11 sites now route through `wt_bin()` and `test_wt_spawns_are_pinned` scans `benches/` too, making the rule exceptionless. The pin's docstring also claimed the hardlink shares the `deps/` artifact's inode: true where cargo uplifts by hardlink (Linux), but macOS uplifts by copy-on-write clone — the pin keeps the clone, whose blocks stay shared with `deps/` (measured: cloning the 70 MB binary consumes 8 KB), so the no-cost conclusion stands with the mechanism now stated per platform, plus why nothing sweeps the directory. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
715af4cd72 |
fix(tests): pin the spawned wt binary against concurrent cargo uplifts (#3784)
Two test-suite flakes fixed at the root, both dependencies on machine
load and concurrent builds.
**Concurrent-cargo spawn `NotFound`.** Cargo uplifts `target/debug/wt`
by removing the path and recreating it, so a second `cargo` against the
same target directory leaves the binary every test spawns absent for a
fraction of a millisecond per rebuild — the one-off `NotFound` spawn
failures that pass on re-run. `wt_bin()` now returns a hardlink pinned
under `target/debug/wt-test-bin/<mtime>-<len>/`: the uplift unlinks only
the uplifted name, so the pin keeps serving the observed binary through
any number of concurrent rebuilds, at no disk cost beyond the `deps/`
artifact whose inode it shares. `test_wt_spawns_are_pinned` keeps every
spawn routed through it. Reproduced by re-creating the uplift every 200
ms alongside a full `cargo nextest run`: 302 of 4583 tests failed before
this change (147 as direct `NotFound` spawn panics, five of them
byte-identical to the original shell-wrapper report), 4585 of 4585 after
— with an unrelated external cargo also rebuilding `wt` mid-validation,
absorbed the same way.
**`--reap` probe races.** `test_remove_reap_kills_process` predicted the
reap guard's verdict with its own `lsof`/`ps` snapshot, and under load
either probe's spawn can stall past the 5 s bound, whose fail-safe empty
result flips the outcome — a prediction `wt` then contradicts, or `wt`
reporting "No processes to reap" for a live child. The prediction now
reads the session's controlling terminal directly (`/dev/tty` opens iff
the session has one — the property the child inherits at spawn), the
probe timeout is env-pinnable (`WORKTRUNK_TEST_PROBE_TIMEOUT_MS`, set to
60 s in the static test baseline; production keeps its 5 s bound), and
the discovery poll uses the suite's 60 s presence-poll convention.
Looped 15/15 green at load average ~60, where the previous shape failed
2/10.
Not covered here, noted as follow-ups: benches still spawn
`env!("CARGO_BIN_EXE_wt")` directly (same hazard, separate runner,
outside the guard's scan), and the reap "spared" branch has no
deterministic end-to-end test (needs a PTY-held child).
> _This was written by Claude Code on behalf of max-sixty_
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
cf822f75cf |
fix(worktree): guard a registered path that no longer holds its worktree (#3785)
A registered worktree's path was trusted to still hold that worktree.
Three defects followed, one of them destroying data, and the check that
would have caught them — where it existed at all — was `Path::exists()`.
## `wt remove --force` deleted an unrelated repository
A clone that came to sit at a stale registration's path was removed
whole, including uncommitted work and, for a repo never pushed, the only
copy of its objects. wt's own hint routed the user there: the dirty gate
reads `git status` in that directory, reports the occupant's changes as
this worktree's, and offers `--force` as the cure.
```console
$ wt remove feature
✗ Cannot remove worktree: feature has uncommitted changes
?? precious.txt # ← the other repo's file
↳ ... to lose uncommitted changes, run wt remove --force feature
```
git refuses that same removal, `--force` included (`validation failed …
is not a .git file`). Worktrunk's fast path renames the directory into
trash rather than asking git to, so git's validation never ran.
`ensure_belongs_to_repo` makes it, comparing the directory's git dir
against this repository's: a linked worktree's sits under
`<common>/worktrees/`, the main worktree's *is* the common dir, anything
else answers to someone else. One comparison covers both worktree kinds
and also rejects a `.git` file pointing at another repo, which git's
shape test accepts.
It runs at planning, ahead of the dirty gate, and again at the rename
for callers that stage without planning. Now:
```console
$ wt remove --force feature
✗ Directory @ ../repo.feature is not this repository's worktree
↳ Removing it could destroy unrelated data; move the directory aside, then run git worktree prune
```
## A recreated worktree directory leaked git's exit 128
`wt switch`, `wt merge`, and `wt step push` walked into `git rev-parse
--git-dir failed (exit 128)`. Two of them probed `Path::exists()` first,
which a deleted-and-recreated directory passes; the third asked nothing.
`worktree_is_unusable` is the union of both tests, because neither
implies the other. `exists()` catches the absent directory; git's
`prunable` catches the recreated one. `prunable` alone is *not* the
wider test it looks like — git withholds the attribute from a **locked**
worktree even when its directory is gone, since prunability is its
pruning policy and a lock means "don't prune this":
```console
$ git worktree list --porcelain # wt2 locked, all three directories removed
worktree /tmp/ptest/wt1
prunable gitdir file points to non-existent location
worktree /tmp/ptest/wt2
locked removable media # ← no prunable line
worktree /tmp/ptest/wt3
prunable gitdir file points to non-existent location
```
A locked worktree on an unmounted volume is exactly what
`prepare_worktree_removal`'s lock guard exists for, so a `prunable`-only
test would read it as healthy. All three commands now give the message
the merely-deleted case already gave.
`wt remove` keeps `exists()`, deliberately: there it is the precondition
for the cleanup path rather than a health test, since
`prune_worktree_entry` unregisters via `git worktree remove`, which
skips validation only while the directory is absent. No scoped git
command clears the recreated case, so it reports and names the repo-wide
`git worktree prune` that does.
## `wt switch docs/` missed a branch sitting right there
Git's ref format forbids a trailing `/`, so the branch lookup never had
a candidate — and shell completion produces exactly that spelling
whenever a `docs` directory sits beside the branch. Selectors are
normalized before resolution.
## Resolving selectors through one ladder
The three fixes landed in three of the four places that assemble "expand
shortcuts, try the branch, try the path, classify the failure" by hand.
Each gated its path attempt on "did something rewrite this token?",
answered by comparing an expansion's output against its input:
| where | the comparison |
|---|---|
| `resolve_worktree` | `branch == name` |
| `plan_switch` | `target.branch == branch` |
| `target_worktree_at_path` | `target.filter(\|t\| *t == resolved)` |
| `resolve_base_ref` | `resolved == base` |
That is a fact the rewriting step knows, re-derived downstream from its
output, and it is wrong in both directions. A shortcut can expand to the
token it was given — `-` pointing at the branch you are already on — and
string equality reads that as a literal, turning the path arm back on
for a token nobody typed. Normalization breaks it the other way, which
is why the trailing-separator fix needed threading through three call
sites.
`Selector` carries the fact instead: `expand_shortcut` reports whether
it fired, `wt switch` reports its `pr:`/`mr:` dispatch and remote-prefix
strip, and `names_a_path()` replaces all four comparisons.
`resolve_selector` is the ladder, and `plan_switch` expands into it
rather than re-implementing its phases.
`names_a_path()` gates both path steps together — the worktree-by-path
lookup and the directory verdict — which is what `wt switch --create`
needs: the argument names a branch to create, so `branch_only()` takes
the arm off at the producer rather than each consumer re-testing
`create`.
It also reaches the directory verdict, so `ResolvedWorktree` gains
`NoWorktreeAtPath` and the four sites that called `path_selector_error`
themselves stop re-deriving it. The docstring defending that laziness
didn't survive checking — the function returns on `is_valid_branch_name`
before touching the filesystem, so every ordinary branch name already
short-circuited.
| | before | after |
|---|---|---|
| `normalize_selector` call sites | 3 | 1 |
| `path_selector_*` call sites | 4 | 2 |
| "was it rewritten?" comparisons | 4 | 0 |
## Navigating the diff
- `src/git/repository/mod.rs` — `Selector`, `normalize_selector`, the
new `ResolvedWorktree` variant.
- `src/git/repository/worktrees.rs` — `expand_shortcut`,
`expand_selector`, `resolve_selector`, `usable_worktree_for_branch`.
- `src/git/repository/working_tree.rs` — `ensure_belongs_to_repo`, the
ownership check.
- `src/git/remove.rs`, `src/commands/repository_ext.rs` — where it gates
removal, and why before the dirty gate.
- Call sites: `commands/worktree/switch.rs`,
`commands/worktree/push.rs`, `commands/merge.rs`, `commands/remove.rs`,
`git/repository/config.rs`.
## Size
Comments and docstrings are the largest share: the ownership check and
the four conditions behind the directory verdict all look like things to
simplify away, so the reason each exists is recorded where it's
enforced.
| | + | − |
|---|---:|---:|
| Production code | 277 | 142 |
| Comments & docstrings | 320 | 75 |
| Tests | 325 | 8 |
| Snapshots | 186 | 0 |
| Docs | 6 | 0 |
| **Total** | **1114** | **225** |
## Testing
Seven new tests. The data-safety one drives the real binary and asserts
the filesystem afterwards, not just the exit code — removal stages by
rename and deletes in a detached process, so a passing exit would not
have caught a staged-then-deleted tree. The others cover the recreated
directory (switch and remove), the trailing separator, `--create`
against a worktree registered at that path, and, at the unit boundary,
the four states of `worktree_is_unusable` — healthy, absent,
locked-and-absent, recreated — and the selector's path-ness, including
the degenerate case string equality got wrong. Each new test was
confirmed to fail with its fix reverted.
One more covers an omitted merge target in a repo whose default branch
can't be determined. `^` had a test for that error; the omitted-target
route to the same message had none. The gap predates this branch — the
closure is byte-identical to the one it replaces and codecov records
those lines as missed at the base commit too — but relocating them into
`resolve_target_selector` re-counted them as patch lines, which is what
surfaced it.
Local gate green: 4593 tests, lints, doctests, rustdoc under
`-Dwarnings`.
<details>
<summary>Behavioral matrix, verified against a build</summary>
```console
docs/ (trailing sep) ▲ Worktree for docs @ ../repo.docs
detached by path ▲ Worktree for detached worktree @ ../repo.det
leftover dir ✗ No worktree @ ../repo.leftover
recreated dir ✗ Worktree directory missing for rec
shortcut ^ ▲ Worktree for main @ ../repo
remove leftover ✗ No worktree @ ../repo.leftover
--base docs/ ✓ Created branch nf from docs
foreign-repo remove ✗ Directory @ ../repo.frn is not this repository's worktree
precious.txt survives
```
</details>
<details>
<summary>Also swept, and one thing left alone</summary>
Three more instances of the same shape, fixed here:
- `resolve_base_ref` was the fourth copy of the comparison, so `--base
docs/` now resolves too.
- `hint_for_repo` suggested `wt switch ^` after an existence probe a
recreated directory passes, pointing at a worktree the switch then
refuses.
- The pre-switch hook's `target` var used the bare shortcut expander, so
a hook saw `docs/` where the switch resolved `docs`.
The identical unborn/stale default-branch block in
`require_target_branch` and `require_target_ref` is extracted. The rest
of that pair differs in its existence predicate, extra arms, and final
error; sharing it would cost more in parameters than the duplication
does.
Left alone: `live_sibling_checkout` decides whether another worktree
still holds a branch during removal, and also uses `exists()`. Switching
it to `prunable` would make branch deletion *more* likely in a corner
case where the detached path already answers the other way. That is a
data-safety surface and a separate decision.
</details>
> _This was written by Claude Code on behalf of max-sixty_
|
||
|
Worktrunk Bot
|
ec0f3a344f |
fix(output): strip stderr color on a pipe where std's eprint! kept it (#3771)
## Problem
`worktrunk::styling`'s `eprint!` is anstream's and strips ANSI when
stderr isn't a terminal; std's prelude macro of the same name keeps it.
A file that imports one but not the other — or neither — gets a mix, and
adjacent lines of the same message block disagree about whether a
redirected stderr carries escapes.
`wt list 2>&1 >/dev/null | cat -v` in a repo with a deprecated `[ci]`
block, on `main`:
```
^[[33mM-bM-^VM-2^[[39m ^[[33mProject config: ^[[1m[ci]^[[22m is deprecated in favor of ^[[1m[forge]^[[22m^[[39m
M-bM-^FM-3 To see details, run wt config show; to apply updates, run wt config update
```
The warning is `eprint!("{warnings}")` at `src/config/deprecation.rs`,
which resolved to std's macro; the hint directly beneath it is the
`eprintln!` imported from `styling` four lines later. Anyone redirecting
`wt` narration to a file gets escapes on one line and not the next.
This is the stderr counterpart of what #3746 and #3766 fixed on stdout,
and it went unnoticed for the reason named in `verbatim.rs`'s own
docstring: the suite sets `CLICOLOR_FORCE=1`, which forces color on
*both* printers, so no snapshot could disagree no matter which macro was
in scope. `output_system_guard` doesn't cover it either — it scans for
`print!`/`println!` tokens under `src/commands/`, not for which
`eprint!` a file imported.
## Solution
The rule is now structural rather than per-site.
`check_stderr_macros_come_from_styling` in `output_system_guard.rs`
walks every `.rs` file under `src/` and flags a bare
`eprint!`/`eprintln!` whose file lacks the matching `worktrunk::styling`
import. A call satisfies it either way — importing the macro, or
qualifying the call as `styling::eprintln!(…)`, which several files
(`git/repository/mod.rs`, `config/user/mod.rs`,
`commands/config/alias.rs`) already do. Two files are allowlisted with a
reason: `testing/mock_stub.rs` relays a stub's captured stderr verbatim,
so its bytes are fixture data; `remove_dir.rs`'s one call is a
`#[cfg(test)]` skip diagnostic, not narration a user redirects.
Reverting the source fixes below makes it name exactly those five lines
and nothing else.
The sites it fixes:
- `src/config/deprecation.rs` — the deprecation warning block above.
- `src/commands/config/update.rs` — the `format_update_preview` block
shown before `wt config update`'s prompt, reachable with a tty stdin and
a redirected stderr.
- `src/output/prompt.rs` — the `[y/N/?]` prompt; its blank-line
`eprintln!` was already explicitly qualified as
`worktrunk::styling::eprintln!`, so the two disagreed within four lines.
Both now come from one import.
- `src/output/global.rs` — the file the first scan couldn't see, because
that scan looked for "imported `eprintln` but not `eprint`" and this
file imports neither. Its four styled `eprintln!` calls
(`print_outdated_shell_wrapper_hint_once`, `warn_retired_exec_once`,
`warn_exec_scrubbed_once`) all resolve to std's, so a user mid-upgrade
running `wt … 2>log` gets `ESC[…m` around the shell-wrapper repair hint.
The module's own docstring already claimed the contract the code didn't
have — *"Regular output still uses `eprintln!`/`println!` directly (from
`worktrunk::styling` for color support)"*. One added import makes it
true; under the suite's `CLICOLOR_FORCE=1` no snapshot moves.
- `src/commands/for_each.rs` — the pre-spawn ANSI reset.
`output/handlers.rs` runs the identical three lines
(`stderr().flush()?`, `eprint!("{}", anstyle::Reset)`,
`stderr().flush().ok()`) immediately before building its `Cmd`, but
through anstream's `eprint` *and* anstream's `stderr`; `for_each` used
std's for both, so the same operation wrote a literal `ESC[0m` into a
redirected stderr where `handlers` dropped it. Both halves move together
— the flushes have to name the stream the reset was written to, so
switching `eprint!` alone would flush std's handle while anstream's
buffer held the write. The `std::io::stderr()` handed to `Stdio::from`
four lines down is a different thing and stays.
## Tests
`test_stderr_narration_strips_ansi_when_piped` in
`output_system_guard.rs`, alongside the closed-consumer test #3766
added. It clears `CLICOLOR_FORCE` and sets `NO_COLOR` (which only
anstream honors), triggers the `[ci]` deprecation, and asserts stderr
carries the warning and no `\x1b`. Confirmed to fail on the pre-fix
source with exactly the escapes quoted above, and to pass with it.
That test proves what the property buys at one site;
`check_stderr_macros_come_from_styling` is what holds it at all of them.
No runtime test can: the suite's `CLICOLOR_FORCE=1` forces color on both
printers, so a snapshot agrees whichever macro is in scope, and the
property is about every stderr write in the binary rather than any one
path.
The module docstring is updated for both — the `Allowed:` list no longer
reads flatly as "`eprintln!` / `eprint!` (stderr is safe)", which was
the sentence someone skims before making this exact mistake.
## Verification
`cargo clippy --all-targets`, `cargo fmt --check`, `cargo test --lib
--bins` (2,454 passed), and the integration suite (1,961 passed) all run
locally. One integration test fails in this sandbox and is unrelated:
`test_copy_ignored_preserves_file_executable_permissions` expects `0644`
and sees `0664`, because the sandbox's umask is `002` rather than the
runner's `022` (confirmed by `umask` → `0002`). It touches none of these
files; CI will confirm.
---------
Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
|
||
|
Maximilian Roos
|
4d7f8c944e |
fix(env): drop Intel macOS from the flake, and install pwsh and jq for web setup (#3776)
Three follow-ups from #3768, plus a bug the verification turned up. **The flake stops declaring outputs for `x86_64-darwin`.** nixpkgs drops Intel macOS in 26.11: evaluating anything for that system against `nixos-unstable` (`26.11pre-git`) throws. The rev `flake.lock` pins is 26.05-era, so it still evaluates today, carrying nixpkgs' own warning that "26.05 will be the last release to support x86_64-darwin". Naming three systems rather than `eachDefaultSystem` drops Nix support for Intel Macs now, ahead of that bump. Release binaries are untouched: `dist-workspace.toml` still ships `x86_64-apple-darwin` and nightly still tests it on `macos-15-intel`. **`git` leaves the devShell's `packages`.** It arrives with the `checks`, which crane folds in via `inputsFrom`, the same mechanism that already supplied `python3`, `procps` and `lsof`. **`task setup-web` installs `pwsh` and `jq`.** Without them Claude Code web can't run `--features shell-integration-tests`, which is what the pre-merge gate runs. PowerShell comes from the release `.deb` rather than the tarball, because pwsh aborts at startup without libicu and only the `.deb` declares that dependency for apt to resolve. The verification loop runs each tool instead of looking for it on PATH, since the tarball install left a `pwsh` that was on PATH and still aborted. **A `set -e` abort found while testing that.** The `sources.list.d` cleanup was an `&&` chain, and under `set -e` a chain ending false takes the whole task down. This one ends false on an unmatched glob and on a `.list` file with no `[` line, so setup was dying before it installed anything on a stock Debian box as well as an empty one. It's an `if` now. ## Verification No `nix` on the machine this was written on, so the flake was checked in a `nixos/nix` container and the Taskfile block in an amd64 Debian one. <details> <summary>flake: three systems evaluate, x86_64-darwin is gone, git survives its deletion</summary> ``` == devShell evaluates per system == x86_64-linux OK g172vwl0g339zsxx9l6mz5pca6w9jbcx-nix-shell.drv aarch64-linux OK pgvq71zs48bx3naddncms954jyqpl0bl-nix-shell.drv aarch64-darwin OK d17q1772si0x0hj1lgpnin8wiq4zlpr2-nix-shell.drv x86_64-darwin FAIL: flake does not provide attribute 'devShells.x86_64-darwin.default' == systems the flake declares == ["aarch64-darwin","aarch64-linux","x86_64-linux"] == tools in the x86_64-linux devShell == git: present jq: present nushell: present powershell: present python3: present procps: present lsof: present fish: present zsh: present bash: present gh: present pre-commit: present == nixfmt --check flake.nix == clean (exit 0) ``` The x86_64-darwin claim, checked against nixpkgs directly rather than inferred: ``` == nixos-unstable lib.version == "26.11pre-git" == x86_64-darwin eval on nixos-unstable == error, pointing at release-notes#x86_64-darwin-26.11 == x86_64-darwin eval on the pinned rev (flake.lock) == evaluation warning: Nixpkgs 26.05 will be the last release to support x86_64-darwin "hello-2.12.3" ``` Not verified: nothing was built, only evaluated. The nightly `nix-flake` job runs `nix flake check` on PRs touching `flake.nix`, which covers that on x86_64-linux. </details> <details> <summary>setup-web: the block run under Task's own interpreter, in an amd64 Debian container</summary> The edited block was extracted into a minimal Taskfile and run by `task` itself, so mvdan/sh parses it rather than bash. `curl` and nushell are container prereqs, not part of what's under test. ``` === running the extracted block under Task === Installing shell-integration test dependencies... pwsh installed bash available zsh available fish available nu available pwsh available jq available task exit: 0 === does the installed pwsh actually run? === 7.6.4 jq-1.6 /usr/bin/pwsh === rerun is idempotent === Installing shell-integration test dependencies... bash available zsh available fish available nu available pwsh available jq available ``` Two earlier runs are why the shape changed. The first died at the `sources.list.d` glob. The second installed PowerShell from the release tarball: every tool reported "available" and `pwsh` then aborted with `Couldn't find a valid ICU package installed on the system`, which is what moved the install to the `.deb` and the check from `command -v` to `--version`. </details> `cargo run -- hook pre-merge --yes` passes: 4574 tests, 1 skipped. ## Notes `task setup-web` still requires nushell to be present rather than installing it, unchanged here. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
c37d07d87a |
refactor(output): resolve color in one place via anstream's global ColorChoice (#3777)
`wt` had five independent mechanisms deciding whether escape sequences reach the consumer: the anstream macros, a `println_verbatim!` macro that bypassed them, `ProgressiveTable`'s raw stdout writes, clap's per-command `ColorChoice`, and a raw `anstyle::Reset` in `terminate_output`. This collapses them onto the ecosystem primitive: `anstream::ColorChoice::write_global()`, which `AutoStream::choice` consults before tty detection — the mechanism behind every `--color=always` flag. Color now resolves in exactly one place, and code emits styles freely while the stream decides what survives. For navigating the diff: - **Payload surfaces declare their choice once.** The statusline (`src/commands/statusline.rs`) declares `Always` — a shell prompt or Claude Code captures and re-renders its line, so the escapes are the answer, not presentation; `Always` outranks `NO_COLOR` per the no-color.org convention, preserving shipped behavior byte-for-byte. `--help-page` declares `Always`/`Never` per mode and `--help-md` declares `Never` (`src/help.rs`). `println_verbatim!` is deleted; these surfaces print through the ordinary macros. - **`help.rs` keeps one color mapping.** The five per-command `cmd.color(...)` calls were inert — clap consults `color_when` only in `err.print()`, and every path here uses `err.render()` — so the embedded `--help` reference block always renders `.ansi()` and the stream strips or passes it. All 16 generated doc outputs (`--help-page`, `--plain`, `--help-md` across commands) are byte-identical before/after. - **`ProgressiveTable` consults the same choice rather than writing through a stream** (`src/commands/list/progressive_table.rs`) — anstream's strip adapter would eat its cursor-control CSI, so it reads `AutoStream::choice` once and strips content with `anstream::adapter::strip_str`, the exact transform the buffered path applies. `NO_COLOR` now works on default `wt list`. - **One truncator** (`src/styling/line.rs`): `display::truncate_to_width` — which cut escape-blind — is merged into `truncate_visible`, which now trims trailing whitespace before the `…` and appends a reset only when the kept prefix carries an escape. Styled output is byte-identical (`ansi_cut`'s style closers both block the trim and trigger the reset); one snapshot line changes, a plain branch name losing a needless `[0m`. - **stderr joins the model**: `-v` diagnostics route through `AutoStream::auto(stderr)` (`src/logging.rs`), so `NO_COLOR` and piping reach them, and the lone `ceprintln!` (std stderr, hardcoded escapes) becomes the anstream macro (`src/main.rs`). - **The guard widens** (`tests/integration_tests/output_system_guard.rs`): the stdout scan covers all of `src/` rather than `src/commands/`, and the new `test_color_follows_the_consumer` pins six unforced-color cases — the suite's global `CLICOLOR_FORCE=1` previously made anstream's strip/pass decision invisible to every test. Behavior changes, all in the strip direction: `NO_COLOR` and piped-strip now reach progressive `wt list`, `-v` stderr diagnostics, and the clap error tips; `terminate_output`'s reset is stripped when color is off; truncation no longer leaves a `[0m` on plain text or a space before the ellipsis. The statusline's `Always` semantic is now pinned by test rather than incidental. Testing: the full pre-merge gate is green (4581 tests) plus the feature-gated PTY picker suite; `test_color_follows_the_consumer` covers the unforced matrix, and `test_list_progressive_honors_no_color` pins the progressive path on a real PTY (no SGR under `NO_COLOR`, cursor-control CSI still flowing); docs-sync verifies the generated pages byte-for-byte. The `writing-user-outputs` skill paragraph that described `println_verbatim!` now describes the `write_global()` declaration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
00ab0ffa26 |
Canonicalize benchmark fixtures and variants (#3761)
Benchmark fixtures still encoded the benchmark that first needed each repository state, which left overlapping recipes and variants after the earlier harness consolidation. This change reduces the fixture catalog to two provenance-based bases: `Generated` builds an ordinary Git repository locally, while `Imported` copies the pinned `rust-lang/rust` corpus. Worktree, branch, and remote-ref populations remain parameters on `Generated`; prune candidates and backdrop are overlays that work with either base. The generated base deliberately combines heterogeneous worktree states, history-spread branches, and optional remote refs so ordinary list, completion, picker, first-output, alias, remove, and prune benchmarks can share it. Imported history-spread branches and clean base-tip worktrees carry their own commits, preserving the base populations without making them incidental prune candidates when overlays advance the default branch. The benchmark matrix now keeps single-factor contrasts: list scaling uses the 1- and 8-worktree endpoints; alias dispatch has a startup floor, two population endpoints, and one warm/cold variable-resolution pair; completion keeps one full-surface case; remove and prune vary cache or hook state only where the command exercises it. Historical recipes, redundant cache rows, and intermediate scaling points are removed. Manual setup paths live under `target/`, and the benchmark guide documents the resulting fixture and cache model. Tests: `cargo run -- hook pre-merge --yes` after merging current `main` (4,571 tests); targeted Criterion test-mode runs; `cargo test -p wt-perf`; benchmark check, clippy, formatting, and diff checks. > _This was written by Codex on behalf of max-sixty_ |
||
|
Maximilian Roos
|
f3ea2fd598 |
fix(errors): report a directory holding no worktree as a directory (#3773)
Passing a path that holds no worktree used to be reported as a missing branch: ``` $ wt remove /repo/.claude/worktrees/ghost ✗ No branch named /repo/.claude/worktrees/ghost ↳ To list branches, run wt list --branches --remotes ``` The argument is plainly a path; the error calls it a branch and sends the user to a listing it could never appear in. Now: ``` $ wt remove /repo/.claude/worktrees/ghost ✗ No worktree @ /repo/.claude/worktrees/ghost ↳ The directory exists but is not a worktree; to list worktrees, run wt list ``` #3767 fixed the hook that produced the leftover directory in #3753; this fixes the message a person gets when they type such a path themselves, which judewang named there as why it was hard to diagnose. Ref #3753. Two neighbouring commands had the same cause and are also fixed. `wt switch <path>` and `wt merge <path>` used to offer `wt switch --create <path>`, which fails with `fatal: … is not a valid branch name`; and `wt config state marker set --branch <path> foo` **succeeded silently**, storing state keyed by a path string. ## How the decision is made `Repository::path_selector_error` reports a directory only when all four conditions hold. Each is here because dropping it made `wt` assert something false, reproduced against a build: | Condition | Without it | |---|---| | Git could never accept the selector as a branch name | `wt step rebase HEAD~9` reports revision syntax as a path | | A directory is there | `wt remove docs` stops meaning the branch when `docs/` sits beside it | | No worktree of this repo is registered at it | a *detached* worktree — reachable by path alone — is reported as absent, contradicting the `wt list` in the hint | | It holds no git data | a sibling repo's live checkout, or a bare repository, is called a leftover only the user can delete | That last one is the data-safety case: the new variant's hint says only the user can delete the directory, so a false positive reads as an invitation to `rm -rf` a checkout with uncommitted work. Absence of git data is *established* rather than assumed — `symlink_metadata` with only `NotFound` counting, so an unreadable clone or a dangling `.git` symlink withholds the claim instead of earning it. The first condition is git's own ref-format rules, implemented in-process and pinned against `git check-ref-format` by a test. ## Navigating the diff - `src/git/repository/worktrees.rs` — `Repository::path_selector_error` and `holds_git_data`, with the rationale for each condition recorded where it's enforced. - `src/git/repository/branch.rs` — `is_valid_branch_name`, the ref-format predicate. - `src/git/error.rs` — the new `GitError::WorktreeNotFoundAtPath` variant. - `src/git/repository/config.rs` — `target_branch_at_path` → `target_worktree_at_path`, returning the worktree whole rather than collapsing "no worktree here" with "a detached worktree here". - Call sites: `commands/remove.rs`, `commands/worktree/switch.rs`, `commands/config/state.rs`. Reporting a detached *target* made `DetachedHead`'s fixed hint wrong: `git switch` acts on the tree it runs in, so `wt merge ../B` was telling the user to switch the tree they were standing in. The variant now carries the detached worktree when that isn't the current one, and the hint becomes `git -C ../B switch <branch>` — the same reason `OperationInProgress` carries `branch`. Set at the three sites where the detached worktree isn't the current one (`require_target_branch`, `require_selected_branch`, and `wt step promote`, which raises this about the main worktree); every other site passes `None` and renders as before. 140 of the 257 added production lines are comments and docstrings — three of the four conditions look like obvious things to simplify away, so the reason each exists is written down next to it. ## Testing Unit tests at the resolver boundary cover each condition and its complement, both `directory_exists` arms, the shortcut-expansion route (`wt merge -`), detached targets, revision syntax, unresolvable `.git` entries, and bare repositories. Two integration tests drive the real binary end-to-end against `../repo.leftover` — `wt remove` and `wt switch` — covering the relative-path spelling and each command's wiring, and the `switch` snapshot pins the thing that would regress silently: that no `--create` hint is offered for a name git would reject. The ref-format predicate is verified beyond its committed table: 2,791 generated names were run against real `git check-ref-format` with zero disagreements, and two review passes did the same over their own inputs. That sweep isn't committed — it needs Python and a live `git` — so the committed table is what guards against drift. Not exercised locally: the Windows-specific behaviours are reasoned, not run — `Path::new("foo.").is_dir()` succeeding against `foo`, and drive-relative `C:foo`. CI exercises the platform but not those inputs. <details> <summary>Known gaps this would ship with</summary> - `wt switch docs/` (trailing slash, branch `docs` has a worktree, `./docs` is a source directory) answers `No worktree @ docs/`. True, unhelpful. The real fix is stripping trailing separators before resolution — a resolution change, not a message change. On Windows it reads `No worktree @ …/docs`, because `path_slash`'s `to_slash_lossy` rebuilds the string from `Path::components()` there and drops the trailing separator — still true of the path, minus the character that explains it. The doc comment on `path_selector_error` now says so rather than promising a spelling guarantee that holds on one platform. - A worktree whose registration was pruned out from under it gets a safe-but-vague message. The useful answer is a distinct `git worktree repair`-or-delete diagnosis; that's new detection, and nobody has reported hitting it. - Under `-C`, a `./`-prefixed selector renders a visible `/./` (`wt -C repo remove ./ghost` → `repo/./ghost`). Cosmetic; tidying it would discard the trailing separator the `docs/` case needs. - `require_target_branch` and `require_target_ref` remain near-copies of one ladder, and this adds a line to each. Pre-existing; extracting it is a separate refactor. - `CommandEnv::require_branch` passes `worktree: None` unconditionally, and `CommandEnv::for_selector` builds an env whose `worktree_path` is the worktree the *argument* named — so a future caller combining the two would get the unqualified hint. Nothing reaches that combination today: `merge` and `squash` are the only `require_branch` callers and both build with `for_action`, and the one `for_selector` caller (`wt step commit --branch`) never calls it. Two pre-existing bugs surfaced while reviewing this, unrelated to the diff and reproducible by branch name: `wt remove` will trash a clone that now sits at a registered worktree's path, and a recreated worktree directory leaks a raw `git rev-parse --git-dir failed (exit 128)`. </details> > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
497551e076 |
fix(nix): give the devShell every tool the test suite shells out to (#3768)
`devShells.default` listed `bash`, `zsh`, `fish` under a `# For shell integration tests` comment, but that feature drives two more shells and shells out to `jq`. So `nix develop` could not run `--features shell-integration-tests`, which is what the repo's own gate runs (`.config/wt.toml` → `cargo insta test … --all-features`). This adds `nushell`, `powershell` and `jq`. The same dependency claim was stated, wrongly, in three other places. All four now name the real set: | Where | Was | Now | |---|---|---| | `flake.nix` devShell | bash, zsh, fish | + nushell, powershell, jq | | `Cargo.toml` feature comment | bash, zsh, fish | + nu, pwsh, jq | | `tests/CLAUDE.md` | bash/zsh/fish + PTY | + nushell, pwsh, jq | | `docs/content/faq.md` | bash, zsh, fish, nushell | + pwsh, jq | `flake.nix` also referenced a `CLAUDE.md → "Shell/PTY Integration Tests"` heading that exists nowhere in the repo; it now points at the section that does. ## Verification There is no `nix` on the machine this was written on, so the two claims were checked separately rather than by entering the shell. **Is the list right?** A symlink farm modelling a devShell's `PATH` — nixpkgs stdenv's own tools plus the candidate list, and nothing else — with the full `--all-features` suite run under it. `configure_pty_command` propagates the test process's `PATH` into PTY children, so this reaches the shell tests. <details> <summary>Runs (the control is what makes the failures attributable)</summary> | PATH | Result | |---|---| | Full ambient PATH (control) | 4572 passed, 0 failed | | stdenv + every tool the suite needs | 4572 passed, 0 failed | | …minus `pwsh` | 3 `shell_powershell` tests fail | | …minus `jq` | `test_worktree_remove_hook_skips_path_holding_no_worktree` fails | | …minus `python3` / `lsof` / `ps` | 6 failed: 2 `for_each`/`post_start` (python3), 1 `remove::test_remove_reap_kills_process` (lsof), 3 pgid/process-probe (ps) | The control run matters: it establishes that every failure above is caused by the withheld tool rather than by a local flake. The last row is about what the *suite* needs, not what the shell was missing — see the correction below. </details> **Does the flake still evaluate?** `nixos/nix` in a container, evaluating `devShells.<system>.default` for all four systems `flake-utils` covers, against unmodified `main` as a control. All four evaluate, and every tool resolves on each. Not verified: nothing here was *built*, only evaluated, so a package that evaluates but fails to build would not have been caught. The nightly `nix-flake` job runs `nix flake check` on PRs touching `flake.nix`, which covers that on x86_64-linux. <details> <summary>A correction: python3/procps/lsof were never missing</summary> The first version of this PR also added `python3`, `procps` and `lsof`, claiming the shell could not run a plain `cargo test`. That was wrong, and worktrunk-bot caught it. `craneLib.devShell` sets `inputsFrom = builtins.attrValues checks ++ inputsFrom`, and `mkShell` folds each `inputsFrom` derivation's `nativeBuildInputs` into its own. `checks` includes `worktrunk-tests`, whose `nativeBuildInputs` already carry `git`, `python3`, `procps` and `lsof` — so the devShell inherited all four. Evaluating the unmodified `main` tree confirms it: its `x86_64-linux` devShell derivation already contains `python3`, `procps` and `lsof`, and contains no `nushell`, `powershell` or `jq`. The symlink farm could not have caught this: it modelled stdenv plus the literal `packages` list, so crane's inherited inputs were invisible to it by construction. The experiment established what the *suite* needs; it said nothing about what the *shell already had*. Those three lines are dropped. `git` remains listed in both places — pre-existing, and left alone here. </details> <details> <summary>A guard I added and then removed</summary> `powershell` first went in behind `lib.meta.availableOn`, because nixos-unstable's PowerShell has no `x86_64-darwin` source. Testing that guard against nixpkgs HEAD showed the actual cause: nixpkgs 26.11 dropped Intel macOS wholesale, so the entire flake fails to evaluate there regardless of the guard. On every system nixpkgs still supports, PowerShell has a build — the guard protected against nothing, and its comment justified it with the wrong mechanism. Removed; `powershell` is listed plainly with the other shells. </details> ## Notes `task setup-web` checks for `bash zsh fish nu` and not `pwsh`/`jq` — the same gap in a different environment, left alone here since its install mechanics are unrelated. > _This was written by Claude Code on behalf of max-sixty_ |
||
|
Maximilian Roos
|
f57fac365b |
Release v0.72.0 (#3759)
Release v0.72.0 — 55 commits since v0.71.0. Minor bump: `cargo semver-checks` reports 5 breaking library changes, so patch is disallowed pre-1.0. ## Headline changes - **`wt merge` / `wt step push` no longer autostash the target worktree** (#3703). Both strategies now advance the target through one `advance_target` — a compare-and-swap `update-ref`, then `read-tree -m -u` in the target worktree — so `refs/stash` is never entered and staged changes stay staged. - **Forge classification returns to brand-in-hostname** (#3673), reverting the exact-DNS-label rule 0.71.0 shipped. `github-enterprise.acme.com` and friends resolve again with no config. - **`[projects."…"]` keys match by `*` pattern and carry forge settings** (#3701), so one user-config entry covers every repository on a self-hosted host. - **A published JSON Schema for `wt list --format=json` schema 2** (#3747), plus machine-readable approval state and `branch_outcome` (#3710). Full detail in `CHANGELOG.md`. ## One fix made during the release cut The release audit surfaced a gap this release's own `advance_target` rewrite introduced, fixed here rather than deferred: **`wt merge` / `wt step push` now refuse a target worktree parked mid-operation.** The target sync is a two-tree merge, which refuses an unmerged index but *not* a stopped cherry-pick or rebase whose conflict has already been staged. A target paused between steps could therefore have the push range written into it, and the user's `--continue` would commit the synced tree as the step's result. The old fast-forward path got this check for free from `receive.denyCurrentBranch=updateInstead`, which refused any unclean target outright; both strategies now ask directly, and the refusal names the worktree holding the operation. `test_push_refuses_target_mid_operation` covers it in both shapes a stopped operation can take, and both are mutation-verified. With the gate disabled, the push succeeds and writes `feature.txt` into the mid-cherry-pick worktree. The rebase case was added in response to review feedback on this PR, and pins a second dependency. A rebase detaches HEAD, so `git worktree list --porcelain` reports the target with no branch and `worktree_for_branch` finds it only because `finalize_worktree` backfills from `rebase-merge/head-name`. That makes the rebase arm the one place this guarantee rests on a helper of ours rather than on git — the fast-forward path it replaced got the refusal from `find_shared_symref`. With the backfill disabled, `wt step push` succeeds against a worktree parked mid-rebase while the cherry-pick case still passes, so the gap was real. ## Validation - Local gate green: `cargo run -- hook pre-merge --yes` — 4570 tests, clippy, fmt, doc sync. - Cross-platform nightly green on the cut-from tip `3817df079` (run 31133551751): full nextest matrix on linux/macOS/Windows, feature-powerset, all three release triples, nix-flake, minimal-versions, unused-deps, crate-build, link-check. - Changelog verified entry-by-entry against the diffs by an independent pass; every one of the 55 commits either maps to an entry or is a documented skip. - `main` advanced during the CI wait. #3762 ships in this release and now has a changelog entry; the other two commits that landed (#3758, #3749) touch only `.github/`. - Data-loss surface reviewed by four independent finders over the cumulative diff. One further finding — a pre-0.72 `approvals.toml` key containing `*` being reinterpreted as a wildcard on upgrade — was reviewed and accepted as out of scope for this release. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
bb580ed46b |
fix(plugin): WorktreeRemove hook skips a path holding no worktree (#3767)
## Problem The `WorktreeRemove` hook guards with `[ -e "$p" ]` — #3493's narrowing, which made the hook a no-op when the recorded worktree path is gone. #3753 reports the third state between "gone" and "live": a path that **exists but holds no worktree**. A skeleton directory left by an interrupted create or remove has no `.git`, is invisible to both `git worktree list` and `wt list`, and passes the existence guard, so `wt remove <path>` runs and resolves the path as a *branch* name: ``` ✗ No branch named /…/.claude/worktrees/<name> ↳ To list branches, run wt list --branches --remotes # exit 1 ``` An out-of-tree skeleton gives `fatal: not a git repository` instead. Claude Code reads a nonzero `WorktreeRemove` as a failed removal and keeps the session row, so the finished background session can never be deleted with Ctrl+X — the same end symptom as #3488, but permanent: prune ignores a directory that was never a worktree, so nothing heals it. ## Solution Test for git's marker rather than for the directory: ```diff - [ -e "$p" ] || exit 0 + [ -e "$p/.git" ] || exit 0 ``` Every linked worktree carries a `.git` file, and a dirty, locked, or unmerged one still does, so genuine failures keep surfacing loudly — the #2939 spirit the #3493 guard was written to preserve. Leniency stays scoped to "no worktree lives here" rather than becoming a blanket success, and the change stays inside the single-quoted `bash -c` body, so outer login-shell (fish/zsh/bash) parsing is untouched. `-e` rather than `-f` is deliberate: the *main* worktree's `.git` is a directory, so `-e` keeps `✗ The main worktree cannot be removed` loud where `-f` would silently no-op it. This composes with #3754 rather than overlapping it: the guard now runs before `-C "$p"`, so `-C` only ever resolves a path that really is a worktree. One state changes beyond the reported one, in the same direction: a registered worktree whose `.git` file was deleted by hand already failed the hook (`fatal: not a git repository`), and now exits 0, leaving a prunable registration for `git worktree prune`. Nothing `wt remove` could previously remove is skipped. ## Testing `test_worktree_remove_hook_skips_path_holding_no_worktree` runs the real command out of `hooks.json` under `bash`, feeding it the recorded path on stdin exactly as Claude Code does. It pins three directions, so neither a blanket `exit 0` nor a swallowed `wt remove` failure can satisfy it: a skeleton is a no-op and is left on disk (both in-tree and out-of-tree), a dirty worktree still fails with `uncommitted changes` and stays put, and that same worktree once clean is still removed. <details><summary>Mutation evidence and hand-verified states</summary> Each mutation was applied to `hooks.json` and the test re-run: | mutation | result | |---|---| | `[ -e "$p" ]` (the pre-fix guard) | fails — `hook must be a no-op for a path holding no worktree` | | whole body replaced with `exit 0` | fails — `hook must still refuse a dirty worktree, and for that reason` | | `wt remove … \|\| exit 0` (failure swallowed) | fails — same assertion | Hand-verified against the built binary via `WORKTRUNK_BIN`, firing the hook as Claude Code does: | state | before | after | |---|---|---| | skeleton dir, in-tree | exit 1, `No branch named …` | exit 0, directory untouched | | skeleton dir, out-of-tree | exit 1, `fatal: not a git repository` | exit 0, directory untouched | | recorded path gone | exit 0 | exit 0 | | clean worktree | removed | removed | | dirty worktree | exit 1, `has uncommitted changes` | unchanged | | unmerged branch | worktree removed, branch retained + `-D` hint | unchanged | | main worktree | exit 1, `The main worktree cannot be removed` | unchanged | | registered worktree, `.git` deleted by hand | exit 1, `fatal: not a git repository` | exit 0, prunable entry left | The test is gated on `all(unix, feature = "shell-integration-tests")`, which the required `test` jobs and the coverage run both enable; the hook parses its stdin with `jq`. **Not verified:** Claude Code's own session-row teardown — CI can't drive the agent UI. The evidence here is the hook's exit status, which is what Claude Code branches on per #3488/#3493. </details> The full pre-merge gate passes locally (4571 tests, clippy, doctests, docs sync, no pending snapshots). ## Relation to #3755 Supersedes worktrunk-bot's #3755, whose one-line hook edit is byte-identical to this one. The difference is test coverage: #3755's test passes against a hook with `|| exit 0` appended to `wt remove`, which would silently discard the dirty-worktree refusal — verified by running that test file against the mutation. This one also covers the out-of-tree skeleton. #3755's `flake.nix` addition of `jq` is left out: the devShell already omits nushell so it can't run the shell-integration suite regardless, and the nix test derivation runs default features only, where this test isn't compiled. Closes #3753. Thanks to @judewang for the report, the reproduction, and the fix direction — including the note that `git -C "$p" rev-parse --is-inside-work-tree` is not a usable test, since discovery walks up to the parent for a nested skeleton. > _This was written by Claude Code on behalf of max-sixty_ Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
5576eec1a4 |
fix(output): don't panic when a consumer stops reading stdout (#3766)
`wt list | head -3` exited 101 with `failed printing to stdout: Broken pipe`, and so did `wt list statusline | head -1` — the surface a shell prompt runs on every redraw. std's `print!`/`println!` panic on a `BrokenPipe`; anstream's drop it, which is why the `--format=json` surfaces #3746 converted already exit cleanly. Seven stdout surfaces were still on std's macros. Five of them — the `wt list` table, `--version`, `--help-md`, `--help-description`, and `wt config update --print` — are read by a person, so they go through anstream's printer, the canonical color-aware one. ## `wt list` also stops coloring a pipe `wt --help` documents `NO_COLOR` ("Disable colored output") and `CLICOLOR_FORCE` ("Force colored output even when not a TTY"). That second row only means something if color is off when stdout isn't a terminal — and `src/testing/mod.rs` sets `CLICOLOR_FORCE=1` in `STATIC_TEST_ENV_VARS` precisely so ANSI still appears in snapshots. But `wt list` wrote its escapes through std's macros, so it colored a pipe unconditionally and neither variable ever reached it. It now behaves as documented for the piped case: color on a terminal, plain on a pipe, `CLICOLOR_FORCE=1` to keep it on a pipe. `NO_COLOR` is fixed wherever `print_buffered_table` runs, which is the piped case plus `--no-progressive` on a terminal. It does **not** reach the default terminal rendering: `RenderTarget::detect` hands every tty that didn't pass `--no-progressive` to `ProgressiveTable`, which writes to a raw `std::io::stdout()` with each row's escapes already baked in. Closing that last gap means stripping SGR from the progressive rows while leaving the redraw's cursor control and the URL column's OSC 8 intact (`src/styling/hyperlink.rs` states `NO_COLOR` must not affect hyperlink support), so it's a separate change on the most visible surface in the tool. <details> <summary>PTY measurements</summary> On a terminal, with `CLICOLOR_FORCE` unset: | invocation | escapes | |---|---| | `wt list` (progressive, default) | 4590 | | `NO_COLOR=1 wt list` | 4663 — unchanged, the gap above | | `wt list --no-progressive` | 322 | | `NO_COLOR=1 wt list --no-progressive` | 0 | | `NO_COLOR=1 CLICOLOR_FORCE=1 wt list --no-progressive` | 0 — anstream checks `no_color()` first | Piped: 0 escapes by default, 17 under `CLICOLOR_FORCE=1`. Two earlier drafts of this description got this wrong in both directions, and @worktrunk-bot caught each. </details> The five changed snapshots all belong to `BareRepoTest`, whose `configure_wt_cmd` strips `CLICOLOR_FORCE` to capture "the terminal's plain output" and, until now, didn't get it. The diffs remove ANSI and change nothing else — same content, same column widths. ## Two surfaces keep the escapes For the statusline and `--help-page`, the pipe is a courier rather than the destination. A shell prompt or Claude Code captures the statusline and *renders* it; the docs pipeline converts the help page's escapes into HTML spans. Neither consumer is ever a tty, so anstream would strip them every time in production — and no test would catch it, because the suite forces color. They get `crate::output::println_verbatim!`, a sibling of `print_json` that writes the bytes through unchanged. It drops a `BrokenPipe` and still panics on any other write error, matching anstream, so both printers fail the same way on a full disk. Output is byte-identical: the help snapshots and `test_docs_are_in_sync` pass untouched. <details> <summary>Test, and two cleanups that fall out</summary> `test_stdout_surfaces_survive_a_closed_consumer` drives nine invocations across both printers with the child's stdout pipe closed before it is waited on, so the first write has no reader. That's deterministic rather than racing a consumer's exit, and `--version`'s few dozen bytes trip it as readily as the help page's 23KB — `EPIPE` is about whether a reader is attached, not about filling the buffer. Every case was checked to fail against the unfixed code. `statusline.rs` leaves `STDOUT_ALLOWED_PATHS`, since it no longer writes stdout directly; a stray `println!` there fails the guard again. `print_first_buffered_line` is gone. Its one caller is the `WORKTRUNK_FIRST_OUTPUT` benchmark hook, which wrote the same header line through a third path; a LineWriter flushes on the newline that measurement is timing, so the hook is unaffected. </details> > _This was written by Claude Code on behalf of max-sixty_ |
||
|
Maximilian Roos
|
0b27755726 |
fix(help): name the doc-entry-point command from clap, not an argv scan (#3762)
Three developer entry points — `--help-page`, `--help-description`, and
`--print-schema` — each carried its own copy of a scan that picked the
subcommand out of argv by rejecting entries that looked like the binary:
```rust
.filter(|a| *a != "--help-page" && !a.starts_with('-') && !a.ends_with("/wt"))
.find(|a| !a.contains("target/") && *a != "wt")
```
Neither `ends_with("/wt")` nor `contains("target/")` matches `wt.exe`
under a backslash path, so on every Windows install all three read the
binary's own path as the command. `--print-schema` exited 2; the other
two exited 0 with empty stdout and `Unknown command: C:\...\wt.exe` on
stderr. The scan also took the value of a value-carrying global as the
command, so `wt -C <path> list --print-schema` answered `No JSON schema
for '<path>'`.
clap had already parsed the same argv one call earlier.
`parse_early_globals` runs the real `Cli` definition with
`ignore_errors(true)` and reads `matches.subcommand()` to pick the
alias-splice context, and the comment above it already said that exists
"so the splice path in `augment_help` has no separate arg scanner". The
doc entry points just never asked it. It now carries the name in an
`EarlyGlobals` struct, and the three handlers take `Option<&str>`
instead of `&[String]`.
Error messages are unchanged: `src/cli/mod.rs` has
`#[command(external_subcommand)]` for user aliases, so a name the `Cli`
definition doesn't declare still arrives intact and can be named back in
`Unknown command: X`.
`--print-schema` now prints through `crate::output::print_json` rather
than `serde_json::to_string_pretty` plus std's `println!`. std's panics
with exit 101 when a consumer closes the pipe; anstream's drops the
`BrokenPipe`. This was the last JSON-to-stdout surface still panicking
after #3746 — the schema landed via #3747 while that PR was in flight,
and it lives outside `src/commands/`, where #3746's stdout guard doesn't
scan.
<details>
<summary>Why CI never caught this</summary>
`tests/integration_tests/help.rs` and
`tests/integration_tests/readme_sync.rs` are both
`#![cfg(not(windows))]`, and on Unix the test binary is always named
`wt` under `target/`, which is exactly the shape the scan was written
against. The new test drives all four cases through a symlink named
`wt.exe` — a symlink rather than a copy because the debug binary is
67MB. Verified to fail against the pre-change code.
</details>
`wt merge --help-page` and `wt merge --help-description` still panic on
a closed pipe. Those use std's `println!`/`print!` deliberately, to
preserve the ANSI codes anstream's would strip, so they need a different
mechanism than this one. Left for a follow-up.
> _This was written by Claude Code on behalf of max-sixty_
|
||
|
Jude Wang
|
3817df0795 | fix(plugin): resolve WorktreeRemove against the worktree path, not the project dir (#3754) | ||
|
Maximilian Roos
|
860fa02ecf |
fix(output): don't panic when a --format=json consumer closes the pipe (#3746)
`print_json` — the pretty-JSON printer behind `--format=json` — lived in
`src/commands/list/mod.rs`, under the one command that happened to need
it first, while `wt list statusline` and `wt config approvals list`
reached across for it. It now lives in `src/output/json.rs`, re-exported
as `crate::output::print_json`.
Moving it surfaced thirty other call sites that had each open-coded the
same two lines, and that those two lines were not equivalent everywhere.
Four printed through anstream's `println!` (via `worktrunk::styling`),
the other twenty-six through std's. anstream drops a `BrokenPipe` write
error; std panics. So on main today:
```console
$ wt config state get --format=json | head -3
{
"ci_status": [
{
thread 'main' panicked at library/std/src/io/stdio.rs:1165:9:
failed printing to stdout: Broken pipe (os error 32)
```
while `wt config show --format=json | head -3` exits cleanly — same
command family, different behavior, decided by which macro happened to
be imported in that file. `EPIPE` depends on whether a reader is still
attached, not on how much is written, so this is a race rather than a
size threshold: an output that fits the pipe buffer usually lands before
the consumer goes away, which is why it stayed hidden rather than why it
is safe. A three-byte write panics just as readily once the reader has
closed.
All thirty-six call sites now go through `print_json`, and `print_json`
prints through anstream, so no `--format=json` surface panics on a
closed pipe. The command above now exits 0 with an empty stderr; also
checked by hand on `wt list`, `wt step prune --dry-run`, `wt config
show`, `wt step eval`, `wt config approvals list`, and `wt list
statusline`.
`wt switch --format=json` is the one surface that isn't a `print_json`
caller: it emits its single result as one compact line, which is what a
shell loop reads, so converting it would change a machine-readable
format for no gain. It gets the same anstream printer instead, which is
the part that matters here.
`wt list statusline --format=json` had a third instance hiding in its
schema-1 empty path, a bare `println!("[]")` on std's macro — on the
surface that runs on every prompt redraw. It now serializes an empty
array through `print_json`, which emits the same two bytes;
`test_statusline_json_outside_worktree` already asserted them. A
module-level `println` import would have been wrong there, since the
text statusline's `println!` deliberately bypasses anstream so its
pre-rendered ANSI survives a non-tty stdout.
Pruned eight now-dead entries from `STDOUT_ALLOWED_PATHS` in
`tests/integration_tests/output_system_guard.rs`. Centralizing these
writes left seven allowlisted files with no direct `println!` at all, so
their exemptions covered no code and the guard quietly stopped
protecting exactly the files this branch touched; a stray `println!` in
`merge.rs` or `remove.rs` fails the test again. (`alias.rs`'s entry was
already dead.)
Serialization is otherwise byte-identical: serde escapes control
characters, so raw ANSI never reaches this output and anstream's
stripping is a no-op on it.
`src/commands/CLAUDE.md` gains one line under "Adding a CLI Command"
stating the rule — stdout through anstream, `print_json` as the printer,
switch's compact line as the one shape difference — so the next JSON
surface doesn't open-code it again.
Two `print_json` calls the diff rewrote were uncovered on the base
commit, which pulled their misses into `codecov/patch` without any
behavior changing. Rather than hand over a waiver, both now have tests:
`wt remove --format=json` with no branch argument (the single-worktree
path, distinct from the named removals already tested) and `wt step
for-each --format=json` interrupted mid-loop, which still owes its
consumer the results collected before the signal. Each was
mutation-checked — deleting the line under test fails it — because a
one-item array is what both the abort and completion paths produce, so a
weaker assertion would pass either way.
> _This was written by Claude Code on behalf of max-sixty_
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
7f8ac8e5d7 |
feat(list): publish a JSON Schema for the schema-2 envelope (#3747)
`wt list --format=json` schema 2 now has a published, machine-readable contract at [worktrunk.dev/schema/list-v2.json](https://worktrunk.dev/schema/list-v2.json). The schema was already derived — `test_schema_generates` built one, asserted it compiled, and threw it away, with a comment saying "until the schema export ships." This ships it: `wt list --print-schema` prints the document (a developer entry point alongside `--help-page`, intercepted before clap), and a new step in `test_docs_are_in_sync` commits it to `docs/static/schema/list-v2.json`, the same generate-and-commit pattern as `llms.txt`. It shells out rather than calling `schema_for!` because `JsonEnvelope` lives in the bin-only `crate::commands` tree. Two things had to be fixed for the document to be usable. **The contract.** `schema_for!` generates under schemars' *deserialize* contract, which marks a `skip_serializing_if` field required — nothing supplies it on the way in. The first document I generated therefore required `default_branch`, `upstream`, `pr`, `checks`, `summary` and `vars` on every item, all of which the absence rule routinely omits, so it rejected the output it documents. Generating under `for_serialize()` fixes it. **The vocabularies.** Four fields — `checks.status`, `display.state`, `default_branch.integration.reason` and `worktree.operation` — were `&'static str`, so the schema described them as bare strings. They are now `JsonCheckStatus`, `JsonMainState`, `JsonIntegrationReason` and `JsonOperation`, each converted from its domain enum by an exhaustive match, so a new `CiStatus`, `MainState`, `IntegrationReason` or `InProgressOperation` variant is a compile error rather than a value silently missing from the published vocabulary. **The emitted JSON is unchanged**; the existing envelope snapshot passes untouched. <details> <summary>Before and after, for one item</summary> ```json // before — rejects its own output, and loses the vocabulary "required": ["default_branch", "upstream", "pr", "checks", "summary", "vars", "display"], "status": { "type": "string" } // after "required": ["branch", "head", "display"], "status": { "enum": ["passed", "running", "failed"] } ``` </details> ## Testing `test_schema_accepts_envelopes` validates a battery — every `CiStatus` over both sources, every `MainState`, a populated worktree row, an integrated row with an upstream and a dev server, plus the absent and null arms of the absence rule — against the same document `--print-schema` emits. Validating proves nothing about a type the battery never instantiates, so the test also pins every non-`Nullable_` type in the document to a path that must carry a non-null value. A new `Json*` type fails until the battery reaches it, and a row that stops populating one fails too — the check reports the type names rather than leaving the gap to a reader. This needed a `jsonschema` dev-dependency: schemars only generates, and derives the document from the types without ever seeing an envelope, so nothing otherwise tied the two together. The test was confirmed to fail on the bug it exists for. Reverting to `for_deserialize()` makes it report `pr`, `checks`, `summary`, `vars` and `display.columns` as wrongly required. The dependency is dev-only: `reqwest`, `rustls` and `async-trait` stay unselected so no HTTP stack comes along, and `cargo tree --package worktrunk --edges normal -i jsonschema` finds no path to it. One direction it deliberately does not cover: a *loosening*. If a field reverted to `&'static str` the schema would say `type: string` and anything would validate. That direction is held by the compiler instead, via the exhaustive matches. ## Notes for review - `schema_document()` lives in `json_v2.rs` beside the types, not in `help.rs`, so `--print-schema` and the test compile the same document rather than two constructions that could drift on the contract setting. - The lychee exclusion for `worktrunk.dev/schema/` follows the entry directly above it: a generated link that 404s until the site deploys. > _This was written by Claude Code on behalf of max-sixty_ |
||
|
Worktrunk Bot
|
bdd7113c95 | fix(shell): catch a failing --execute body in the nushell wrapper (#3734) | ||
|
Worktrunk Bot
|
78010166bd |
fix(shell): bypass rm aliases in the nushell wrapper's cleanup (#3732)
## Problem The nushell wrapper's temp-file cleanup called a bare `rm -f`. Nushell resolves aliases at parse time, and `config.nu` runs before the `$nu.vendor-autoload-dirs` file the wrapper installs into — so a user's `alias rm = ...` is already in scope when the wrapper's `def` is parsed, and intercepts the cleanup. #3714 fixed the same shadowing in the bash and zsh wrappers with `command rm`; nushell has no `command` builtin, so it was left out. Two failures, both verified end-to-end against nushell 0.114.1 (the version CI pins) driving the real rendered wrapper: | `alias rm =` | before | after | |---|---|---| | *(none)* | exit 0, stdout returned, 0 temp files left | unchanged | | `^false` | exit 1, **stdout empty**, **3 temp files leaked** | exit 0, stdout returned, 0 left | | `^echo TRASHED` | `TRASHED -f /tmp/...` lines **injected into stdout**, **3 temp files leaked** | exit 0, stdout returned, 0 left | The `^false` row is the serious one. Cleanup sits between `let output = (open $stdout_file --raw)` and the function's return, and nushell 0.98+ raises `ShellError` on a non-zero external exit — so an `rm` alias that fails, prompts, or isn't installed on that machine aborts the wrapper before it returns, and the command's stdout is silently discarded. Only stdout is affected; stderr has already streamed to the terminal, which is why the symptom hides on `wt switch` (its output is on stderr) and shows on `wt config show`. ## Solution Consolidate the two cleanup calls into one, after the last read of any temp file, and branch at runtime on `$nu.os-info.family`: ```nu if $nu.os-info.family == "windows" { try { rm -f $cd_file $exec_file $stdout_file } } else { try { ^rm -f $cd_file $exec_file $stdout_file } } ``` `^rm` bypasses alias expansion entirely — the Unix fix is complete, not partial. Windows keeps the builtin because `^rm` is external-only and Windows has no external `rm`; `try` covers both branches so cleanup can never abort the wrapper again. Branching at *runtime* rather than forking the template by build platform is what keeps this cheap: `wt config shell init nu` renders one text on every platform, so there's a single `init_nu` snapshot and no platform-specific rendering to maintain. <details><summary>Options considered and rejected</summary> - **`` `rm` `` (backtick-quoted)** — bypasses the alias, but `` `rm` `` with `PATH` emptied reports ``Command `rm` not found``, and `` `rm` `` with no args prints `/usr/bin/rm: missing operand`. It resolves to the *external*, so it's just `^rm` spelled differently — same Windows gap, no benefit. - **`hide rm`** — works inside the `def`, but `hide` is a parse-time keyword that leaks into the enclosing scope. For an autoloaded file that scope is the user's session, so installing shell integration would silently unbind their own `rm` alias. Same leak inside a `do {}` closure. Worse than the bug. - **`try` alone, no `^rm`** — cross-platform and kills the abort, but leaves the alias running: a `trash` alias still trashes worktrunk's temp files on every invocation, and an `rm -i`-style alias still prompts. - **Forking the template by build platform** — same end state on each platform, at the cost of platform-specific `init_nu` snapshots. The runtime branch gets there without them. </details> ## Testing `test_nu_wrapper_cleanup_survives_rm_alias` in `tests/integration_tests/shell_wrapper.rs` runs the real wrapper through a PTY with `alias rm = ^false` declared ahead of it, against a dedicated `TMPDIR`, and asserts both symptoms: the wrapper's stdout comes back non-empty, and the temp dir is empty afterwards. It fails on the current template (marker absent — the wrapper aborted) and passes with the fix. Also run locally: `cargo test --lib --bins`, and the full `shell_wrapper` + `config_show` + `test_docs_are_in_sync` integration set with `--features shell-integration-tests` (201 passed). ## Not fixed here A user alias to a *custom command or builtin* whose signature rejects `-f` — e.g. `alias rm = print "…"` — makes the whole wrapper file fail to parse, leaving `wt` undefined rather than merely broken. That's pre-existing (today's template has two bare `rm -f` calls) and survives this change, because the Windows branch is still parsed on Unix even though it never runs. Eliminating it means emitting no bare `rm` at all on Unix, which is the template fork above. Flagged on #3730 rather than bundled in here. --- Closes #3730 — automated triage Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Maximilian Roos
|
970976bd32 |
Consolidate benchmark recipes and cases (#3721)
Benchmark fixtures had accumulated around individual call sites, leaving the same repository shapes and command modes expressed several ways. This change makes repository state the organizing concept: benchmark groups select semantic `FixtureRecipe`s, share table-driven cases, and retain separate fixtures only when a controlled contrast, destructive precondition, or disproportionate setup cost requires one. The real-repository list benchmarks now share one pinned `rust-lang/rust` fixture with eight worktrees and fifty branches spread across history. The list matrix keeps default, branch, warm, and cold coverage without maintaining several “real” repository handles. Remove and prune cases share the same case machinery, while the destructive large-repository prune state remains separate. The scheduled workflow now converts Criterion estimates directly with `jq`, removing the one-off Python converter and its tests. The benchmark guide records the canonical-fixture principle and the remaining recipe-to-group mapping. Tests: `cargo run -- hook pre-merge --yes` (4,551 tests); `cargo bench --bench list large_repository -- --test`; `cargo test -p wt-perf`; benchmark check, clippy, formatting, and diff checks. > _This was written by Codex on behalf of max-sixty_. |
||
|
Worktrunk Bot
|
2203c414f5 |
fix(docs): convert a config-example link whose text holds a bracketed span (#3731)
`wt config create --project` writes a comment into the user's `.config/wt.toml` — and `wt config create --help` prints the same text — carrying a raw, unresolvable Zola link: ``` # When many repositories share one self-hosted host, name it once in user config with a [pattern-keyed `[projects]` entry](@/config.md#user-project-specific-settings) instead of repeating this block in each repo. ``` Every other cross-reference in that file is a plain URL (`… see \`wt hook\` (https://worktrunk.dev/hook/) …`), because `transform_config_source_to_toml` converts the `after_long_help` markdown to plain text on the way into `dev/wt.example.toml`. This one link isn't converted: `convert_markdown_links_for_config` matched link text with `[^\]]+`, which stops at the first `]` — here the one closing the nested `` `[projects]` `` span — so the regex failed to match and the markdown survived verbatim. The line arrived with #3701; it's the only link in either generated example file with a bracketed span in its text. ## The fix **One rule for `]` in link text.** `ZOLA_LINK_PATTERN`, earlier in the same file, already solves this problem for the skill mirrors — it alternates a backticked code span with any non-`]`-non-backtick char, which is why `skills/worktrunk/reference/config.md` renders this very sentence with a resolved URL while the TOML example didn't. `convert_markdown_links_for_config` now uses that same class rather than a second, weaker one. Brackets in these link texts always sit inside a code span, so the class fits the shape exactly, and it covers `[[…]]` array-of-tables names as well — these sections already document `[[projects."…".post-start]]` pipelines, so a link naming one is the next form to arrive. Regenerating produces the intended form: ``` # When many repositories share one self-hosted host, name it once in user config with a pattern-keyed `[projects]` entry (https://worktrunk.dev/config/#user-project-specific-settings) instead of repeating this block in each repo. ``` **A shape the regex declines now fails loudly.** Widening the class fixes the shapes we know about; it can't fix the next one. `finalize_skill_content` already handled that risk with a guardrail — after the rewrite it scans for a stray `](@/…md` and panics with the offending line, precisely because "the regex declined on an unexpected character in the link text" is the expected failure mode. `transform_config_source_to_toml` had no equivalent, which is why this one reached `dev/wt.example.toml` and the `--help` output. That check is now extracted into `assert_no_untransformed_zola_links` and called from both surfaces, so the next unsupported shape is a test failure naming the line rather than a raw `@/config.md` target in a user's config file. ## Why nothing caught it `test_project_config_source_generates_example_toml` compares `dev/wt.example.toml` against the output of this same transform, so an unconverted link is "in sync" by construction — the sync test can't see the difference between a link that converted and one the regex declined to match. Two tests close that gap: - `test_config_markdown_links_convert_to_plain_text` asserts the transform's output directly. It fails on `main`'s regex with exactly the reported symptom, and pins the forms already working (Zola page, Zola page + anchor, absolute URL, two links on one line, the `[[…]]` array-of-tables name) plus the case that must *not* convert — a bare `` `[forge]` `` span is not a link and has to survive verbatim. - `test_untransformed_zola_link_fails_the_config_transform` covers the backstop itself: an unbalanced backtick in link text makes the rewrite decline, and the assertion turns that into a panic naming the line. ## Files - `tests/integration_tests/readme_sync.rs` — the shared link-text class, the guardrail extraction and its second call site, and both tests. - `dev/wt.example.toml` — regenerated by the sync test (one line). - `tests/snapshots/…help_config_create.snap` — the same line, as `wt config create --help` renders it. Ran locally on the final state: `readme_sync::` (15), `test_help` (47), `cargo clippy --tests --all-features`, and `cargo fmt --check`. All green; the generated files are byte-identical under the new class, so the sync tests pass without regenerating. The full gate runs in CI. --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Worktrunk Bot
|
d4538f2047 |
fix(alias): carry the shell's cwd into alias and hook bodies (#3724)
## Problem Since #939 / #3344, `wt switch` and `wt remove` preserve the user's subdirectory position — from `monorepo.feature/subproject/` you land in `monorepo/subproject/`. #3723 reports that this is lost one layer down: with `[aliases] finish = "wt remove -y"`, `wt finish` drops you at the primary worktree root. The resolution reads the user's position from the wt process's own cwd (`resolve_subdir_in_target`, called with `std::env::current_dir()`). That answers "where is the user standing?" only for a top-level invocation. Alias and hook bodies run with the worktree root as their working directory, so the nested `wt` strips the source root off a cwd that *is* the source root, gets an empty relative path, and falls back to the destination root. ## Solution The CD directive file already travels to exactly the children that are allowed to move the user's shell. The shell's directory now travels with it: `apply_cd_directive_env` sets `WORKTRUNK_SHELL_CWD` wherever the CD file is re-added (`Cmd::stream` and the concurrent runner), and `scrub_directive_env_vars` strips it alongside the other directive vars, so an untrusted child neither keeps nor receives it. `shell_exec::shell_cwd()` reads it back, preferring the inherited value over the process cwd — which is what makes nesting compose, since each layer forwards the shell's directory rather than its own. Three sites ask that question and now go through it: `wt switch`, `wt remove` (`prepare_remove_directory_change`), and `wt step relocate`, whose existing comment already asks to behave identically to the other two. Nothing about the working directory of alias or hook *bodies* changes — `{{ cwd }}` is still the worktree root, per the documented contract. The only change is what a nested `wt` believes about the user's position. ## Testing Two integration tests in `tests/integration_tests/step_alias.rs`, both failing before the change with the exact symptom reported: ``` CD file should preserve the subdirectory (…/repo/apps/gateway), got: "…/repo\n" ``` - `test_alias_wrapping_remove_preserves_subdir` — the reported case (`[aliases] finish = "wt remove -y"` run from `feature/apps/gateway`). - `test_alias_wrapping_switch_preserves_subdir` — the same for `wt switch` inside an alias. The existing subdirectory-preservation tests in `directives.rs` (including the fall-back-to-root cases) still pass, as does the full integration suite — apart from `test_copy_ignored_preserves_file_executable_permissions`, which fails identically on `main` in this sandbox (umask `0002`, expects `0644` gets `0664`) and is unrelated. The open question from the first revision is answered: the new remove test leaves two processes with a cwd inside the worktree being removed (the alias parent in the subdirectory, the nested `wt` at the root) where the existing test has one, and `test (windows)` passes on it. Review follow-ups are in |
||
|
Worktrunk Bot
|
e898a10154 | test(picker): quiesce background fills before arming the awaited preview key (#3726) | ||
|
Aral
|
e949ed5fda | fix(shell): use command rm in wrapper cleanup to bypass rm aliases (#3714) | ||
|
Maximilian Roos
|
a3daf0ded6 |
fix: strip only the executable suffix when naming wt from argv[0] (#3719)
`mock_stub::command_name` and `invocation::binary_name` both read `argv[0]` to name this process, and disagreed on how. The mock dispatch stripped `EXE_SUFFIX`; `binary_name` took `file_stem`. So `wt.old` resolved to `wt.old` in one and `wt` in the other, while `command_name`'s doc comment asked that the two be kept aligned. Follow-up from #3712. The comment was right about which way to align. `binary_name`'s own doc already claimed the narrower behavior ("On Windows, strips `.exe`"), and `file_stem` doesn't do that — it cuts at the last dot wherever the dot is, on every platform. `validate_shell_command_name` accepts `.`, so `wt.old` is a name a user can install shell integration for, and `wt config shell init bash` under it emitted a wrapper for `wt`, a command they may not have. The `EXE_SUFFIX` strip on the mock side is load-bearing (it replaced `file_stem` so a dotted mock name resolves alike on Unix `python3.11` and the Windows link `python3.11.exe`), and it is the right answer for shell integration too. There is now one derivation, `path::executable_name`, in the lib where both crates reach it: `argv[0]`'s file name with `EXE_SUFFIX` stripped, matched case-insensitively, since Windows resolves `WT.EXE` and `wt.exe` to one file and `compute_shell_warning_reason`'s Windows branch needs `wraps` to be the suffix-free spelling to tell the user to drop it. `binary_name` is that plus its `"wt"` fallback; `mock_stub` calls it directly, and `command_name` is gone along with the comment asking for alignment. The shell-integration warning still derives its own display name from `argv[0]` and deliberately keeps the `.exe`; the doc says so, so it doesn't read as a third caller someone should fold in. Two behavior changes beyond the dotted name. A non-UTF8 `argv[0]` converts lossily rather than falling back to `"wt"`, so `wt config shell init` rejects it with `Invalid shell integration command name` instead of quietly generating integration for a command other than the one that ran — the rule the existing `wt;touch` symlink test already enforces. A missing `argv[0]` still yields `"wt"`. The suffix is a parameter of the private `strip_suffix_ignoring_case` rather than read from `env::consts` in place: `EXE_SUFFIX` is empty on Unix and the merge gate runs one platform, so the unit test drives the Windows spellings (`WT.EXE`, `wt.Exe`, `wt.exe.old`, a name whose trailing bytes fall mid-character) everywhere. `str::get` rather than a byte slice for the same reason `shell::extract_filename_from_path` should use one — see below. Tests: the derivation table and the suffix cases in `src/path.rs`, plus two integration tests beside the existing `argv[0]` ones — a `wt.old` symlink whose `config shell init bash` must define `wt.old()` and not `wt()`, and a non-UTF8 `argv[0]` that must be rejected. The three existing `argv[0]` tests are untouched and pass. Pre-merge gate green, 4548/4548. Not done here: `shell::extract_filename_from_path` is a third `.exe`-stripping name derivation, over `$SHELL` and process-tree names. It stays separate because it strips `.exe` on every platform, which is correct for a Git Bash `$SHELL` carrying a Unix-form path with a Windows suffix on it. It does have a latent panic — `filename[filename.len() - 4..]` slices without a char-boundary check, so a `$SHELL` of `/bin/€ab` panics — worth a small separate fix. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
dd453304ed |
test: fold the mock stub into the wt binary (#3712)
`cargo test --test integration` neither built nor rebuilt `mock-stub`: a
target filter deselects the dummy test that pulled it in, so a fresh
tree panicked ("mock-stub binary not found") and a warm one could run a
stale stub. The chain that compensated — the separate helper package,
its dummy `builds.rs`, the `default-members` entry, nextest's
experimental `build-bins` setup script, `workspace_bin()` — existed only
because cargo-dist ships every `[[bin]]`, and carried a TODO to collapse
once that changed. dist 0.30.2 does support per-binary exclusion now
(`[dist.binaries]`, since 0.29.0; verified with `dist plan`), but the
TODO's plan has a hole it predates: `cargo install worktrunk` installs
every feature-satisfied `[[bin]]`, and dist config doesn't govern
crates.io installs.
So the mock commands are now the `wt` binary itself. `mock_commands`
links `wt` under the mock's name (`gh`, `glab`, …), and `main()`
dispatches to the ported playback (`testing::mock_stub`) when
`WORKTRUNK_TEST_MOCK_CONFIG_DIR` is set, argv[0] is a foreign name, and
the config dir holds `<argv0>.json` for it. The existence check keeps wt
under a foreign argv[0] *without* a config being wt — the
argv0-validation security test symlinks it as `wt;touch` and must reach
wt's own rejection. The shipped binary already compiles the whole
`testing` module unconditionally, so this adds no new category of test
code to it. Windows links `wt.exe` with `hard_link` (copy fallback for
cross-drive dirs); the debug binary is ~67 MB, so per-mock copies stay
the fallback.
Every runner is now safe by construction — cargo rebuilds a package's
own binaries whenever its integration tests build, so there is no
separate artifact to go missing or stale. Deleted: the helper package,
the setup script plus `experimental = ["setup-scripts"]`, the
`default-members` trick, `workspace_bin()`, and `wt_bin()`'s dead
compile-time branch (unit-test targets get neither the runtime variable
nor the `option_env!` value, so the runtime resolution is the one
mechanism).
Validated locally: the pre-merge gate's full suite passes (4542/4542;
its doc step also caught unescaped `argv[0]` intra-doc links in the new
comments, fixed and `cargo doc -Dwarnings` re-verified). With all stub
artifacts purged from `target/`, plain `cargo test --test integration`
on mock-dependent tests builds them and passes — the command that used
to hit the trap. Two integration tests pin the dispatch's argv[0] edges
(an empty argv[0], and a non-UTF8 one), alongside the existing
`wt;touch` carve-out test.
The first commit is the investigation that preceded the fix: it verified
the `wt` binary itself was never subject to the staleness the mock-stub
was, and documented that in `tests/CLAUDE.md`; the fix then narrows that
paragraph further, since the gap it scoped no longer exists.
A two-reviewer subagent round (one prosecuting the diff against the
failure modes documented in the repo's own mock history — the #401/#407
Windows era, #547, #654, #127, #2544, #2730, #2744 — the other
adversarial) then hardened the dispatch. The reserved-name guard is
case-insensitive, matching the config probe, which goes through a
filesystem that equates `WT.json` with `wt.json` on macOS and Windows;
`command_name()` reads `args_os` — `env::args()` panics on a non-Unicode
argument, and this runs inside `main()` on every invocation (caught by
the tend review) — and returns `None` for a degenerate argv[0] instead
of panicking; the `.exe` suffix is stripped explicitly rather than via
`file_stem`, so a dotted mock name (`python3.11`) resolves identically
on every platform; and `copy_mock_binary` is now private —
`MockConfig::write` writes `<name>.json` before linking and is the only
way to create a mock, so a link cannot exist without its config, and the
dispatch's missing-config fall-through can only mean "wt under a foreign
name" (the argv0-validation tests' `wt;touch`), never a half-configured
mock that silently runs real wt with the mocked tool's arguments. The
`Option<&str>` mock helpers whose `None` arm produced exactly such
configless links lost the arm (every caller passed `Some`), and 25
redundant standalone link calls went with it.
The review also surfaced the one remaining spawn-a-stale-binary path
outside the suite: `wt-perf timeline` resolved a sibling `wt` by path,
checked only existence, and told the user to build it manually — so
`cargo run -p wt-perf -- timeline` after a `src/` edit silently measured
stale code. It now builds `wt` first and takes the artifact path from
cargo's `--message-format=json` report rather than deriving a sibling
location, so target-dir and profile overrides can't divert the build
away from where it's resolved; a release wt-perf builds and measures a
release wt. The build runs before the timeline's wall-clock measurement
starts, cargo's progress streams on stderr, and stdout keeps the
`--chrome` JSON contract.
> _This was written by Claude Code on behalf of max-sixty_
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
305f8fcd19 |
fix(gitea): key tea api errors on the HTTP status line, not the response shape (#3713)
`tea api` copies the response body to stdout and exits 0 whatever the
HTTP status, so both Gitea call sites guessed from the body: a `message`
key meant the request had failed, anything else was the resource. `tea
api --include` writes the status line and headers to stderr and leaves
stdout the bare body, so the status is available after all.
## Availability
`--include` shipped with the `api` subcommand itself in **v0.12.0** —
`cmd/api.go` doesn't exist at v0.11.1 — and the implementation is
byte-identical through v0.15.1. A `tea` old enough to lack the flag has
no `api` to send it to, so nothing that can make this call rejects it.
tea's own `api_test.go` passes flags ahead of the endpoint (`{"-d",
"@file", "/test"}`), which settles the argument order.
## What the status buys
Two things the shape check couldn't do:
**A body Gitea didn't write is now an error.** A reverse proxy's HTML
page has no `message` key, so it used to read as the resource — and `wt
list` then logged `Failed to parse tea api pulls JSON`, blaming a Gitea
API change for a gateway that never reached Gitea. Verified by restoring
the old key and re-running: `proxy_page_is_an_error` fails against it
and passes with the status.
**Retriability stops being a text sniff.** `is_retriable_error` was
looking for `429` or `connection` somewhere in Gitea's prose, which
never carries the code. 429 and 5xx are now the server saying "later";
every other 4xx is a token or a missing resource that repeating the call
won't change. `gh` and `glab` still sniff because they bury the status
inside prose.
Visible change: a blanked 500 shows the error indicator where it used to
paint the same blank cell as a healthy branch with no CI.
`gitea_blanked_500` and `gitea_not_found` pin both halves of that
contrast.
## The body's three states
The status decides; the body only supplies text, and its three states
are three different things to say — so `api_error_message` reports all
three rather than flattening them. The switch path prints the code in
its headline:
```
✗ Gitea API error 404 for PR #9999 on owner/test-repo: pull request does not exist [index: 9999]
✗ Gitea API error 500 for PR #101 on owner/test-repo, but the response carried no message — Gitea hides 5xx messages from non-admin tokens
✗ Gitea API error 502 for PR #101 on owner/test-repo, and the response body is not one Gitea sends
```
An exit-0 `tea` that writes no status line is a backstop rather than a
fall-through: `--include` prints the moment the response does, so its
absence means nothing arrived, and reading stdout as the resource there
is the failure the status line exists to prevent.
`switch_pr_gitea_no_status_line` covers it.
With the CI path no longer reading the message, `api_error_message`
drops to module-private.
## Tests
Every mock `tea` response now carries the two streams a real one writes,
via `tea_api_include_stderr`. `setup_mock_tea` takes the status
explicitly, so each test states the HTTP response it stands for instead
of leaving it implied — which also collapses four hand-rolled inline
mocks into it.
## Cost
`--include` writes the whole header block, not just the status line, so
captured stderr now holds whatever the server returns — a `Set-Cookie`
among it, on a Gitea that issues one. That reaches disk only under
`-vv`, which logs every subprocess's output to `subprocess.log`. There's
no narrower flag, and the status isn't available any other way; it's
noted in the module docs.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
> _This was written by Claude Code on behalf of Maximilian_
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
10e8bad2c5 |
fix(gitea): key the tea api error shape on the envelope, not on a non-empty message (#3600)
Gitea blanks a 500's message in production unless the token belongs to
an admin (`services/context/api.go`: `if setting.IsProd && !(ctx.Doer !=
nil && ctx.Doer.IsAdmin) { message = "" }`), so the body arrives as
`{"message":"","url":"…/api/swagger"}`. Both Gitea call sites required a
*non-empty* message before they would call a response an error, so that
body fell through to the data path:
- `wt list` logged `Failed to parse tea api pulls JSON`, once per branch
— blaming an API change for an API error. The CI cell was blank either
way, and the stderr layer is off at the default verbosity, so this
reached whoever was already debugging with `-v` or `RUST_LOG`.
- `wt switch pr:<n>` failed with `Failed to parse Gitea API response for
PR #N. This may indicate a Gitea API change.` — the same misattribution,
in the user's face.
## The key
The discriminator is now the presence of the `message` key, which is the
whole shape. `tea api` never reads the HTTP status — `runApi` copies the
body to stdout and returns nil regardless — so the body is the only
channel, and none of the resources read here carries a `message`:
verified against gitea `main` for `PullRequest`, `CombinedStatus`, and
`CommitStatus`.
`url` is deliberately *not* part of the key. The two mistakes aren't
symmetric: an error body that omits `url` would read as the resource,
which is the bug this key exists to prevent, while requiring it would
only guard against a resource one day growing a `message` field. Gitea
already ships error shapes without one (`APIInvalidTopicsError` is
`message` plus `invalidTopics`), and `CombinedStatus` *does* carry
`url`.
## Shape
One parser — `remote_ref::gitea::api_error_message` — now serves both
sites; the CI-status backend drops its duplicate struct, and the PR path
checks the envelope before the resource parse, so "may indicate a Gitea
API change" is reserved for a body that is neither. A blanked message
reaches the user as an error that says why there's no detail:
```
✗ Gitea API error for PR #101 on owner/test-repo, but the response carried no message — Gitea hides 500 messages from non-admin tokens
```
The CI cell for a blanked 500 stays blank, unchanged:
`is_retriable_error` is the one gate every backend uses to turn a
failure into the error indicator, and an empty message isn't retriable.
Only the misleading log line is gone.
## Tests
Extended rather than duplicated:
`test_tea_api_error_reads_the_response_shape` flips its blank-message
assertion (that assertion *was* the bug), a new
`test_api_error_message_reads_the_response_shape` pins the key against
every shape both sides see, and `switch_pr_gitea_blank_error_message`
snapshots the user-visible path alongside the existing 401/403/404
cases.
`test_list_full_gitea_parse_warning_is_reserved_for_unknown_bodies`
covers the PR-list side end to end. It drives `wt list --full` under
`RUST_LOG=warn` and reads stderr rather than snapshotting, since the
warning is a `tracing` record that the stderr layer suppresses at the
default verbosity (`-v` would turn it on but bury it under a template
expansion per worktree). A second case sends a body that is neither the
resource nor the envelope, where the warning does belong — that pins the
wording to unknown shapes and keeps the first case from passing merely
because nothing logs. Checked against the pre-fix key: the blank case
fails there with the misleading warning, the unknown case passes.
## Noted, not done
tea's `api` command has an `--include` flag that writes the status line
to stderr — a genuine structured channel that would beat shape-sniffing.
It's in tea's `main` source but in no released changelog entry, so
sending it would break every user whose `tea` predates it. It would also
compose with this change rather than replace it, since the body still
supplies the message text.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
> _This was written by Claude Code on behalf of Maximilian_
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
5c42c5b7d5 |
feat: machine-readable approval state and branch-removal outcomes (#3710)
Two of the five machine-readable-output requests NathanaelRea opened (#3696–#3700), reviewed as a set and implemented where the gap was real. ## `wt config approvals list --format=json` (#3698) The command already computed the four distinctions an orchestrator needs — no commands, approved, approval-required, and stale — read-only, without prompting or writing. It had no `--format` flag, so the only way to learn that a non-interactive run would stop for approval was to run the operation and catch `NotInteractive`, or to pass `--yes` and approve whatever was there. ```json { "state": "approval_required", "commands": [ {"phase": "post-start", "name": "dev", "template": "npm run dev", "approved": false}, {"phase": "pre-merge", "template": "cargo test", "approved": true} ], "stale": ["some removed command"] } ``` `state` is what a caller branches on. `stale` stays a separate list rather than a fourth `state`, because it co-occurs with all three — and those are the approvals `--yes` would silently re-approve after their command template changed, which is exactly what an orchestrator preserving the approval model needs to see. A flag on the existing read command rather than a new `status` verb, matching `wt config show`, `wt config state get`, `wt config state logs`, and `wt list`. Closes #3698. ## `branch_outcome` on removal (#3700, partly) `wt remove --format=json` reported the branch as one boolean, collapsing five internal outcomes into two values: | Internal outcome | `branch_deleted` was | |---|---| | `Deleted` | `true` | | `Deferred` — handed to a detached process, result never observed | `true` | | `NotAttempted` — no branch, or `--no-delete-branch` | `false` | | `Retained` — a sibling worktree has it checked out | `false` | | `Retained` — **the CAS refused; the ref moved under us** | `false` | The last row is the exact race #3700 asks for protection against. Worktrunk already deletes with `git update-ref -d <ref> <oid>` and already fails closed when the ref has moved — then reported it as the same `false` that means "you asked me not to". And `Deferred` reported `true` on intent. `branch_outcome` names it instead: `deleted`, `deferred`, `not_attempted`, `retained_unmerged`, `retained_checked_out`, `retained_raced`, `retained_failed`. A caller that sees `retained_raced` knows to re-read the ref and retry, which is what the guard detects it for. **This does not close #3700.** That issue asks for an *input* — a caller-supplied expected OID that makes `wt` fail closed against the orchestrator's own observation. This is an *output*. They land in the same place on the default path, because the integration check already refuses to delete unintegrated content, so the caller was never going to lose commits — they just couldn't classify the refusal. Where the gap is real is `--force-delete` / `-D`, which takes the early return in `delete_branch_if_safe` and runs `git branch -D` with no integration check and no CAS. If an `--expected-oid` flag lands, it has to gate that path. ## Notes - **Output-format break.** `branch_deleted` is replaced, not supplemented, on `wt remove --format=json` and on `wt step prune --format=json`'s live path. Per CLAUDE.md, output formatting is on the flexible side of the interface line; flagging it here so the release changelog picks it up. - **`wt step prune --dry-run` keeps `branch_deleted`.** A dry run predicts; it runs nothing to have an outcome. Different thing, different name, documented as such. - **`retained_raced` and `retained_checked_out` have no deterministic CLI trigger.** Both come from windows between `wt`'s own fresh read and the ref mutation, which no hook can be scheduled inside. They're covered at the unit level (`branch_fate_from_result_mapping`, `branch_fate_json_outcome_is_distinct_per_fate`, and `cas_rejects_delete_when_branch_advances` in `src/git/remove.rs`, which drives the race with a stale snapshot). The integration tests cover the two reachable contrasts: `retained_unmerged` via a `pre-remove` hook that commits, and `not_attempted` via `--no-delete-branch`. - **`print_json` lives under `src/commands/list/`** and now has a third caller from outside that module. Worth a more central home; not moved here. ## The other three Reviewed but not implemented: - **#3696** — already possible. `wt --config-set 'list.json-schema = 2' list --format=json` pins the schema per invocation above every config layer, as does `WORKTRUNK_LIST__JSON_SCHEMA`. Answered on the issue; what's left is a docs gap and making an out-of-range value fail rather than degrade in JSON mode. - **#3697** — the machine-readable error channel. A real gap and the one policy call in the set; not started. - **#3699** — aimed at `wt config state logs --format=json`, which is a directory listing reconstructed from paths, under a model that overwrites. The append-only run record it wants is `commands.jsonl`. ## Testing `cargo run -- hook pre-merge --yes` green: 4533 tests, clippy, fmt, doctests, rustdoc, docs sync. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
cbc752276f |
test(switch): drop duplicate configure_mock_glab_env helper (#3707)
Nightly sweep finding (survey of `tests/integration_tests/switch.rs`): the `configure_mock_glab_env` helper was a functional duplicate of `configure_mock_cli_env` — identical `WORKTRUNK_TEST_MOCK_CONFIG_DIR` env setup and the same case-insensitive PATH-prepend logic, differing only in comments. It existed solely for the GitLab MR tests. This PR removes the duplicate and points its 16 call sites at `configure_mock_cli_env`, so the mock-`PATH` setup has a single home. Net −24 lines with no behavior change. Verified with the full GitLab/gitea/azure/MR switch suites (`cargo test --test integration -- switch::…_mr switch::test_switch_pr_gitea switch::test_switch_pr_azure`) — all green — and a clean `--no-run` compile (no unused-function warning). No new regression test: this is a pure test-helper consolidation covered by the existing tests that call it. Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Maximilian Roos
|
4a6fd1ec44 |
refactor(merge): leave target-worktree changes in place, drop the autostash (#3703)
`wt merge` / `wt step push` previously moved a dirty target worktree's uncommitted changes aside with an autostash (`git stash push -u`) and restored them after the push. That design entered `refs/stash` — a repo-global namespace any process can mutate, the source of #3683's race class — restored staged changes as unstaged, and existed only because the fast-forward's `receive.denyCurrentBranch=updateInstead` refuses any dirty worktree at all. Both strategies now advance the target through one `advance_target`: - a compare-and-swap `update-ref` (fails cleanly if the target moved since the snapshot; reflog entries are labeled), - `update-index -q --refresh` + `read-tree -m -u <old> <new>` in the target worktree — git's documented lenient `push-to-checkout` policy (githooks(5)), - a CAS rollback when the sync can't apply, so branch and worktree move together or not at all. Uncommitted changes at paths the push doesn't touch never move: unstaged edits stay unstaged, staged entries stay staged, untracked files stay put, and `refs/stash` is never involved. The autostash machinery (`TargetWorktreeStash`, `StashData`, `stash_restore_failed` and its exit-code path from #3693) is deleted — including the staged-restored-as-unstaged flaw, which disappears with the restore itself. Behavior changes: - Receive hooks no longer fire on the fast-forward path — there is no `git push`. A `git merge` run in the target wouldn't run them either, which is the line the module spec draws. - A sync that can't apply fails the whole command with the ref rolled back; `--no-ff` previously warned and left the worktree stale behind its own branch. - An untracked-path collision is refused upfront, naming the file, instead of stashed and later maybe-conflicting. Review highlights (three adversarial passes over the diff): - The upfront conflict check reads `status --porcelain -uall`, so untracked files inside untracked directories get the named upfront refusal rather than a generic sync failure (and one subprocess is dropped). - A commit racing into the CAS→sync window is detected by a post-sync ref re-read — receive-pack used to give the fast-forward path this check structurally — and warned about. - `read-tree` runs under `-c submodule.recurse=false`, keeping #1604 fixed for `submodule.recurse=true` users. - The ignored-file carve-out (an ignored file at a path the push tracks is overwritten, as a `git merge` there would) is pinned by an end-to-end test: two review passes claimed `read-tree` refuses it; experiment on git 2.55 refuted both. > _This was written by Claude Code on behalf of max-sixty_ Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
02a12c7f59 |
feat(config): match [projects."…"] keys by pattern, and carry forge there (#3701)
## Problem Forge platform is readable only from project config (`[forge].platform`) or a brand substring in the remote hostname. A self-hosted host carrying none of `github`/`gitlab`/`gitea` — a GitLab at `git.company.example`, a company git server — needs the same `[forge]` block in every repository's `.config/wt.toml`. Closes #3678. The user-level `[projects."…"]` table is where per-repository settings already live without touching each repo, but its keys are exact, so covering a host means one entry per repository. ## Solution **Pattern keys.** A `[projects]` key containing `*` matches any run of characters, `/` included, so one entry covers every repository on a host, nested groups and all. `*` is the only metacharacter. ```toml [projects."git.company.example/*"] forge.platform = "gitlab" [projects."git.company.example/platform/*"] worktree-path = ".worktrees/{{ branch | sanitize }}" ``` Every matching entry applies, least- to most-specific, so a narrower key wins where two set the same field and leaves the rest alone. A literal key is the most specific of all; specificity is the count of non-`*` characters. Rules and rationale: the `project_match` module docstring. **`forge` on `[projects]`.** Same shape as the repository's own block, carrying `platform` and `hostname`. Both describe the host rather than the repository — which is why an SSH alias resolved through `~/.ssh/config`, a name local to one machine, belongs in user config rather than a repository's committed one. A repository's own `[forge]` still wins field by field, being the more specific of the two: a repository that sets only `platform` still takes a matching entry's `hostname`. **One resolver.** `wt list`, its statusline, `wt switch pr:`, and CI-platform detection each read project config separately, so a configured platform could resolve in one command and read `unknown` in the next. They now share `Repository::configured_forge_platform` (and `forge_hostname` for the API host). ## Approvals `approved-commands` matches by the same rules, so a pattern entry approves its commands for every repository it covers. That widening is the user's to opt into — only a hand-written key is ever a pattern: - `wt config approvals add` and the interactive prompt record under the exact project identifier, so approving in one repository never reaches another. An identifier that itself contains `*` (a starred remote URL or no-remote path fallback) is refused outright — persisting it verbatim would create an entry reads treat as a pattern; the interactive flow degrades to a warning plus a per-run approval. - `wt config approvals clear` empties only the exact entry, leaving a pattern other repositories share intact — and both its outcomes end with a hint naming any pattern entries still approving commands for the project, so a surviving approval is traceable to the hand-written entry supplying it. - `--stale` judges only the exact entry, so one repository's config can't revoke approvals the others rely on. ## Tests `project_match` unit tests cover `*` spanning `/`, `.` staying literal, specificity ordering, and the lexicographic tie-break. Config tests cover a host-wide entry applying to nested groups, exact-over-pattern precedence, field-by-field layering, hooks appending across both entries, and forge platform/hostname. Forge resolution tests cover the unbranded host, nested groups, a narrower entry winning, project config overriding, falling through to inference, and an invalid value leaving the host unresolved. Approvals tests cover pattern lookup plus the two exactness guarantees above. ## Docs `src/cli/mod.rs` (the primary source) gains "Matching several repositories with one entry" and "Forge platform and hostname" under user project-specific settings, plus a pointer from the project-config forge section. Generated mirrors and `--help` snapshots regenerated. ## Review hardening An adversarial review pass surfaced eight findings, all fixed: - **Approval widening (moderate)**: the starred-identifier refusal above. Previously such an approval persisted verbatim and silently approved its commands for every repository the star matched. - **Literal-key tie (moderate)**: a pattern whose stars all match empty (`github.com/owner/repo*`) ties the exact key on literal count and sorted after it, so its values won the fold. Literal keys now outrank any pattern outright. - **Docs vs behavior (moderate)**: the layering paragraph claimed "most specific wins" for everything; hooks and aliases actually append across matching entries (all run, least-specific first). Docs now say so, and state the forge field-by-field precedence. - **Minor**: `matches()` is a two-pointer byte glob (was a per-call regex compile, ~0.7 ms per pattern key, a few hundred calls per `wt list`), pinned by an exhaustive differential test against a reference matcher; the invalid-platform diagnostics name their two possible config homes; a root `[forge]` in user config now points at `[projects."<id>"].forge`; docs note a host-wide key should end in `/*`; `approve_command` delegates to `approve_commands`, unifying their dedup predicates. ## Relationship to #3681 This is an alternative to #3681, which adds a bespoke `[forge-hosts]` section for the same issue. Both can't land — they'd be two ways to write one sentence. This one puts the setting in the table that already carries per-repository user config, and the pattern keys are reusable for the workspace-scoped ask in #3654 where repositories share a host or namespace. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Maximilian Roos
|
bbd24fb8f3 |
refactor(remove): make one function own the pre-removal gate (#3694)
Follow-up to #3690, which recorded the decision that removal's final dirty-worktree gate is answered by the fsmonitor daemon, exactly as `git worktree remove`'s own gate is. The decision went into the `src/git/remove.rs` module spec — but the default removal path never reads that module. Both paths ran the same three ordered steps before the worktree directory stopped existing: dirty-worktree gate, fsmonitor stop, rename into trash. They were two copies with near-identical comments. `execute_instant_removal_or_fallback` (`src/output/handlers.rs`, the default background path for `wt remove` and `wt merge --remove`) assembled them inline and never called `remove_worktree_with_cleanup`, so a reader working on the gate that issue #3646 was actually about would not find the reasoning written for it. `stage_worktree_removal` now owns all three steps and both paths call it. The gate becomes one decision instead of a sequence each caller re-assembles, and the ordering rationale — why the gate precedes the daemon stop, why the stop precedes the rename — lives on the function that implements it. The rename-and-prune step it previously named becomes the private `rename_into_trash`. No behavior change. ## Stale spec claims this surfaced - `stop_fsmonitor_daemon` documented three callers ("the library `remove_worktree_with_cleanup`, the foreground handler, and the background `spawn_background_removal`"). The foreground handler goes through `remove_worktree_with_cleanup`, so it was two; after this change it is one. - The module opened by calling itself "the canonical removal flow used by `wt remove`, `wt merge --remove`, and the TUI picker". `wt merge --remove` takes the background path, and so does `wt remove` unless `--foreground` is passed. ## Test Nothing pinned the final gate on the **default** path — the existing test covers `--foreground` only. Planning validates cleanliness before `pre-remove` runs, so a hook that dirties the worktree can only be caught by the gate immediately before the mutation. The test is now parameterized over both execution modes. Verified by mutation rather than by assuming the assertion binds: with the gate disabled, the background case reports `Removed feature-hook-dirties worktree & branch` and destroys the hook's file, which is the data-loss scenario #3646 described. Ref #3646 > _This was written by Claude Code on behalf of Maximilian_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
03f49ce93a |
fix(merge): exit non-zero when the target autostash can't be restored (#3693)
Follows #3684. The Windows test un-gating that was stacked here is split out into #3695, now merged; this PR is the exit-code change alone. ## Problem `wt merge` reported success when the target worktree's autostash failed to replay. The warning named the recovery command, but exit 0 said the user's uncommitted changes were back in their worktree while they were still in a stash — and that warning scrolls past under the worktree-removal and post-merge-hook output that follows it. ## Solution The restore outcome travels out through `PushResult`. The command finishes everything it started — ref advanced, worktree removed, hooks run, `--format=json` payload printed — and only then returns `AlreadyDisplayed { exit_code: 1 }`. Aborting at the restore instead would leave a landed merge with its cleanup half-done, trading a recoverable stash for a worse mess. The shell wrapper applies its `cd` directive whenever the directive file is non-empty, independent of exit code, so a non-zero exit strands nobody in a removed directory. Both output channels name the failure: the `--format=json` payloads of `wt merge` and `wt step push` carry `stash_restore_failed`, present on every payload like the other outcome booleans. The exit code alone would leave a consumer reading stdout with a success-shaped object and no signal. This also closes a gap the change surfaced: `handle_no_ff_merge`'s already-up-to-date early return never called `restore_stash`, so a dirty target worktree with nothing to merge restored through `Drop` and reported nothing. It restores on that path too now, which is what makes the guarantee hold — every remaining `Drop` of the guard happens on a path already returning an error. ## On diverging from git `git rebase --autostash` exits 0 in this situation: it prints "applying them resulted in conflicts" and still reports "Successfully rebased". The difference is what the user is left looking at. git's failure leaves conflict markers in the working tree, met immediately; a failed `git stash apply` here can leave the worktree untouched — an untracked path re-created underneath it, for instance — so nothing but the exit code outlives the warning. The reason is recorded on the field the exit code hangs off, so it doesn't read later as an oversight. ## Testing `test_merge_autostash_restore_failure_exits_non_zero_after_cleanup` covers the guarantee end to end: exit 1, `stash_restore_failed` in the JSON, ref advanced, source worktree removed, stash entry still present for recovery. `test_push_autostash_restore_failure_warns` moves from asserting `success()` to asserting exit 1 plus the push having landed. Also verified against a real build outside the suite, on both commands: the merge lands, the worktree is cleaned up, exit is 1, the JSON reports `"stash_restore_failed": true`, and the warning names the exact `git stash apply <sha>`. `wt step push --help` gained the failure contract, since it previously described only the success path; the generated mirrors are regenerated with it. Local (macOS): `cargo run -- hook pre-merge --yes` green — 4500 tests, clippy, fmt, doc sync. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
8a2a2f92f6 |
test: run the git-hook autostash tests on every platform (#3695)
Split out of #3693 so it can be reviewed on its own. Follows #3684. ## Problem The three tests that install a native git hook — `test_push_autostash_survives_concurrent_stash`, `test_push_autostash_restore_failure_warns`, and `test_merge_target_diverges_during_receive_restores_autostash` — were gated `#[cfg(unix)]`, so the autostash regression they guard went unverified on Windows. Nothing they assert is unix-specific; git runs hooks through a shell it ships on every platform. The gate was carrying two incidental blockers: - `std::os::unix::fs::PermissionsExt`, for an executable bit Windows has none of. - A `root.display()` path interpolated into the hook script, whose Windows backslashes would reach `sh` as escapes. ## Solution `common::write_git_hook` writes the hook and gates only the chmod, so the platform fact lives in one place instead of being re-derived per test. The repo root is interpolated with `path_slash`'s `to_slash_lossy` — the form `step_prune.rs` already uses to embed a path in a hook command that runs on Windows. ## Why un-gating is safe Each test asserts that its own hook fired: the push tests require `INTERLOPER` in the stash list, and the merge test requires the target ref to have advanced. A hook that silently fails to run on Windows therefore fails the test rather than passing vacuously. Confirmed rather than assumed — on the stacked branch these tests ran green on `test (windows)`, and pulling that job's junit artifact shows all three present by name, so they executed rather than being filtered out. ## Sweep The rest of the suite's platform gates were checked and left alone; the remaining ones are gated for real reasons: symlinks, signal delivery, unix permission bits, `lsof`/fsmonitor daemon reaping, shell-script `git` shims on `PATH` (Windows can't exec a shebang script that way), ConPTY output capture, and clap's differing `[experimental]` tag rendering in Windows snapshots (`step_alias.rs`, `help.rs` — already documented in-place). The `#[ignore]`s and elevated-privileges runtime skips are likewise documented and intentional. > _This was written by Claude Code on behalf of max-sixty_ |
||
|
Worktrunk Bot
|
4485312c50 |
fix(shell): preview legacy-file removal in the wt switch first-run offer (#3656)
Nightly sweep finding. The `wt switch` first-run shell-integration offer could delete a deprecated wrapper file it never named in the confirmation prompt — the same unpreviewed-deletion defect [#3644](https://github.com/max-sixty/worktrunk/issues/3644) fixed for `wt config shell install`, left on the sibling first-run path. ## The gap When `wt switch` shows *"Install shell integration?"* on first run, [`prompt_shell_integration`](https://github.com/max-sixty/worktrunk/blob/c4e439409445b300c052a7761450511fa1ada0a6/src/output/shell_integration.rs#L474) passed an **empty** legacy-cleanups list to the prompt, then called `handle_configure_shell(None, /* skip_confirmation */ true, …)`. That call removes any deprecated wrapper file as a side effect — the fish `conf.d/wt.fish` superseded by `functions/wt.fish` (#566), or a nushell wrapper stranded at a legacy autoload path (#2878). So a user on an unconfigured bash who also had a stale, worktrunk-managed `~/.config/fish/conf.d/wt.fish` would accept *"Install shell integration?"* and have that fish file deleted — reported only after the fact. This is exactly what `wt config shell install` now previews (#3644, merged in #3648); the first-run offer was the one path still deleting without naming the file first. It runs against [CLAUDE.md's data-safety rule](https://github.com/max-sixty/worktrunk/blob/c4e439409445b300c052a7761450511fa1ada0a6/CLAUDE.md): *"No implicit destructive side effects — never silently delete/overwrite as a side effect of an unrelated operation."* ## The fix Compute the same dry-run legacy-cleanup list `handle_configure_shell` re-derives internally (both come from the same `scan_shell_configs(None, true, …)` scan, so the lists are identical) and pass it to the offer's prompt. The deletion is now named before consent. The change is **purely additive to the prompt** — it does not change what gets removed, only what the prompt discloses. ## Test A PTY regression test drives the first-run offer with bash unconfigured (so the offer fires) and a deprecated fish `conf.d/wt.fish` present, requests the preview (`?`), and asserts it names the removal. Verified it **fails without the fix** (the preview shows only bash's *"Will add"* line) and **passes with it**. ## Note for review The prior code carried a comment documenting the empty-list behavior as deliberate (*"resolves no legacy cleanups of its own … reports removal after the fact, as before"*). Reading it as *"#3644's fix wasn't extended here"* rather than *"the first-run offer should delete without previewing"*, this completes that fix — but flagging it so the call is explicit. If the boundary was intentional, this is a safe no-op to close. Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Worktrunk Bot
|
8506c98a42 | fix(prune): serialize worktree-registry teardowns behind a lock (#3692) | ||
|
Worktrunk Bot
|
194edd5ea2 | fix(merge): restore autostash by commit SHA, not stash@{0} (#3684) |