Skip to content

fix: the picker panicked when TERM named no usable terminal - #647

Merged
blooop merged 6 commits into
mainfrom
fix/picker-term-unset
Sep 28, 2026
Merged

blooop merged 6 commits into
mainfrom
fix/picker-term-unset

Conversation

@blooop

@blooop blooop commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

The workspace picker panicked when TERM named no usable terminal. This PR fixes that. The picker is the one that dl stop, dl rm and the other verbs open when you name no workspace. #646 also uses it for aid resume.

skim draws the picker. Before it draws, it looks up the terminfo entry that TERM names, and it calls unwrap() on the result (Skim::run_with in skim 0.20.5). So these real terminals ended the command with a panic before any row was drawn:

  • TERM is unset. Examples are env -i, or a docker exec -t that sets no TERM.
  • TERM names an entry that this machine does not have. An example is a terminal's own name, such as xterm-ghostty, in a container without its terminfo.

A second case did not panic, but it drew a garbled picker. For any name that starts with xterm, tmux, screen, rxvt and a few others, the term crate returns a small built-in entry when the database has none. That entry has colours but no cursor movement (cup), so skim cannot put the rows in place.

What this changes

  • The picker checks the terminal first. select::pick looks up the entry for TERM. It then makes one of three decisions. plan in rust/dl/src/select.rs makes the decision, and it is a pure function:
    • If the entry can move the cursor, nothing changes.
    • If it cannot, and xterm-256color can, the picker sets TERM=xterm-256color while skim runs.
    • If neither can, skim does not start. The new Pick::Undrawable arm ends the command with the reason and the command to type instead: dl <workspace> <verb> for dl, and aid resume <workspace> for aid.
  • Your TERM comes back. When skim returns, the picker puts the old value back, or removes TERM if it was unset. So the session that a pick opens gets your own TERM, not xterm-256color. One line on stderr says what the picker did, for example TERM is unset, so the picker was drawn as xterm-256color.
  • One read of TERM. The note and the restore use the same var_os("TERM") value. So a TERM with bytes that are not UTF-8 is not reported as unset.
  • term = "0.7" is now a direct dependency of dl. skim already uses this version, so Cargo.lock gets one new line and no new crate.

How I checked it

  • New pty tests in rust/dl/tests/picker.rs run dl stop against the fake devpod with TERM unset, TERM=no-such-terminal and TERM=xterm-no-such-entry. Each run picks a row and checks that stop blooop-wayfinder reaches devpod, and that the note is shown. Before the fix, the first two runs panicked and drew nothing. The third drew with the stripped entry and printed no note.
  • The fake devpod now also records the TERM of each call in a second log. The test checks that the stop call ran with your own TERM. I deleted the restore to test the test, and the test then failed with TERM=xterm-256color stop blooop-wayfinder.
  • Other new tests: Esc with TERM unset still prints the note and stops nothing, and a good TERM prints no note. Unit tests cover the three outcomes of plan.
  • The refusal has pty tests too. When TERMINFO_DIRS is set, the term crate searches only that list. So the tests set it to an empty directory, and then even xterm-256color cannot move the cursor. dl stop, dl and aid resume each stop with the line to type, exit 1, and send nothing to devpod but list.
  • The whole gate passed on this branch: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo test --locked --workspace (2702 passed, 0 failed), the Python suite (828 passed) and prek run --from-ref origin/main --to-ref HEAD.

Review notes

  • The refusal hint is a template. It says dl <workspace> rm, not dl <workspace> rm --force, so it does not repeat the flags you typed.
  • An entry with cup but no smcup also has no rmcup. skim can draw with it, but the rows stay on the screen when the picker closes.
  • The set_var calls are unsafe. The comments give the reason that they are safe here: no other thread in dl reads the environment while the picker runs, and skim joins its input thread before it returns.

🤖 Generated with Claude Code

Summary by Sourcery

Make workspace selection robust when the current terminal lacks usable terminfo capabilities.

Bug Fixes:

  • Prevent the workspace picker from panicking or rendering incorrectly when TERM is unset or names an unavailable or unusable terminal.
  • Restore the caller’s original TERM after picker interaction so subsequently opened sessions inherit the user’s terminal setting.
  • Refuse to open the picker with a clear workspace-specific command when neither the requested terminal nor the fallback terminal can render it.

