lotabout.skim/AGENTS.md
LoricAndre 88ce5b97ac
test: replace tmux e2e harness with cross-platform Zellij harness (#1139)
* test: replace tmux e2e harness with cross-platform Zellij harness

Rewrite the end-to-end test harness to drive `sk` through Zellij instead of
tmux, keeping the same capabilities and public surface (ZellijController,
Keys, wait, sk, the sk_test! DSL and the line!/keys!/out! helpers) so the
existing tests port over with only import/type renames.

Zellij has no detached-server model like tmux, so the harness spawns a Zellij
client attached to an in-process pseudo-terminal via portable-pty (openpty on
Unix, ConPTY on Windows). Because Zellij 0.44+ and portable-pty are both
cross-platform, the harness — and the tests that only rely on it — are now
available on Windows too: the interactive tests (formerly unix.rs) are
un-gated. execute.rs, popup.rs and listen.rs stay unix-only for reasons
unrelated to the multiplexer (PermissionsExt, a mock sh/tmux binary, unix
sockets).

Key harness details:
- Session per test via `zellij attach --create` on a fixed 80x24 PTY.
- Keys injected as raw terminal bytes with `zellij action write`; screen read
  back with `zellij action dump-screen [--ansi]`, reversed to match the old
  bottom-anchored indexing.
- A generated config disables startup tips, pane frames, mouse mode and — the
  crucial bit — the kitty keyboard protocol, so injected legacy escape
  sequences (arrows, etc.) reach sk.
- All zellij CLI calls are run under a timeout and wait() has a wall-clock
  budget, so a wedged server surfaces as a fast retryable error instead of
  hanging a test.

popup.rs unsets $ZELLIJ and sets $TMUX so skim selects its tmux popup backend
(the mock) rather than the zellij one while running inside a Zellij pane.

Because each test spins up a full Zellij session, the e2e binaries are put in
a serialized nextest test-group; CI installs Zellij (all three OSes) in place
of tmux, and the obsolete tmux setup-scripts are removed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* ci: fix rustfmt and stop Windows from cancelling the other nextest legs

- Run `cargo +nightly fmt` on the new Zellij harness (rustfmt CI was red).
- Set `fail-fast: false` on the nextest matrix so a failing OS leg no longer
  cancels the others, giving a clear pass/fail signal per platform.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* ci: install zellij via winget on Windows

taiki-e/install-action has no prebuilt Zellij binary for Windows and falls
back to `cargo install zellij`, which fails building openssl-sys from source
on the runner. Install via winget on Windows instead (taiki-e still handles
Linux/macOS), and expose winget's shim dir on PATH for the test step.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test/ci: address review feedback on the Zellij harness

- Pin the Windows winget Zellij install to 0.44.3 to match the Linux/macOS
  runners (reproducible CI).
- Drop the unused `&locale` YAML anchor (actionlint flagged it).
- `wait` now surfaces the last predicate error on timeout instead of a
  generic one, so a persistent failure keeps its diagnostic cause.
- `output_with_timeout` tears down the child and reader threads on a
  `try_wait` error instead of leaking them.
- Add rustdoc to the public harness surface (`sk`, `wait`, `Keys`,
  `ZellijController` and its methods).

Deliberately not changed: a non-zero `zellij` exit is still not treated as an
error (some `zellij action` calls exit non-zero in transient states — e.g.
inline `sk` viewport teardown — while returning usable output; propagating it
broke `inline_clear_on_exit`), and `to_lines` keeps trimming to preserve the
tmux-parity bottom-anchored indexing the ported tests rely on.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* ci: put the real zellij.exe dir on PATH for the Windows test step

The winget install succeeds, but its Links shim wasn't reliably visible to the
`cargo nextest` step's processes, so `which("zellij")` failed and every
interactive test panicked at setup. Locate the installed zellij.exe under the
WinGet Packages dir and add its directory to GITHUB_PATH instead, failing the
step loudly if it isn't found.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* ci: reload PATH from registry after the MSI zellij install on Windows

The winget Zellij package is an MSI installer that installs to Program Files
and updates the machine PATH in the registry, not a portable under
WinGet\Packages — so the previous "search Packages" lookup threw. Reload PATH
from the machine/user registry values (with a Program Files fallback), then
export zellij's directory via GITHUB_PATH for the test step.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test/ci: gate interactive e2e tests off Windows

Enabling the interactive tests on the Windows runner surfaced a real gap: the
PATH/install issues are fixed (winget install works), but under the Windows
runner's ConPTY the Zellij session never renders — dump-screen stays empty and
wait_ready times out with "pane not rendered yet" for every interactive test.
That's a harness-runtime gap on Windows (and sk's escape-code disambiguation on
Windows would be a further blocker), so gate interactive.rs `#![cfg(not(windows))]`
with a TODO, keeping the harness code cross-platform.

