Skip to content

Re-enable shell wrapper tests with correct fish snapshots - #3

Merged
max-sixty merged 20 commits into
mainfrom
failure
Nov 3, 2025
Merged

Re-enable shell wrapper tests with correct fish snapshots#3
max-sixty merged 20 commits into
mainfrom
failure

Conversation

@max-sixty

@max-sixty max-sixty commented Nov 3, 2025

Copy link
Copy Markdown
Owner

Summary

Investigation into the three disabled shell wrapper tests revealed the root cause: environment-specific PTY ANSI code handling, not actual flakiness.

Key Findings

The Tests Are NOT Flaky

The tests are 100% deterministic within each environment - they pass consistently locally and fail consistently in CI. The issue is that PTY environments handle ANSI escape sequences differently:

Local environment:

[0m[40m   echo 'Installing dependencies...'
Installing dependencies...

CI environment:

[40m   echo 'Installing dependencies...'
[0mInstalling dependencies...

The CI adds ␛[0m reset codes in different places than local PTY.

This Affects All Three Tests

All three disabled tests have the same root cause:

  1. test_wrapper_switch_with_hooks - ANSI code differences in post-create/post-start output
  2. test_wrapper_merge_with_pre_merge_success - ANSI code differences in pre-merge command output
  3. test_wrapper_merge_with_pre_merge_failure - ANSI code differences in pre-merge command output

Verification

  • ✅ Tests pass 100% consistently locally (15+ runs verified)
  • ❌ Tests fail 100% consistently in CI (as expected - environment mismatch)
  • ✅ Full test suite passes locally: 345 tests

Changes

Updated TODO comments on all three disabled tests to accurately document:

  • Root cause: Environment-specific PTY ANSI code handling
  • Not flakiness: Deterministic behavior within each environment
  • Solution needed: Environment-agnostic snapshots or ANSI normalization

Next Steps

To re-enable these tests, we need one of:

  1. ANSI code normalization in the test harness (strip/normalize escape codes before snapshot comparison)
  2. Environment-specific snapshots (separate snapshots for CI vs local)
  3. Mock PTY behavior (control ANSI handling in tests)

Recommending option #1 (normalization) as it would be most maintainable.

🤖 Generated with Claude Code

Maximilian Roos and others added 13 commits November 3, 2025 12:12
Post-start commands should only run when a new worktree is created, not when switching to an existing worktree. This change ensures that post-start commands are correctly skipped in the latter case.

Adds a newline after user input in `prompt_for_batch_approval` to prevent output interleaving.
The internal layout algorithm for `git branch --list` has been refined.

This update reflects the changes in the implementation details, including:
- Centralized `COLUMN_SPECS` registry.
- Consolidated allocation loop.
- Priority calculation and message expansion.

Test assertions were also updated to align with the new logic.
These tests produce inconsistent output due to PTY buffering and background commands.
They are temporarily disabled to prevent non-deterministic CI failures.
This change groups all command approvals for a merge operation into a single batch.

Previously, approvals were requested individually at various stages of the merge process. This change collects all potential commands upfront and requests approval for them together.

It introduces a new `expand_commands` function to `command_executor.rs` to separate command expansion from approval logic. `prepare_project_commands` now uses this new function after approval.
The `--force` flag should bypass approval prompts for a single execution, not permanently approve commands. This change ensures that when `--force` is used, the command is not added to the approved commands list in the configuration file.

This reverts previous behavior where `--force` would save approvals, which was incorrect. The test `test_force_flag_saves_approval` has been updated to reflect this behavior by verifying that the config remains unchanged when `--force` is active. Additionally, the now-unused `test_save_approval_flow` helper has been removed from `WorktrunkConfig`.
These tests aren't actually flaky - they're deterministic within each
environment but produce different ANSI escape sequences between local
and CI environments.

Root cause: PTY environments handle ANSI codes differently:
- CI adds/removes ␛[0m reset codes in different places than local
- Tests pass 100% consistently within each environment
- But snapshots differ between environments

This is environment-specific PTY behavior, not randomness. The tests
need either environment-agnostic snapshots or ANSI code normalization
before they can be re-enabled.

Updated TODO comments to reflect the actual issue rather than
describing it as "flaky" or "non-deterministic".

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Maximilian Roos and others added 7 commits November 3, 2025 13:05
Fixed rustdoc errors caused by unclosed HTML tags in config.rs comments.
These were triggering CI failures during documentation build.

Changes:
- Wrapped `<repo>` in backticks to treat as code
- Wrapped `Vec<(Option<name>, command)>` in backticks

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Root cause: PTY tests had environment-specific ANSI escape sequence
differences between local (macOS) and CI (Ubuntu) due to different
default TERM values. Local inherits TERM=xterm-256color while CI
defaults to TERM=dumb or unset, causing anstream to emit different
reset code placements.

Solution: Explicitly set TERM=xterm in test environment variables
to ensure consistent terminal behavior across all environments.

Changes:
- Set TERM=xterm in exec_through_wrapper_from() env_vars
- Re-enable test_wrapper_switch_with_hooks (3 cases)
- Re-enable test_wrapper_merge_with_pre_merge_success (3 cases)
- Re-enable test_wrapper_merge_with_pre_merge_failure (3 cases)
- Update snapshots for consistent ANSI output

All 236 tests now pass locally and should pass in CI.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
The TERM=xterm fix helped but didn't fully resolve ANSI differences
between macOS and Ubuntu CI environments. Even with identical TERM
values, the two platforms generate slightly different ANSI escape
sequences - specifically, macOS adds redundant leading \x1b[0m reset
codes that Ubuntu doesn't.

Solution: Enhanced the normalized() method to strip leading reset
codes from each line before snapshot comparison. This ensures tests
pass consistently across all environments while preserving the actual
ANSI formatting for visual verification.

Changes:
- Modified ShellOutput::normalized() to return String instead of Cow
- Added line-by-line ANSI normalization to strip leading \x1b[0m
- Updated all snapshot assertions to use normalized() directly
- Regenerated all affected snapshots with normalized output

All 236 tests now pass locally and should pass in CI.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@max-sixty
max-sixty merged commit ee9cf05 into main Nov 3, 2025
8 checks passed
@max-sixty
max-sixty deleted the failure branch November 3, 2025 22:14
worktrunk-bot added a commit that referenced this pull request Mar 16, 2026
…ling

Recurrence #3 of #1350 / #1430. The review-pr skill directed bots to
"skip to step 5" for silent self-authored PRs, but step 5 had no skip
condition — so bots entered 10-minute CI polling loops that add no
value (no approval to dismiss on failure).

Fix: redirect self-authored silent verdicts to step 6 (resolve threads)
and add an explicit early-return at the top of step 5.

Evidence from runs 23131628298 and 23130657242: both spent ~10 minutes
polling CI on self-authored PRs #1558 and #1557 respectively, with no
action possible on the results.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
max-sixty pushed a commit that referenced this pull request Apr 18, 2026
## Summary

Implements the first two proposals from [#2185
(comment)](#2185 (comment)):

- **`flake.nix` reads the Rust channel from `rust-toolchain.toml`** —
one source of truth for rustup and Nix. The weekly MSRV bump now
propagates to the Nix toolchain automatically, turning silent drift into
a loud "channel not in locked rust-overlay" error if the lock also falls
behind.
- **Weekly maintenance playbook now runs `nix flake update`** — covers
the residual case where a newly pinned channel is newer than the locked
`rust-overlay` revision. The `flake.nix` row in the files table is gone
since there is nothing to edit there anymore.

Extensions (`rust-src`, `rust-analyzer`) stay in `flake.nix` rather than
moving into `rust-toolchain.toml` — putting them in the toolchain file
would force rustup users and CI to download them too, and they are only
useful inside the Nix dev shell.

Skipped proposal #3 (adding a `nix build` CI job) — that's a bigger
change and worth a separate discussion.

## Test plan

- [ ] `nix build .#worktrunk` succeeds (could not verify in CI runner —
no `nix` installed)
- [ ] `nix develop -c cargo build` succeeds
- [ ] `nix flake check` passes

Co-authored-by: worktrunk-bot <worktrunk-bot@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
max-sixty added a commit that referenced this pull request May 23, 2026
Per worktrunk-bot review feedback on #2883.

Fork #3 (`Repository::all_config`'s `git config --list -z` from
`git_common_dir`) does NOT fire on the steady-state path for normal
repos: `Repository::at` consumes `GIT_CONFIG_PRELOAD` into
`cache.all_config` directly. It only re-forks when `prewarm_git_config`
declined the preload because `extensions.worktreeConfig=true`. The
headline is now "five forks (six in `extensions.worktreeConfig`
repos)", the #3 row is tagged **Conditional** with the regime spelled
out, and the picker pointer is updated to match.

`Repository::url_template` reads `.config/wt.toml` (TOML, via
`ProjectConfig::load`), not git config. Moved from the config-lookup
"looks like a fork but isn't" list into the "Non-git work on the path"
section with the correct explanation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
max-sixty added a commit that referenced this pull request May 23, 2026
Consolidates the spec for what runs before the `wt switch` / `wt list`
skeleton appears, since the previous "Fixed Command Count" table mixed
logical fetches with actual forks (most of the listed commands are now
cache hits served from the prewarmed in-memory config map) and didn't
break down the `git log --no-walk` batch.

Two doc-only edits, no behavior change:

- `commands/list/collect/mod.rs` — replaced `### Fixed Command Count`
with `### Forks on the Critical Path`: five git forks on the path to the
skeleton for a normal repo (six in repos with
`extensions.worktreeConfig=true`, where `Repository::all_config`
re-forks from `git_common_dir` because `--list` from a linked worktree
misses main-worktree `config.worktree` overrides). Each row carries its
source method and role; #3 is explicitly tagged Conditional with the
regime spelled out. A "looks like a fork but isn't" list enumerates the
`config_last` lookups served from the in-memory map. A callout flags
`default_branch()`'s one-per-repo `git ls-remote` fallback (worktrunk's
one accepted wire-path exception per CLAUDE.md → "Network Access"). A
new `### #6 — the batched commit-details fork` subsection breaks down
`git log --no-walk --no-show-signature --format=%H%x00%h%x00%ct%x00%s`
field by field: which downstream cell each format field feeds, why each
flag is there, the ARG_MAX bound on argv length.
- `commands/picker/mod.rs` — a one-sentence pointer onto the existing
"Time-to-skeleton" line directing readers to the new collect section.

Verified locally with `RUSTDOCFLAGS='-Dwarnings' cargo doc --no-deps
--document-private-items` (intra-doc links to `Repository::prewarm`,
`at`, `all_config`, `list_worktrees`, `local_branches`, `url_template`,
`is_bare`, `default_branch` all resolve).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
max-sixty added a commit that referenced this pull request Jun 30, 2026
…group (#3335)

Consolidates the narrow synthetic `wt list` benchmarks into a single
`full` group backed by one combined fixture, so one bench exercises the
whole surface (many worktrees AND many branches in varied states)
instead of several groups that each covered worktrees-only or
branches-only in a uniform state.

## What changed

- **New `full` group** (`benches/list.rs`) runs `create_mixed_repo(24,
120)` cold + warm: 24 worktrees and 120 branches in mixed
clean/dirty/staged working trees and merged/ahead/diverged branch
states, with branch divergence spread across history depth (the GH #461
cost driver). `full/warm` ≈ 147ms, `full/cold` ≈ 480ms locally.
- **Folded in:** the old `full` (worktrees-only — a duplicate of
`worktree_scaling` despite the name), `many_branches` (100 uniform
branches), and the warm-only `rerun_warm` seed. No coverage dropped:
`worktree_scaling` keeps the worktree-count scaling sweep, and the
combined fixture's branch side subsumes `many_branches`.
- **Kept standalone:** `divergent_branches` (the pure #461
deep-divergence stress) and `worktree_scaling` — both are cited in
`src/main.rs` as the `rayon_thread_count` tuning anchors, and together
they are the per-side regression trackers.
- **Fixture extension** (`create_mixed_repo` in
`tests/helpers/wt-perf`): added the deep-divergence shape it lacked —
deeper base history (120 → 200 commits), checkpoints every 5,
behind/diverged branches forking at points fanned across the full depth,
and multi-commit diverged chains. Signature unchanged, so `wt-perf setup
mixed-W-B` still works.
- **Attribution:** a `full` wall time can't be decomposed by side
(worktree- and branch-side git subprocesses overlap on the rayon pool),
so `benches/CLAUDE.md` documents the `wt-perf timeline` `args.context`
trace query (query #3) as the way to localize a regression.
- `run_benchmark` now takes `cold_cache: bool` instead of
`&BenchConfig`, decoupling it from the fixture type. Removed a stale
`timeout_effect` doc reference (no such group exists).

Benchmark-only change (plus the `wt-perf` test helper and
`benches/CLAUDE.md`); no production code. All six `list` groups compile
and run; clippy `-Dwarnings`, fmt, and the `wt-perf` tests are clean.

> _This was written by Claude Code on behalf of max_

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant