utils-shell/lotabout.skim
1
0
Fork 0
mirror of https://github.com/lotabout/skim.git synced 2026-09-10 07:16:23 -04:00
Code Issues Actions Packages Projects Releases Wiki Activity
1698 commits 6 branches 181 tags 63 MiB
Commit graph

3 commits

Author SHA1 Message Date
LoricAndre d7e294eb7d
fix: allow execute actions to run interactive commands (#1132) 
* fix(tui): stop input reader and give execute() children their own tty

Interactive/ncurses programs run via an `execute()` action (e.g. `ncdu`)
would freeze after a few keystrokes. Two independent problems caused it:

1. skim's background input reader (the `EventStream` task started in
   `Tui::start`) kept reading the terminal while the child ran, so skim and
   the child raced for keystrokes on the same tty — roughly half the keys
   were stolen from the child.

2. The child inherited skim's stdin (fd 0), which is a pipe whenever items
   are piped in (`find | sk`). An interactive child then had no keyboard
   source at all.

Fix both:

- Add `Tui::stop_and_join`, which cancels the event-pump task and blocks
  until it has dropped its `EventStream`, guaranteeing skim has released the
  terminal before the child starts. `run_foreground` calls it before running
  the child and `Tui::start` after.

- Give the child its own stdin opened from the controlling terminal
  (`/dev/tty`, or `CONIN$` on Windows), falling back to inheriting skim's
  stdin if that fails.

Because running a foreground process needs the `Tui` (which `handle_action`
does not have), `Execute` now only expands the command and returns a new
`Event::RunExecute`, which `handle_event` runs via `run_foreground`. This
mirrors the existing `RunPreview` pattern. `execute-silent` is unchanged.

Update ARCHITECTURE.md (event dispatch table, terminal lifecycle, and
cross-reference line numbers) and add tests.

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

* test(execute): add tmux e2e test; fix reader restart and post-execute repaint

Add a tmux-based integration test (tests/execute.rs, unix-gated so it runs
on the Linux and macOS CI legs) that drives an interactive child through an
`execute` action and asserts it keeps receiving keystrokes, then that skim
is interactive again once the child exits. It covers both the fullscreen and
inline (`--height`) layouts.

Writing the test surfaced two bugs in the execute reader-suspend work that
unit tests could not catch:

1. Reader never resumed. `Tui::stop_and_join` cancels the shared
   `CancellationToken`, but `Tui::start` reused that same token — and a
   cancelled token stays cancelled — so the respawned reader observed the
   cancellation immediately and exited without reading input. `start` now
   installs a fresh token on every call (also fixing the latent
   restart-while-running path).

2. Post-execute repaint hung when stdout was redirected. The repaint went
   through `Event::Redraw` → `tui.clear()`, and ratatui's `Terminal::clear`
   queries the cursor position, which crossterm writes to stdout via
   `ESC [ 6 n`. skim renders to stderr and its stdout is routinely redirected
   (`sk > file`), so the query reached no terminal, got no reply, and stalled
   the UI for seconds before erroring out. Replace it with
   `Tui::force_full_redraw`, which resets ratatui's diff buffers for a full
   repaint with no cursor query and works for both fullscreen and inline
   viewports.

Update ARCHITECTURE.md and the cross-reference table accordingly.

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

* ci: fix matrix

* fix: pop kitty keyboard flag before entering execute

* chore: remove duplication in backend.rs

* chore: refactor

* fix: kitty maintains a different set of flags in alt screen

---------

Co-authored-by: Claude <noreply@anthropic.com>
 2026-07-21 09:01:53 +00:00
LoricAndre 24a24619ea
ci: publish .deb, .rpm and winget packages on release (#1129) 
* ci: add .deb and .rpm packages to releases via dist

Configure cargo-deb and cargo-generate-rpm to build Linux packages
containing the sk executable, the man pages (sk.1, sk-tmux.1) and the
bash/zsh/fish shell completions.

A new reusable workflow (package.yml) builds both packages and uploads
them under an artifacts-* name. It is wired into the release pipeline as
a dist global-artifacts-job in dist-workspace.toml, and release.yml is
regenerated with `dist generate` (not hand-edited) so dist's host job
attaches the packages to the GitHub Release.

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

* ci: drop sk-tmux man page from .deb and .rpm packages

Package only the sk.1 man page; the sk-tmux.1 page is no longer shipped.

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

* ci: temporarily build release artifacts on PRs

Set dist pr-run-mode = "upload" so the .deb and .rpm (and the other
release artifacts) are built and uploaded on pull requests, allowing the
packages to be downloaded and verified before merging.

This is temporary and should be reverted before merge.

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

* ci: install packaging tools via taiki-e/install-action

Address review feedback on package.yml:
- Install cargo-deb and cargo-generate-rpm with taiki-e/install-action
  (prebuilt binaries) plus a Swatinem/rust-cache step, matching the
  patterns used in test.yml, instead of compiling them with cargo install.
- Drop the `--output target/debian` flag from `cargo deb`; the default
  target/debian/ directory is what the collect step expects.

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

* ci: build arm64 .deb and .rpm packages too

Turn the package job into a matrix that builds natively for both amd64
(ubuntu-22.04) and arm64 (ubuntu-22.04-arm), producing a .deb and .rpm
per architecture. Artifacts are uploaded under per-arch names so dist's
host job attaches all of them to the release.

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

* ci: submit releases to winget via winget-releaser

Add a dist publish job that, after the GitHub Release is created, submits
the new version to the Windows Package Manager Community Repository using
vedantmgoyal9/winget-releaser and the Windows .zip artifact dist already
attaches to the release.

Uses the package identifier and installer regex from skim-rs/skim#769:
  identifier: skim-rs.skim
  installers-regex: '-pc-windows-msvc\.zip$'

Wired in through publish-jobs in dist-workspace.toml; release.yml is
regenerated with `dist generate` (not hand-edited). Prereleases are never
submitted. Requires a WINGET_TOKEN secret (a public_repo-scoped PAT that
owns a microsoft/winget-pkgs fork under skim-rs).

Refs: skim-rs/skim#769

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

* ci: validate package version against the release plan

Consume the `plan` input dist passes to the package job: assert the
crate version equals the version dist planned for this release, so the
source-built .deb/.rpm can't silently drift from the release.

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

* Update .github/workflows/winget.yml

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Update .github/workflows/package.yml

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* docs: document .deb, .rpm, winget and scoop installation

Add the new install methods to the README: winget and Scoop rows plus a
Debian/RPM section with install commands, covering amd64 and arm64.

Also clarify in winget.yml that WINGET_TOKEN must be a classic PAT
(fine-grained tokens can't open the winget-pkgs PR).

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

* fix: install cargo-deb manually to avoid libc version mismatch

* ci: use blacksmith runners for long job

* fix: version spec for cargo install

* chore: revert pr action upload

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
 2026-07-20 22:11:51 +00:00
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