Since no Windows test now uses the harness, drop the winget Zellij install from
the Windows leg; Linux/macOS still install it via taiki-e. Adjust the docs that
claimed the e2e tests run on Windows.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): gate Zellij harness tests to Linux only

The Zellij-backed e2e harness renders reliably under the Linux CI runner,
but on the macOS and Windows runners the pane never comes up under their
PTY (`wait_ready` times out with "pane not rendered yet"). Restrict all
four e2e test files (interactive, execute, popup, listen) to
`#![cfg(target_os = "linux")]`, install Zellij only on the Linux runner,
and update the harness/agent/architecture docs to match.

The harness code stays cross-platform so macOS/Windows e2e can be
re-enabled once their runners render the session.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): make the Zellij harness render on macOS and Windows

The Zellij e2e harness previously only came up reliably on the Linux CI
runner; on macOS and Windows the pane never rendered and every e2e test
timed out with "pane not rendered yet". Root cause (surfaced by capturing
the Zellij client's PTY output): Zellij's client/server startup handshake
is racy — the client occasionally dies with "Received empty unknown from
server" and the session never renders. It's rare on Linux (flaky) but
frequent on the cold macOS/Windows runners.

Harden the harness so it renders everywhere instead of gating tests to
Linux:

- Detect a dead session fast (drain thread flags client PTY EOF) and
  respawn a fresh session, up to SESSION_SPAWN_ATTEMPTS times, instead of
  waiting out the whole render budget and failing.
- Resolve the pane's shell to an absolute `bash` path via `which`; the
  Zellij server's own environment may not have `bash` on PATH on the
  macOS/Windows runners, which would leave the pane with no shell to render.
- Nudge the client's terminal size until the server gives the pane a
  non-zero geometry to render into (the initial size can be dropped under
  ConPTY / a cold runner).
- Give the first render its own longer budget and, on timeout, surface a
  tail of the Zellij client output for diagnosing runners we can't
  reproduce locally.

Un-gate the tests accordingly: interactive.rs (pure harness) now runs on
Linux, macOS and Windows; execute.rs/popup.rs/listen.rs go back to
#![cfg(unix)] (Linux + macOS) — their Windows-incompatibility is POSIX
mock binaries / a unix socket, unrelated to the multiplexer. CI installs
Zellij on all three OSes (taiki-e on Linux/macOS, winget on Windows) and
the nextest job gets a 45-minute cap so a harness regression fails fast.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): fix macOS session-name rejection via short ZELLIJ_SOCKET_DIR

The macOS runner failed every Zellij e2e test at CLI-parse time:

  error: Invalid value "skim_e2e_..." for '--session <SESSION>':
         session name must be less than 0 characters