Enhancements:

  • Add terminal capability detection and a fallback to xterm-256color for drawable picker sessions.
  • Add direct term dependency support for terminal capability checks.

Documentation:

  • Document the picker behavior for missing and unusable terminal definitions in the unreleased changelog.

Tests:

  • Add unit and pseudo-terminal coverage for terminal fallback, environment restoration, picker dismissal, and refusal paths across dl and aid resume.

skim's terminal setup looks up the terminfo entry TERM names and unwraps
the result. A real terminal with TERM unset (env -i, a docker exec -t that
sets none), or naming an entry the machine lacks (a terminal's own name in
a container without its terminfo), aborted `dl stop`, `dl rm` and
`aid resume` with a panic before a row was drawn.

The picker now checks the entry first. When it cannot be used, it swaps
TERM for xterm-256color while skim runs, which always resolves through the
term crate's ANSI fallback, and puts the old value back afterwards so the
session a pick opens inherits the user's TERM. One line says what was done
once the picker has given the screen back.

Claude-Session: https://claude.ai/code/session_01DiHLcVgAuJAMdsS4skh48w
…able

term 0.7 answers any xterm*, screen* or tmux* name that has no database
entry with a built-in ANSI entry that holds colours and no cup, and
skim-tuikit writes nothing for a capability an entry lacks. So
TERM=xterm-kitty in a container without that entry drew a garbled
picker, and with no terminfo database at all the xterm-256color
fallback was that same entry while the note said the picker was drawn
as xterm-256color.

The choice is now made on whether the entry has cup. When neither TERM's
entry nor xterm-256color's has it, skim is not started, and dl and
aid resume say why and how to name the workspace instead.

Claude-Session: https://claude.ai/code/session_01DiHLcVgAuJAMdsS4skh48w
TermInfo::from_env reads TERM with env::var, which answers nothing for
bytes that are not UTF-8, so its TermUnset came back for a TERM that
was set, and the note called it unset while the restore put the bytes
back. TERM is now read once, with var_os, into a Was that both the note
and the restore use. from_env only decides whether the entry can draw.

Claude-Session: https://claude.ai/code/session_01DiHLcVgAuJAMdsS4skh48w
Deleting the restore in DrawableTerm's drop left every test passing,
because the fake devpod logged its arguments and nothing of its
environment. It now also logs the TERM each call other than list ran
under, to a second file so the exact matches on the call log are
untouched, and the TERM test asserts the stop ran under the user's own
TERM and nothing ran under xterm-256color. With the restore deleted it
fails.

Also: a quit picker under a swapped TERM still gives the note and stops
nothing, and a TERM that can draw is not swapped.

Claude-Session: https://claude.ai/code/session_01DiHLcVgAuJAMdsS4skh48w
@sourcery-ai

sourcery-ai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

The PR prevents skim from panicking or drawing a garbled picker for missing or unusable TERM values by validating cursor-capable terminfo, temporarily falling back to xterm-256color with RAII restoration, and providing actionable workspace-explicit errors when no drawable terminal is available; unit and PTY tests verify the decision logic and environment restoration.

Sequence diagram for safe picker fallback and TERM restoration

sequenceDiagram
    participant Command
    participant Picker as select::pick
    participant Terminfo
    participant Skim
    participant Session

    Command->>Picker: pick(workspaces, arity, cache_dir)
    Picker->>Terminfo: TermInfo::from_env()
    Terminfo-->>Picker: missing or non-drawable TERM
    Picker->>Terminfo: TermInfo::from_name(xterm-256color)
    Terminfo-->>Picker: drawable fallback
    Picker->>Picker: DrawableTerm::swap()
    Picker->>Skim: run_skim()
    Skim-->>Picker: selected workspace or escape
    Picker->>Picker: DrawableTerm::drop()
    Picker->>Picker: restore original TERM
    Picker-->>Command: Pick result
    Command->>Session: open selected workspace
    Session->>Session: inherit original TERM
Loading

State diagram for picker terminal outcomes

