lotabout.skim/AGENTS.md
LoricAndre 026a622a9d
feat: add cargo-fuzz targets for hand-rolled text parsers (#1106)
* feat: add cargo-fuzz targets for hand-rolled text parsers

Skim's most panic-prone code is the hand-written byte/char-index
bookkeeping over untrusted input: ANSI stripping, --nth/--with-nth field
extraction, the fuzzy matching algorithms, the query->engine->match
pipeline, and the --bind key-map parser. Add five cargo-fuzz (libFuzzer)
targets covering each, asserting real invariants (char-boundary safety,
monotonic index mappings, match indices in bounds) rather than just
catching panics, plus a CI workflow that runs them on every push/PR
touching src/ or fuzz/ and a longer nightly session via cron.

* ci: wire fuzz targets into the existing test matrix

Replace the standalone fuzz.yml workflow with a `fuzz` job in the main
test.yml matrix, running each of the 5 fuzz targets for 60s (5 minutes
total per CI run) alongside nextest/clippy/msrv.

* ci: remove standalone fuzz workflow

Superseded by the fuzz job now in test.yml.

* ci: run fuzz job as a single job on the platform matrix

Reuse the existing linux/macos/windows matrix instead of a separate
per-target matrix; run all 5 fuzz targets sequentially in one step
(60s each, 5 minutes total). cargo-fuzz doesn't support Windows, so
the fuzzing step is skipped there while still installing the
toolchain for consistency with the rest of the matrix.

* ci: reuse existing yaml anchors in the fuzz job

Use *toolchain instead of a bespoke nightly-install step, matching
the coverage job's pattern of letting `cargo +nightly` auto-provision
the toolchain on demand.

* nix: add cargo-fuzz to the tests devShell

Makes cargo-fuzz available via `nix develop` alongside the other test
tooling, matching what CI installs for the fuzz job.

* ci: install cargo-fuzz via taiki-e/install-action

Matches how the other CI-only cargo subcommands (nextest, cargo-msrv,
cargo-llvm-cov) are installed, and is faster than compiling it from
source with cargo install.

* ci: force the native host target for cargo-fuzz

cargo-fuzz was picking a statically-linked musl target on the runner,
which fails since ASan can't link against a static libc. Pass the
actual host triple (from `rustc -vV`) explicitly so the sanitizer
build always targets the dynamically-linked gnu/darwin toolchain.

* ci: skip the whole fuzz job on windows

Rather than skipping just the fuzzing step, exclude the job entirely
for the windows-latest matrix entry via a job-level `if`, since
cargo-fuzz/libFuzzer has no Windows support.

* ci: gate the fuzz job with runner.os instead of matrix.os

Matches the runner.os-based conditionals already used elsewhere in
this workflow (the linux/macos dependency install steps) rather than
comparing matrix.os directly.

* ci: enable the fuzz job on windows without ASan

cargo-fuzz does support Windows, but AddressSanitizer on the MSVC
target needs the separate "C++ AddressSanitizer" VS component plus a
PATH tweak for its DLL, which this runner doesn't have configured.
Rather than skip the job, disable the sanitizer on Windows only
(--sanitizer none) and keep coverage-guided fuzzing there; our
targets assert via plain Rust panics so they don't depend on ASan.

* test: assert exact char_idx correctness in ansi_strip fuzz target

Replace the bounds-only char_idx check with an exact-equality check
against the char position of byte_pos in the original string. This
subsumes (and is stronger than) the monotonicity CodeRabbit flagged,
since strictly-increasing byte positions on char boundaries always
imply strictly-increasing char positions.

* ci: skip windows in fuzz job, scope job permissions

CI showed the Windows fuzz build fails with a real MSVC linker error
(LNK2001: unresolved __start/__stop___sancov_pcs) even with
--sanitizer none: MSVC's linker doesn't synthesize the section
boundary symbols that libFuzzer's coverage instrumentation requires,
so this is unrelated to the earlier ASan/PATH discussion and isn't
fixable by a sanitizer flag. Skip Windows via step-level `if`
(job-level `if` can't reference runner/matrix contexts). Also add an
explicit contents:read permissions block to the job.

* fix(fzy): fix unicode case-folding inconsistency causing overflow panic

The new fuzzy_match fuzz target found a real crash: FzyMatcher panicked
with "attempt to multiply with overflow" on choice="ű\0\0\0\u{1e}ű",
pattern="Űű".

Root cause: fzy_score's case-insensitive comparison used
char::to_ascii_lowercase (a no-op on non-ASCII letters like Ű/ű), while
the shared cheap_matches() prefilter (and the other matchers) use the
Unicode-aware char_equal(). This let cheap_matches accept a pattern
that fzy_score's own DP could then never actually align, since needle
char 'Ű' never matched any haystack position under ASCII-only folding.
The DP's SCORE_MIN sentinel ("impossible") isn't an absorbing element
under plain integer addition, so the broken alignment accumulated to a
value close to, but not exactly, SCORE_MIN, which then overflowed on
the final *SCORE_TO_SKIM conversion since only the exact sentinel was
special-cased.

Fix is_match to use the shared char_equal() so fzy.rs's case folding
matches cheap_matches and the other two matchers (skim.rs, clangd.rs
already do this). Also switch internal_to_skim_score to saturating_mul
as defense in depth, since fzy_score structurally always returns
Some(..) and has no other way to signal "no valid alignment" to the
caller.

* ci: try lld-link to get windows fuzzing working (no ASan)

MSVC ASan is documented broken on GitHub-hosted Windows runners
(actions/runner-images#8891 — ASan binaries crash with
STATUS_DLL_INIT_FAILED even with the runtime DLL on PATH, unresolved
upstream), so it's not viable here regardless of our config. Separately,
the sancov coverage instrumentation cargo-fuzz needs doesn't link with
MSVC's link.exe at all (missing __start/__stop section symbols).
Try switching the Windows leg to rustc's bundled LLD linker
(-C linker-flavor=lld-link -C link-self-contained=+linker) with
--sanitizer none, to at least get coverage-guided fuzzing (no ASan)
working there. Validating live against this PR's CI.

* ci: revert windows fuzzing attempt, exclude it again

The lld-link experiment ruled out the remaining option: LLD's COFF
driver hit the exact same missing __start/__stop___sancov_* symbols as
MSVC's link.exe. This confirms the section-boundary-symbol synthesis
libFuzzer's coverage instrumentation needs simply isn't implemented
for the COFF/Windows target in current LLVM/rustc — an upstream gap,
not a linker choice or CI config problem. Combined with MSVC ASan
being separately documented broken on GH-hosted Windows runners
(actions/runner-images#8891), there's no remaining avenue to try from
the workflow side. Back to excluding Windows from the fuzz job.

* ci: try windows fuzzing with default sanitizer + msvc dev env

Previous Windows attempts both used --sanitizer none, which removes
the ASan runtime that (on Windows) supplies the __start/__stop section
symbol shims libFuzzer's coverage instrumentation needs -- neither
linker synthesizes those on COFF. That's very likely why they failed
to link. Revert to the default sanitizer (address) and add
ilammy/msvc-dev-cmd to put the MSVC ASan DLL directory on PATH, per
the cargo-fuzz Windows setup guide and actions/runner-images#8891.
Testing live whether this builds, and whether the previously-reported
STATUS_DLL_INIT_FAILED runtime crash still reproduces on this runner
image.

* ci: point cargo at the real MSVC linker on windows

msvc-dev-cmd correctly set up Path, but Git Bash prepends its own
usr/bin ahead of it, so cargo picked up Git's coreutils `link`
(hardlink tool) instead of MSVC's link.exe. Set
CARGO_TARGET_X86_64_PC_WINDOWS_MSVC_LINKER explicitly using
VCToolsInstallDir (set by msvc-dev-cmd) to sidestep PATH ordering
entirely.

* fix(event): parse_action returns None instead of panicking on missing args

The keymap_parse fuzz target found a real crash: KeyMap::from("/:if-")
panicked ("no arg specified for event if-") since parse_action's
documented behavior was to panic on if-* actions missing their
argument, even though the function already returns Option<Action> and
every other malformed/unrecognized action already resolves to None
via the surrounding parse_action_chain/KeyMap plumbing.

Fixed that case, and while checking for the same pattern elsewhere in
the function found four more reachable panics of the same kind
(add-char, execute, execute-silent, set-preview-cmd, set-query parsed
without their required argument), confirmed each panics via a small
repro before fixing. All now return None like every other malformed
action, consistent with the function's existing contract, instead of
panicking on user-supplied --bind strings.

* ci: add a single aggregate status check for branch rulesets

Add a ci-success job that depends on every other job in the workflow
and fails if any of them failed or were cancelled (tolerating
deploy-coverage-page's expected skip off master). This gives branch
protection / repository rulesets one stable check name to require,
instead of enumerating every matrix leg (nextest (linux), fuzz
(windows), etc.) individually.

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-07-04 18:01:46 +02:00

6.2 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 --tests (will need tmux under the hood)
  • 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

This application can be tested by :

  • creating a new tmux session in the background (tmux new-session -s <session name> -d). Make sure to clear the SKIM_DEFAULT_OPTIONS env var.
  • creating a new named tmux window in that session : tmux new-window -d -P -F '#I' -n <window name> -t <session name> and configuring the pane naming using tmux set-window-option -t <window name> pane-base-index 0
  • sending the command to run and input using tmux send-keys -t <window name> <keys>
  • when ready, capturing the window using tmux capture-pane -b <window name> -t <window name>.0 and then saving the capture to a file using tmux save-buffer -b <window name> <output file>

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.