This is not the render race the previous commit addressed. Zellij places
each session's unix socket at `$ZELLIJ_SOCKET_DIR/<protocol>/<session>`,
and a unix socket path is length-capped by the OS (~104 bytes on macOS).
Zellij's default base is `$TMPDIR/zellij-<uid>`; on the macOS runners
`$TMPDIR` is a long `/var/folders/…` path that leaves ~0 bytes for the
session name, so Zellij rejects every name and the client exits before it
attaches (zellij-org/zellij#4211). Linux's short `/run`|`/tmp` base never
hits this, which is why it only failed on macOS.

- Export ZELLIJ_SOCKET_DIR=/tmp/skim-zj (a short base) on every zellij
  invocation — the attached client, `action`, and `run` — so they share a
  short socket path well under the cap on Linux and macOS alike.
- Shorten session names (`sk_<=10 chars_<6 rand>`): several were derived
  from long test names (e.g. execute_interactive_child_keeps_receiving_
  keys_fullscreen) and exceeded Zellij's ~36-char limit and ate socket
  budget; the random suffix still keeps them unique.

Also fix a stale doc command in AGENTS.md (`cargo nextest --tests` ->
`cargo nextest run --tests`), per PR review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): answer the DSR cursor-position probe so Windows renders

The Windows nextest leg hung on every interactive.rs e2e test:

  Error: pane not rendered within 60s. zellij client output tail:
  \u{1b}[6n

The captured client output was a single `ESC[6n` — a Device Status
Report requesting the cursor position. Under the Windows ConPTY the
Zellij client probes the terminal size by asking for the cursor position
and blocks until the terminal replies; on Unix the size comes from the
PTY ioctl, so the client never waits (which is why only Windows hung).

The harness owns the master PTY — it *is* the terminal — so the drain
thread now watches for `ESC[6n` and writes back a Cursor Position Report
(`ESC[24;80R`, reporting the 24x80 pane). This unblocks the client so the
pane renders. The reply is harmless on Linux/macOS (all 45 e2e tests
still pass there), keeping interactive.rs on all three platforms.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): address review nits in the Zellij harness

Follow-ups from PR review, none affecting the cross-platform fixes:

- zellij_socket_dir() now returns io::Result and propagates a
  create_dir_all failure through run()/action()/spawn_once() instead of
  swallowing it, so a socket-dir problem surfaces directly rather than as
  a confusing downstream Zellij error.
- Fix a latent typo in the (currently unused) assert_line!/line! macro:
  std::io::std::io::Error{,Kind} -> std::io::Error / std::io::ErrorKind,
  so the macro compiles if a test ever uses it.
- tempfile() returns an InvalidData error instead of panicking on a
  non-UTF-8 temp path.

Skipped the reviewer's suggestion to stop trimming captured output: the
trim is load-bearing. It drops Zellij's blank padding rows so capture()[0]
is the bottom content line that every test indexes against; stripping only
CR/LF would reintroduce ~20 empty rows and shift every index. No test
exercises intentionally-spaced items, so there is no real defect.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): silence unused_assignments warning in wait()

`last_err` was initialised to `None` and always overwritten before it
could be read, so the initial assignment was dead (unused_assignments
warning at the top of every test build). Return the current predicate
error directly on timeout instead of stashing it — same behaviour (the
most recent error is surfaced), no dead variable, no warning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): wrap assert_line! timeout error to 120 columns

Pure formatting: split the Err/Error::new/format! construction in the
(rustfmt-skipped) assert_line! macro body across lines to satisfy the
repo's 120-column limit. No behaviour change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): guard against [-0] in the negative-index DSL macro

@method_neg_dispatch used `lines.len() >= $idx`, which is always true for
$idx == 0, so `@capture[-0]` would index `lines[lines.len()]` and panic.
Require `$idx > 0` in both the predicate and diagnostic paths so a `[-0]`
index falls through to the graceful "not enough lines" / "<no line>"
handling instead. No current test uses negative indices; this only closes
the latent edge case. Per PR review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): use forward slashes for Windows paths in the bash command