stateDiagram-v2
    [*] --> CheckTERM
    CheckTERM --> RunPicker: current terminfo has cup
    CheckTERM --> CheckFallback: current entry missing or lacks cup
    CheckFallback --> FallbackPicker: xterm-256color has cup
    CheckFallback --> Undrawable: fallback also lacks cup
    FallbackPicker --> RestoreTERM
    RunPicker --> Result
    RestoreTERM --> Result
    Undrawable --> ExplicitCommand
    Result --> [*]
    ExplicitCommand --> [*]
Loading

Flow diagram for terminal validation before opening the picker

flowchart TD
    A[select::pick] --> B[TermInfo::from_env]
    B --> C{drawable current entry?}
    C -->|Yes| D[run_skim]
    C -->|No| E[TermInfo::from_name xterm-256color]
    E --> F{drawable fallback entry?}
    F -->|Yes| G[DrawableTerm::swap]
    G --> D
    F -->|No| H[Pick::Undrawable]
    D --> I[DrawableTerm::drop]
    I --> J[Restore original TERM]
Loading

File-Level Changes

Change Details Files
Add terminal-capability planning before launching skim, with a safe fallback or an actionable refusal instead of allowing skim to panic or render incorrectly.
  • Look up the current and fallback terminfo entries and require cursor-positioning support (cup).
  • Introduce pure plan outcomes for keeping the terminal, temporarily swapping to xterm-256color, or refusing to draw.
  • Handle the new undrawable outcome with workspace-explicit instructions for both dl verbs and aid resume.
rust/dl/src/select.rs
rust/dl/src/commands.rs
Temporarily override and reliably restore TERM around picker rendering while preserving the original environment value and communicating any substitution.
  • Read TERM once with var_os, including non-UTF-8 values, and restore either its original value or its unset state on drop.
  • Use an RAII guard to set the fallback only during skim and emit a post-picker diagnostic.
  • Add the direct term 0.7 dependency shared with skim’s resolved version.
rust/dl/src/select.rs
rust/Cargo.toml
rust/dl/Cargo.toml
rust/Cargo.lock
Expand regression coverage for missing, unusable, and restored terminal environments.
  • Add unit tests covering keep, fallback, refusal, unset, and non-UTF-8 TERM planning.
  • Add PTY tests proving selection works with missing or unsupported entries, quit behavior remains safe, diagnostics are emitted, and downstream devpod calls receive the user’s original TERM.
rust/dl/src/select.rs
rust/dl/tests/picker.rs
Document the picker’s behavior when the configured terminal cannot draw it.
  • Add the fix and fallback/refusal behavior to the unreleased changelog.
CHANGELOG.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.33333% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.78%. Comparing base (35fa764) to head (ba0fc4e).

Files with missing lines Patch % Lines
rust/dl/src/select.rs 80.00% 22 Missing ⚠️
rust/dl/src/commands.rs 0.00% 10 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
python 42.98% <ø> (ø)
rust 95.01% <73.33%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
shipped code (rust) 95.01% <73.33%> (-0.04%) ⬇️
harness and tooling (python) 42.98% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The Pick::Undrawable arms in render_select and pick_one had no test.
They are reachable: with TERMINFO_DIRS set, the term crate searches only
that list, so an empty directory leaves even the xterm-256color fallback
with the built-in ANSI entry, which has no cup.

picker.rs runs `dl stop` and `dl` under that environment and checks the
refusal line each verb gets, exit 1, no alternate screen, and no devpod
call past the listing. interactive.rs does the same for `aid resume`.
Both wait for the exit against a deadline, so a picker that opens anyway
fails the test in place of hanging it.

Claude-Session: https://claude.ai/code/session_01DiHLcVgAuJAMdsS4skh48w
The draw works without smcup, but the entry then lacks rmcup as well,
so skim's pause writes nothing to restore the screen and the picker's
rows stay on it. The comment now says so.

Claude-Session: https://claude.ai/code/session_01DiHLcVgAuJAMdsS4skh48w
@blooop
blooop merged commit 6d3e268 into main Sep 28, 2026
15 checks passed
@blooop
blooop deleted the fix/picker-term-unset branch September 28, 2026 21:28
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