mirror of
https://github.com/max-sixty/worktrunk.git
synced 2026-09-14 20:00:38 +08:00
codex/remove-codex-cloud-specific-tests
280 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
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
|
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_ |
||
|
Maximilian Roos
|
4554f50ce6 |
perf(tests): stop leaking a temp dir per test, and measure where the suite's CPU goes (#3604)
Measured where the test suite's CPU actually goes, fixed what was doing
real extra work, and added `task profile-tests` so the measurement is
repeatable.
## Baseline
`cargo nextest run --features shell-integration-tests` on an 18-core
M-series machine: 4,570 tests, ~95s wall, **988 CPU-seconds (332 user +
655 sys)**.
Two thirds is kernel time, so the cost is process creation and
filesystem churn rather than computation. The integration binary is 86%
of summed self-time: 2,184 tests at ~0.6s each, each spawning `wt` (a 65
MB debug binary, ~11ms CPU per spawn against 2.6ms for a trivial
process) and `git` against a fresh fixture copy. It sits in a broad
middle, not a few outliers: 73% of self-time is in tests taking 0.25 to
2.0s.
## One leaked temp directory per test
`isolated_test_cwd()` held a `TempDir` in a `LazyLock`. Statics don't
run destructors at process exit and nextest runs one process per test,
so every test leaked an empty directory into the system temp root.
Measured with `TMPDIR` pointed at a fresh directory: **704 per
integration-suite run**. This machine had accumulated **454,907 entries,
353,268 of them empty strays older than a day**.
Stale entries are cheap to ignore but expensive to enumerate, and
`git::recover::recover_from_path` reads every ancestor directory of a
deleted CWD:
| temp root | `test_recover_from_path_returns_none_for_unrelated_path` |
|---|---|
| 454k entries | 14.2s (34.4s on a quieter run) |
| empty | 0.27s |
One fixed directory replaces it. Leaks per run: 704 to 0, verified
across full suite runs, and the directory is still empty after ~9,000
test executions.
I also checked whether a crowded temp root slows ordinary temp
operations. It does not: create, populate and delete of a fixture-sized
tree ran at 21ms/iter in a 454k-entry parent against 40 to 60ms in an
empty one. The leak's cost is concentrated entirely in code that
enumerates.
## Fixture temp dirs no longer sit in the shared temp root
The fixtures created their temp directories directly in the system temp
dir, among however many entries the machine had put there.
`test_temp_root()` (`$TMPDIR/wt`) roots them one level in, and
`test_tempdir()` replaces `TempDir::new()` across every `TestRepo`
constructor, the mock-command helper, the `temp_home` fixture and the
recovery tests. That test again: **14.2s to 0.06s**.
Two constraints worth knowing, both found by trying the more aggressive
version first:
- **A cache-dir root fails.** `~/Library/Caches` is under `/Users/`,
which a conditional `includeIf "gitdir:/Users/"` matches, so 16 picker
tests fail their commits under `commit.gpgsign` — they drive git through
`Repository::run_command`, the production API with no isolated config.
`step_promote` above was not a one-off: the suite was hermetic against
host git config only because macOS puts `$TMPDIR` outside `/Users/`.
That is the hole the next section closes — at the layer that covers
in-process git, not by choosing where temp files live.
- **The root's name is load-bearing.** A unix socket path cannot exceed
`sun_path`, 104 bytes on macOS. The canonicalized per-user `$TMPDIR` is
56, and `test_copy_ignored_skips_non_regular_files` binds a listener 89
bytes in. `worktrunk-tests` (16 bytes with its slash) overflowed by two;
`wt` costs 3 of the 14 spare.
The ~240 tests that call `tempfile` directly still use the system temp
dir. They are transient and a clean run leaks nothing, so converting
them is tidiness rather than a fix.
## A test that passed by accident of TMPDIR location
`step_promote::test_promote_bare_repo_with_worktrees` drove git through
bare `Cmd::new("git")` instead of `configure_git_env`, so the host's
config applied. A conditional `includeIf "gitdir:/Users/"` enabling
`commit.gpgsign` fails its commit, but only when `TMPDIR` sits inside
the matched tree. macOS puts `TMPDIR` under `/var/folders`, so it
passed; pointing `TMPDIR` anywhere under the home directory failed it.
## The suite was not actually isolated from the developer's git config
Chasing the temp-root change turned up a real hole. `TestRepo` exposes
the production `Repository` type, and `Repository::run_command` builds a
plain `Cmd::new("git")` with no `GIT_CONFIG_GLOBAL`, so it inherits the
test process's own environment. Separately, a bare `wt_command()` had no
`GIT_CONFIG_GLOBAL` at all and fell through to `~/.gitconfig`. 280
`Repository::at/current/discover` constructions across 31 test files and
156 direct `run_command*` calls in test code sat on that path.
Signing was only the symptom that surfaced. Confirmed leaking from a
real developer config: `commit.gpgsign`, `core.fsmonitor` (spawns a
daemon per fixture repo), `worktree.guessremote` (changes `git worktree
add`, directly under test), `help.autocorrect = prompt` (a mistyped git
command blocks). Structurally: `core.hooksPath`, `credential.helper`,
`filter.*` clean/smudge and `diff.external` all execute arbitrary
programs; `url.*.insteadOf` rewrites remotes; `merge.conflictstyle`,
`diff.context`, `rebase.autostash`, `fetch.prune`, `push.default` all
change what the code under test observes. That set is unbounded, which
is why hardening each fixture's local config was rejected — a denylist
can't cover it, and local config cannot unset an inherited `[include]`
or `credential.helper` at all.
The floor is one constant, `shell_exec::HERMETIC_TEST_GIT_ENV` — the
deny pair pointing `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM` at a path
that does not exist, plus the settings the suite needs through
`GIT_CONFIG_COUNT` / `GIT_CONFIG_KEY_n` / `GIT_CONFIG_VALUE_n` — applied
to every child at its spawn site. There is no git-config file anywhere
in the repo. For the spawn sites the harness owns (`git_test_env`,
`configure_git_cmd`, `isolate_subprocess_env` for `wt` children,
`pty_env_vars` for `env_clear`ed PTY children) that's ordinary per-child
env. For the git that *production* code spawns while a test drives it
in-process, the test never holds the command — so the harness flips an
atomic latch (`shell_exec::enable_hermetic_test_env`, called from the
fixture constructors), and `Cmd`, the choke point every production spawn
passes through, applies the floor to each child while the latch is set.
Setting env on a child is safe; it was setting the test process's *own*
env that wasn't (`set_var` races the other test threads), and the latch
dissolves the need for it — no `unsafe`, no pre-`main` constructor, no
cargo `[env]`. Because the latch lives in the binary, every runner
agrees by construction: `cargo test`, nextest, `cargo llvm-cov`, `cargo
bench`, an IDE, a debugger, a directly executed
`target/debug/deps/integration-*`. `.config/nextest.toml` was tested and
rejected, and is now ruled out standing: nextest 0.9.132 has no `[env]`
key, and a `$NEXTEST_ENV` setup script would miss the three non-nextest
runners CI uses (`cargo llvm-cov`, the Nix `cargo test` derivation,
`cargo bench`). A runner-specific knob doesn't fail loudly when another
runner misses it — it yields a different result, usually in the coverage
job whose numbers gate a merge. `tests/CLAUDE.md` → One Result Per Test,
Whatever Runs It records the rule; `.config/nextest.toml` points at it
from the place someone would be tempted.
Acceptance test, since the suite passed before only by accident of
`$TMPDIR` sitting outside `/Users/`: with `test_temp_root()` temporarily
pointed under `$HOME` so a conditional `includeIf "gitdir:/Users/"`
fires, the picker tests go from **20 of 38 failing to 38 of 38
passing**.
**The cost:** the latch is a test-serving switch compiled into
`shell_exec` — one static, one relaxed load per spawn, marked
`TODO(hermetic-env)` with the structural alternative (threading an
explicit env value through `Repository`). `cargo run -- <cmd>` is
untouched: nothing in production latches it, so a developer's own
invocations keep their aliases, credential helper, and identity. The one
production git spawn that bypasses `Cmd` (the fsmonitor daemon launch)
re-applies the floor by hand. (Two earlier shapes were tried and
replaced: cargo's `[env]`, which taxed every `cargo run` and vanished
whenever a test binary ran outside cargo, and a pre-`main` constructor
crate, which every test target had to link and which put the floor on
developers' `cargo bench`-adjacent runs too.)
## In-process tests were reading the developer's worktrunk config too
`config_path()`'s third priority is the real
`~/.config/worktrunk/config.toml`, and a lib-crate test cannot set the
second for itself — `set_var` is `unsafe` and this crate forbids
`unsafe`. So a test reaching priority 3 got the developer's own config.
A `panic!` build of the guard named the live callers immediately:
`git::repository::tests::prewarm_*` read it on every run, and
`set_skip_shell_integration_prompt` /
`set_skip_commit_generation_prompt` reach the same resolver to
**write**.
Priority 3 is now absent under `#[cfg(test)]`, which is compiled out of
the real binary — so unlike the git floor above, this one costs `cargo
run` nothing. It returns `None` rather than panicking like
`approvals_path()`: that guards a mutation target where silent absence
would let a test believe it saved something, whereas this is a lookup
whose absent state is already handled — `require_config_path()` turns it
into an error, so a write still fails loudly while a best-effort read
preloads nothing.
The guard covers lib-crate tests only; `src/commands/` and `src/output/`
link the lib in non-test mode. Nothing there exercises the fall-through
today (968 bin-crate tests create no config under a scratch `$HOME`), so
that is a requirement on new tests, recorded in `tests/CLAUDE.md`.
`system_config_path()` stays unguarded on purpose — machine-wide file,
and `config::deprecation`'s `PendingDefault` rules need the lookup.
## Merging main's parallel fix
#3620 attacked the same in-process hole from the other side, writing
`LOCAL_TEST_CONFIG` into each fixture's own `.git/config`. The two
compose rather than compete and both are kept: the floor denies the
host's config to every git, and the local config supplies what a
hermetic in-process git still needs — an identity, which the floor
deliberately withholds because it has per-command homes already
(`git_test_env`, `LOCAL_TEST_CONFIG`) and `useConfigOnly` fails loudly
if a path misses both. main's structure (`TestConfigPaths`,
`TestRepo::bare`) is kept as-is.
Two docstrings were true on each side and false together:
`test_gitconfig_path` restated the gitconfig inline, and
`LOCAL_TEST_CONFIG` said in-process git reads the developer's
`~/.gitconfig`, which is what the floor prevents. `test_gitconfig_path`
also held its `TempDir` in a `LazyLock`, the leak this PR removes. Both
are moot now: the function is gone with the files.
## Why there is no gitconfig file
The isolation went through two file-based shapes before this one, and
neither earned its keep. Denial never needed a file, because
`GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM` *are* the denial; the file
existed only to *set* things, and `GIT_CONFIG_COUNT` is git's
environment spelling of `-c`. Deleting both files also deletes the
`[include]` that kept them from drifting, the per-process write that
broke `test (windows)` on a shared path, and the config-path argument
threaded through 22 call sites.
The floor that remains is the deny pair plus two settings.
`user.useConfigOnly` is a backstop: denial alone leaves git *guessing*
an identity from the OS username and hostname rather than failing, which
is the one way a hermetic suite could still author a commit as the
developer. Nothing exercises it, and that is the reason to keep it.
`rerere.enabled = false` is *set* rather than left unset, so the suite's
rerere state cannot depend on what a fixture happens to carry.
`commit.gpgsign`, `advice.mergeConflict` and `advice.resolveConflict`
are gone: denial leaves git on its own default for the first, and the
snapshot layer strips the gutter-prefixed `hint:` lines the other two
quieted (they vary across git versions), so nothing depends on
suppressing them at the source. The long-dead
`tests/fixtures/template-repo/` fixture went with them.
Once the latch made denial universal, the redundant copies went too:
`git_test_env` no longer restates the deny pair per command (the floor
is denial's only writer, so its value is uniform across every
transport), `LOCAL_TEST_CONFIG` dropped its `commit.gpgsign` (denial
guarantees the default), the platform-dependent `NULL_DEVICE` constant
is deleted, and the `.env.GIT_CONFIG_GLOBAL` snapshot redaction is gone
— the recorded value is one cross-platform constant, so there is nothing
volatile to redact.
I removed `rerere.enabled` first, on a local measurement that was wrong,
and CI failed on all three platforms. The standard fixture is built once
into `target/debug/wt-test-fixtures/` and copied per test, and that
cached copy held an `rr-cache` directory left by a rebase run while the
floor still enabled rerere. Git turns rerere on by itself whenever
`rr-cache` exists, so every local test kept the behavior the change had
just removed, while CI built the fixture fresh and lost it.
`tests/CLAUDE.md` now records the trap: clear the fixture cache before
trusting a local measurement of a git-config change.
**Why the floor can't live in a fixture:** every other test variable is
set on a *child* — `git_test_env` on a git command,
`configure_cli_command` on a `wt` subprocess. In-process git is not a
child the test configures: `Repository::run_command` is production code
building a plain `Cmd::new("git")`, and the test never holds that
command, so there is no place to set env on it — while setting the test
process's own environment is the one thing a test can't do safely
(`set_var` races the other test threads). The identity did move to the
fixtures and the per-command env this way; the denial reaches
production's children through the latch at `Cmd`, the choke point they
all pass through.
Two things fall out of `-c` semantics, both pinned:
- **It outranks a repository's own config**, where a global file would
yield to it. So `init.defaultBranch` cannot live in the floor:
`default_branch.rs` sets that key in a repo to prove `wt` reads it, and
an entry would silently win. The three harness `git init` calls that
relied on the floor now name their branch, as the other two already did.
- **A PTY child is `env_clear`ed**, so it inherits nothing and used to
get the floor through the file. It now gets the family from
`configure_pty_command`, the choke point every PTY spawn routes through,
and again from `pty_env_vars`, whose vector declares a PTY `wt` child's
complete environment. Each copy is pinned by its own test, because the
settings only quiet advice and refuse a guessed identity, so no PTY
assertion would catch their loss.
Four tests wrote their own gitconfig to get `init.defaultBranch` plus an
identity; the harness supplies both, so those writes are gone too. Net
45 lines lighter, and no snapshot changed.
## Measurement
`task profile-tests` builds first, then runs the suite under bash's
`time` keyword (task's own interpreter, mvdan/sh, parses `time` but
hardcodes `user`/`sys` to zero): CPU totals on the console, every
per-test duration in the default profile's `junit.xml`. It began three
sizes larger — a scratch-`TMPDIR` leak check that dragged a `sun_path`
byte budget into the Taskfile, a `/usr/bin/time` dependency GNU-less
Linux lacks, and a `perf` nextest profile whose console slow-listing
restated what junit already carries — and each piece fell to the same
question, whether the measuring goal needed it. Method and how to read
the numbers: `tests/CLAUDE.md` under Profiling the Suite.
## Found, measured, not changed
**The gate keeps a duplicate cargo artifact set.** `RUSTFLAGS='-D
warnings'` on the insta step is part of cargo's fingerprint, so it forks
all 343 crates into a second artifact set: 261 CPU-seconds to prime plus
a duplicate of `target/debug/deps` (`target/debug` here is 35 GB, in a
52 GB `target/`). I removed it and then put it back: the clippy step
that would cover it runs on ubuntu only, while the cross-platform matrix
runs `wt hook pre-merge --yes insta`, so this RUSTFLAGS is the only
thing denying warnings on macOS and Windows. `[lints.rust] warnings =
"deny"` would keep that coverage everywhere without forking the graph
(verified locally: fails on a planted unused variable, recompiles only
worktrunk and wt-perf, feature-check commands still pass), but it also
makes plain local builds fail on warnings and newly exposes `cargo msrv
verify` and the minimal-versions job. That is a workflow call. The
duplication is a one-time cost per artifact set rather than per-edit, so
it is second-order next to the ~600 CPU-seconds each suite run costs.
**`recover_from_path` enumerates every ancestor up to `/`.** At each
ancestor it reads the directory and stats `.git` in every child.
Bounding the child scan to the first *existing* ancestor would preserve
both documented layouts (sibling and nested) and both of that module's
regression tests, but it would break a custom `worktree-path` layout
where the repo is a child of a higher ancestor, so it needs a decision
about which layouts recovery must support.
**Nothing in the gate is the biggest cost; concurrency is.** The five
`[[pre-merge]]` keys are one table, so they run concurrently, and each
is a cargo command that wants the whole machine. They serialize on
cargo's build-directory lock (`Blocking waiting for file lock on build
directory` appears in every run) while test execution overlaps another
step's build. The `lockfile` comment says it "must be first", which
concurrent execution does not provide. Separately, agent worktrees run
whole gates at once: during this work a second worktree ran its own `wt
hook pre-merge` alongside mine, load average hit **154 on 18 cores**,
and the same suite took 147.7s instead of 92.9s.
## Tried and rejected
`[profile.dev] debug = "line-tables-only"` first measured 14% less CPU,
but per-spawn CPU was unchanged, which did not fit the proposed
mechanism. Re-measuring both configurations on a quiet machine gave
602.6s (baseline) against 606.5s (line-tables). The original delta was
contention from a sibling worktree running its own suite.
macOS Gatekeeper (`syspolicyd`) looked like a candidate at 230% CPU, but
300 spawns of the freshly built `wt` cost it 0s, and 0.2s after a
relink, against 0.4s during a 10s idle baseline.
## Verification
`cargo run -- hook pre-merge --yes` green, 4,611 tests passed, run
against a freshly built fixture cache after the switch to the latch. The
latch was verified to be the only source of the variables: the invoking
shell carries no `GIT_CONFIG_*`, and the meta-tests
(`in_process_git_reads_only_the_hermetic_config` asserting the *origin*
of every resolved setting, `pty_env_vars_carry_the_git_config_floor`,
and the `isolate_subprocess_env` scrub test asserting the floor is
re-set after the scrub) pin each transport. Across earlier runs one hit
a single intermittent PTY failure in `shell_wrapper` (exit 127) that
passes 3/3 in isolation and whose code path never touches `wt_command()`
or the shared cwd; a later run was green under heavier load than the one
that failed.
## Review round
A full review of the branch (three finder lenses, findings adversarially
verified) landed one more commit:
- **The wrapper-suite PTY children never got the floor.**
`configure_pty_command` env-clears, skips the `Cmd` latch, and sets the
real `HOME`, and the shell-wrapper call sites layer only fixture paths
and an identity on top. Every git those ~89 tests ran therefore read the
developer's real `~/.gitconfig` (and lost the gpgsign shield when
`LOCAL_TEST_CONFIG` dropped it). The floor now rides that choke point,
pinned by `configure_pty_command_carries_the_git_config_floor`; every
raw `CommandBuilder` site was checked to route through it.
- **`task profile-tests` could never report CPU.** mvdan/sh's `time`
hardcodes `user`/`sys` to `0m0.000s`, so the numbers the docs said to
track were unproducible; now `bash -c 'time "$@"' bash ...`. Measured
post-fix: user 5m47s / sys 12m8s, a 67.7% kernel share, confirming the
two-thirds claim above; the integration mean measured ~1s and the docs
were corrected from ~0.6s.
- Smaller: two raw `Command::new("git")` asserts in `remove.rs` tests
now go through `configure_git_cmd`; the hermetic meta-test keys on
`--show-scope` scopes rather than git's origin-path spelling; the dead
`template-repo` fixture is deleted; three test `git init`s name `-b
main`; doc corrections (sun_path arithmetic, recover-walk attribution,
dead `[TEST_GIT_CONFIG]` remnants, volatile counts).
Final gate on the finished tree: green, 4,612 tests. Deferred with
rationale: ~985 snapshots carry stale env-block metadata (insta never
compares it; it churns on future re-records), `$TMPDIR/wt` is not
per-user on Linux (a root run poisons it for later users), and
`spawn_detached_exec_*` / `step tether` do not hand-apply the floor (no
in-process test reaches them today).
> _This was written by Claude Code on behalf of Maximilian_
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
a285ed308d |
refactor(tests): bring the mock-stub env vars under the WORKTRUNK_TEST_ prefix (#3621)
`MOCK_CONFIG_DIR` and `MOCK_CALL_LOG_DIR` were the only two worktrunk-invented environment variables without the `WORKTRUNK_` prefix. That is not just a naming inconsistency: `isolate_subprocess_env` scrubs the parent environment by prefix — `GIT_*` and `WORKTRUNK_*` — so an unprefixed name is the one thing a test child inherits from whoever ran the suite. Renaming them brings them under that scrub. - `MOCK_CONFIG_DIR` → `WORKTRUNK_TEST_MOCK_CONFIG_DIR` - `MOCK_CALL_LOG_DIR` → `WORKTRUNK_TEST_MOCK_CALL_LOG_DIR` `TEST` rather than a bare `WORKTRUNK_` because both are read only by `tests/helpers/mock-stub` — they are the protocol between the harness and its helper binary, and `wt` itself never reads either one. That matches the ~15 existing `WORKTRUNK_TEST_*` knobs. ## The snapshot half Mechanical but not a substitution: the `env:` block is byte-sorted by key, so the renamed entry moves position within it and a `sed` in place would leave it where the old name sorted. All 971 affected blocks were rewritten by dropping the old line, inserting the new one, and re-sorting — the aggregate diff is exactly one removed and one added line per file: ``` 971 files changed, 971 insertions(+), 971 deletions(-) - MOCK_CONFIG_DIR: "[MOCK_CONFIG_DIR]" + WORKTRUNK_TEST_MOCK_CONFIG_DIR: "[TEST_MOCK_CONFIG]" ``` The redaction placeholder follows its neighbours' convention in `add_standard_env_redactions` (`WORKTRUNK_TEST_NU_VENDOR_AUTOLOAD_DIR` → `[TEST_NU_VENDOR_AUTOLOAD]`), so it now reads `[TEST_MOCK_CONFIG]` rather than repeating the full key. `MOCK_CALL_LOG_DIR` appears in no snapshot — its two call sites are `.output()` assertion tests — so it needs no redaction. ## Keeping it from regressing `tests/CLAUDE.md` gains the rule under "Where a new environment variable goes": name it `WORKTRUNK_TEST_*`, and the rule covers the harness↔helper-binary protocol, not just knobs `wt` itself reads. Without it the next helper-binary variable gets named `MOCK_*` again and the hermeticity hole reopens. ## Verification `cargo run -- hook pre-merge --yes` passes (exit 0) on the merged tree: 4607 tests including `--features shell-integration-tests`, `pre-commit run --all-files`, clippy, doctests. No pending snapshots. A repo-wide sweep finds no remaining unprefixed spelling. ## Merged main #3620 landed while this was in flight and regenerated several `for_each` snapshots that still carried the old key, so main is merged in here. It resolved with no conflicts, and the result is what you'd want rather than what git happened to produce: those blocks now carry #3620's new keys (`GIT_ALLOW_PROTOCOL`, `CLAUDE_CONFIG_DIR`, `WORKTRUNK_TEST_PARENT_SHELL`) *and* the renamed key, each in sorted position. The sweep and the gate above both ran after the merge. #3620 also rewrote the "Where a new environment variable goes" section this branch adds to — three layers became four. Both edits survived; the new naming paragraph follows the updated layer list. The two advisory `affected tests` checks are red for the same reason, and merging clears them: `cargo affected` errors on `git diff stdout was not valid UTF-8`, because the diff from this PR's base contains 16 binary files — the `tests/fixtures/standard/` git objects and index files that #3620 deleted. This branch's own commit contributes none. A PR based after #3620 won't see them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) > _This was written by Claude Code on behalf of Maximilian Roos_ Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
9032308400 |
refactor(tests): give the PTY test environment one home (#3618)
Follow-up to #3616, which fixed a PTY snapshot flake by adding one env knob — and to add it I had to touch three separate env builders, none of which knew about the others. This consolidates that surface. ## What was there The environment a test subprocess runs in was assembled at nine sites: five copies of the PTY prologue (`env_clear`, HOME, PATH, the Windows block, coverage passthrough) and four partial restatements of the determinism knobs. There was no rule for which belonged where, so adding a knob meant finding every copy, and a missed copy surfaced later as a flake somewhere unrelated. ## What's there now Three named layers, each with one home in `src/testing/mod.rs`: | Layer | Home | Contents | |---|---|---| | Baseline | `STATIC_TEST_ENV_VARS` | knobs every child needs, whatever it's attached to | | Terminal | `PTY_TEST_ENV_VARS` (new) | knobs only a TTY triggers — `WORKTRUNK_TEST_SPINNERS=0` | | Fixture | `pty_env_vars(TestEnvPaths { … })` (new) | the paths that vary per fixture | `configure_cli_command` and `configure_pty_command` apply them by transport. That turns the standing `// NOTE: TERM is intentionally NOT in STATIC_TEST_ENV_VARS` comment into a consequence rather than an exception: `TERM` is transport-level, so it can't sit in a baseline both transports share. `WORKTRUNK_TEST_SPINNERS` stays out of the shared baseline deliberately. It's inert on a pipe, and insta-cmd records the whole environment into every snapshot it writes (`Info::from_std_command` builds it unconditionally from `cmd.get_envs()` — there's no hook to suppress it), so putting it there would add a no-op line to 1043 snapshot files. `configure_pty_command` is now the only place a PTY child's isolation is set up. `shell_command`, `execute_shell_script`, `configure_pty_environment`, `exec_in_pty_shell`, `exec_bash_truly_interactive` and two `wt switch` spawns all delegate to it. `shell_wrapper`'s `STANDARD_TEST_ENV` and `bare_repository`'s hand-rolled `test_env_vars` are gone, as are `configure_shell`'s hand-copied knobs and four redundant `CLICOLOR_FORCE` lines in `switch_picker`. Net −149 lines. One spawn stays outside: the Windows ConPTY smoke test, which runs PowerShell against a deliberately bare environment and isn't a wt child at all. ## Reviewing Start at `src/testing/mod.rs` — the three layers and their doc comments are the whole design. Everything under `tests/` is deletion plus a delegation call. Two snapshots change: `install_preview_with_gutter` and `install_preview_declined` now carry ANSI, because those two tests previously ran without `CLICOLOR_FORCE`. Text is identical. Arguably a fix — the test named "with_gutter" couldn't see the gutter (a background-color block), while its own prompt line was already colored, so the file was internally inconsistent. ## Testing Full `wt hook pre-merge --yes` green: 4601 tests, `--features shell-integration-tests`, `RUSTFLAGS='-D warnings'`, `insta --check`. The knob's delivery path was verified by probe rather than by inspection. With `sleep 6` in the mock `llm`, `test_readme_example_hooks_pre_merge` passes; flipping `PTY_TEST_ENV_VARS` to `"1"` reproduces the original failure byte for byte: ``` +␛[1G␛[J␛[2m↳␛[22m ␛[2mWaiting for the commit generation command (4s)␛[22m␛[1G␛[J␛[2m↳␛[22m ␛[2mWaiting for the commit generation command (5s)␛[22m ``` So the knob reaches the shell-wrapper PTY child through the shared setup, not through a surviving copy. Both probe edits are reverted. > _This was written by Claude Code on behalf of max_ Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
0a1cd19cc9 |
chore: drop the NEXTEST_NO_INPUT_HANDLER workaround (#3482)
nextest#2878 (the run suspending with SIGTTOU when a test spawns an interactive shell, as the `shell-integration-tests` feature does) was fixed in nextest 0.9.118, so the `NEXTEST_NO_INPUT_HANDLER=1` workaround threaded through the insta hook, CI, and the test docs is obsolete. Verified on nextest 0.9.132: the shell-wrapper tests run to completion under a real PTY without the variable. `.config/nextest.toml` now pins `nextest-version = "0.9.118"`, so an older nextest fails with a clear version error instead of suspending mid-run. > _This was written by Claude Code on behalf of max_ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
0b413ae3b2 |
fix(list): reserve prompt rows so the table doesn't jump at exit (#3409)
`wt list`'s progressive table sits stable on screen through the loading phase, then jumps up at an unpredictable moment: the command exits and the shell prints its multi-line prompt, and the terminal scrolls to make room. Every other command scrolls too, but there the output scroll and the prompt scroll merge into one Enter-time event; the progressive table's delay between paint and exit is what makes the jump read as a jerk. The renderer now emits two blank rows below the footer with the skeleton and moves the cursor back over them. The prompt renders into those pre-scrolled rows, so the settled table doesn't move at exit; the scroll happens at Enter-time with the skeleton paint, where scroll is expected. The exact prompt height is unknowable, but the failure modes are asymmetric, so a constant works: extra reserved rows are consumed invisibly by whatever prints next (verified in tmux: a 1-line prompt leaves scrollback byte-identical to before), while a prompt taller than 3 rows scrolls by just the difference. Two rows absorb fish's default (1), starship's default (2), and tide/powerlevel10k (typically 3). Details: - The viewport budget goes from `h − 4` to `h − 6` so a bottom-pinned skeleton plus reserve can't scroll its own header off, which would break the `MoveUp` redraw math. Short terminals show two fewer skeleton rows; the overflow finalize still prints the full table at the end. - The overflow finalize path re-emits the reserve after its reprint. - The resting cursor position (line after the footer) is unchanged, so the in-place update code needed no changes. - The PTY test harness now exposes the final cursor position; the overflow test asserts the cursor rests two reserved rows above the bottom, and the fast-command test pins the resting position (catching an emit-without-`MoveUp` mismatch). Verified end-to-end in tmux (fish, 3-row prompt, full screen, 11 worktrees): the header row stays fixed across run, exit, and prompt, where the released binary jumps two rows. The error path (post-table error blocks printed after finalize) consumes the reserve and keeps today's behavior, which is fine: new content appearing at completion legitimately grows the screen. 🤖 Generated with [Claude Code](https://claude.com/claude-code) > _This was written by Claude Code on behalf of max_ Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
1d46c0083b |
fix(test): treat Linux PTY EIO-on-close as EOF in read helpers (#3398)
## Problem
The `code-coverage` job failed on [run
29053905721](https://github.com/max-sixty/worktrunk/actions/runs/29053905721)
(default branch, commit
|
||
|
Maximilian Roos
|
830cc850dd |
feat(list): show the main…± column by default; --full gates only off-machine columns (#3236)
Move the `main…±` branch-diff column (line diffs since the merge-base) into the default `wt list` view — it's pure local git backed by a persistent content-addressed cache, so the original blocking-walk concern no longer applies. `--full` now gates only the two off-machine columns: CI status (network) and LLM branch summaries. The interactive picker (`wt switch`) follows suit and is effectively `wt list --full`; on narrow terminals with the preview shown, CI clips past the split and alt-p reveals it. Also adds a `.typos.toml` ignore rule for truncated word fragments glued to the … ellipsis, so the narrower Message column's truncated quickstart embed doesn't get spell-"corrected" by pre-commit.ci. |
||
|
Maximilian Roos
|
958b9c188a |
refactor(picker): migrate to skim 4.8 (ratatui), drop vendored skim-tuikit (#3137)
Migrates the `wt switch` picker from skim 0.20.5 (tuikit backend) to
skim 4.8.0 (ratatui/crossterm), removes the `vendor/skim-tuikit/` patch
tree we carried against the old line, and makes the full test suite
green under the new backend.
## Why the vendor tree goes away
We vendored skim-tuikit for two patches: `alt-<digit>` key parsing and
an `Output::flush` `write_all` fix for dropped bytes under PTY pressure.
Both are moot in 4.8. Key parsing is native — `binds.rs` routes every
single character (digits included) through `KeyCode::Char` rather than a
hardcoded `alt-a..alt-z` arm list. The flush fix is subsumed by stdlib
`BufWriter::flush` in the ratatui backend, which loops over partial
writes correctly. So the migration carries zero vendor patches against
skim, and the build machinery that kept the vendored source alive
(Taskfile `vendor-diff`, flake `vendorSrc`/`extraDummyScript`) is
deleted too.
## The picker rewrite
skim 4.x is a different API. `WorktreeSkimItem::display()` now returns
`ratatui::text::Line` instead of an `AnsiString`. alt-r removal rides
skim's `reload(remove {})` token (the selected row's `output()`,
expanded into the reload command) instead of the old
`as_any().downcast_ref` path (which never worked across compilation
units) plus a signal file. Preview-tab switching and the progressive
list redraw are driven by `Skim::event_sender()` +
`Event::Render`/`Event::RunPreview`.
## The regression that motivated the bulk of this
skim 4.x renders on demand. There is no 100ms tuikit heartbeat
re-rendering the frame, so the progressive `wt switch` list rendered
blank while collection ran in the background — the original report. The
fix pokes `Event::Render` from the list-collect callbacks (throttled to
16ms). The same on-demand model surfaced four more regressions, all
fixed here and verified head-to-head against the 0.20 picker in a
terminal: preview-tab lag, current-row highlight, Shift-Tab back-cycle
(crossterm reports Shift-Tab under three distinct `KeyEvent` shapes, so
all three are bound), and alt-r removal (4.x `execute-silent` is
fire-and-forget and raced its reader).
## Help text and the test harness
Two things the integration suite caught that the unit tests did not (the
prior pass ran `--lib --bins` only):
- **Styled help.** skim 0.20 transitively pulled in
`clap/unstable-markdown`, which renders worktrunk's `///` doc-comments
as styled help. skim 4 with `default-features = false` drops it, so help
reverted to raw text (`[experimental]` → `\[experimental\]`). Restored
by depending on `unstable-markdown` explicitly, keeping help output
identical and the `test_help` / `test_step_alias` /
`test_docs_are_in_sync` snapshots passing unchanged.
- **PTY harness.** skim 4.x queries cursor position (`ESC[6n`) at
startup in partial-height mode and blocks in `select()` for the reply;
`portable_pty` is a bare PTY and never answered, so every
`switch_picker` test failed init with "Cursor position detection timed
out." The Unix harness now answers the query, mirroring the existing
ConPTY responder. skim 4.x also draws the list/preview separator one
column left, so the snapshot panel-split columns shifted to match. The
regenerated picker snapshots show two cosmetic changes: the match
counter no longer overlaps the preview tab header, and the HEAD column
shows the full short-SHA instead of a truncated one.
## Minimal-versions CI
The skim 4.8 dep tree pins tighter floors than our manifests declared,
so the nightly `minimal-versions` job needed updating. It now runs `-Z
direct-minimal-versions` — minimize only our own direct deps and let
transitive crates resolve normally — and the manifests raise each
under-specified floor to the minimum the workspace builds against
(largely mirroring skim 4.8's own requirements). Full `-Z
minimal-versions` would instead drag in skim's transitive TUI/image
stack (ansi-to-tui, ratatui's `instability` macro, color-eyre,
ratatui-image → image/avif), whose crates under-declare their floors and
don't compile at the picked versions; direct minimization confines the
check to floors we own, so no transitive pins are needed. Normal
resolution (the committed `Cargo.lock`) is unchanged. Full floor list
and the `signal-hook` libc-pin detail are in the `ci(min-versions)`
commit message.
## Reviewing
Start at `src/commands/picker/mod.rs` (the `run_skim` entry point, the
action keybinds, and `parse_reload_remove_token`), then
`progressive_handler.rs` (the render pokes) and `items.rs` (the
`Line`-based `display()`). Test-harness changes are in
`tests/common/pty.rs` (the `ESC[6n` responder) and
`tests/integration_tests/switch_picker.rs` (the panel-split columns).
The rest is the dependency swap, the vendor and build-machinery
deletions, and regenerated snapshots.
All 3980 tests pass (`cargo run -- hook pre-merge --yes`), including the
`shell-integration-tests` PTY suite that exercises the picker end-to-end
across the list, previews, scroll, create/remove, and accept flows. No
automated test drives a real terminal; the interactive surface was also
checked by hand against the 0.20 picker.
> _This was written by Claude Code on behalf of max_
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
d31663e2fa |
test: drop env-scrub workaround, bump insta-cmd to 0.7 (#3076)
## What Removes the `drop_empty_env_entries` snapshot redaction (and its unit test) now that [insta-cmd 0.7](https://crates.io/crates/insta-cmd/0.7.0) fixes the underlying problem at the source, and bumps the dev-dependency `0.6` → `0.7`. ## Why insta-cmd records a command's environment in the snapshot `info:` block from `Command::get_envs()`, which yields `None` for a var removed via `env_remove` and `Some("")` for one deliberately set to empty. Under 0.6 both collapsed to `KEY: ""`, so a removal was indistinguishable from a real empty — and since `isolate_subprocess_env` removes whichever `GIT_*` / `WORKTRUNK_*` keys happen to exist in the *parent* environment, the set of `KEY: ""` markers depended on the host. That churned regenerated snapshots between machines. worktrunk worked around it by stripping every empty-valued entry from the `env:` block. insta-cmd 0.7 ([mitsuhiko/insta-cmd#20](https://github.com/mitsuhiko/insta-cmd/pull/20)) instead drops removals at the source and keeps `Some("")` as `KEY: ""`, so the workaround is no longer needed — and removing it lets deliberate empties be recorded truthfully. ## Refreshed snapshots - `list_config_env_override_validation_failure` **gains** `WORKTRUNK_WORKTREE_PATH: ""` — the var the test sets to empty to trigger the "worktree-path cannot be empty" warning, now recorded faithfully instead of dropped. - Two `step_squash_show_prompt` snapshots **lose** stale `GIT_DIR`/`GIT_EDITOR`/`GIT_WORK_TREE`/`NO_COLOR`/`SHELL`/`PSModulePath` (etc.) `env_remove` markers — committed host-dependent noise that 0.7 no longer records. Deliberately excluded one incidental refresh: `switch_nonexistent_with_fetch_time`'s `WORKTRUNK_TEST_EPOCH` is a wall-clock-derived value (FETCH_HEAD mtime + 3h) that drifts every run — unrelated to this fix (a non-empty dynamic value), harmless since the `env:` block isn't compared. `tests/CLAUDE.md` is updated to describe the 0.7 behavior in place of the workaround. ## Testing `cargo test --test integration` (1807 passed), `cargo clippy`, `cargo fmt`, and `pre-commit` all pass locally. The `shell-integration-tests` feature set was not run locally (covered by CI); it's unaffected — the only other deliberate empty, `GIT_PREFIX=""` in `bare_repository.rs`, goes through `.output()`, not a snapshot. > _This was written by Claude Code on behalf of max_ Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
aebe278a2c | refactor(tests): drop empty-valued env entries from snapshot headers (#3063) | ||
|
Maximilian Roos
|
5fba0bdc9b | refactor(tests): remove byte-normalization snapshot filters (#3061) | ||
|
Worktrunk Bot
|
4bf6ef750b |
test: make snapshot env blocks robust to the contributor's environment (#3037)
Opened at @max-sixty's request in https://github.com/max-sixty/worktrunk/pull/3021#issuecomment-4682713506 — make running/regenerating tests robust to the contributor's local environment so nobody has to match CI's `GIT_*` setup. ## The problem `insta_cmd` records every env var a test command sets **or removes** — Rust's `Command::env_remove` registers in `get_envs()`, and insta-cmd serializes a removal as `KEY: ""`. `isolate_subprocess_env` scrubs whichever `GIT_*` / `WORKTRUNK_*` keys exist in the **parent** environment, so the recorded `env:` block depends on the host: - CI has `GIT_EDITOR` set → `GIT_EDITOR: ""` - A contributor's box might have `GIT_PAGER` → `GIT_PAGER: ""`, or neither, or both This is what bit #3021: `help_list_long.snap` / `help_list_narrow_80.snap` were regenerated on a box with `GIT_PAGER` and picked up `GIT_PAGER: ""` instead of CI's `GIT_EDITOR: ""`. Note this is **cosmetic, not a CI failure**: insta's `matches_fully` compares only the snapshot body (`self.contents == other.contents`), never the `info:`/`env:` metadata. But the spurious machine-to-machine diff is exactly what makes contributors think they have to match CI's environment. ## The fix A dynamic `.env` redaction in `add_standard_env_redactions` (`strip_host_scrubbed_env_markers`) drops every empty-valued `GIT_*` / `WORKTRUNK_*` entry from the recorded block. It runs as its own pass *after* the per-key `.env.*` redactions, so affirmatively-set vars (config paths → `[TEST_CONFIG]`, coverage → `[LLVM_PROFILE_FILE]`) already hold non-empty placeholders and are kept. The explicitly-named unconditional scrubs (`NO_COLOR`, `SHELL`, `PSModulePath`) are host-independent and left intact. Result: regenerating on any machine produces the same `env:` block, regardless of the contributor's `GIT_*` environment. ## Why no snapshots are regenerated The `info:` block is never compared, so a metadata-only difference never triggers a rewrite — even `INSTA_UPDATE=always` leaves an unchanged-body snapshot alone. Existing committed snapshots keep their (now-redundant) markers until their body next changes, at which point the rewrite drops them cleanly. This avoids a disruptive ~1000-file cosmetic diff that would conflict with every in-flight snapshot PR; the robustness applies to all future regenerations from this point on. If you'd prefer the tree uniform now, a follow-up `cargo insta test --accept` bulk regen is a clean separate commit — happy to push it. ## Verification - New unit test `strip_host_scrubbed_env_markers_normalizes_env_block` covers drop/keep/passthrough. - Confirmed end-to-end: deleting `help_list_long.snap` and forcing a fresh write drops `GIT_EDITOR: ""` while keeping `NO_COLOR`/`SHELL`/`PSModulePath` and the `"0"`-valued `WORKTRUNK_TEST_*` vars. - `help` (57), `snapshot_formatting_guard` (6), and `ci_status` (68) snapshot suites pass; `cargo fmt --check` and `cargo clippy --tests --all-features` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
f2a8cb696d |
refactor(styling): end bash-gutter lines at their content, not a reopened dim (#3056)
Dispatched to fix a reported ANSI dim-bleed after bash-gutter blocks: in committed snapshots, multi-command hook announcements appear to inherit an unclosed `[2m` from the preceding gutter line (e.g. `post_start_named_commands.snap`, where the gutter line ends `'Installing deps'[0m[2m`). Diagnosis: the bleed doesn't exist in real output. Fresh `cat -v` captures of `wt hook pre-merge --yes` and `wt switch --create` with two-key hook tables show every gutter line closing with a full `[0m`, and later `◎ Running …` lines rendering un-dimmed. The snapshots are misleading by construction: the cross-platform filter in `tests/common/mod.rs` deletes every line-final `[0m` before snapshotting, and the formatter reopens dim after the last highlight token before its per-line reset, so filtered snapshots end `[0m[2m` and read exactly like a dangling dim. The change removes that misleading byte pattern at the source: phase 2 of `format_bash_with_gutter_impl` strips the no-op reopened dim (and the lone dim on blank lines) before appending each line's closing reset. Rendering is identical; lines now end at their content plus one reset. Also bundled: - A CAUTION comment on the reset-stripping snapshot filter, so future readers don't diagnose SGR bleed from `.snap` bytes. - `config_show_theme` now binds the standard env redactions; regenerating its snapshot leaked a host `LLVM_PROFILE_FILE` path and tripped `test_no_host_specific_paths_in_snapshots` (pre-existing gap, invisible until regeneration since insta never compares `info:` blocks). - 108 regenerated snapshots. Verified mechanically: ANSI-stripped bodies are byte-identical; raw diffs are confined to line-end SGR sequences plus stale `env:` header refreshes (`RUST_LOG: warn` from an older harness). Possible follow-up, not done here: the line-final-reset filter itself may be vestigial (anstream pass-through suggests piped output is identical across platforms now); removing it would make snapshots byte-truthful but churns nearly every snapshot and needs Windows CI to confirm. The first Windows CI run caught a real latent bug the strip exposed: askama strips a template's final newline, so on a CRLF checkout (Windows autocrlf) the fish wrapper render ends with a lone `\r` that the formatter's pair-wise CRLF normalization missed. The `\r` reached tree-sitter and came back as a trailing token after the highlight closed, defeating the end-of-line cleanup (and historically invisible because insta trims trailing whitespace when comparing). Fixed by trimming trailing `\r` in the formatter's normalization, plus `templates/* text eol=lf` in `.gitattributes` since CRLF templates embedded at compile time would leak `\r` into the shell code Windows-built binaries emit at runtime. 🤖 Generated with [Claude Code](https://claude.com/claude-code) > _This was written by Claude Code on behalf of max_ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
bd278e5d1f |
test: guard committed snapshots against host-specific paths (#3026)
## Guard committed snapshots against host-specific paths Follow-up to #3009, which fixed one instance of this class; investigating the rest of the corpus turned up two more committed leaks — from two *different* machines (`/var/folders/v3/…` and `/var/folders/wf/…`), direct proof of cross-machine churn. ### The mechanism insta compares only snapshot content (stdout/stderr/exit); the `info:` block insta-cmd records (`args:`, `env:`) is metadata that is written but never compared, and `add_filter` doesn't apply to it. A test missing a redaction bakes the generating machine's paths into the committed file while passing everywhere — surfacing only as churn when someone regenerates the snapshot elsewhere. Under libtest, the `repo` fixture's leaked settings binding (rstest has no teardown) can additionally mask the omission, so what gets committed depends on runner and thread scheduling. ### Enforcement at the boundary `test_no_host_specific_paths_in_snapshots` (in `snapshot_formatting_guard.rs`, whose walker now recurses over all ~1200 snapshots under src/tests/docs instead of 2 flat dirs) fails on any committed `.snap` line carrying a host-specific marker: macOS `/var/folders/`, the `wt-test-profraw` LLVM fallback dir, tempfile's `.tmpXXXXXX`, or a home path with a non-fake username. A detector self-test pins sensitivity in both directions, since a clean corpus would otherwise pass vacuously if a regex rotted. Pre-fix, the test caught exactly the two known leaks across 1182 files with zero false positives. Why not fix the fixture leak itself: storing the insta guard in `TestRepo` is structurally impossible (insta is a dev-dependency invisible to the lib crate defining `TestRepo`; the guard is `!Send` by design; rstest has no teardown), and a wrapper type would churn ~1700 test signatures to fix a divergence the corpus guard makes harmless. The fixture comment now documents the real contract instead of its previous (false) safety claim. ### The two leaks - `list_with_c_flag`: the temp repo path leaked through the **`args:` block** (`wt -C <root>`) — a channel `.env.*` redactions can't reach. Fixed with a `.args[]` dynamic redaction mirroring the body filters' `_REPO_…` form. - `quickstart_merge`: `WORKTRUNK_COMMIT__GENERATION__COMMAND` pointed at a mock llm binary in the per-test temp dir. Fixed subtractively — the test now uses the suite's standard inline `cat >/dev/null && echo '…'` command (snapshot body unchanged), and the now-unused `create_mock_llm_quickstart` plus the never-called `create_mock_llm_api` are deleted. Also: `test_list_empty_repo` holds a scoped settings guard instead of a cargo-culted `mem::forget`, and tests/CLAUDE.md documents the args channel, the guard test, and the libtest-vs-nextest caveat (nextest is authoritative). Local gate: 3906 tests + lints pass; review iterated to convergence. > _This was written by Claude Code on behalf of max_ Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
b75b3cf1a7 |
fix(shell): install nushell wrapper to vendor-autoload dir (#2878) (#2992)
Fixes #2878. Implements the approach agreed in the issue: query `$nu.vendor-autoload-dirs` directly and install to its last (user-writable) entry, with cross-platform tests and stranded-file cleanup on install/uninstall. ## The bug The nushell integration wrote `wt.nu` to `$nu.default-config-dir/vendor/autoload`, which Nushell **never autoloads**. It happened to work on macOS/Windows because there `$nu.default-config-dir == $nu.data-dir`, so the wrong subpath resolved to the real vendor dir; on Linux the wrapper was written but silently never loaded — so `wt` was never wrapped. Verified against `nu` 0.113.1: `$nu.vendor-autoload-dirs | last` is `<data-dir>/vendor/autoload`, and `$nu.default-config-dir/vendor/autoload` is in neither `$nu.vendor-autoload-dirs` nor `$nu.user-autoload-dirs`. ## The fix - **Resolve the install target from `$nu.vendor-autoload-dirs | last`** (one memoised `nu` spawn that also reads `$nu.default-config-dir` for legacy cleanup). - **Fallback when `nu` isn't on PATH** mirrors nushell's own `nu_path::data_dir`: `XDG_DATA_HOME` (when absolute) wins on every platform, otherwise `dirs::data_dir()` — matching nushell on Linux/macOS/Windows. - **Wrapper-based shells now always write to the canonical location** (`paths.first()`), so a wrapper stranded at a legacy path can't become the write target. (No-op for fish, which has a single canonical path.) - **Stranded-file cleanup**: install writes the correct path and removes any worktrunk-managed wrapper left under the legacy `<config-dir>/vendor/autoload` locations; uninstall already iterates every candidate, which now includes those legacy paths. Legacy files are only removed when they carry the `# worktrunk shell integration for nushell` header, so a user's own `wt.nu` is never touched. - `config_line`, the `wt config shell init` manual-setup help, and the FAQ/shell-integration docs now use `$nu.vendor-autoload-dirs | last | path join wt.nu`. ## Tests - New `WORKTRUNK_TEST_NU_VENDOR_AUTOLOAD_DIR` override pins the install dir so the install/uninstall/cleanup tests are **deterministic on Linux, macOS, and Windows** and don't depend on `nu` being installed. - New `test_nushell_install_target_is_a_vendor_autoload_dir` (gated on `shell-integration-tests`, where CI has `nu`) runs the **real `nu` flow** and asserts the wrapper worktrunk wrote is a member of `$nu.vendor-autoload-dirs`. This fails against `main` (the old config-dir path is in neither autoload list) and passes with the fix. - New `test_nushell_install_cleans_stranded_legacy` and a reworked `test_uninstall_nushell_cleans_all_candidate_locations` cover the migration cleanup. - Existing nushell tests updated to the data-dir path; full suite green (`cargo test --lib` + `cargo test --test integration`, and the nushell subset with real `nu` under `--features shell-integration-tests`). ## Not in scope "Not best practice" cleanup flagged in the issue (the overlap between the candidate-building helpers, and the dubious Windows branch) — the helpers here are restructured around the autoload model, but a broader tidy can be a follow-up. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
ec62580c29 |
revert(hooks): keep docs on pre-start/post-start; code accepts both (#2857)
Per @max-sixty's [direction in #2838](https://github.com/max-sixty/worktrunk/issues/2838#issuecomment-4509447593): revert the docs portion of #2840 and keep the code. Docs continue to recommend `pre-start`/`post-start`; both names work in code so anyone who already followed the briefly-changed docs (e.g. @EcksDy) isn't stranded once a release ships these aliases. ## User-visible — back to `pre-start`/`post-start` - README, docs site, skill mirrors, `dev/*.example.toml`, `plugins/worktrunk/README.md`, `flake.nix`, `.config/wt.toml` - `src/cli/mod.rs` / `src/cli/config.rs` / `src/cli/step.rs` / `src/help.rs` after_long_help and example snippets — and the auto-synced `docs/content/` and `skills/worktrunk/reference/` mirrors - `wt hook --help` canonical subcommand names; completion advertises `-start` only - `HookType` Display via strum, serde `rename`, and clap `ValueEnum` name — all `pre-start`/`post-start`. The Rust variant identifiers stay `PreCreate`/`PostCreate` (internal; we already paid for that rename in #2840, and now the eventual flip is a Display-only change) - `HooksConfig` serde canonical fields ## `*-create` still works (kept code) - `wt hook pre-create` / `post-create` — CLI alias on the canonical subcommand - `pre-create` / `post-create` in config: top-level, `[hooks.*]`, and per-project, in string, `[table]`, and `[[array-of-tables]]` form. Mechanism: serde `alias = ...` on the field, plus a silent in-memory rename in `migrate_content()` so the round-trip in `unknown_tree` doesn't flag table forms as schema-unknown. - The pre-0.32.0 `post-create` fatal-load-error machinery stays removed — the name is reclaimed, and both forms load without error. ## Smaller bits - `valid_user_config_keys()` / `valid_project_config_keys()` append `pre-create` / `post-create` so the unknown-field round-trip skips them. `test_valid_*_keys_all_deserialize` skips both aliases (they can't sit alongside the canonical without a duplicate-field error). - `DEPRECATED_SECTION_KEYS` drops the `pre-start`/`post-start` entries #2840 added — `pre-start`/`post-start` are canonical again. - `find_pre_start_from_doc` / `find_post_start_from_doc` / `find_renamed_hook_key` / `is_non_empty_item` / `migrate_start_hooks_doc` and their tests are removed; the migration direction flips via a new `migrate_create_hooks_doc` (silent, mirrors the prior shape). - Test files `e2e_shell_post_create.rs` and `post_create_commands.rs` rename back to `_post_start_` (via `git mv`, so the rename shows as a rename). ## Testing `cargo run -- hook pre-merge --yes` — 3806 tests pass; the 10 failures are all `case_4` of `shell_wrapper::unix_tests::*` (nu-shell case; `nu` isn't installed in this runner; same failures occur on `main`). Also manually verified that a fresh `wt switch --create` against a project config with `[post-create]` loads cleanly with no unknown-field warning and the hook fires as `post-start`. ## Follow-up Per @max-sixty: in a couple of weeks, once a release with both-names-work is out and users have had a chance to upgrade, the docs flip is straightforward (most of it is in `src/cli/mod.rs`'s `after_long_help` and the doc-sync test propagates). Re #2838. Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
d7e3f88422 |
feat(hooks): rename worktree-creation hooks to pre-create/post-create (#2840)
Phase 1 of the staged hook rename tracked in #2838: the worktree-creation hooks `pre-start`/`post-start` become `pre-create`/`post-create`. The old names keep working with no deprecation warning yet (Phase 2, months out, adds the warning). ## What changes - `pre-create`/`post-create` are canonical everywhere: the `HookType` enum, the `HooksConfig` serde fields, the `wt hook` CLI, completion, and all docs. - Old names keep working: `migrate_content()` rewrites `pre-start`/`post-start` config keys to `-create` before serde, and `parse_hook_type` accepts the old CLI names as silent aliases. `wt config update` rewrites them on disk; `wt config show` shows the migration diff. `wt hook <type>` execution and `wt hook show` both accept the old names; completion and `--help` advertise only the canonical names. - `detect_deprecations()` flags the old keys so `update`/`show` act on them, but `format_deprecation_warnings()` stays silent (Phase 2 adds the warning). A new empty-warnings guard in `check_and_migrate` keeps a `-start`-only config from emitting a stray hint. - The dead pre-0.32.0 `post-create` machinery is removed: the fatal `POST_CREATE_REMOVED_MSG` load error, the vestigial `HooksConfig.post_create` merge-fold, and `find_post_create_from_doc`. `post-create` is reclaimed as the canonical background creation hook. ## Semantic flip Before v0.32.0, the key `post-create` named a *blocking* hook. It now names the *background* one. Since v0.44.0, a pre-0.32.0 `post-create` config is a fatal load error on the `check_and_migrate` paths: `ProjectConfig::load` and user/system config loading, which fire on essentially every `wt` command. A repo carrying one has been unusable ever since. The one path that skips that check is `project_config_at_ref` (the base-ref read behind `wt switch --create`), which applies only structural migration. A pre-0.32.0 `post-create` surviving solely on a base ref, never checked out into a worktree, would now load as a background hook rather than folding into the blocking `pre-start`. That edge case is accepted: once `post-create` is valid again, reclaiming the name and detecting the dead key are mutually exclusive. ## Reviewing this diff 205 files, but the substance is ~36 files under `src/`. The rest is regenerated snapshots and auto-synced doc mirrors. Start with: - `src/config/deprecation.rs` — detection (`find_renamed_hook_key`), migration (`rename_hook_key`), removal of the fatal block, the empty-warnings guard, and the `DEPRECATED_SECTION_KEYS` entries that stop unknown-field detection from flagging the migrated keys. - `src/config/hooks.rs`, `src/git/mod.rs` — the serde field and enum renames. - `src/config/project.rs` — `ProjectConfig::load` deserializes `check_and_migrate`'s migrated content, so a current-worktree config using the old keys loads into the canonical fields. - `src/cli/hook.rs`, `src/commands/hook_commands.rs`, `src/completion.rs`, `src/main.rs` — the CLI alias layer; `wt hook show` accepts the old type names as hidden value-parser aliases. - `src/cli/mod.rs` — the `wt hook` docs, including the soft-deprecation note linking #2838. The ~93 modified snapshots also pick up deterministic env-block lines (`GIT_*: ""`, `LLVM_PROFILE_FILE`) that pre-existing snapshots already carry. That is stale-snapshot drift surfaced by the regeneration, not a behavior change. ## Testing Full suite green (3799 tests). New coverage: `snapshot_migrate_start_to_create` (migration preserves value shape and position), `test_deprecated_start_hook_key_runs_silently` and `test_standalone_hook_start_alias_runs_silently` (old config and CLI names run with no warning), `test_config_show_displays_start_hook_migration` (`config show` reveals the diff without an "unknown field" warning), and `test_hook_show_accepts_deprecated_start_hooks` (a current-worktree config using the old keys loads, and `wt hook show` takes both the canonical and the deprecated type arguments). Part of #2838. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Maximilian Roos
|
6174f66488 |
feat(list,statusline): include untracked in HEAD± for --full and statusline (#2764)
`wt list --full` and `wt statusline` now count untracked-file lines in the `HEAD±` working-diff segment, matching what `wt step diff` shows. Default `wt list` and the picker stay on the cheap `git diff --shortstat HEAD` path (tracked-only). Putting the untracked-inclusive variant behind the same `--full` boundary as `BranchDiff` / `CiStatus` / `SummaryGenerate` means the explanation stays simple — "`--full` includes untracked too" — with no new exception class. ## Mechanism `WorkingTree::working_tree_diff_stats_with_untracked()` registers untracked files in a temporary copy of the index via `git add --intent-to-add .`, then runs `git diff --shortstat HEAD` against that temp index. Same trick `wt step diff` uses; the worktree's real index is never touched (a unit test diffs the index bytes before/after to lock that contract in). `WorkingTreeDiffTask` branches on the new flag and on the cached porcelain — when no untracked entries exist, the expensive path is skipped (the two paths are bit-equivalent for tracked-only worktrees). ## Refactor A new `TempIndex` helper consolidates the "copy index + stage stuff into it + run a follow-up git command" pattern that was previously open-coded in three places (`wt step diff`, `WorkingTreeConflictsTask::write_tree_with_working_tree`, and the new method above). `TempIndex::git(args)` returns a `Cmd` already wired with `current_dir`, log context, and `GIT_INDEX_FILE`, so each caller collapses to a few lines. ## Wiring A new `include_untracked_in_working_diff: bool` on `CollectOptions` flows through `TaskContext` to `WorkingTreeDiffTask`. Set from `show_full` in `collect/mod.rs`, hard-coded `true` in `statusline.rs`, hard-coded `false` for the picker (the only other `ShowConfig::Resolved` consumer, which intentionally runs the same fast bucket as default `wt list`). ## Snapshot env scrub fix (second commit) A pre-existing latent bug surfaced when regenerating the six `--full` / statusline snapshots: `LLVM_PROFILE_FILE` (set on every test subprocess by `isolate_subprocess_env` since #2730) leaked into the env block as a platform-specific tempdir path. The intended scrub at `setup_snapshot_settings` was using `add_filter`, which only substitutes on captured snapshot content — never on the YAML info/env block — so it was dead code. Replaced with `add_redaction(".env.LLVM_PROFILE_FILE", ...)` (and the same for `CARGO_LLVM_COV*`) inside `add_standard_env_redactions`, which is already bound by the `repo` rstest fixture; values now render as stable `[LLVM_PROFILE_FILE]` placeholders matching the existing convention for HOME / GIT_CONFIG_GLOBAL / etc. ## Tests 3695 pass: one new unit test for `working_tree_diff_stats_with_untracked` (verifies untracked lines counted, real index byte-identical), plus snapshot updates for nine `--full` / statusline tests where untracked files now contribute to `HEAD±`. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
bd4c0455da |
fix(testing): default LLVM_PROFILE_FILE to a temp-dir path when not inherited (#2730)
## Summary Defaults `LLVM_PROFILE_FILE` to `std::env::temp_dir()/wt-test-profraw/cov-%m_%p.profraw` when nothing is inherited from the parent. `isolate_subprocess_env` previously only *propagated* the var (per #2713), leaving it unset under plain `cargo test`. An instrumented child — most commonly a stale `mock-stub` left instrumented by an earlier coverage build — then fell back to writing `default_<hash>_<pid>.profraw` into its cwd. For any `wt list` snapshot that spawns a mock, that cwd is the test worktree, so `git status --porcelain` saw the profraw as an untracked file and `wt list` rendered "Showing 5 worktrees, 1 with changes, …" instead of "Showing 5 worktrees, 4 ahead". Different subsets of the gitea CI-status tests would flake every parallel run. `default_llvm_profile_file()` is a thin wrapper over `default_llvm_profile_file_with(inherited)` (same outer/inner split as `isolate_subprocess_env` / `isolate_subprocess_env_from`) so a unit test exercises both the inherited and the temp-dir-fallback branches without mutating process env. `COVERAGE_ENV_VARS` is shrunk to `[CARGO_LLVM_COV, CARGO_LLVM_COV_TARGET_DIR]` — `LLVM_PROFILE_FILE` is no longer in it because it's now governed by `default_llvm_profile_file()` — and all three call sites (`isolate_subprocess_env`, `pass_coverage_env_to_pty_cmd`, `configure_pty_environment`) route through that helper plus the constant. Under `cargo llvm-cov`, behavior is unchanged: the inherited `LLVM_PROFILE_FILE` wins, and profraws still land at the path the runner chose. The `%m_%p` placeholders are expanded by the LLVM runtime in the instrumented child; uninstrumented children ignore the env var entirely. The CI guard from #2719 will catch any future helper that bypasses this contract. ## Test plan Verified against a deliberately-reinstalled instrumented `mock-stub` each time: - [x] 5× `cargo test --test integration -- ci_status::test_list_full_with_gitea` — 9/9 pass each run (was flaking ~50%) - [x] 5× `cargo nextest run --test integration ci_status` (67 tests, parallel) — all pass each run - [x] `cargo llvm-cov nextest --test integration ci_status::test_list_full_with_gitea_pr_status::case_1` — passes; profraws land in `target/llvm-cov-target/…` (cargo-llvm-cov's chosen path), not in the worktree - [x] `cargo test --test integration -- ci_status` (full 67) — pass - [x] `cargo test --lib testing::tests::default_llvm` — both new unit tests green (inherited branch + temp-dir fallback) - [x] `cargo clippy --workspace --tests --features shell-integration-tests -- -D warnings` — clean - [x] No stray `default_*.profraw` anywhere after the runs; redirected profraws land in `$TMPDIR/wt-test-profraw/cov-%m_%p.profraw` --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
434b8475d0 |
fix(testing): propagate LLVM coverage env from isolate_subprocess_env (#2713)
## Summary Move the `LLVM_PROFILE_FILE` / `CARGO_LLVM_COV*` passthrough loop into `isolate_subprocess_env` and hoist the env-var list to a shared `COVERAGE_ENV_VARS` constant. Today the passthrough only runs inside `configure_cli_command` (the test-side superset). Benches use `isolate_subprocess_env` directly per its docstring, so any future caller that `env_clear()`s before us would silently drop the coverage env and the instrumented child would write `default_<hash>_<pid>_*.profraw` next to its cwd instead of the path `cargo llvm-cov` chose. The original report was a stray `default_*.profraw` at the worktree root from an earlier session. I could not deterministically reproduce the leak under `cargo llvm-cov --benches` or `cargo llvm-cov --test integration` after auditing the test/bench call sites, so this is defense-in-depth at the helper that owns the contract rather than a targeted root-cause fix — flagged in commentary above the new const. ## Changes - `src/testing/mod.rs` — new `pub const COVERAGE_ENV_VARS`; `isolate_subprocess_env` re-applies them at the end; matching block removed from `configure_cli_command`. - `tests/common/mod.rs`, `tests/common/progressive_output.rs` — the PTY helpers (`pass_coverage_env_to_pty_cmd`, `configure_pty_environment`) iterate the same constant. `.gitignore` is intentionally untouched so a future regression surfaces in `git status` instead of getting silently hidden. ## Test plan - `cargo run -- hook pre-merge --yes` — green (3627 tests passed, pre-commit hooks clean, doctest + cargo doc clean). - `cargo llvm-cov --benches --no-report -- skeleton/warm/1` — green, no stray `*.profraw` at repo root. - `cargo llvm-cov --test integration --no-report -- 'integration_tests::shell_wrapper::...test_bash_completion_produces_correct_output' '...user_hooks::test_var_flag_invalid_format_fails' '...readme_sync' '...help::'` — 61 passed, no stray `*.profraw`. - Existing `isolate_subprocess_env_scrubs_git_and_worktrunk_keys` unit test still passes. Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
0309fca3e0 |
test(snapshots): redact injected latest-version env so config-show snapshots don't churn (#2687)
The four `wt config show --full` tests inject
`WORKTRUNK_TEST_LATEST_VERSION` to make the version-check (`curl` to
GitHub releases) deterministic. Two of them pass
`env!("CARGO_PKG_VERSION")` so the line renders "Up to date". insta-cmd
records that value in the snapshot's `info.env:` metadata block, so
every release bump left the block stale (`0.47.0 → 0.48.0 → 0.49.0 …`)
until an unrelated bulk re-accept happened to refresh it. Tests never
*failed* on the drift — insta compares snapshot contents, not the `info`
header — but it's needless churn.
This adds a dynamic redaction for `.env.WORKTRUNK_TEST_LATEST_VERSION`
in `add_standard_env_redactions` (the helper every snapshot setup
calls): any semver-shaped value maps to `[VERSION]`; the non-semver
`"error"` sentinel from
`test_config_show_full_version_check_unavailable` passes through
unchanged. The injection stays faithful — `latest == current ⇒ "Up to
date"` — only the recorded metadata is normalized. A
`Settings::add_filter` (regex-on-text) can't reach the `info` block;
it's structured metadata redacted by path selector, like `HOME`/`PATH`,
so this had to be a redaction.
Re-accepted the three affected snapshots — the `env:` line is the only
change in each (`0.48.0`×2 and the `99.0.0` update-available sentinel →
`[VERSION]`).
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
5144b1fbdc |
style(output): bold paths in warning messages (#2677)
Two follow-ups from the #2674 review session, plus a coverage fix. `style(output)`: with the cross-platform bold-around-placeholder trap closed in #2674, paths in warning messages can be styled per the `writing-user-outputs` convention. Bolds the path in four warning sites — config-file-not-found (user config load), failed-to-remove-deprecated (legacy fish migration), completions-not-configured and outdated-shell-extension (both in `wt config show`). `refactor(test)`: inlines `setup_snapshot_settings_impl` into its two callers. The wrapper only allocated an empty worktrees map; both callers now call `setup_snapshot_settings_for_paths_with_home` directly. `test(configure-shell)`: covers the legacy fish cleanup remove-failure branch that the styling commit lit up under `codecov/patch`. Unix-only — chmods the conf.d dir read-only so `fs::remove_file` returns EACCES, then asserts the install succeeds and emits the warning. Skips when running as root. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
6243ef9e6f |
test(filters): close cross-platform bold-around-placeholder trap (#2674)
## Summary `format_path_for_display` produces two shapes for the test-config path: - macOS: `/var/folders/.../.tmp.../test-config.toml` (absolute — HOME canonicalizes to `/private/var/...` but the path doesn't, so `strip_prefix` falls through) - Linux CI: `~/test-config.toml` (HOME == tempdir, prefix strips cleanly) `add_temp_path_placeholder_filters` only matched the absolute form. The follow-up bold-strip pass only fires on already-substituted placeholders, so on Linux the `[TEST_CONFIG]` placeholder was established later (via the generic `~/` → `_PARENT_/` filter and a test-specific filter), and any `<bold>` wrapping the path leaked through into snapshots. #2661 worked around this by reverting the `<bold>` styling on the user-config parse warning. The same trap was latent for any future code that bolded a redacted path; this PR closes it. ## Changes - **`tests/common/mod.rs`** — extend the path-substitution filter to match the tilde forms (`~/test-config.toml` and `~/.tmp.../test-config.toml`) so the placeholder lands consistently across platforms. Invoke the ANSI-strip pass at the end of every `setup_*_snapshot_settings` helper — `setup_snapshot_settings*`, `setup_home_snapshot_settings`, and `setup_temp_snapshot_settings` — so it sees every in-setup placeholder including `[PROJECT_ID]`, `[TEMP_HOME]`, and `[TEMP]` (all latent gaps). Strip list is targeted to path-redaction placeholders, so value placeholders (`[VERSION]`, `[HASH]`, `[BUILD_MODE]`, `[BINARY_PATH]`) keep their bold styling. - **`add_path_placeholder_filter` helper** — for test-specific path redactions added after setup (which the late strip pass can't reach). Used at the two `_REPO_/system-config.toml` callsites and documented as the canonical pattern. - **`tests/integration_tests/list_config.rs`** — drop the now-redundant `_PARENT_/...test-config.toml` filters; route the two system-config substitutions through the helper. - **`tests/CLAUDE.md`** — document the trap, the helper, and the contract that every `setup_*_snapshot_settings` ends with the strip pass. - **`src/git/repository/mod.rs`** — re-add `<bold>{path_display}</>` to the user-config parse warning, matching the `writing-user-outputs` convention. - **regression test** — exercises the macOS absolute form plus two Linux tilde forms (HOME == tempdir, HOME above tempdir) to assert they collapse to the same snapshot output. - **snapshot acceptance** — `test_config_show_invalid_user_toml` now shows `[1m~/.config/worktrunk/config.toml[22m` (literal path, legitimate user-visible bold preserved). ## Test plan - [x] `cargo run -- hook pre-merge --yes` (3548 tests + lints, all green on macOS) - [ ] CI (Linux/Windows) green — the actual cross-platform proof 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
1fe346be42 |
ci(nightly): re-enable nix-flake job & fix sandbox failures (#2648)
## Summary Restore the `nix-flake` nightly job (disabled in #2647) and fix the sandbox-specific test failures it surfaces. With this merged, the nightly cron run is back to gating against packaging-environment bugs. ## What's in here - **Build-mode snapshot redaction**: 49 snapshots hardcoded `target/debug/wt` in "Invoked as:" and diagnostic output, which broke under crane's release builds. Collapse both modes to `target/[BUILD_MODE]/wt` in the shared insta filter. - **Nix sandbox env**: add `pkgs.python3` and `pkgs.procps` to `worktrunk-tests`' native build inputs (needed by argv-quoting, post-start, and pgid-invariant tests). Replace `#!/usr/bin/env python3` with the absolute python path resolved at script-write time, since `/usr/bin/env` doesn't exist on NixOS. - **PTY filter ordering**: extend `add_pty_binary_path_filters`' alternation to match the `[BUILD_MODE]` placeholder so PTY snapshots still collapse to `[BIN]` after the prelude rewrite runs first (caught by worktrunk-bot review). - **One inherited-CWD test fix**: `test_config_init_already_exists` branched on whether the inherited CWD had a project config — pin it to a no-config tempdir so the snapshot is deterministic across cargo and nix. ## Test plan - [x] `nix-flake` job passes in CI (1633 / 1633, was 54 failing) - [x] All required `ci` checks pass - [x] Auto-reviewer approved 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
2f8807eb36 |
ci: consolidate slow checks onto the nightly workflow (#2636)
## Summary Move slow / advisory CI work onto `nightly.yaml` and add two new nightly jobs to close release-time gaps. **Moved from `ci.yaml` → `nightly.yaml`:** - `benchmarks` (full `cargo bench`, ~80 min) — was already advisory non-required (CLAUDE.md: *"Don't wait for CI benchmarks before merging"*) - `check-unused-dependencies` (`cargo udeps`) — needs nightly toolchain, rarely flips between cargo-touching PRs **Folded in:** - `nightly-benchmark.yaml` deleted; its time-series gist-append step is now the tail of the consolidated `benchmarks` job, gated on `event_name == 'schedule'` so PR/push runs don't pollute the gist **New nightly jobs:** - `release-target` matrix (`x86_64-unknown-linux-musl`, `aarch64-unknown-linux-musl`, `x86_64-apple-darwin`) — `dist-workspace.toml` ships these triples but the existing `test` matrix doesn't cover any of them. Today the first build of those targets is at release-tag time, which blocks releases instead of PRs. Runs the integration suite at default features (no `shell-integration-tests` — those PTY tests read `$SHELL` from the runner env, which resolves differently on `ubuntu-24.04-arm` and would mask musl/arm signal with environment noise). - `minimal-versions` — `cargo update -Z minimal-versions && cargo check`. Catches under-specified `Cargo.toml` constraints that downstream library consumers (lib/CLI cleave is real; see `feature-check`) can resolve through. **Triggers:** `nightly.yaml` now runs on cron (existing), `workflow_dispatch` (existing), and on push/PR when Cargo-touching files change. Catching cargo-affecting regressions at PR time avoids a 24-hour bisect window. Cron still catches drift that's not commit-correlated (registry updates, transitive resolution). `concurrency` group cancels in-flight nightly runs on PR pushes, matching `ci.yaml`. This direction was anticipated by the existing TODO in `ci.yaml`'s cargo-affected comment block: *"The full suite could fold into the existing nightly workflow, which already hosts slower integration-time checks."* ## Bugs caught (and fixed) by the new jobs on first run The new jobs surfaced three real gaps on their first run, all addressed in follow-up commits on this branch: - `minimal-versions` caught `color-print = "0.3"` resolving to `0.3.0` under minimum-version resolution, which doesn't expose `ceprintln` (added in `0.3.7`). Bumped the floor to `"0.3.7"`. - `release-target` caught a snapshot path-redaction gap: the `add_snapshot_path_prelude_filters` chain normalized `target/llvm-cov-target/` and `target/affected/build/` to `target/` but didn't handle `target/<triple>/` from cross-target builds. Added a vendor-anchored normalization (`/target/[a-z0-9_]+-(?:unknown|apple|pc|wasi)-[a-z0-9_-]+/` → `/target/`) alongside the existing two. - `release-target` also flagged the PTY snapshot redaction in `add_pty_binary_path_filters` was hardcoded to `target/(debug|release)/wt`. Broadened the regex to allow any path segments between `target/` and `(debug|release)/wt` — defensive in case shell-integration-tests is ever run on a cross-target. ## Test plan - [x] `actionlint` clean on both `ci.yaml` and `nightly.yaml` - [x] `feature-powerset` runs on PR (Cargo files changed) and passes - [x] `release-target` matrix builds and tests on all three triples - [x] `benchmarks` runs on this PR; `gist append` step skips on PR/push (cron-only) - [x] `check-unused-dependencies` and `minimal-versions` run successfully on nightly toolchain - [x] `nix-flake` runs (note: the `nix-flake` failure on this PR is pre-existing on `main` — same `test_current_or_recover_returns_repo_when_cwd_exists` panic in the latest main nightly run; not introduced here) - [x] All three required `test (linux/macos/windows)` checks pass - [x] `codecov/patch` passes - [x] Documentation in `.claude/skills/running-tend/SKILL.md` updated to reflect the moved call sites (cargo-install pin list and setup-nu locations) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
c2e38f6347 |
fix(config): clean up wt config show shell-integration section (#2574)
On a stock zsh-only macOS, `wt config show` listed `bash`, `fish`, and `nu` as `Skipped; ~/.foorc not found` — clutter for shells the user doesn't have installed. The block was the loudest thing in the SHELL INTEGRATION section for users who'd never touch those shells. After this PR, that fresh-user case shows just the warning + install hint with no per-shell clutter. Together with #2572 (which renamed the warning to "not configured" in this state), the section reads: ``` SHELL INTEGRATION ▲ Shell integration not configured ↳ To configure, run wt config shell install Invoked as: … $SHELL: /bin/zsh ``` ## What changed Adds `Shell::is_installed()` (PATH lookup with `WORKTRUNK_TEST_<SHELL>_INSTALLED` env override) and gates the `skipped.push` in `scan_shell_configs`. Shells with rc files still render normally — the filter only affects the "binary absent AND no rc file" case, which is exactly when the entry has no useful information. `nu` and `pwsh` are filtered cleanly because neither ships with macOS/Linux. `bash` and `zsh` on macOS still report (`/bin/bash` and `/bin/zsh` are preinstalled), but that's the honest answer. Side fixes: - Removed a redundant `Err("No shell config files found")` from `scan_shell_configs`. The install command's main.rs handler already covered this case. - Made the blank line between header status and the per-shell list conditional, so the trailing `↳ To configure…` hint stays attached to its subject when no shells render. - Replaced the standalone `is_nushell_available()` helper with `Shell::Nushell.is_installed()` — same semantics, one fewer function. The historical `WORKTRUNK_TEST_NUSHELL_ENV` env-var name is preserved to avoid churning hundreds of recorded snapshots. ## Tests - New `test_configure_shell_skipped_lists_only_installed_shells` pairs with `test_configure_shell_no_files` to cover both arms of the install guard (binary on PATH + rc absent → Skipped; binary absent + rc absent → drop entirely). - Test defaults moved into `STATIC_TEST_ENV_VARS` so PTY tests inherit them through `TestRepo::test_env_vars`. - 50+ snapshot files updated to reflect the merged behavior. Coverage caveat: the `which::which` fallback in `is_installed()` is not exercised by tests (every test sets the env override). Same shape as the prior `is_nushell_available()` helper, documented inline. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
ce66d9cfb9 | fix(tests): normalize cargo-affected target dir in PTY snapshots (#2565) | ||
|
Maximilian Roos
|
e5d0c138ea |
Add wt step {commit,squash} --dry-run, hide --show-prompt (#2557)
Adds `--dry-run` to `wt step commit` and `wt step squash`. It renders
the prompt, prints the shell invocation that would call the LLM, calls
the LLM and prints the generated message in three labeled sections
(PROMPT, COMMAND, MESSAGE), then exits without staging, running hooks,
or committing. For commit, `--stage` is honored against a temp index —
the previewed prompt matches what a real run would send the LLM, but the
user's real index is never touched. Output is routed through
`show_help_in_pager` so the prompt section (50+ lines) doesn't scroll
off; piping (`| grep`, `| jq`) still works because the helper
TTY-detects.
`--show-prompt` is hidden via clap (`hide = true`) but kept working —
it's still the right shape for piping the cheap rendered prompt to
another LLM (`wt step commit --show-prompt | llm -m gpt-5-nano`), and
`GitError::LlmCommandFailed` still suggests it as a reproduction
command.
A new "When to page output" section in the `writing-user-outputs` skill
documents the policy: page long human-oriented stdout (`--help`, `wt
config show`, `wt hook show`, `--dry-run`); don't page pipe-first data
or output already paged by a delegated tool (`git diff`).
Follow-ups during review: `render_llm_invocation` now uses the shell's
basename (no install-path leakage), with an insta filter normalizing
`bash.exe` → `sh` for cross-platform snapshot stability. Added a
`run_git_capture` helper around the new direct `Cmd::new("git")` calls
that bails on non-zero exit (was a non-blocking reviewer observation —
without it, a failing `git diff --staged` would silently feed an empty
diff to the LLM). `stage_to_temp_index` was refactored to take argv
directly, eliminating a dead `StageMode::None` arm. Added unit tests for
`render_llm_invocation` and integration tests for `--dry-run
--stage=tracked` and `--dry-run` without LLM configured.
`codecov/patch` reports 95.29% with 9 misses. The remaining gap is split
between error-path fault-injection (`stage_to_temp_index` git-add
failure, `show_help_in_pager` spawn failure) and a codecov attribution
mismatch — local `cargo llvm-cov` shows `render_llm_invocation` as
covered by the new unit tests, but codecov doesn't credit them. Merged
with explicit override.
Test plan:
- [x] `cargo test --test integration -- merge::test_step_commit_dry_run
merge::test_step_squash_dry_run merge::test_step_commit_show_prompt
merge::test_step_squash_show_prompt`
- [x] `cargo run -- hook pre-merge --yes` (3438 tests pass, clippy
clean)
- [x] End-to-end smoke test in `/tmp/wt-dry-test`: untracked file
appears in dry-run prompt, real index stays untouched
- [x] `--dry-run` and `--show-prompt` are mutually exclusive (clap
`conflicts_with`)
- [x] CI green on Linux/macOS/Windows after the cross-platform shell
filter fix
> _This was written by Claude Code on behalf of @max-sixty_
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
810cdc6636 |
feat(remove): add TTY progress spinner to --foreground (#2420)
## Summary Adds a stderr spinner (`⠼ Removing 7,272 files · 64.5 MiB`) to `wt remove --foreground`, driven by the same machinery #2413 introduced for `wt step copy-ignored`. Large worktrees (e.g. a fat `node_modules`) no longer hang with no feedback while the trash directory is unlinked. ## Approach Generalizes the spinner from copy-specific to verb-parameterized: - `src/copy_progress.rs` → `src/progress.rs`; `CopyProgress` → `Progress`; `start(verb)` takes `"Copying"` or `"Removing"`; method renamed `file_copied` → `record`. The TTY gate, 300ms startup delay, IEC-byte formatting, and Drop-based line clearing all carry over unchanged. A private `Progress::enabled(verb)` ctor is shared between `start()` (TTY-gated) and the test code that needs an enabled instance without a real terminal. - New `src/remove_dir.rs` — `remove_dir_with_progress(path, &Progress)` walks the tree iteratively, unlinks files on a dedicated 4-thread `REMOVE_POOL` (matching `copy::COPY_POOL`), then `rmdir`s deepest-first. Returns `(files, bytes)` so the post-op summary can match the spinner. Best-effort throughout — read/unlink/rmdir errors are silently skipped so the caller can always report the count of leaves we did manage to remove (the worktree is already pruned at this point, a stuck trash entry is recoverable). - `src/output/handlers.rs` — `cleanup_staged_with_progress` helper gates on `verbosity() >= 1`. Plumbed into both foreground branches (named + detached HEAD). `print_message` now takes `Option<(usize, u64)>`; background passes `None`. - The foreground success line gains a gray `(N files · X MiB)` suffix using `format_stats_paren`, matching the spinner's units (per the user-output skill's "stats parentheses" convention). `remove_worktree_with_cleanup`, the background path, and the picker flow are intentionally untouched. ## Snapshot stability The byte total walks the renamed worktree's `.git` pointer file, whose content is the gitdir's absolute path — so the byte count is sensitive to the temp-dir prefix (macOS `/var/folders/...` vs Linux `/tmp/...` vs Windows). An insta filter normalizes the byte value to `[BYTES]` inside the `(N files · X UNIT)` paren shape; the deterministic copy-ignored summary (`Copied N files · X B` — no surrounding parens) stays asserted unchanged. ## Verification - Unit tests in `progress` and `remove_dir` modules (including a permission-skip test for the read_dir-fails path) - Integration snapshots updated to include the new gray suffix - `cargo run -- hook pre-merge --yes` passes locally - Manual TTY smoke test under tmux: spinner renders, ticks, gets cleared before the summary; success line shows `(10,001 files · [BYTES] KiB)` for a 10k-file worktree - Manual pipe smoke test: no spinner frames, summary still shows the counts 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
e18c55943a |
test(pty): raise prompt-marker timeout to 30s for parallel-load headroom (#2376)
The 10s ceiling in `prompted_pty_interaction` tripped ~10 `approval_pty::*` tests under full-suite nextest parallelism (3325 tests). The tests pass instantly in isolation — they were racing the scheduler, not broken. 30s matches the `tests/CLAUDE.md` "long timeouts with fast polling" convention used by the `wait_for_file` helpers. Polling stays at 10ms, so isolated runs are unchanged; only the pathological slow case waits longer before panicking. Verified with `cargo run -- hook pre-merge --yes` — 3325 tests pass. > _This was written by Claude Code on behalf of Maximilian Roos_ Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
5daf70db6b |
feat(config): add WORKTRUNK_PROJECT_CONFIG_PATH override (#2267)
Adds a \`WORKTRUNK_PROJECT_CONFIG_PATH\` environment variable that overrides the project config path, mirroring the existing \`WORKTRUNK_CONFIG_PATH\` (user) and \`WORKTRUNK_SYSTEM_CONFIG_PATH\` (system) overrides. Missing files at the overridden path resolve to no project config — same as a missing \`.config/wt.toml\` today. Uses it to fully isolate completion tests in \`tests/integration_tests/shell_wrapper.rs\`, which previously only isolated user and system config. After #2266 made aliases appear as top-level completion candidates at \`wt <Tab>\`, any \`[aliases]\` added to this repo's own \`.config/wt.toml\` would silently pollute completion snapshots. The helper \`set_empty_user_config\` is renamed \`set_empty_configs\` and now points all three env vars at \`/dev/null\`. Also cleans up \`wt config show --format=json\` to use \`project_config_path()\` for the \`project.path\` field (was hardcoded \`.config/wt.toml\`), so the override is reflected in both path and config. \`configure_cli_command\` intentionally leaves \`WORKTRUNK_PROJECT_CONFIG_PATH\` unset — the \`WORKTRUNK_*\` env_remove loop prevents host leakage, and setting it would break tests that rely on the default \`.config/wt.toml\` lookup in their own test repo. Completion tests opt in explicitly via \`set_empty_configs\`. ## Test plan - [x] \`cargo run --quiet --bin wt -- hook pre-merge --yes\` — 3222/3222 green - [x] New integration test \`test_project_config_path_env_var_override\` covers override-to-different-file and override-to-missing-file Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
f126d7493c |
fix(test): disable placeholder reveal delay in progressive tests (#2202)
## Problem
`test_list_progressive_rendering` started failing on main in the
[code-coverage job of run
24358287156](https://github.com/max-sixty/worktrunk/actions/runs/24358287156)
with:
```
called `Result::unwrap()` on an `Err` value: "Progressive filling verification failed: no placeholder dots observed in any snapshot. ... Dots progression: [0, 0, 0, ...]"
```
Commit
[
|
||
|
Maximilian Roos
|
7afa2533e3 |
Collapse Status loading/timeout placeholders to · (#2177)
The Status column's loading glyph `⋯` (horizontal ellipsis) was too visually prominent in the tight per-position slots. Loading and the post-deadline timeout state don't really need to be distinguished via glyph — the surrounding context (progressive fill in `wt list`, picker appearing after deadline in `wt switch`) already tells the user which is which — so both states now render as a dim `·`. The change is centralized in a new `PLACEHOLDER` constant in `src/commands/list/render.rs`. Its TODO documents the future re-split: we'd like a subtle second glyph once we can evaluate candidates side-by-side in real tables. `render_list_item_stale` is kept as a separate entry point (passing the same `PLACEHOLDER` today) so the picker-side call site doesn't need re-auditing when the re-split lands. Most of the diff is mechanical: doc-comment references to the `⋯` glyph, test helpers counting `·` instead of `⋯`, regenerated insta snapshots, and the user-facing help table in `src/cli/mod.rs` collapsing two rows into one. > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
60f571d279 |
Remove global backslash snapshot filter + fix nushell multi-line exec (#2134)
The test snapshot framework had a global `\\` → `/` filter (`tests/common/mod.rs`) intended to normalize Windows paths before path-specific filters ran. It silently corrupted intentional backslashes in output: - **JSON-encoded ANSI escapes** (`\u001b` → `/u001b`) in `wt list --format=json` and diff snapshots. The snapshots had been capturing corrupted JSON for a long time. - **Shell line continuations** (`command \` → `command /`) in `wt config shell init` snapshots, masking template regressions. Worktrunk emits forward-slash paths via `path_slash` everywhere, so the blanket normalization wasn't carrying its weight. Dropped it; a follow-up comment points future contributors at `add_repo_and_worktree_path_filters` if any test ever produces a raw Windows path. 14 JSON/diff snapshots and 3 init snapshots regenerated with the real output (visible in the diff: `/u001b` → `\u001b`, `cmd /` → `cmd \`). While here: nushell wrapper executed the exec directive file line-by-line (`^sh -c $directive` in a loop), so multi-line `--execute` payloads ran as separate shell sessions — variables didn't persist, `cd` didn't affect later lines, etc. Switched to a single `^sh -c $script` invocation matching bash/zsh/fish `source` semantics. > _This was written by Claude Code on behalf of Maximilian_ --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Maximilian Roos
|
b174658e29 |
Split directive file into CD (raw path) and EXEC (shell) files (#2118)
The shell wrapper previously used a single `WORKTRUNK_DIRECTIVE_FILE` where wt wrote shell commands (`cd '/path'`, arbitrary `--execute` payloads). This meant the cd path went through shell parsing — any content wt wrote was sourced as shell. This splits the protocol into two files with different trust levels: - **`WORKTRUNK_DIRECTIVE_CD_FILE`** — raw path, read with `cd -- "$(< file)"`. No shell parsing, no escaping, no injection surface. Safe to pass through to alias/hook child processes. - **`WORKTRUNK_DIRECTIVE_EXEC_FILE`** — arbitrary shell (from `--execute`), sourced by the wrapper. Scrubbed from alias/hook child environments so hook bodies cannot inject shell into the parent session. When a nested `wt` inside an alias body tries `--execute` without the EXEC file, the command is dropped with a warning linking to #2101 for user feedback. The old `WORKTRUNK_DIRECTIVE_FILE` is silently honored for one release (users who upgrade wt without restarting their shell). Bash, zsh, fish, and PowerShell self-update on restart; nushell requires `wt config shell install`. Closes #2101 > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
474083a11d |
perf(ci): speed up CI with D: drive, skip redundant pre-commit, CARGO_INCREMENTAL=0 (#2010)
The CI critical path (test windows) took ~12 minutes. Profiling showed test execution was 75% of the time, bottlenecked by slow C: drive I/O on GitHub Actions Windows runners. **Changes to ci.yaml:** - Move TEMP/TMP to the D: drive on Windows — tests create hundreds of temp git repos via `tempfile`; the D: drive is local SSD with [up to 30x faster I/O](https://github.com/actions/runner-images/issues/8755) - Skip pre-commit in test jobs — the lint job already covers it. Run only `wt hook pre-merge --yes insta doctest doc` - Remove the now-unused uv, pre-commit, and lychee install steps from test jobs - Set `CARGO_INCREMENTAL=0` and `RUSTFLAGS=-C debuginfo=0` globally — CI does clean builds so incremental tracking is pure overhead, and debuginfo isn't needed **Cache consistency** (`CARGO_INCREMENTAL` and `RUSTFLAGS` across all workflows): `Swatinem/rust-cache` hashes `CARGO*` and `RUST*` env vars into the cache key. All workflows sharing a cache must set the same values. Added both env vars to `nightly-benchmark.yaml` and `claude-setup/action.yaml` too. Documented the principle in `.github/CLAUDE.md`. **Snapshot test fix** (`tests/common/mod.rs`): added `add_os_temp_dir_filter()` to redact `tempfile::tempdir()` paths under non-standard TEMP locations. The existing `add_project_id_filters` had hardcoded patterns for standard OS temp dirs; the D: drive TEMP needed a runtime filter. Handles macOS `/private` prefix and quoted paths. **Other:** bump `wt` to 0.35.2 (multi-filter hook syntax). **Results** (baseline → final): | Job | Before | After | Saved | |-----|--------|-------|-------| | test (linux) | 4m42s | 3m11s | 1m31s (32%) | | test (macos) | 8m35s | 5m51s | 2m44s (32%) | | test (windows) | 11m48s | 8m44s | 3m04s (26%) | > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
c84c87d47b |
refactor: remove lifetime guard field from TestRepo (#2000)
The `_lifetime_guard: Option<Box<dyn Any>>` field and `set_lifetime_guard()` method existed to hold insta snapshot filter guards alive via type erasure — insta is a dev-dependency and can't appear in `src/` code. Replace with `std::mem::forget(guard)` at the two call sites (the `repo()` rstest fixture and `test_list_empty_repo`). The insta settings are thread-local, so leaking the scope guard keeps them active without needing to store anything in TestRepo. > _This was written by Claude Code on behalf of @max-sixty_ Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
c03aa3aaef |
refactor: consolidate TestRepo into single src/testing module (#1991)
Two `TestRepo` structs existed — a lightweight one in `src/testing.rs` (74 lines, for unit tests) and a full-featured one in `tests/common/mod.rs` (~1500 lines, for integration tests). This consolidates them into one struct in `src/testing/` that serves both. The key blocker was the integration TestRepo's `_snapshot_guard: insta::internals::SettingsBindDropGuard` field — `insta` is a dev-dependency and can't appear in `pub` code in `src/`. Replaced with `Option<Box<dyn Any>>` (type-erased). Snapshot guard setup moved to the `repo()` rstest fixture. **What moved to `src/testing/`:** - `TestRepo` struct with all ~80 methods - `BareRepoTest`, `TestRepoBase` trait - `mock_commands` module - Helper functions (env isolation, fixture copying, CLI command builders, wait utilities) - Constants (`TEST_EPOCH`, timing constants, etc.) **What stayed in `tests/common/`:** - rstest fixtures (`repo()`, `repo_with_*`, etc.) - Snapshot settings functions (all `insta::Settings` code) - PTY functions (`portable_pty` code) - Re-exports: `pub use worktrunk::testing::*` **Constructor semantics:** - `TestRepo::new()` — lightweight `git init` + identity (backward-compatible with unit tests) - `TestRepo::standard()` — fixture copy with remote + worktrees + mock gh (what integration `new()` was) - `TestRepo::with_initial_commit()` — lightweight + one commit - `TestRepo::empty()` — `git init` with no commits > _This was written by Claude Code on behalf of maximilian_ --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
1ad18073db |
refactor: consolidate duplicated TestRepo across unit tests (#1944)
Four identical `TestRepo` structs existed across `src/config/` (in `test.rs`, `user/tests.rs`, `expansion.rs`, and `mod.rs`), and `configure_test_identity()` was duplicated between `src/git/recover.rs` and `src/commands/picker/summary.rs`. This extracts a shared `src/testutil.rs` module (gated with `#[cfg(test)]`) that the library-side tests import. The new `TestRepo::new()` uses `git init -b main` for determinism (the old copies used bare `git init` which depends on system config). A standalone `set_test_identity()` function handles the cases where tests manage their own temp directories and repo paths. Binary-side tests (`src/commands/`) can't access `#[cfg(test)]` library modules, so they keep their local helpers — this is a Rust crate boundary limitation, not oversight. Also adds cross-referencing doc comments to `wt_perf::isolate_cmd()` and `tests/common::configure_cli_command()` explaining why they're intentionally separate (benchmarks need minimal isolation for realistic timing; tests need full determinism for snapshots). > _This was written by Claude Code on behalf of @max-sixty_ Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
5cb9f954d4 |
fix: resolve analyze_trace test timeouts under nextest (#1933)
The 4 `analyze_trace` tests called `cargo build -p wt-perf` at runtime, which blocks on the Cargo lock when nextest runs tests in parallel — causing 60-second timeouts locally. Replace with compile-time binary path resolution via a shared `workspace_bin()` helper (in `tests/common/mod.rs`) that derives the path from `current_exe()`. Consolidate `mock_stub_binary()` to use the same helper. Add `wt-perf` to `default-members` with a dummy integration test (matching the existing `mock-stub` pattern) so `cargo test` places the binary in `target/debug/`. > _This was written by Claude Code on behalf of @max-sixty_ Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
a2fb7a7682 |
fix: redact PWD in snapshot env blocks (#1928)
Three merge test snapshots had a bare `PWD` env var leaking the machine-specific temp path. Added `PWD` to `add_standard_env_redactions()` alongside the existing `PATH`, `HOME`, etc. redactions. > _This was written by Claude Code on behalf of @max-sixty_ Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Axel H.
|
4ef64c47ad |
feat(opencode): add OpenCode integration (activity tracking, plugin, config) (#1807)
This PR adds OpenCode integration: activity tracking markers in `wt list`, plugin installation via CLI, `wt config show` diagnostics, and LLM commit generation detection. Continues the work started in #1295 and #1533 which added OpenCode to the docs and example config. ## What's included - **Activity tracking plugin** (`dev/opencode-plugin.ts`): maps `session.status`/`session.idle`/`session.deleted` to branch markers, same pattern as Claude Code's plugin - **`wt config plugins opencode install/uninstall`**: installs the plugin to `~/.config/opencode/plugins/worktrunk.ts` — source embedded via `include_str!()`, no npm needed. Sits under `wt config plugins` alongside Claude Code's `wt config plugins claude`. - **`wt config show` OPENCODE section**: shows plugin install status with actionable hints (only when `opencode` is on PATH) - **`LlmTool::OpenCode` variant**: detected via PATH for commit generation auto-config ## Docs approach Kept deliberately low-profile — no dedicated docs page, no README mention. OpenCode is discoverable via `wt config show` and a mention in tips-patterns. If it becomes popular, docs prominence can increase. > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Maximilian Roos <m@maxroos.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
bd87f2a146 |
fix: background removal blocks for 1s due to shell parsing bug (#1858)
`spawn_detached_unix` constructed the shell command as `sh -c "sleep 1
&& rmdir ...; rm -rf ... &"`. In POSIX shell, `;` has lower precedence
than `&`, so this parses as two statements: `sleep 1 && rmdir ...` runs
**synchronously** (1 second block), then only `rm -rf ... &` is
backgrounded. Every `wt remove` paid a 1-second penalty.
The fix wraps compound commands in braces — `{ sleep 1 && rmdir ...; rm
-rf ...; } &` — so `&` backgrounds the entire group. This affects all
callers of `spawn_detached` (remove, prune, merge, hooks).
Tests that asserted `!path.exists()` after removal now use
`assert_worktree_removed()` which accepts an empty placeholder directory
(the placeholder is cleaned up by the now-correctly-backgrounded `sleep
1 && rmdir`). Also fixes a pre-existing race in
`test_standalone_hook_post_merge` / `post_create` where background hooks
were checked immediately instead of polled.
Also adds `benches/remove.rs` for measuring end-to-end remove
performance.
> _This was written by Claude Code on behalf of maximilian_
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
0afc50d677 |
feat: add wt config plugins claude install/uninstall (#1830)
Adds a `wt config plugins` subcommand group for managing AI tool plugins, starting with Claude Code: - `wt config plugins claude install [--yes]` — runs `claude plugin marketplace add` + `claude plugin install` - `wt config plugins claude uninstall [--yes]` — runs `claude plugin uninstall` - Handles already-installed, not-installed, and CLI-not-found cases gracefully - Updates `wt config show` hint to suggest `wt config plugins claude install` instead of raw claude CLI commands - The `plugins` namespace is extensible for future tool integrations > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
worktrunk-bot
|
3ae83fcf8e |
fix: bypass config for picker timeout in tests (macOS CI) (#1815)
## Problem Two picker snapshot tests fail intermittently on macOS CI ([failed run](https://github.com/max-sixty/worktrunk/actions/runs/23760785525)): - `test_switch_picker_preview_panel_log` - `test_switch_picker_with_multiple_worktrees` Both show `·` stale placeholders in data columns instead of actual values. The triggering commit (`bacca9fb` — docs-only dead code removal) confirms this is a pre-existing timing flake, not a regression. The previous fix (#1694) disabled the picker's 500ms collect timeout via `[switch-picker] timeout-ms = 0` in the test config file. However, the PTY subprocess occasionally doesn't pick up the config on macOS CI, reverting to the 500ms default — too short for slow runners. ## Solution Add `WORKTRUNK_TEST_PICKER_NO_TIMEOUT` env var that the picker checks directly, bypassing config file loading entirely. Set it in `test_env_vars()` so all PTY-based picker tests benefit automatically. This follows the same pattern as `WORKTRUNK_TEST_SKIP_EXPENSIVE_THRESHOLD` and `WORKTRUNK_TEST_EPOCH`. The existing config-based `disable_picker_timeout()` helper is retained as a belt-and-suspenders approach. ## Testing - All 14 `switch_picker` tests pass locally - `pre-commit run --all-files` passes (clippy, fmt, etc.) --- Automated fix for [failed run](https://github.com/max-sixty/worktrunk/actions/runs/23760785525) --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Maximilian Roos
|
c5c446539e |
Simplify code by extracting helpers and normalizing config (#1813)
Refactoring branch with 13 commits that decompose large functions into smaller, focused helpers across the codebase. Net -104 lines. Key changes: - **Config system**: Extract `merged_project_config()` for repetitive project-override lookups; move deprecated config normalization (`[commit-generation]` → `[commit.generation]`, `[select]` → `[switch.picker]`) from lazy per-access to eager at load time; extract persistence helpers (`update_bool_flag`, `sync_string_field`, `sync_serialized_section`) - **Output handlers**: Split `handle_switch_output` and `handle_removed_worktree_output` into phase-based helpers; extract `BranchDeletionDisplay` struct and `print_retained_unmerged_branch`; parameterize `FlagNote::after()` by color instead of duplicating methods - **main.rs**: Decompose `main()` into `parse_cli`, `init_logging`, `dispatch_command`, `handle_command_failure`, etc. - **shell_exec**: Extract `ExternalCommandLog`, `apply_common_settings`, shared builder logic between `run()` and `stream()` - **remote_ref**: Extract `run_cli_api()` and `CliApiRequest` shared between GitHub and GitLab modules - **Tests**: Extract snapshot filter groups into semantic helpers; consolidate shell wrapper test setup The one behavioral change is in config loading: deprecated config sections are now normalized eagerly on load rather than checked lazily on every accessor call. This simplifies accessor methods and is functionally equivalent. > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Maximilian Roos <maximilian@Maximilians-MacBook-Pro.local> Co-authored-by: Claude <noreply@anthropic.com> |