With the DSR fix the Windows pane now renders and runs the command, which
surfaced the next issue: the harness drives a `bash` shell but embedded
native Windows paths (backslashes) into the command string. bash treats
`\` as an escape, so `.\target\release\sk.exe` collapsed to
`.targetreleasesk.exe` ("command not found") and the `C:\Users\...`
redirect/mv targets would mangle the same way.

Convert `\` to `/` for the `sk` binary and the outfile when building the
bash command in sk(); bash on Windows accepts `./target/release/sk.exe`
and `C:/Users/...`. On Unix the paths have no backslashes so it is a
no-op, and the stored outfile the test reads back keeps native separators.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

* test(e2e): reap the Zellij client child in Drop

ZellijController::drop killed the client with child.kill() but never
waited on it, so on Unix each dropped controller left a zombie until the
test binary exited — and many controllers are created per binary. Pair
the kill with child.wait() (matching output_with_timeout) so the process
is reaped immediately. Per PR review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wTqRsi31RYJXEM3ZZQhQU

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-07-23 16:32:29 +00:00

7.3 KiB

Skim Agent Guidelines

Build/Test/Lint Commands

  • Build: cargo build [--release]
  • Run: cargo run [--release]
  • Test (all): cargo nextest run
  • Test (single): cargo nextest test_name
  • Integration/E2E tests: cargo nextest run --tests (drives sk through Zellij under the hood; needs zellij >= 0.44 and bash on $PATH)
  • Memory leak detection: cargo nextest run --profile valgrind
  • Thread leak/race detection:
    1. Build: RUSTFLAGS="-Zsanitizer=thread" cargo +nightly build --tests -Zbuild-std --target x86_64-unknown-linux-gnu
    2. Run: TSAN_OPTIONS="detect_deadlocks=1" cargo +nightly nextest run --profile tsan --target x86_64-unknown-linux-gnu
  • Lint: cargo clippy
  • Format: cargo +nightly fmt (check only: cargo +nightly fmt --check)
  • Fuzz (requires nightly + cargo install cargo-fuzz): cargo +nightly fuzz run <target> — see fuzz/README.md for target list

Code Style

  • Format with 120 char line width (defined in .rustfmt.toml)
  • Use standard Rust naming conventions (snake_case for functions/variables, CamelCase for types)
  • Organize imports by standard library, external crates, then internal modules
  • Prefer Option/Result types for error handling over panicking
  • Use proper error propagation with ? operator
  • Document public API with rustdoc comments
  • Use meaningful type annotations, especially for public functions
  • Follow the existing structure for new modules (see src/engine/ or src/model/)
  • Implement relevant traits (SkimItem, etc.) for new types when needed

Architecture Documentation

  • ARCHITECTURE.md documents the full architecture: data flow, operating modes, subsystems, threading model, and public API.
  • Update ARCHITECTURE.md whenever you make structural changes, including:
    • Adding, removing, or renaming modules, structs, or traits
    • Changing the data flow between subsystems (reader → pool → matcher → TUI)
    • Adding new operating modes or modifying existing ones
    • Changing the threading model or synchronization primitives
    • Adding or removing public API surface (SkimItem, SkimOptions, SkimOutput, etc.)
    • Changing the event/action system or key binding infrastructure
  • Keep call-site line numbers in the cross-reference table up to date when the referenced functions move.

Testing

The end-to-end tests drive a real sk process through a terminal, using the Zellij-backed harness in tests/common/zellij.rs (ZellijController + the sk_test! DSL). It requires zellij (>= 0.44) and bash on $PATH. The harness is cross-platform (Linux, macOS and Windows). The pure-harness tests in interactive.rs run on all three platforms; execute.rs, popup.rs and listen.rs stay #![cfg(unix)] for reasons unrelated to the multiplexer (they install POSIX mock binaries / bind a unix socket), so they run on Linux and macOS. A few harness details make the non-Linux runners work: the pane's shell is resolved to an absolute bash path (the Zellij server's environment may lack bash on PATH); ZELLIJ_SOCKET_DIR is forced to a short path so the session's unix socket path stays under the OS cap (macOS's default $TMPDIR is too long); the drain thread answers the client's cursor-position report (ESC[6n), which the Windows ConPTY client blocks on to learn the terminal size; and wait_ready nudges the client's terminal size until the server gives the pane a non-zero geometry to render into. The harness drives Zellij with:

  • zellij attach --create <session> (spawned on an in-process PTY via portable-pty) to start a detached session; SKIM_DEFAULT_OPTIONS and friends are cleared on the spawned process.
  • zellij --session <session> action write <bytes...> to inject keystrokes.
  • zellij --session <session> action dump-screen [--ansi] to capture the pane.

When exploring manually you can reproduce the same flow with those commands; the config the harness writes disables startup tips, pane frames and the kitty keyboard protocol (so injected legacy escape sequences reach sk).

Insta Snapshot Tests

Most TUI behaviour is covered by insta snapshot tests in tests/. The infrastructure lives in tests/common/insta.rs and is exposed through two macros: snap! and insta_test!.

insta_test! — writing tests

Simple variant (single snapshot, no interaction):

insta_test!(my_test, ["item1", "item2"], &["--opt1", "opt2"]);
insta_test!(my_test, @cmd "printf 'a\nb'", &["--ansi"]);
insta_test!(my_test, @interactive, &["-i", "--cmd", "echo {q}"]);

DSL variant (multiple snapshots with interaction between them):

insta_test!(my_test, ["a", "b", "c"], &["--multi"], {
    @snap;                      // take a snapshot (cell text only)
    @snap_color;                // snapshot cell styling (fg/bg/modifier) instead
    @key Up;                    // send a named key (Enter, Down, Tab, …)
    @char 'f';                  // send a single character
    @type "foo";                // type a string
    @ctrl 'w';                  // Ctrl+key
    @alt 'b';                   // Alt+key
    @shift Tab;                 // Shift+key
    @action Last;               // send an Action variant (no args)
    @action Down(1);            // send an Action variant (with args)
    @snap;                      // take another snapshot
    @assert(|h| condition);     // boolean assertion (does not snapshot)
    @exited 0;                  // assert the app exited with this code
});

Snapshot file naming

Macro form File pattern Example
Simple variant {file}__{test}.snap options__opt_wrap.snap
DSL variant — Nth @snap {file}__{test}@{NNN}.snap options__opt_cycle@002.snap
DSL variant — Nth @snap_color {file}__{test}@color{NNN}.snap ansi__ansi_flag_enabled@color002.snap

DSL snapshots use a zero-padded three-digit suffix (@001, @002, …) so that cargo insta review presents them in the order they were taken.

Snapshot front-matter

Every snapshot includes a description field in its YAML front-matter. For a DSL test the description shows the input, options, and the DSL commands that ran since the previous @snap, making it easy to understand what state each screenshot captures:

description: "input: items [\"a\", \"b\", \"c\"]\noptions: --multi\nafter:\n  @key Up\n  @shift Tab"

The expression field is intentionally omitted (omit_expression = true) to keep the files free of internal implementation details.

Snapshot workflow

Generate / update snapshots:

# Generate all missing snapshots and accept them immediately:
INSTA_UPDATE=always cargo nextest run

# Generate missing snapshots as .snap.new files for manual review:
INSTA_UPDATE=new cargo nextest run
cargo insta review          # accept / reject interactively

When adding new tests that produce snapshots:

  1. Write the test with @snap markers.
  2. Run INSTA_UPDATE=always cargo nextest run --test <file> <test_name> to generate the initial snapshot files.
  3. Inspect the generated .snap files to verify the rendered output is correct.
  4. Commit both the test and its snapshot files.

When changing rendering logic that affects many existing snapshots:

  1. Delete the affected .snap files: find tests/snapshots -name "prefix__*.snap" -delete
  2. Regenerate: INSTA_UPDATE=always cargo nextest run
  3. Review the diff with git diff tests/snapshots/ before committing.